[PATCH -next 0/2] fix two bugs when trying rmmod sata_fsl

[PATCH -next 0/2] fix two bugs when trying rmmod sata_fsl

[Baokun Li]

Baokun Li (2):
  sata_fsl: fix UAF in sata_fsl_port_stop when rmmod sata_fsl
  sata_fsl: fix warning in remove_proc_entry when rmmod sata_fsl

 drivers/ata/sata_fsl.c | 21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)

-- 
2.31.1

[PATCH -next 1/2] sata_fsl: fix UAF in sata_fsl_port_stop when rmmod sata_fsl

[Baokun Li]

When the `rmmod sata_fsl.ko` command is executed in the PPC64 GNU/Linux,
a bug is reported:
 ==================================================================
 BUG: Unable to handle kernel data access on read at 0x80000800805b502c
 Oops: Kernel access of bad area, sig: 11 [#1]
 NIP [c0000000000388a4] .ioread32+0x4/0x20
 LR [80000000000c6034] .sata_fsl_port_stop+0x44/0xe0 [sata_fsl]
 Call Trace:
  .free_irq+0x1c/0x4e0 (unreliable)
  .ata_host_stop+0x74/0xd0 [libata]
  .release_nodes+0x330/0x3f0
  .device_release_driver_internal+0x178/0x2c0
  .driver_detach+0x64/0xd0
  .bus_remove_driver+0x70/0xf0
  .driver_unregister+0x38/0x80
  .platform_driver_unregister+0x14/0x30
  .fsl_sata_driver_exit+0x18/0xa20 [sata_fsl]
  .__se_sys_delete_module+0x1ec/0x2d0
  .system_call_exception+0xfc/0x1f0
  system_call_common+0xf8/0x200
 ==================================================================

The triggering of the BUG is shown in the following stack:

driver_detach
  device_release_driver_internal
    __device_release_driver
      drv->remove(dev) --> platform_drv_remove/platform_remove
        drv->remove(dev) --> sata_fsl_remove
          iounmap(host_priv->hcr_base);			<---- unmap
          kfree(host_priv);                             <---- free
      devres_release_all
        release_nodes
          dr->node.release(dev, dr->data) --> ata_host_stop
            ap->ops->port_stop(ap) --> sata_fsl_port_stop
                ioread32(hcr_base + HCONTROL)           <---- UAF
            host->ops->host_stop(host)

The iounmap(host_priv->hcr_base) and kfree(host_priv) commands should
not be executed in drv->remove. These commands should be executed in
host_stop after port_stop. Therefore, we move these commands to the
new function sata_fsl_host_stop and bind the new function to host_stop
by referring to achi.

Reported-by: Hulk Robot <hulkci@huawei.com>
Signed-off-by: Baokun Li <libaokun1@huawei.com>
---
 drivers/ata/sata_fsl.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/drivers/ata/sata_fsl.c b/drivers/ata/sata_fsl.c
index e5838b23c9e0..30759fd1c3a2 100644
--- a/drivers/ata/sata_fsl.c
+++ b/drivers/ata/sata_fsl.c
@@ -1430,12 +1430,25 @@ static struct ata_port_operations sata_fsl_ops = {
 	.pmp_detach = sata_fsl_pmp_detach,
 };
 
+static void sata_fsl_host_stop(struct ata_host *host)
+{
+	struct sata_fsl_host_priv *host_priv = host->private_data;
+
+	iounmap(host_priv->hcr_base);
+	kfree(host_priv);
+}
+
+static struct ata_port_operations sata_fsl_platform_ops = {
+	.inherits       = &sata_fsl_ops,
+	.host_stop      = sata_fsl_host_stop,
+};
+
 static const struct ata_port_info sata_fsl_port_info[] = {
 	{
 	 .flags = SATA_FSL_HOST_FLAGS,
 	 .pio_mask = ATA_PIO4,
 	 .udma_mask = ATA_UDMA6,
-	 .port_ops = &sata_fsl_ops,
+	 .port_ops = &sata_fsl_platform_ops,
 	 },
 };
 
@@ -1558,8 +1571,6 @@ static int sata_fsl_remove(struct platform_device *ofdev)
 	ata_host_detach(host);
 
 	irq_dispose_mapping(host_priv->irq);
-	iounmap(host_priv->hcr_base);
-	kfree(host_priv);
 
 	return 0;
 }
