November 2020 - Alsa-devel - mailman.alsa-project.org

Re: [PATCH 000/141] Fix fall-through warnings for Clang
by Miguel Ojeda 26 Nov '20

26 Nov '20

On Wed, Nov 25, 2020 at 11:44 PM Edward Cree <ecree.xilinx(a)gmail.com> wrote: > > To make the intent clear, you have to first be certain that you > understand the intent; otherwise by adding either a break or a > fallthrough to suppress the warning you are just destroying the > information that "the intent of this code is unknown". If you don't know what the intent of your own code is, then you *already* have a problem in your hands. > Figuring out the intent of a piece of unfamiliar code takes more > than 1 minute; just because > case foo: > thing; > case bar: > break; > produces identical code to > case foo: > thing; > break; > case bar: > break; > doesn't mean that *either* is correct — maybe the author meant What takes 1 minute is adding it *mechanically* by the author, i.e. so that you later compare whether codegen is the same. > to write > case foo: > return thing; > case bar: > break; > and by inserting that break you've destroyed the marker that > would direct someone who knew what the code was about to look > at that point in the code and spot the problem. Then it means you already have a bug. This patchset gives the maintainer a chance to notice it, which is a good thing. The "you've destroyed the market" claim is bogus, because: 1. you were not looking into it 2. you didn't notice the bug so far 3. is implicit -- harder to spot 4. is only useful if you explicitly take a look at this kind of bug. So why don't you do it now? > Thus, you *always* have to look at more than just the immediate > mechanical context of the code, to make a proper judgement that > yes, this was the intent. I find that is the responsibility of the maintainers and reviewers for tree-wide patches like this, assuming they want. They can also keep the behavior (and the bugs) without spending time. Their choice. > If you think that that sort of thing > can be done in an *average* time of one minute, then I hope you > stay away from code I'm responsible for! Please don't accuse others of recklessness or incompetence, especially if you didn't understand what they said. > A warning is only useful because it makes you *think* about the > code. If you suppress the warning without doing that thinking, > then you made the warning useless; and if the warning made you > think about code that didn't *need* it, then the warning was > useless from the start. We are not suppressing the warning. Quite the opposite, in fact. > So make your mind up: does Clang's stricter -Wimplicit-fallthrough > flag up code that needs thought (in which case the fixes take > effort both to author and to review) As I said several times already, it does take time to review if the maintainer wants to take the chance to see if they had a bug to begin with, but it does not require thought for the author if they just go for equivalent codegen. > or does it flag up code > that can be mindlessly "fixed" (in which case the warning is > worthless)? Proponents in this thread seem to be trying to > have it both ways. A warning is not worthless just because you can mindlessly fix it. There are many counterexamples, e.g. many checkpatch/lint/lang-format/indentation warnings, functional ones like the `if (a = b)` warning... Cheers, Miguel

3 3

pcm: snd_pcm_readi - plugin type multi, the overun appears when read
by GitHub pull_request - opened 26 Nov '20

26 Nov '20

alsa-project/alsa-lib pull request #102 was opened from ouwenchen: After type route use type multi, merge the two input streams.There was PCM error due to insufficient readable data.Cause the underlying read pointer cannot be updated, and then it will be overrun. Request URL : https://github.com/alsa-project/alsa-lib/pull/102 Patch URL : https://github.com/alsa-project/alsa-lib/pull/102.patch Repository URL: https://github.com/alsa-project/alsa-lib

1 0

[PATCH v3 0/1] ALSA: compress: allow pause and resume during draining
by Gyeongtaek Lee 26 Nov '20

26 Nov '20

