mbox series

[v3,0/9] PCI: Remove pcim_iounmap_regions()

Message ID 20240822134744.44919-1-pstanner@redhat.com
Headers show
Series PCI: Remove pcim_iounmap_regions() | expand

Message

Philipp Stanner Aug. 22, 2024, 1:47 p.m. UTC
Changes in v3:
  - fpga/dfl-pci.c: remove now surplus wrapper around
    pcim_iomap_region(). (Andy)
  - block: mtip32xx: remove now surplus label. (Andy)
  - vdpa: solidrun: Bugfix: Include forgotten place where stack UB
    occurs. (Andy, Christophe)
  - Some minor wording improvements in commit messages. (Me)

Changes in v2:
  - Add a fix for the UB stack usage bug in vdap/solidrun. Separate
    patch, put stable kernel on CC. (Christophe, Andy).
  - Drop unnecessary pcim_release_region() in mtip32xx (Andy)
  - Consequently, drop patch "PCI: Make pcim_release_region() a public
    function", since there's no user anymore. (obsoletes the squash
    requested by Damien).
  - vdap/solidrun:
    • make 'i' an 'unsigned short' (Andy, me)
    • Use 'continue' to simplify loop (Andy)
    • Remove leftover blank line
  - Apply given Reviewed- / acked-bys (Andy, Damien, Bartosz)


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_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
  vdpa: solidrun: Fix UB bug with devres
  vdap: solidrun: Replace deprecated PCI functions
  PCI: Remove pcim_iounmap_regions()

 .../driver-api/driver-model/devres.rst        |  1 -
 drivers/block/mtip32xx/mtip32xx.c             | 16 +++---
 drivers/fpga/dfl-pci.c                        | 16 ++----
 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                          | 24 +-------
 drivers/vdpa/solidrun/snet_main.c             | 57 ++++++++-----------
 include/linux/pci.h                           |  2 +-
 10 files changed, 61 insertions(+), 122 deletions(-)

Comments

Christophe JAILLET Aug. 22, 2024, 2:34 p.m. UTC | #1
Le 22/08/2024 à 15:47, Philipp Stanner a écrit :
> In psnet_open_pf_bar() and snet_open_vf_bar() a string later passed to
> pcim_iomap_regions() is placed on the stack. Neither
> pcim_iomap_regions() nor the functions it calls copy that string.
> 
> Should the string later ever be used, this, consequently, causes
> undefined behavior since the stack frame will by then have disappeared.
> 
> Fix the bug by allocating the strings on the heap through
> devm_kasprintf().
> 
> Cc: stable@vger.kernel.org	# v6.3
> Fixes: 51a8f9d7f587 ("virtio: vdpa: new SolidNET DPU driver.")
> Reported-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> Closes: https://lore.kernel.org/all/74e9109a-ac59-49e2-9b1d-d825c9c9f891@wanadoo.fr/
> Suggested-by: Andy Shevchenko <andy@kernel.org>
> Signed-off-by: Philipp Stanner <pstanner@redhat.com>
> ---
>   drivers/vdpa/solidrun/snet_main.c | 13 +++++++++----
>   1 file changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/vdpa/solidrun/snet_main.c b/drivers/vdpa/solidrun/snet_main.c
> index 99428a04068d..67235f6190ef 100644
> --- a/drivers/vdpa/solidrun/snet_main.c
> +++ b/drivers/vdpa/solidrun/snet_main.c
> @@ -555,7 +555,7 @@ 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];
> +	char *name;
>   	int ret, i, mask = 0;
>   	/* We don't know which BAR will be used to communicate..
>   	 * We will map every bar with len > 0.
> @@ -573,7 +573,10 @@ static int psnet_open_pf_bar(struct pci_dev *pdev, struct psnet *psnet)
>   		return -ENODEV;
>   	}
>   
> -	snprintf(name, sizeof(name), "psnet[%s]-bars", pci_name(pdev));
> +	name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "psnet[%s]-bars", pci_name(pdev));
> +	if (!name)
> +		return -ENOMEM;
> +
>   	ret = pcim_iomap_regions(pdev, mask, name);
>   	if (ret) {
>   		SNET_ERR(pdev, "Failed to request and map PCI BARs\n");
> @@ -590,10 +593,12 @@ 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];
> +	char *name;
>   	int ret;
>   
> -	snprintf(name, sizeof(name), "snet[%s]-bar", pci_name(pdev));
> +	name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "psnet[%s]-bars", pci_name(pdev));

s/psnet/snet/

> +	if (!name)
> +		return -ENOMEM;
>   	/* Request and map BAR */
>   	ret = pcim_iomap_regions(pdev, BIT(snet->psnet->cfg.vf_bar), name);
>   	if (ret) {
Andy Shevchenko Aug. 22, 2024, 2:47 p.m. UTC | #2
On Thu, Aug 22, 2024 at 03:47:39PM +0200, Philipp Stanner wrote:
> In psnet_open_pf_bar() and snet_open_vf_bar() a string later passed to
> pcim_iomap_regions() is placed on the stack. Neither
> pcim_iomap_regions() nor the functions it calls copy that string.
> 
> Should the string later ever be used, this, consequently, causes
> undefined behavior since the stack frame will by then have disappeared.
> 
> Fix the bug by allocating the strings on the heap through
> devm_kasprintf().

...

> -	snprintf(name, sizeof(name), "psnet[%s]-bars", pci_name(pdev));
> +	name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "psnet[%s]-bars", pci_name(pdev));
> +	if (!name)
> +		return -ENOMEM;
> +
>  	ret = pcim_iomap_regions(pdev, mask, name);

