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

[PATCH v6 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: 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.

  v6:
       - Still use init_completion() in loongson_se_init_engine.
       - Serialize command submission with a new poll_lock spinlock
       - Mask the target interrupt, clear pending status, and
         reinitialize the completion under dev_lock before issuing a
         command, re-enabling the interrupt after triggering new cmd
       - 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       | 105 ++++++++++++++++++++++++++++----
 include/linux/mfd/loongson-se.h |   1 +
 2 files changed, 93 insertions(+), 13 deletions(-)


base-commit: cee9395acd8043be0644b25c34bfa86623f2b935
-- 
2.47.2

[PATCH v6 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 v6 1/2] mfd: loongson-se: Add multi-node support

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [Medium] Architecture Guidelines Violation: Bespoke helper functions exported for MFD child drivers.
--

commit 5586c039b03a6072df309c0fa8d59ce3ed3dc98b
Author: Qunqin Zhao <zhaoqunqin@loongson.cn>

mfd: loongson-se: Add multi-node support

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.

> diff --git a/drivers/mfd/loongson-se.c b/drivers/mfd/loongson-se.c
> index 3902ba377d690..7f552a8ee6a26 100644
> --- a/drivers/mfd/loongson-se.c
> +++ b/drivers/mfd/loongson-se.c

[ ... ]

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

[Severity: Medium]
This is a pre-existing issue, but does this driver export bespoke accessor
functions specifically for child drivers?

Looking at drivers/mfd/loongson-se.c:

struct loongson_se_engine *loongson_se_init_engine(struct device *dev, int id)
{
    ...
}

and loongson_se_send_engine_cmd(), these appear to be bespoke APIs explicitly
bypassing standard kernel subsystems for cross-driver communication.

The MFD subsystem guidelines expressly forbid writing bespoke accessors
or helper functions in the parent to pass state to child devices, as this
creates tight inter-driver coupling and initialization interdependencies.

Is it possible to refactor this so child drivers directly access parent data
using standard APIs?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260907023827.14905-1-zhaoqunqin@163.com?part=1

[PATCH v6 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.

- 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@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       | 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;
 	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,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

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:

New issues:
- [High] Read-modify-write operations on the SE_S2LINT_EN register are inconsistently protected by two different spinlocks, causing a data race and lost interrupt masks.
- [High] The loongson_se_stop cleanup handler uses an interruptible wait, allowing pending signals to bypass hardware teardown and cause DMA use-after-free.
- [High] Re-initializing an actively monitored completion structure dynamically in `loongson_se_init_engine` causes a race condition with active IRQ handlers.

Pre-existing issues:
- [High] The exported API `loongson_se_init_engine()` lacks bounds validation on the caller-provided `id`, allowing array overruns and DMA buffer overlaps.
- [Medium] The parent driver exposes bespoke accessors and driver-level callbacks to child drivers, directly violating MFD subsystem guidelines.
- [Medium] Driver-specific header file is inappropriately placed in the global include/linux/mfd/ directory.
--

commit d7b07e09007f31031f54d602ea81c002547a5aa5
Author: Qunqin Zhao <zhaoqunqin@loongson.cn>
mfd: loongson-se: Fix miscellaneous issues

This patch addresses several issues discovered in the Loongson Security
Engine driver, including adding bounds checks, serializing command
submissions, handling EPROBE_DEFER, and setting up an explicit stop
cleanup handler.

Link: https://lore.kernel.org/all/20260618095949.GB1672911@google.com/ [1]

> diff --git a/drivers/mfd/loongson-se.c b/drivers/mfd/loongson-se.c
> index 7f552a8ee6a26..1d332c418bea7 100644
> --- a/drivers/mfd/loongson-se.c
> +++ b/drivers/mfd/loongson-se.c
[ ... ]
> @@ -42,7 +44,7 @@ static int loongson_se_poll(struct loongson_se *se, u32 int_bit)
[ ... ]
> -	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);

[Severity: High]
Does this introduce a data race on the SE_S2LINT_EN register?
This modifies the register under poll_lock, while the new
loongson_se_reinit_completion() function modifies the exact same register
under dev_lock. If two child drivers submit commands concurrently to
different engines, their read-modify-write cycles could interleave,
causing one thread to overwrite and erase the interrupt mask modifications
made by the other. This could lead to permanently stalled commands.

[ ... ]
> @@ -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)
>  {

[Severity: Medium]
This is a pre-existing issue, but does exporting this driver-level
callback directly violate MFD subsystem guidelines forbidding tight functional
coupling? The guidelines state sibling or child drivers should not make
direct function calls to each other bypassing standard kernel APIs.

> +	int err;
> +
> +	loongson_se_reinit_completion(engine->se, &engine->completion, BIT(engine->id));
[ ... ]
> @@ -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);

