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

[Qunqin Zhao]

在 2026/9/7 22:12, Huacai Chen 写道:
> Hi, Qunqin,
>
> On Mon, Sep 7, 2026 at 10:38 AM 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.
>>
>> - Switch from devm_kmalloc() to devm_kzalloc() and explicitly
>>   initialize all engine completion structures in probe() to avoid a
>>   kernel panic from complete() dereferencing a NULL wait head when a
>>   spurious interrupt fires before child drivers call
>>   loongson_se_init_engine().
>>
>> - Serialize command submission with a new poll_lock spinlock and
>>   protect the completion reset with the interrupt lock.  Before
>>   issuing a command, mask the target interrupt, clear any pending
>>   status, and reinitialize the completion under dev_lock, then
>>   re-enable the interrupt 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.  The spin_lock_irq() around the 10 ms poll is dropped so
>>   interrupts are not disabled for the full busy-wait.
>>
>> - 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       | 90 +++++++++++++++++++++++++++++----
>>  include/linux/mfd/loongson-se.h |  1 +
>>  2 files changed, 82 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/mfd/loongson-se.c b/drivers/mfd/loongson-se.c
>> index 7f552a8ee..1d332c418 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;
> Can't we reuse dev_lock?
>
> Huacai
>
>>         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);

dev_lock may be taken in IRQ handler. Without interrupt masking, 
deadlock could occur.

Thanks,

Qunqin

