Re: [PATCH v6 06/11] ASoC: renesas: fsi: Move fsi_clk_init()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 8 potential issue(s) to consider:

Pre-existing issues:
- [High] The driver releases DMA channels in `fsi_remove` before the devm-managed ALSA component is unregistered, leading to a NULL pointer dereference.
- [High] TOCTOU race condition in `fsi_pio_transfer` leads to a NULL pointer dereference on `io->substream->runtime`.
- [High] Concurrency bug and logic error in `fsi_clk_enable` breaks full-duplex audio and corrupts the shared clock refcount.
- [High] Unintended clearing of shared interrupt status causes dropped interrupts and audio dropouts for concurrent streams.
- [High] Missing cleanup on hardware startup failure leaves a dangling substream pointer, leading to a Use-After-Free.
- [High] Recursive fallback to PIO mode in `fsi_dma_probe` causes a permanent DMA channel leak.
- [Medium] Data race on hardware error registers and error counters causes dropped interrupt events.
- [Medium] PIO transfer functions perform raw 16-bit memory accesses without endianness conversion, corrupting audio on Big-Endian systems.
--

commit 143abdd60b62ddb2bbcd91ac88b1dcc0e574a4ad
Author: bui duc phuc <phucduc.bui@gmail.com>

ASoC: renesas: fsi: Move fsi_clk_init()

Move fsi_clk_init() after set_rate() functions to prepare for subsequent
refactoring.

> diff --git a/sound/soc/renesas/fsi.c b/sound/soc/renesas/fsi.c
> index 3e3c6fd7c56b8..1df7dc0143630 100644
> --- a/sound/soc/renesas/fsi.c
> +++ b/sound/soc/renesas/fsi.c

[ ... ]

[Severity: High]
This is a pre-existing issue, but I wanted to ask about the cleanup
sequence in fsi_remove().

It appears fsi_remove() calls fsi_stream_remove(), which eventually
calls fsi_dma_remove() and sets io->chan to NULL:

fsi_remove()
   fsi_stream_remove()
      fsi_dma_remove()
         io->chan = NULL;

Since the ALSA component is registered with devres, it is unregistered
after fsi_remove() completes. During this devres cleanup, ALSA stops active
streams by invoking fsi_dai_trigger() with SNDRV_PCM_TRIGGER_STOP, which
calls into fsi_dma_push_start_stop():

fsi_dma_push_start_stop() {
        ...
        dmaengine_terminate_all(io->chan);
        ...
}

Does this cause a NULL pointer dereference on unbind if a stream is running?

[Severity: High]
This is a pre-existing issue, but does fsi_pio_transfer() have a race
condition during stream stop?

In fsi_pio_transfer(), the code briefly takes a lock inside
fsi_stream_is_working() to ensure io->substream is valid, but then
locklessly calls fsi_pio_get_area():

fsi_pio_transfer() {
        if (!fsi_stream_is_working(fsi, io))
                return -EINVAL;

        buf = fsi_pio_get_area(fsi, io);
}

fsi_pio_get_area() dereferences the substream:

fsi_pio_get_area() {
        struct snd_pcm_runtime *runtime = io->substream->runtime;
        ...
}

If the ALSA core concurrently triggers SNDRV_PCM_TRIGGER_STOP on another CPU,
it calls fsi_stream_quit() which sets io->substream = NULL. Could this
cause a NULL pointer dereference in fsi_pio_get_area()?

[Severity: High]
This is a pre-existing issue, but I have a question about the logic
in fsi_clk_enable().

fsi_clk_enable() {
        int ret = -EINVAL;
        ...
        if (0 == clock->count) {
                ...
                clock->count++;
        }

        return ret;
}

If one stream is already running (so clock->count is 1) and the opposite
stream starts, this skips the initialization block and returns the
uninitialized -EINVAL. Does this break full-duplex audio by failing the
startup of the second stream?

