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

[Qunqin Zhao]

在 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? 

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;
>>  }
>>