LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH] ASoC: Intel: atom: Remove redundant check to simplify the code
@ 2021-11-25  7:50 Tang Bin
  2021-11-29 16:22 ` Pierre-Louis Bossart
  2021-11-30  9:46 ` Andy Shevchenko
  0 siblings, 2 replies; 8+ messages in thread
From: Tang Bin @ 2021-11-25  7:50 UTC (permalink / raw)
  To: broonie, cezary.rojewski, pierre-louis.bossart, liam.r.girdwood,
	yang.jie, perex, tiwai
  Cc: alsa-devel, linux-kernel, Tang Bin

In the function sst_platform_get_resources(), if platform_get_irq()
failed, the return should not be zero, as the example in
platform.c is
  * int irq = platform_get_irq(pdev, 0)
  * if (irq < 0)
  * return irq;
So remove the redundant check to simplify the code.

Signed-off-by: Tang Bin <tangbin@cmss.chinamobile.com>
---
 sound/soc/intel/atom/sst/sst_acpi.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/sound/soc/intel/atom/sst/sst_acpi.c b/sound/soc/intel/atom/sst/sst_acpi.c
index 3be64430c..696d547c5 100644
--- a/sound/soc/intel/atom/sst/sst_acpi.c
+++ b/sound/soc/intel/atom/sst/sst_acpi.c
@@ -226,8 +226,8 @@ static int sst_platform_get_resources(struct intel_sst_drv *ctx)
 	/* Find the IRQ */
 	ctx->irq_num = platform_get_irq(pdev,
 				ctx->pdata->res_info->acpi_ipc_irq_index);
-	if (ctx->irq_num <= 0)
-		return ctx->irq_num < 0 ? ctx->irq_num : -EIO;
+	if (ctx->irq_num < 0)
+		return ctx->irq_num;
 
 	return 0;
 }
-- 
2.20.1.windows.1




^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] ASoC: Intel: atom: Remove redundant check to simplify the code
  2021-11-25  7:50 [PATCH] ASoC: Intel: atom: Remove redundant check to simplify the code Tang Bin
@ 2021-11-29 16:22 ` Pierre-Louis Bossart
  2021-11-29 17:11   ` Mark Brown
  2022-01-25 18:09   ` Andy Shevchenko
  2021-11-30  9:46 ` Andy Shevchenko
  1 sibling, 2 replies; 8+ messages in thread
From: Pierre-Louis Bossart @ 2021-11-29 16:22 UTC (permalink / raw)
  To: Tang Bin, broonie, cezary.rojewski, liam.r.girdwood, yang.jie,
	perex, tiwai, Andy Shevchenko
  Cc: alsa-devel, linux-kernel



On 11/25/21 1:50 AM, Tang Bin wrote:
> In the function sst_platform_get_resources(), if platform_get_irq()
> failed, the return should not be zero, as the example in
> platform.c is
>   * int irq = platform_get_irq(pdev, 0)
>   * if (irq < 0)
>   * return irq;
> So remove the redundant check to simplify the code.

Humm, it's a bit of a gray area.

the comments for platform_get_irq and platform_get_irq_optional say:

* Return: non-zero IRQ number on success, negative error number on failure.

but if you look at platform_get_irq_optional, there are two references
to zero being a possible return value:

	if (num == 0 && has_acpi_companion(&dev->dev)) {
		ret = acpi_dev_gpio_irq_get(ACPI_COMPANION(&dev->dev), num);
		/* Our callers expect -ENXIO for missing IRQs. */
		if (ret >= 0 || ret == -EPROBE_DEFER)
			goto out;

out_not_found:
	ret = -ENXIO;
out:
	WARN(ret == 0, "0 is an invalid IRQ number\n");
	return ret;

https://elixir.bootlin.com/linux/latest/source/drivers/base/platform.c#L234

I am not sure if there's any merit in removing the test for the zero
return value. It may be on the paranoid side but it's aligned with a
possible code path in the platform code.

Or it could be that the platform code is wrong, and the label used
should have been

/* Our callers expect -ENXIO for missing IRQs. */
if (ret >= 0 || ret == -EPROBE_DEFER)
	goto out_not_found;

Adding Andy Shevchenko for advice.

> 
> Signed-off-by: Tang Bin <tangbin@cmss.chinamobile.com>
> ---
>  sound/soc/intel/atom/sst/sst_acpi.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/sound/soc/intel/atom/sst/sst_acpi.c b/sound/soc/intel/atom/sst/sst_acpi.c
> index 3be64430c..696d547c5 100644
> --- a/sound/soc/intel/atom/sst/sst_acpi.c
> +++ b/sound/soc/intel/atom/sst/sst_acpi.c
> @@ -226,8 +226,8 @@ static int sst_platform_get_resources(struct intel_sst_drv *ctx)
>  	/* Find the IRQ */
>  	ctx->irq_num = platform_get_irq(pdev,
>  				ctx->pdata->res_info->acpi_ipc_irq_index);
> -	if (ctx->irq_num <= 0)
> -		return ctx->irq_num < 0 ? ctx->irq_num : -EIO;
> +	if (ctx->irq_num < 0)
> +		return ctx->irq_num;
>  
>  	return 0;
>  }
> 

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] ASoC: Intel: atom: Remove redundant check to simplify the code
  2021-11-29 16:22 ` Pierre-Louis Bossart
@ 2021-11-29 17:11   ` Mark Brown
  2021-11-29 19:01     ` Andy Shevchenko
  2022-01-25 18:09   ` Andy Shevchenko
  1 sibling, 1 reply; 8+ messages in thread
