ASoC: Add support for AN7581 PCM driver

Review

[Severity: Medium]
Will this work correctly on big-endian architectures? Since the descriptors are shared with the hardware DMA, using native u32 instead of explicit __le32 means the data will be written and read backwards if the kernel is compiled for a Big Endian target. Should we use __le32 and explicit cpu_to_le32() conversions?

Review

[Severity: Medium]
Is it correct to advertise SNDRV_PCM_INFO_INTERLEAVED here? Later in an7581_pcm_fe_hw_params(), the hardware is configured with a per-channel memory offset, indicating that the hardware expects a non-interleaved layout:

	regmap_update_bits(priv->map, AN7581_PCM_CHBFOSR,
			   AN7581_PCM_DESC_CHBF_OFFSET,
			   FIELD_PREP(AN7581_PCM_DESC_CHBF_OFFSET,
				      AN7581_PCM_FRAMES_PER_DESC * bit_width / 8));

If userspace writes interleaved samples as advertised, might the hardware misinterpret the audio data?

[ ... ]

Review

[Severity: High]
Does this code inadvertently continue execution and modify driver state when snd_pcm_hw_constraint_integer() fails? By missing a return statement, the function proceeds to cache the substream pointer and increment priv->users before returning the error code. Because the startup failed, ALSA will abort and never call shutdown, permanently leaking the users count and leaving a dangling substream pointer in priv->stream that could cause a Use-After-Free during the next interrupt.

[ ... ]

Review

[Severity: High]
Is it possible for a concurrent interrupt handler to execute here? By clearing the stream pointer without calling synchronize_irq(), an already executing handler on another CPU might have already passed the NULL check. Once this function returns, ALSA will free the substream runtime. If the delayed IRQ handler then resumes, could it dereference the freed runtime pointer?

[ ... ]

Review

[Severity: High]
Does this inverted logic unconditionally reject the first stream? Since an7581_pcm_fe_startup() increments priv->users, it will be exactly 1 for the first stream. The condition priv->users <= 1 evaluates to true, and since the uninitialized priv->current_rate is 0, it will mismatch the requested rate. This causes the driver to return -EBUSY immediately, preventing hardware setup. Should this check be for priv->users > 1 instead?

[ ... ]

Review

[Severity: High]
Could the continuously accumulating tx_pos and rx_pos cause a discontinuous jump when the 32-bit integers wrap? After accumulating 4GB (which wraps to 0), the value is almost certainly not an exact multiple of the ALSA buffer_size. Applying modulo here will cause the ALSA frame pointer to jump backwards unexpectedly, triggering an XRUN and halting the audio stream. Should the byte counters be bounded or wrapped explicitly at the buffer boundary instead?

[ ... ]

Review

[Severity: High]
Does this handler leave the interrupt asserted? After reading the AN7581_PCM_ISR register, there is no corresponding regmap_write() to clear the asserted status bits. Without a write-1-to-clear acknowledgment, the interrupt line may remain asserted, potentially leading to an unrecoverable interrupt storm.

Review

[Severity: High]
Could these early exits abandon completed descriptors and stall the stream? The loop iterates over descriptors that the hardware has already completed. If the driver returns early here (or jumps to handle_tx) due to an ALSA underrun, it breaks out of the loop and skips processing the rest of the completed descriptors. This leaves them permanently owned by the CPU, stalls the DMA, and misses the wakeup write to the polling register. Because ALSA core already handles overruns and underruns safely, shouldn't the driver always process all completed descriptors without aborting the loop?

[ ... ]

Review

[Severity: High]
Is it valid to pass IRQF_ONESHOT with a NULL thread_fn? The core IRQ subsystem rejects this combination because it doesn't make sense to keep the interrupt masked for a thread that doesn't exist. This will cause devm_request_threaded_irq() to unconditionally return -EINVAL, causing probe to fail every time.

Review

[Severity: High]
Could this allocation order lead to a Use-After-Free during driver unbind? Since devres frees resources in reverse order (LIFO), priv->tx_descs and priv->rx_descs will be freed before the IRQ is released. Because an7581_pcm_remove() doesn't call synchronize_irq(), an executing interrupt handler might access the DMA descriptors after devres has already freed them.

Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2