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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential
issues to consider:
- [High] ALSA: maya44e: pseudonymous Signed-off-by trailer and
  MODULE_AUTHOR
- [High] ALSA: maya44e: unrestored D0 power state in maya_resume() error
  path
- [High] ALSA: maya44e: NULL runtime->dma_area dereference in maya_ack()
  after hw_free
- [Medium] ALSA: maya44e: missing joint-duplex rate constraint in
  maya_open()
- [Medium] ALSA: maya44e: skipped PGA updates at zero volume in
  maya_input_levels()
- [Medium] ALSA: maya44e: unlocked last_appl updates in maya_pcm_ioctl()
  and maya_prepare()
--

commit 06d75b60c918848008af6c5fc9d3ea134cf8c6cd
Author: Nordschamane <nordschamane1@proton.me>

ALSA: pci: add analog support for ESI MAYA44e

This commit adds an ALSA PCI driver for the ESI MAYA44e PCIe audio
interface supporting four-channel analog playback and capture, mixer
controls, and power management.

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

[ ... ]

> +static const struct snd_pcm_hardware maya_pcm_hardware = {
> +	.info = SNDRV_PCM_INFO_INTERLEAVED | SNDRV_PCM_INFO_BLOCK_TRANSFER |
> +		SNDRV_PCM_INFO_MMAP | SNDRV_PCM_INFO_MMAP_VALID |
> +		SNDRV_PCM_INFO_SYNC_APPLPTR | SNDRV_PCM_INFO_NO_REWINDS,
> +	.formats = SNDRV_PCM_FMTBIT_S16_LE | SNDRV_PCM_FMTBIT_S24_3LE |
> +		SNDRV_PCM_FMTBIT_S24_LE | SNDRV_PCM_FMTBIT_S32_LE,
> +	.rates = SNDRV_PCM_RATE_8000_48000 | SNDRV_PCM_RATE_24000 |
> +		SNDRV_PCM_RATE_88200 | SNDRV_PCM_RATE_96000,
> +	.rate_min = 8000, .rate_max = 96000,
> +	.channels_min = 4, .channels_max = 4,
> +	.buffer_bytes_max = MAYA_PCM_BYTES_MAX,
> +	.period_bytes_min = MAYA_MIN_PERIOD * 8,
> +	.period_bytes_max = MAYA_MAX_PERIOD * 16,
> +	.periods_min = 2, .periods_max = 2,
> +};
> +
> +/* Full duplex shares one clock, but each direction may use its own period.
> + * Reserve at hw_params, not just while running, to protect prepared peers.
> + */
> +static int maya_hw_params(struct snd_pcm_substream *s, struct snd_pcm_hw_params *p)
> +{
> +	struct maya *m = snd_pcm_substream_chip(s);
> +	unsigned int direction = s->stream;
> +	int err = 0;
> +
> +	mutex_lock(&m->codec_lock);
> +	if (m->reserved_rate[1 - direction] &&
> +	    m->reserved_rate[1 - direction] != params_rate(p))
> +		err = -EBUSY;
> +	else
> +		m->reserved_rate[direction] = params_rate(p);
> +	mutex_unlock(&m->codec_lock);
> +	return err;
> +}

[Severity: Medium]
Should maya_pcm_hardware include SNDRV_PCM_INFO_JOINT_DUPLEX and constrain
SNDRV_PCM_HW_PARAM_RATE when the opposite stream has already reserved a
rate?

During snd_pcm_hw_params(), snd_pcm_hw_params_choose() picks the minimum
rate in the refined interval before calling maya_hw_params().  When one
stream has reserved a rate above the minimum (such as 48000 Hz) and a
second stream passes a rate range containing 48000 Hz, ALSA selects 8000
Hz or 32000 Hz and maya_hw_params() rejects it with -EBUSY.  In addition,
because playback allows 8000..24000 Hz while capture starts at 32000 Hz,
reserving a sub-32 kHz playback rate prevents capture from configuring
hw_params at all.

[ ... ]

> +static int maya_pcm_ioctl(struct snd_pcm_substream *s, unsigned int cmd, void *arg)
> +{
> +	struct maya *m = snd_pcm_substream_chip(s);
> +	int err = snd_pcm_lib_ioctl(s, cmd, arg);
> +
> +	if (!err && cmd == SNDRV_PCM_IOCTL1_RESET)
> +		m->last_appl = s->runtime->status->hw_ptr;
> +	return err;
> +}
> +
> +static int maya_prepare(struct snd_pcm_substream *s)
> +{

[ ... ]

> +	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);
> +	m->last_appl = 0;
> +	dma_wmb();
> +	return 0;
> +}

[Severity: Medium]
Can updating m->last_appl without holding the PCM stream lock desynchronize
it from runtime->control->appl_ptr?