>>
>>         /* Notify the controller that the engine needs to be started */
>>         writel(int_bit, se->base + SE_L2SINT_SET);
>> @@ -52,17 +54,48 @@ static int loongson_se_poll(struct loongson_se *se, u32 int_bit)
>>                                                 !(status & int_bit),
>>                                                 1, LOONGSON_ENGINE_CMD_TIMEOUT_US);
>>
>> -       spin_unlock_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(&se->poll_lock);
>>
>>         return err;
>>  }
>>
>> +/*
>> + * Prepare a completion for a new command: mask the corresponding interrupt,
>> + * clear any pending interrupt status, and reset the completion.  This runs
>> + * under dev_lock so that the IRQ handler cannot race with it.  The interrupt
>> + * is re-enabled in loongson_se_poll() after the command has been issued.
>> + */
>> +static void loongson_se_reinit_completion(struct loongson_se *se,
>> +                                         struct completion *completion, u32 int_bit)
>> +{
>> +       spin_lock_irq(&se->dev_lock);
>> +
>> +       writel(readl(se->base + SE_S2LINT_EN) & ~int_bit, se->base + SE_S2LINT_EN);
>> +
>> +       writel(int_bit, se->base + SE_S2LINT_CL);
>> +
>> +       reinit_completion(completion);
>> +
>> +       spin_unlock_irq(&se->dev_lock);
>> +}
>> +
>>  static int loongson_se_send_controller_cmd(struct loongson_se *se,
>>                                            struct loongson_se_controller_cmd *cmd)
>>  {
>>         u32 *send_cmd = (u32 *)cmd;
>>         int err, i;
>>
>> +       loongson_se_reinit_completion(se, &se->cmd_completion, SE_INT_CONTROLLER);
>> +
>>         for (i = 0; i < SE_SEND_CMD_REG_LEN; i++)
>>                 writel(send_cmd[i], se->base + SE_SEND_CMD_REG + i * 4);
>>
>> @@ -75,12 +108,16 @@ static int loongson_se_send_controller_cmd(struct loongson_se *se,
>>
>>  int loongson_se_send_engine_cmd(struct loongson_se_engine *engine)
>>  {
>> +       int err;
>> +
>> +       loongson_se_reinit_completion(engine->se, &engine->completion, BIT(engine->id));
>> +
>>         /*
>>          * After engine initialization, the controller already knows
>>          * where to obtain engine commands from. Now all we need to
>>          * do is notify the controller that the engine needs to be started.
>>          */
>> -       int err = loongson_se_poll(engine->se, BIT(engine->id));
>> +       err = loongson_se_poll(engine->se, BIT(engine->id));
>>
>>         if (err)
>>                 return err;
>> @@ -93,7 +130,7 @@ struct loongson_se_engine *loongson_se_init_engine(struct device *dev, int id)
>>  {
>>         struct loongson_se *se = dev_get_drvdata(dev);
>>         struct loongson_se_engine *engine = &se->engines[id];
>> -       struct loongson_se_controller_cmd cmd;
>> +       struct loongson_se_controller_cmd cmd = {0};
>>
>>         engine->se = se;
>>         engine->id = id;
>> @@ -155,7 +192,8 @@ static irqreturn_t se_irq_handler(int irq, void *dev_id)
>>         /* For engines */
>>         while (int_status) {
>>                 id = __ffs(int_status);
>> -               complete(&se->engines[id].completion);
>> +               if (id < SE_ENGINE_MAX)
>> +                       complete(&se->engines[id].completion);
>>                 int_status &= ~BIT(id);
>>                 writel(BIT(id), se->base + SE_S2LINT_CL);
>>         }
>> @@ -167,7 +205,7 @@ static irqreturn_t se_irq_handler(int irq, void *dev_id)
>>
>>  static int loongson_se_init(struct loongson_se *se, dma_addr_t addr, int size)
>>  {
>> -       struct loongson_se_controller_cmd cmd;
>> +       struct loongson_se_controller_cmd cmd = {0};
>>         int err;
>>
>>         cmd.command_id = SE_CMD_START;
>> @@ -188,6 +226,17 @@ static const struct mfd_cell engines[] = {
>>         { .name = "tpm_loongson" },
>>  };
>>
>> +static void loongson_se_stop(void *data)
>> +{
>> +       struct loongson_se *se = data;
>> +       struct loongson_se_controller_cmd cmd = {0};
>> +
>> +       cmd.command_id = SE_CMD_STOP;
>> +       loongson_se_send_controller_cmd(se, &cmd);
>> +
>> +       writel(0, se->base + SE_S2LINT_EN);
>> +}
>> +
>>  static int loongson_se_probe(struct platform_device *pdev)
>>  {
>>         struct device *dev = &pdev->dev;
>> @@ -195,19 +244,34 @@ static int loongson_se_probe(struct platform_device *pdev)
>>         int nr_irq, irq, err, i;
>>         dma_addr_t paddr;
>>
>> -       se = devm_kmalloc(dev, sizeof(*se), GFP_KERNEL);
>> +       se = devm_kzalloc(dev, sizeof(*se), GFP_KERNEL);
>>         if (!se)
>>                 return -ENOMEM;
>>
>>         dev_set_drvdata(dev, se);
>>         init_completion(&se->cmd_completion);
>>         spin_lock_init(&se->dev_lock);
>> +       spin_lock_init(&se->poll_lock);
>>         mutex_init(&se->engine_init_lock);
>>
>> +       for (i = 0; i < SE_ENGINE_MAX; i++)
>> +               init_completion(&se->engines[i].completion);
>> +
>>         dma_set_mask_and_coherent(dev, DMA_BIT_MASK(64));
>>         if (device_property_read_u32(dev, "dmam_size", &se->dmam_size))
>>                 return -ENODEV;
>>
>> +       /*
>> +        * Engine 0 does not exist and its data buffer is reused as the command
>> +        * buffer for the other engines.  The command buffers of all engines
>> +        * therefore must fit within engine 0's data region, i.e.
>> +        * dmam_size / SE_ENGINE_MAX >= SE_ENGINE_MAX * 2 * SE_ENGINE_CMD_SIZE.
>> +        * Enforce this to keep each engine's data and command buffers from
>> +        * overlapping.
>> +        */
>> +       if ((se->dmam_size / SE_ENGINE_MAX) < (SE_ENGINE_MAX * 2 * SE_ENGINE_CMD_SIZE))
>> +               return -EINVAL;
>> +
>>         se->dmam_base = dmam_alloc_coherent(dev, se->dmam_size, &paddr, GFP_KERNEL);
>>         if (!se->dmam_base)
>>                 return -ENOMEM;
>> @@ -217,20 +281,28 @@ static int loongson_se_probe(struct platform_device *pdev)
>>                 return PTR_ERR(se->base);
>>
>>         nr_irq = platform_irq_count(pdev);
>> -       if (nr_irq <= 0)
>> +       if (nr_irq == 0)
>>                 return -ENODEV;
>> +       if (nr_irq < 0)
>> +               return nr_irq;
>>
>>         writel(SE_INT_ALL, se->base + SE_S2LINT_CL);
>>
>>         for (i = 0; i < nr_irq; i++) {
>>                 irq = platform_get_irq(pdev, i);
>>                 err = devm_request_irq(dev, irq, se_irq_handler, IRQF_SHARED, "loongson-se", se);
>> -               if (err)
>> +               if (err) {
>>                         dev_err(dev, "failed to request IRQ: %d\n", irq);
>> +                       return err;
>> +               }
>>         }
>>
>>         writel(SE_INT_ALL, se->base + SE_S2LINT_EN);
>>
>> +       err = devm_add_action_or_reset(dev, loongson_se_stop, se);
>> +       if (err)
>> +               return err;
>> +
>>         err = loongson_se_init(se, paddr, se->dmam_size);
>>         if (err)
>>                 return err;
>> diff --git a/include/linux/mfd/loongson-se.h b/include/linux/mfd/loongson-se.h
>> index 07afa0c25..8237ccab7 100644
>> --- a/include/linux/mfd/loongson-se.h
>> +++ b/include/linux/mfd/loongson-se.h
>> @@ -9,6 +9,7 @@
>>  #define SE_SEND_CMD_REG_LEN            0x8
>>  /* Controller command ID */
>>  #define SE_CMD_START                   0x0
>> +#define SE_CMD_STOP                    0x1
>>  #define SE_CMD_SET_DMA                 0x3
>>  #define SE_CMD_SET_ENGINE_CMDBUF       0x4
>>
>> --
>> 2.47.2
>>