[PATCH] ata: handle failure of devm_ioremap()

[PATCH] ata: handle failure of devm_ioremap()

[Li Qiong]

As the possible failure of the devm_ioremap(), the return value
could be NULL. Therefore it should be better to check it and
print error message, return '-ENOMEM' error code.

Signed-off-by: Li Qiong <liqiong@nfschina.com>
---
 drivers/ata/pata_pxa.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/ata/pata_pxa.c b/drivers/ata/pata_pxa.c
index 985f42c4fd70..cd1a8f37f920 100644
--- a/drivers/ata/pata_pxa.c
+++ b/drivers/ata/pata_pxa.c
@@ -228,6 +228,11 @@ static int pxa_ata_probe(struct platform_device *pdev)
 	ap->ioaddr.bmdma_addr	= devm_ioremap(&pdev->dev, dma_res->start,
 						resource_size(dma_res));
 
+	if (!ap->ioaddr.cmd_addr || !ap->ioaddr.ctl_addr || !ap->ioaddr.bmdma_addr) {
+		dev_err(&pdev->dev, "failed to map ap->ioaddr\n");
+		return -ENOMEM;
+	}
+
 	/*
 	 * Adjust register offsets
 	 */
-- 
2.11.0

Re: [PATCH] ata: handle failure of devm_ioremap()

[Sergey Shtylyov]

Hello!

   The subject should include the driver's name, like below:

ata: pata_pxa: handle failure of devm_ioremap()

On 6/12/22 10:32 AM, Li Qiong wrote:

> As the possible failure of the devm_ioremap(), the return value
> could be NULL. Therefore it should be better to check it and
> print error message, return '-ENOMEM' error code.
> 
> Signed-off-by: Li Qiong <liqiong@nfschina.com>


Reviewed-by: Sergey Shtylyov <s.shtylyov@omp.ru>

[...]

MBR, Sergey

Re: [PATCH] ata: handle failure of devm_ioremap()

[liqiong]

在 2022年06月12日 17:06, Sergey Shtylyov 写道:
> Hello!
>
>    The subject should include the driver's name, like below:
>
> ata: pata_pxa: handle failure of devm_ioremap()
>
> On 6/12/22 10:32 AM, Li Qiong wrote:
>
>> As the possible failure of the devm_ioremap(), the return value
>> could be NULL. Therefore it should be better to check it and
>> print error message, return '-ENOMEM' error code.
>>
>> Signed-off-by: Li Qiong <liqiong@nfschina.com>
>
> Reviewed-by: Sergey Shtylyov <s.shtylyov@omp.ru>
>
> [...]
>
> MBR, Sergey

Thanks, I will submit a v2 patch.

[PATCH v2] ata: pata_pxa: handle failure of devm_ioremap()

[Li Qiong]

As the possible failure of the devm_ioremap(), the return value
could be NULL. Therefore it should be better to check it and
print error message, return '-ENOMEM' error code.

Signed-off-by: Li Qiong <liqiong@nfschina.com>
Reviewed-by: Sergey Shtylyov <s.shtylyov@omp.ru>
---
v2:
- add driver's name (pata_pxa) to subject.
---
 drivers/ata/pata_pxa.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/ata/pata_pxa.c b/drivers/ata/pata_pxa.c
index 985f42c4fd70..cd1a8f37f920 100644
--- a/drivers/ata/pata_pxa.c
+++ b/drivers/ata/pata_pxa.c
@@ -228,6 +228,11 @@ static int pxa_ata_probe(struct platform_device *pdev)
 	ap->ioaddr.bmdma_addr	= devm_ioremap(&pdev->dev, dma_res->start,
 						resource_size(dma_res));
 
+	if (!ap->ioaddr.cmd_addr || !ap->ioaddr.ctl_addr || !ap->ioaddr.bmdma_addr) {
+		dev_err(&pdev->dev, "failed to map ap->ioaddr\n");
+		return -ENOMEM;
+	}
+
 	/*
 	 * Adjust register offsets
 	 */
-- 
2.11.0

Re: [PATCH v2] ata: pata_pxa: handle failure of devm_ioremap()

[Sergei Shtylyov]

On 6/12/22 3:57 PM, Li Qiong wrote:

> As the possible failure of the devm_ioremap(), the return value
> could be NULL. Therefore it should be better to check it and
> print error message, return '-ENOMEM' error code.
> 
> Signed-off-by: Li Qiong <liqiong@nfschina.com>
> Reviewed-by: Sergey Shtylyov <s.shtylyov@omp.ru>
> ---
> v2:
> - add driver's name (pata_pxa) to subject.
> ---
>  drivers/ata/pata_pxa.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/ata/pata_pxa.c b/drivers/ata/pata_pxa.c
> index 985f42c4fd70..cd1a8f37f920 100644
> --- a/drivers/ata/pata_pxa.c
> +++ b/drivers/ata/pata_pxa.c
> @@ -228,6 +228,11 @@ static int pxa_ata_probe(struct platform_device *pdev)
>  	ap->ioaddr.bmdma_addr	= devm_ioremap(&pdev->dev, dma_res->start,
>  						resource_size(dma_res));

   Looking again into this driver, this statement doesn't make sense: dma_res