[Severity: Medium]
This is also a pre-existing issue, but does exporting this custom parent-child
accessor violate the MFD subsystem guidelines? The guidelines state
that bespoke accessors or helper functions should not be written in the parent
to pass state to child devices.

>  	struct loongson_se_engine *engine = &se->engines[id];

[Severity: High]
This is a pre-existing issue, but is it safe to use the unvalidated id
argument directly as an array index? If a caller passes an id greater
than or equal to SE_ENGINE_MAX, it would cause an out-of-bounds memory
access. Also, if id is 0, the assigned data buffer would completely overlap
with the command streams of all other engines.

> -	struct loongson_se_controller_cmd cmd;
> +	struct loongson_se_controller_cmd cmd = {0};
[ ... ]
> @@ -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);

[Severity: High]
Could this cleanup handler fail to stop the controller if a signal is pending?
loongson_se_send_controller_cmd() waits for completion using an
interruptible wait. If a signal is pending (like the user hitting Ctrl-C
during probe failure or module unload), the wait instantly returns without
waiting for the hardware. Because loongson_se_stop() ignores this abort,
the devres core would immediately free the coherent DMA memory while the
hardware is potentially still active, causing a use-after-free.

> +
> +	writel(0, se->base + SE_S2LINT_EN);
> +}
[ ... ]
> @@ -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);
> +

[Severity: High]
Could this duplicate initialization conflict with the existing
init_completion() call inside loongson_se_init_engine()? Since hardware
interrupts are fully unmasked later in probe(), a spurious interrupt could
arrive concurrently while child drivers probe and call
loongson_se_init_engine(). If the IRQ handler calls complete() exactly
when the dynamic init_completion() resets the internal wait queue lock,
it could corrupt the spinlock and cause a kernel panic.

[ ... ]
> diff --git a/include/linux/mfd/loongson-se.h b/include/linux/mfd/loongson-se.h
> index 07afa0c2524d5..8237ccab7bf1e 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

[Severity: Medium]
This is a pre-existing issue, but does placing this driver-specific header
file in the global include/linux/mfd/ directory violate MFD guidelines?
The guidelines state that header files only used by the parent and its
immediate children should be kept local to drivers/mfd/ to avoid global
namespace pollution.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260907023827.14905-1-zhaoqunqin@163.com?part=2

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

[Lee Jones]

On Mon, 07 Sep 2026, sashiko-bot@kernel.org wrote:

> Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
> 
> New issues:
> - [High] Read-modify-write operations on the SE_S2LINT_EN register are inconsistently protected by two different spinlocks, causing a data race and lost interrupt masks.
> - [High] The loongson_se_stop cleanup handler uses an interruptible wait, allowing pending signals to bypass hardware teardown and cause DMA use-after-free.
> - [High] Re-initializing an actively monitored completion structure dynamically in `loongson_se_init_engine` causes a race condition with active IRQ handlers.

Please review, explain or fix the Sashiko review(s).

If this is a set, this request includes the other patches too.