From: Mark Brown @ 2021-11-29 17:11 UTC (permalink / raw)
  To: Pierre-Louis Bossart
  Cc: Tang Bin, cezary.rojewski, liam.r.girdwood, yang.jie, perex,
	tiwai, Andy Shevchenko, alsa-devel, linux-kernel

[-- Attachment #1: Type: text/plain, Size: 936 bytes --]

On Mon, Nov 29, 2021 at 10:22:41AM -0600, Pierre-Louis Bossart wrote:
> On 11/25/21 1:50 AM, Tang Bin wrote:

> > In the function sst_platform_get_resources(), if platform_get_irq()
> > failed, the return should not be zero, as the example in
> > platform.c is
> >   * int irq = platform_get_irq(pdev, 0)
> >   * if (irq < 0)
> >   * return irq;
> > So remove the redundant check to simplify the code.

> Humm, it's a bit of a gray area.

> the comments for platform_get_irq and platform_get_irq_optional say:

> * Return: non-zero IRQ number on success, negative error number on failure.

> but if you look at platform_get_irq_optional, there are two references
> to zero being a possible return value:

Zero is (or was, people were working on changing it partly due to
confusion and partly due to moving to newer infrastructure which
doesn't use it) a valid IRQ on some architectures.  x86 wasn't one of
those though, at least AFAIR.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] ASoC: Intel: atom: Remove redundant check to simplify the code
  2021-11-29 17:11   ` Mark Brown
@ 2021-11-29 19:01     ` Andy Shevchenko
  2021-11-29 19:05       ` Andy Shevchenko
  2021-11-29 19:08       ` Mark Brown
  0 siblings, 2 replies; 8+ messages in thread
From: Andy Shevchenko @ 2021-11-29 19:01 UTC (permalink / raw)
  To: Mark Brown
  Cc: Pierre-Louis Bossart, Tang Bin, cezary.rojewski, liam.r.girdwood,
	yang.jie, perex, tiwai, alsa-devel, linux-kernel

On Mon, Nov 29, 2021 at 05:11:52PM +0000, Mark Brown wrote:
> On Mon, Nov 29, 2021 at 10:22:41AM -0600, Pierre-Louis Bossart wrote:
> > On 11/25/21 1:50 AM, Tang Bin wrote:
> 
> > > In the function sst_platform_get_resources(), if platform_get_irq()
> > > failed, the return should not be zero, as the example in
> > > platform.c is
> > >   * int irq = platform_get_irq(pdev, 0)
> > >   * if (irq < 0)
> > >   * return irq;
> > > So remove the redundant check to simplify the code.
> 
> > Humm, it's a bit of a gray area.
> 
> > the comments for platform_get_irq and platform_get_irq_optional say:
> 
> > * Return: non-zero IRQ number on success, negative error number on failure.
> 
> > but if you look at platform_get_irq_optional, there are two references
> > to zero being a possible return value:
> 
> Zero is (or was, people were working on changing it partly due to
> confusion and partly due to moving to newer infrastructure which
> doesn't use it) a valid IRQ on some architectures.  x86 wasn't one of
> those though, at least AFAIR.

I guess it's about x86, but the API returns Linux virtual IRQ and 0 shouldn't
be among them (hardware IRQ != Linux virtual IRQ). Legacy x86 used 1:1 mapping
for ISA IRQs (lower 16) among which the Timer IRQ is 0. I believe that timer
code does not use any of those APIs (it most likely and IIRC has it hardcoded).

Nevertheless, I have planned to make platform_irq_get_optional() to be optional
indeed, where we return 0 when there is no IRQ provided and error when it's a
real error happens. This needs to clean up the current (mis-)use of the API.

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] ASoC: Intel: atom: Remove redundant check to simplify the code
  2021-11-29 19:01     ` Andy Shevchenko
@ 2021-11-29 19:05       ` Andy Shevchenko
  2021-11-29 19:08       ` Mark Brown
  1 sibling, 0 replies; 8+ messages in thread
From: Andy Shevchenko @ 2021-11-29 19:05 UTC (permalink / raw)
  To: Mark Brown
  Cc: Pierre-Louis Bossart, Tang Bin, cezary.rojewski, liam.r.girdwood,
	yang.jie, perex, tiwai, alsa-devel, linux-kernel

On Mon, Nov 29, 2021 at 09:01:16PM +0200, Andy Shevchenko wrote:
> On Mon, Nov 29, 2021 at 05:11:52PM +0000, Mark Brown wrote:
> > On Mon, Nov 29, 2021 at 10:22:41AM -0600, Pierre-Louis Bossart wrote:
> > > On 11/25/21 1:50 AM, Tang Bin wrote:
> > 
> > > > In the function sst_platform_get_resources(), if platform_get_irq()
> > > > failed, the return should not be zero, as the example in
> > > > platform.c is
> > > >   * int irq = platform_get_irq(pdev, 0)
> > > >   * if (irq < 0)
> > > >   * return irq;
> > > > So remove the redundant check to simplify the code.
> > 
> > > Humm, it's a bit of a gray area.
> > 
> > > the comments for platform_get_irq and platform_get_irq_optional say:
> > 
> > > * Return: non-zero IRQ number on success, negative error number on failure.
> > 
> > > but if you look at platform_get_irq_optional, there are two references
> > > to zero being a possible return value:
> > 
> > Zero is (or was, people were working on changing it partly due to
> > confusion and partly due to moving to newer infrastructure which
> > doesn't use it) a valid IRQ on some architectures.  x86 wasn't one of
> > those though, at least AFAIR.
> 
> I guess it's about x86, but the API returns Linux virtual IRQ and 0 shouldn't
> be among them (hardware IRQ != Linux virtual IRQ). Legacy x86 used 1:1 mapping
> for ISA IRQs (lower 16) among which the Timer IRQ is 0. I believe that timer
> code does not use any of those APIs (it most likely and IIRC has it hardcoded).
> 
> Nevertheless, I have planned to make platform_irq_get_optional() to be optional
> indeed, where we return 0 when there is no IRQ provided and error when it's a
> real error happens. This needs to clean up the current (mis-)use of the API.

Link for previous work: https://lore.kernel.org/lkml/20210331144526.19439-1-andriy.shevchenko@linux.intel.com/T/#u

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] ASoC: Intel: atom: Remove redundant check to simplify the code
  2021-11-29 19:01     ` Andy Shevchenko
  2021-11-29 19:05       ` Andy Shevchenko
@ 2021-11-29 19:08       ` Mark Brown
  1 sibling, 0 replies; 8+ messages in thread
From: Mark Brown @ 2021-11-29 19:08 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Pierre-Louis Bossart, Tang Bin, cezary.rojewski, liam.r.girdwood,
	yang.jie, perex, tiwai, alsa-devel, linux-kernel

[-- Attachment #1: Type: text/plain, Size: 976 bytes --]

On Mon, Nov 29, 2021 at 09:01:16PM +0200, Andy Shevchenko wrote:
> On Mon, Nov 29, 2021 at 05:11:52PM +0000, Mark Brown wrote:

> > Zero is (or was, people were working on changing it partly due to
> > confusion and partly due to moving to newer infrastructure which
> > doesn't use it) a valid IRQ on some architectures.  x86 wasn't one of
> > those though, at least AFAIR.

> I guess it's about x86, but the API returns Linux virtual IRQ and 0 shouldn't
> be among them (hardware IRQ != Linux virtual IRQ). Legacy x86 used 1:1 mapping
> for ISA IRQs (lower 16) among which the Timer IRQ is 0. I believe that timer
> code does not use any of those APIs (it most likely and IIRC has it hardcoded).

Right, the virtual IRQs are the newer stuff.  32 bit arm was another
platform that had 0 as a valid IRQ for similar reasons, I don't know if
any of the platforms are still affected though and I'm going to go out
on a limb and say they're not using platform_irq_get_optional().

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] ASoC: Intel: atom: Remove redundant check to simplify the code
  2021-11-25  7:50 [PATCH] ASoC: Intel: atom: Remove redundant check to simplify the code Tang Bin
  2021-11-29 16:22 ` Pierre-Louis Bossart
@ 2021-11-30  9:46 ` Andy Shevchenko
  1 sibling, 0 replies; 8+ messages in thread
From: Andy Shevchenko @ 2021-11-30  9:46 UTC (permalink / raw)
  To: Tang Bin
  Cc: Mark Brown, Cezary Rojewski, Pierre-Louis Bossart, Liam Girdwood,
	Jie Yang, Jaroslav Kysela, Takashi Iwai,
	ALSA Development Mailing List, Linux Kernel Mailing List

On Fri, Nov 26, 2021 at 2:37 PM Tang Bin <tangbin@cmss.chinamobile.com> wrote:
>
> In the function sst_platform_get_resources(), if platform_get_irq()
> failed, the return should not be zero, as the example in
> platform.c is
>   * int irq = platform_get_irq(pdev, 0)
>   * if (irq < 0)
>   * return irq;
> So remove the redundant check to simplify the code.

FWIW,
Acked-by: Andy Shevchenko <andy.shevchenko@gmail.com>

Code is correct, I haven't checked the rest, though.

> Signed-off-by: Tang Bin <tangbin@cmss.chinamobile.com>
> ---
>  sound/soc/intel/atom/sst/sst_acpi.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/sound/soc/intel/atom/sst/sst_acpi.c b/sound/soc/intel/atom/sst/sst_acpi.c
> index 3be64430c..696d547c5 100644
> --- a/sound/soc/intel/atom/sst/sst_acpi.c
> +++ b/sound/soc/intel/atom/sst/sst_acpi.c
> @@ -226,8 +226,8 @@ static int sst_platform_get_resources(struct intel_sst_drv *ctx)
>         /* Find the IRQ */
>         ctx->irq_num = platform_get_irq(pdev,
>                                 ctx->pdata->res_info->acpi_ipc_irq_index);
> -       if (ctx->irq_num <= 0)
> -               return ctx->irq_num < 0 ? ctx->irq_num : -EIO;
> +       if (ctx->irq_num < 0)
> +               return ctx->irq_num;
>
>         return 0;
>  }
> --
> 2.20.1.windows.1
>
>
>


-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] ASoC: Intel: atom: Remove redundant check to simplify the code
  2021-11-29 16:22 ` Pierre-Louis Bossart
  2021-11-29 17:11   ` Mark Brown
