[PATCH 0/9] PCI: Remove pcim_iounmap_regions()

[PATCH 0/9] PCI: Remove pcim_iounmap_regions()

[Philipp Stanner]

Important things first:
This series is based on [1] and [2] which Bjorn Helgaas has currently
queued for v6.12 in the PCI tree.

This series shall remove pcim_iounmap_regions() in order to make way to
remove its brother, pcim_iomap_regions().

@Bjorn: Feel free to squash the PCI commits.

Regards,
P.

[1] https://lore.kernel.org/all/20240729093625.17561-4-pstanner@redhat.com/
[2] https://lore.kernel.org/all/20240807083018.8734-2-pstanner@redhat.com/

Philipp Stanner (9):
  PCI: Make pcim_release_region() a public function
  PCI: Make pcim_iounmap_region() a public function
  fpga/dfl-pci.c: Replace deprecated PCI functions
  block: mtip32xx: Replace deprecated PCI functions
  gpio: Replace deprecated PCI functions
  ethernet: cavium: Replace deprecated PCI functions
  ethernet: stmicro: Simplify PCI devres usage
  vdap: solidrun: Replace deprecated PCI functions
  PCI: Remove pcim_iounmap_regions()

 .../driver-api/driver-model/devres.rst        |  1 -
 drivers/block/mtip32xx/mtip32xx.c             | 11 +++--
 drivers/fpga/dfl-pci.c                        |  9 ++--
 drivers/gpio/gpio-merrifield.c                | 14 +++---
 .../net/ethernet/cavium/common/cavium_ptp.c   | 10 ++--
 .../ethernet/stmicro/stmmac/dwmac-loongson.c  | 25 +++-------
 .../net/ethernet/stmicro/stmmac/stmmac_pci.c  | 18 +++----
 drivers/pci/devres.c                          | 25 ++--------
 drivers/pci/pci.h                             |  1 -
 drivers/vdpa/solidrun/snet_main.c             | 47 +++++++------------
 include/linux/pci.h                           |  3 +-
 11 files changed, 57 insertions(+), 107 deletions(-)

-- 
2.46.0

[PATCH 1/9] PCI: Make pcim_release_region() a public function

[Philipp Stanner]

pcim_release_region() is the managed counterpart of
pci_release_region(). It can be useful in some cases where drivers want
to manually release a requested region before the driver's remove()
callback is invoked.

Make pcim_release_region() a public function.

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

diff --git a/drivers/pci/devres.c b/drivers/pci/devres.c
index b97589e99fad..608f13ef2a4b 100644
--- a/drivers/pci/devres.c
+++ b/drivers/pci/devres.c
@@ -906,6 +906,7 @@ void pcim_release_region(struct pci_dev *pdev, int bar)
 	devres_release(&pdev->dev, pcim_addr_resource_release,
 			pcim_addr_resources_match, &res_searched);
 }
+EXPORT_SYMBOL(pcim_release_region);
 
 
 /**
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 2fe6055a334d..01b55ed2867c 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -889,7 +889,6 @@ static inline pci_power_t mid_pci_get_power_state(struct pci_dev *pdev)
 int pcim_intx(struct pci_dev *dev, int enable);
 int pcim_request_region_exclusive(struct pci_dev *pdev, int bar,
 				  const char *name);
-void pcim_release_region(struct pci_dev *pdev, int bar);
 
 /*
  * Config Address for PCI Configuration Mechanism #1
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 01b9f1a351be..dfa9af3a9c22 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -2295,6 +2295,7 @@ void __iomem *pcim_iomap(struct pci_dev *pdev, int bar, unsigned long maxlen);
 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);
+void pcim_release_region(struct pci_dev *pdev, int bar);
 void __iomem *pcim_iomap_region(struct pci_dev *pdev, int bar,
 				       const char *name);
 int pcim_iomap_regions(struct pci_dev *pdev, int mask, const char *name);
-- 
2.46.0

Re: [PATCH 1/9] PCI: Make pcim_release_region() a public function

[Damien Le Moal]

On 8/20/24 01:51, Philipp Stanner wrote:
> pcim_release_region() is the managed counterpart of
> pci_release_region(). It can be useful in some cases where drivers want
> to manually release a requested region before the driver's remove()
> callback is invoked.
> 
> Make pcim_release_region() a public function.
> 
> Signed-off-by: Philipp Stanner <pstanner@redhat.com>

Looks fine to me. But I think this should be squashed with patch 2 (I do not see
the point of having 2 patches to export 2 functions that are complementary).
Either way:

Reviewed-by: Damien Le Moal <dlemoal@kernel.org>

-- 
Damien Le Moal
Western Digital Research

[PATCH 2/9] PCI: Make pcim_iounmap_region() a public function

[Philipp Stanner]

The function pcim_iounmap_regions() is problematic because it uses a
bitmask mechanism to release / iounmap multiple BARs at once. It, thus,
prevents getting rid of the problematic iomap table mechanism which was
deprecated in commit e354bb84a4c1 ("PCI: Deprecate pcim_iomap_table(),
pcim_iomap_regions_request_all()").

Make pcim_iounmap_region() public as the successor of
pcim_iounmap_regions().

Signed-off-by: Philipp Stanner <pstanner@redhat.com>
---
 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 608f13ef2a4b..30c813766e8b 100644
--- a/drivers/pci/devres.c
+++ b/drivers/pci/devres.c
@@ -771,7 +771,7 @@ EXPORT_SYMBOL(pcim_iomap_region);
  * Unmap a BAR and release its region manually. Only pass BARs that were
  * previously mapped by pcim_iomap_region().
  */
-static void pcim_iounmap_region(struct pci_dev *pdev, int bar)
+void pcim_iounmap_region(struct pci_dev *pdev, int bar)
 {
 	struct pcim_addr_devres res_searched;
 
@@ -782,6 +782,7 @@ static void pcim_iounmap_region(struct pci_dev *pdev, int bar)
 	devres_release(&pdev->dev, pcim_addr_resource_release,
 			pcim_addr_resources_match, &res_searched);
 }
+EXPORT_SYMBOL(pcim_iounmap_region);
 
 /**
  * pcim_iomap_regions - Request and iomap PCI BARs (DEPRECATED)
diff --git a/include/linux/pci.h b/include/linux/pci.h
index dfa9af3a9c22..7de75900854a 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -2298,6 +2298,7 @@ int pcim_request_region(struct pci_dev *pdev, int bar, const char *name);
 void pcim_release_region(struct pci_dev *pdev, int bar);
 void __iomem *pcim_iomap_region(struct pci_dev *pdev, int bar,
 				       const char *name);
+void pcim_iounmap_region(struct pci_dev *pdev, int bar);
 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);
-- 
2.46.0

Re: [PATCH 2/9] PCI: Make pcim_iounmap_region() a public function

[Damien Le Moal]

On 8/20/24 01:51, Philipp Stanner wrote:
> The function pcim_iounmap_regions() is problematic because it uses a
> bitmask mechanism to release / iounmap multiple BARs at once. It, thus,
> prevents getting rid of the problematic iomap table mechanism which was
> deprecated in commit e354bb84a4c1 ("PCI: Deprecate pcim_iomap_table(),
> pcim_iomap_regions_request_all()").
> 
> Make pcim_iounmap_region() public as the successor of
> pcim_iounmap_regions().
> 
> Signed-off-by: Philipp Stanner <pstanner@redhat.com>

Looks fine, but as commented on patch 1, I think this would look better squashed
with patch 1.

Anyway:

Reviewed-by: Damien Le Moal <dlemoal@kernel.org>

-- 
Damien Le Moal
Western Digital Research

[PATCH 3/9] fpga/dfl-pci.c: Replace deprecated PCI functions

[Philipp Stanner]

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

Port dfl-pci.c to the successor, pcim_iomap_region().

Consistently, replace pcim_iounmap_regions() with pcim_iounmap_region().

Signed-off-by: Philipp Stanner <pstanner@redhat.com>
---
 drivers/fpga/dfl-pci.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/fpga/dfl-pci.c b/drivers/fpga/dfl-pci.c
index 80cac3a5f976..2099c497feec 100644
--- a/drivers/fpga/dfl-pci.c
+++ b/drivers/fpga/dfl-pci.c
@@ -41,10 +41,13 @@ struct cci_drvdata {
 
 static void __iomem *cci_pci_ioremap_bar0(struct pci_dev *pcidev)
 {
-	if (pcim_iomap_regions(pcidev, BIT(0), DRV_NAME))
+	void __iomem *bar0;
+
+	bar0 = pcim_iomap_region(pcidev, 0, DRV_NAME);
+	if (IS_ERR(bar0))
 		return NULL;
 
-	return pcim_iomap_table(pcidev)[0];
+	return bar0;
 }
 
 static int cci_pci_alloc_irq(struct pci_dev *pcidev)
@@ -296,7 +299,7 @@ static int find_dfls_by_default(struct pci_dev *pcidev,
 	}
 
 	/* release I/O mappings for next step enumeration */