On Wed, 25 Nov 2020 16:28:05 +0530, Vinod Koul wrote: >On 27-10-20, 10:57, Gyeongtaek Lee wrote: >> diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c >> index 0e53f6f31916..a071485383ed 100644 >> --- a/sound/core/compress_offload.c >> +++ b/sound/core/compress_offload.c >> @@ -708,11 +708,24 @@ static int snd_compr_pause(struct snd_compr_stream *stream) >> { >> int retval; >> >> - if (stream->runtime->state != SNDRV_PCM_STATE_RUNNING) >> + switch (stream->runtime->state) { >> + case SNDRV_PCM_STATE_RUNNING: >> + retval = stream->ops->trigger(stream, >> + SNDRV_PCM_TRIGGER_PAUSE_PUSH); > >this seems to fit single line with new 100char limit and makes it a >better read, can we please do that here and few places below .. The line >split is making it look bit ugly IMO > >> + if (!retval) >> + stream->runtime->state = SNDRV_PCM_STATE_PAUSED; >> + break; >> + case SNDRV_PCM_STATE_DRAINING: >> + if (!stream->device->use_pause_in_draining) >> + return -EPERM; > >I am expecting patches to tinycompress to handle pause while drain. Esp >this case.. > >> + retval = stream->ops->trigger(stream, >> + SNDRV_PCM_TRIGGER_PAUSE_PUSH); >> + if (!retval) >> + stream->pause_in_draining = true; >> + break; >> + default: >> return -EPERM; >> - retval = stream->ops->trigger(stream, SNDRV_PCM_TRIGGER_PAUSE_PUSH); >> - if (!retval) >> - stream->runtime->state = SNDRV_PCM_STATE_PAUSED; >> + } >> return retval; >> } >> >> @@ -720,11 +733,25 @@ static int snd_compr_resume(struct snd_compr_stream *stream) >> { >> int retval; >> >> - if (stream->runtime->state != SNDRV_PCM_STATE_PAUSED) >> + switch (stream->runtime->state) { >> + case SNDRV_PCM_STATE_PAUSED: >> + retval = stream->ops->trigger(stream, >> + SNDRV_PCM_TRIGGER_PAUSE_RELEASE); >> + if (!retval) >> + stream->runtime->state = SNDRV_PCM_STATE_RUNNING; >> + break; >> + case SNDRV_PCM_STATE_DRAINING: >> + if (!stream->device->use_pause_in_draining || >> + !stream->pause_in_draining) >> + return -EPERM; > >does this condition make sense for resume part..? We have already >checked for this while going into pause. I am not expecting drain state >to change while we are paused :) > I have fixed the patch as your review comment and send version 3. For your comment about tinycompress, I'll try to make a patch after study about tinycompress. Thanks, Gyeongtaek Gyeongtaek Lee (1): ALSA: compress: allow pause and resume during draining include/sound/compress_driver.h | 16 ++++++++++++++ sound/core/compress_offload.c | 39 ++++++++++++++++++++++++++------- 2 files changed, 47 insertions(+), 8 deletions(-) base-commit: 418baf2c28f3473039f2f7377760bd8f6897ae18 -- 2.21.0

1 0

Re: drivers/soundwire/qcom.c:767: undefined reference to `slimbus_bus'
by Randy Dunlap 26 Nov '20

26 Nov '20

On 11/2/20 11:47 AM, kernel test robot wrote: > tree: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master > head: 3cea11cd5e3b00d91caf0b4730194039b45c5891 > commit: 09309093d5e8f8774e4a3a0d42b73cf47e9421cf soundwire: qcom: fix SLIBMUS/SLIMBUS typo > date: 8 weeks ago > config: openrisc-randconfig-r005-20201102 (attached as .config) > compiler: or1k-linux-gcc (GCC) 9.3.0 > reproduce (this is a W=1 build): > wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross > chmod +x ~/bin/make.cross > # https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?… > git remote add linus https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git > git fetch --no-tags linus master > git checkout 09309093d5e8f8774e4a3a0d42b73cf47e9421cf > # save the attached .config to linux build tree > COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=openrisc > > If you fix the issue, kindly add following tag as appropriate > Reported-by: kernel test robot <lkp(a)intel.com> > > All errors (new ones prefixed by >>): > > or1k-linux-ld: drivers/soundwire/qcom.o: in function `qcom_swrm_probe': >>> drivers/soundwire/qcom.c:767: undefined reference to `slimbus_bus' >>> or1k-linux-ld: drivers/soundwire/qcom.c:771: undefined reference to `slimbus_bus' > > vim +767 drivers/soundwire/qcom.c > > 02efb49aa805cee Srinivas Kandagatla 2020-01-13 756 > 02efb49aa805cee Srinivas Kandagatla 2020-01-13 757 static int qcom_swrm_probe(struct platform_device *pdev) > 02efb49aa805cee Srinivas Kandagatla 2020-01-13 758 { > 02efb49aa805cee Srinivas Kandagatla 2020-01-13 759 struct device *dev = &pdev->dev; > 02efb49aa805cee Srinivas Kandagatla 2020-01-13 760 struct sdw_master_prop *prop; > 02efb49aa805cee Srinivas Kandagatla 2020-01-13 761 struct sdw_bus_params *params; > 02efb49aa805cee Srinivas Kandagatla 2020-01-13 762 struct qcom_swrm_ctrl *ctrl; > 02efb49aa805cee Srinivas Kandagatla 2020-01-13 763 int ret; > 02efb49aa805cee Srinivas Kandagatla 2020-01-13 764 u32 val; > 02efb49aa805cee Srinivas Kandagatla 2020-01-13 765 > 02efb49aa805cee Srinivas Kandagatla 2020-01-13 766 ctrl = devm_kzalloc(dev, sizeof(*ctrl), GFP_KERNEL); > 02efb49aa805cee Srinivas Kandagatla 2020-01-13 @767 if (!ctrl) > 02efb49aa805cee Srinivas Kandagatla 2020-01-13 768 return -ENOMEM; > 02efb49aa805cee Srinivas Kandagatla 2020-01-13 769 > 09309093d5e8f87 Jonathan Marek 2020-09-08 770 #if IS_ENABLED(CONFIG_SLIMBUS) > 02efb49aa805cee Srinivas Kandagatla 2020-01-13 @771 if (dev->parent->bus == &slimbus_bus) { > 5bd773242f75da3 Jonathan Marek 2020-09-05 772 #else > 5bd773242f75da3 Jonathan Marek 2020-09-05 773 if (false) { > 5bd773242f75da3 Jonathan Marek 2020-09-05 774 #endif > :::::: The code at line 767 was first introduced by commit > :::::: 02efb49aa805cee643a643ab61a1118c2fd08b80 soundwire: qcom: add support for SoundWire controller > > :::::: TO: Srinivas Kandagatla <srinivas.kandagatla(a)linaro.org> > :::::: CC: Vinod Koul <vkoul(a)kernel.org> config SOUNDWIRE_QCOM tristate "Qualcomm SoundWire Master driver" imply SLIMBUS depends on SND_SOC The kernel config that was attached has: CONFIG_SOUNDWIRE_QCOM=y CONFIG_SLIMBUS=m I expected that "imply" would make SLIMBUS=y since SOUNDWIRE_QCOM=y, but I guess that's not the case. :( Any ideas about what to do here? -- ~Randy

3 4

[PATCH] ASoC: fsl_xcvr: fix potential resource leak
by Viorel Suman (OSS) 25 Nov '20

25 Nov '20

From: Viorel Suman <viorel.suman(a)nxp.com> "fw" variable must be relased before return. Signed-off-by: Viorel Suman <viorel.suman(a)nxp.com> --- sound/soc/fsl/fsl_xcvr.c | 1 + 1 file changed, 1 insertion(+) diff --git a/sound/soc/fsl/fsl_xcvr.c b/sound/soc/fsl/fsl_xcvr.c index 2a28810d0e29..3d58c88ea603 100644 --- a/sound/soc/fsl/fsl_xcvr.c +++ b/sound/soc/fsl/fsl_xcvr.c @@ -706,6 +706,7 @@ static int fsl_xcvr_load_firmware(struct fsl_xcvr *xcvr) /* RAM is 20KiB = 16KiB code + 4KiB data => max 10 pages 2KiB each */ if (rem > 16384) { dev_err(dev, "FW size %d is bigger than 16KiB.\n", rem); + release_firmware(fw); return -ENOMEM; } -- 2.26.2

2 1

[PATCH] ASoC: wm_adsp: fix error return code in wm_adsp_load()
by Luo Meng 25 Nov '20

25 Nov '20

Fix to return a negative error code from the error handling case instead of 0 in function wm_adsp_load(), as done elsewhere in this function. Fixes: 170b1e123f38 ("ASoC: wm_adsp: Add support for new Halo core DSPs") Reported-by: Hulk Robot <hulkci(a)huawei.com> Signed-off-by: Luo Meng <luomeng12(a)huawei.com> --- sound/soc/codecs/wm_adsp.c | 1 + 1 file changed, 1 insertion(+) diff --git a/sound/soc/codecs/wm_adsp.c b/sound/soc/codecs/wm_adsp.c index bcf18bf15a02..e61d00486c65 100644 --- a/sound/soc/codecs/wm_adsp.c +++ b/sound/soc/codecs/wm_adsp.c @@ -1937,6 +1937,7 @@ static int wm_adsp_load(struct wm_adsp *dsp) mem = wm_adsp_find_region(dsp, type); if (!mem) { adsp_err(dsp, "No region of type: %x\n", type); + ret = -EINVAL; goto out_fw; } -- 2.25.4

3 2

[PATCH 0/4] ASoC: SOF: Add 'memory_info' file to debugfs
by Kai Vehmanen 25 Nov '20

25 Nov '20

Series that adds a new debugfs entry to query status of SOF FW memory allocation at runtime. Information on used and free memory is shown separately for each zone. A new IPC message is added for this purpose and ABI minor revision is bumped accordingly to 3.18. To implement this, additional FW configuration data needs to be parsed from the firmware file and this is done in the first patch. A few cleanups to surrounding code is done in separate patches. Karol Trzcinski (4): ASoC: SOF: ext_manifest: Parse firmware config dictionary ASoC: SOF: Improve code alignment in header.h ASoC: SOF: Change section comment for SOF_IPC_TEST_ ASoC: SOF: Add `memory_info` file to debugfs include/sound/sof/debug.h | 41 +++++++++++ include/sound/sof/ext_manifest.h | 20 ++++++ include/sound/sof/header.h | 10 ++- include/uapi/sound/sof/abi.h | 2 +- sound/soc/sof/debug.c | 117 +++++++++++++++++++++++++++++++ sound/soc/sof/ipc.c | 9 +++ sound/soc/sof/loader.c | 51 ++++++++++++++ sound/soc/sof/sof-priv.h | 2 + 8 files changed, 248 insertions(+), 4 deletions(-) create mode 100644 include/sound/sof/debug.h -- 2.28.0

2 5

[PATCH v2] ALSA: compress: allow pause and resume during draining
by Gyeongtaek Lee 25 Nov '20

25 Nov '20

With a stream with low bitrate, user can't pause or resume the stream near the end of the stream because current ALSA doesn't allow it. If the stream has very low bitrate enough to store whole stream into the buffer, user can't do anything except stop the stream and then restart it from the first because most of applications call draining after sending last frame to the kernel. If pause, resume are allowed during draining, user experience can be enhanced. To prevent malfunction in HW drivers which don't support pause during draining, pause during draining will only work if HW driver enable this feature explicitly by calling snd_compr_use_pause_in_draining(). Signed-off-by: Gyeongtaek Lee <gt82.lee(a)samsung.com> Cc: stable(a)vger.kernel.org --- include/sound/compress_driver.h | 17 +++++++++++++ sound/core/compress_offload.c | 44 +++++++++++++++++++++++++++------ 2 files changed, 53 insertions(+), 8 deletions(-) diff --git a/include/sound/compress_driver.h b/include/sound/compress_driver.h index 70cbc5095e72..5a0d6737de5e 100644 --- a/include/sound/compress_driver.h +++ b/include/sound/compress_driver.h @@ -67,6 +67,7 @@ struct snd_compr_runtime { * @metadata_set: metadata set flag, true when set * @next_track: has userspace signal next track transition, true when set * @partial_drain: undergoing partial_drain for stream, true when set + * @pause_in_draining: paused during draining state, true when set * @private_data: pointer to DSP private data * @dma_buffer: allocated buffer if any */ @@ -80,6 +81,7 @@ struct snd_compr_stream { bool metadata_set; bool next_track; bool partial_drain; + bool pause_in_draining; void *private_data; struct snd_dma_buffer dma_buffer; }; @@ -142,6 +144,7 @@ struct snd_compr_ops { * @direction: Playback or capture direction * @lock: device lock * @device: device id + * @use_pause_in_draining: allow pause in draining, true when set */ struct snd_compr { const char *name; @@ -152,6 +155,7 @@ struct snd_compr { unsigned int direction; struct mutex lock; int device; + bool use_pause_in_draining; #ifdef CONFIG_SND_VERBOSE_PROCFS /* private: */ char id[64]; @@ -166,6 +170,19 @@ int snd_compress_deregister(struct snd_compr *device); int snd_compress_new(struct snd_card *card, int device, int type, const char *id, struct snd_compr *compr); +/** + * snd_compr_use_pause_in_draining - Allow pause and resume in draining state + * @substream: compress substream to set + * + * Allow pause and resume in draining state. + * Only HW driver supports this transition can call this API. + */ +static inline void snd_compr_use_pause_in_draining( + struct snd_compr_stream *substream) +{ + substream->device->use_pause_in_draining = true; +} + /* dsp driver callback apis * For playback: driver should call snd_compress_fragment_elapsed() to let the * framework know that a fragment has been consumed from the ring buffer diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c index 0e53f6f31916..a071485383ed 100644 --- a/sound/core/compress_offload.c +++ b/sound/core/compress_offload.c @@ -708,11 +708,24 @@ static int snd_compr_pause(struct snd_compr_stream *stream) { int retval; - if (stream->runtime->state != SNDRV_PCM_STATE_RUNNING) + switch (stream->runtime->state) { + case SNDRV_PCM_STATE_RUNNING: + retval = stream->ops->trigger(stream, + SNDRV_PCM_TRIGGER_PAUSE_PUSH); + if (!retval) + stream->runtime->state = SNDRV_PCM_STATE_PAUSED; + break; + case SNDRV_PCM_STATE_DRAINING: + if (!stream->device->use_pause_in_draining) + return -EPERM; + retval = stream->ops->trigger(stream, + SNDRV_PCM_TRIGGER_PAUSE_PUSH); + if (!retval) + stream->pause_in_draining = true; + break; + default: return -EPERM; - retval = stream->ops->trigger(stream, SNDRV_PCM_TRIGGER_PAUSE_PUSH); - if (!retval) - stream->runtime->state = SNDRV_PCM_STATE_PAUSED; + } return retval; } @@ -720,11 +733,25 @@ static int snd_compr_resume(struct snd_compr_stream *stream) { int retval; - if (stream->runtime->state != SNDRV_PCM_STATE_PAUSED) + switch (stream->runtime->state) { + case SNDRV_PCM_STATE_PAUSED: + retval = stream->ops->trigger(stream, + SNDRV_PCM_TRIGGER_PAUSE_RELEASE); + if (!retval) + stream->runtime->state = SNDRV_PCM_STATE_RUNNING; + break; + case SNDRV_PCM_STATE_DRAINING: + if (!stream->device->use_pause_in_draining || + !stream->pause_in_draining) + return -EPERM; + retval = stream->ops->trigger(stream, + SNDRV_PCM_TRIGGER_PAUSE_RELEASE); + if (!retval) + stream->pause_in_draining = false; + break; + default: return -EPERM; - retval = stream->ops->trigger(stream, SNDRV_PCM_TRIGGER_PAUSE_RELEASE); - if (!retval) - stream->runtime->state = SNDRV_PCM_STATE_RUNNING; + } return retval; } @@ -767,6 +794,7 @@ static int snd_compr_stop(struct snd_compr_stream *stream) /* clear flags and stop any drain wait */ stream->partial_drain = false; stream->metadata_set = false; + stream->pause_in_draining = false; snd_compr_drain_notify(stream); stream->runtime->total_bytes_available = 0; stream->runtime->total_bytes_transferred = 0; -- 2.21.0

3 5

[PATCH] slimbus: qcom-ngd-ctrl: Avoid sending power requests without QMI
by Bjorn Andersson 25 Nov '20

25 Nov '20

Attempting to send a power request during PM operations, when the QMI handle isn't initialized results in a NULL pointer dereference. So check if the QMI handle has been initialized before attempting to post the power requests. Fixes: 917809e2280b ("slimbus: ngd: Add qcom SLIMBus NGD driver") Signed-off-by: Bjorn Andersson <bjorn.andersson(a)linaro.org> --- drivers/slimbus/qcom-ngd-ctrl.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/drivers/slimbus/qcom-ngd-ctrl.c b/drivers/slimbus/qcom-ngd-ctrl.c index d8decb345e9d..c502e9e93965 100644 --- a/drivers/slimbus/qcom-ngd-ctrl.c +++ b/drivers/slimbus/qcom-ngd-ctrl.c @@ -1229,6 +1229,9 @@ static int qcom_slim_ngd_runtime_resume(struct device *dev) struct qcom_slim_ngd_ctrl *ctrl = dev_get_drvdata(dev); int ret = 0; + if (!ctrl->qmi.handle) + return 0; + if (ctrl->state >= QCOM_SLIM_NGD_CTRL_ASLEEP) ret = qcom_slim_ngd_power_up(ctrl); if (ret) { @@ -1616,6 +1619,9 @@ static int __maybe_unused qcom_slim_ngd_runtime_suspend(struct device *dev) struct qcom_slim_ngd_ctrl *ctrl = dev_get_drvdata(dev); int ret = 0; + if (!ctrl->qmi.handle) + return 0; + ret = qcom_slim_qmi_power_request(ctrl, false); if (ret && ret != -EBUSY) dev_info(ctrl->dev, "slim resource not idle:%d\n", ret); -- 2.29.2

2 1

[PATCH 0/5] soundwire: only clear valid interrupts
by Bard Liao 25 Nov '20

25 Nov '20

We wrote 1 to the handled interrupts bits along with 0 to all other bits to the SoundWire DPx interrupt register. However, DP0 has reserved fields and the read-only SDCA_CASCADE bit. DPN also has reserved fields. We should not try to write values in these fields. Besides, we deal with pending interrupts in a loop but we didn't reset the slave_notify status. Pierre-Louis Bossart (5): soundwire: bus: add comments to explain interrupt loop filter soundwire: bus: reset slave_notify status at each loop soundwire: registers: add definitions for clearable interrupt fields soundwire: bus: only clear valid DP0 interrupts soundwire: bus: only clear valid DPN interrupts drivers/soundwire/bus.c | 27 +++++++++++++++++-------- include/linux/soundwire/sdw_registers.h | 11 ++++++++++ 2 files changed, 30 insertions(+), 8 deletions(-) -- 2.17.1

2 6