[PATCH v7 0/2] mfd: loongson-se: Add multi-node support and fix miscellaneous issues

[PATCH v7 0/2] mfd: loongson-se: Add multi-node support and fix miscellaneous issues

[Qunqin Zhao]

From: Qunqin Zhao <zhaoqunqin@loongson.cn>

- Patch 1 introduces the multi-node platform support.

  v6-v7: No changes
  v5: Clear pending interrupts with S2LINT_CL before requesting IRQs,
       and move S2LINT_EN to after all interrupt handlers are registered.
  v4: Safely handling shared interrupt handler returns.
  v3: Using shared interrupts (IRQF_SHARED) instead of manually
      iterating through all devices to check for interrupts.

- Patch 2 focuses on structural and error-path hardening for the
  pre-existing baseline code. This includes resolving critical issues
  discovered by the Sashiko Automation system such as: completion
  race conditions, missing engine completion initialization, spinlock
  held with interrupts disabled for up to 10 ms, EPROBE_DEFER
  handling, DMA buffer overflow validation, interrupt cleanup on
  probe failure, and device removal ordering.

  v7:
       - Re-enable the interrupt under dev_lock in loongson_se_poll()
         instead of poll_lock, so both the mask and re-enable of
         SE_S2LINT_EN are protected by the same lock and cannot race
       - Wait for controller command completion with a bounded
         non-interruptible wait so a pending signal cannot leave the
         controller running while DMA buffers are torn down
       - Validate the id argument in loongson_se_init_engine() to
         reject engine 0 and out-of-bounds indices
       - Drop the redundant init_completion() in
         loongson_se_init_engine() now that probe() initializes all
         engine completions
  v6:
       - Mask the target interrupt, clear pending status, and
         reinitialize the completion under dev_lock before issuing a
         command, re-enabling the interrupt after triggering
       - Keep the original engine_init_lock scope instead of the
         broader cmd_lock mutex
       - Reuse loongson_se_send_controller_cmd() in the STOP path and
         register the stop handler before loongson_se_init()
       - Correct the dmam_size validation formula
  v5: Fix all issues reported by Sashiko analysis:
       - Initialize engine completions in probe to avoid NULL deref
       - Use cmd_lock mutex for command serialization, drop spin_lock_irq
       - Add reinit_completion to controller and engine command paths
       - Fix EPROBE_DEFER handling in platform_irq_count()
       - Validate dmam_size against minimum required size
       - Disable IRQs in probe error path after init failure
       - Add loongson_se_stop() devres cleanup with proper ordering
       - Use non-interruptible wait for STOP command
  v4: new patch


Qunqin Zhao (2):
  mfd: loongson-se: Add multi-node support
  mfd: loongson-se: Fix miscellaneous issues

 drivers/mfd/loongson-se.c       | 127 ++++++++++++++++++++++++++++----
 include/linux/mfd/loongson-se.h |   1 +
 2 files changed, 113 insertions(+), 15 deletions(-)


base-commit: cee9395acd8043be0644b25c34bfa86623f2b935
-- 
2.47.2

[PATCH v7 1/2] mfd: loongson-se: Add multi-node support

[Qunqin Zhao]

From: Qunqin Zhao <zhaoqunqin@loongson.cn>

On the Loongson platform, each node is equipped with a security engine
device. However, due to a hardware flaw, only the device on node 0 can
trigger interrupts. Therefore, interrupts from other nodes are forwarded
by node 0. We need to check in the interrupt handler of node 0 whether
this interrupt is intended for other nodes, this can be accomplished via
shared interrupt handling.

Signed-off-by: Qunqin Zhao <zhaoqunqin@loongson.cn>
---
 drivers/mfd/loongson-se.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/drivers/mfd/loongson-se.c b/drivers/mfd/loongson-se.c
index 3902ba377..7f552a8ee 100644
--- a/drivers/mfd/loongson-se.c
+++ b/drivers/mfd/loongson-se.c
@@ -140,6 +140,11 @@ static irqreturn_t se_irq_handler(int irq, void *dev_id)
 
 	int_status = readl(se->base + SE_S2LINT_STAT);
 