-- 
2.31.1

[PATCH -next 2/2] sata_fsl: fix warning in remove_proc_entry when rmmod sata_fsl

[Baokun Li]

Trying to remove the fsl-sata module in the PPC64 GNU/Linux
leads to the following warning:
 ------------[ cut here ]------------
 remove_proc_entry: removing non-empty directory 'irq/69',
   leaking at least 'fsl-sata[ff0221000.sata]'
 WARNING: CPU: 3 PID: 1048 at fs/proc/generic.c:722
   .remove_proc_entry+0x20c/0x220
 IRQMASK: 0
 NIP [c00000000033826c] .remove_proc_entry+0x20c/0x220
 LR [c000000000338268] .remove_proc_entry+0x208/0x220
 Call Trace:
  .remove_proc_entry+0x208/0x220 (unreliable)
  .unregister_irq_proc+0x104/0x140
  .free_desc+0x44/0xb0
  .irq_free_descs+0x9c/0xf0
  .irq_dispose_mapping+0x64/0xa0
  .sata_fsl_remove+0x58/0xa0 [sata_fsl]
  .platform_drv_remove+0x40/0x90
  .device_release_driver_internal+0x160/0x2c0
  .driver_detach+0x64/0xd0
  .bus_remove_driver+0x70/0xf0
  .driver_unregister+0x38/0x80
  .platform_driver_unregister+0x14/0x30
  .fsl_sata_driver_exit+0x18/0xa20 [sata_fsl]
 ---[ end trace 0ea876d4076908f5 ]---

The driver creates the mapping by calling irq_of_parse_and_map(),
so it also has to dispose the mapping. But the easy way out is to
simply use platform_get_irq() instead of irq_of_parse_map().

In this case the mapping is not managed by the device but by
the of core, so the device has not to dispose the mapping.

Reported-by: Hulk Robot <hulkci@huawei.com>
Signed-off-by: Baokun Li <libaokun1@huawei.com>
---
 drivers/ata/sata_fsl.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/ata/sata_fsl.c b/drivers/ata/sata_fsl.c
index 30759fd1c3a2..011daac4a14e 100644
--- a/drivers/ata/sata_fsl.c
+++ b/drivers/ata/sata_fsl.c
@@ -1493,7 +1493,7 @@ static int sata_fsl_probe(struct platform_device *ofdev)
 	host_priv->ssr_base = ssr_base;
 	host_priv->csr_base = csr_base;
 
-	irq = irq_of_parse_and_map(ofdev->dev.of_node, 0);
+	irq = platform_get_irq(ofdev, 0);
 	if (!irq) {
 		dev_err(&ofdev->dev, "invalid irq from platform\n");
 		goto error_exit_with_cleanup;
@@ -1570,8 +1570,6 @@ static int sata_fsl_remove(struct platform_device *ofdev)
 
 	ata_host_detach(host);
 
-	irq_dispose_mapping(host_priv->irq);
-
 	return 0;
 }
 
-- 
2.31.1

Re: [PATCH -next 2/2] sata_fsl: fix warning in remove_proc_entry when rmmod sata_fsl

[Sergei Shtylyov]

Hello!

On 19.11.2021 7:11, Baokun Li wrote:

