[PATCH] ASoC: Intel: atom: Remove redundant check to simplify the code
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; }
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;
}
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.
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.
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....
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().
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.r...
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
participants (5)
-
Andy Shevchenko -
Andy Shevchenko -
Mark Brown -
Pierre-Louis Bossart -
Tang Bin