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@xxxxxxxxxxxx> >>>> Reviewed-by: Sergey Shtylyov <s.shtylyov@xxxxxx> >>>> --- >>>> 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
