Re: [PATCH 6/9] ethernet: cavium: Replace deprecated PCI functions

[Andy Shevchenko <andy@kernel.org>]

On Tue, Aug 20, 2024 at 09:40:09AM +0200, Philipp Stanner wrote:
> On Mon, 2024-08-19 at 21:23 +0300, Andy Shevchenko wrote:
> > On Mon, Aug 19, 2024 at 06:51:46PM +0200, Philipp Stanner wrote:

...

> > cavium_ptp_probe()
> > 
> > > -	pcim_iounmap_regions(pdev, 1 << PCI_PTP_BAR_NO);
> > > +	pcim_iounmap_region(pdev, PCI_PTP_BAR_NO);
> > >  
> > >  error_free:
> > >  	devm_kfree(dev, clock);
> > 
> > Both are questionable. Why do we need either of them?
> 
> You seem to criticize my pcim_iounmap_region() etc. in other unwind
> paths, too.

Yes, having devm/pcim/etc_m in the clean up / error paths seems at bare minimum
confusing, or reveals wrong use of them or even misunderstanding the concept...

And it's not your fault, it was already in those drivers like that...

> I think your criticism is often justified. This driver
> here, however, was the one which made me suspicious and hesitate and
> removing those calls; because of the code below:
> 
> 
> 	pcim_iounmap_region(pdev, PCI_PTP_BAR_NO);
> 
> error_free:
> 	devm_kfree(dev, clock);
> 
> error:
> 	/* For `cavium_ptp_get()` we need to differentiate between the case
> 	 * when the core has not tried to probe this device and the case when
> 	 * the probe failed.  In the later case we pretend that the
> 	 * initialization was successful and keep the error in
> 	 * `dev->driver_data`.
> 	 */
> 	pci_set_drvdata(pdev, ERR_PTR(err));
> 	return 0;
> }
> 
> So in case of an error they return 0 and do... stuff.
> 
> I don't want to touch that without someone who maintains (and, ideally,
> understands) the code details what's going on here.

Thanks for elaboration, indeed it was not enough context to see the full
picture. This seems like an ugly hack that has to be addressed at some point.
But again, not your fault.

-- 
With Best Regards,
Andy Shevchenko