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

[liqiong <liqiong@nfschina.com>]

在 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
>