Re: [PATCH v3 03/10] ASoC: renesas: fsi: Fix trigger stop ordering

The points raised may be valid.
However, this patch series is currently focused on fixing the system hang i=
ssue.
The other concerns will be addressed in a separate follow-up series.

On Tue, May 12, 2026 at 5:44=E2=80=AFAM <[email protected]> wrote:
>
> Thank you for your contribution! Sashiko AI review found 4 potential issu=
e(s) to consider:
> - [High] TOCTOU race and lack of synchronization in stream stopping leads=
 to NULL pointer dereference.
> - [High] Teardown asymmetry in `fsi_remove` leads to use-after-free and N=
ULL pointer dereference.
> - [High] Lockless read-modify-write on shared FSI port registers causes h=
ardware corruption.
> - [High] Lockless modifications to shared `clock->count` cause reference =
count corruption.
> --
>
> commit 7ce4e3deeaefe8e811044e7b0472a2e9a2ae1d64
> Author: bui duc phuc <[email protected]>
>
> ASoC: renesas: fsi: Fix trigger stop ordering
>
> This commit reorders calls in the stop trigger to execute fsi_stream_stop=
()
> before fsi_hw_shutdown(). It ensures register accesses complete before th=
e
> clock is disabled to prevent a system hang.
>
> > diff --git a/sound/soc/renesas/fsi.c b/sound/soc/renesas/fsi.c
> > index 8cbd7acc26f49..94ab2e4908108 100644
> > --- a/sound/soc/renesas/fsi.c
> > +++ b/sound/soc/renesas/fsi.c
> > @@ -1586,9 +1586,9 @@ static int fsi_dai_trigger(struct snd_pcm_substre=
am *substream, int cmd,
> >                       ret =3D fsi_stream_transfer(io);
> >               break;
> >       case SNDRV_PCM_TRIGGER_STOP:
>
> Playback and capture substreams on the same FSI port share port-level
> configuration registers like CKG1, CKG2, and OUT_SEL.
>
> Since the ALSA core serializes triggers at the substream level using
> the self_group.lock, playback and capture triggers for the same DAI
> can execute concurrently on different CPUs.
>
> Does fsi_hw_startup() perform lockless read-modify-write operations on th=
ese
> shared registers, and can this lack of exclusion lead to lost writes and
> corrupted hardware state?
>
> > +             fsi_stream_stop(fsi, io);
>
> During a stream stop, fsi_stream_stop() disables DMA via
> dmaengine_terminate_all() and masks device interrupts, but it does not ap=
pear
> to wait for in-flight DMA callbacks or IRQ handlers to finish.
>
> Concurrently, an executing IRQ handler calling fsi_pio_transfer() will
> temporarily take master->lock in fsi_stream_is_working(), drop it, and th=
en
> locklessly access io->substream->runtime inside fsi_pio_get_area().
>
> Similarly, an asynchronous fsi_dma_complete() callback unconditionally
> dereferences io->substream in fsi_pointer_update().
>
> Since there is no dmaengine_terminate_sync() or synchronize_irq(), could
> fsi_stream_quit() race with these callbacks and clear io->substream,
> resulting in a NULL pointer dereference?
>
> The driver uses managed resources during fsi_probe(), meaning the IRQ han=
dler
> and ALSA component are unregistered after fsi_remove() returns. However,
> fsi_remove() explicitly tears down the DMA streams via fsi_stream_remove(=
),
> which calls dma_release_channel(io->chan) and sets io->chan to NULL.
>
> Because the ALSA component is still fully registered and active while
> fsi_remove() completes, userspace can still trigger ALSA operations.
>
> If a STOP trigger occurs during this window, would calling
> dmaengine_terminate_all(io->chan) here cause a NULL pointer dereference?
>
> >               if (!ret)
> >                       ret =3D fsi_hw_shutdown(fsi, dai->dev);
>
> The shared clock state for an FSI port is tracked using clock->count insi=
de
> fsi_priv.
>
> Since playback and capture triggers for the same port can execute
> concurrently on different CPUs, do fsi_clk_enable() and fsi_clk_disable()
> manually modify this counter without holding any lock?
>
> Could concurrent updates to clock->count corrupt the counter, potentially
> leaving the peripheral clocks enabled permanently or disabling them while=
 a
> stream is still active?
>
> > -             fsi_stream_stop(fsi, io);
> >               fsi_stream_quit(fsi, io);
> >               break;
> >       }
>
> --
> Sashiko AI review =C2=B7 https://sashiko.dev/#/patchset/20260510084303.12=
[email protected]?part=3D3