[PATCH 00/10] Remove pcim_iomap_regions_request_all()

[PATCH 00/10] Remove pcim_iomap_regions_request_all()

[Philipp Stanner]

All Acked-by's are in place now.

Changes in v5:
  - Add Acked-by's from Alexander and Bharat (the latter sent off-list,
    because of some issue with receiving the previous patch sets).

Changes in v4:
  - Add Acked-by's from Giovanni and Kalle.

Changes in v3:
  - Add missing full stops to commit messages (Andy).

Changes in v2:
  - Fix a bug in patch №4 ("crypto: marvell ...") where an error code
    was not set before printing it. (Me)
  - Apply Damien's Reviewed- / Acked-by to patches 1, 2 and 10. (Damien)
  - Apply Serge's Acked-by to patch №7. (Serge)
  - Apply Jiri's Reviewed-by to patch №8. (Jiri)
  - Apply Takashi Iwai's Reviewed-by to patch №9. (Takashi)


Hi all,

the PCI subsystem is currently working on cleaning up its devres API. To
do so, a few functions will be replaced with better alternatives.

This series removes pcim_iomap_regions_request_all(), which has been
deprecated already, and accordingly replaces the calls to
pcim_iomap_table() (which were only necessary because of
pcim_iomap_regions_request_all() in the first place) with calls to
pcim_iomap().

Would be great if you can take a look whether this behaves as you
intended for your respective component.

Cheers,
Philipp

Philipp Stanner (10):
  PCI: Make pcim_request_all_regions() a public function
  ata: ahci: Replace deprecated PCI functions
  crypto: qat - replace deprecated PCI functions
  crypto: marvell - replace deprecated PCI functions
  intel_th: pci: Replace deprecated PCI functions
  wifi: iwlwifi: replace deprecated PCI functions
  ntb: idt: Replace deprecated PCI functions
  serial: rp2: Replace deprecated PCI functions
  ALSA: korg1212: Replace deprecated PCI functions
  PCI: Remove pcim_iomap_regions_request_all()

 .../driver-api/driver-model/devres.rst        |  1 -
 drivers/ata/acard-ahci.c                      |  6 +-
 drivers/ata/ahci.c                            |  6 +-
 drivers/crypto/intel/qat/qat_420xx/adf_drv.c  | 11 +++-
 drivers/crypto/intel/qat/qat_4xxx/adf_drv.c   | 11 +++-
 .../marvell/octeontx2/otx2_cptpf_main.c       | 14 +++--
 .../marvell/octeontx2/otx2_cptvf_main.c       | 13 ++--
 drivers/hwtracing/intel_th/pci.c              |  9 ++-
 .../net/wireless/intel/iwlwifi/pcie/trans.c   | 16 ++---
 drivers/ntb/hw/idt/ntb_hw_idt.c               | 13 ++--
 drivers/pci/devres.c                          | 59 +------------------
 drivers/tty/serial/rp2.c                      | 12 ++--
 include/linux/pci.h                           |  3 +-
 sound/pci/korg1212/korg1212.c                 |  6 +-
 14 files changed, 76 insertions(+), 104 deletions(-)

-- 
2.47.0

[PATCH 01/10] PCI: Make pcim_request_all_regions() a public function

[Philipp Stanner]

In order to remove the deprecated function
pcim_iomap_regions_request_all(), a few drivers need an interface to
request all BARs a PCI-Device offers.

Make pcim_request_all_regions() a public interface.

Signed-off-by: Philipp Stanner <pstanner@redhat.com>
Reviewed-by: Damien Le Moal <dlemoal@kernel.org>
---
 drivers/pci/devres.c | 3 ++-
 include/linux/pci.h  | 1 +
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/devres.c b/drivers/pci/devres.c
index b133967faef8..2a64da5c91fb 100644
--- a/drivers/pci/devres.c
+++ b/drivers/pci/devres.c
@@ -939,7 +939,7 @@ static void pcim_release_all_regions(struct pci_dev *pdev)
  * desired, release individual regions with pcim_release_region() or all of
  * them at once with pcim_release_all_regions().
  */
-static int pcim_request_all_regions(struct pci_dev *pdev, const char *name)
+int pcim_request_all_regions(struct pci_dev *pdev, const char *name)
 {
 	int ret;
 	int bar;
@@ -957,6 +957,7 @@ static int pcim_request_all_regions(struct pci_dev *pdev, const char *name)
 
 	return ret;
 }
+EXPORT_SYMBOL(pcim_request_all_regions);
 
 /**
  * pcim_iomap_regions_request_all - Request all BARs and iomap specified ones
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 573b4c4c2be6..3b151c8331e5 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -2293,6 +2293,7 @@ static inline void pci_fixup_device(enum pci_fixup_pass pass,
 				    struct pci_dev *dev) { }
 #endif
 
+int pcim_request_all_regions(struct pci_dev *pdev, const char *name);
 void __iomem *pcim_iomap(struct pci_dev *pdev, int bar, unsigned long maxlen);
 void __iomem *pcim_iomap_region(struct pci_dev *pdev, int bar,
 				const char *name);
-- 
2.47.0

Re: [PATCH 01/10] PCI: Make pcim_request_all_regions() a public function

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 2066 bytes --]

On Fri, 25 Oct 2024, Philipp Stanner wrote:

> In order to remove the deprecated function
> pcim_iomap_regions_request_all(), a few drivers need an interface to
> request all BARs a PCI-Device offers.

Hi Philipp,

I don't know why you put that dash there, it looks a bit odd. Other than 
that,

Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

--
 i.

> Make pcim_request_all_regions() a public interface.
> 
> Signed-off-by: Philipp Stanner <pstanner@redhat.com>
> Reviewed-by: Damien Le Moal <dlemoal@kernel.org>
> ---
>  drivers/pci/devres.c | 3 ++-
>  include/linux/pci.h  | 1 +
>  2 files changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/devres.c b/drivers/pci/devres.c
> index b133967faef8..2a64da5c91fb 100644
> --- a/drivers/pci/devres.c
> +++ b/drivers/pci/devres.c
> @@ -939,7 +939,7 @@ static void pcim_release_all_regions(struct pci_dev *pdev)
>   * desired, release individual regions with pcim_release_region() or all of
>   * them at once with pcim_release_all_regions().
>   */
> -static int pcim_request_all_regions(struct pci_dev *pdev, const char *name)
> +int pcim_request_all_regions(struct pci_dev *pdev, const char *name)
>  {
>  	int ret;
>  	int bar;
> @@ -957,6 +957,7 @@ static int pcim_request_all_regions(struct pci_dev *pdev, const char *name)
>  
>  	return ret;
>  }
> +EXPORT_SYMBOL(pcim_request_all_regions);
>  
>  /**
>   * pcim_iomap_regions_request_all - Request all BARs and iomap specified ones
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 573b4c4c2be6..3b151c8331e5 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -2293,6 +2293,7 @@ static inline void pci_fixup_device(enum pci_fixup_pass pass,
>  				    struct pci_dev *dev) { }
>  #endif
>  
> +int pcim_request_all_regions(struct pci_dev *pdev, const char *name);
>  void __iomem *pcim_iomap(struct pci_dev *pdev, int bar, unsigned long maxlen);
>  void __iomem *pcim_iomap_region(struct pci_dev *pdev, int bar,
>  				const char *name);
> 

[PATCH 02/10] ata: ahci: Replace deprecated PCI functions

[Philipp Stanner]

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;
 
 	/* detect remapped nvme devices */
 	ahci_remap_check(pdev, ahci_pci_bar, hpriv);