-	pcim_iounmap_regions(pcidev, BIT(0));
+	pcim_iounmap_region(pcidev, 0);
 
 	return ret;
 }
-- 
2.46.0

[PATCH 4/9] block: mtip32xx: Replace deprecated PCI functions

[Philipp Stanner]

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);
 	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);
-- 
2.46.0

Re: [PATCH 4/9] block: mtip32xx: Replace deprecated PCI functions

[Andy Shevchenko]

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…

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 4/9] block: mtip32xx: Replace deprecated PCI functions

[Philipp Stanner]

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.

> 

Re: [PATCH 4/9] block: mtip32xx: Replace deprecated PCI functions

[Andy Shevchenko]

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.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 4/9] block: mtip32xx: Replace deprecated PCI functions

[Philipp Stanner]

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

[PATCH 5/9] gpio: Replace deprecated PCI functions

[Philipp Stanner]

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 those functions with calls to pcim_iomap_region().

Signed-off-by: Philipp Stanner <pstanner@redhat.com>
---
 drivers/gpio/gpio-merrifield.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/gpio/gpio-merrifield.c b/drivers/gpio/gpio-merrifield.c
index 421d7e3a6c66..274afcba31e6 100644
--- a/drivers/gpio/gpio-merrifield.c
+++ b/drivers/gpio/gpio-merrifield.c
@@ -78,24 +78,24 @@ static int mrfld_gpio_probe(struct pci_dev *pdev, const struct pci_device_id *id
 	if (retval)
 		return retval;
 
-	retval = pcim_iomap_regions(pdev, BIT(1) | BIT(0), pci_name(pdev));
-	if (retval)
-		return dev_err_probe(dev, retval, "I/O memory mapping error\n");
-
-	base = pcim_iomap_table(pdev)[1];
+	base = pcim_iomap_region(pdev, 1, pci_name(pdev));
+	if (IS_ERR(base))
+		return dev_err_probe(dev, PTR_ERR(base), "I/O memory mapping error\n");
 
 	irq_base = readl(base + 0 * sizeof(u32));
 	gpio_base = readl(base + 1 * sizeof(u32));
 
 	/* Release the IO mapping, since we already get the info from BAR1 */
-	pcim_iounmap_regions(pdev, BIT(1));
+	pcim_iounmap_region(pdev, 1);
 
 	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
 	if (!priv)
 		return -ENOMEM;
 
 	priv->dev = dev;
-	priv->reg_base = pcim_iomap_table(pdev)[0];
+	priv->reg_base = pcim_iomap_region(pdev, 0, pci_name(pdev));
+	if (IS_ERR(priv->reg_base))
+		return dev_err_probe(dev, PTR_ERR(base), "I/O memory mapping error\n");
 
 	priv->pin_info.pin_ranges = mrfld_gpio_ranges;
 	priv->pin_info.nranges = ARRAY_SIZE(mrfld_gpio_ranges);
-- 
2.46.0

Re: [PATCH 5/9] gpio: Replace deprecated PCI functions

[Andy Shevchenko]

On Mon, Aug 19, 2024 at 06:51:45PM +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 those functions with calls to pcim_iomap_region().

Reviewed-by: Andy Shevchenko <andy@kernel.org>

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 5/9] gpio: Replace deprecated PCI functions

[Bartosz Golaszewski]

On Mon, Aug 19, 2024 at 6:53 PM Philipp Stanner <pstanner@redhat.com> 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 those functions with calls to pcim_iomap_region().
>
> Signed-off-by: Philipp Stanner <pstanner@redhat.com>
> ---

Acked-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

[PATCH 6/9] ethernet: cavium: Replace deprecated PCI functions

[Philipp Stanner]

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

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);
-- 
2.46.0

Re: [PATCH 6/9] ethernet: cavium: Replace deprecated PCI functions

[Andy Shevchenko]

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?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 6/9] ethernet: cavium: Replace deprecated PCI functions

[Philipp Stanner]

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.

Re: [PATCH 6/9] ethernet: cavium: Replace deprecated PCI functions

[Andy Shevchenko]

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.

-- 
With Best Regards,
Andy Shevchenko

[PATCH 7/9] ethernet: stmicro: Simplify PCI devres usage

[Philipp Stanner]

stmicro uses PCI devres in the wrong way. Resources requested
through pcim_* functions don't need to be cleaned up manually in the
remove() callback or in the error unwind path of a probe() function.

Moreover, there is an unnecessary loop which only requests and ioremaps
BAR 0, but iterates over all BARs nevertheless.

Furthermore, 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 pcim_iomap_region().

Remove the unnecessary manual pcim_* cleanup calls.

Remove the unnecessary loop over all BARs.

Signed-off-by: Philipp Stanner <pstanner@redhat.com>
---
 .../ethernet/stmicro/stmmac/dwmac-loongson.c  | 25 +++++--------------
 .../net/ethernet/stmicro/stmmac/stmmac_pci.c  | 18 +++++--------
 2 files changed, 12 insertions(+), 31 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c
