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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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 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
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/[email protected]/T/#u
--
With Best Regards,
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:
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 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 Fri, Nov 26, 2021 at 2:37 PM Tang Bin <[email protected]> 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 <[email protected]>
Code is correct, I haven't checked the rest, though.
> Signed-off-by: Tang Bin <[email protected]>
> ---
> 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
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/[email protected]/T/#u
--
With Best Regards,
Andy Shevchenko