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:
> > pcim_iomap_regions() and pcim_iomap_table() have been deprecated by
> > the PCI subsystem in commit e354bb84a4c1 ("PCI: Deprecate
> > pcim_iomap_table(), pcim_iomap_regions_request_all()").
> >
> > Replace these functions with the function pcim_iomap_region().
>
> ...
>
> 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. 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.
P.