-- 
2.47.0

Re: [PATCH 02/10] ata: ahci: Replace deprecated PCI functions

[Ilpo Järvinen]

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.)

-- 
 i.

Re: [PATCH 02/10] ata: ahci: Replace deprecated PCI functions

[Philipp Stanner]

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.)
> 

[PATCH 03/10] crypto: qat - replace deprecated PCI functions

[Philipp Stanner]

pcim_iomap_table() and pcim_iomap_regions_request_all() 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: Giovanni Cabiddu <giovanni.cabiddu@intel.com>
---
 drivers/crypto/intel/qat/qat_420xx/adf_drv.c | 11 ++++++++---
 drivers/crypto/intel/qat/qat_4xxx/adf_drv.c  | 11 ++++++++---
 2 files changed, 16 insertions(+), 6 deletions(-)

diff --git a/drivers/crypto/intel/qat/qat_420xx/adf_drv.c b/drivers/crypto/intel/qat/qat_420xx/adf_drv.c
index f49818a13013..788a11cdb34b 100644
--- a/drivers/crypto/intel/qat/qat_420xx/adf_drv.c
+++ b/drivers/crypto/intel/qat/qat_420xx/adf_drv.c
@@ -129,16 +129,21 @@ static int adf_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 	/* Find and map all the device's BARS */
 	bar_mask = pci_select_bars(pdev, IORESOURCE_MEM) & ADF_GEN4_BAR_MASK;
 
-	ret = pcim_iomap_regions_request_all(pdev, bar_mask, pci_name(pdev));
+	ret = pcim_request_all_regions(pdev, pci_name(pdev));
 	if (ret) {
-		dev_err(&pdev->dev, "Failed to map pci regions.\n");
+		dev_err(&pdev->dev, "Failed to request PCI regions.\n");
 		goto out_err;
 	}
 
 	i = 0;
 	for_each_set_bit(bar_nr, &bar_mask, PCI_STD_NUM_BARS) {
 		bar = &accel_pci_dev->pci_bars[i++];
-		bar->virt_addr = pcim_iomap_table(pdev)[bar_nr];
+		bar->virt_addr = pcim_iomap(pdev, bar_nr, 0);
+		if (!bar->virt_addr) {
+			dev_err(&pdev->dev, "Failed to ioremap PCI region.\n");
+			ret = -ENOMEM;
+			goto out_err;
+		}
 	}
 
 	pci_set_master(pdev);
diff --git a/drivers/crypto/intel/qat/qat_4xxx/adf_drv.c b/drivers/crypto/intel/qat/qat_4xxx/adf_drv.c
index 659905e45950..115eabfd1f6b 100644
--- a/drivers/crypto/intel/qat/qat_4xxx/adf_drv.c
+++ b/drivers/crypto/intel/qat/qat_4xxx/adf_drv.c
@@ -131,16 +131,21 @@ static int adf_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 	/* Find and map all the device's BARS */
 	bar_mask = pci_select_bars(pdev, IORESOURCE_MEM) & ADF_GEN4_BAR_MASK;
 
-	ret = pcim_iomap_regions_request_all(pdev, bar_mask, pci_name(pdev));
+	ret = pcim_request_all_regions(pdev, pci_name(pdev));
 	if (ret) {
-		dev_err(&pdev->dev, "Failed to map pci regions.\n");
+		dev_err(&pdev->dev, "Failed to request PCI regions.\n");
 		goto out_err;
 	}
 
 	i = 0;
 	for_each_set_bit(bar_nr, &bar_mask, PCI_STD_NUM_BARS) {
 		bar = &accel_pci_dev->pci_bars[i++];
-		bar->virt_addr = pcim_iomap_table(pdev)[bar_nr];
+		bar->virt_addr = pcim_iomap(pdev, bar_nr, 0);
+		if (!bar->virt_addr) {
+			dev_err(&pdev->dev, "Failed to ioremap PCI region.\n");
+			ret = -ENOMEM;
+			goto out_err;
+		}
 	}
 
 	pci_set_master(pdev);
-- 
2.47.0

[PATCH 04/10] crypto: marvell - replace deprecated PCI functions

[Philipp Stanner]

pcim_iomap_table() and pcim_iomap_regions_request_all() 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: Bharat Bhushan <bbhushan2@marvell.com>
---
 drivers/crypto/marvell/octeontx2/otx2_cptpf_main.c | 14 +++++++++-----
 drivers/crypto/marvell/octeontx2/otx2_cptvf_main.c | 13 +++++++++----
 2 files changed, 18 insertions(+), 9 deletions(-)

diff --git a/drivers/crypto/marvell/octeontx2/otx2_cptpf_main.c b/drivers/crypto/marvell/octeontx2/otx2_cptpf_main.c
index 400e36d9908f..94d0e73e42de 100644
--- a/drivers/crypto/marvell/octeontx2/otx2_cptpf_main.c
+++ b/drivers/crypto/marvell/octeontx2/otx2_cptpf_main.c
@@ -739,18 +739,22 @@ static int otx2_cptpf_probe(struct pci_dev *pdev,
 		dev_err(dev, "Unable to get usable DMA configuration\n");
 		goto clear_drvdata;
 	}
-	/* Map PF's configuration registers */
-	err = pcim_iomap_regions_request_all(pdev, 1 << PCI_PF_REG_BAR_NUM,
-					     OTX2_CPT_DRV_NAME);
+	err = pcim_request_all_regions(pdev, OTX2_CPT_DRV_NAME);
 	if (err) {
-		dev_err(dev, "Couldn't get PCI resources 0x%x\n", err);
+		dev_err(dev, "Couldn't request PCI resources 0x%x\n", err);
 		goto clear_drvdata;
 	}
 	pci_set_master(pdev);
 	pci_set_drvdata(pdev, cptpf);
 	cptpf->pdev = pdev;
 
