March 2022 - Alsa-devel - mailman.alsa-project.org

[PATCH v2 00/17] ASoC: Intel: AVS - Audio DSP for cAVS
by Cezary Rojewski 01 Mar '22

01 Mar '22

A continuation of cleanup work of Intel SST solutions found in sound/soc/intel/. With two major chapters released last year catpt [1] and removal of haswell solution [2], time has come for Skylake-driver. Througout 2019, 2020 and 2021 Skylake-driver has had many fixes applied and even attempts of refactors as seen in fundamental overhaul [3], IPC flow adjustments [4] and LARGE_CONFIG overhaul [5] series. Unfortunately, story repeats itself - problems are found within the core of a driver. Painting it with different colors does not change the fact that is it still a house of cards. As changes needed to address those issues would make Skylake solution incompatible with its previous revisions, a decision has been made to provide a new solution instead. In time it would deprecate and replace Skylake-driver. That solution has been called AVS - from AudioDSP architecture name: Audio-Voice-Speech. It is meant to provide support for the exact same range of platforms as its predecessor: SKL, KBL, AML and APL. Note: this series is dependent upon HDA-series [6] which exposes several codec-organization functions allowing for reduced code size on avs-driver side. Note: this series does not add fully functional driver as its size would get out of control. Here, focus is put on adding IPC protocol and code loading code. Changes v2 -> v1: Almost all updates here are thanks to feedback from Pierre. - several comments and few kernel-docs have been added in areas which felt more or less unclear - avs_ipc_wait_busy_completion() now spins up to 'repeats_left' number of times before giving up - 'adsp_ba' field of struct avs_dev has been renamed to 'dsp_ba' - 'dops' field of struct avs_spec has been renamed to 'dsp_ops' - IPC abstraction has been simplified: SKL_ADSP_REG_HIP* regs are used directly - fixed allnoconfig with AVS enabled compilation issues - fixed code loading error paths: previously requested firmware is now released before function return to the caller - code and function arguments tied to D0IX support have been removed from this patchset and will be part of followup series adding that feature instead - enriched dev_err() messages in avs_dsp_get_core() and avs_dsp_put_core() - numerous wording fixes used in power/reset/stall DSP operations Changes RFC v1 [7]: -> v1: - separated HDA codec-organization patches, path and topology handling, PCM and complementary features such as recovery from this series to ease the review process - fixed EXPORT_SYMBOL_GPL for exported members of ASoC framework - result of stall() is now checked when sending ROM message - result of snd_hdac_ext_stream_set_spib() is now checked when loading basefw - if basefw is not ready, notification processing is now skipped - documented several topology parsing helpers Changes [internal] RFC v2 -> [public] RFC v1: - dropped any sysfs related changes from this series, moved to follow up one - dropped entire subscription-mechanism found in ipc.c. Handlers that are delegated to service certain firmware notifications are now called directly - fixed kernel doc for snd_soc_dapm_new_dai_widgets() as reported by ikp - prefixed snd_hda_codec_device_init() as suggested by Amadeo - improved comments for d0ix transitions for APL-based platforms as suggested by Pierre - a ton of spelling related fixes in most of the commit messages - fixed remaining warnings pointed by scan-build (variable assigned but not used) - replaced most of 'cAVS X.Y' expression usages with 'platform-based' equivalents as suggested by Pierre e.g.: cAVS 1.5 -> SKL-based Changes [internal] RFC v1 -> [internal] RFC v2: - fixed memleak caused by lack of kfree(vols) if memory allocation fails in avs_peakvol_create() as reported by Curtis - fixed missing 'i' iterator incrementation in avs_widget_ready() causing reference loss as reported by Curtis - replace hardcode: 0x40 usage with snd_hdac_calc_stream_format as suggested by Curtis. In consequence, readability for all code loading (CL) procedues has increased and such approach auto-documents the CL stream preparation - updated behavior of all index-fetching functions found in utils.c: avs_module_entry_index(), avs_module_id_entry_index() and follow ups: avs_get_module_entry(), avs_get_module_id_entry() to better conform to linux-kernel standard when no entry is found (return -ENOENT) rather than C++ standard (return -1, what in kernel case translated to -EPERM) as suggested by Curtis and Peter - several suggestions have been made regarding spacing, and so far, I've agreed and applied with all of them. None proposed seemed out of place or redundant - avs_path_stop() renamed to avs_path_pause() pipeline states are represented by RESET/PAUSED/RUNNING. avs_path_reset() and avs_path_run() were already there and avs_path_stop() just didn't look cohesive - added missing parsers for num_output_pin and num_input_pin which are required for custom modules such as WAVES or DSM - dropped 'priv_param_length' from custom module descriptor as this field is obsolete in firmware - parse_dictionary() has been split into parse_dictionary_header() and parse_dictionary_entries() to drop code duplication present in several parsing function which could not re-use entire parse_dictionary() - added avs_tplg_vendor_array_lookup_next() and avs_tplg_vendor_entry_next() to drop code duplicated present in several parsing functions. This change greatly impacted readability of all parsers - parsing helpers such avs_tplg_vendor_array_lookup() now return offset by updating specified in function argument list u32 *offset variable. This is to address problem when u32 offset would be greater than max int, which is the return type for these functions - AVS_DEFINE_PTR_PARSER() macro has been introduced to drop code duplication for all ptr-parsing users - all struct avs_path_module creators have had their declaration updated: function argument *owner ceased to exist as it could already be accessed by mod->owner - fixed the order of operation for conditional paths (e.g.: echo reference) so these are no longer controlled by "source" path and instead are impacted by state changes of source and sink paths both. Previously only source path e.g. playback sourcing echo reference would trigger RUNNING status for conditional path. Equivalent RUNNING on WoV path which is in this case sink path, would not do so, leading to order-of-operation problems. Behavior has been changed to: both source and sink need to be RUNNING for conditional path to be set to RUNNING too. PAUSED for either source or sink will cause PAUSED transition for conditional path. - to achieve the above, path states are now saved in 'state' i.e. new u32 field for struct avs_path - resigned from fw_filename field usage in favour of newly added tplg_filename for machine board descriptors as suggested by Pierre - platform descriptor fields have had their names update better reflect their purpose as suggested by Pierre - fixed comp_list missing locking when manipulated - all message senders now accept request as pointer as suggeseted by Peter - resigned of AZX_ usage for all ADSP-related registers, leaving them only for HOST memory space related operations - fixed disable path for core DSP operations: power/reset/stall as reported by Peter - safety when locking between received responses (reply vs notification) has been lowered as suggested by Pierre. Most usages are not performed in IRQ context and none is done in hard-IRQ one - s/master/main/ plus AVS_MAIN_CORE_MASK has replaced ->master_mask - several functions have had their logging updated - logs have been moved to lower level functions as suggested by Pierre - hdac_ext_stream usage has been streamlined to estream, hdac_streams are represented by hstream instead - hw_params() are resilient to scenarios when they are called mutliple times as reported by Pierre - avs_dsp_enable() now collapses if any of its steps fails as reported by Pierre and Peter - avs_module_ida_empty() now returns value of type bool as suggested by Bard [1]: https://www.spinics.net/lists/alsa-devel/msg116440.html [2]: https://www.spinics.net/lists/alsa-devel/msg116901.html [3]: https://www.spinics.net/lists/alsa-devel/msg94199.html [4]: https://www.spinics.net/lists/alsa-devel/msg92588.html [5]: https://lore.kernel.org/all/20190808181549.12521-1-cezary.rojewski@intel.co… [6]: https://lore.kernel.org/alsa-devel/20220207114906.3759800-1-cezary.rojewski… [7]: https://lore.kernel.org/all/20211208111301.1817725-1-cezary.rojewski@intel.… Cezary Rojewski (17): ALSA: hda: Add helper macros for DSP capable devices ASoC: Export DAI register and widget ctor and dctor functions ASoC: Intel: Introduce AVS driver ASoC: Intel: avs: Inter process communication ASoC: Intel: avs: Add code loading requests ASoC: Intel: avs: Add pipeline management requests ASoC: Intel: avs: Add module management requests ASoC: Intel: avs: Add power management requests ASoC: Intel: avs: Add ROM requests ASoC: Intel: avs: Add basefw runtime-parameter requests ASoC: Intel: avs: Firmware resources management utilities ASoC: Intel: avs: Declare module configuration types ASoC: Intel: avs: Dynamic firmware resources management ASoC: Intel: avs: General code loading flow ASoC: Intel: avs: Implement CLDMA transfer ASoC: Intel: avs: Code loading over CLDMA ASoC: Intel: avs: Code loading over HDA include/sound/hdaudio.h | 2 + include/sound/hdaudio_ext.h | 50 +++ include/sound/soc-dapm.h | 1 + sound/soc/intel/Kconfig | 12 + sound/soc/intel/Makefile | 1 + sound/soc/intel/avs/Makefile | 6 + sound/soc/intel/avs/avs.h | 248 +++++++++++ sound/soc/intel/avs/cldma.c | 317 ++++++++++++++ sound/soc/intel/avs/cldma.h | 29 ++ sound/soc/intel/avs/core.c | 62 +++ sound/soc/intel/avs/dsp.c | 303 +++++++++++++ sound/soc/intel/avs/ipc.c | 393 +++++++++++++++++ sound/soc/intel/avs/loader.c | 604 +++++++++++++++++++++++++ sound/soc/intel/avs/messages.c | 697 +++++++++++++++++++++++++++++ sound/soc/intel/avs/messages.h | 753 ++++++++++++++++++++++++++++++++ sound/soc/intel/avs/registers.h | 75 ++++ sound/soc/intel/avs/utils.c | 301 +++++++++++++ sound/soc/soc-core.c | 1 + sound/soc/soc-dapm.c | 15 + 19 files changed, 3870 insertions(+) create mode 100644 sound/soc/intel/avs/Makefile create mode 100644 sound/soc/intel/avs/avs.h create mode 100644 sound/soc/intel/avs/cldma.c create mode 100644 sound/soc/intel/avs/cldma.h create mode 100644 sound/soc/intel/avs/core.c create mode 100644 sound/soc/intel/avs/dsp.c create mode 100644 sound/soc/intel/avs/ipc.c create mode 100644 sound/soc/intel/avs/loader.c create mode 100644 sound/soc/intel/avs/messages.c create mode 100644 sound/soc/intel/avs/messages.h create mode 100644 sound/soc/intel/avs/registers.h create mode 100644 sound/soc/intel/avs/utils.c -- 2.25.1

