I notice that there is a 'WARN(ret == 0, "0 is an invalid IRQ
number\n");' before 'return ret;', which means that it is possible to
return 0 if fails.
Therefore, it might be better to correct the wrong comment.
And also there is reply sent by Damien Le Moal because I submitted a
patch to remove the non-zero check of the platform_get_irq() previously.
Damien Le Moal said that the comment for platform_get_irq() is wrond
because it can actually return 0.
Moreover, platform_get_irq() returns platform_get_irq_optional().
As a conclusion, the comments of the platform_get_irq() and
platform_get_irq_optional() should be fixed.
Not only that, the comments of platform_get_irq_byname() and
platform_get_irq_byname_optional() have the same error.
This time I only submit one as an example.
If the patch is right, I will submit another version to fix all.
But, I also notice that the 'return 0' is removed intentionally in the
fixed tag.
I am not sure which one is right.
Anyway, the success IRQ number should be 'postive' other than
'non-zero'.
So the comment should be corrected.
Here is the mail from Damien Le Moal.
Link:
https://lore.kernel.org/lkml/[email protected]/
On Thu, Dec 23, 2021 at 08:41:54AM +0800, Damien Le Moal wrote:
>> It can be found that platform_get_irq() returns nagative code but not
>> null when fails.
>> The comment of the platform_get_irq clearly shows that.
>> Therefore it should be better to remove the useless check.
>
> Nope, platform_get_irq() can actually return 0 (the comment for that
> function is wrong). So testing for !irq is valid and must be kept.
>
>>
>> Fixes: fd990556f0fa ("ata: move library code from ahci_platform.c to
libahci_platform.c")
>> Signed-off-by: Jiasheng Jiang <[email protected]>
>> ---
>> drivers/ata/libahci_platform.c | 2 --
>> 1 file changed, 2 deletions(-)
>>
>> diff --git a/drivers/ata/libahci_platform.c
b/drivers/ata/libahci_platform.c
>> index b2f552088291..5ec68f138c28 100644
>> --- a/drivers/ata/libahci_platform.c
>> +++ b/drivers/ata/libahci_platform.c
>> @@ -587,8 +587,6 @@ int ahci_platform_init_host(struct
platform_device *pdev,
>> dev_err(dev, "no irq\n");
>> return irq;
>> }
>> - if (!irq)
>> - return -EINVAL;
>>
>> hpriv->irq = irq;
>>
Fixes: c2f3f755f5c7 ("Revert "driver core: platform: Make platform_get_irq_optional() optional"")
Signed-off-by: Jiasheng Jiang <[email protected]>
---
drivers/base/platform.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index 9cd34def2237..fcba2559cc90 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -181,10 +181,10 @@ EXPORT_SYMBOL_GPL(devm_platform_ioremap_resource_byname);
* For example::
*
* int irq = platform_get_irq_optional(pdev, 0);
- * if (irq < 0)
+ * if (irq <= 0)
* return irq;
*
- * Return: non-zero IRQ number on success, negative error number on failure.
+ * Return: positive IRQ number on success, negative error number and zero on failure.
*/
int platform_get_irq_optional(struct platform_device *dev, unsigned int num)
{
--
2.25.1
On Sat, Dec 25, 2021 at 2:02 AM Jiasheng Jiang <[email protected]> wrote:
>
> I notice that there is a 'WARN(ret == 0, "0 is an invalid IRQ
> number\n");' before 'return ret;', which means that it is possible to
> return 0 if fails.
> Therefore, it might be better to correct the wrong comment.
> And also there is reply sent by Damien Le Moal because I submitted a
> patch to remove the non-zero check of the platform_get_irq() previously.
> Damien Le Moal said that the comment for platform_get_irq() is wrond
> because it can actually return 0.
> Moreover, platform_get_irq() returns platform_get_irq_optional().
> As a conclusion, the comments of the platform_get_irq() and
> platform_get_irq_optional() should be fixed.
> Not only that, the comments of platform_get_irq_byname() and
> platform_get_irq_byname_optional() have the same error.
> This time I only submit one as an example.
> If the patch is right, I will submit another version to fix all.
> But, I also notice that the 'return 0' is removed intentionally in the
> fixed tag.
> I am not sure which one is right.
> Anyway, the success IRQ number should be 'postive' other than
> 'non-zero'.
> So the comment should be corrected.
> Here is the mail from Damien Le Moal.
...
> Fixes: c2f3f755f5c7 ("Revert "driver core: platform: Make platform_get_irq_optional() optional"")
How can it be a Revert?
--
With Best Regards,
Andy Shevchenko
On 12/25/21 08:47, Andy Shevchenko wrote:
> On Sat, Dec 25, 2021 at 2:02 AM Jiasheng Jiang <[email protected]> wrote:
>>
>> I notice that there is a 'WARN(ret == 0, "0 is an invalid IRQ
>> number\n");' before 'return ret;', which means that it is possible to
>> return 0 if fails.
>> Therefore, it might be better to correct the wrong comment.
>> And also there is reply sent by Damien Le Moal because I submitted a
>> patch to remove the non-zero check of the platform_get_irq() previously.
>> Damien Le Moal said that the comment for platform_get_irq() is wrond
>> because it can actually return 0.
>> Moreover, platform_get_irq() returns platform_get_irq_optional().
>> As a conclusion, the comments of the platform_get_irq() and
>> platform_get_irq_optional() should be fixed.
>> Not only that, the comments of platform_get_irq_byname() and
>> platform_get_irq_byname_optional() have the same error.
>> This time I only submit one as an example.
>> If the patch is right, I will submit another version to fix all.
>> But, I also notice that the 'return 0' is removed intentionally in the
>> fixed tag.
>> I am not sure which one is right.
>> Anyway, the success IRQ number should be 'postive' other than
>> 'non-zero'.
>> So the comment should be corrected.
>> Here is the mail from Damien Le Moal.
>
> ...
>
>> Fixes: c2f3f755f5c7 ("Revert "driver core: platform: Make platform_get_irq_optional() optional"")
>
> How can it be a Revert?
That's the "title" of that commit:
commit c2f3f755f5c7
Author: Greg Kroah-Hartman <[email protected]>
Date: Wed Apr 7 11:47:56 2021 +0200
Revert "driver core: platform: Make platform_get_irq_optional() optional"
--
~Randy
On Sat, Dec 25, 2021 at 6:50 PM Randy Dunlap <[email protected]> wrote:
>
>
>
> On 12/25/21 08:47, Andy Shevchenko wrote:
> > On Sat, Dec 25, 2021 at 2:02 AM Jiasheng Jiang <[email protected]> wrote:
> >>
> >> I notice that there is a 'WARN(ret == 0, "0 is an invalid IRQ
> >> number\n");' before 'return ret;', which means that it is possible to
> >> return 0 if fails.
> >> Therefore, it might be better to correct the wrong comment.
> >> And also there is reply sent by Damien Le Moal because I submitted a
> >> patch to remove the non-zero check of the platform_get_irq() previously.
> >> Damien Le Moal said that the comment for platform_get_irq() is wrond
> >> because it can actually return 0.
> >> Moreover, platform_get_irq() returns platform_get_irq_optional().
> >> As a conclusion, the comments of the platform_get_irq() and
> >> platform_get_irq_optional() should be fixed.
> >> Not only that, the comments of platform_get_irq_byname() and
> >> platform_get_irq_byname_optional() have the same error.
> >> This time I only submit one as an example.
> >> If the patch is right, I will submit another version to fix all.
> >> But, I also notice that the 'return 0' is removed intentionally in the
> >> fixed tag.
> >> I am not sure which one is right.
> >> Anyway, the success IRQ number should be 'postive' other than
> >> 'non-zero'.
> >> So the comment should be corrected.
> >> Here is the mail from Damien Le Moal.
> >
> > ...
> >
> >> Fixes: c2f3f755f5c7 ("Revert "driver core: platform: Make platform_get_irq_optional() optional"")
> >
> > How can it be a Revert?
>
> That's the "title" of that commit:
>
> commit c2f3f755f5c7
> Author: Greg Kroah-Hartman <[email protected]>
> Date: Wed Apr 7 11:47:56 2021 +0200
>
> Revert "driver core: platform: Make platform_get_irq_optional() optional"
My point is that the proposed change should fix a revert, how come?
--
With Best Regards,
Andy Shevchenko