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

Re: [PATCH v4] ASoC: SOF: topology: Fix null pointer dereference
by Mark Brown 01 May '25

01 May '25

On Tue, 29 Apr 2025 11:19:22 +0200, Julien Massot wrote: > The "get_function_tplg_files" function is only applicable to > ACPI-based devices (sof_pdata->machine and not sof_pdata->of_machine). > Skip this check for OF-based devices to avoid a NULL pointer > dereference in snd_sof_load_topology(). > > Applied to https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next Thanks! [1/1] ASoC: SOF: topology: Fix null pointer dereference commit: 114a6e63d9cc11311587773d5db4cf4105cf658f All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted. You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed. If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced. Please add any relevant lists and maintainers to the CCs when replying to this mail. Thanks, Mark

1 0

Re: [PATCH] ASoC: SOF: ipc4-pcm: Adjust pipeline_list->pipelines allocation type
by Mark Brown 01 May '25

01 May '25

On Fri, 25 Apr 2025 23:25:12 -0700, Kees Cook wrote: > In preparation for making the kmalloc family of allocators type aware, > we need to make sure that the returned type from the allocation matches > the type of the variable being assigned. (Before, the allocator would > always return "void *", which can be implicitly cast to any pointer type.) > > The assigned type is "struct snd_sof_pipeline **", but the returned type > will be "struct snd_sof_widget **". These are the same size allocation > (pointer size) but the types don't match. Adjust the allocation type to > match the assignment. > > [...] Applied to https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next Thanks! [1/1] ASoC: SOF: ipc4-pcm: Adjust pipeline_list->pipelines allocation type commit: 00a371adbbfb46db561db85a9d7b53b2363880a1 All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted. You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed. If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced. Please add any relevant lists and maintainers to the CCs when replying to this mail. Thanks, Mark

1 0

Re: [PATCH] ASoC: SOF: ipc4-pcm: Adjust pipeline_list->pipelines allocation type
by Péter Ujfalusi 28 Apr '25

28 Apr '25

On 26/04/2025 09:25, Kees Cook wrote: > In preparation for making the kmalloc family of allocators type aware, > we need to make sure that the returned type from the allocation matches > the type of the variable being assigned. (Before, the allocator would > always return "void *", which can be implicitly cast to any pointer type.) > > The assigned type is "struct snd_sof_pipeline **", but the returned type > will be "struct snd_sof_widget **". These are the same size allocation > (pointer size) but the types don't match. Adjust the allocation type to > match the assignment. It looks like that this is indeed an oversight and a bug introduced by a mostly mechanical change (introduced in v6.3). Can you add: Fixes: 9c04363d222b ("ASoC: SOF: Introduce struct snd_sof_pipeline") and: Acked-by: Peter Ujfalusi <peter.ujfalusi(a)linux.intel.com> > Signed-off-by: Kees Cook <kees(a)kernel.org> > --- > Cc: Liam Girdwood <lgirdwood(a)gmail.com> > Cc: Peter Ujfalusi <peter.ujfalusi(a)linux.intel.com> > Cc: Bard Liao <yung-chuan.liao(a)linux.intel.com> > Cc: Ranjani Sridharan <ranjani.sridharan(a)linux.intel.com> > Cc: Daniel Baluta <daniel.baluta(a)nxp.com> > Cc: Kai Vehmanen <kai.vehmanen(a)linux.intel.com> > Cc: Pierre-Louis Bossart <pierre-louis.bossart(a)linux.dev> > Cc: Mark Brown <broonie(a)kernel.org> > Cc: Jaroslav Kysela <perex(a)perex.cz> > Cc: Takashi Iwai <tiwai(a)suse.com> > Cc: <sound-open-firmware(a)alsa-project.org> > Cc: <linux-sound(a)vger.kernel.org> > --- > sound/soc/sof/ipc4-pcm.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/sound/soc/sof/ipc4-pcm.c b/sound/soc/sof/ipc4-pcm.c > index 1a2841899ff5..52903503cf3b 100644 > --- a/sound/soc/sof/ipc4-pcm.c > +++ b/sound/soc/sof/ipc4-pcm.c > @@ -784,7 +784,8 @@ static int sof_ipc4_pcm_setup(struct snd_sof_dev *sdev, struct snd_sof_pcm *spcm > > /* allocate memory for max number of pipeline IDs */ > pipeline_list->pipelines = kcalloc(ipc4_data->max_num_pipelines, > - sizeof(struct snd_sof_widget *), GFP_KERNEL); > + sizeof(*pipeline_list->pipelines), > + GFP_KERNEL); > if (!pipeline_list->pipelines) { > sof_ipc4_pcm_free(sdev, spcm); > return -ENOMEM; -- Péter

1 0

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