2 20

[PATCH v5 0/2] Add support for SoundWire1.6 audio cgcr register control
by Srinivasa Rao Mandadapu 01 Mar '22

01 Mar '22

This patch series is to add v1.6.0 compatible name for qcom soundwire driver and corresponding dt bindings. Changes Since V4: -- Dropped audio cgcr control patch due to dependency on clock patches. -- Update dt-bindings as per new reset control properties. Changes Since V3: -- Add v1.6.0 compatible name and soundwire data structure. -- Change macro define name properly. -- Update bindings for new property. -- Change commit message description. -- Change signedoff by sequence. Changes since v2: -- Update error check after ioremap. Changes since v1: -- Add const name to mask value. Srinivasa Rao Mandadapu (2): soundwire: qcom: Add compatible name for v1.6.0 dt-bindings: soundwire: qcom: Add bindings for audio CSR reset control property Documentation/devicetree/bindings/soundwire/qcom,sdw.txt | 12 ++++++++++++ drivers/soundwire/qcom.c | 6 ++++++ 2 files changed, 18 insertions(+) -- 2.7.4

2 6

[PATCH] Asoc: au1x: Use devm_snd_soc_register_component()
by Zhen Ni 01 Mar '22

01 Mar '22

Using devm_snd_soc_register_component() instead of snd_soc_register_component() Signed-off-by: Zhen Ni <nizhen(a)uniontech.com> --- sound/soc/au1x/ac97c.c | 2 +- sound/soc/au1x/i2sc.c | 2 +- sound/soc/au1x/psc-ac97.c | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/sound/soc/au1x/ac97c.c b/sound/soc/au1x/ac97c.c index 3b1700e665f5..795bba756585 100644 --- a/sound/soc/au1x/ac97c.c +++ b/sound/soc/au1x/ac97c.c @@ -275,7 +275,7 @@ static int au1xac97c_drvprobe(struct platform_device *pdev) if (ret) return ret; - ret = snd_soc_register_component(&pdev->dev, &au1xac97c_component, + ret = devm_snd_soc_register_component(&pdev->dev, &au1xac97c_component, &au1xac97c_dai_driver, 1); if (ret) return ret; diff --git a/sound/soc/au1x/i2sc.c b/sound/soc/au1x/i2sc.c index 740d4e052e4d..a3c9d52ef337 100644 --- a/sound/soc/au1x/i2sc.c +++ b/sound/soc/au1x/i2sc.c @@ -265,7 +265,7 @@ static int au1xi2s_drvprobe(struct platform_device *pdev) platform_set_drvdata(pdev, ctx); - return snd_soc_register_component(&pdev->dev, &au1xi2s_component, + return devm_snd_soc_register_component(&pdev->dev, &au1xi2s_component, &au1xi2s_dai_driver, 1); } diff --git a/sound/soc/au1x/psc-ac97.c b/sound/soc/au1x/psc-ac97.c index 05eb36991f14..3f60313822e7 100644 --- a/sound/soc/au1x/psc-ac97.c +++ b/sound/soc/au1x/psc-ac97.c @@ -411,7 +411,7 @@ static int au1xpsc_ac97_drvprobe(struct platform_device *pdev) if (ret) return ret; - ret = snd_soc_register_component(&pdev->dev, &au1xpsc_ac97_component, + ret = devm_snd_soc_register_component(&pdev->dev, &au1xpsc_ac97_component, &wd->dai_drv, 1); if (ret) return ret; -- 2.20.1

2 1

[PATCH] ASoC: fsi: Add check for clk_enable
by Jiasheng Jiang 01 Mar '22

01 Mar '22

As the potential failure of the clk_enable(), it should be better to check it and return error if fails. Fixes: ab6f6d85210c ("ASoC: fsi: add master clock control functions") Signed-off-by: Jiasheng Jiang <jiasheng(a)iscas.ac.cn> --- sound/soc/sh/fsi.c | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-) diff --git a/sound/soc/sh/fsi.c b/sound/soc/sh/fsi.c index cdf3b7f69ba7..93cfa77b8b54 100644 --- a/sound/soc/sh/fsi.c +++ b/sound/soc/sh/fsi.c @@ -816,14 +816,26 @@ static int fsi_clk_enable(struct device *dev, return ret; } - clk_enable(clock->xck); - clk_enable(clock->ick); - clk_enable(clock->div); + ret = clk_enable(clock->xck); + if (ret) + goto err; + ret = clk_enable(clock->ick); + if (ret) + goto err; + ret = clk_enable(clock->div); + if (ret) + goto err; clock->count++; } return ret; + +err: + clk_disable(clock->xck); + clk_disable(clock->ick); + clk_disable(clock->div); + return ret; } static int fsi_clk_disable(struct device *dev, -- 2.25.1

2 1

[PATCH] ASoC: soc-generic-dmaengine-pcm: Use default config when none is given
by Sascha Hauer 01 Mar '22

01 Mar '22

snd_dmaengine_pcm_register() can be passed a NULL pointer for the config which means that the we have to test for pcm->config being non NULL before accessing it. Make the code more straight forward by providing a default config when none is passed. Signed-off-by: Sascha Hauer <s.hauer(a)pengutronix.de> --- sound/soc/soc-generic-dmaengine-pcm.c | 55 ++++++++++++--------------- 1 file changed, 24 insertions(+), 31 deletions(-) diff --git a/sound/soc/soc-generic-dmaengine-pcm.c b/sound/soc/soc-generic-dmaengine-pcm.c index 0f6c9c55a22d6..734d46b9783f7 100644 --- a/sound/soc/soc-generic-dmaengine-pcm.c +++ b/sound/soc/soc-generic-dmaengine-pcm.c @@ -79,29 +79,19 @@ static int dmaengine_pcm_hw_params(struct snd_soc_component *component, { struct dmaengine_pcm *pcm = soc_component_to_pcm(component); struct dma_chan *chan = snd_dmaengine_pcm_get_chan(substream); - int (*prepare_slave_config)(struct snd_pcm_substream *substream, - struct snd_pcm_hw_params *params, - struct dma_slave_config *slave_config); struct dma_slave_config slave_config; + int ret; - memset(&slave_config, 0, sizeof(slave_config)); - - if (!pcm->config) - prepare_slave_config = snd_dmaengine_pcm_prepare_slave_config; - else - prepare_slave_config = pcm->config->prepare_slave_config; + if (!pcm->config->prepare_slave_config) + return 0; - if (prepare_slave_config) { - int ret = prepare_slave_config(substream, params, &slave_config); - if (ret) - return ret; + memset(&slave_config, 0, sizeof(slave_config)); - ret = dmaengine_slave_config(chan, &slave_config); - if (ret) - return ret; - } + ret = pcm->config->prepare_slave_config(substream, params, &slave_config); + if (ret) + return ret; - return 0; + return dmaengine_slave_config(chan, &slave_config); } static int @@ -121,7 +111,7 @@ dmaengine_pcm_set_runtime_hwparams(struct snd_soc_component *component, return -EINVAL; } - if (pcm->config && pcm->config->pcm_hardware) + if (pcm->config->pcm_hardware) return snd_soc_set_runtime_hwparams(substream, pcm->config->pcm_hardware); @@ -188,7 +178,6 @@ static struct dma_chan *dmaengine_pcm_compat_request_channel( { struct dmaengine_pcm *pcm = soc_component_to_pcm(component); struct snd_dmaengine_dai_dma_data *dma_data; - dma_filter_fn fn = NULL; if (rtd->num_cpus > 1) { dev_err(rtd->dev, @@ -201,13 +190,11 @@ static struct dma_chan *dmaengine_pcm_compat_request_channel( if ((pcm->flags & SND_DMAENGINE_PCM_FLAG_HALF_DUPLEX) && pcm->chan[0]) return pcm->chan[0]; - if (pcm->config && pcm->config->compat_request_channel) + if (pcm->config->compat_request_channel) return pcm->config->compat_request_channel(rtd, substream); - if (pcm->config) - fn = pcm->config->compat_filter_fn; - - return snd_dmaengine_pcm_request_channel(fn, dma_data->filter_data); + return snd_dmaengine_pcm_request_channel(pcm->config->compat_filter_fn, + dma_data->filter_data); } static bool dmaengine_pcm_can_report_residue(struct device *dev, @@ -239,7 +226,7 @@ static int dmaengine_pcm_new(struct snd_soc_component *component, size_t max_buffer_size; unsigned int i; - if (config && config->prealloc_buffer_size) { + if (config->prealloc_buffer_size) { prealloc_buffer_size = config->prealloc_buffer_size; max_buffer_size = config->pcm_hardware->buffer_bytes_max; } else { @@ -252,7 +239,7 @@ static int dmaengine_pcm_new(struct snd_soc_component *component, if (!substream) continue; - if (!pcm->chan[i] && config && config->chan_names[i]) + if (!pcm->chan[i] && config->chan_names[i]) pcm->chan[i] = dma_request_slave_channel(dev, config->chan_names[i]); @@ -365,10 +352,10 @@ static int dmaengine_pcm_request_chan_of(struct dmaengine_pcm *pcm, struct dma_chan *chan; if ((pcm->flags & SND_DMAENGINE_PCM_FLAG_NO_DT) || (!dev->of_node && - !(config && config->dma_dev && config->dma_dev->of_node))) + !(config->dma_dev && config->dma_dev->of_node))) return 0; - if (config && config->dma_dev) { + if (config->dma_dev) { /* * If this warning is seen, it probably means that your Linux * device structure does not match your HW device structure. @@ -385,7 +372,7 @@ static int dmaengine_pcm_request_chan_of(struct dmaengine_pcm *pcm, name = "rx-tx"; else name = dmaengine_pcm_dma_channel_names[i]; - if (config && config->chan_names[i]) + if (config->chan_names[i]) name = config->chan_names[i]; chan = dma_request_chan(dev, name); if (IS_ERR(chan)) { @@ -423,6 +410,10 @@ static void dmaengine_pcm_release_chan(struct dmaengine_pcm *pcm) } } +static const struct snd_dmaengine_pcm_config snd_dmaengine_pcm_default_config = { + .prepare_slave_config = snd_dmaengine_pcm_prepare_slave_config, +}; + /** * snd_dmaengine_pcm_register - Register a dmaengine based PCM device * @dev: The parent device for the PCM device @@ -443,6 +434,8 @@ int snd_dmaengine_pcm_register(struct device *dev, #ifdef CONFIG_DEBUG_FS pcm->component.debugfs_prefix = "dma"; #endif + if (!config) + config = &snd_dmaengine_pcm_default_config; pcm->config = config; pcm->flags = flags; @@ -450,7 +443,7 @@ int snd_dmaengine_pcm_register(struct device *dev, if (ret) goto err_free_dma; - if (config && config->process) + if (config->process) driver = &dmaengine_pcm_component_process; else driver = &dmaengine_pcm_component; -- 2.30.2

1 0

[PATCH] ALSA: n64: Use platform_get_irq() to get the interrupt
by Zhen Ni 01 Mar '22

01 Mar '22

platform_get_resource(pdev, IORESOURCE_IRQ, ..) relies on static allocation of IRQ resources in DT core code, this causes an issue when using hierarchical interrupt domains using "interrupts" property in the node as this bypassed the hierarchical setup and messed up the irq chaining. In preparation for removal of static setup of IRQ resource from DT core code use platform_get_irq(). Signed-off-by: Zhen Ni <nizhen(a)uniontech.com> --- sound/mips/snd-n64.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/sound/mips/snd-n64.c b/sound/mips/snd-n64.c index 463a6fe589eb..19365aab15ba 100644 --- a/sound/mips/snd-n64.c +++ b/sound/mips/snd-n64.c @@ -289,7 +289,7 @@ static int __init n64audio_probe(struct platform_device *pdev) struct snd_card *card; struct snd_pcm *pcm; struct n64audio *priv; - struct resource *res; + int irq; int err; err = snd_card_new(&pdev->dev, SNDRV_DEFAULT_IDX1, @@ -337,12 +337,12 @@ static int __init n64audio_probe(struct platform_device *pdev) strcpy(card->shortname, "N64 Audio"); strcpy(card->longname, "N64 Audio"); - res = platform_get_resource(pdev, IORESOURCE_IRQ, 0); - if (!res) { + irq = platform_get_irq(pdev, 0); + if (irq < 0) { err = -EINVAL; goto fail_dma_alloc; } - if (devm_request_irq(&pdev->dev, res->start, n64audio_isr, + if (devm_request_irq(&pdev->dev, irq, n64audio_isr, IRQF_SHARED, "N64 Audio", priv)) { err = -EBUSY; goto fail_dma_alloc; -- 2.20.1

2 1

Issue in pcm_dsnoop.c in alsa-lib
by GitHub issues - opened 01 Mar '22

01 Mar '22

alsa-project/alsa-lib issue #213 was opened from TE-N-ShengjiuWang: Hi Takashi Iwai, Jaroslav Kysela We encountered an issue in the pcm_dsnoop use case, could you please help to have a look? Issue description: With two instances for dsnoop type device running in parallel, after suspend/resume, one of the instances will be hung in memcpy because the very large copy size is obtained. #3 0x0000ffffa78d5098 in snd_pcm_dsnoop_sync_ptr (pcm=0xaaab06563da0) at pcm_dsnoop.c:158 dsnoop = 0xaaab06563c20 slave_hw_ptr = 64 old_slave_hw_ptr = 533120 avail = 187651522444320 Reason analysis: The root cause that I analysis is that after suspend/resume, one instance will get the SND_PCM_STATE_SUSPENDED state from slave pcm device, then it will do snd_pcm_prepare() and snd_pcm_start(), which will reset the dsnoop->slave_hw_ptr and the hw_ptr of slave pcm device, then the state of this instance is correct. But another instance may not get the SND_PCM_STATE_SUSPENDED state from slave pcm device because slave device may have been recovered by first instance, so the dsnoop->slave_hw_ptr is not reset. but because hw_ptr of slave pcm device has been reset, so there will be a very large "avail" size. Solution: I didn't come up with a fix for this issue, seems there is no easy way to let another instance know this case and reset the dsnoop->slave_hw_ptr, could you please help? Issue URL : https://github.com/alsa-project/alsa-lib/issues/213 Repository URL: https://github.com/alsa-project/alsa-lib

1 0

chancel.liu@nxp.com, shengjiu.wang@nxp.com
by Shengjiu Wang 01 Mar '22

01 Mar '22

Hi Takashi Iwai, Jaroslav Kysela We encountered an issue in the pcm_dsnoop use case, could you please help to have a look? *Issue description:* With two instances for dsnoop type device running in parallel, after suspend/resume, one of the instances will be hung in memcpy because the very large copy size is obtained. #3 0x0000ffffa78d5098 in snd_pcm_dsnoop_sync_ptr (pcm=0xaaab06563da0) at pcm_dsnoop.c:158 dsnoop = 0xaaab06563c20 slave_hw_ptr = 64 old_slave_hw_ptr = 533120 avail = *187651522444320* * Reason analysis: * The root cause that I analysis is that after suspend/resume, one instance will get the SND_PCM_STATE_SUSPENDED state from slave pcm device, then it will do snd_pcm_prepare() and snd_pcm_start(), which will reset the dsnoop->slave_hw_ptr and the hw_ptr of slave pcm device, then the state of this instance is correct. But another instance may not get the SND_PCM_STATE_SUSPENDED state from slave pcm device because slave device may have been recovered by first instance, so the dsnoop->slave_hw_ptr is not reset. but because hw_ptr of slave pcm device has been reset, so there will be a very large "avail" size. *Solution:* I didn't come up with a fix for this issue, seems there is no easy way to let another instance know this case and reset the dsnoop->slave_hw_ptr, could you please help? Best regards Wang shengjiu

1 0

RE: [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr
by David Laight 01 Mar '22

01 Mar '22

From: Linus Torvalds > Sent: 28 February 2022 19:56 > On Mon, Feb 28, 2022 at 4:19 AM Christian König > <christian.koenig(a)amd.com> wrote: > > > > I don't think that using the extra variable makes the code in any way > > more reliable or easier to read. > > So I think the next step is to do the attached patch (which requires > that "-std=gnu11" that was discussed in the original thread). > > That will guarantee that the 'pos' parameter of list_for_each_entry() > is only updated INSIDE the for_each_list_entry() loop, and can never > point to the (wrongly typed) head entry. > > And I would actually hope that it should actually cause compiler > warnings about possibly uninitialized variables if people then use the > 'pos' pointer outside the loop. Except > > (a) that code in sgx/encl.c currently initializes 'tmp' to NULL for > inexplicable reasons - possibly because it already expected this > behavior > > (b) when I remove that NULL initializer, I still don't get a warning, > because we've disabled -Wno-maybe-uninitialized since it results in so > many false positives. > > Oh well. > > Anyway, give this patch a look, and at least if it's expanded to do > "(pos) = NULL" in the entry statement for the for-loop, it will avoid > the HEAD type confusion that Jakob is working on. And I think in a > cleaner way than the horrid games he plays. > > (But it won't avoid possible CPU speculation of such type confusion. > That, in my opinion, is a completely different issue) > > I do wish we could actually poison the 'pos' value after the loop > somehow - but clearly the "might be uninitialized" I was hoping for > isn't the way to do it. > > Anybody have any ideas? > > Linus diff --git a/include/linux/list.h b/include/linux/list.h index dd6c2041d09c..bab995596aaa 100644 --- a/include/linux/list.h +++ b/include/linux/list.h @@ -634,10 +634,10 @@ static inline void list_splice_tail_init(struct list_head *list, * @head: the head for your list. * @member: the name of the list_head within the struct. */ -#define list_for_each_entry(pos, head, member) \ - for (pos = list_first_entry(head, typeof(*pos), member); \ - !list_entry_is_head(pos, head, member); \ - pos = list_next_entry(pos, member)) +#define list_for_each_entry(pos, head, member) \ + for (typeof(pos) __iter = list_first_entry(head, typeof(*pos), member); \ + !list_entry_is_head(__iter, head, member) && (((pos)=__iter),1); \ + __iter = list_next_entry(__iter, member)) /** * list_for_each_entry_reverse - iterate backwards over list of given type. I think you actually want: !list_entry_is_head(__iter, head, member) ? (((pos)=__iter),1) : (((pos) = NULL),0); Which can be done in the original by: !list_entry_is_head(pos, head, member) ? 1 : (((pos) = NULL), 0); Although it would be safer to have (without looking up the actual name): for (item *__item = head; \ __item ? (((pos) = list_item(__item, member)), 1) : (((pos) = NULL), 0); __item = (pos)->member) The local does need 'fixing' to avoid shadowing for nested loops. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)

1 0