...

> -	snprintf(name, sizeof(name), "snet[%s]-bar", pci_name(pdev));
> +	name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "psnet[%s]-bars", pci_name(pdev));
> +	if (!name)
> +		return -ENOMEM;

+ Blank line as in the above snippet?

>  	/* Request and map BAR */
>  	ret = pcim_iomap_regions(pdev, BIT(snet->psnet->cfg.vf_bar), name);
Andy Shevchenko Aug. 22, 2024, 2:50 p.m. UTC | #3
On Thu, Aug 22, 2024 at 03:47:32PM +0200, Philipp Stanner wrote:

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

For the non-commented ones (by me or by others)
Reviewed-by: Andy Shevchenko <andy@kernel.org>
Philipp Stanner Aug. 26, 2024, 6:55 a.m. UTC | #4
On Thu, 2024-08-22 at 16:34 +0200, Christophe JAILLET wrote:
> Le 22/08/2024 à 15:47, Philipp Stanner a écrit :
> > In psnet_open_pf_bar() and snet_open_vf_bar() a string later passed
> > to
> > pcim_iomap_regions() is placed on the stack. Neither
> > pcim_iomap_regions() nor the functions it calls copy that string.
> > 
> > Should the string later ever be used, this, consequently, causes
> > undefined behavior since the stack frame will by then have
> > disappeared.
> > 
> > Fix the bug by allocating the strings on the heap through
> > devm_kasprintf().
> > 
> > Cc: stable@vger.kernel.org	# v6.3
> > Fixes: 51a8f9d7f587 ("virtio: vdpa: new SolidNET DPU driver.")
> > Reported-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> > Closes:
> > https://lore.kernel.org/all/74e9109a-ac59-49e2-9b1d-d825c9c9f891@wanadoo.fr/
> > Suggested-by: Andy Shevchenko <andy@kernel.org>
> > Signed-off-by: Philipp Stanner <pstanner@redhat.com>
> > ---
> >   drivers/vdpa/solidrun/snet_main.c | 13 +++++++++----
> >   1 file changed, 9 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/vdpa/solidrun/snet_main.c
> > b/drivers/vdpa/solidrun/snet_main.c
> > index 99428a04068d..67235f6190ef 100644
> > --- a/drivers/vdpa/solidrun/snet_main.c
> > +++ b/drivers/vdpa/solidrun/snet_main.c
> > @@ -555,7 +555,7 @@ 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];
> > +	char *name;
> >   	int ret, i, mask = 0;
> >   	/* We don't know which BAR will be used to communicate..
> >   	 * We will map every bar with len > 0.
> > @@ -573,7 +573,10 @@ static int psnet_open_pf_bar(struct pci_dev
> > *pdev, struct psnet *psnet)
> >   		return -ENODEV;
> >   	}
> >   
> > -	snprintf(name, sizeof(name), "psnet[%s]-bars",
> > pci_name(pdev));
> > +	name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "psnet[%s]-
> > bars", pci_name(pdev));
> > +	if (!name)
> > +		return -ENOMEM;
> > +
> >   	ret = pcim_iomap_regions(pdev, mask, name);
> >   	if (ret) {
> >   		SNET_ERR(pdev, "Failed to request and map PCI
> > BARs\n");
> > @@ -590,10 +593,12 @@ 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];
> > +	char *name;
> >   	int ret;
> >   
> > -	snprintf(name, sizeof(name), "snet[%s]-bar",
> > pci_name(pdev));
> > +	name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "psnet[%s]-
> > bars", pci_name(pdev));
> 
> s/psnet/snet/

sharp eyes ;)

Thx,
P.

> 
> > +	if (!name)
> > +		return -ENOMEM;
> >   	/* Request and map BAR */
> >   	ret = pcim_iomap_regions(pdev, BIT(snet->psnet-
> > >cfg.vf_bar), name);
> >   	if (ret) {
>
Philipp Stanner Aug. 26, 2024, 2:51 p.m. UTC | #5
On Thu, 2024-08-22 at 17:44 +0300, Andy Shevchenko wrote:
> On Thu, Aug 22, 2024 at 03:47:37PM +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().
> 
> ...
> 
> > -	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];
> > +	}
> 
> Perhaps
> 
> 	clock->reg_base = pcim_iomap_region(pdev, PCI_PTP_BAR_NO,
> pci_name(pdev));
> 	err = PTR_ERR_OR_ZERO(clock->reg_base);
> 	if (err)
> 		goto error_free;
> 
> This will make your patch smaller and neater.
> 
> P.S. Do you use --histogram diff algo when preparing patches?