-	cptpf->reg_base = pcim_iomap_table(pdev)[PCI_PF_REG_BAR_NUM];
+	/* Map PF's configuration registers */
+	cptpf->reg_base = pcim_iomap(pdev, PCI_PF_REG_BAR_NUM, 0);
+	if (!cptpf->reg_base) {
+		err = -ENOMEM;
+		dev_err(dev, "Couldn't ioremap PCI resource 0x%x\n", err);
+		goto clear_drvdata;
+	}
 
 	/* Check if AF driver is up, otherwise defer probe */
 	err = cpt_is_pf_usable(cptpf);
diff --git a/drivers/crypto/marvell/octeontx2/otx2_cptvf_main.c b/drivers/crypto/marvell/octeontx2/otx2_cptvf_main.c
index 527d34cc258b..d0b6ee901f62 100644
--- a/drivers/crypto/marvell/octeontx2/otx2_cptvf_main.c
+++ b/drivers/crypto/marvell/octeontx2/otx2_cptvf_main.c
@@ -358,9 +358,8 @@ static int otx2_cptvf_probe(struct pci_dev *pdev,
 		dev_err(dev, "Unable to get usable DMA configuration\n");
 		goto clear_drvdata;
 	}
-	/* Map VF's configuration registers */
-	ret = pcim_iomap_regions_request_all(pdev, 1 << PCI_PF_REG_BAR_NUM,
-					     OTX2_CPTVF_DRV_NAME);
+
+	ret = pcim_request_all_regions(pdev, OTX2_CPTVF_DRV_NAME);
 	if (ret) {
 		dev_err(dev, "Couldn't get PCI resources 0x%x\n", ret);
 		goto clear_drvdata;
@@ -369,7 +368,13 @@ static int otx2_cptvf_probe(struct pci_dev *pdev,
 	pci_set_drvdata(pdev, cptvf);
 	cptvf->pdev = pdev;
 
-	cptvf->reg_base = pcim_iomap_table(pdev)[PCI_PF_REG_BAR_NUM];
+	/* Map VF's configuration registers */
+	cptvf->reg_base = pcim_iomap(pdev, PCI_PF_REG_BAR_NUM, 0);
+	if (!cptvf->reg_base) {
+		ret = -ENOMEM;
+		dev_err(dev, "Couldn't ioremap PCI resource 0x%x\n", ret);
+		goto clear_drvdata;
+	}
 
 	otx2_cpt_set_hw_caps(pdev, &cptvf->cap_flag);
 
-- 
2.47.0

[PATCH 05/10] intel_th: pci: Replace deprecated PCI functions

[Philipp Stanner]

pcim_iomap_table() and pcim_iomap_regions_request_all() 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: Alexander Shishkin <alexander.shishkin@linux.intel.com>
---
 drivers/hwtracing/intel_th/pci.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/hwtracing/intel_th/pci.c b/drivers/hwtracing/intel_th/pci.c
index 0d7b9839e5b6..e9d8d28e055f 100644
--- a/drivers/hwtracing/intel_th/pci.c
+++ b/drivers/hwtracing/intel_th/pci.c
@@ -23,7 +23,6 @@ enum {
 	TH_PCI_RTIT_BAR		= 4,
 };
 
-#define BAR_MASK (BIT(TH_PCI_CONFIG_BAR) | BIT(TH_PCI_STH_SW_BAR))
 
 #define PCI_REG_NPKDSC	0x80
 #define NPKDSC_TSACT	BIT(5)
@@ -83,10 +82,16 @@ static int intel_th_pci_probe(struct pci_dev *pdev,
 	if (err)
 		return err;
 
-	err = pcim_iomap_regions_request_all(pdev, BAR_MASK, DRIVER_NAME);
+	err = pcim_request_all_regions(pdev, DRIVER_NAME);
 	if (err)
 		return err;
 
+	if (!pcim_iomap(pdev, TH_PCI_CONFIG_BAR, 0))
+		return -ENOMEM;
+
+	if (!pcim_iomap(pdev, TH_PCI_STH_SW_BAR, 0))
+		return -ENOMEM;
+
 	if (pdev->resource[TH_PCI_RTIT_BAR].start) {
 		resource[TH_MMIO_RTIT] = pdev->resource[TH_PCI_RTIT_BAR];
 		r++;
-- 
2.47.0

[PATCH 06/10] wifi: iwlwifi: replace deprecated PCI functions

[Philipp Stanner]

pcim_iomap_table() and pcim_iomap_regions_request_all() 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: Kalle Valo <kvalo@kernel.org>
---
 drivers/net/wireless/intel/iwlwifi/pcie/trans.c | 16 ++++------------
 1 file changed, 4 insertions(+), 12 deletions(-)

diff --git a/drivers/net/wireless/intel/iwlwifi/pcie/trans.c b/drivers/net/wireless/intel/iwlwifi/pcie/trans.c
index 3b9943eb6934..4b41613ad89d 100644
--- a/drivers/net/wireless/intel/iwlwifi/pcie/trans.c
+++ b/drivers/net/wireless/intel/iwlwifi/pcie/trans.c
@@ -3533,7 +3533,6 @@ struct iwl_trans *iwl_trans_pcie_alloc(struct pci_dev *pdev,
 	struct iwl_trans_pcie *trans_pcie, **priv;
 	struct iwl_trans *trans;
 	int ret, addr_size;
-	void __iomem * const *table;
 	u32 bar0;
 
 	/* reassign our BAR 0 if invalid due to possible runtime PM races */
@@ -3659,22 +3658,15 @@ struct iwl_trans *iwl_trans_pcie_alloc(struct pci_dev *pdev,
 		}
 	}
 
-	ret = pcim_iomap_regions_request_all(pdev, BIT(0), DRV_NAME);
+	ret = pcim_request_all_regions(pdev, DRV_NAME);
 	if (ret) {
-		dev_err(&pdev->dev, "pcim_iomap_regions_request_all failed\n");
+		dev_err(&pdev->dev, "pcim_request_all_regions failed\n");
 		goto out_no_pci;
 	}
 
-	table = pcim_iomap_table(pdev);
-	if (!table) {
-		dev_err(&pdev->dev, "pcim_iomap_table failed\n");
-		ret = -ENOMEM;
-		goto out_no_pci;
-	}
-
-	trans_pcie->hw_base = table[0];
+	trans_pcie->hw_base = pcim_iomap(pdev, 0, 0);
 	if (!trans_pcie->hw_base) {
-		dev_err(&pdev->dev, "couldn't find IO mem in first BAR\n");
+		dev_err(&pdev->dev, "pcim_iomap failed\n");
 		ret = -ENODEV;
 		goto out_no_pci;
 	}
-- 
2.47.0

Re: [PATCH 06/10] wifi: iwlwifi: replace deprecated PCI functions

[Ilpo Järvinen]

On Fri, 25 Oct 2024, Philipp Stanner wrote:

> pcim_iomap_table() and pcim_iomap_regions_request_all() 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: Kalle Valo <kvalo@kernel.org>
> ---
>  drivers/net/wireless/intel/iwlwifi/pcie/trans.c | 16 ++++------------
>  1 file changed, 4 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/net/wireless/intel/iwlwifi/pcie/trans.c b/drivers/net/wireless/intel/iwlwifi/pcie/trans.c
> index 3b9943eb6934..4b41613ad89d 100644
> --- a/drivers/net/wireless/intel/iwlwifi/pcie/trans.c
> +++ b/drivers/net/wireless/intel/iwlwifi/pcie/trans.c
> @@ -3533,7 +3533,6 @@ struct iwl_trans *iwl_trans_pcie_alloc(struct pci_dev *pdev,
>  	struct iwl_trans_pcie *trans_pcie, **priv;
>  	struct iwl_trans *trans;
>  	int ret, addr_size;
> -	void __iomem * const *table;
>  	u32 bar0;
>  
>  	/* reassign our BAR 0 if invalid due to possible runtime PM races */
> @@ -3659,22 +3658,15 @@ struct iwl_trans *iwl_trans_pcie_alloc(struct pci_dev *pdev,
>  		}
>  	}
>  
> -	ret = pcim_iomap_regions_request_all(pdev, BIT(0), DRV_NAME);
> +	ret = pcim_request_all_regions(pdev, DRV_NAME);
>  	if (ret) {
> -		dev_err(&pdev->dev, "pcim_iomap_regions_request_all failed\n");
> +		dev_err(&pdev->dev, "pcim_request_all_regions failed\n");
>  		goto out_no_pci;
>  	}
>  
> -	table = pcim_iomap_table(pdev);
> -	if (!table) {
> -		dev_err(&pdev->dev, "pcim_iomap_table failed\n");
> -		ret = -ENOMEM;
> -		goto out_no_pci;
> -	}
> -
> -	trans_pcie->hw_base = table[0];
> +	trans_pcie->hw_base = pcim_iomap(pdev, 0, 0);
>  	if (!trans_pcie->hw_base) {
> -		dev_err(&pdev->dev, "couldn't find IO mem in first BAR\n");
> +		dev_err(&pdev->dev, "pcim_iomap failed\n");

This seems a step backwards as a human readable English error message was 
replaced with a reference to a function name.

-- 
 i.

>  		ret = -ENODEV;
>  		goto out_no_pci;
>  	}
> 

Re: [PATCH 06/10] wifi: iwlwifi: replace deprecated PCI functions

[Philipp Stanner]

On Fri, 2024-10-25 at 18:31 +0300, Ilpo Järvinen wrote:
> On Fri, 25 Oct 2024, Philipp Stanner wrote:
> 
> > pcim_iomap_table() and pcim_iomap_regions_request_all() 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: Kalle Valo <kvalo@kernel.org>
> > ---
> >  drivers/net/wireless/intel/iwlwifi/pcie/trans.c | 16 ++++---------
> > ---
> >  1 file changed, 4 insertions(+), 12 deletions(-)
> > 
> > diff --git a/drivers/net/wireless/intel/iwlwifi/pcie/trans.c
> > b/drivers/net/wireless/intel/iwlwifi/pcie/trans.c
> > index 3b9943eb6934..4b41613ad89d 100644
> > --- a/drivers/net/wireless/intel/iwlwifi/pcie/trans.c
> > +++ b/drivers/net/wireless/intel/iwlwifi/pcie/trans.c
> > @@ -3533,7 +3533,6 @@ struct iwl_trans *iwl_trans_pcie_alloc(struct
> > pci_dev *pdev,
> >  	struct iwl_trans_pcie *trans_pcie, **priv;
> >  	struct iwl_trans *trans;
> >  	int ret, addr_size;
> > -	void __iomem * const *table;
> >  	u32 bar0;
> >  
> >  	/* reassign our BAR 0 if invalid due to possible runtime
> > PM races */
> > @@ -3659,22 +3658,15 @@ struct iwl_trans
> > *iwl_trans_pcie_alloc(struct pci_dev *pdev,
> >  		}
> >  	}
> >  
> > -	ret = pcim_iomap_regions_request_all(pdev, BIT(0),
> > DRV_NAME);
> > +	ret = pcim_request_all_regions(pdev, DRV_NAME);
> >  	if (ret) {
> > -		dev_err(&pdev->dev,
> > "pcim_iomap_regions_request_all failed\n");
> > +		dev_err(&pdev->dev, "pcim_request_all_regions
> > failed\n");
> >  		goto out_no_pci;
> >  	}
> >  
> > -	table = pcim_iomap_table(pdev);
> > -	if (!table) {
> > -		dev_err(&pdev->dev, "pcim_iomap_table failed\n");
> > -		ret = -ENOMEM;
> > -		goto out_no_pci;
> > -	}
> > -
> > -	trans_pcie->hw_base = table[0];
> > +	trans_pcie->hw_base = pcim_iomap(pdev, 0, 0);
> >  	if (!trans_pcie->hw_base) {
> > -		dev_err(&pdev->dev, "couldn't find IO mem in first
> > BAR\n");
> > +		dev_err(&pdev->dev, "pcim_iomap failed\n");
> 
> This seems a step backwards as a human readable English error message
> was 
> replaced with a reference to a function name.

I think it's still an improvement because "couldn't find IO mem in
first BAR" is a nonsensical statement. What the author probably meant
was: "Couldn't find first BAR's IO mem in magic pci_iomap_table" ;)

The reason I just wrote "pcim_iomap failed\n" is that this seems to be
this driver's style for those messages. See the dev_err() above, there
they also just state that this or that function failed.

I am indifferent about the message, though. Whatever the maintainer
prefers is fine.

P.

Re: [PATCH 06/10] wifi: iwlwifi: replace deprecated PCI functions

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 3993 bytes --]

On Fri, 25 Oct 2024, Philipp Stanner wrote:

> On Fri, 2024-10-25 at 18:31 +0300, Ilpo Järvinen wrote:
> > On Fri, 25 Oct 2024, Philipp Stanner wrote:
> > 
> > > pcim_iomap_table() and pcim_iomap_regions_request_all() 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: Kalle Valo <kvalo@kernel.org>
> > > ---
> > >  drivers/net/wireless/intel/iwlwifi/pcie/trans.c | 16 ++++---------
> > > ---
> > >  1 file changed, 4 insertions(+), 12 deletions(-)
> > > 
> > > diff --git a/drivers/net/wireless/intel/iwlwifi/pcie/trans.c
> > > b/drivers/net/wireless/intel/iwlwifi/pcie/trans.c
> > > index 3b9943eb6934..4b41613ad89d 100644
> > > --- a/drivers/net/wireless/intel/iwlwifi/pcie/trans.c
> > > +++ b/drivers/net/wireless/intel/iwlwifi/pcie/trans.c
> > > @@ -3533,7 +3533,6 @@ struct iwl_trans *iwl_trans_pcie_alloc(struct
> > > pci_dev *pdev,
> > >  	struct iwl_trans_pcie *trans_pcie, **priv;
> > >  	struct iwl_trans *trans;
> > >  	int ret, addr_size;
> > > -	void __iomem * const *table;
> > >  	u32 bar0;
> > >  
> > >  	/* reassign our BAR 0 if invalid due to possible runtime
> > > PM races */
> > > @@ -3659,22 +3658,15 @@ struct iwl_trans
> > > *iwl_trans_pcie_alloc(struct pci_dev *pdev,
> > >  		}
> > >  	}
> > >  
> > > -	ret = pcim_iomap_regions_request_all(pdev, BIT(0),
> > > DRV_NAME);
> > > +	ret = pcim_request_all_regions(pdev, DRV_NAME);
> > >  	if (ret) {
> > > -		dev_err(&pdev->dev,
> > > "pcim_iomap_regions_request_all failed\n");
> > > +		dev_err(&pdev->dev, "pcim_request_all_regions
> > > failed\n");
> > >  		goto out_no_pci;
> > >  	}
> > >  
> > > -	table = pcim_iomap_table(pdev);
> > > -	if (!table) {
> > > -		dev_err(&pdev->dev, "pcim_iomap_table failed\n");
> > > -		ret = -ENOMEM;
> > > -		goto out_no_pci;
> > > -	}
> > > -
> > > -	trans_pcie->hw_base = table[0];
> > > +	trans_pcie->hw_base = pcim_iomap(pdev, 0, 0);
> > >  	if (!trans_pcie->hw_base) {
> > > -		dev_err(&pdev->dev, "couldn't find IO mem in first
> > > BAR\n");
> > > +		dev_err(&pdev->dev, "pcim_iomap failed\n");
> > 
> > This seems a step backwards as a human readable English error message
> > was 
> > replaced with a reference to a function name.
> 
> I think it's still an improvement because "couldn't find IO mem in
> first BAR" is a nonsensical statement. What the author probably meant
> was: "Couldn't find first BAR's IO mem in magic pci_iomap_table" ;)

Well, that's just spelling things on a too low level too. It's irrelevant
detail to the _user_ that kernel used some "magic table". Similarly, it's 
irrelevant to the user that function called pcim_iomap failed.

> The reason I just wrote "pcim_iomap failed\n" is that this seems to be
> this driver's style for those messages. See the dev_err() above, there
> they also just state that this or that function failed.

The problem in using function names is they have obvious meaning for 
developers/coders but dev_err() is presented to user with varying level
of knowledge about kernel internals/code.

While users might be able to derive some information from the function 
name, it would be simply better to explain on higher level what failed 
which is what I think the original message tried to do even if it was
a bit clumsy. There is zero need to know about kernel internals to 
interpret that message (arguably one needs to know some PCI to understand 
BAR, though).

(Developers can find the internals by looking up the error message from
the code so it doesn't take away something from developers.)

-- 
 i.

> I am indifferent about the message, though. Whatever the maintainer
> prefers is fine.

Re: [PATCH 06/10] wifi: iwlwifi: replace deprecated PCI functions

[Philipp Stanner]

On Fri, 2024-10-25 at 19:11 +0300, Ilpo Järvinen wrote:
> On Fri, 25 Oct 2024, Philipp Stanner wrote:
> 
> > On Fri, 2024-10-25 at 18:31 +0300, Ilpo Järvinen wrote:
> > > On Fri, 25 Oct 2024, Philipp Stanner wrote:
> > > 
> > > > pcim_iomap_table() and pcim_iomap_regions_request_all() 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: Kalle Valo <kvalo@kernel.org>
> > > > ---
> > > >  drivers/net/wireless/intel/iwlwifi/pcie/trans.c | 16 ++++-----
> > > > ----
> > > > ---
> > > >  1 file changed, 4 insertions(+), 12 deletions(-)
> > > > 
> > > > diff --git a/drivers/net/wireless/intel/iwlwifi/pcie/trans.c
> > > > b/drivers/net/wireless/intel/iwlwifi/pcie/trans.c
> > > > index 3b9943eb6934..4b41613ad89d 100644
> > > > --- a/drivers/net/wireless/intel/iwlwifi/pcie/trans.c
> > > > +++ b/drivers/net/wireless/intel/iwlwifi/pcie/trans.c
> > > > @@ -3533,7 +3533,6 @@ struct iwl_trans
> > > > *iwl_trans_pcie_alloc(struct
> > > > pci_dev *pdev,
> > > >  	struct iwl_trans_pcie *trans_pcie, **priv;
> > > >  	struct iwl_trans *trans;
> > > >  	int ret, addr_size;
> > > > -	void __iomem * const *table;
> > > >  	u32 bar0;
> > > >  
> > > >  	/* reassign our BAR 0 if invalid due to possible
> > > > runtime
> > > > PM races */
> > > > @@ -3659,22 +3658,15 @@ struct iwl_trans
> > > > *iwl_trans_pcie_alloc(struct pci_dev *pdev,
> > > >  		}
> > > >  	}
> > > >  
> > > > -	ret = pcim_iomap_regions_request_all(pdev, BIT(0),
> > > > DRV_NAME);
> > > > +	ret = pcim_request_all_regions(pdev, DRV_NAME);
> > > >  	if (ret) {
> > > > -		dev_err(&pdev->dev,
> > > > "pcim_iomap_regions_request_all failed\n");
> > > > +		dev_err(&pdev->dev, "pcim_request_all_regions
> > > > failed\n");
> > > >  		goto out_no_pci;
> > > >  	}
> > > >  
> > > > -	table = pcim_iomap_table(pdev);
> > > > -	if (!table) {
> > > > -		dev_err(&pdev->dev, "pcim_iomap_table
> > > > failed\n");
> > > > -		ret = -ENOMEM;
> > > > -		goto out_no_pci;
> > > > -	}
> > > > -
> > > > -	trans_pcie->hw_base = table[0];
> > > > +	trans_pcie->hw_base = pcim_iomap(pdev, 0, 0);
> > > >  	if (!trans_pcie->hw_base) {
> > > > -		dev_err(&pdev->dev, "couldn't find IO mem in
> > > > first
> > > > BAR\n");
> > > > +		dev_err(&pdev->dev, "pcim_iomap failed\n");
> > > 
> > > This seems a step backwards as a human readable English error
> > > message
> > > was 
> > > replaced with a reference to a function name.
> > 
> > I think it's still an improvement because "couldn't find IO mem in
> > first BAR" is a nonsensical statement. What the author probably
> > meant
> > was: "Couldn't find first BAR's IO mem in magic pci_iomap_table" ;)
> 
> Well, that's just spelling things on a too low level too. It's
> irrelevant
> detail to the _user_ that kernel used some "magic table". Similarly,
> it's 
> irrelevant to the user that function called pcim_iomap failed.
> 
> > The reason I just wrote "pcim_iomap failed\n" is that this seems to
> > be
> > this driver's style for those messages. See the dev_err() above,
> > there
> > they also just state that this or that function failed.
> 
> The problem in using function names is they have obvious meaning for 
> developers/coders but dev_err() is presented to user with varying
> level
> of knowledge about kernel internals/code.
> 
> While users might be able to derive some information from the
> function 
> name, it would be simply better to explain on higher level what
> failed 
> which is what I think the original message tried to do even if it was
> a bit clumsy. There is zero need to know about kernel internals to 
> interpret that message (arguably one needs to know some PCI to
> understand 
> BAR, though).
> 
> (Developers can find the internals by looking up the error message
> from
> the code so it doesn't take away something from developers.)

Feel free to make a suggestion for a better error message.

sth like "could not ioremap PCI BAR 0.\n" could satisfy your criteria.

(I just now noticed that so far it called BAR 0 the "first bar", which
is also not gold standard)

P.


> 

Re: [PATCH 06/10] wifi: iwlwifi: replace deprecated PCI functions

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 4773 bytes --]

On Fri, 25 Oct 2024, Philipp Stanner wrote:

> On Fri, 2024-10-25 at 19:11 +0300, Ilpo Järvinen wrote:
> > On Fri, 25 Oct 2024, Philipp Stanner wrote:
> > 
> > > On Fri, 2024-10-25 at 18:31 +0300, Ilpo Järvinen wrote:
> > > > On Fri, 25 Oct 2024, Philipp Stanner wrote:
> > > > 
> > > > > pcim_iomap_table() and pcim_iomap_regions_request_all() 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: Kalle Valo <kvalo@kernel.org>
> > > > > ---
> > > > >  drivers/net/wireless/intel/iwlwifi/pcie/trans.c | 16 ++++-----
> > > > > ----
> > > > > ---
> > > > >  1 file changed, 4 insertions(+), 12 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/net/wireless/intel/iwlwifi/pcie/trans.c
> > > > > b/drivers/net/wireless/intel/iwlwifi/pcie/trans.c
> > > > > index 3b9943eb6934..4b41613ad89d 100644
> > > > > --- a/drivers/net/wireless/intel/iwlwifi/pcie/trans.c
> > > > > +++ b/drivers/net/wireless/intel/iwlwifi/pcie/trans.c
> > > > > @@ -3533,7 +3533,6 @@ struct iwl_trans
> > > > > *iwl_trans_pcie_alloc(struct
> > > > > pci_dev *pdev,
> > > > >  	struct iwl_trans_pcie *trans_pcie, **priv;
> > > > >  	struct iwl_trans *trans;
> > > > >  	int ret, addr_size;
> > > > > -	void __iomem * const *table;
> > > > >  	u32 bar0;
> > > > >  
> > > > >  	/* reassign our BAR 0 if invalid due to possible
> > > > > runtime
> > > > > PM races */
> > > > > @@ -3659,22 +3658,15 @@ struct iwl_trans
> > > > > *iwl_trans_pcie_alloc(struct pci_dev *pdev,
> > > > >  		}
> > > > >  	}
> > > > >  
> > > > > -	ret = pcim_iomap_regions_request_all(pdev, BIT(0),
> > > > > DRV_NAME);
> > > > > +	ret = pcim_request_all_regions(pdev, DRV_NAME);
> > > > >  	if (ret) {
> > > > > -		dev_err(&pdev->dev,
> > > > > "pcim_iomap_regions_request_all failed\n");
> > > > > +		dev_err(&pdev->dev, "pcim_request_all_regions
> > > > > failed\n");
> > > > >  		goto out_no_pci;
> > > > >  	}
> > > > >  
> > > > > -	table = pcim_iomap_table(pdev);
> > > > > -	if (!table) {
> > > > > -		dev_err(&pdev->dev, "pcim_iomap_table
> > > > > failed\n");
> > > > > -		ret = -ENOMEM;
> > > > > -		goto out_no_pci;
> > > > > -	}
> > > > > -
> > > > > -	trans_pcie->hw_base = table[0];
> > > > > +	trans_pcie->hw_base = pcim_iomap(pdev, 0, 0);
> > > > >  	if (!trans_pcie->hw_base) {
> > > > > -		dev_err(&pdev->dev, "couldn't find IO mem in
> > > > > first
> > > > > BAR\n");
> > > > > +		dev_err(&pdev->dev, "pcim_iomap failed\n");
> > > > 
> > > > This seems a step backwards as a human readable English error
> > > > message
> > > > was 
> > > > replaced with a reference to a function name.
> > > 
> > > I think it's still an improvement because "couldn't find IO mem in
> > > first BAR" is a nonsensical statement. What the author probably
> > > meant
> > > was: "Couldn't find first BAR's IO mem in magic pci_iomap_table" ;)
> > 
> > Well, that's just spelling things on a too low level too. It's
> > irrelevant
> > detail to the _user_ that kernel used some "magic table". Similarly,
> > it's 
> > irrelevant to the user that function called pcim_iomap failed.
> > 
> > > The reason I just wrote "pcim_iomap failed\n" is that this seems to
> > > be
> > > this driver's style for those messages. See the dev_err() above,
> > > there
> > > they also just state that this or that function failed.
> > 
> > The problem in using function names is they have obvious meaning for 
> > developers/coders but dev_err() is presented to user with varying
> > level
> > of knowledge about kernel internals/code.
> > 
> > While users might be able to derive some information from the
> > function 
> > name, it would be simply better to explain on higher level what
> > failed 
> > which is what I think the original message tried to do even if it was
> > a bit clumsy. There is zero need to know about kernel internals to 
> > interpret that message (arguably one needs to know some PCI to
> > understand 
> > BAR, though).
> > 
> > (Developers can find the internals by looking up the error message
> > from
> > the code so it doesn't take away something from developers.)
> 
> Feel free to make a suggestion for a better error message.
> 
> sth like "could not ioremap PCI BAR 0.\n" could satisfy your criteria.

Yes.

-- 
 i.

> (I just now noticed that so far it called BAR 0 the "first bar", which
> is also not gold standard)

[PATCH 07/10] ntb: idt: Replace deprecated PCI functions

[Philipp Stanner]

pcim_iomap_table() and pcim_iomap_regions_request_all() 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: Serge Semin <fancer.lancer@gmail.com>
---
 drivers/ntb/hw/idt/ntb_hw_idt.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/ntb/hw/idt/ntb_hw_idt.c b/drivers/ntb/hw/idt/ntb_hw_idt.c
index 6fc9dfe82474..544d8a4d2af5 100644
--- a/drivers/ntb/hw/idt/ntb_hw_idt.c
+++ b/drivers/ntb/hw/idt/ntb_hw_idt.c
@@ -2671,15 +2671,20 @@ static int idt_init_pci(struct idt_ntb_dev *ndev)
 	 */
 	pci_set_master(pdev);
 
-	/* Request all BARs resources and map BAR0 only */
-	ret = pcim_iomap_regions_request_all(pdev, 1, NTB_NAME);
+	/* Request all BARs resources */
+	ret = pcim_request_all_regions(pdev, NTB_NAME);
 	if (ret != 0) {
 		dev_err(&pdev->dev, "Failed to request resources\n");
 		goto err_clear_master;
 	}
 
-	/* Retrieve virtual address of BAR0 - PCI configuration space */
-	ndev->cfgspc = pcim_iomap_table(pdev)[0];
+	/* ioremap BAR0 - PCI configuration space */
+	ndev->cfgspc = pcim_iomap(pdev, 0, 0);
+	if (!ndev->cfgspc) {
+		dev_err(&pdev->dev, "Failed to ioremap BAR 0\n");
+		ret = -ENOMEM;
+		goto err_clear_master;
+	}
 
 	/* Put the IDT driver data pointer to the PCI-device private pointer */
 	pci_set_drvdata(pdev, ndev);
-- 
2.47.0

[PATCH 08/10] serial: rp2: Replace deprecated PCI functions

[Philipp Stanner]

pcim_iomap_table() and pcim_iomap_regions_request_all() 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>
Reviewed-by: Jiri Slaby <jirislaby@kernel.org>
---
 drivers/tty/serial/rp2.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/tty/serial/rp2.c b/drivers/tty/serial/rp2.c
index 8bab2aedc499..6d99a02dd439 100644
--- a/drivers/tty/serial/rp2.c
+++ b/drivers/tty/serial/rp2.c
@@ -698,7 +698,6 @@ static int rp2_probe(struct pci_dev *pdev,
 	const struct firmware *fw;
 	struct rp2_card *card;
 	struct rp2_uart_port *ports;
-	void __iomem * const *bars;
 	int rc;
 
 	card = devm_kzalloc(&pdev->dev, sizeof(*card), GFP_KERNEL);
@@ -711,13 +710,16 @@ static int rp2_probe(struct pci_dev *pdev,
 	if (rc)
 		return rc;
 
-	rc = pcim_iomap_regions_request_all(pdev, 0x03, DRV_NAME);
+	rc = pcim_request_all_regions(pdev, DRV_NAME);
 	if (rc)
 		return rc;
 
-	bars = pcim_iomap_table(pdev);
-	card->bar0 = bars[0];
-	card->bar1 = bars[1];
+	card->bar0 = pcim_iomap(pdev, 0, 0);
+	if (!card->bar0)
+		return -ENOMEM;
+	card->bar1 = pcim_iomap(pdev, 1, 0);
+	if (!card->bar1)
+		return -ENOMEM;
 	card->pdev = pdev;
 
 	rp2_decode_cap(id, &card->n_ports, &card->smpte);
-- 
2.47.0

[PATCH 09/10] ALSA: korg1212: Replace deprecated PCI functions

[Philipp Stanner]

pcim_iomap_table() and pcim_iomap_regions_request_all() 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>
Reviewed-by: Takashi Iwai <tiwai@suse.de>
---
 sound/pci/korg1212/korg1212.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/sound/pci/korg1212/korg1212.c b/sound/pci/korg1212/korg1212.c
index e62fb1ad6d77..49b71082c485 100644
--- a/sound/pci/korg1212/korg1212.c
+++ b/sound/pci/korg1212/korg1212.c
@@ -2108,7 +2108,7 @@ static int snd_korg1212_create(struct snd_card *card, struct pci_dev *pci)
         for (i=0; i<kAudioChannels; i++)
                 korg1212->volumePhase[i] = 0;
 
-	err = pcim_iomap_regions_request_all(pci, 1 << 0, "korg1212");
+	err = pcim_request_all_regions(pci, "korg1212");
 	if (err < 0)
 		return err;
 
@@ -2130,7 +2130,9 @@ static int snd_korg1212_create(struct snd_card *card, struct pci_dev *pci)
 		   korg1212->iomem2, iomem2_size,
 		   stateName[korg1212->cardState]);
 
-	korg1212->iobase = pcim_iomap_table(pci)[0];
+	korg1212->iobase = pcim_iomap(pci, 0, 0);
+	if (!korg1212->iobase)
+		return -ENOMEM;
 
 	err = devm_request_irq(&pci->dev, pci->irq, snd_korg1212_interrupt,
                           IRQF_SHARED,
-- 
2.47.0

[PATCH 10/10] PCI: Remove pcim_iomap_regions_request_all()

[Philipp Stanner]

pcim_iomap_regions_request_all() have been deprecated in
commit e354bb84a4c1 ("PCI: Deprecate pcim_iomap_table(),
pcim_iomap_regions_request_all()").

All users of this function have been ported to other interfaces by now.

Remove pcim_iomap_regions_request_all().

Signed-off-by: Philipp Stanner <pstanner@redhat.com>
Reviewed-by: Damien Le Moal <dlemoal@kernel.org>
---
 .../driver-api/driver-model/devres.rst        |  1 -
 drivers/pci/devres.c                          | 56 -------------------
 include/linux/pci.h                           |  2 -
 3 files changed, 59 deletions(-)

diff --git a/Documentation/driver-api/driver-model/devres.rst b/Documentation/driver-api/driver-model/devres.rst
index 5f2ee8d717b1..3a30cf4f6c0d 100644
--- a/Documentation/driver-api/driver-model/devres.rst
+++ b/Documentation/driver-api/driver-model/devres.rst
@@ -394,7 +394,6 @@ PCI
   pcim_enable_device()		: after success, some PCI ops become managed
   pcim_iomap()			: do iomap() on a single BAR
   pcim_iomap_regions()		: do request_region() and iomap() on multiple BARs
-  pcim_iomap_regions_request_all() : do request_region() on all and iomap() on multiple BARs
   pcim_iomap_table()		: array of mapped addresses indexed by BAR
   pcim_iounmap()		: do iounmap() on a single BAR
   pcim_iounmap_regions()	: do iounmap() and release_region() on multiple BARs
diff --git a/drivers/pci/devres.c b/drivers/pci/devres.c
index 2a64da5c91fb..319a477a2135 100644
--- a/drivers/pci/devres.c
+++ b/drivers/pci/devres.c
@@ -959,62 +959,6 @@ int pcim_request_all_regions(struct pci_dev *pdev, const char *name)
 }
 EXPORT_SYMBOL(pcim_request_all_regions);
 
-/**
- * pcim_iomap_regions_request_all - Request all BARs and iomap specified ones
- *			(DEPRECATED)
- * @pdev: PCI device to map IO resources for
- * @mask: Mask of BARs to iomap
- * @name: Name associated with the requests
- *
- * Returns: 0 on success, negative error code on failure.
- *
- * Request all PCI BARs and iomap regions specified by @mask.
- *
- * To release these resources manually, call pcim_release_region() for the
- * regions and pcim_iounmap() for the mappings.
- *
- * This function is DEPRECATED. Don't use it in new code. Instead, use one
- * of the pcim_* region request functions in combination with a pcim_*
- * mapping function.
- */
-int pcim_iomap_regions_request_all(struct pci_dev *pdev, int mask,
-				   const char *name)
-{
-	int bar;
-	int ret;
-	void __iomem **legacy_iomap_table;
-
-	ret = pcim_request_all_regions(pdev, name);
-	if (ret != 0)
-		return ret;
-
-	for (bar = 0; bar < PCI_STD_NUM_BARS; bar++) {
-		if (!mask_contains_bar(mask, bar))
-			continue;
-		if (!pcim_iomap(pdev, bar, 0))
-			goto err;
-	}
-
-	return 0;
-
-err:
-	/*
-	 * If bar is larger than 0, then pcim_iomap() above has most likely
-	 * failed because of -EINVAL. If it is equal 0, most likely the table
-	 * couldn't be created, indicating -ENOMEM.
-	 */
-	ret = bar > 0 ? -EINVAL : -ENOMEM;
-	legacy_iomap_table = (void __iomem **)pcim_iomap_table(pdev);
-
-	while (--bar >= 0)
-		pcim_iounmap(pdev, legacy_iomap_table[bar]);
-
-	pcim_release_all_regions(pdev);
-
-	return ret;
-}
-EXPORT_SYMBOL(pcim_iomap_regions_request_all);
-
 /**
  * pcim_iounmap_regions - Unmap and release PCI BARs
  * @pdev: PCI device to map IO resources for
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 3b151c8331e5..b59197635c5c 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -2301,8 +2301,6 @@ void pcim_iounmap(struct pci_dev *pdev, void __iomem *addr);
 void __iomem * const *pcim_iomap_table(struct pci_dev *pdev);
 int pcim_request_region(struct pci_dev *pdev, int bar, const char *name);
 int pcim_iomap_regions(struct pci_dev *pdev, int mask, const char *name);
-int pcim_iomap_regions_request_all(struct pci_dev *pdev, int mask,
-				   const char *name);
 void pcim_iounmap_regions(struct pci_dev *pdev, int mask);
 void __iomem *pcim_iomap_range(struct pci_dev *pdev, int bar,
 				unsigned long offset, unsigned long len);
-- 
2.47.0

Re: [PATCH 00/10] Remove pcim_iomap_regions_request_all()

[Philipp Stanner]

Aaaaaaaaaaaand of course I forgot to mark this series as a Version 5.

I am sorry.

P.


On Fri, 2024-10-25 at 16:59 +0200, Philipp Stanner wrote:
> All Acked-by's are in place now.
> 
> Changes in v5:
>   - Add Acked-by's from Alexander and Bharat (the latter sent off-
> list,
>     because of some issue with receiving the previous patch sets).
> 
> Changes in v4:
>   - Add Acked-by's from Giovanni and Kalle.
> 
> Changes in v3:
>   - Add missing full stops to commit messages (Andy).
> 
> Changes in v2:
>   - Fix a bug in patch №4 ("crypto: marvell ...") where an error code
>     was not set before printing it. (Me)
>   - Apply Damien's Reviewed- / Acked-by to patches 1, 2 and 10.
> (Damien)
>   - Apply Serge's Acked-by to patch №7. (Serge)
>   - Apply Jiri's Reviewed-by to patch №8. (Jiri)
>   - Apply Takashi Iwai's Reviewed-by to patch №9. (Takashi)
> 
> 
> Hi all,
> 
> the PCI subsystem is currently working on cleaning up its devres API.
> To
> do so, a few functions will be replaced with better alternatives.
> 
> This series removes pcim_iomap_regions_request_all(), which has been
> deprecated already, and accordingly replaces the calls to
> pcim_iomap_table() (which were only necessary because of
> pcim_iomap_regions_request_all() in the first place) with calls to
> pcim_iomap().
> 
> Would be great if you can take a look whether this behaves as you
> intended for your respective component.
> 
> Cheers,
> Philipp
> 
> Philipp Stanner (10):
>   PCI: Make pcim_request_all_regions() a public function
>   ata: ahci: Replace deprecated PCI functions
>   crypto: qat - replace deprecated PCI functions
>   crypto: marvell - replace deprecated PCI functions
>   intel_th: pci: Replace deprecated PCI functions
>   wifi: iwlwifi: replace deprecated PCI functions
>   ntb: idt: Replace deprecated PCI functions
>   serial: rp2: Replace deprecated PCI functions
>   ALSA: korg1212: Replace deprecated PCI functions
>   PCI: Remove pcim_iomap_regions_request_all()
> 
>  .../driver-api/driver-model/devres.rst        |  1 -
>  drivers/ata/acard-ahci.c                      |  6 +-
>  drivers/ata/ahci.c                            |  6 +-
>  drivers/crypto/intel/qat/qat_420xx/adf_drv.c  | 11 +++-
>  drivers/crypto/intel/qat/qat_4xxx/adf_drv.c   | 11 +++-
>  .../marvell/octeontx2/otx2_cptpf_main.c       | 14 +++--
>  .../marvell/octeontx2/otx2_cptvf_main.c       | 13 ++--
>  drivers/hwtracing/intel_th/pci.c              |  9 ++-
>  .../net/wireless/intel/iwlwifi/pcie/trans.c   | 16 ++---
>  drivers/ntb/hw/idt/ntb_hw_idt.c               | 13 ++--
>  drivers/pci/devres.c                          | 59 +----------------
> --
>  drivers/tty/serial/rp2.c                      | 12 ++--
>  include/linux/pci.h                           |  3 +-
>  sound/pci/korg1212/korg1212.c                 |  6 +-
>  14 files changed, 76 insertions(+), 104 deletions(-)
>