> Trying to remove the fsl-sata module in the PPC64 GNU/Linux
> leads to the following warning:
>   ------------[ cut here ]------------
>   remove_proc_entry: removing non-empty directory 'irq/69',
>     leaking at least 'fsl-sata[ff0221000.sata]'
>   WARNING: CPU: 3 PID: 1048 at fs/proc/generic.c:722
>     .remove_proc_entry+0x20c/0x220
>   IRQMASK: 0
>   NIP [c00000000033826c] .remove_proc_entry+0x20c/0x220
>   LR [c000000000338268] .remove_proc_entry+0x208/0x220
>   Call Trace:
>    .remove_proc_entry+0x208/0x220 (unreliable)
>    .unregister_irq_proc+0x104/0x140
>    .free_desc+0x44/0xb0
>    .irq_free_descs+0x9c/0xf0
>    .irq_dispose_mapping+0x64/0xa0
>    .sata_fsl_remove+0x58/0xa0 [sata_fsl]
>    .platform_drv_remove+0x40/0x90
>    .device_release_driver_internal+0x160/0x2c0
>    .driver_detach+0x64/0xd0
>    .bus_remove_driver+0x70/0xf0
>    .driver_unregister+0x38/0x80
>    .platform_driver_unregister+0x14/0x30
>    .fsl_sata_driver_exit+0x18/0xa20 [sata_fsl]
>   ---[ end trace 0ea876d4076908f5 ]---
> 
> The driver creates the mapping by calling irq_of_parse_and_map(),
> so it also has to dispose the mapping. But the easy way out is to
> simply use platform_get_irq() instead of irq_of_parse_map().

    Not that easy. :-)