points to a DMA resource, calling devm_ioremap() on it is just wrong... and
'ap->ioaddr.bmdma_addr' doesn;t seem to be used anyways...

MBR, Sergey

Re: [PATCH v2] ata: pata_pxa: handle failure of devm_ioremap()

[Damien Le Moal]

On 6/13/22 03:37, Sergei Shtylyov wrote:
> On 6/12/22 3:57 PM, Li Qiong wrote:
> 
>> As the possible failure of the devm_ioremap(), the return value
>> could be NULL. Therefore it should be better to check it and
>> print error message, return '-ENOMEM' error code.

This error is very unlikely. So unless you are seeing actual problems in
the field, I do not think it is worth fixing.

>>
>> Signed-off-by: Li Qiong <liqiong@nfschina.com>
>> Reviewed-by: Sergey Shtylyov <s.shtylyov@omp.ru>
>> ---
>> v2:
>> - add driver's name (pata_pxa) to subject.
>> ---
>>  drivers/ata/pata_pxa.c | 5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/drivers/ata/pata_pxa.c b/drivers/ata/pata_pxa.c
>> index 985f42c4fd70..cd1a8f37f920 100644
>> --- a/drivers/ata/pata_pxa.c
>> +++ b/drivers/ata/pata_pxa.c
>> @@ -228,6 +228,11 @@ static int pxa_ata_probe(struct platform_device *pdev)
>>  	ap->ioaddr.bmdma_addr	= devm_ioremap(&pdev->dev, dma_res->start,
>>  						resource_size(dma_res));
> 
>    Looking again into this driver, this statement doesn't make sense: dma_res
> points to a DMA resource, calling devm_ioremap() on it is just wrong... and

Yes, having to do an ioremap of an IORESOURCE_DMA resource is rather
unusual. dmaengine_slave_config() should be doing anything that is
required for that resource.

> 'ap->ioaddr.bmdma_addr' doesn;t seem to be used anyways...

It is used in lbata-sff.c.

A much cleaner fix would be to use
devm_platform_get_and_ioremap_resource() or
devm_platform_ioremap_resource() which will also remove the call to
platform_get_resource((). But as mentioned above, unless this is fixing an
actual bug in production, I do not think this is worth it.

> 
> MBR, Sergey


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v2] ata: pata_pxa: handle failure of devm_ioremap()

[liqiong]

在 2022年06月13日 06:47, Damien Le Moal 写道:
> On 6/13/22 03:37, Sergei Shtylyov wrote:
>> On 6/12/22 3:57 PM, Li Qiong wrote:
>>
>>> As the possible failure of the devm_ioremap(), the return value
>>> could be NULL. Therefore it should be better to check it and
>>> print error message, return '-ENOMEM' error code.
> This error is very unlikely. So unless you are seeing actual problems in
> the field, I do not think it is worth fixing.
Agree, It's very unlikely.
This patch follows the guide of  "KernelJanitors",  wants to refine the code a little bit .
I searched and found that most of the other  codes had checked the return value. 
Actually,  no actual bug relation to this patch.

Thanks,
Li Qiong