Additionally, fsi_clk_enable() and fsi_clk_disable() increment and
decrement clock->count locklessly. Since playback and capture can be
triggered concurrently, could this corrupt the shared clock refcount?

[Severity: Medium]
This is a pre-existing issue, but is it safe to read and modify these
hardware registers locklessly?

In fsi_count_fifo_err():

fsi_count_fifo_err() {
        u32 ostatus = fsi_reg_read(fsi, DOFF_ST);
        ...
        fsi->playback.oerr_num++;
        ...
        fsi_reg_write(fsi, DOFF_ST, 0);
}

Since this function can be invoked locklessly from both fsi_interrupt()
and fsi_dma_complete(), could concurrent execution cause lost hardware error
bits or corrupt the error counters?

[Severity: High]
This is a pre-existing issue, but I wanted to ask about the interrupt
clearing logic during stream startup.

During stream initialization in fsi_hw_startup(), fsi_irq_clear_status()
is unconditionally called. It constructs a mask that includes both playback
and capture ports:

fsi_irq_clear_status() {
        data |= AB_IO(1, fsi_get_port_shift(fsi, &fsi->playback));
        data |= AB_IO(1, fsi_get_port_shift(fsi, &fsi->capture));

        /* clear interrupt factor */
        fsi_core_mask_set(master, int_st, data, 0);
}

If one stream is already actively running and the second stream is started,
will this unconditionally clear any pending interrupts for the active
stream, potentially causing dropped frames?

[Severity: High]
This is a pre-existing issue, but does fsi_dai_trigger() leave a dangling
pointer if hardware startup fails?

In fsi_dai_trigger():

fsi_dai_trigger() {
        ...
        fsi_stream_init(fsi, io, substream);
        if (!ret)
                ret = fsi_hw_startup(fsi, io, dai->dev);
        if (!ret)
                ret = fsi_stream_start(fsi, io);
        ...
}

fsi_stream_init() unconditionally assigns io->substream = substream.
If fsi_hw_startup() fails, it returns an error but does not call
fsi_stream_quit() to clear io->substream.

Since the ALSA core will destroy the failed substream runtime, could a
subsequent interrupt trigger fsi_count_fifo_err(), which invokes
fsi_stream_is_working() and evaluates the dangling io->substream->runtime?

[Severity: High]
This is a pre-existing issue, but I wanted to ask about the fallback
logic in fsi_dma_probe().

If the DMA probe fails for capture, it switches the handler to PIO and
calls fsi_stream_probe() recursively:

fsi_dma_probe() {
        io->chan = dma_request_chan(dev, is_play ? "tx" : "rx");
        ...
        /* fallback to PIO */
        ...
        return fsi_stream_probe(fsi, dev);
}

The recursive call to fsi_stream_probe() will loop over both streams and
probe playback again. Since playback's handler is still DMA, it executes
fsi_dma_probe() a second time.

Will this unconditionally call dma_request_chan() for playback and
overwrite the existing io->chan without freeing it, causing a DMA
channel leak?

[Severity: Medium]
This is a pre-existing issue, but does fsi_pio_push16() handle endianness
correctly on big-endian architectures?

ALSA formats S16_LE and S24_LE define audio data as little-endian.
However, the function casts the buffer directly to a native u16 pointer
and reads it:

fsi_pio_push16() {
        u16 *buf = (u16 *)_buf;

        for (i = 0; i < samples; i++)
                fsi_reg_write(fsi, DODT, ((u32)*(buf + i) << 8));
}

Similarly, fsi_pio_pop16() directly casts and writes into the buffer:

fsi_pio_pop16() {
        u16 *buf = (u16 *)_buf;
        ...
        *(buf + i) = (u16)(fsi_reg_read(fsi, DIDT) >> 8);
}

On big-endian systems, wouldn't this yield byte-swapped data before
writing to the hardware registers or into the ALSA buffer? Should it
use little-endian conversion helpers?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260609113836.45079-1-phucduc.bui@gmail.com?part=6