So far not.
Should one do that?

P.


>
Andy Shevchenko Aug. 26, 2024, 3:41 p.m. UTC | #6
On Mon, Aug 26, 2024 at 5:51 PM Philipp Stanner <pstanner@redhat.com> wrote:
> On Thu, 2024-08-22 at 17:44 +0300, Andy Shevchenko wrote:
> > On Thu, Aug 22, 2024 at 03:47:37PM +0200, Philipp Stanner wrote:

...

> > > -   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];
> > > +   }
> >
> > Perhaps
> >
> >       clock->reg_base = pcim_iomap_region(pdev, PCI_PTP_BAR_NO,
> > pci_name(pdev));
> >       err = PTR_ERR_OR_ZERO(clock->reg_base);
> >       if (err)
> >               goto error_free;
> >
> > This will make your patch smaller and neater.
> >
> > P.S. Do you use --histogram diff algo when preparing patches?
>
> So far not.
> Should one do that?

Id doesn't alter your code, it's in addition to what I suggested, but
as Linus shared that there is no reason to avoid using --histogram not
only in Linux kernel, but in general as it produces more
human-readable diff:s.
Philipp Stanner Aug. 26, 2024, 3:52 p.m. UTC | #7
On Mon, 2024-08-26 at 18:41 +0300, Andy Shevchenko wrote:
> On Mon, Aug 26, 2024 at 5:51 PM Philipp Stanner <pstanner@redhat.com>
> wrote:
> > On Thu, 2024-08-22 at 17:44 +0300, Andy Shevchenko wrote:
> > > On Thu, Aug 22, 2024 at 03:47:37PM +0200, Philipp Stanner wrote:
> 
> ...
> 
> > > > -   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];
> > > > +   }
> > > 
> > > Perhaps
> > > 
> > >       clock->reg_base = pcim_iomap_region(pdev, PCI_PTP_BAR_NO,
> > > pci_name(pdev));
> > >       err = PTR_ERR_OR_ZERO(clock->reg_base);
> > >       if (err)
> > >               goto error_free;
> > > 
> > > This will make your patch smaller and neater.
> > > 
> > > P.S. Do you use --histogram diff algo when preparing patches?
> > 
> > So far not.
> > Should one do that?
> 
> Id doesn't alter your code, it's in addition to what I suggested, but
> as Linus shared that there is no reason to avoid using --histogram
> not
> only in Linux kernel, but in general as it produces more
> human-readable diff:s.

If the Boss says so, one can surely do that \o/

Though if it has 0 disadvantages I'd propose proposing to the git-devs
to make it the default.


P.

>
Andy Shevchenko Aug. 26, 2024, 5:12 p.m. UTC | #8
On Mon, Aug 26, 2024 at 6:52 PM Philipp Stanner <pstanner@redhat.com> wrote:
> On Mon, 2024-08-26 at 18:41 +0300, Andy Shevchenko wrote:

...

> Though if it has 0 disadvantages I'd propose proposing to the git-devs
> to make it the default.

It's slower. so the people from https://occ.deadnet.se/about/ won't be happy.
And more power consuming, so maybe not so environment friendly after all :-P