>
>>> Signed-off-by: Li Qiong <liqiong@nfschina.com>
>>> Reviewed-by: Sergey Shtylyov <s.shtylyov@omp.ru>
>>> ---
>>> v2:
>>> - add driver's name (pata_pxa) to subject.
>>> ---
>>>  drivers/ata/pata_pxa.c | 5 +++++
>>>  1 file changed, 5 insertions(+)
>>>
>>> diff --git a/drivers/ata/pata_pxa.c b/drivers/ata/pata_pxa.c
>>> index 985f42c4fd70..cd1a8f37f920 100644
>>> --- a/drivers/ata/pata_pxa.c
>>> +++ b/drivers/ata/pata_pxa.c
>>> @@ -228,6 +228,11 @@ static int pxa_ata_probe(struct platform_device *pdev)
>>>  	ap->ioaddr.bmdma_addr	= devm_ioremap(&pdev->dev, dma_res->start,
>>>  						resource_size(dma_res));
>>    Looking again into this driver, this statement doesn't make sense: dma_res
>> points to a DMA resource, calling devm_ioremap() on it is just wrong... and
> Yes, having to do an ioremap of an IORESOURCE_DMA resource is rather
> unusual. dmaengine_slave_config() should be doing anything that is
> required for that resource.
>
>> 'ap->ioaddr.bmdma_addr' doesn;t seem to be used anyways...
> It is used in lbata-sff.c.
>
> A much cleaner fix would be to use
> devm_platform_get_and_ioremap_resource() or
> devm_platform_ioremap_resource() which will also remove the call to
> platform_get_resource((). But as mentioned above, unless this is fixing an
> actual bug in production, I do not think this is worth it.
>
>> MBR, Sergey
>

Re: [PATCH v2] ata: pata_pxa: handle failure of devm_ioremap()

[Sergey Shtylyov]

On 6/13/22 1:47 AM, Damien Le Moal wrote:

>>> As the possible failure of the devm_ioremap(), the return value
>>> could be NULL. Therefore it should be better to check it and
>>> print error message, return '-ENOMEM' error code.
> 
> This error is very unlikely. So unless you are seeing actual problems in
> the field, I do not think it is worth fixing.

   The error paths should absolutely be fixed. It helps avoid an oops later...

>>>
>>> Signed-off-by: Li Qiong <liqiong@nfschina.com>
>>> Reviewed-by: Sergey Shtylyov <s.shtylyov@omp.ru>
>>> ---
>>> v2:
>>> - add driver's name (pata_pxa) to subject.
>>> ---
>>>  drivers/ata/pata_pxa.c | 5 +++++
>>>  1 file changed, 5 insertions(+)
>>>
>>> diff --git a/drivers/ata/pata_pxa.c b/drivers/ata/pata_pxa.c
>>> index 985f42c4fd70..cd1a8f37f920 100644
>>> --- a/drivers/ata/pata_pxa.c
>>> +++ b/drivers/ata/pata_pxa.c
>>> @@ -228,6 +228,11 @@ static int pxa_ata_probe(struct platform_device *pdev)
>>>  	ap->ioaddr.bmdma_addr	= devm_ioremap(&pdev->dev, dma_res->start,
>>>  						resource_size(dma_res));
>>
>>    Looking again into this driver, this statement doesn't make sense: dma_res
>> points to a DMA resource, calling devm_ioremap() on it is just wrong... and
> 
> Yes, having to do an ioremap of an IORESOURCE_DMA resource is rather
> unusual. dmaengine_slave_config() should be doing anything that is
> required for that resource.
> 
>> 'ap->ioaddr.bmdma_addr' doesn;t seem to be used anyways...
> 
> It is used in lbata-sff.c.

   Where exactly? To me, it looked like all ata_bmdma_port_ops were overridden
by the driver... Even if not so, I don't think such code is correct...

> 
> A much cleaner fix would be to use
> devm_platform_get_and_ioremap_resource() or
> devm_platform_ioremap_resource() which will also remove the call to
> platform_get_resource(().

   This is an -rc1 material.

> But as mentioned above, unless this is fixing an
> actual bug in production, I do not think this is worth it.

   I strongly disagree here.

MBR, Sergey

Re: [PATCH v2] ata: pata_pxa: handle failure of devm_ioremap()

[Damien Le Moal]

On 6/13/22 20:50, Sergey Shtylyov wrote:
> On 6/13/22 1:47 AM, Damien Le Moal wrote:
> 
>>>> As the possible failure of the devm_ioremap(), the return value
>>>> could be NULL. Therefore it should be better to check it and
>>>> print error message, return '-ENOMEM' error code.
>>
>> This error is very unlikely. So unless you are seeing actual problems in
>> the field, I do not think it is worth fixing.
> 
>    The error paths should absolutely be fixed. It helps avoid an oops later...
> 
>>>>
>>>> Signed-off-by: Li Qiong <liqiong@nfschina.com>
>>>> Reviewed-by: Sergey Shtylyov <s.shtylyov@omp.ru>
>>>> ---
>>>> v2:
>>>> - add driver's name (pata_pxa) to subject.
>>>> ---
>>>>  drivers/ata/pata_pxa.c | 5 +++++
>>>>  1 file changed, 5 insertions(+)
>>>>
>>>> diff --git a/drivers/ata/pata_pxa.c b/drivers/ata/pata_pxa.c
>>>> index 985f42c4fd70..cd1a8f37f920 100644
>>>> --- a/drivers/ata/pata_pxa.c
>>>> +++ b/drivers/ata/pata_pxa.c
>>>> @@ -228,6 +228,11 @@ static int pxa_ata_probe(struct platform_device *pdev)
>>>>  	ap->ioaddr.bmdma_addr	= devm_ioremap(&pdev->dev, dma_res->start,
>>>>  						resource_size(dma_res));
>>>
>>>    Looking again into this driver, this statement doesn't make sense: dma_res
>>> points to a DMA resource, calling devm_ioremap() on it is just wrong... and
>>
>> Yes, having to do an ioremap of an IORESOURCE_DMA resource is rather
>> unusual. dmaengine_slave_config() should be doing anything that is
>> required for that resource.
>>
>>> 'ap->ioaddr.bmdma_addr' doesn;t seem to be used anyways...
>>
>> It is used in lbata-sff.c.
> 
>    Where exactly? To me, it looked like all ata_bmdma_port_ops were overridden
> by the driver... Even if not so, I don't think such code is correct...
> 
>>
>> A much cleaner fix would be to use
>> devm_platform_get_and_ioremap_resource() or
>> devm_platform_ioremap_resource() which will also remove the call to
>> platform_get_resource(().
> 
>    This is an -rc1 material.
> 
>> But as mentioned above, unless this is fixing an
>> actual bug in production, I do not think this is worth it.
> 
>    I strongly disagree here.

Not opposed to fixing this driver. But definitely low priority.

> 
> MBR, Sergey


-- 
Damien Le Moal
Western Digital Research