Re: [PATCH v7 2/2] mfd: loongson-se: Fix miscellaneous issues

[Huacai Chen]

On Mon, Sep 14, 2026 at 4:22 PM Qunqin Zhao <zhaoqunqin@xxxxxxxxxxx> wrote:
>
>
> 在 2026/9/14 15:36, Huacai Chen 写道:
> > Hi, Qunqin,
> >
> > On Fri, Sep 11, 2026 at 5:03 PM Qunqin Zhao <zhaoqunqin@xxxxxxx> wrote:
> >> From: Qunqin Zhao <zhaoqunqin@xxxxxxxxxxx>
> >>
> >> Address multiple historical driver issues discovered by the Sashiko
> >> Automation system within the loongson_se_probe() initialization flow
> >> and the driver's interrupt service routines [1].
> >>
> >> - Add an explicit bounds check in se_irq_handler() before accessing
> >>   the engines array to prevent an out-of-bounds memory access.
> >>
> >> - Validate the id argument in loongson_se_init_engine() and reject
> >>   engine 0, whose data buffer is reused for the command buffers of all
> >>   other engines, to prevent out-of-bounds accesses and overlapping DMA
> >>   buffers.
> >>
> >> - Switch from devm_kmalloc() to devm_kzalloc() and initialize all
> >>   engine completion structures in probe(), dropping the redundant
> >>   init_completion() in loongson_se_init_engine().
> >>
> >> - Introduce loongson_se_reinit_completion() to mask the target
> >>   interrupt, clear any pending status, and reset the completion under
> >>   dev_lock before a command is issued, then re-enable the interrupt
> >>   under the same lock after the command has been triggered.  This
> >>   closes the race where a stale interrupt from a previously
> >>   interrupted command could complete the current command's completion.
> >>   A new poll_lock serializes the trigger and poll busy-wait so
> >>   interrupts are not disabled for up to 10 ms.
> > The locking of this driver is more and more complicated, I doubt
> > whether AI's review is really correct here. For me, the existing
> > locking code is just fine and simple.
>
> I'm not sure  whether disabling interrupts for 10 ms will cause kernel damage.
>
> Or keep the current lock and fix it if we actually run into problems?
I think we can do it like this.

Huacai

>
> After all, this extreme case will not occur. In practical tests, polling
>
> averages only a few microseconds, with the worst case under 1 ms.
>
> Thanks.
>
> >
> > Huacai
> >
> >> - Wait for controller command completion with a bounded
> >>   non-interruptible wait so a pending signal cannot leave the
> >>   controller running while its DMA buffers are being torn down, and a
> >>   wedged controller cannot hang the kernel indefinitely.
> >>
> >> - Fix EPROBE_DEFER handling: propagate the error directly from
> >>   platform_irq_count() instead of overwriting it with ENODEV so that
> >>   probe deferral works when the interrupt provider is not yet ready.
> >>
> >> - Validate dmam_size from firmware against the minimum required size
> >>   to keep the command buffers of all engines within engine 0's data
> >>   region and prevent overlapping DMA buffers.
> >>
> >> - Return the error code from devm_request_irq() instead of silently
> >>   continuing to prevent an indefinite hang.
> >>
> >> - Add a loongson_se_stop() cleanup handler registered with
> >>   devm_add_action_or_reset() before loongson_se_init() so that a
> >>   failed init still stops the controller and masks all interrupts via
> >>   devres, preventing DMA access to freed memory.
> >>
> >> - Zero-initialize the local controller command structures in
> >>   loongson_se_init() and loongson_se_init_engine() to prevent
> >>   uninitialized stack data from being written to device registers.
> >>
> >> - Add the SE_CMD_STOP command definition.
> >>
> >> Link: https://lore.kernel.org/all/20260618095949.GB1672911@xxxxxxxxxx/ [1]
> >> Fixes: e551fa3159e3 ("mfd: Add support for Loongson Security Engine chip controller")
> >> Signed-off-by: Qunqin Zhao <zhaoqunqin@xxxxxxxxxxx>
> >> ---
> >>  drivers/mfd/loongson-se.c       | 112 ++++++++++++++++++++++++++++----
> >>  include/linux/mfd/loongson-se.h |   1 +
> >>  2 files changed, 102 insertions(+), 11 deletions(-)
> >>
> >> diff --git a/drivers/mfd/loongson-se.c b/drivers/mfd/loongson-se.c
> >> index 7f552a8ee..18075d4fa 100644
> >> --- a/drivers/mfd/loongson-se.c
> >> +++ b/drivers/mfd/loongson-se.c
> >> @@ -23,6 +23,8 @@
> >>  struct loongson_se {
> >>         void __iomem *base;
> >>         spinlock_t dev_lock;
> >> +       /* Synchronizes command submission between users of different engines */
> >> +       spinlock_t poll_lock;
> >>         struct completion cmd_completion;
> >>
> >>         void *dmam_base;
> >> @@ -42,7 +44,7 @@ static int loongson_se_poll(struct loongson_se *se, u32 int_bit)
> >>         u32 status;
> >>         int err;
> >>
> >> -       spin_lock_irq(&se->dev_lock);
> >> +       spin_lock(&se->poll_lock);
> >>
> >>         /* Notify the controller that the engine needs to be started */
> >>         writel(int_bit, se->base + SE_L2SINT_SET);
> >> @@ -52,17 +54,51 @@ static int loongson_se_poll(struct loongson_se *se, u32 int_bit)
> >>                                                 !(status & int_bit),
> >>                                                 1, LOONGSON_ENGINE_CMD_TIMEOUT_US);
> >>
> >> +       spin_unlock(&se->poll_lock);
> >> +
> >> +       spin_lock_irq(&se->dev_lock);
> >> +       /*
> >> +        * Re-enable the interrupt that loongson_se_reinit_completion() masked.
> >> +        * The hardware guarantees that once the interrupt is re-enabled, only
> >> +        * interrupts for the command just issued can arrive, so a stale
> >> +        * interrupt from a previously interrupted command can never complete
> >> +        * this command's completion.
> >> +        */
> >> +       writel(int_bit | readl(se->base + SE_S2LINT_EN), se->base + SE_S2LINT_EN);
> >> +
> >>         spin_unlock_irq(&se->dev_lock);
> >>
> >>         return err;
> >>  }
> >>
>
>