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

[Damien Le Moal <damien.lemoal@opensource.wdc.com>]

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