pci_request_regions() is a hybrid function which becomes managed if
pcim_enable_device() was called before. This hybrid nature is deprecated
and should not be used anymore.

Replace pci_request_regions() with the always-managed function
pcim_request_all_regions().

Remove the goto jump to pci_release_regions(), since pcim_ functions
clean up automatically.

Signed-off-by: Philipp Stanner <phasta@kernel.org>
---
 sound/soc/intel/avs/core.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/sound/soc/intel/avs/core.c b/sound/soc/intel/avs/core.c
index 8fbf33e30dfc..dafe46973146 100644
--- a/sound/soc/intel/avs/core.c
+++ b/sound/soc/intel/avs/core.c
@@ -445,7 +445,7 @@ static int avs_pci_probe(struct pci_dev *pci, const struct pci_device_id *id)
 		return ret;
 	}
 
-	ret = pci_request_regions(pci, "AVS HDAudio");
+	ret = pcim_request_all_regions(pci, "AVS HDAudio");
 	if (ret < 0)
 		return ret;
 
@@ -454,8 +454,7 @@ static int avs_pci_probe(struct pci_dev *pci, const struct pci_device_id *id)
 	bus->remap_addr = pci_ioremap_bar(pci, 0);
 	if (!bus->remap_addr) {
 		dev_err(bus->dev, "ioremap error\n");
-		ret = -ENXIO;
-		goto err_remap_bar0;
+		return -ENXIO;
 	}
 
 	adev->dsp_ba = pci_ioremap_bar(pci, 4);
@@ -512,8 +511,6 @@ static int avs_pci_probe(struct pci_dev *pci, const struct pci_device_id *id)
 	iounmap(adev->dsp_ba);
 err_remap_bar4:
 	iounmap(bus->remap_addr);
-err_remap_bar0:
-	pci_release_regions(pci);
 	return ret;
 }
 
-- 
2.48.1

On Wed, Apr 16, 2025 at 03:12:12PM +0200, Philipp Stanner wrote:
> pci_request_regions() is a hybrid function which becomes managed if
> pcim_enable_device() was called before. This hybrid nature is deprecated
> and should not be used anymore.
> 
> Replace pci_request_regions() with the always-managed function
> pcim_request_all_regions().
> 
> Remove the goto jump to pci_release_regions(), since pcim_ functions
> clean up automatically.

...

>  	bus->remap_addr = pci_ioremap_bar(pci, 0);
>  	if (!bus->remap_addr) {
>  		dev_err(bus->dev, "ioremap error\n");
> -		ret = -ENXIO;
> -		goto err_remap_bar0;
> +		return -ENXIO;

Here and everywhere else these can now be converted to dev_err_probe().
Are you planning to do so?

...

>  err_remap_bar4:
>  	iounmap(bus->remap_addr);

This looks weird if the driver already is using pcim_enable_device().
Doesn't this look to you as an existing bug?

> -err_remap_bar0:
> -	pci_release_regions(pci);
>  	return ret;

-- 
With Best Regards,
Andy Shevchenko

+Cc Bjorn, Krzysztof

On Wed, 2025-04-16 at 18:39 +0300, Andy Shevchenko wrote:
> On Wed, Apr 16, 2025 at 03:12:12PM +0200, Philipp Stanner wrote:
> > pci_request_regions() is a hybrid function which becomes managed if
> > pcim_enable_device() was called before. This hybrid nature is
> > deprecated
> > and should not be used anymore.
> > 
> > Replace pci_request_regions() with the always-managed function
> > pcim_request_all_regions().
> > 
> > Remove the goto jump to pci_release_regions(), since pcim_
> > functions
> > clean up automatically.
> 
> ...
> 
> >  	bus->remap_addr = pci_ioremap_bar(pci, 0);
> >  	if (!bus->remap_addr) {
> >  		dev_err(bus->dev, "ioremap error\n");
> > -		ret = -ENXIO;
> > -		goto err_remap_bar0;
> > +		return -ENXIO;
> 
> Here and everywhere else these can now be converted to
> dev_err_probe().
> Are you planning to do so?

I want to do what's necessary to get PCI in better shape, since that's
what the GPUs and accelerators we / I care about use :)

IOW, I want pci_request_regions() removed from here.

> 
> ...
> 
> >  err_remap_bar4:
> >  	iounmap(bus->remap_addr);
> 
> This looks weird if the driver already is using pcim_enable_device().
> Doesn't this look to you as an existing bug?

I looked briefly at it and it doesn't appear like an obvious bug to me
because the drivers uses the (very old? deprecated?) pci_ioremap_bar().

In any case the driver doesn't set up any devres callback, so has to
iounmap() manually.

@Bjorn:
Any comments on pci_ioremap_bar()? Should we mark that as deprecated?

P.

> 
> > -err_remap_bar0:
> > -	pci_release_regions(pci);
> >  	return ret;
> 

On Wed, Apr 16, 2025 at 06:25:52PM +0200, Philipp Stanner wrote:
> On Wed, 2025-04-16 at 18:39 +0300, Andy Shevchenko wrote:
> > On Wed, Apr 16, 2025 at 03:12:12PM +0200, Philipp Stanner wrote:

...

> > >  	bus->remap_addr = pci_ioremap_bar(pci, 0);
> > >  	if (!bus->remap_addr) {
> > >  		dev_err(bus->dev, "ioremap error\n");
> > > -		ret = -ENXIO;
> > > -		goto err_remap_bar0;
> > > +		return -ENXIO;
> > 
> > Here and everywhere else these can now be converted to
> > dev_err_probe().
> > Are you planning to do so?
> 
> I want to do what's necessary to get PCI in better shape, since that's
> what the GPUs and accelerators we / I care about use :)
> 
> IOW, I want pci_request_regions() removed from here.

Okay!

...

> > >  err_remap_bar4:
> > >  	iounmap(bus->remap_addr);
> > 
> > This looks weird if the driver already is using pcim_enable_device().
> > Doesn't this look to you as an existing bug?
> 
> I looked briefly at it and it doesn't appear like an obvious bug to me
> because the drivers uses the (very old? deprecated?) pci_ioremap_bar().

> In any case the driver doesn't set up any devres callback, so has to
> iounmap() manually.

Okay, so they are using managed and non-managed APIs, but release / error path
ordering is fine. So, false alarm then.

> @Bjorn:
> Any comments on pci_ioremap_bar()? Should we mark that as deprecated?

> > > -err_remap_bar0:
> > > -	pci_release_regions(pci);
> > >  	return ret;

-- 
With Best Regards,
Andy Shevchenko