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

On Tue, Aug 4, 2026 at 10:29 PM Huacai Chen <[email protected]> wrote:
>
> On Tue, Aug 4, 2026 at 10:09 AM Qunqin Zhao <[email protected]> wrote:
> >
> >
> > 在 2026/8/3 16:01, Huacai Chen 写道:
> > > Hi, Qunqin,
> > >
> > > On Thu, Jul 30, 2026 at 4:41 PM Qunqin Zhao <[email protected]> wrote:
> > >> 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 out-of-bounds memory writes.
> > >>
> > >> - 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().
> > >>
> > >> - Replace engine_init_lock with a broader cmd_lock mutex that
> > >>    serializes all command submissions, and move the lock into
> > >>    loongson_se_send_controller_cmd() and
> > >>    loongson_se_send_engine_cmd() to cover the full register write +
> > >>    poll + wait sequence.
> > >>
> > >> - Drop the spin_lock_irq from loongson_se_poll() so that interrupts
> > >>    are not disabled for up to 10 ms during the poll.  Keep the
> > >>    readl_relaxed_poll_timeout_atomic() busy-wait to avoid scheduling
> > >>    latency for fast hardware completions.
> > >>
> > >> - Add reinit_completion() to loongson_se_send_controller_cmd() and
> > >>    loongson_se_send_engine_cmd() before waiting to prevent stale
> > >>    completions from falsely returning success after a signal
> > >>    interruption.
> > >>
> > >> - 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 prevent command buffers from pointing outside the allocated
> > >>    DMA region.
> > >>
> > >> - Return the error code from devm_request_irq() instead of silently
> > >>    continuing to prevent an indefinite hang.
> > >>
> > >> - Disable hardware interrupts in the probe error path when
> > >>    loongson_se_init() fails to prevent an unhandled interrupt storm.
> > >>
> > >> - Add a loongson_se_stop() cleanup handler registered with
> > >>    devm_add_action_or_reset() to send SE_CMD_STOP to the controller
> > >>    and mask all interrupts during device removal.  Using devres
> > >>    ensures that child MFD devices are unbound before the controller
> > >>    is stopped.  The STOP command uses a non-interruptible wait to
> > >>    avoid leaving hardware running while DMA buffers are freed.
> > >>
> > >> - Zero-initialize the local controller command structure in
> > >>    loongson_se_init() to prevent uninitialized stack memory from
> > >>    being written to device registers.
> > >>
> > >> - Add the SE_CMD_STOP command definition.
> > >>
> > >> Link: https://lore.kernel.org/all/[email protected]/ [1]
> > >> Fixes: e551fa3159e3 ("mfd: Add support for Loongson Security Engine chip controller")
> > >> Signed-off-by: Qunqin Zhao <[email protected]>
> > >> ---
> > >>   drivers/mfd/loongson-se.c       | 87 ++++++++++++++++++++++++++-------
> > >>   include/linux/mfd/loongson-se.h |  1 +
> > >>   2 files changed, 69 insertions(+), 19 deletions(-)
> > >>
> > >> diff --git a/drivers/mfd/loongson-se.c b/drivers/mfd/loongson-se.c
> > >> index 7f552a8ee6..2c8afad1b0 100644
> > >> --- a/drivers/mfd/loongson-se.c
> > >> +++ b/drivers/mfd/loongson-se.c
> > >> @@ -28,7 +28,7 @@ struct loongson_se {
> > >>          void *dmam_base;
> > >>          int dmam_size;
> > >>
> > >> -       struct mutex engine_init_lock;
> > >> +       struct mutex cmd_lock;
> > >>          struct loongson_se_engine engines[SE_ENGINE_MAX];
> > >>   };
> > >>
> > >> @@ -42,8 +42,6 @@ static int loongson_se_poll(struct loongson_se *se, u32 int_bit)
> > >>          u32 status;
> > >>          int err;
> > >>
> > >> -       spin_lock_irq(&se->dev_lock);
> > > You said you don't want to disable irq here, but I think the spinlock
> > > is still needed. That means you should use spin_lock/spin_unlock to
> > > replace spin_lock_irq/spin_unlock_irq.
> > Since loongson_se_poll always executes in the thread context, is it
> > better to use mutex_lock(
> > mutex_lock(&se->cmd_lock)  outside of loongson_se_poll) for
> > synchronization?
> If all callers have used mutex_lock(&se->cmd_lock), then spinlock is
> unnecessary.
After some thinking, maybe the AI suggestion is wrong, we shouldn't
use mutex to replace spinlock here.

Yes, loongson_se_poll() is executed in process context, but there
isn't a rule "don't use spinlock in process context".

The real rules are:
1. Keep critical sections as short as possible.
2. Don't use sleepable functions in spinlock critical sections.

writel() and readl_relaxed_poll_timeout_atomic() are not sleepable
functions, so spinlock is just fine.


Huacai

