linux-kernel - Re: [PATCH v5 3/5] ASoC: q6apm-dai: make use of q6apm_get_hw

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CACr-zFBgQsiO=EVD-sCyvQHonbRLS+7J=q+Y8WNbwSPkF_5kug@mail.gmail.com>
Date: Mon, 31 Mar 2025 17:35:57 +0200
From: Christopher Obbard <christopher.obbard@...aro.org>
To: srinivas.kandagatla@...aro.org
Cc: broonie@...nel.org, perex@...ex.cz, tiwai@...e.com, 
	krzysztof.kozlowski@...aro.org, linux-sound@...r.kernel.org, 
	linux-arm-msm@...r.kernel.org, linux-kernel@...r.kernel.org, 
	dmitry.baryshkov@...aro.org, johan+linaro@...nel.org, stable@...r.kernel.org
Subject: Re: [PATCH v5 3/5] ASoC: q6apm-dai: make use of q6apm_get_hw_pointer

Hi Srini,

On Fri, 14 Mar 2025 at 18:49, <srinivas.kandagatla@...aro.org> wrote:
>
> From: Srinivas Kandagatla <srinivas.kandagatla@...aro.org>
>
> With the existing code, the buffer position is only reset in pointer
> callback, which leaves the possiblity of it going over the size of
> buffer size and reporting incorrect position to userspace.
>
> Without this patch, its possible to see errors like:
> snd-x1e80100 sound: invalid position: pcmC0D0p:0, pos = 12288, buffer size = 12288, period size = 1536
> snd-x1e80100 sound: invalid position: pcmC0D0p:0, pos = 12288, buffer size = 12288, period size = 1536
>
> Fixes: 9b4fe0f1cd79 ("ASoC: qdsp6: audioreach: add q6apm-dai support")
> Cc: stable@...r.kernel.org
> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@...aro.org>
> Tested-by: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
> Tested-by: Johan Hovold <johan+linaro@...nel.org>

Seems like I missed adding my T-b to this patch. If it's not too late,
please add:

Tested-by: Christopher Obbard <christopher.obbard@...aro.org>

> ---
>  sound/soc/qcom/qdsp6/q6apm-dai.c | 23 ++++-------------------
>  1 file changed, 4 insertions(+), 19 deletions(-)
>
> diff --git a/sound/soc/qcom/qdsp6/q6apm-dai.c b/sound/soc/qcom/qdsp6/q6apm-dai.c
> index 9d8e8e37c6de..90cb24947f31 100644
> --- a/sound/soc/qcom/qdsp6/q6apm-dai.c
> +++ b/sound/soc/qcom/qdsp6/q6apm-dai.c
> @@ -64,7 +64,6 @@ struct q6apm_dai_rtd {
>         phys_addr_t phys;
>         unsigned int pcm_size;
>         unsigned int pcm_count;
> -       unsigned int pos;       /* Buffer position */
>         unsigned int periods;
>         unsigned int bytes_sent;
>         unsigned int bytes_received;
> @@ -124,23 +123,16 @@ static void event_handler(uint32_t opcode, uint32_t token, void *payload, void *
>  {
>         struct q6apm_dai_rtd *prtd = priv;
>         struct snd_pcm_substream *substream = prtd->substream;
> -       unsigned long flags;
>
>         switch (opcode) {
>         case APM_CLIENT_EVENT_CMD_EOS_DONE:
>                 prtd->state = Q6APM_STREAM_STOPPED;
>                 break;
>         case APM_CLIENT_EVENT_DATA_WRITE_DONE:
> -               spin_lock_irqsave(&prtd->lock, flags);
> -               prtd->pos += prtd->pcm_count;
> -               spin_unlock_irqrestore(&prtd->lock, flags);
>                 snd_pcm_period_elapsed(substream);
>
>                 break;
>         case APM_CLIENT_EVENT_DATA_READ_DONE:
> -               spin_lock_irqsave(&prtd->lock, flags);
> -               prtd->pos += prtd->pcm_count;
> -               spin_unlock_irqrestore(&prtd->lock, flags);
>                 snd_pcm_period_elapsed(substream);
>                 if (prtd->state == Q6APM_STREAM_RUNNING)
>                         q6apm_read(prtd->graph);
> @@ -247,7 +239,6 @@ static int q6apm_dai_prepare(struct snd_soc_component *component,
>         }
>
>         prtd->pcm_count = snd_pcm_lib_period_bytes(substream);
> -       prtd->pos = 0;
>         /* rate and channels are sent to audio driver */
>         ret = q6apm_graph_media_format_shmem(prtd->graph, &cfg);
>         if (ret < 0) {
> @@ -445,16 +436,12 @@ static snd_pcm_uframes_t q6apm_dai_pointer(struct snd_soc_component *component,
>         struct snd_pcm_runtime *runtime = substream->runtime;
>         struct q6apm_dai_rtd *prtd = runtime->private_data;
>         snd_pcm_uframes_t ptr;
> -       unsigned long flags;
>
> -       spin_lock_irqsave(&prtd->lock, flags);
> -       if (prtd->pos == prtd->pcm_size)
> -               prtd->pos = 0;
> -
> -       ptr =  bytes_to_frames(runtime, prtd->pos);
> -       spin_unlock_irqrestore(&prtd->lock, flags);
> +       ptr = q6apm_get_hw_pointer(prtd->graph, substream->stream) * runtime->period_size;
> +       if (ptr)
> +               return ptr - 1;
>
> -       return ptr;
> +       return 0;
>  }
>
>  static int q6apm_dai_hw_params(struct snd_soc_component *component,
> @@ -669,8 +656,6 @@ static int q6apm_dai_compr_set_params(struct snd_soc_component *component,
>         prtd->pcm_size = runtime->fragments * runtime->fragment_size;
>         prtd->bits_per_sample = 16;
>
> -       prtd->pos = 0;
> -
>         if (prtd->next_track != true) {
>                 memcpy(&prtd->codec, codec, sizeof(*codec));
>
> --
> 2.39.5
>
>