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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [High] ALSA: maya44e: pseudonymous Signed-off-by trailer and
  MODULE_AUTHOR
- [Medium] ALSA: maya44e: missing reset callback in maya_capture_ops
- [Medium] ALSA: maya44e: all-ones MMIO read treated as reset completion
  in maya_rearm()
--

commit 08f5d983b63627639e3f62af3419f16aa31a3501
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 playback DMA rings and ALSA staging buffer are cleared during the
> reset that completes prepare. This keeps clearing and pointer rebasing
> under the same PCM stream lock, including for linked streams.
>
> 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.
>
> Signed-off-by: Nordschamane <nordschamane1@proton.me>

[Severity: High]
Does the Signed-off-by trailer identify a known individual contributor as
required by Documentation/process/submitting-patches.rst for Developer
Certificate of Origin certification?

The commit Author and Signed-off-by trailer use the single-word handle
"Nordschamane", and MODULE_AUTHOR() in sound/pci/maya44e.c uses the
generic placeholder "MAYA44e Linux reverse-engineering project" instead of
an individual author name.

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

[ ... ]

> +static int maya_rearm(struct maya *m, unsigned int pair)
> +{
> +	unsigned int b = 0xc0 + pair * 0x20;
> +	unsigned int irq = 0x198 + pair * 4;
> +	unsigned int i;
> +
> +	mw32(m, b, 0xff000000);
> +	mw8(m, irq + 2, 2);
> +	mw8(m, b, 1);
> +	for (i = 0; i < MAYA_POLL_LIMIT; ++i) {
> +		if (mr32(m, b) & 1)
> +			break;
> +		/* Reset callers may hold m->lock, so this wait must not sleep. */
> +		udelay(10);
> +	}
> +	if (i == MAYA_POLL_LIMIT) {
> +		maya_fatal(m);
> +		return -ETIMEDOUT;
> +	}

[Severity: Medium]
Does maya_rearm() treat an all-ones MMIO read (~0U) from an inaccessible
PCIe device as a completed DMA reset?

Unlike maya_irq(), which checks for summary == ~0U, and maya_codec_ready(),
which waits for bit 7 to clear, maya_rearm() and maya_capture_reset() only
check (mr32(m, b) & 1).  If the PCIe link drops during prepare or resume,
0xffffffff satisfies the condition on the first iteration instead of
calling maya_fatal() and returning -ETIMEDOUT, allowing the driver to
rewrite DMA descriptors, mark streams as running, or re-enable bus
mastering on an inaccessible device.

[ ... ]

> +static const struct snd_pcm_ops maya_capture_ops = {
> +	.open = maya_capture_open,
> +	.close = maya_capture_close,
> +	.ioctl = snd_pcm_lib_ioctl,
> +	.hw_params = maya_hw_params,
> +	.hw_free = maya_hw_free,
> +	.prepare = maya_capture_prepare,
> +	.trigger = maya_capture_trigger,
> +	.sync_stop = maya_sync_stop,
> +	.pointer = maya_capture_pointer,
> +};

[Severity: Medium]
Should maya_capture_ops set .reset = snd_pcm_lib_reset?

Without .reset, capture streams fall back to the legacy multi-step reset
path in snd_pcm_reset_substream() and snd_pcm_post_prepare():

CPU0 (SNDRV_PCM_IOCTL_RESET)      CPU1 (<hardirq>)
snd_pcm_reset_substream(capture)
  snd_pcm_lib_ioctl_reset()
    lock(capture)
    status->hw_ptr %= buffer_size
    unlock(capture)
                                  maya_irq()
                                    snd_pcm_period_elapsed(capture)
                                      snd_pcm_update_hw_ptr0()
                                        /* uses un-reset hw_ptr_base */
  lock(capture)
  runtime->hw_ptr_base = 0        <-- status->hw_ptr left offset or dropped
  unlock(capture)

Once capture has run past one buffer so hw_ptr_base >= buffer_size, a
period interrupt arriving in that window combines the old hw_ptr_base with
the reduced status->hw_ptr, leaving status->hw_ptr offset by buffer_size
or triggering hw_ptr_error().  In addition, during a linked prepare where
capture precedes playback, snd_pcm_post_prepare() overwrites capture
appl_ptr without holding the capture stream lock after playback finishes
maya_prepare().

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