>
> > >
> > >> -
> > >>          /* Notify the controller that the engine needs to be started */
> > >>          writel(int_bit, se->base + SE_L2SINT_SET);
> > >>
> > >> @@ -52,8 +50,6 @@ 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);
> > >> -
> > >>          return err;
> > >>   }
> > >>
> > >> @@ -63,24 +59,40 @@ static int loongson_se_send_controller_cmd(struct loongson_se *se,
> > >>          u32 *send_cmd = (u32 *)cmd;
> > >>          int err, i;
> > >>
> > >> +       mutex_lock(&se->cmd_lock);
> > >> +
> > >> +       reinit_completion(&se->cmd_completion);
> > >> +
> > >>          for (i = 0; i < SE_SEND_CMD_REG_LEN; i++)
> > >>                  writel(send_cmd[i], se->base + SE_SEND_CMD_REG + i * 4);
> > >>
> > >>          err = loongson_se_poll(se, SE_INT_CONTROLLER);
> > >>          if (err)
> > >> -               return err;
> > >> +               goto out;
> > >> +
> > >> +       err = wait_for_completion_interruptible(&se->cmd_completion);
> > >>
> > >> -       return wait_for_completion_interruptible(&se->cmd_completion);
> > >> +out:
> > >> +       mutex_unlock(&se->cmd_lock);
> > >> +       return err;
> > >>   }
> > >>
> > >>   int loongson_se_send_engine_cmd(struct loongson_se_engine *engine)
> > >>   {
> > >> +       int err;
> > >> +
> > >> +       mutex_lock(&engine->se->cmd_lock);
> > >> +
> > >> +       reinit_completion(&engine->completion);
> > >> +
> > >>          /*
> > >>           * 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));
> > >> +
> > >> +       mutex_unlock(&engine->se->cmd_lock);
> > >>
> > >>          if (err)
> > >>                  return err;
> > >> @@ -97,7 +109,7 @@ struct loongson_se_engine *loongson_se_init_engine(struct device *dev, int id)
> > >>
> > >>          engine->se = se;
> > >>          engine->id = id;
> > >> -       init_completion(&engine->completion);
> > >> +       reinit_completion(&engine->completion);
> > > I'm not sure, but I think loongson_se_init_engine() is only called at
> > > init, so we need init_completion here.
> >
> > To prevent an spurious interrupt from completing an uninitialized object,
> > all objects have already been fully initialized with init_completion during the probe stage.
> In my opinion, if a function can be called multiple times, we need
> reinit_completion(), if it is only called for probe, we need
> init_completion(), and loongson_se_init_engine() looks like the later
> case.
>
> Huacai
>
> >
> > >
> > > Huacai
> > >
> > >>          /* Divide DMA memory equally among all engines */
> > >>          engine->buffer_size = se->dmam_size / SE_ENGINE_MAX;
> > >> @@ -113,8 +125,6 @@ struct loongson_se_engine *loongson_se_init_engine(struct device *dev, int id)
> > >>          engine->command = se->dmam_base + id * (2 * SE_ENGINE_CMD_SIZE);
> > >>          engine->command_ret = engine->command + SE_ENGINE_CMD_SIZE;
> > >>
> > >> -       mutex_lock(&se->engine_init_lock);
> > >> -
> > >>          /* Tell the controller where to find engine command */
> > >>          cmd.command_id = SE_CMD_SET_ENGINE_CMDBUF;
> > >>          cmd.info[0] = id;
> > >> @@ -124,8 +134,6 @@ struct loongson_se_engine *loongson_se_init_engine(struct device *dev, int id)
> > >>          if (loongson_se_send_controller_cmd(se, &cmd))
> > >>                  engine = NULL;
> > >>
> > >> -       mutex_unlock(&se->engine_init_lock);
> > >> -
> > >>          return engine;
> > >>   }
> > >>   EXPORT_SYMBOL_GPL(loongson_se_init_engine);
> > >> @@ -155,7 +163,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 +176,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 +197,30 @@ 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};
> > >> +       u32 *send_cmd = (u32 *)&cmd;
> > >> +       int i;
> > >> +
> > >> +       mutex_lock(&se->cmd_lock);
> > >> +
> > >> +       cmd.command_id = SE_CMD_STOP;
> > >> +
> > >> +       reinit_completion(&se->cmd_completion);
> > >> +
> > >> +       for (i = 0; i < SE_SEND_CMD_REG_LEN; i++)
> > >> +               writel(send_cmd[i], se->base + SE_SEND_CMD_REG + i * 4);
> > >> +
> > >> +       if (!loongson_se_poll(se, SE_INT_CONTROLLER))
> > >> +               wait_for_completion(&se->cmd_completion);
> > >> +
> > >> +       writel(0, se->base + SE_S2LINT_EN);
> > >> +
> > >> +       mutex_unlock(&se->cmd_lock);
> > >> +}
> > >> +
> > >>   static int loongson_se_probe(struct platform_device *pdev)
> > >>   {
> > >>          struct device *dev = &pdev->dev;
> > >> @@ -195,19 +228,25 @@ 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);
> > >> -       mutex_init(&se->engine_init_lock);
> > >> +       mutex_init(&se->cmd_lock);
> > >> +
> > >> +       for (i = 0; i < SE_ENGINE_MAX; i++)
> > >> +               init_completion(&se->engines[i].completion);
> > Thanks
> >