> Pre-existing issues:
> - [High] The exported API `loongson_se_init_engine()` lacks bounds validation on the caller-provided `id`, allowing array overruns and DMA buffer overlaps.
> - [Medium] The parent driver exposes bespoke accessors and driver-level callbacks to child drivers, directly violating MFD subsystem guidelines.
> - [Medium] Driver-specific header file is inappropriately placed in the global include/linux/mfd/ directory.
> --
> 
> commit d7b07e09007f31031f54d602ea81c002547a5aa5
> Author: Qunqin Zhao <zhaoqunqin@loongson.cn>
> mfd: loongson-se: Fix miscellaneous issues
> 
> This patch addresses several issues discovered in the Loongson Security
> Engine driver, including adding bounds checks, serializing command
> submissions, handling EPROBE_DEFER, and setting up an explicit stop
> cleanup handler.
> 
> Link: https://lore.kernel.org/all/20260618095949.GB1672911@google.com/ [1]
> 
> > diff --git a/drivers/mfd/loongson-se.c b/drivers/mfd/loongson-se.c
> > index 7f552a8ee6a26..1d332c418bea7 100644
> > --- a/drivers/mfd/loongson-se.c
> > +++ b/drivers/mfd/loongson-se.c
> [ ... ]
> > @@ -42,7 +44,7 @@ static int loongson_se_poll(struct loongson_se *se, u32 int_bit)
> [ ... ]
> > -	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);
> 
> [Severity: High]
> Does this introduce a data race on the SE_S2LINT_EN register?
> This modifies the register under poll_lock, while the new
> loongson_se_reinit_completion() function modifies the exact same register
> under dev_lock. If two child drivers submit commands concurrently to
> different engines, their read-modify-write cycles could interleave,
> causing one thread to overwrite and erase the interrupt mask modifications
> made by the other. This could lead to permanently stalled commands.
> 
> [ ... ]
> > @@ -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)
> >  {
> 
> [Severity: Medium]
> This is a pre-existing issue, but does exporting this driver-level
> callback directly violate MFD subsystem guidelines forbidding tight functional
> coupling? The guidelines state sibling or child drivers should not make
> direct function calls to each other bypassing standard kernel APIs.
> 
> > +	int err;
> > +
> > +	loongson_se_reinit_completion(engine->se, &engine->completion, BIT(engine->id));
> [ ... ]
> > @@ -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);
> 
> [Severity: Medium]
> This is also a pre-existing issue, but does exporting this custom parent-child
> accessor violate the MFD subsystem guidelines? The guidelines state
> that bespoke accessors or helper functions should not be written in the parent
> to pass state to child devices.
> 
> >  	struct loongson_se_engine *engine = &se->engines[id];
> 
> [Severity: High]
> This is a pre-existing issue, but is it safe to use the unvalidated id
> argument directly as an array index? If a caller passes an id greater
> than or equal to SE_ENGINE_MAX, it would cause an out-of-bounds memory
> access. Also, if id is 0, the assigned data buffer would completely overlap
> with the command streams of all other engines.
> 
> > -	struct loongson_se_controller_cmd cmd;
> > +	struct loongson_se_controller_cmd cmd = {0};
> [ ... ]
> > @@ -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);
> 
> [Severity: High]
> Could this cleanup handler fail to stop the controller if a signal is pending?
> loongson_se_send_controller_cmd() waits for completion using an
> interruptible wait. If a signal is pending (like the user hitting Ctrl-C
> during probe failure or module unload), the wait instantly returns without
> waiting for the hardware. Because loongson_se_stop() ignores this abort,
> the devres core would immediately free the coherent DMA memory while the
> hardware is potentially still active, causing a use-after-free.
> 
> > +
> > +	writel(0, se->base + SE_S2LINT_EN);
> > +}
> [ ... ]
> > @@ -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);
> > +
> 
> [Severity: High]
> Could this duplicate initialization conflict with the existing
> init_completion() call inside loongson_se_init_engine()? Since hardware
> interrupts are fully unmasked later in probe(), a spurious interrupt could
> arrive concurrently while child drivers probe and call
> loongson_se_init_engine(). If the IRQ handler calls complete() exactly
> when the dynamic init_completion() resets the internal wait queue lock,
> it could corrupt the spinlock and cause a kernel panic.
> 
> [ ... ]
> > diff --git a/include/linux/mfd/loongson-se.h b/include/linux/mfd/loongson-se.h
> > index 07afa0c2524d5..8237ccab7bf1e 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
> 
> [Severity: Medium]
> This is a pre-existing issue, but does placing this driver-specific header
> file in the global include/linux/mfd/ directory violate MFD guidelines?
> The guidelines state that header files only used by the parent and its
> immediate children should be kept local to drivers/mfd/ to avoid global
> namespace pollution.
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260907023827.14905-1-zhaoqunqin@163.com?part=2

-- 
Lee Jones

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

[Huacai Chen]

Hi, Qunqin,

On Mon, Sep 7, 2026 at 10:38 AM 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.
>
> - 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@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       | 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);
>
>         /* 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
>

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@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.
>>
>> - 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@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       | 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
>>