+	if (int_status == 0) {
+		spin_unlock(&se->dev_lock);
+		return IRQ_NONE;
+	}
+
 	/* For controller */
 	if (int_status & SE_INT_CONTROLLER) {
 		complete(&se->cmd_completion);
@@ -211,24 +216,26 @@ static int loongson_se_probe(struct platform_device *pdev)
 	if (IS_ERR(se->base))
 		return PTR_ERR(se->base);
 
-	writel(SE_INT_ALL, se->base + SE_S2LINT_EN);
-
 	nr_irq = platform_irq_count(pdev);
 	if (nr_irq <= 0)
 		return -ENODEV;
 
+	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, 0, "loongson-se", se);
+		err = devm_request_irq(dev, irq, se_irq_handler, IRQF_SHARED, "loongson-se", se);
 		if (err)
 			dev_err(dev, "failed to request IRQ: %d\n", irq);
 	}
 
+	writel(SE_INT_ALL, se->base + SE_S2LINT_EN);
+
 	err = loongson_se_init(se, paddr, se->dmam_size);
 	if (err)
 		return err;
 
-	return devm_mfd_add_devices(dev, PLATFORM_DEVID_NONE, engines,
+	return devm_mfd_add_devices(dev, PLATFORM_DEVID_AUTO, engines,
 				    ARRAY_SIZE(engines), NULL, 0, NULL);
 }
 
-- 
2.47.2

Re: [PATCH v7 1/2] mfd: loongson-se: Add multi-node support

[Lee Jones]

On Fri, 11 Sep 2026, Qunqin Zhao wrote:

> From: Qunqin Zhao <zhaoqunqin@loongson.cn>
> 
> On the Loongson platform, each node is equipped with a security engine
> device. However, due to a hardware flaw, only the device on node 0 can
> trigger interrupts. Therefore, interrupts from other nodes are forwarded
> by node 0. We need to check in the interrupt handler of node 0 whether
> this interrupt is intended for other nodes, this can be accomplished via
> shared interrupt handling.
> 
> Signed-off-by: Qunqin Zhao <zhaoqunqin@loongson.cn>
> ---
>  drivers/mfd/loongson-se.c | 15 +++++++++++----
>  1 file changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/mfd/loongson-se.c b/drivers/mfd/loongson-se.c
> index 3902ba377..7f552a8ee 100644
> --- a/drivers/mfd/loongson-se.c
> +++ b/drivers/mfd/loongson-se.c
> @@ -140,6 +140,11 @@ static irqreturn_t se_irq_handler(int irq, void *dev_id)
>  
>  	int_status = readl(se->base + SE_S2LINT_STAT);
>  

Nit: Checks should follow immediately from the call that allocated them.

Thus, remove this empty line please.

> +	if (int_status == 0) {
> +		spin_unlock(&se->dev_lock);
> +		return IRQ_NONE;
> +	}
> +
>  	/* For controller */
>  	if (int_status & SE_INT_CONTROLLER) {
>  		complete(&se->cmd_completion);
> @@ -211,24 +216,26 @@ static int loongson_se_probe(struct platform_device *pdev)
>  	if (IS_ERR(se->base))
>  		return PTR_ERR(se->base);
>  
> -	writel(SE_INT_ALL, se->base + SE_S2LINT_EN);
> -
>  	nr_irq = platform_irq_count(pdev);
>  	if (nr_irq <= 0)
>  		return -ENODEV;
>  
> +	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, 0, "loongson-se", se);
> +		err = devm_request_irq(dev, irq, se_irq_handler, IRQF_SHARED, "loongson-se", se);
>  		if (err)
>  			dev_err(dev, "failed to request IRQ: %d\n", irq);
>  	}
>  
> +	writel(SE_INT_ALL, se->base + SE_S2LINT_EN);
> +
>  	err = loongson_se_init(se, paddr, se->dmam_size);
>  	if (err)
>  		return err;
>  
> -	return devm_mfd_add_devices(dev, PLATFORM_DEVID_NONE, engines,
> +	return devm_mfd_add_devices(dev, PLATFORM_DEVID_AUTO, engines,
>  				    ARRAY_SIZE(engines), NULL, 0, NULL);
>  }
>  
> -- 
> 2.47.2
> 

-- 
Lee Jones

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

[Qunqin Zhao]

From: Qunqin Zhao <zhaoqunqin@loongson.cn>

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.

- 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@google.com/ [1]
Fixes: e551fa3159e3 ("mfd: Add support for Loongson Security Engine chip controller")
Signed-off-by: Qunqin Zhao <zhaoqunqin@loongson.cn>
---
 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;
 }
 