@ 2022-01-25 18:09   ` Andy Shevchenko
  1 sibling, 0 replies; 8+ messages in thread
From: Andy Shevchenko @ 2022-01-25 18:09 UTC (permalink / raw)
  To: Pierre-Louis Bossart
  Cc: Tang Bin, broonie, cezary.rojewski, liam.r.girdwood, yang.jie,
	perex, tiwai, alsa-devel, linux-kernel

On Mon, Nov 29, 2021 at 10:22:41AM -0600, Pierre-Louis Bossart wrote:
> On 11/25/21 1:50 AM, Tang Bin wrote:

> > In the function sst_platform_get_resources(), if platform_get_irq()
> > failed, the return should not be zero, as the example in
> > platform.c is
> >   * int irq = platform_get_irq(pdev, 0)
> >   * if (irq < 0)
> >   * return irq;
> > So remove the redundant check to simplify the code.
> 
> Humm, it's a bit of a gray area.
> 
> the comments for platform_get_irq and platform_get_irq_optional say:
> 
> * Return: non-zero IRQ number on success, negative error number on failure.
> 
> but if you look at platform_get_irq_optional, there are two references
> to zero being a possible return value:
> 
> 	if (num == 0 && has_acpi_companion(&dev->dev)) {
> 		ret = acpi_dev_gpio_irq_get(ACPI_COMPANION(&dev->dev), num);
> 		/* Our callers expect -ENXIO for missing IRQs. */
> 		if (ret >= 0 || ret == -EPROBE_DEFER)

This is bogus == 0 check.

> 			goto out;
> 
> out_not_found:
> 	ret = -ENXIO;
> out:
> 	WARN(ret == 0, "0 is an invalid IRQ number\n");
> 	return ret;
> 
> https://elixir.bootlin.com/linux/latest/source/drivers/base/platform.c#L234
> 
> I am not sure if there's any merit in removing the test for the zero
> return value. It may be on the paranoid side but it's aligned with a
> possible code path in the platform code.
> 
> Or it could be that the platform code is wrong, and the label used
> should have been
> 
> /* Our callers expect -ENXIO for missing IRQs. */
> if (ret >= 0 || ret == -EPROBE_DEFER)
> 	goto out_not_found;

In case one wants to dive into new discussion on the topic:

https://lore.kernel.org/linux-serial/20220110195449.12448-2-s.shtylyov@omp.ru/T/#u

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2022-01-25 18:11 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-11-25  7:50 [PATCH] ASoC: Intel: atom: Remove redundant check to simplify the code Tang Bin
2021-11-29 16:22 ` Pierre-Louis Bossart
2021-11-29 17:11   ` Mark Brown
2021-11-29 19:01     ` Andy Shevchenko
2021-11-29 19:05       ` Andy Shevchenko
2021-11-29 19:08       ` Mark Brown
2022-01-25 18:09   ` Andy Shevchenko
2021-11-30  9:46 ` Andy Shevchenko

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).