In snd_pcm_reset(), snd_pcm_do_reset() -> snd_pcm_lib_ioctl_reset() drops
the PCM stream lock before maya_pcm_ioctl() reads runtime->status->hw_ptr,
and snd_pcm_post_reset() re-acquires the lock afterward to copy hw_ptr into
runtime->control->appl_ptr:

CPU0                                  CPU1
snd_pcm_reset()
  snd_pcm_do_reset()
    maya_pcm_ioctl()
      snd_pcm_lib_ioctl_reset()
      m->last_appl = ...->hw_ptr;
                                      maya_irq()
                                        snd_pcm_period_elapsed()
                                          /* advances hw_ptr */
  snd_pcm_post_reset()
    appl_ptr = ...->hw_ptr;           <-- appl_ptr != m->last_appl

When a period interrupt lands in that window on a running playback stream,
m->last_appl lags behind appl_ptr by a period, causing a subsequent write
of more than one period in maya_ack() to compute count > r->buffer_size and
fail with -EINVAL.  Similarly, snd_pcm_sync_ptr() calls maya_ack() under
only pcm_stream_lock_irq() without holding buffer_mutex, so unlocked writes
to m->last_appl in maya_prepare() and maya_pcm_ioctl() race with concurrent
SNDRV_PCM_IOCTL_SYNC_PTR calls.

[ ... ]