+/*
+ * 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);
 
@@ -70,17 +106,31 @@ static int loongson_se_send_controller_cmd(struct loongson_se *se,
 	if (err)
 		return err;
 
-	return wait_for_completion_interruptible(&se->cmd_completion);
+	/*
+	 * Wait for the controller to complete the command.  Use a bounded
+	 * non-interruptible wait: a pending signal must not leave the
+	 * controller running while its DMA buffers are being torn down.
+	 * One second is far more than any controller command should take,
+	 * while still bounding the wait if the hardware wedges.
+	 */
+	if (!wait_for_completion_timeout(&se->cmd_completion, HZ))
+		return -ETIMEDOUT;
+
+	return 0;
 }
 
 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;
@@ -92,12 +142,17 @@ EXPORT_SYMBOL_GPL(loongson_se_send_engine_cmd);
 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_engine *engine;
+	struct loongson_se_controller_cmd cmd = {0};
+
+	/* Engine 0 does not exist and its data buffer is reused for commands */
+	if (id <= 0 || id >= SE_ENGINE_MAX)
+		return NULL;
+
+	engine = &se->engines[id];
 
 	engine->se = se;
 	engine->id = id;
-	init_completion(&engine->completion);
 
 	/* Divide DMA memory equally among all engines */
 	engine->buffer_size = se->dmam_size / SE_ENGINE_MAX;
@@ -155,7 +210,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 +223,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 +244,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 +262,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 +299,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

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

[Huacai Chen]

Hi, Qunqin,

On Fri, Sep 11, 2026 at 5:03 PM Qunqin Zhao <zhaoqunqin@163.com> wrote:
>
> From: Qunqin Zhao <zhaoqunqin@loongson.cn>
>
> 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.

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@google.com/ [1]
> Fixes: e551fa3159e3 ("mfd: Add support for Loongson Security Engine chip controller")
> Signed-off-by: Qunqin Zhao <zhaoqunqin@loongson.cn>
> ---
>  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;
>  }
>
> +/*
> + * 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);
>
> @@ -70,17 +106,31 @@ static int loongson_se_send_controller_cmd(struct loongson_se *se,
>         if (err)
>                 return err;
>
> -       return wait_for_completion_interruptible(&se->cmd_completion);
> +       /*
> +        * Wait for the controller to complete the command.  Use a bounded
> +        * non-interruptible wait: a pending signal must not leave the
> +        * controller running while its DMA buffers are being torn down.
> +        * One second is far more than any controller command should take,
> +        * while still bounding the wait if the hardware wedges.
> +        */
> +       if (!wait_for_completion_timeout(&se->cmd_completion, HZ))
> +               return -ETIMEDOUT;
> +
> +       return 0;
>  }
>
>  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;
> @@ -92,12 +142,17 @@ EXPORT_SYMBOL_GPL(loongson_se_send_engine_cmd);
>  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_engine *engine;
> +       struct loongson_se_controller_cmd cmd = {0};
> +
> +       /* Engine 0 does not exist and its data buffer is reused for commands */
> +       if (id <= 0 || id >= SE_ENGINE_MAX)
> +               return NULL;
> +
> +       engine = &se->engines[id];
>
>         engine->se = se;
>         engine->id = id;
> -       init_completion(&engine->completion);
>
>         /* Divide DMA memory equally among all engines */
>         engine->buffer_size = se->dmam_size / SE_ENGINE_MAX;
> @@ -155,7 +210,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 +223,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 +244,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 +262,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 +299,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
>

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@163.com> wrote:
>> From: Qunqin Zhao <zhaoqunqin@loongson.cn>
>>
>> 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@google.com/ [1]
>> Fixes: e551fa3159e3 ("mfd: Add support for Loongson Security Engine chip controller")
>> Signed-off-by: Qunqin Zhao <zhaoqunqin@loongson.cn>
>> ---
>>  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;
>>  }
>>

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@loongson.cn> wrote:
>
>
> 在 2026/9/14 15:36, Huacai Chen 写道:
> > Hi, Qunqin,
> >
> > On Fri, Sep 11, 2026 at 5:03 PM Qunqin Zhao <zhaoqunqin@163.com> wrote:
> >> From: Qunqin Zhao <zhaoqunqin@loongson.cn>
> >>
> >> 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@google.com/ [1]
> >> Fixes: e551fa3159e3 ("mfd: Add support for Loongson Security Engine chip controller")
> >> Signed-off-by: Qunqin Zhao <zhaoqunqin@loongson.cn>
> >> ---
> >>  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;
> >>  }
> >>
>
>