linux-kernel - [PATCH] driver core: platform: Fix wrong comment

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 23 Dec 2021 10:53:46 +0800
From:   Jiasheng Jiang <jiasheng@...as.ac.cn>
To:     gregkh@...uxfoundation.org, rafael@...nel.org
Cc:     linux-kernel@...r.kernel.org, Jiasheng Jiang <jiasheng@...as.ac.cn>
Subject: [PATCH] driver core: platform: Fix wrong comment

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/dd6e6054-3d7e-b43a-0386-71323c49ab27@opensource.wdc.com/
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 <jiasheng@...as.ac.cn>
>> ---
>>  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 <jiasheng@...as.ac.cn>
---
 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