April 2025 - Sound-open-firmware - mailman.alsa-project.org

Re: [PATCH v2 2/4] ASoC: intel/avs: Use pure devres PCI
by Amadeusz Sławiński 24 Apr '25

24 Apr '25

On 2025-04-23 10:28, 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. > > Signed-off-by: Philipp Stanner <phasta(a)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); Hm... shouldn't we also drop call to pci_release_regions() in avs_pci_remove()? > return ret; > } > Nitpick: If there will be v2, can you also align title with how it usually is in this directory: ASoC: Intel: avs: Use pure devres PCI

1 0

Re: [PATCH v2 0/4] AsoC: Phase out hybrid PCI devres
by Cezary Rojewski 23 Apr '25

23 Apr '25

On 2025-04-23 10:28 AM, Philipp Stanner wrote: > Changes in v2: > - sof: simplify return. (Andy) > - intel/atom: simplify return. (Andy) > - Send a separate series for AsoC. (Andy) > - intel/atom: Add another patch that switches EINVAL to ENOMEM. (Andy) > > Hi, > > a year ago we spent quite some work trying to get PCI into better shape. > Some pci_ functions can be sometimes managed with devres, which is > obviously bad. We want to provide an obvious API, where pci_ functions > are never, and pcim_ functions are always managed. > > Thus, everyone enabling his device with pcim_enable_device() must be > ported to pcim_ functions. Porting all users will later enable us to > significantly simplify parts of the PCI subsystem. See here [1] for > details. > > This patch series does that for sound. > > Feel free to squash the commits as you see fit. > > P. > > [1] https://elixir.bootlin.com/linux/v6.14-rc4/source/drivers/pci/devres.c#L18 > > Philipp Stanner (4): > ASoC: sof: Use pure devres PCI > ASoC: intel/avs: Use pure devres PCI > AsoC: intel/atom: Use pure devres PCI > AsoC: intel/atom: Return -ENOMEM if pcim_iomap() fails Nitpick: the scopes used in commit titles do not match recommendations. Below the suggested ones: 'ASoC: Intel: atom:' for the atom-driver 'ASoC: Intel: avs:' for the avs-driver > sound/soc/intel/atom/sst/sst_pci.c | 58 +++++++++++++----------------- > sound/soc/intel/avs/core.c | 7 ++-- > sound/soc/sof/sof-pci-dev.c | 16 ++------- > 3 files changed, 29 insertions(+), 52 deletions(-) > Hi Philipp, Thank you for the contribution. I do not see any major issues so feel free to add: Reviewed-by: Cezary Rojewski <cezary.rojewski(a)intel.com> Kind Regards, Czarek

1 0

Re: [PATCH v2 4/4] AsoC: intel/atom: Return -ENOMEM if pcim_iomap() fails
by Cezary Rojewski 23 Apr '25

23 Apr '25

