| Message ID | 20240819165148.58201-6-pstanner@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | PCI: Remove pcim_iounmap_regions() | expand |
On Mon, Aug 19, 2024 at 06:51:44PM +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()"). > > In mtip32xx, these functions can easily be replaced by their respective > successors, pcim_request_region() and pcim_iomap(). Moreover, the > driver's call to pcim_iounmap_regions() is not necessary, because it's > invoked in the remove() function. Cleanup can, hence, be performed by > PCI devres automatically. > > Replace pcim_iomap_regions() and pcim_iomap_table(). > > Remove the call to pcim_iounmap_regions(). ... int mtip_pci_probe() > setmask_err: > - pcim_iounmap_regions(pdev, 1 << MTIP_ABAR); > + pcim_release_region(pdev, MTIP_ABAR); But why? ... mtip_pci_remove() > pci_disable_msi(pdev); > > - pcim_iounmap_regions(pdev, 1 << MTIP_ABAR); This is okay. ... > pci_set_drvdata(pdev, NULL); Side note: This is done by driver core for the last 10+ years…
On Mon, 2024-08-19 at 18:51 +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()"). > > In mtip32xx, these functions can easily be replaced by their > respective > successors, pcim_request_region() and pcim_iomap(). Moreover, the > driver's call to pcim_iounmap_regions() is not necessary, because > it's > invoked in the remove() function. Cleanup can, hence, be performed by > PCI devres automatically. > > Replace pcim_iomap_regions() and pcim_iomap_table(). > > Remove the call to pcim_iounmap_regions(). > > Signed-off-by: Philipp Stanner <pstanner@redhat.com> > --- > drivers/block/mtip32xx/mtip32xx.c | 11 ++++++----- > 1 file changed, 6 insertions(+), 5 deletions(-) > > diff --git a/drivers/block/mtip32xx/mtip32xx.c > b/drivers/block/mtip32xx/mtip32xx.c > index c6ef0546ffc9..c7da6090954e 100644 > --- a/drivers/block/mtip32xx/mtip32xx.c > +++ b/drivers/block/mtip32xx/mtip32xx.c > @@ -2716,7 +2716,9 @@ static int mtip_hw_init(struct driver_data *dd) > int rv; > unsigned long timeout, timetaken; > > - dd->mmio = pcim_iomap_table(dd->pdev)[MTIP_ABAR]; > + dd->mmio = pcim_iomap(dd->pdev, MTIP_ABAR, 0); > + if (!dd->mmio) > + return -ENOMEM; > > mtip_detect_product(dd); > if (dd->product_type == MTIP_PRODUCT_UNKNOWN) { > @@ -3726,9 +3728,9 @@ static int mtip_pci_probe(struct pci_dev *pdev, > } > > /* Map BAR5 to memory. */ > - rv = pcim_iomap_regions(pdev, 1 << MTIP_ABAR, > MTIP_DRV_NAME); > + rv = pcim_request_region(pdev, 1, MTIP_DRV_NAME); That's a bug here, btw. Should be MTIP_ABAR instead of 1. Will fix in v2. P. > if (rv < 0) { > - dev_err(&pdev->dev, "Unable to map regions\n"); > + dev_err(&pdev->dev, "Unable to request regions\n"); > goto iomap_err; > } > > @@ -3849,7 +3851,7 @@ static int mtip_pci_probe(struct pci_dev *pdev, > drop_cpu(dd->work[2].cpu_binding); > } > setmask_err: > - pcim_iounmap_regions(pdev, 1 << MTIP_ABAR); > + pcim_release_region(pdev, MTIP_ABAR); > > iomap_err: > kfree(dd); > @@ -3925,7 +3927,6 @@ static void mtip_pci_remove(struct pci_dev > *pdev) > > pci_disable_msi(pdev); > > - pcim_iounmap_regions(pdev, 1 << MTIP_ABAR); > pci_set_drvdata(pdev, NULL); > > put_disk(dd->disk);
On Mon, 2024-08-19 at 21:04 +0300, Andy Shevchenko wrote: > On Mon, Aug 19, 2024 at 06:51:44PM +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()"). > > > > In mtip32xx, these functions can easily be replaced by their > > respective > > successors, pcim_request_region() and pcim_iomap(). Moreover, the > > driver's call to pcim_iounmap_regions() is not necessary, because > > it's > > invoked in the remove() function. Cleanup can, hence, be performed > > by > > PCI devres automatically. > > > > Replace pcim_iomap_regions() and pcim_iomap_table(). > > > > Remove the call to pcim_iounmap_regions(). > > ... > > int mtip_pci_probe() > > > setmask_err: > > - pcim_iounmap_regions(pdev, 1 << MTIP_ABAR); > > + pcim_release_region(pdev, MTIP_ABAR); > > But why? EMOREINFOREQUIRED Why I replace it or why I don't remove it completely? > > ... > > mtip_pci_remove() > > > pci_disable_msi(pdev); > > > > - pcim_iounmap_regions(pdev, 1 << MTIP_ABAR); > > This is okay. Removing it is okay, you mean. > > ... > > > pci_set_drvdata(pdev, NULL); > > Side note: This is done by driver core for the last 10+ years… Ah you know Andy, kernel programmers be like: "When you're hunting you better make sure the wild sow is really dead before you load it in your trunk" ;p P. >
On Tue, Aug 20, 2024 at 09:29:52AM +0200, Philipp Stanner wrote: > On Mon, 2024-08-19 at 21:04 +0300, Andy Shevchenko wrote: > > On Mon, Aug 19, 2024 at 06:51:44PM +0200, Philipp Stanner wrote: ... > > int mtip_pci_probe() > > > > > setmask_err: > > > - pcim_iounmap_regions(pdev, 1 << MTIP_ABAR); > > > + pcim_release_region(pdev, MTIP_ABAR); > > > > But why? > > EMOREINFOREQUIRED > Why I replace it or why I don't remove it completely? The latter one: Why did you leave it and not remove? ... > > mtip_pci_remove() > > > > > pci_disable_msi(pdev); > > > > > > - pcim_iounmap_regions(pdev, 1 << MTIP_ABAR); > > > > This is okay. > > Removing it is okay, you mean. Yes! ... > > > pci_set_drvdata(pdev, NULL); > > > > Side note: This is done by driver core for the last 10+ years… > > Ah you know Andy, kernel programmers be like: "When you're hunting you > better make sure the wild sow is really dead before you load it in your > trunk" ;p Indeed, I had been told many times myself to improve / cleanup things unrelated to the working area before actually considering my little work... But, I specifically mark it as a "Side note:", so it's up to you to address or not.
diff --git a/drivers/block/mtip32xx/mtip32xx.c b/drivers/block/mtip32xx/mtip32xx.c index c6ef0546ffc9..c7da6090954e 100644 --- a/drivers/block/mtip32xx/mtip32xx.c +++ b/drivers/block/mtip32xx/mtip32xx.c @@ -2716,7 +2716,9 @@ static int mtip_hw_init(struct driver_data *dd) int rv; unsigned long timeout, timetaken; - dd->mmio = pcim_iomap_table(dd->pdev)[MTIP_ABAR]; + dd->mmio = pcim_iomap(dd->pdev, MTIP_ABAR, 0); + if (!dd->mmio) + return -ENOMEM; mtip_detect_product(dd); if (dd->product_type == MTIP_PRODUCT_UNKNOWN) { @@ -3726,9 +3728,9 @@ static int mtip_pci_probe(struct pci_dev *pdev, } /* Map BAR5 to memory. */ - rv = pcim_iomap_regions(pdev, 1 << MTIP_ABAR, MTIP_DRV_NAME); + rv = pcim_request_region(pdev, 1, MTIP_DRV_NAME); if (rv < 0) { - dev_err(&pdev->dev, "Unable to map regions\n"); + dev_err(&pdev->dev, "Unable to request regions\n"); goto iomap_err; } @@ -3849,7 +3851,7 @@ static int mtip_pci_probe(struct pci_dev *pdev, drop_cpu(dd->work[2].cpu_binding); } setmask_err: - pcim_iounmap_regions(pdev, 1 << MTIP_ABAR); + pcim_release_region(pdev, MTIP_ABAR); iomap_err: kfree(dd); @@ -3925,7 +3927,6 @@ static void mtip_pci_remove(struct pci_dev *pdev) pci_disable_msi(pdev); - pcim_iounmap_regions(pdev, 1 << MTIP_ABAR); pci_set_drvdata(pdev, NULL); put_disk(dd->disk);
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()"). In mtip32xx, these functions can easily be replaced by their respective successors, pcim_request_region() and pcim_iomap(). Moreover, the driver's call to pcim_iounmap_regions() is not necessary, because it's invoked in the remove() function. Cleanup can, hence, be performed by PCI devres automatically. Replace pcim_iomap_regions() and pcim_iomap_table(). Remove the call to pcim_iounmap_regions(). Signed-off-by: Philipp Stanner <pstanner@redhat.com> --- drivers/block/mtip32xx/mtip32xx.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-)