> In this case the mapping is not managed by the device but by
> the of core, so the device has not to dispose the mapping.
> 
> Reported-by: Hulk Robot <hulkci@huawei.com>
> Signed-off-by: Baokun Li <libaokun1@huawei.com>
> ---
>   drivers/ata/sata_fsl.c | 4 +---
>   1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/ata/sata_fsl.c b/drivers/ata/sata_fsl.c
> index 30759fd1c3a2..011daac4a14e 100644
> --- a/drivers/ata/sata_fsl.c
> +++ b/drivers/ata/sata_fsl.c
> @@ -1493,7 +1493,7 @@ static int sata_fsl_probe(struct platform_device *ofdev)
>   	host_priv->ssr_base = ssr_base;
>   	host_priv->csr_base = csr_base;
>   
> -	irq = irq_of_parse_and_map(ofdev->dev.of_node, 0);
> +	irq = platform_get_irq(ofdev, 0);
>   	if (!irq) {

	if (irq < 0) {

    platform_get_irq() returns negative error codes, not 0 on failure.

[...]

MBR, Sergey

Re: [PATCH -next 2/2] sata_fsl: fix warning in remove_proc_entry when rmmod sata_fsl

[Damien Le Moal]

On 11/19/21 13:11, Baokun Li wrote:
> Trying to remove the fsl-sata module in the PPC64 GNU/Linux
> leads to the following warning:
>  ------------[ cut here ]------------
>  remove_proc_entry: removing non-empty directory 'irq/69',
>    leaking at least 'fsl-sata[ff0221000.sata]'
>  WARNING: CPU: 3 PID: 1048 at fs/proc/generic.c:722
>    .remove_proc_entry+0x20c/0x220
>  IRQMASK: 0
>  NIP [c00000000033826c] .remove_proc_entry+0x20c/0x220
>  LR [c000000000338268] .remove_proc_entry+0x208/0x220
>  Call Trace:
>   .remove_proc_entry+0x208/0x220 (unreliable)
>   .unregister_irq_proc+0x104/0x140
>   .free_desc+0x44/0xb0
>   .irq_free_descs+0x9c/0xf0
>   .irq_dispose_mapping+0x64/0xa0
>   .sata_fsl_remove+0x58/0xa0 [sata_fsl]
>   .platform_drv_remove+0x40/0x90
>   .device_release_driver_internal+0x160/0x2c0
>   .driver_detach+0x64/0xd0
>   .bus_remove_driver+0x70/0xf0
>   .driver_unregister+0x38/0x80
>   .platform_driver_unregister+0x14/0x30
>   .fsl_sata_driver_exit+0x18/0xa20 [sata_fsl]
>  ---[ end trace 0ea876d4076908f5 ]---
> 
> The driver creates the mapping by calling irq_of_parse_and_map(),
> so it also has to dispose the mapping. But the easy way out is to
> simply use platform_get_irq() instead of irq_of_parse_map().
> 
> In this case the mapping is not managed by the device but by
> the of core, so the device has not to dispose the mapping.
> 
> Reported-by: Hulk Robot <hulkci@huawei.com>
> Signed-off-by: Baokun Li <libaokun1@huawei.com>
> ---
>  drivers/ata/sata_fsl.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/ata/sata_fsl.c b/drivers/ata/sata_fsl.c
> index 30759fd1c3a2..011daac4a14e 100644
> --- a/drivers/ata/sata_fsl.c
> +++ b/drivers/ata/sata_fsl.c
> @@ -1493,7 +1493,7 @@ static int sata_fsl_probe(struct platform_device *ofdev)
>  	host_priv->ssr_base = ssr_base;
>  	host_priv->csr_base = csr_base;
>  
> -	irq = irq_of_parse_and_map(ofdev->dev.of_node, 0);
> +	irq = platform_get_irq(ofdev, 0);
>  	if (!irq) {

Please see the kdoc comment for platform_get_irq() in
drivers/base/platform.c. The error check must be "if (irq < 0)".

Can you send a V2 with that fixed and tested ?

>  		dev_err(&ofdev->dev, "invalid irq from platform\n");
>  		goto error_exit_with_cleanup;
> @@ -1570,8 +1570,6 @@ static int sata_fsl_remove(struct platform_device *ofdev)
>  
>  	ata_host_detach(host);
>  
> -	irq_dispose_mapping(host_priv->irq);
> -
>  	return 0;
>  }
>  
> 


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH -next 2/2] sata_fsl: fix warning in remove_proc_entry when rmmod sata_fsl

[libaokun (A)]

在 2021/11/19 23:43, Sergei Shtylyov 写道:
> Hello!
>
> On 19.11.2021 7:11, Baokun Li wrote:
>
>> Trying to remove the fsl-sata module in the PPC64 GNU/Linux
>> leads to the following warning:
>>   ------------[ cut here ]------------
>>   remove_proc_entry: removing non-empty directory 'irq/69',
>>     leaking at least 'fsl-sata[ff0221000.sata]'
>>   WARNING: CPU: 3 PID: 1048 at fs/proc/generic.c:722
>>     .remove_proc_entry+0x20c/0x220
>>   IRQMASK: 0
>>   NIP [c00000000033826c] .remove_proc_entry+0x20c/0x220
>>   LR [c000000000338268] .remove_proc_entry+0x208/0x220
>>   Call Trace:
>>    .remove_proc_entry+0x208/0x220 (unreliable)
>>    .unregister_irq_proc+0x104/0x140
>>    .free_desc+0x44/0xb0
>>    .irq_free_descs+0x9c/0xf0
>>    .irq_dispose_mapping+0x64/0xa0
>>    .sata_fsl_remove+0x58/0xa0 [sata_fsl]
>>    .platform_drv_remove+0x40/0x90
>>    .device_release_driver_internal+0x160/0x2c0
>>    .driver_detach+0x64/0xd0
>>    .bus_remove_driver+0x70/0xf0
>>    .driver_unregister+0x38/0x80
>>    .platform_driver_unregister+0x14/0x30
>>    .fsl_sata_driver_exit+0x18/0xa20 [sata_fsl]
>>   ---[ end trace 0ea876d4076908f5 ]---
>>
>> The driver creates the mapping by calling irq_of_parse_and_map(),
>> so it also has to dispose the mapping. But the easy way out is to
>> simply use platform_get_irq() instead of irq_of_parse_map().
>
>   Not that easy. :-)
>
>> In this case the mapping is not managed by the device but by
>> the of core, so the device has not to dispose the mapping.
>>
>> Reported-by: Hulk Robot <hulkci@huawei.com>
>> Signed-off-by: Baokun Li <libaokun1@huawei.com>
>> ---
>>   drivers/ata/sata_fsl.c | 4 +---
>>   1 file changed, 1 insertion(+), 3 deletions(-)
>>
>> diff --git a/drivers/ata/sata_fsl.c b/drivers/ata/sata_fsl.c
>> index 30759fd1c3a2..011daac4a14e 100644
>> --- a/drivers/ata/sata_fsl.c
>> +++ b/drivers/ata/sata_fsl.c
>> @@ -1493,7 +1493,7 @@ static int sata_fsl_probe(struct 
>> platform_device *ofdev)
>>       host_priv->ssr_base = ssr_base;
>>       host_priv->csr_base = csr_base;
>>   -    irq = irq_of_parse_and_map(ofdev->dev.of_node, 0);
>> +    irq = platform_get_irq(ofdev, 0);
>>       if (!irq) {
>
>     if (irq < 0) {
>
>   platform_get_irq() returns negative error codes, not 0 on failure.
>
> [...]
>
> MBR, Sergey
> .

I didn't notice the change in this return value, and the test didn't 
cover the error branch.

Thank you very much for your correction.

I'm about to send a patch v2 with the changes suggested by you.

-- 
With Best Regards,
Baokun Li
.

Re: [PATCH -next 2/2] sata_fsl: fix warning in remove_proc_entry when rmmod sata_fsl

[libaokun (A)]

在 2021/11/19 23:43, Sergei Shtylyov 写道:
> Hello!
>
> On 19.11.2021 7:11, Baokun Li wrote:
>
>> Trying to remove the fsl-sata module in the PPC64 GNU/Linux
>> leads to the following warning:
>>   ------------[ cut here ]------------
>>   remove_proc_entry: removing non-empty directory 'irq/69',
>>     leaking at least 'fsl-sata[ff0221000.sata]'
>>   WARNING: CPU: 3 PID: 1048 at fs/proc/generic.c:722
>>     .remove_proc_entry+0x20c/0x220
>>   IRQMASK: 0
>>   NIP [c00000000033826c] .remove_proc_entry+0x20c/0x220
>>   LR [c000000000338268] .remove_proc_entry+0x208/0x220
>>   Call Trace:
>>    .remove_proc_entry+0x208/0x220 (unreliable)
>>    .unregister_irq_proc+0x104/0x140
>>    .free_desc+0x44/0xb0
>>    .irq_free_descs+0x9c/0xf0
>>    .irq_dispose_mapping+0x64/0xa0
>>    .sata_fsl_remove+0x58/0xa0 [sata_fsl]
>>    .platform_drv_remove+0x40/0x90
>>    .device_release_driver_internal+0x160/0x2c0
>>    .driver_detach+0x64/0xd0
>>    .bus_remove_driver+0x70/0xf0
>>    .driver_unregister+0x38/0x80
>>    .platform_driver_unregister+0x14/0x30
>>    .fsl_sata_driver_exit+0x18/0xa20 [sata_fsl]
>>   ---[ end trace 0ea876d4076908f5 ]---
>>
>> The driver creates the mapping by calling irq_of_parse_and_map(),
>> so it also has to dispose the mapping. But the easy way out is to
>> simply use platform_get_irq() instead of irq_of_parse_map().
>
>   Not that easy. :-)
>
>> In this case the mapping is not managed by the device but by
>> the of core, so the device has not to dispose the mapping.
>>
>> Reported-by: Hulk Robot <hulkci@huawei.com>
>> Signed-off-by: Baokun Li <libaokun1@huawei.com>
>> ---
>>   drivers/ata/sata_fsl.c | 4 +---
>>   1 file changed, 1 insertion(+), 3 deletions(-)
>>
>> diff --git a/drivers/ata/sata_fsl.c b/drivers/ata/sata_fsl.c
>> index 30759fd1c3a2..011daac4a14e 100644
>> --- a/drivers/ata/sata_fsl.c
>> +++ b/drivers/ata/sata_fsl.c
>> @@ -1493,7 +1493,7 @@ static int sata_fsl_probe(struct 
>> platform_device *ofdev)
>>       host_priv->ssr_base = ssr_base;
>>       host_priv->csr_base = csr_base;
>>   -    irq = irq_of_parse_and_map(ofdev->dev.of_node, 0);
>> +    irq = platform_get_irq(ofdev, 0);
>>       if (!irq) {
>
>     if (irq < 0) {
>
>   platform_get_irq() returns negative error codes, not 0 on failure.
>
> [...]
>
> MBR, Sergey
> .

I didn't notice the change in this return value, and the test didn't 
cover the error branch.

Thank you very much for your advice.

I'm about to send a patch v2 with the changes suggested by you.

-- 
With Best Regards,
Baokun Li
.

Re: [PATCH -next 2/2] sata_fsl: fix warning in remove_proc_entry when rmmod sata_fsl

[libaokun (A)]

在 2021/11/20 6:18, Damien Le Moal 写道:
> On 11/19/21 13:11, Baokun Li wrote:
>> Trying to remove the fsl-sata module in the PPC64 GNU/Linux
>> leads to the following warning:
>>   ------------[ cut here ]------------
>>   remove_proc_entry: removing non-empty directory 'irq/69',
>>     leaking at least 'fsl-sata[ff0221000.sata]'
>>   WARNING: CPU: 3 PID: 1048 at fs/proc/generic.c:722
>>     .remove_proc_entry+0x20c/0x220
>>   IRQMASK: 0
>>   NIP [c00000000033826c] .remove_proc_entry+0x20c/0x220
>>   LR [c000000000338268] .remove_proc_entry+0x208/0x220
>>   Call Trace:
>>    .remove_proc_entry+0x208/0x220 (unreliable)
>>    .unregister_irq_proc+0x104/0x140
>>    .free_desc+0x44/0xb0
>>    .irq_free_descs+0x9c/0xf0
>>    .irq_dispose_mapping+0x64/0xa0
>>    .sata_fsl_remove+0x58/0xa0 [sata_fsl]
>>    .platform_drv_remove+0x40/0x90
>>    .device_release_driver_internal+0x160/0x2c0
>>    .driver_detach+0x64/0xd0
>>    .bus_remove_driver+0x70/0xf0
>>    .driver_unregister+0x38/0x80
>>    .platform_driver_unregister+0x14/0x30
>>    .fsl_sata_driver_exit+0x18/0xa20 [sata_fsl]
>>   ---[ end trace 0ea876d4076908f5 ]---
>>
>> The driver creates the mapping by calling irq_of_parse_and_map(),
>> so it also has to dispose the mapping. But the easy way out is to
>> simply use platform_get_irq() instead of irq_of_parse_map().
>>
>> In this case the mapping is not managed by the device but by
>> the of core, so the device has not to dispose the mapping.
>>
>> Reported-by: Hulk Robot <hulkci@huawei.com>
>> Signed-off-by: Baokun Li <libaokun1@huawei.com>
>> ---
>>   drivers/ata/sata_fsl.c | 4 +---
>>   1 file changed, 1 insertion(+), 3 deletions(-)
>>
>> diff --git a/drivers/ata/sata_fsl.c b/drivers/ata/sata_fsl.c
>> index 30759fd1c3a2..011daac4a14e 100644
>> --- a/drivers/ata/sata_fsl.c
>> +++ b/drivers/ata/sata_fsl.c
>> @@ -1493,7 +1493,7 @@ static int sata_fsl_probe(struct platform_device *ofdev)
>>   	host_priv->ssr_base = ssr_base;
>>   	host_priv->csr_base = csr_base;
>>   
>> -	irq = irq_of_parse_and_map(ofdev->dev.of_node, 0);
>> +	irq = platform_get_irq(ofdev, 0);
>>   	if (!irq) {
> Please see the kdoc comment for platform_get_irq() in
> drivers/base/platform.c. The error check must be "if (irq < 0)".
>
> Can you send a V2 with that fixed and tested ?
>
>>   		dev_err(&ofdev->dev, "invalid irq from platform\n");
>>   		goto error_exit_with_cleanup;
>> @@ -1570,8 +1570,6 @@ static int sata_fsl_remove(struct platform_device *ofdev)
>>   
>>   	ata_host_detach(host);
>>   
>> -	irq_dispose_mapping(host_priv->irq);
>> -
>>   	return 0;
>>   }
>>   
>>
>
I didn't notice the change in this return value, and the test didn't 
cover the error branch.

Thank you very much for your advice.

I'm about to send a patch v2 with the changes suggested by you.

-- With Best Regards, Baokun Li .

Re: [PATCH -next 2/2] sata_fsl: fix warning in remove_proc_entry when rmmod sata_fsl

[Damien Le Moal]

On 11/20/21 00:43, Sergei Shtylyov wrote:
>> diff --git a/drivers/ata/sata_fsl.c b/drivers/ata/sata_fsl.c
>> index 30759fd1c3a2..011daac4a14e 100644
>> --- a/drivers/ata/sata_fsl.c
>> +++ b/drivers/ata/sata_fsl.c
>> @@ -1493,7 +1493,7 @@ static int sata_fsl_probe(struct platform_device *ofdev)
>>   	host_priv->ssr_base = ssr_base;
>>   	host_priv->csr_base = csr_base;
>>   
>> -	irq = irq_of_parse_and_map(ofdev->dev.of_node, 0);
>> +	irq = platform_get_irq(ofdev, 0);
>>   	if (!irq) {
> 
> 	if (irq < 0) {
> 
>     platform_get_irq() returns negative error codes, not 0 on failure.

Sergei,

By the way, the kdoc comment for platform_get_irq() says:

"Return: non-zero IRQ number on success, negative error number on failure."

But irq	0 is valid, isn't it ? So shouldn't this be changed to something
like:

"Return: IRQ number on success, negative error number on failure."

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH -next 2/2] sata_fsl: fix warning in remove_proc_entry when rmmod sata_fsl

[Sergei Shtylyov]

On 20.11.2021 9:08, Damien Le Moal wrote:
> On 11/20/21 00:43, Sergei Shtylyov wrote:
>>> diff --git a/drivers/ata/sata_fsl.c b/drivers/ata/sata_fsl.c
>>> index 30759fd1c3a2..011daac4a14e 100644
>>> --- a/drivers/ata/sata_fsl.c
>>> +++ b/drivers/ata/sata_fsl.c
>>> @@ -1493,7 +1493,7 @@ static int sata_fsl_probe(struct platform_device *ofdev)
>>>    	host_priv->ssr_base = ssr_base;
>>>    	host_priv->csr_base = csr_base;
>>>    
>>> -	irq = irq_of_parse_and_map(ofdev->dev.of_node, 0);
>>> +	irq = platform_get_irq(ofdev, 0);
>>>    	if (!irq) {
>>
>> 	if (irq < 0) {
>>
>>      platform_get_irq() returns negative error codes, not 0 on failure.
> 
> Sergei,
> 
> By the way, the kdoc comment for platform_get_irq() says:
> 
> "Return: non-zero IRQ number on success, negative error number on failure."
> 
> But irq	0 is valid, isn't it ? So shouldn't this be changed to something
> like:
> 
> "Return: IRQ number on success, negative error number on failure."

    No, it's not valid (the current code WARN()s about it) and won't be 
returned anymore after my patch [1] gets applied.

[1] https://marc.info/?l=linux-kernel&m=163623041902285

MBR, Sergei

Re: [PATCH -next 2/2] sata_fsl: fix warning in remove_proc_entry when rmmod sata_fsl

[Damien Le Moal]

On 2021/11/20 18:51, Sergei Shtylyov wrote:
> On 20.11.2021 9:08, Damien Le Moal wrote:
>> On 11/20/21 00:43, Sergei Shtylyov wrote:
>>>> diff --git a/drivers/ata/sata_fsl.c b/drivers/ata/sata_fsl.c
>>>> index 30759fd1c3a2..011daac4a14e 100644
>>>> --- a/drivers/ata/sata_fsl.c
>>>> +++ b/drivers/ata/sata_fsl.c
>>>> @@ -1493,7 +1493,7 @@ static int sata_fsl_probe(struct platform_device *ofdev)
>>>>    	host_priv->ssr_base = ssr_base;
>>>>    	host_priv->csr_base = csr_base;
>>>>    
>>>> -	irq = irq_of_parse_and_map(ofdev->dev.of_node, 0);
>>>> +	irq = platform_get_irq(ofdev, 0);
>>>>    	if (!irq) {
>>>
>>> 	if (irq < 0) {
>>>
>>>      platform_get_irq() returns negative error codes, not 0 on failure.
>>
>> Sergei,
>>
>> By the way, the kdoc comment for platform_get_irq() says:
>>
>> "Return: non-zero IRQ number on success, negative error number on failure."
>>
>> But irq	0 is valid, isn't it ? So shouldn't this be changed to something
>> like:
>>
>> "Return: IRQ number on success, negative error number on failure."
> 
>     No, it's not valid (the current code WARN()s about it) and won't be 
> returned anymore after my patch [1] gets applied.
> 
> [1] https://marc.info/?l=linux-kernel&m=163623041902285

OK. Got it. Thanks.

> 
> MBR, Sergei
> 


-- 
Damien Le Moal
Western Digital Research