| Message ID | 20240819165148.58201-8-pstanner@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | PCI: Remove pcim_iounmap_regions() | expand |
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?
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.
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.
diff --git a/drivers/net/ethernet/cavium/common/cavium_ptp.c b/drivers/net/ethernet/cavium/common/cavium_ptp.c index 9fd717b9cf69..1849c62cde1d 100644 --- a/drivers/net/ethernet/cavium/common/cavium_ptp.c +++ b/drivers/net/ethernet/cavium/common/cavium_ptp.c @@ -239,11 +239,11 @@ static int cavium_ptp_probe(struct pci_dev *pdev, if (err) goto error_free; - err = pcim_iomap_regions(pdev, 1 << PCI_PTP_BAR_NO, pci_name(pdev)); - if (err) + clock->reg_base = pcim_iomap_region(pdev, PCI_PTP_BAR_NO, pci_name(pdev)); + if (IS_ERR(clock->reg_base)) { + err = PTR_ERR(clock->reg_base); goto error_free; - - clock->reg_base = pcim_iomap_table(pdev)[PCI_PTP_BAR_NO]; + } spin_lock_init(&clock->spin_lock); @@ -292,7 +292,7 @@ static int cavium_ptp_probe(struct pci_dev *pdev, clock_cfg = readq(clock->reg_base + PTP_CLOCK_CFG); clock_cfg &= ~PTP_CLOCK_CFG_PTP_EN; writeq(clock_cfg, clock->reg_base + PTP_CLOCK_CFG); - pcim_iounmap_regions(pdev, 1 << PCI_PTP_BAR_NO); + pcim_iounmap_region(pdev, PCI_PTP_BAR_NO); error_free: devm_kfree(dev, clock);
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(). Signed-off-by: Philipp Stanner <pstanner@redhat.com> --- drivers/net/ethernet/cavium/common/cavium_ptp.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)