| Message ID | 20241025145959.185373-3-pstanner@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Remove pcim_iomap_regions_request_all() | expand |
On Fri, 25 Oct 2024, Philipp Stanner wrote: > pcim_iomap_regions_request_all() 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 their successors, pcim_iomap() and > pcim_request_all_regions(). > > Signed-off-by: Philipp Stanner <pstanner@redhat.com> > Acked-by: Damien Le Moal <dlemoal@kernel.org> > --- > drivers/ata/acard-ahci.c | 6 ++++-- > drivers/ata/ahci.c | 6 ++++-- > 2 files changed, 8 insertions(+), 4 deletions(-) > > diff --git a/drivers/ata/acard-ahci.c b/drivers/ata/acard-ahci.c > index 547f56341705..3999305b5356 100644 > --- a/drivers/ata/acard-ahci.c > +++ b/drivers/ata/acard-ahci.c > @@ -370,7 +370,7 @@ static int acard_ahci_init_one(struct pci_dev *pdev, const struct pci_device_id > /* AHCI controllers often implement SFF compatible interface. > * Grab all PCI BARs just in case. > */ > - rc = pcim_iomap_regions_request_all(pdev, 1 << AHCI_PCI_BAR, DRV_NAME); > + rc = pcim_request_all_regions(pdev, DRV_NAME); > if (rc == -EBUSY) > pcim_pin_device(pdev); > if (rc) > @@ -386,7 +386,9 @@ static int acard_ahci_init_one(struct pci_dev *pdev, const struct pci_device_id > if (!(hpriv->flags & AHCI_HFLAG_NO_MSI)) > pci_enable_msi(pdev); > > - hpriv->mmio = pcim_iomap_table(pdev)[AHCI_PCI_BAR]; > + hpriv->mmio = pcim_iomap(pdev, AHCI_PCI_BAR, 0); > + if (!hpriv->mmio) > + return -ENOMEM; > > /* save initial config */ > ahci_save_initial_config(&pdev->dev, hpriv); > diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c > index 45f63b09828a..2043dfb52ae8 100644 > --- a/drivers/ata/ahci.c > +++ b/drivers/ata/ahci.c > @@ -1869,7 +1869,7 @@ static int ahci_init_one(struct pci_dev *pdev, const struct pci_device_id *ent) > /* AHCI controllers often implement SFF compatible interface. > * Grab all PCI BARs just in case. > */ > - rc = pcim_iomap_regions_request_all(pdev, 1 << ahci_pci_bar, DRV_NAME); > + rc = pcim_request_all_regions(pdev, DRV_NAME); > if (rc == -EBUSY) > pcim_pin_device(pdev); > if (rc) > @@ -1893,7 +1893,9 @@ static int ahci_init_one(struct pci_dev *pdev, const struct pci_device_id *ent) > if (ahci_sb600_enable_64bit(pdev)) > hpriv->flags &= ~AHCI_HFLAG_32BIT_ONLY; > > - hpriv->mmio = pcim_iomap_table(pdev)[ahci_pci_bar]; > + hpriv->mmio = pcim_iomap(pdev, ahci_pci_bar, 0); > + if (!hpriv->mmio) > + return -ENOMEM; Hi, I've probably lost the big picture somewhere and the coverletter wasn't helpful focusing only the most immediate goal of getting rid of the deprecated function. These seem to only pcim_iomap() a single BAR. So my question is, what is the reason for using pcim_request_all_regions() and not pcim_request_region() as mentioned in the commit message of the commit e354bb84a4c1 ("PCI: Deprecate pcim_iomap_table(), pcim_iomap_regions_request_all()")? I understand it's strictly not wrong to use pcim_request_all_regions() but I'm just trying to understand the logic behind the selection. I'm sorry if this is a stupid question, it's just what I couldn't figure out on my own while trying to review these patches. (I admit not reading all the related discussions in the earlier versions.)
On Fri, 2024-10-25 at 18:55 +0300, Ilpo Järvinen wrote: > On Fri, 25 Oct 2024, Philipp Stanner wrote: > > > pcim_iomap_regions_request_all() 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 their successors, pcim_iomap() and > > pcim_request_all_regions(). > > > > Signed-off-by: Philipp Stanner <pstanner@redhat.com> > > Acked-by: Damien Le Moal <dlemoal@kernel.org> > > --- > > drivers/ata/acard-ahci.c | 6 ++++-- > > drivers/ata/ahci.c | 6 ++++-- > > 2 files changed, 8 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/ata/acard-ahci.c b/drivers/ata/acard-ahci.c > > index 547f56341705..3999305b5356 100644 > > --- a/drivers/ata/acard-ahci.c > > +++ b/drivers/ata/acard-ahci.c > > @@ -370,7 +370,7 @@ static int acard_ahci_init_one(struct pci_dev > > *pdev, const struct pci_device_id > > /* AHCI controllers often implement SFF compatible > > interface. > > * Grab all PCI BARs just in case. > > */ > > - rc = pcim_iomap_regions_request_all(pdev, 1 << > > AHCI_PCI_BAR, DRV_NAME); > > + rc = pcim_request_all_regions(pdev, DRV_NAME); > > if (rc == -EBUSY) > > pcim_pin_device(pdev); > > if (rc) > > @@ -386,7 +386,9 @@ static int acard_ahci_init_one(struct pci_dev > > *pdev, const struct pci_device_id > > if (!(hpriv->flags & AHCI_HFLAG_NO_MSI)) > > pci_enable_msi(pdev); > > > > - hpriv->mmio = pcim_iomap_table(pdev)[AHCI_PCI_BAR]; > > + hpriv->mmio = pcim_iomap(pdev, AHCI_PCI_BAR, 0); > > + if (!hpriv->mmio) > > + return -ENOMEM; > > > > /* save initial config */ > > ahci_save_initial_config(&pdev->dev, hpriv); > > diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c > > index 45f63b09828a..2043dfb52ae8 100644 > > --- a/drivers/ata/ahci.c > > +++ b/drivers/ata/ahci.c > > @@ -1869,7 +1869,7 @@ static int ahci_init_one(struct pci_dev > > *pdev, const struct pci_device_id *ent) > > /* AHCI controllers often implement SFF compatible > > interface. > > * Grab all PCI BARs just in case. > > */ > > - rc = pcim_iomap_regions_request_all(pdev, 1 << > > ahci_pci_bar, DRV_NAME); > > + rc = pcim_request_all_regions(pdev, DRV_NAME); > > if (rc == -EBUSY) > > pcim_pin_device(pdev); > > if (rc) > > @@ -1893,7 +1893,9 @@ static int ahci_init_one(struct pci_dev > > *pdev, const struct pci_device_id *ent) > > if (ahci_sb600_enable_64bit(pdev)) > > hpriv->flags &= ~AHCI_HFLAG_32BIT_ONLY; > > > > - hpriv->mmio = pcim_iomap_table(pdev)[ahci_pci_bar]; > > + hpriv->mmio = pcim_iomap(pdev, ahci_pci_bar, 0); > > + if (!hpriv->mmio) > > + return -ENOMEM; > > Hi, > > I've probably lost the big picture somewhere and the coverletter > wasn't > helpful focusing only the most immediate goal of getting rid of the > deprecated function. > > These seem to only pcim_iomap() a single BAR. So my question is, what > is > the reason for using pcim_request_all_regions() and not > pcim_request_region() as mentioned in the commit message of the > commit > e354bb84a4c1 ("PCI: Deprecate pcim_iomap_table(), > pcim_iomap_regions_request_all()")? That commit message isn't that precise and / or was written when pcim_request_all_regions() was still an internal helper function. > > I understand it's strictly not wrong to use > pcim_request_all_regions() > but I'm just trying to understand the logic behind the selection. > I'm sorry if this is a stupid question, it's just what I couldn't > figure > out on my own while trying to review these patches. > The reason pcim_request_all_regions() is used in the entire series is to keep behavior of the drivers 100% identical. pcim_iomap_regions_request_all() performs a region request on *all* PCI BARs and then ioremap()s *specific* ones; namely those set by the barmask. It seems to me that those drivers were only using pcim_iomap_regions_request_all() precisely because of that feature: they want to reserve all BARs through a region request. You could do that manually with for (int i = 0; i < PCI_STD_NUM_BARS; i++) pcim_request_region(); mem = pcim_iomap(...); When you look at Patch #10 you'll see the implementation of pcim_iomap_regions_request_all() and will discover that it itself uses pcim_request_all_regions(). So you could consider this series a partial code-move that handily also gets rid of a complicated function that prevents us from removing, ultimately, the problematic function pcim_iomap_table(). Hope this helps, P. > (I admit not reading all the related discussions in the earlier > versions.) >
diff --git a/drivers/ata/acard-ahci.c b/drivers/ata/acard-ahci.c index 547f56341705..3999305b5356 100644 --- a/drivers/ata/acard-ahci.c +++ b/drivers/ata/acard-ahci.c @@ -370,7 +370,7 @@ static int acard_ahci_init_one(struct pci_dev *pdev, const struct pci_device_id /* AHCI controllers often implement SFF compatible interface. * Grab all PCI BARs just in case. */ - rc = pcim_iomap_regions_request_all(pdev, 1 << AHCI_PCI_BAR, DRV_NAME); + rc = pcim_request_all_regions(pdev, DRV_NAME); if (rc == -EBUSY) pcim_pin_device(pdev); if (rc) @@ -386,7 +386,9 @@ static int acard_ahci_init_one(struct pci_dev *pdev, const struct pci_device_id if (!(hpriv->flags & AHCI_HFLAG_NO_MSI)) pci_enable_msi(pdev); - hpriv->mmio = pcim_iomap_table(pdev)[AHCI_PCI_BAR]; + hpriv->mmio = pcim_iomap(pdev, AHCI_PCI_BAR, 0); + if (!hpriv->mmio) + return -ENOMEM; /* save initial config */ ahci_save_initial_config(&pdev->dev, hpriv); diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c index 45f63b09828a..2043dfb52ae8 100644 --- a/drivers/ata/ahci.c +++ b/drivers/ata/ahci.c @@ -1869,7 +1869,7 @@ static int ahci_init_one(struct pci_dev *pdev, const struct pci_device_id *ent) /* AHCI controllers often implement SFF compatible interface. * Grab all PCI BARs just in case. */ - rc = pcim_iomap_regions_request_all(pdev, 1 << ahci_pci_bar, DRV_NAME); + rc = pcim_request_all_regions(pdev, DRV_NAME); if (rc == -EBUSY) pcim_pin_device(pdev); if (rc) @@ -1893,7 +1893,9 @@ static int ahci_init_one(struct pci_dev *pdev, const struct pci_device_id *ent) if (ahci_sb600_enable_64bit(pdev)) hpriv->flags &= ~AHCI_HFLAG_32BIT_ONLY; - hpriv->mmio = pcim_iomap_table(pdev)[ahci_pci_bar]; + hpriv->mmio = pcim_iomap(pdev, ahci_pci_bar, 0); + if (!hpriv->mmio) + return -ENOMEM; /* detect remapped nvme devices */ ahci_remap_check(pdev, ahci_pci_bar, hpriv);
pcim_iomap_regions_request_all() 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 their successors, pcim_iomap() and pcim_request_all_regions(). Signed-off-by: Philipp Stanner <pstanner@redhat.com> Acked-by: Damien Le Moal <dlemoal@kernel.org> --- drivers/ata/acard-ahci.c | 6 ++++-- drivers/ata/ahci.c | 6 ++++-- 2 files changed, 8 insertions(+), 4 deletions(-)