> +static int maya_ack(struct snd_pcm_substream *s)
> +{
> +	struct maya *m = snd_pcm_substream_chip(s);
> +	struct snd_pcm_runtime *r = s->runtime;
> +	snd_pcm_uframes_t appl = READ_ONCE(r->control->appl_ptr);
> +	snd_pcm_sframes_t count = appl - m->last_appl;
> +	snd_pcm_uframes_t frame;
> +	unsigned int sample_bytes = maya_sample_bytes(r->format);
> +	__le32 *out12 = m->ring[0], *out34 = m->ring[1];
> +
> +	if (READ_ONCE(m->fault) || READ_ONCE(m->removing) || READ_ONCE(m->suspended))
> +		return -EIO;
> +	if (!sample_bytes)
> +		return -EINVAL;
> +	if (count < 0)
> +		count += r->boundary;
> +	if (count < 0 || count > (snd_pcm_sframes_t)r->buffer_size) {
> +		dev_err_ratelimited(&m->pci->dev,
> +				    "invalid appl_ptr delta=%ld old=%lu new=%lu boundary=%lu\n",
> +				    count, m->last_appl, appl, r->boundary);
> +		return -EINVAL;
> +	}
> +	for (frame = 0; frame < (snd_pcm_uframes_t)count; ++frame) {
> +		unsigned int slot = (m->last_appl + frame) % r->buffer_size;
> +		const u8 *source = r->dma_area + slot * sample_bytes * 4;
> +
> +		out12[slot * 2] = cpu_to_le32(maya_unpack_sample(source, r->format));

[Severity: High]
Can maya_ack() dereference a NULL or concurrently freed r->dma_area if
SNDRV_PCM_IOCTL_SYNC_PTR is called after SNDRV_PCM_IOCTL_HW_FREE?

When SNDRV_PCM_IOCTL_HW_FREE runs after SNDRV_PCM_IOCTL_HW_PARAMS,
do_hw_free() -> snd_pcm_lib_free_pages() frees the managed vmalloc buffer
and sets r->dma_area to NULL under r->buffer_mutex, while leaving
r->format, r->buffer_size and r->boundary populated.  Because
snd_pcm_sync_ptr() only takes pcm_stream_lock_irq() and calls
pcm_lib_apply_appl_ptr() -> maya_ack() without checking r->state or
r->dma_area:

sound/core/pcm_native.c:snd_pcm_sync_ptr() {
    ...
	scoped_guard(pcm_stream_lock_irq, substream) {
		if (!(sflags & SNDRV_PCM_SYNC_PTR_APPL)) {
			err = pcm_lib_apply_appl_ptr(substream, scontrol.appl_ptr);
    ...
}

calling SNDRV_PCM_IOCTL_SYNC_PTR with flags = 0 and a non-zero appl_ptr
passes the bounds checks in maya_ack() and dereferences NULL r->dma_area in
maya_unpack_sample() (or races with snd_pcm_lib_free_pages() freeing the
buffer).

[ ... ]

> +static int maya_input_levels(struct maya *m, unsigned int pair)
> +{
> +	unsigned int ch;
> +	int err;
> +
> +	for (ch = 0; ch < 2; ++ch) {
> +		unsigned int volume = m->input_volume[pair][ch];
> +		unsigned int raw = 62 + volume + m->input_gain[pair];
> +
> +		/* -infinity is a digital gate in maya_capture_period. Keep the
> +		 * last PGA setting there: code 0 causes a bias/settling transient
> +		 * on this board when returning to normal gain.
> +		 */
> +		if (!volume)
> +			continue;
> +		err = maya_codec_write(m, pair, 14 + ch, 0x100 | raw);
> +		if (err)
> +			return err;
> +	}
> +	return 0;
> +}

[Severity: Medium]
Does skipping the WM8776 R14/R15 write when volume is 0 leave the analog
PGA unmuted on the hardware direct-monitoring path and after resume?

Input 1-2/3-4 Capture Volume advertises 0 as mute in input_volume_db, and
maya_capture_period() gates captured samples in software.  However, WM8776
R14/R15 also feeds the analog monitor bypass enabled by Input 1-2/3-4
Monitor Playback Switch, so setting volume to 0 leaves the monitored input
audible at its previous gain and ignores Input Capture Gain updates made
while volume is 0.  Furthermore, on S3 resume maya_codec_init() resets R14
and R15 to 0x1cf (0 dB), and maya_restore_after_sleep() ->
maya_input_levels() then skips any channel whose cached volume is 0.

[ ... ]

> +static int maya_suspend(struct device *dev)
> +{
> +	struct snd_card *card = dev_get_drvdata(dev);
> +	struct maya *m = card->private_data;
> +	unsigned long flags;
> +
> +	snd_power_change_state(card, SNDRV_CTL_POWER_D3hot);
> +	/* ALSA stream locks must be acquired before our register/codec locks. */
> +	snd_pcm_suspend_all(m->pcm);
> +	mutex_lock(&m->codec_lock);
> +	spin_lock_irqsave(&m->lock, flags);
> +	m->suspend_rate = m->rate;
> +	if (!m->suspended) {
> +		mw8(m, 0x481, mr8(m, 0x481) & ~0x40);
> +		maya_quiesce(m);
> +		m->suspended = true;
> +	}
> +	spin_unlock_irqrestore(&m->lock, flags);
> +	synchronize_irq(m->pci->irq);
> +	mutex_unlock(&m->codec_lock);
> +	/* The gate prevents new work; complete any pending peer notification. */
> +	flush_work(&m->fault_work);
> +	return 0;
> +}
> +
> +static int maya_resume(struct device *dev)
> +{
> +	struct snd_card *card = dev_get_drvdata(dev);
> +	struct maya *m = card->private_data;
> +	unsigned long flags;
> +	int err;
> +
> +	mutex_lock(&m->codec_lock);
> +	if (!m->suspended) {
> +		mutex_unlock(&m->codec_lock);
> +		return 0;
> +	}
> +	if (m->fault || m->removing) {
> +		err = -EIO;
> +		goto fail;
> +	}
> +	pci_intx(m->pci, 0);
> +	pci_clear_master(m->pci);
> +	err = maya_restore_after_sleep(m);
> +	if (err)
> +		goto fail;
> +	spin_lock_irqsave(&m->lock, flags);
> +	m->irq_window = jiffies;
> +	m->irq_window_handled = 0;
> +
> +	m->suspended = false;
> +	pci_set_master(m->pci);
> +	pci_intx(m->pci, 1);
> +	spin_unlock_irqrestore(&m->lock, flags);
> +	mutex_unlock(&m->codec_lock);
> +	snd_power_change_state(card, SNDRV_CTL_POWER_D0);
> +	return 0;
> +fail:
> +	spin_lock_irqsave(&m->lock, flags);
> +
> +	maya_fatal(m);
> +	spin_unlock_irqrestore(&m->lock, flags);
> +	mutex_unlock(&m->codec_lock);
> +	dev_err(dev, "resume failed: %d; DMA and interrupts disabled\n", err);
> +	return err;
> +}

[Severity: High]
Does the fail path in maya_resume() leave the card stuck in
SNDRV_CTL_POWER_D3hot with m->suspended still set?

If m->fault is set before suspend or maya_restore_after_sleep() fails,
maya_resume() returns without calling snd_power_change_state(card,
SNDRV_CTL_POWER_D0) or clearing m->suspended.  Any process that later
closes or accesses a PCM substream or mixer control sleeps forever in
TASK_UNINTERRUPTIBLE inside snd_power_ref_and_wait() / snd_power_wait().
Keeping m->suspended true also causes maya_fatal() -> maya_quiesce() and
maya_free() to skip MMIO register shutdown and fault_work scheduling even
though the PCI device is in D0, and a subsequent maya_suspend() will
overwrite m->suspend_rate with 0 if maya_restore_after_sleep() failed after
clearing m->rate.

[ ... ]

> +MODULE_AUTHOR("MAYA44e Linux reverse-engineering project");
> +MODULE_DESCRIPTION("ESI MAYA44e analog audio driver");
> +MODULE_LICENSE("GPL");

[Severity: High]
Under Documentation/process/submitting-patches.rst, Signed-off-by and
author attribution must use a known real full name rather than a single-word
handle ("Nordschamane <nordschamane1@proton.me>") or a generic project
string in MODULE_AUTHOR.  Also, the transient notes in the commit message
about the tree base and RFC feedback requests belong below the "---"
separator line so they are not recorded in the permanent git history.

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