index 9e40c28d453a..5d42a9fad672 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c
@@ -50,7 +50,7 @@ static int loongson_dwmac_probe(struct pci_dev *pdev, const struct pci_device_id
 	struct plat_stmmacenet_data *plat;
 	struct stmmac_resources res;
 	struct device_node *np;
-	int ret, i, phy_mode;
+	int ret, phy_mode;
 
 	np = dev_of_node(&pdev->dev);
 
@@ -88,14 +88,11 @@ static int loongson_dwmac_probe(struct pci_dev *pdev, const struct pci_device_id
 		goto err_put_node;
 	}
 
-	/* Get the base address of device */
-	for (i = 0; i < PCI_STD_NUM_BARS; i++) {
-		if (pci_resource_len(pdev, i) == 0)
-			continue;
-		ret = pcim_iomap_regions(pdev, BIT(0), pci_name(pdev));
-		if (ret)
-			goto err_disable_device;
-		break;
+	memset(&res, 0, sizeof(res));
+	res.addr = pcim_iomap_region(pdev, 0, pci_name(pdev));
+	if (IS_ERR(res.addr)) {
+		ret = PTR_ERR(res.addr);
+		goto err_disable_device;
 	}
 
 	plat->bus_id = of_alias_get_id(np, "ethernet");
@@ -116,8 +113,6 @@ static int loongson_dwmac_probe(struct pci_dev *pdev, const struct pci_device_id
 
 	loongson_default_data(plat);
 	pci_enable_msi(pdev);
-	memset(&res, 0, sizeof(res));
-	res.addr = pcim_iomap_table(pdev)[0];
 
 	res.irq = of_irq_get_byname(np, "macirq");
 	if (res.irq < 0) {
@@ -158,18 +153,10 @@ static void loongson_dwmac_remove(struct pci_dev *pdev)
 {
 	struct net_device *ndev = dev_get_drvdata(&pdev->dev);
 	struct stmmac_priv *priv = netdev_priv(ndev);
-	int i;
 
 	of_node_put(priv->plat->mdio_node);
 	stmmac_dvr_remove(&pdev->dev);
 
-	for (i = 0; i < PCI_STD_NUM_BARS; i++) {
-		if (pci_resource_len(pdev, i) == 0)
-			continue;
-		pcim_iounmap_regions(pdev, BIT(i));
-		break;
-	}
-
 	pci_disable_msi(pdev);
 	pci_disable_device(pdev);
 }
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c
index 352b01678c22..f89a8a54c4e8 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c
@@ -188,11 +188,11 @@ static int stmmac_pci_probe(struct pci_dev *pdev,
 		return ret;
 	}
 
-	/* Get the base address of device */
+	/* Request the base address BAR of device */
 	for (i = 0; i < PCI_STD_NUM_BARS; i++) {
 		if (pci_resource_len(pdev, i) == 0)
 			continue;
-		ret = pcim_iomap_regions(pdev, BIT(i), pci_name(pdev));
+		ret = pcim_request_region(pdev, i, pci_name(pdev));
 		if (ret)
 			return ret;
 		break;
@@ -205,7 +205,10 @@ static int stmmac_pci_probe(struct pci_dev *pdev,
 		return ret;
 
 	memset(&res, 0, sizeof(res));
-	res.addr = pcim_iomap_table(pdev)[i];
+	/* Get the base address of device */
+	res.addr = pcim_iomap(pdev, i, 0);
+	if (!res.addr)
+		return -ENOMEM;
 	res.wol_irq = pdev->irq;
 	res.irq = pdev->irq;
 
@@ -231,16 +234,7 @@ static int stmmac_pci_probe(struct pci_dev *pdev,
  */
 static void stmmac_pci_remove(struct pci_dev *pdev)
 {
-	int i;
-
 	stmmac_dvr_remove(&pdev->dev);
-
-	for (i = 0; i < PCI_STD_NUM_BARS; i++) {
-		if (pci_resource_len(pdev, i) == 0)
-			continue;
-		pcim_iounmap_regions(pdev, BIT(i));
-		break;
-	}
 }
 
 static int __maybe_unused stmmac_pci_suspend(struct device *dev)
-- 
2.46.0

Re: [PATCH 7/9] ethernet: stmicro: Simplify PCI devres usage

[Andy Shevchenko]

On Mon, Aug 19, 2024 at 06:51:47PM +0200, Philipp Stanner wrote:
> stmicro uses PCI devres in the wrong way. Resources requested
> through pcim_* functions don't need to be cleaned up manually in the
> remove() callback or in the error unwind path of a probe() function.
> 
> Moreover, there is an unnecessary loop which only requests and ioremaps
> BAR 0, but iterates over all BARs nevertheless.
> 
> Furthermore, 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 pcim_iomap_region().
> 
> Remove the unnecessary manual pcim_* cleanup calls.
> 
> Remove the unnecessary loop over all BARs.

...

loongson_dwmac_probe()

> +	memset(&res, 0, sizeof(res));
> +	res.addr = pcim_iomap_region(pdev, 0, pci_name(pdev));
> +	if (IS_ERR(res.addr)) {
> +		ret = PTR_ERR(res.addr);
> +		goto err_disable_device;

It seems your series reveals issues in the error paths of .probe():s
in many drivers...

If we use pcim variant to enable device, why do we need to explicitly
disable it?

>  	}

...

loongson_dwmac_remove()

>  	pci_disable_msi(pdev);
>  	pci_disable_device(pdev);

Not sure why we need these either...

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 7/9] ethernet: stmicro: Simplify PCI devres usage

[Philipp Stanner]

On Mon, 2024-08-19 at 21:28 +0300, Andy Shevchenko wrote:
> On Mon, Aug 19, 2024 at 06:51:47PM +0200, Philipp Stanner wrote:
> > stmicro uses PCI devres in the wrong way. Resources requested
> > through pcim_* functions don't need to be cleaned up manually in
> > the
> > remove() callback or in the error unwind path of a probe()
> > function.
> > 
> > Moreover, there is an unnecessary loop which only requests and
> > ioremaps
> > BAR 0, but iterates over all BARs nevertheless.
> > 
> > Furthermore, 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 pcim_iomap_region().
> > 
> > Remove the unnecessary manual pcim_* cleanup calls.
> > 
> > Remove the unnecessary loop over all BARs.
> 
> ...
> 
> loongson_dwmac_probe()
> 
> > +	memset(&res, 0, sizeof(res));
> > +	res.addr = pcim_iomap_region(pdev, 0, pci_name(pdev));
> > +	if (IS_ERR(res.addr)) {
> > +		ret = PTR_ERR(res.addr);
> > +		goto err_disable_device;
> 
> It seems your series reveals issues in the error paths of .probe():s
> in many drivers...
> 
> If we use pcim variant to enable device, why do we need to explicitly
> disable it?

No.

> 
> >  	}
> 
> ...
> 
> loongson_dwmac_remove()
> 
> >  	pci_disable_msi(pdev);
> >  	pci_disable_device(pdev);
> 
> Not sure why we need these either...

It's complicated.

The code uses pciM_enable_device(), but here in remove
pci_disable_device().

pcim_enable_device() sets up a disable callback which only calls
pci_disable_device() if pcim_pin_device() has not been called.

This code doesn't seem to call pcim_pin_device(), so I think
pci_disable_device() could be removed.


I definitely would not feel confident touching pci_disable_msi(),
though. The AFAIK biggest problem remaining in PCI devres is that the
MSI code base implicitly calls into devres, see here [1]


P.


[1] https://lore.kernel.org/all/ee44ea7ac760e73edad3f20b30b4d2fff66c1a85.camel@redhat.com/

Re: [PATCH 7/9] ethernet: stmicro: Simplify PCI devres usage

[Andy Shevchenko]

On Tue, Aug 20, 2024 at 09:52:40AM +0200, Philipp Stanner wrote:
> On Mon, 2024-08-19 at 21:28 +0300, Andy Shevchenko wrote:
> > On Mon, Aug 19, 2024 at 06:51:47PM +0200, Philipp Stanner wrote:

...

> > loongson_dwmac_probe()
> > 
> > > +	memset(&res, 0, sizeof(res));
> > > +	res.addr = pcim_iomap_region(pdev, 0, pci_name(pdev));
> > > +	if (IS_ERR(res.addr)) {
> > > +		ret = PTR_ERR(res.addr);
> > > +		goto err_disable_device;
> > 
> > It seems your series reveals issues in the error paths of .probe():s
> > in many drivers...
> > 
> > If we use pcim variant to enable device, why do we need to explicitly
> > disable it?
> 
> No.

Can you elaborate? No issues being revealed, or no need to disable it
explicitly, or...?

> > >  	}

...

> > loongson_dwmac_remove()
> > 
> > >  	pci_disable_msi(pdev);
> > >  	pci_disable_device(pdev);
> > 
> > Not sure why we need these either...
> 
> It's complicated.
> 
> The code uses pciM_enable_device(), but here in remove
> pci_disable_device().
> 
> pcim_enable_device() sets up a disable callback which only calls
> pci_disable_device() if pcim_pin_device() has not been called.
> 
> This code doesn't seem to call pcim_pin_device(), so I think
> pci_disable_device() could be removed.
> 
> 
> I definitely would not feel confident touching pci_disable_msi(),
> though. The AFAIK biggest problem remaining in PCI devres is that the
> MSI code base implicitly calls into devres, see here [1]

But isn't it a busyness of PCI core to call pci_disable_msi() at the right
moment? Okay, I admit that there might be devices that require a special
workflow WRT MSI, is this the case here?

> [1] https://lore.kernel.org/all/ee44ea7ac760e73edad3f20b30b4d2fff66c1a85.camel@redhat.com/

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 7/9] ethernet: stmicro: Simplify PCI devres usage

[Philipp Stanner]

On Tue, 2024-08-20 at 13:37 +0300, Andy Shevchenko wrote:
> On Tue, Aug 20, 2024 at 09:52:40AM +0200, Philipp Stanner wrote:
> > On Mon, 2024-08-19 at 21:28 +0300, Andy Shevchenko wrote:
> > > On Mon, Aug 19, 2024 at 06:51:47PM +0200, Philipp Stanner wrote:
> 
> ...
> 
> > > loongson_dwmac_probe()
> > > 
> > > > +	memset(&res, 0, sizeof(res));
> > > > +	res.addr = pcim_iomap_region(pdev, 0, pci_name(pdev));
> > > > +	if (IS_ERR(res.addr)) {
> > > > +		ret = PTR_ERR(res.addr);
> > > > +		goto err_disable_device;
> > > 
> > > It seems your series reveals issues in the error paths of
> > > .probe():s
> > > in many drivers...
> > > 
> > > If we use pcim variant to enable device, why do we need to
> > > explicitly
> > > disable it?
> > 
> > No.
> 
> Can you elaborate? No issues being revealed, or no need to disable it
> explicitly, or...?

Oh, my bad, I overlooked your "why" in that question.

We do not explicitly have to disable it. It's wrong / unnecessary, as
many of the other calls you criticized in this series.

pcim_enable_device() (in pci/devres.c) calls devm_add_action(...,
pcim_disable_device,  ...), which will disable the device on driver
detach.

So the call of pci_disable_device() above is redundant. We could remove
it.

> 
> > > >  	}
> 
> ...
> 
> > > loongson_dwmac_remove()
> > > 
> > > >  	pci_disable_msi(pdev);
> > > >  	pci_disable_device(pdev);
> > > 
> > > Not sure why we need these either...
> > 
> > It's complicated.
> > 
> > The code uses pciM_enable_device(), but here in remove
> > pci_disable_device().
> > 
> > pcim_enable_device() sets up a disable callback which only calls
> > pci_disable_device() if pcim_pin_device() has not been called.
> > 
> > This code doesn't seem to call pcim_pin_device(), so I think
> > pci_disable_device() could be removed.
> > 
> > 
> > I definitely would not feel confident touching pci_disable_msi(),
> > though. The AFAIK biggest problem remaining in PCI devres is that
> > the
> > MSI code base implicitly calls into devres, see here [1]
> 
> But isn't it a busyness of PCI core to call pci_disable_msi() at the
> right
> moment? Okay, I admit that there might be devices that require a
> special
> workflow WRT MSI, is this the case here?

I don't know enough about how MSI is intended to be used.

From what I've seen in the code base, pcim_setup_msi_release() does
register a devres callback that will indeed call pci_disable_msi()
after some intermediate calls.

But in my honest opinion, that code is _very_ broken. I was thinking
about how we might clean it up, but couldn't come up with an idea yet.

Only after the code in pci/msi/ has been cleanly separated from
implicit devres I myself would start touching function calls related to
MSI.

That being said, I suspect that one can remove pci_disable_msi() in the
line above. But the risk-benefit-ratio doesn't pay off for me.

P.


> 
> > [1]
> > https://lore.kernel.org/all/ee44ea7ac760e73edad3f20b30b4d2fff66c1a85.camel@redhat.com/
> 

[PATCH 8/9] vdap: solidrun: Replace deprecated PCI functions

[Philipp Stanner]

solidrun utilizes pcim_iomap_regions(), which has been deprecated by the
PCI subsystem in commit e354bb84a4c1 ("PCI: Deprecate
pcim_iomap_table(), pcim_iomap_regions_request_all()"), among other
things because it forces usage of quite a complicated bitmask mechanism.
The bitmask handling code can entirely be removed by replacing
pcim_iomap_regions() and pcim_iomap_table().

Replace pcim_iomap_regions() and pcim_iomap_table() with
pci_iomap_region().

Signed-off-by: Philipp Stanner <pstanner@redhat.com>
---
 drivers/vdpa/solidrun/snet_main.c | 47 +++++++++++--------------------
 1 file changed, 16 insertions(+), 31 deletions(-)

diff --git a/drivers/vdpa/solidrun/snet_main.c b/drivers/vdpa/solidrun/snet_main.c
index 99428a04068d..abf027ca35e1 100644
--- a/drivers/vdpa/solidrun/snet_main.c
+++ b/drivers/vdpa/solidrun/snet_main.c
@@ -556,33 +556,24 @@ static const struct vdpa_config_ops snet_config_ops = {
 static int psnet_open_pf_bar(struct pci_dev *pdev, struct psnet *psnet)
 {
 	char name[50];
-	int ret, i, mask = 0;
+	int i;
+
+	snprintf(name, sizeof(name), "psnet[%s]-bars", pci_name(pdev));
+
 	/* We don't know which BAR will be used to communicate..
 	 * We will map every bar with len > 0.
 	 *
 	 * Later, we will discover the BAR and unmap all other BARs.
 	 */
 	for (i = 0; i < PCI_STD_NUM_BARS; i++) {
-		if (pci_resource_len(pdev, i))
-			mask |= (1 << i);
-	}
-
-	/* No BAR can be used.. */
-	if (!mask) {
-		SNET_ERR(pdev, "Failed to find a PCI BAR\n");
-		return -ENODEV;
-	}
-
-	snprintf(name, sizeof(name), "psnet[%s]-bars", pci_name(pdev));
-	ret = pcim_iomap_regions(pdev, mask, name);
-	if (ret) {
-		SNET_ERR(pdev, "Failed to request and map PCI BARs\n");
-		return ret;
-	}
+		if (pci_resource_len(pdev, i)) {
+			psnet->bars[i] = pcim_iomap_region(pdev, i, name);
+			if (IS_ERR(psnet->bars[i])) {
+				SNET_ERR(pdev, "Failed to request and map PCI BARs\n");
+				return PTR_ERR(psnet->bars[i]);
+			}
+		}
 
-	for (i = 0; i < PCI_STD_NUM_BARS; i++) {
-		if (mask & (1 << i))
-			psnet->bars[i] = pcim_iomap_table(pdev)[i];
 	}
 
 	return 0;
@@ -591,18 +582,15 @@ static int psnet_open_pf_bar(struct pci_dev *pdev, struct psnet *psnet)
 static int snet_open_vf_bar(struct pci_dev *pdev, struct snet *snet)
 {
 	char name[50];
-	int ret;
 
 	snprintf(name, sizeof(name), "snet[%s]-bar", pci_name(pdev));
 	/* Request and map BAR */
-	ret = pcim_iomap_regions(pdev, BIT(snet->psnet->cfg.vf_bar), name);
-	if (ret) {
+	snet->bar = pcim_iomap_region(pdev, snet->psnet->cfg.vf_bar, name);
+	if (IS_ERR(snet->bar)) {
 		SNET_ERR(pdev, "Failed to request and map PCI BAR for a VF\n");
-		return ret;
+		return PTR_ERR(snet->bar);
 	}
 
-	snet->bar = pcim_iomap_table(pdev)[snet->psnet->cfg.vf_bar];
-
 	return 0;
 }
 
@@ -650,15 +638,12 @@ static int psnet_detect_bar(struct psnet *psnet, u32 off)
 
 static void psnet_unmap_unused_bars(struct pci_dev *pdev, struct psnet *psnet)
 {
-	int i, mask = 0;
+	int i;
 
 	for (i = 0; i < PCI_STD_NUM_BARS; i++) {
 		if (psnet->bars[i] && i != psnet->barno)
-			mask |= (1 << i);
+			pcim_iounmap_region(pdev, i);
 	}
-
-	if (mask)
-		pcim_iounmap_regions(pdev, mask);
 }
 
 /* Read SNET config from PCI BAR */
-- 
2.46.0

Re: [PATCH 8/9] vdap: solidrun: Replace deprecated PCI functions

[Christophe JAILLET]

Le 19/08/2024 à 18:51, Philipp Stanner a écrit :
> solidrun utilizes pcim_iomap_regions(), which has been deprecated by the
> PCI subsystem in commit e354bb84a4c1 ("PCI: Deprecate
> pcim_iomap_table(), pcim_iomap_regions_request_all()"), among other
> things because it forces usage of quite a complicated bitmask mechanism.
> The bitmask handling code can entirely be removed by replacing
> pcim_iomap_regions() and pcim_iomap_table().
> 
> Replace pcim_iomap_regions() and pcim_iomap_table() with
> pci_iomap_region().
> 
> Signed-off-by: Philipp Stanner <pstanner@redhat.com>
> ---
>   drivers/vdpa/solidrun/snet_main.c | 47 +++++++++++--------------------
>   1 file changed, 16 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/vdpa/solidrun/snet_main.c b/drivers/vdpa/solidrun/snet_main.c
> index 99428a04068d..abf027ca35e1 100644
> --- a/drivers/vdpa/solidrun/snet_main.c
> +++ b/drivers/vdpa/solidrun/snet_main.c
> @@ -556,33 +556,24 @@ static const struct vdpa_config_ops snet_config_ops = {
>   static int psnet_open_pf_bar(struct pci_dev *pdev, struct psnet *psnet)
>   {
>   	char name[50];
> -	int ret, i, mask = 0;
> +	int i;
> +
> +	snprintf(name, sizeof(name), "psnet[%s]-bars", pci_name(pdev));
> +
>   	/* We don't know which BAR will be used to communicate..
>   	 * We will map every bar with len > 0.
>   	 *
>   	 * Later, we will discover the BAR and unmap all other BARs.
>   	 */
>   	for (i = 0; i < PCI_STD_NUM_BARS; i++) {
> -		if (pci_resource_len(pdev, i))
> -			mask |= (1 << i);
> -	}
> -
> -	/* No BAR can be used.. */
> -	if (!mask) {
> -		SNET_ERR(pdev, "Failed to find a PCI BAR\n");
> -		return -ENODEV;
> -	}
> -
> -	snprintf(name, sizeof(name), "psnet[%s]-bars", pci_name(pdev));
> -	ret = pcim_iomap_regions(pdev, mask, name);
> -	if (ret) {
> -		SNET_ERR(pdev, "Failed to request and map PCI BARs\n");
> -		return ret;
> -	}
> +		if (pci_resource_len(pdev, i)) {
> +			psnet->bars[i] = pcim_iomap_region(pdev, i, name);

Hi,

Unrelated to the patch, but is is safe to have 'name' be on the stack?

pcim_iomap_region()
--> __pcim_request_region()
--> __pcim_request_region_range()
--> request_region() or __request_mem_region()
--> __request_region()
--> __request_region_locked()
--> res->name = name;

So an address on the stack ends in the 'name' field of a "struct resource".

According to a few grep, it looks really unusual.

I don't know if it is used, but it looks strange to me.


If it is an issue, it was apparently already there before this patch.

> +			if (IS_ERR(psnet->bars[i])) {
> +				SNET_ERR(pdev, "Failed to request and map PCI BARs\n");
> +				return PTR_ERR(psnet->bars[i]);
> +			}
> +		}
>   
> -	for (i = 0; i < PCI_STD_NUM_BARS; i++) {
> -		if (mask & (1 << i))
> -			psnet->bars[i] = pcim_iomap_table(pdev)[i];
>   	}
>   
>   	return 0;
> @@ -591,18 +582,15 @@ static int psnet_open_pf_bar(struct pci_dev *pdev, struct psnet *psnet)
>   static int snet_open_vf_bar(struct pci_dev *pdev, struct snet *snet)
>   {
>   	char name[50];
> -	int ret;
>   
>   	snprintf(name, sizeof(name), "snet[%s]-bar", pci_name(pdev));
>   	/* Request and map BAR */
> -	ret = pcim_iomap_regions(pdev, BIT(snet->psnet->cfg.vf_bar), name);
> -	if (ret) {
> +	snet->bar = pcim_iomap_region(pdev, snet->psnet->cfg.vf_bar, name);

Same

Just my 2c.

CJ

> +	if (IS_ERR(snet->bar)) {
>   		SNET_ERR(pdev, "Failed to request and map PCI BAR for a VF\n");
> -		return ret;
> +		return PTR_ERR(snet->bar);
>   	}
>   
> -	snet->bar = pcim_iomap_table(pdev)[snet->psnet->cfg.vf_bar];
> -
>   	return 0;
>   }
>   
> @@ -650,15 +638,12 @@ static int psnet_detect_bar(struct psnet *psnet, u32 off)
>   
>   static void psnet_unmap_unused_bars(struct pci_dev *pdev, struct psnet *psnet)
>   {
> -	int i, mask = 0;
> +	int i;
>   
>   	for (i = 0; i < PCI_STD_NUM_BARS; i++) {
>   		if (psnet->bars[i] && i != psnet->barno)
> -			mask |= (1 << i);
> +			pcim_iounmap_region(pdev, i);
>   	}
> -
> -	if (mask)
> -		pcim_iounmap_regions(pdev, mask);
>   }
>   
>   /* Read SNET config from PCI BAR */

Re: [PATCH 8/9] vdap: solidrun: Replace deprecated PCI functions

[Andy Shevchenko]

On Mon, Aug 19, 2024 at 08:19:28PM +0200, Christophe JAILLET wrote:
> Le 19/08/2024 à 18:51, Philipp Stanner a écrit :


...

> Unrelated to the patch, but is is safe to have 'name' be on the stack?
> 
> pcim_iomap_region()
> --> __pcim_request_region()
> --> __pcim_request_region_range()
> --> request_region() or __request_mem_region()
> --> __request_region()
> --> __request_region_locked()
> --> res->name = name;
> 
> So an address on the stack ends in the 'name' field of a "struct resource".
> 
> According to a few grep, it looks really unusual.
> 
> I don't know if it is used, but it looks strange to me.

It might be used when printing /proc/iomem, but I don't remember by heart.

> If it is an issue, it was apparently already there before this patch.

This series seems to reveal a lot of issues with the probe/remove in many
drivers. I think it's better to make fixes of them before this series for
the sake of easier backporting.

If here is a problem, the devm_kasprintf() should be used.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 8/9] vdap: solidrun: Replace deprecated PCI functions

[Philipp Stanner]

On Mon, 2024-08-19 at 21:34 +0300, Andy Shevchenko wrote:
> On Mon, Aug 19, 2024 at 08:19:28PM +0200, Christophe JAILLET wrote:
> > Le 19/08/2024 à 18:51, Philipp Stanner a écrit :
> 
> 
> ...
> 
> > Unrelated to the patch, but is is safe to have 'name' be on the
> > stack?
> > 
> > pcim_iomap_region()
> > --> __pcim_request_region()
> > --> __pcim_request_region_range()
> > --> request_region() or __request_mem_region()
> > --> __request_region()
> > --> __request_region_locked()
> > --> res->name = name;
> > 
> > So an address on the stack ends in the 'name' field of a "struct
> > resource".
> > 
> > According to a few grep, it looks really unusual.
> > 
> > I don't know if it is used, but it looks strange to me.
> 
> It might be used when printing /proc/iomem, but I don't remember by
> heart.
> 
> > If it is an issue, it was apparently already there before this
> > patch.
> 
> This series seems to reveal a lot of issues with the probe/remove in
> many
> drivers. I think it's better to make fixes of them before this series
> for
> the sake of easier backporting.

Just so we're in sync:
I think the only real bug here so far is the one found by Christophe.

The usages of pci_disable_device(), pcim_iounmap_regions() and the like
in remove() and error unwind paths are not elegant and make devres kind
of useless – but they are not bugs. So I wouldn't backport them.

P.

> 
> If here is a problem, the devm_kasprintf() should be used.
> 

Re: [PATCH 8/9] vdap: solidrun: Replace deprecated PCI functions

[Andy Shevchenko]

On Tue, Aug 20, 2024 at 10:13:40AM +0200, Philipp Stanner wrote:
> On Mon, 2024-08-19 at 21:34 +0300, Andy Shevchenko wrote:
> > On Mon, Aug 19, 2024 at 08:19:28PM +0200, Christophe JAILLET wrote:
> > > Le 19/08/2024 à 18:51, Philipp Stanner a écrit :

...

> > > Unrelated to the patch, but is is safe to have 'name' be on the
> > > stack?
> > > 
> > > pcim_iomap_region()
> > > --> __pcim_request_region()
> > > --> __pcim_request_region_range()
> > > --> request_region() or __request_mem_region()
> > > --> __request_region()
> > > --> __request_region_locked()
> > > --> res->name = name;
> > > 
> > > So an address on the stack ends in the 'name' field of a "struct
> > > resource".
> > > 
> > > According to a few grep, it looks really unusual.
> > > 
> > > I don't know if it is used, but it looks strange to me.
> > 
> > It might be used when printing /proc/iomem, but I don't remember by
> > heart.
> > 
> > > If it is an issue, it was apparently already there before this
> > > patch.
> > 
> > This series seems to reveal a lot of issues with the probe/remove in
> > many
> > drivers. I think it's better to make fixes of them before this series
> > for
> > the sake of easier backporting.
> 
> Just so we're in sync:
> I think the only real bug here so far is the one found by Christophe.
> 
> The usages of pci_disable_device(), pcim_iounmap_regions() and the like
> in remove() and error unwind paths are not elegant and make devres kind
> of useless – but they are not bugs. So I wouldn't backport them.

Agree.

> > If here is a problem, the devm_kasprintf() should be used.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 8/9] vdap: solidrun: Replace deprecated PCI functions

[Philipp Stanner]

On Mon, 2024-08-19 at 20:19 +0200, Christophe JAILLET wrote:
> Le 19/08/2024 à 18:51, Philipp Stanner a écrit :
> > solidrun utilizes pcim_iomap_regions(), which has been deprecated
> > by the
> > PCI subsystem in commit e354bb84a4c1 ("PCI: Deprecate
> > pcim_iomap_table(), pcim_iomap_regions_request_all()"), among other
> > things because it forces usage of quite a complicated bitmask
> > mechanism.
> > The bitmask handling code can entirely be removed by replacing
> > pcim_iomap_regions() and pcim_iomap_table().
> > 
> > Replace pcim_iomap_regions() and pcim_iomap_table() with
> > pci_iomap_region().
> > 
> > Signed-off-by: Philipp Stanner <pstanner@redhat.com>
> > ---
> >   drivers/vdpa/solidrun/snet_main.c | 47 +++++++++++---------------
> > -----
> >   1 file changed, 16 insertions(+), 31 deletions(-)
> > 
> > diff --git a/drivers/vdpa/solidrun/snet_main.c
> > b/drivers/vdpa/solidrun/snet_main.c
> > index 99428a04068d..abf027ca35e1 100644
> > --- a/drivers/vdpa/solidrun/snet_main.c
> > +++ b/drivers/vdpa/solidrun/snet_main.c
> > @@ -556,33 +556,24 @@ static const struct vdpa_config_ops
> > snet_config_ops = {
> >   static int psnet_open_pf_bar(struct pci_dev *pdev, struct psnet
> > *psnet)
> >   {
> >   	char name[50];
> > -	int ret, i, mask = 0;
> > +	int i;
> > +
> > +	snprintf(name, sizeof(name), "psnet[%s]-bars",
> > pci_name(pdev));
> > +
> >   	/* We don't know which BAR will be used to communicate..
> >   	 * We will map every bar with len > 0.
> >   	 *
> >   	 * Later, we will discover the BAR and unmap all other
> > BARs.
> >   	 */
> >   	for (i = 0; i < PCI_STD_NUM_BARS; i++) {
> > -		if (pci_resource_len(pdev, i))
> > -			mask |= (1 << i);
> > -	}
> > -
> > -	/* No BAR can be used.. */
> > -	if (!mask) {
> > -		SNET_ERR(pdev, "Failed to find a PCI BAR\n");
> > -		return -ENODEV;
> > -	}
> > -
> > -	snprintf(name, sizeof(name), "psnet[%s]-bars",
> > pci_name(pdev));
> > -	ret = pcim_iomap_regions(pdev, mask, name);
> > -	if (ret) {
> > -		SNET_ERR(pdev, "Failed to request and map PCI
> > BARs\n");
> > -		return ret;
> > -	}
> > +		if (pci_resource_len(pdev, i)) {
> > +			psnet->bars[i] = pcim_iomap_region(pdev,
> > i, name);
> 
> Hi,
> 
> Unrelated to the patch, but is is safe to have 'name' be on the
> stack?
> 
> pcim_iomap_region()
> --> __pcim_request_region()
> --> __pcim_request_region_range()
> --> request_region() or __request_mem_region()
> --> __request_region()
> --> __request_region_locked()
> --> res->name = name;
> 
> So an address on the stack ends in the 'name' field of a "struct
> resource".

Oh oh...

> 
> According to a few grep, it looks really unusual.
> 
> I don't know if it is used, but it looks strange to me.


I have seen it used in the kernel ringbuffer log when you try to
request something that's already owned. I think it's here, right in
__request_region_locked():

/*
 * mm/hmm.c reserves physical addresses which then
 * become unavailable to other users.  Conflicts are
 * not expected.  Warn to aid debugging if encountered.
 */
if (conflict->desc == IORES_DESC_DEVICE_PRIVATE_MEMORY) {
	pr_warn("Unaddressable device %s %pR conflicts with %pR",
		conflict->name, conflict, res);
}


Assuming I interpret the code correctly:
The conflicting resource is found when a new caller (e.g. another
driver) tries to get the same region. So conflict->name on the original
requester's stack is by now gone and you do get UB.

Very unlikely UB, since only rarely drivers race for the same resource,
but still UB.

But there's also a few other places. Grep for "conflict->name".

> 
> 
> If it is an issue, it was apparently already there before this patch.

I think this has to be fixed.

Question would just be whether one wants to fix it locally in this
driver, or prevent it from happening globally by making the common
infrastructure copy the string.


P.


> 
> > +			if (IS_ERR(psnet->bars[i])) {
> > +				SNET_ERR(pdev, "Failed to request
> > and map PCI BARs\n");
> > +				return PTR_ERR(psnet->bars[i]);
> > +			}
> > +		}
> >   
> > -	for (i = 0; i < PCI_STD_NUM_BARS; i++) {
> > -		if (mask & (1 << i))
> > -			psnet->bars[i] =
> > pcim_iomap_table(pdev)[i];
> >   	}
> >   
> >   	return 0;
> > @@ -591,18 +582,15 @@ static int psnet_open_pf_bar(struct pci_dev
> > *pdev, struct psnet *psnet)
> >   static int snet_open_vf_bar(struct pci_dev *pdev, struct snet
> > *snet)
> >   {
> >   	char name[50];
> > -	int ret;
> >   
> >   	snprintf(name, sizeof(name), "snet[%s]-bar",
> > pci_name(pdev));
> >   	/* Request and map BAR */
> > -	ret = pcim_iomap_regions(pdev, BIT(snet->psnet-
> > >cfg.vf_bar), name);
> > -	if (ret) {
> > +	snet->bar = pcim_iomap_region(pdev, snet->psnet-
> > >cfg.vf_bar, name);
> 
> Same
> 
> Just my 2c.
> 
> CJ
> 
> > +	if (IS_ERR(snet->bar)) {
> >   		SNET_ERR(pdev, "Failed to request and map PCI BAR
> > for a VF\n");
> > -		return ret;
> > +		return PTR_ERR(snet->bar);
> >   	}
> >   
> > -	snet->bar = pcim_iomap_table(pdev)[snet->psnet-
> > >cfg.vf_bar];
> > -
> >   	return 0;
> >   }
> >   
> > @@ -650,15 +638,12 @@ static int psnet_detect_bar(struct psnet
> > *psnet, u32 off)
> >   
> >   static void psnet_unmap_unused_bars(struct pci_dev *pdev, struct
> > psnet *psnet)
> >   {
> > -	int i, mask = 0;
> > +	int i;
> >   
> >   	for (i = 0; i < PCI_STD_NUM_BARS; i++) {
> >   		if (psnet->bars[i] && i != psnet->barno)
> > -			mask |= (1 << i);
> > +			pcim_iounmap_region(pdev, i);
> >   	}
> > -
> > -	if (mask)
> > -		pcim_iounmap_regions(pdev, mask);
> >   }
> >   
> >   /* Read SNET config from PCI BAR */
> 
> 

Re: [PATCH 8/9] vdap: solidrun: Replace deprecated PCI functions

[Christophe JAILLET]

Le 20/08/2024 à 10:09, Philipp Stanner a écrit :
>>> @@ -556,33 +556,24 @@ static const struct vdpa_config_ops
>>> snet_config_ops = {
>>>    static int psnet_open_pf_bar(struct pci_dev *pdev, struct psnet
>>> *psnet)
>>>    {
>>>    	char name[50];
>>> -	int ret, i, mask = 0;
>>> +	int i;
>>> +
>>> +	snprintf(name, sizeof(name), "psnet[%s]-bars",
>>> pci_name(pdev));
>>> +
>>>    	/* We don't know which BAR will be used to communicate..
>>>    	 * We will map every bar with len > 0.
>>>    	 *
>>>    	 * Later, we will discover the BAR and unmap all other
>>> BARs.
>>>    	 */
>>>    	for (i = 0; i < PCI_STD_NUM_BARS; i++) {
>>> -		if (pci_resource_len(pdev, i))
>>> -			mask |= (1 << i);
>>> -	}
>>> -
>>> -	/* No BAR can be used.. */
>>> -	if (!mask) {
>>> -		SNET_ERR(pdev, "Failed to find a PCI BAR\n");
>>> -		return -ENODEV;
>>> -	}
>>> -
>>> -	snprintf(name, sizeof(name), "psnet[%s]-bars",
>>> pci_name(pdev));
>>> -	ret = pcim_iomap_regions(pdev, mask, name);
>>> -	if (ret) {
>>> -		SNET_ERR(pdev, "Failed to request and map PCI
>>> BARs\n");
>>> -		return ret;
>>> -	}
>>> +		if (pci_resource_len(pdev, i)) {
>>> +			psnet->bars[i] = pcim_iomap_region(pdev,
>>> i, name);
>>
>> Hi,
>>
>> Unrelated to the patch, but is is safe to have 'name' be on the
>> stack?
>>
>> pcim_iomap_region()
>> --> __pcim_request_region()
>> --> __pcim_request_region_range()
>> --> request_region() or __request_mem_region()
>> --> __request_region()
>> --> __request_region_locked()
>> --> res->name = name;
>>
>> So an address on the stack ends in the 'name' field of a "struct
>> resource".
> 
> Oh oh...
> 
>>
>> According to a few grep, it looks really unusual.
>>
>> I don't know if it is used, but it looks strange to me.
> 
> 
> I have seen it used in the kernel ringbuffer log when you try to
> request something that's already owned. I think it's here, right in
> __request_region_locked():
> 
> /*
>   * mm/hmm.c reserves physical addresses which then
>   * become unavailable to other users.  Conflicts are
>   * not expected.  Warn to aid debugging if encountered.
>   */
> if (conflict->desc == IORES_DESC_DEVICE_PRIVATE_MEMORY) {
> 	pr_warn("Unaddressable device %s %pR conflicts with %pR",
> 		conflict->name, conflict, res);
> }
> 
> 
> Assuming I interpret the code correctly:
> The conflicting resource is found when a new caller (e.g. another
> driver) tries to get the same region. So conflict->name on the original
> requester's stack is by now gone and you do get UB.
> 
> Very unlikely UB, since only rarely drivers race for the same resource,
> but still UB.
> 
> But there's also a few other places. Grep for "conflict->name".
> 
>>
>>
>> If it is an issue, it was apparently already there before this patch.
> 
> I think this has to be fixed.
> 
> Question would just be whether one wants to fix it locally in this
> driver, or prevent it from happening globally by making the common
> infrastructure copy the string.
> 
> 
> P.
> 

Not a perfect script, but the below coccinelle script only find this 
place, so I would +1 only fixing things here only.

Agree?

CJ



@@
identifier name;
expression x;
constant N;
@@
	char name[N];
	...
(
*	pcim_iomap_region(..., name, ...);
|
*	pcim_iomap_regions(..., name, ...);
|
*	request_region(..., name, ...);
|
*	x = pcim_iomap_region(..., name, ...);
|
*	x = pcim_iomap_regions(..., name, ...);
|
*	x = request_region(..., name, ...);
)

Re: [PATCH 8/9] vdap: solidrun: Replace deprecated PCI functions

[Philipp Stanner]

On Tue, 2024-08-20 at 12:50 +0200, Christophe JAILLET wrote:
> Le 20/08/2024 à 10:09, Philipp Stanner a écrit :
> > > > @@ -556,33 +556,24 @@ static const struct vdpa_config_ops
> > > > snet_config_ops = {
> > > >    static int psnet_open_pf_bar(struct pci_dev *pdev, struct
> > > > psnet
> > > > *psnet)
> > > >    {
> > > >    	char name[50];
> > > > -	int ret, i, mask = 0;
> > > > +	int i;
> > > > +
> > > > +	snprintf(name, sizeof(name), "psnet[%s]-bars",
> > > > pci_name(pdev));
> > > > +
> > > >    	/* We don't know which BAR will be used to
> > > > communicate..
> > > >    	 * We will map every bar with len > 0.
> > > >    	 *
> > > >    	 * Later, we will discover the BAR and unmap all other
> > > > BARs.
> > > >    	 */
> > > >    	for (i = 0; i < PCI_STD_NUM_BARS; i++) {
> > > > -		if (pci_resource_len(pdev, i))
> > > > -			mask |= (1 << i);
> > > > -	}
> > > > -
> > > > -	/* No BAR can be used.. */
> > > > -	if (!mask) {
> > > > -		SNET_ERR(pdev, "Failed to find a PCI BAR\n");
> > > > -		return -ENODEV;
> > > > -	}
> > > > -
> > > > -	snprintf(name, sizeof(name), "psnet[%s]-bars",
> > > > pci_name(pdev));
> > > > -	ret = pcim_iomap_regions(pdev, mask, name);
> > > > -	if (ret) {
> > > > -		SNET_ERR(pdev, "Failed to request and map PCI
> > > > BARs\n");
> > > > -		return ret;
> > > > -	}
> > > > +		if (pci_resource_len(pdev, i)) {
> > > > +			psnet->bars[i] =
> > > > pcim_iomap_region(pdev,
> > > > i, name);
> > > 
> > > Hi,
> > > 
> > > Unrelated to the patch, but is is safe to have 'name' be on the
> > > stack?
> > > 
> > > pcim_iomap_region()
> > > --> __pcim_request_region()
> > > --> __pcim_request_region_range()
> > > --> request_region() or __request_mem_region()
> > > --> __request_region()
> > > --> __request_region_locked()
> > > --> res->name = name;
> > > 
> > > So an address on the stack ends in the 'name' field of a "struct
> > > resource".
> > 
> > Oh oh...
> > 
> > > 
> > > According to a few grep, it looks really unusual.
> > > 
> > > I don't know if it is used, but it looks strange to me.
> > 
> > 
> > I have seen it used in the kernel ringbuffer log when you try to
> > request something that's already owned. I think it's here, right in
> > __request_region_locked():
> > 
> > /*
> >   * mm/hmm.c reserves physical addresses which then
> >   * become unavailable to other users.  Conflicts are
> >   * not expected.  Warn to aid debugging if encountered.
> >   */
> > if (conflict->desc == IORES_DESC_DEVICE_PRIVATE_MEMORY) {
> > 	pr_warn("Unaddressable device %s %pR conflicts with %pR",
> > 		conflict->name, conflict, res);
> > }
> > 
> > 
> > Assuming I interpret the code correctly:
> > The conflicting resource is found when a new caller (e.g. another
> > driver) tries to get the same region. So conflict->name on the
> > original
> > requester's stack is by now gone and you do get UB.
> > 
> > Very unlikely UB, since only rarely drivers race for the same
> > resource,
> > but still UB.
> > 
> > But there's also a few other places. Grep for "conflict->name".
> > 
> > > 
> > > 
> > > If it is an issue, it was apparently already there before this
> > > patch.
> > 
> > I think this has to be fixed.
> > 
> > Question would just be whether one wants to fix it locally in this
> > driver, or prevent it from happening globally by making the common
> > infrastructure copy the string.
> > 
> > 
> > P.
> > 
> 
> Not a perfect script, but the below coccinelle script only find this 
> place, so I would +1 only fixing things here only.
> 
> Agree?

Yup, sounds good. Copying the string would cause trouble (GFP flags)
anyways.

I'll provide a fix in v2.

Thanks,
P.

> 
> CJ
> 
> 
> 
> @@
> identifier name;
> expression x;
> constant N;
> @@
> 	char name[N];
> 	...
> (
> *	pcim_iomap_region(..., name, ...);
> > 
> *	pcim_iomap_regions(..., name, ...);
> > 
> *	request_region(..., name, ...);
> > 
> *	x = pcim_iomap_region(..., name, ...);
> > 
> *	x = pcim_iomap_regions(..., name, ...);
> > 
> *	x = request_region(..., name, ...);
> )
> 
> 

Re: [PATCH 8/9] vdap: solidrun: Replace deprecated PCI functions

[Andy Shevchenko]

On Mon, Aug 19, 2024 at 06:51:48PM +0200, Philipp Stanner wrote:
> solidrun utilizes pcim_iomap_regions(), which has been deprecated by the
> PCI subsystem in commit e354bb84a4c1 ("PCI: Deprecate
> pcim_iomap_table(), pcim_iomap_regions_request_all()"), among other
> things because it forces usage of quite a complicated bitmask mechanism.
> The bitmask handling code can entirely be removed by replacing
> pcim_iomap_regions() and pcim_iomap_table().
> 
> Replace pcim_iomap_regions() and pcim_iomap_table() with
> pci_iomap_region().

...

> -	int ret, i, mask = 0;
> +	int i;

Make it signed?

...

>  	for (i = 0; i < PCI_STD_NUM_BARS; i++) {
> +		if (pci_resource_len(pdev, i)) {
> +			psnet->bars[i] = pcim_iomap_region(pdev, i, name);
> +			if (IS_ERR(psnet->bars[i])) {
> +				SNET_ERR(pdev, "Failed to request and map PCI BARs\n");
> +				return PTR_ERR(psnet->bars[i]);
> +			}
> +		}

>  

Blank line leftover.

>  	}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 8/9] vdap: solidrun: Replace deprecated PCI functions

[Andy Shevchenko]

On Mon, Aug 19, 2024 at 09:32:00PM +0300, Andy Shevchenko wrote:
> On Mon, Aug 19, 2024 at 06:51:48PM +0200, Philipp Stanner wrote:

...

> >  	for (i = 0; i < PCI_STD_NUM_BARS; i++) {
> > +		if (pci_resource_len(pdev, i)) {

Btw, looking at this you may invert the conditional

	for (i = 0; i < PCI_STD_NUM_BARS; i++) {
		if (!pci_resource_len(pdev, i))
			continue;

It might make patch neater.

> > +			psnet->bars[i] = pcim_iomap_region(pdev, i, name);
> > +			if (IS_ERR(psnet->bars[i])) {
> > +				SNET_ERR(pdev, "Failed to request and map PCI BARs\n");
> > +				return PTR_ERR(psnet->bars[i]);
> > +			}
> > +		}

> >  
> 
> Blank line leftover.
> 
> >  	}

-- 
With Best Regards,
Andy Shevchenko

[PATCH 9/9] PCI: Remove pcim_iounmap_regions()

[Philipp Stanner]

All users of pcim_iounmap_regions() have been removed by now.

Remove pcim_iounmap_regions().

Signed-off-by: Philipp Stanner <pstanner@redhat.com>
---
 .../driver-api/driver-model/devres.rst        |  1 -
 drivers/pci/devres.c                          | 21 -------------------
 include/linux/pci.h                           |  1 -
 3 files changed, 23 deletions(-)

diff --git a/Documentation/driver-api/driver-model/devres.rst b/Documentation/driver-api/driver-model/devres.rst
index ac9ee7441887..525f08694984 100644
--- a/Documentation/driver-api/driver-model/devres.rst
+++ b/Documentation/driver-api/driver-model/devres.rst
@@ -397,7 +397,6 @@ PCI
   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
   pcim_pin_device()		: keep PCI device enabled after release
   pcim_set_mwi()		: enable Memory-Write-Invalidate PCI transaction
 
diff --git a/drivers/pci/devres.c b/drivers/pci/devres.c
index 30c813766e8b..a1689100a535 100644
--- a/drivers/pci/devres.c
+++ b/drivers/pci/devres.c
@@ -1014,27 +1014,6 @@ int pcim_iomap_regions_request_all(struct pci_dev *pdev, int mask,
 }
 EXPORT_SYMBOL(pcim_iomap_regions_request_all);
 
-/**
- * pcim_iounmap_regions - Unmap and release PCI BARs
- * @pdev: PCI device to map IO resources for
- * @mask: Mask of BARs to unmap and release
- *
- * Unmap and release regions specified by @mask.
- */
-void pcim_iounmap_regions(struct pci_dev *pdev, int mask)
-{
-	int i;
-
-	for (i = 0; i < PCI_STD_NUM_BARS; i++) {
-		if (!mask_contains_bar(mask, i))
-			continue;
-
-		pcim_iounmap_region(pdev, i);
-		pcim_remove_bar_from_legacy_table(pdev, i);
-	}
-}
-EXPORT_SYMBOL(pcim_iounmap_regions);
-
 /**
  * pcim_iomap_range - Create a ranged __iomap mapping within a PCI BAR
  * @pdev: PCI device to map IO resources for
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 7de75900854a..4eee09624932 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -2302,7 +2302,6 @@ void pcim_iounmap_region(struct pci_dev *pdev, int bar);
 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.46.0

Re: [PATCH 9/9] PCI: Remove pcim_iounmap_regions()

[Andy Shevchenko]

On Mon, Aug 19, 2024 at 06:51:49PM +0200, Philipp Stanner wrote:
> All users of pcim_iounmap_regions() have been removed by now.
> 
> Remove pcim_iounmap_regions().

Reviewed-by: Andy Shevchenko <andy@kernel.org>

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 9/9] PCI: Remove pcim_iounmap_regions()

[Damien Le Moal]

On 8/20/24 01:51, Philipp Stanner wrote:
> All users of pcim_iounmap_regions() have been removed by now.
> 
> Remove pcim_iounmap_regions().
> 
> Signed-off-by: Philipp Stanner <pstanner@redhat.com>

Reviewed-by: Damien Le Moal <dlemoal@kernel.org>

-- 
Damien Le Moal
Western Digital Research