[PATCH] ASoC: Intel: atom: Remove redundant check to simplify the code

[PATCH] ASoC: Intel: atom: Remove redundant check to simplify the code

[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

Re: [PATCH] ASoC: Intel: atom: Remove redundant check to simplify the code

[Pierre-Louis Bossart]

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;
>  }
> 

Re: [PATCH] ASoC: Intel: atom: Remove redundant check to simplify the code

[Mark Brown]

[-- 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 --]

Re: [PATCH] ASoC: Intel: atom: Remove redundant check to simplify the code

[Andy Shevchenko]

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

Re: [PATCH] ASoC: Intel: atom: Remove redundant check to simplify the code

[Andy Shevchenko]

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

Re: [PATCH] ASoC: Intel: atom: Remove redundant check to simplify the code

[Mark Brown]

[-- 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 --]

Re: [PATCH] ASoC: Intel: atom: Remove redundant check to simplify the code

[Andy Shevchenko]

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

Re: [PATCH] ASoC: Intel: atom: Remove redundant check to simplify the code

[Andy Shevchenko]

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