Re: [PATCH RFC v2] ALSA: pci: add analog support for ESI MAYA44e

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [High] ALSA: maya44e: pseudonymous Signed-off-by trailer and
  MODULE_AUTHOR
- [Medium] ALSA: maya44e: race between maya_prepare() and
  maya_pcm_reset()
--

commit f092453d5ece85565f1b326e6596d2a12340f871
Author: Nordschamane <nordschamane1@proton.me>

> ALSA: pci: add analog support for ESI MAYA44e
>
> Add four-channel analog playback and capture for the ESI MAYA44e PCIe
> card (19fe:7000, subsystem 0e51:0001). Playback and capture share the
> sample clock and can use independent period sizes. Expose analog source,
> gain, direct monitoring, DAC and crossmix controls.
>
> The ES7010L register sequence comes from observed PCI transactions and
> analysis of the ESI Windows driver. The existing ALSA PCI MAYA44 driver
> was a reference for the WM8776 command format.
>
> OpenAI Codex assisted with PCI trace analysis, driver implementation,
> test tooling and the changelog. The requests covered analog playback,
> capture, mixer controls and suspend/resume. For v2, Sashiko flagged
> potential issues. Codex checked the report against the driver and ALSA
> core and prepared the changes below.
>
> Signed-off-by: Nordschamane <nordschamane1@proton.me>

[Severity: High]
The Author header and Signed-off-by trailer use the handle "Nordschamane"
rather than a real full name as required by the Developer's Certificate of
Origin, and MODULE_AUTHOR() at the bottom of the driver uses the generic
placeholder "MAYA44e Linux reverse-engineering project".

Also, the sentences at the end of the commit message ("For v2, ... prepared
the changes below.") refer to the notes below the "---" separator and
should be moved out of the permanent changelog.

> diff --git a/sound/pci/maya44e.c b/sound/pci/maya44e.c
> new file mode 100644
> index 0000000000000..961dbde73219e
> --- /dev/null
> +++ b/sound/pci/maya44e.c

[ ... ]

> +static int maya_pcm_reset(struct snd_pcm_substream *s)
> +{
> +	struct maya *m = snd_pcm_substream_chip(s);
> +	int err;
> +
> +	/* ALSA rebases appl_ptr from this hw_ptr under the same stream lock. */
> +	err = snd_pcm_lib_reset(s);
> +	if (err < 0)
> +		return err;
> +	m->last_appl = s->runtime->status->hw_ptr;
> +	return 0;
> +}
> +
> +static int maya_prepare(struct snd_pcm_substream *s)
> +{
> +	struct maya *m = snd_pcm_substream_chip(s);
> +	unsigned long flags;
> +	int err;

[ ... ]

> +	/* A linked prepare can leave the playback buffer open to ack. */
> +	snd_pcm_stream_lock_irqsave(s, flags);
> +	memset(m->ring[0], 0, m->buffer_frames * 8);
> +	memset(m->ring[1], 0, m->buffer_frames * 8);
> +	memset(s->runtime->dma_area, 0,
> +	       s->runtime->buffer_size * maya_sample_bytes(s->runtime->format) * 4);
> +	dma_wmb();
> +	snd_pcm_stream_unlock_irqrestore(s, flags);
> +	return 0;
> +}

[Severity: Medium]
Can a concurrent maya_ack() on a linked playback substream race between
maya_prepare() and maya_pcm_reset() when SNDRV_PCM_IOCTL_PREPARE is called
on the capture substream?

When playback and capture are linked and playback is in
SNDRV_PCM_STATE_PREPARED or SNDRV_PCM_STATE_XRUN with a non-zero
m->last_appl, preparing the capture substream only sets
buffer_accessing = -1 on the capture runtime while leaving playback's
buffer_accessing at 0.  snd_pcm_do_prepare() calls ops->prepare() without
the stream lock, then updates PM QoS, and only then takes the stream lock
to call snd_pcm_reset_substream() -> maya_pcm_reset():

CPU0 (capture PREPARE)            CPU1 (playback SYNC_PTR)
snd_pcm_do_prepare(playback)
  maya_prepare(playback)
    snd_pcm_stream_lock_irqsave()
    memset(m->ring[0..1], 0, ...)
    snd_pcm_stream_unlock_irqrestore()
                                  snd_pcm_sync_ptr(playback)
                                    snd_pcm_stream_lock_irq()
                                    maya_ack(playback)
                                      writes m->ring[0..1] at old last_appl
                                      m->last_appl = appl
                                    snd_pcm_stream_unlock_irq()
  pcm_qos_req_update()
  snd_pcm_reset_substream(playback)
    maya_pcm_reset(playback)
      snd_pcm_lib_reset(playback)
      m->last_appl = 0;           <-- ring[0..1] retains stale samples

This leaves non-zero samples from the pre-reset appl_ptr offset in
m->ring[0] and m->ring[1] and discards the acknowledged pointer update.
Zeroing m->ring[0..1] and runtime->dma_area inside maya_pcm_reset() under
the same stream lock acquisition that resets appl_ptr and m->last_appl
would close this window.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261111-codex-alsa-maya44e-v2-1-c5f94d722b54@proton.me?part=1