On 2025-04-23 10:28 AM, Philipp Stanner wrote: > The error checks for pcim_iomap() have the function return -EINVAL. > -ENOMEM is a more appropriate error code. > > Replace -EINVAL with -ENOMEM. Nitpicks: I believe the last sentence is redundant, the title and the message say it all. Next, the suggest scope for the atom-driver is: 'ASoC: Intel: atom:'. > Signed-off-by: Philipp Stanner <phasta(a)kernel.org> > --- > sound/soc/intel/atom/sst/sst_pci.c | 10 +++++----- > 1 file changed, 5 insertions(+), 5 deletions(-) > > diff --git a/sound/soc/intel/atom/sst/sst_pci.c b/sound/soc/intel/atom/sst/sst_pci.c > index eadcf24cbdc3..edc86519816d 100644 > --- a/sound/soc/intel/atom/sst/sst_pci.c > +++ b/sound/soc/intel/atom/sst/sst_pci.c > @@ -49,7 +49,7 @@ static int sst_platform_get_resources(struct intel_sst_drv *ctx) > > ctx->ddr = pcim_iomap(pci, 0, 0); > if (!ctx->ddr) > - return -EINVAL; > + return -ENOMEM; > > dev_dbg(ctx->dev, "sst: DDR Ptr %p\n", ctx->ddr); > } else { > @@ -59,7 +59,7 @@ static int sst_platform_get_resources(struct intel_sst_drv *ctx) > ctx->shim_phy_add = pci_resource_start(pci, 1); > ctx->shim = pcim_iomap(pci, 1, 0); > if (!ctx->shim) > - return -EINVAL; > + return -ENOMEM; > > dev_dbg(ctx->dev, "SST Shim Ptr %p\n", ctx->shim); > > @@ -67,7 +67,7 @@ static int sst_platform_get_resources(struct intel_sst_drv *ctx) > ctx->mailbox_add = pci_resource_start(pci, 2); > ctx->mailbox = pcim_iomap(pci, 2, 0); > if (!ctx->mailbox) > - return -EINVAL; > + return -ENOMEM; > > dev_dbg(ctx->dev, "SRAM Ptr %p\n", ctx->mailbox); > > @@ -76,7 +76,7 @@ static int sst_platform_get_resources(struct intel_sst_drv *ctx) > ctx->iram_base = pci_resource_start(pci, 3); > ctx->iram = pcim_iomap(pci, 3, 0); > if (!ctx->iram) > - return -EINVAL; > + return -ENOMEM; > > dev_dbg(ctx->dev, "IRAM Ptr %p\n", ctx->iram); > > @@ -85,7 +85,7 @@ static int sst_platform_get_resources(struct intel_sst_drv *ctx) > ctx->dram_base = pci_resource_start(pci, 4); > ctx->dram = pcim_iomap(pci, 4, 0); > if (!ctx->dram) > - return -EINVAL; > + return -ENOMEM; > > dev_dbg(ctx->dev, "DRAM Ptr %p\n", ctx->dram); > return 0; Hi Philipp, Thanks for the patch, this is certainly an additional effort, on top of the pcim_xxx one. Couple of nitpicks above but nothing major. Regardless if you decide to address them or not, feel free to add: Reviewed-by: Cezary Rojewski <cezary.rojewski(a)intel.com>

1 0

Re: [PATCH 02/31] ASoC: intel/avs: Use pure devres PCI
by Andy Shevchenko 16 Apr '25

16 Apr '25

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

1 0

Re: [PATCH 00/31] sound: Phase out hybrid PCI devres API
by Andy Shevchenko 16 Apr '25

16 Apr '25

On Wed, Apr 16, 2025 at 03:12:10PM +0200, Philipp Stanner wrote: > Hi, > > a year ago we spent quite some work trying to get PCI into better shape. > Some pci_ functions can be sometimes managed with devres, which is > obviously bad. We want to provide an obvious API, where pci_ functions > are never, and pcim_ functions are always managed. > > Thus, everyone enabling his device with pcim_enable_device() must be > ported to pcim_ functions. Porting all users will later enable us to > significantly simplify parts of the PCI subsystem. See here [1] for > details. > > This patch series does that for sound. AFAIK the ASoC and ALSA maintained by different people and perhaps you would need to split, but I'm not the guy in charge, so wait for them to tell you their preferences. -- With Best Regards, Andy Shevchenko

1 1

Re: [PATCH 03/31] AsoC: intel/atom: Use pure devres PCI
by Andy Shevchenko 16 Apr '25

16 Apr '25

On Wed, Apr 16, 2025 at 03:12:13PM +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 call to pci_release_regions(), since pcim_ functions do > cleanup automatically. > > Pass 0 as length parameter to pcim_iomap(), which is the standard way > for ioremapping an entire BAR. ... > sound/soc/intel/atom/sst/sst_pci.c | 56 ++++++++++++------------------ > 1 file changed, 23 insertions(+), 33 deletions(-) Cool cleanup! ... > /* DRAM */ > ctx->dram_end = pci_resource_end(pci, 4); > ctx->dram_base = pci_resource_start(pci, 4); > + ctx->dram = pcim_iomap(pci, 4, 0); > + if (!ctx->dram) > + return -EINVAL; Why EINVAL and not ENOMEM? mapping is about memory, no? > + > dev_dbg(ctx->dev, "DRAM Ptr %p\n", ctx->dram); > -do_release_regions: > - pci_release_regions(pci); > return ret; Now it seems return 0; should suffice here. -- With Best Regards, Andy Shevchenko

1 0

Re: [PATCH 02/31] ASoC: intel/avs: Use pure devres PCI
by Andy Shevchenko 16 Apr '25

16 Apr '25

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

1 0

Re: [PATCH 01/31] ASoC: sof: Use pure devres PCI
by Andy Shevchenko 16 Apr '25

16 Apr '25

On Wed, Apr 16, 2025 at 03:12:11PM +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 surplus calls to PCI release functions, since pcim_ functions do > cleanup automatically. ... > ret = snd_sof_device_probe(dev, sof_pdata); > > -out: > - if (ret) > - pci_release_regions(pci); > - > return ret; Obviously you can go further return snd_sof_...(...); -- With Best Regards, Andy Shevchenko

1 0