[PATCH 0/8] firmware: imx: scu: Misc update

[PATCH 0/8] firmware: imx: scu: Misc update

[Peng Fan]

various misc update for imx-scu.c and imx-scu-irq.c.
Patch 1 is a bug fix, but not critical.
For other patches, there is no real issue reported, I just reviewed
the code again and see some potential risk, so patch 2-4 is to
avoid potential issues. Other patches are misc patches, regarding
error code update, use devm_x API, suppress bind attr.

Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
Peng Fan (8):
      firmware: imx: scu-irq: fix OF node leak in
      firmware: imx: scu-irq: Free mailbox client on failure
      firmware: imx: scu-irq: Init workqueue before request mbox channel
      firmware: imx: scu-irq: Set mu_resource_id before get handle
      firmware: imx: scu-irq: Remove unused export of imx_scu_enable_general_irq_channel
      firmware: imx: scu: Update error code
      firmware: imx: scu: Suppress bind attrs
      firmware: imx: scu: Use devm_mutex_init

 drivers/firmware/imx/imx-scu-irq.c | 28 +++++++++++++++-------------
 drivers/firmware/imx/imx-scu.c     | 11 +++++++----
 2 files changed, 22 insertions(+), 17 deletions(-)
---
base-commit: 2b763d4652393c90eaa771a5164502ec9dd965ae
change-id: 20251012-imx-firmware-492ba9230ef2

Best regards,
-- 
Peng Fan <peng.fan@nxp.com>

[PATCH 1/8] firmware: imx: scu-irq: fix OF node leak in

[Peng Fan]

imx_scu_enable_general_irq_channel() calls of_parse_phandle_with_args(),
but does not release the OF node reference. Add a of_node_put() call
to release the reference.

Fixes: 851826c7566e ("firmware: imx: enable imx scu general irq function")
Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 drivers/firmware/imx/imx-scu-irq.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/firmware/imx/imx-scu-irq.c b/drivers/firmware/imx/imx-scu-irq.c
index 6125cccc9ba79cd3445a720935b5c0b276c83d73..f2b902e95b738fae90af9cbe54da4f488219906f 100644
--- a/drivers/firmware/imx/imx-scu-irq.c
+++ b/drivers/firmware/imx/imx-scu-irq.c
@@ -226,8 +226,10 @@ int imx_scu_enable_general_irq_channel(struct device *dev)
 	INIT_WORK(&imx_sc_irq_work, imx_scu_irq_work_handler);
 
 	if (!of_parse_phandle_with_args(dev->of_node, "mboxes",
-				       "#mbox-cells", 0, &spec))
+				       "#mbox-cells", 0, &spec)) {
 		i = of_alias_get_id(spec.np, "mu");
+		of_node_put(spec.np);
+	}
 
 	/* use mu1 as general mu irq channel if failed */
 	if (i < 0)

-- 
2.37.1

[PATCH 2/8] firmware: imx: scu-irq: Free mailbox client on failure

[Peng Fan]

With mailbox channel freed, it is pointless to keep mailbox client.
So free the mailbox client in err path.

Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 drivers/firmware/imx/imx-scu-irq.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/firmware/imx/imx-scu-irq.c b/drivers/firmware/imx/imx-scu-irq.c
index f2b902e95b738fae90af9cbe54da4f488219906f..1fbe4c3de5c1592bfcf2334a83776c25d5ca7a3f 100644
--- a/drivers/firmware/imx/imx-scu-irq.c
+++ b/drivers/firmware/imx/imx-scu-irq.c
@@ -255,6 +255,7 @@ int imx_scu_enable_general_irq_channel(struct device *dev)
 
 free_ch:
 	mbox_free_channel(ch);
+	devm_kfree(dev, cl);
 
 	return ret;
 }

-- 
2.37.1

[PATCH 3/8] firmware: imx: scu-irq: Init workqueue before request mbox channel

[Peng Fan]

With mailbox channel requested, there is possibility that interrupts may
come in, so need to make sure the workqueue is initialized before
the queue is scheduled by mailbox rx callback.

Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 drivers/firmware/imx/imx-scu-irq.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/firmware/imx/imx-scu-irq.c b/drivers/firmware/imx/imx-scu-irq.c
index 1fbe4c3de5c1592bfcf2334a83776c25d5ca7a3f..a53ed2040c0cf7065474d681b2eb933a15877380 100644
--- a/drivers/firmware/imx/imx-scu-irq.c
+++ b/drivers/firmware/imx/imx-scu-irq.c
@@ -214,6 +214,8 @@ int imx_scu_enable_general_irq_channel(struct device *dev)
 	cl->dev = dev;
 	cl->rx_callback = imx_scu_irq_callback;
 
+	INIT_WORK(&imx_sc_irq_work, imx_scu_irq_work_handler);
+
 	/* SCU general IRQ uses general interrupt channel 3 */
 	ch = mbox_request_channel_byname(cl, "gip3");
 	if (IS_ERR(ch)) {
@@ -223,8 +225,6 @@ int imx_scu_enable_general_irq_channel(struct device *dev)
 		return ret;
 	}
 
-	INIT_WORK(&imx_sc_irq_work, imx_scu_irq_work_handler);
-
 	if (!of_parse_phandle_with_args(dev->of_node, "mboxes",
 				       "#mbox-cells", 0, &spec)) {
 		i = of_alias_get_id(spec.np, "mu");

-- 
2.37.1

[PATCH 4/8] firmware: imx: scu-irq: Set mu_resource_id before get handle

[Peng Fan]

mu_resource_id is referenced in imx_scu_irq_get_status() and
imx_scu_irq_group_enable() which could be used by other modules, so
need to set correct value before using imx_sc_irq_ipc_handle in
SCU API call.

Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 drivers/firmware/imx/imx-scu-irq.c | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/drivers/firmware/imx/imx-scu-irq.c b/drivers/firmware/imx/imx-scu-irq.c
index a53ed2040c0cf7065474d681b2eb933a15877380..1346b75596293892ccd90a856d46f52171d88734 100644
--- a/drivers/firmware/imx/imx-scu-irq.c
+++ b/drivers/firmware/imx/imx-scu-irq.c
@@ -203,6 +203,18 @@ int imx_scu_enable_general_irq_channel(struct device *dev)
 	struct mbox_chan *ch;
 	int ret = 0, i = 0;
 
+	if (!of_parse_phandle_with_args(dev->of_node, "mboxes",
+				       "#mbox-cells", 0, &spec)) {
+		i = of_alias_get_id(spec.np, "mu");
+		of_node_put(spec.np);
+	}
+
+	/* use mu1 as general mu irq channel if failed */
+	if (i < 0)
+		i = 1;
+
+	mu_resource_id = IMX_SC_R_MU_0A + i;
+
 	ret = imx_scu_get_handle(&imx_sc_irq_ipc_handle);
 	if (ret)
 		return ret;
@@ -225,18 +237,6 @@ int imx_scu_enable_general_irq_channel(struct device *dev)
 		return ret;
 	}
 
-	if (!of_parse_phandle_with_args(dev->of_node, "mboxes",
-				       "#mbox-cells", 0, &spec)) {
-		i = of_alias_get_id(spec.np, "mu");
-		of_node_put(spec.np);
-	}
-
-	/* use mu1 as general mu irq channel if failed */
-	if (i < 0)
-		i = 1;
-
-	mu_resource_id = IMX_SC_R_MU_0A + i;
-
 	/* Create directory under /sysfs/firmware */
 	wakeup_obj = kobject_create_and_add("scu_wakeup_source", firmware_kobj);
 	if (!wakeup_obj) {

-- 
2.37.1

[PATCH 5/8] firmware: imx: scu-irq: Remove unused export of imx_scu_enable_general_irq_channel

[Peng Fan]

Since its introduction, this symbol has not been used by any loadable
modules. It remains only referenced within imx-scu.c, which is always built
together with imx-scu-irq.c

As such, exporting imx_scu_enable_general_irq_channel is unnecessary, so
remove the export.

Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 drivers/firmware/imx/imx-scu-irq.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/firmware/imx/imx-scu-irq.c b/drivers/firmware/imx/imx-scu-irq.c
index 1346b75596293892ccd90a856d46f52171d88734..0be9c4c75d826a641e7078a265d30f146e8eb14d 100644
--- a/drivers/firmware/imx/imx-scu-irq.c
+++ b/drivers/firmware/imx/imx-scu-irq.c
@@ -259,4 +259,3 @@ int imx_scu_enable_general_irq_channel(struct device *dev)
 
 	return ret;
 }
-EXPORT_SYMBOL(imx_scu_enable_general_irq_channel);

-- 
2.37.1

[PATCH 6/8] firmware: imx: scu: Update error code

[Peng Fan]

IMX_SC_ERR_NOTFOUND should map with -ENOENT, not -EEXIST. -ENODEV makes
more sense for IMX_SC_ERR_NOPOWER, and -ECOMM makes more sense for
IMX_SC_ERR_IPC.

Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 drivers/firmware/imx/imx-scu.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/firmware/imx/imx-scu.c b/drivers/firmware/imx/imx-scu.c
index 8c28e25ddc8a650d2d191c40193cf1fb5e70bc35..6046156bc3c9abcd8f62b46f04571b1a9decc0eb 100644
--- a/drivers/firmware/imx/imx-scu.c
+++ b/drivers/firmware/imx/imx-scu.c
@@ -73,9 +73,9 @@ static int imx_sc_linux_errmap[IMX_SC_ERR_LAST] = {
 	-EACCES, /* IMX_SC_ERR_NOACCESS */
 	-EACCES, /* IMX_SC_ERR_LOCKED */
 	-ERANGE, /* IMX_SC_ERR_UNAVAILABLE */
-	-EEXIST, /* IMX_SC_ERR_NOTFOUND */
-	-EPERM,	 /* IMX_SC_ERR_NOPOWER */
-	-EPIPE,	 /* IMX_SC_ERR_IPC */
+	-ENOENT, /* IMX_SC_ERR_NOTFOUND */
+	-ENODEV, /* IMX_SC_ERR_NOPOWER */
+	-ECOMM,	 /* IMX_SC_ERR_IPC */
 	-EBUSY,	 /* IMX_SC_ERR_BUSY */
 	-EIO,	 /* IMX_SC_ERR_FAIL */
 };

-- 
2.37.1

[PATCH 7/8] firmware: imx: scu: Suppress bind attrs

[Peng Fan]

The SCU driver is critical for system working properly, it should
never be removed and binded again. So suppress the bind attrs

Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 drivers/firmware/imx/imx-scu.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/firmware/imx/imx-scu.c b/drivers/firmware/imx/imx-scu.c
index 6046156bc3c9abcd8f62b46f04571b1a9decc0eb..630e3dba4db15961ae4d77273af6248be614145e 100644
--- a/drivers/firmware/imx/imx-scu.c
+++ b/drivers/firmware/imx/imx-scu.c
@@ -352,6 +352,7 @@ static struct platform_driver imx_scu_driver = {
 	.driver = {
 		.name = "imx-scu",
 		.of_match_table = imx_scu_match,
+		.suppress_bind_attrs = true,
 	},
 	.probe = imx_scu_probe,
 };

-- 
2.37.1

[PATCH 8/8] firmware: imx: scu: Use devm_mutex_init

[Peng Fan]

In normal case, there is no need to invoke mutex_destroy in error path,
but it is useful when CONFIG_DEBUG_MUTEXES, so use devm_mutex_init().

Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 drivers/firmware/imx/imx-scu.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/firmware/imx/imx-scu.c b/drivers/firmware/imx/imx-scu.c
index 630e3dba4db15961ae4d77273af6248be614145e..67b267a7408a12deed77d2c8f52d5f64b239a408 100644
--- a/drivers/firmware/imx/imx-scu.c
+++ b/drivers/firmware/imx/imx-scu.c
@@ -324,7 +324,9 @@ static int imx_scu_probe(struct platform_device *pdev)
 	}
 
 	sc_ipc->dev = dev;
-	mutex_init(&sc_ipc->lock);
+	ret = devm_mutex_init(dev, &sc_ipc->lock);
+	if (ret)
+		return ret;
 	init_completion(&sc_ipc->done);
 
 	imx_sc_ipc_handle = sc_ipc;

-- 
2.37.1

Re: [PATCH 1/8] firmware: imx: scu-irq: fix OF node leak in

[Frank Li]

On Tue, Oct 14, 2025 at 12:54:38PM +0800, Peng Fan wrote:
> imx_scu_enable_general_irq_channel() calls of_parse_phandle_with_args(),
> but does not release the OF node reference. Add a of_node_put() call
> to release the reference.
>
> Fixes: 851826c7566e ("firmware: imx: enable imx scu general irq function")
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---

Reviewed-by: Frank Li <Frank.Li@nxp.com>

>  drivers/firmware/imx/imx-scu-irq.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/firmware/imx/imx-scu-irq.c b/drivers/firmware/imx/imx-scu-irq.c
> index 6125cccc9ba79cd3445a720935b5c0b276c83d73..f2b902e95b738fae90af9cbe54da4f488219906f 100644
> --- a/drivers/firmware/imx/imx-scu-irq.c
> +++ b/drivers/firmware/imx/imx-scu-irq.c
> @@ -226,8 +226,10 @@ int imx_scu_enable_general_irq_channel(struct device *dev)
>  	INIT_WORK(&imx_sc_irq_work, imx_scu_irq_work_handler);
>
>  	if (!of_parse_phandle_with_args(dev->of_node, "mboxes",
> -				       "#mbox-cells", 0, &spec))
> +				       "#mbox-cells", 0, &spec)) {
>  		i = of_alias_get_id(spec.np, "mu");
> +		of_node_put(spec.np);
> +	}
>
>  	/* use mu1 as general mu irq channel if failed */
>  	if (i < 0)
>
> --
> 2.37.1
>

Re: [PATCH 2/8] firmware: imx: scu-irq: Free mailbox client on failure

[Frank Li]

On Tue, Oct 14, 2025 at 12:54:39PM +0800, Peng Fan wrote:
> With mailbox channel freed, it is pointless to keep mailbox client.
> So free the mailbox client in err path.
>
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
>  drivers/firmware/imx/imx-scu-irq.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/firmware/imx/imx-scu-irq.c b/drivers/firmware/imx/imx-scu-irq.c
> index f2b902e95b738fae90af9cbe54da4f488219906f..1fbe4c3de5c1592bfcf2334a83776c25d5ca7a3f 100644
> --- a/drivers/firmware/imx/imx-scu-irq.c
> +++ b/drivers/firmware/imx/imx-scu-irq.c
> @@ -255,6 +255,7 @@ int imx_scu_enable_general_irq_channel(struct device *dev)
>
>  free_ch:
>  	mbox_free_channel(ch);
> +	devm_kfree(dev, cl);


you use devm_kmalloc(), when return failure, framework will auto free cl.

Avoid mixing manual free and management free code.

So I think this patch is not neccesary.

Frank
>
>  	return ret;
>  }
>
> --
> 2.37.1
>

Re: [PATCH 3/8] firmware: imx: scu-irq: Init workqueue before request mbox channel

[Frank Li]

On Tue, Oct 14, 2025 at 12:54:40PM +0800, Peng Fan wrote:
> With mailbox channel requested, there is possibility that interrupts may
> come in, so need to make sure the workqueue is initialized before
> the queue is scheduled by mailbox rx callback.
>
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---

Reviewed-by: Frank Li <Frank.Li@nxp.com>

>  drivers/firmware/imx/imx-scu-irq.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/firmware/imx/imx-scu-irq.c b/drivers/firmware/imx/imx-scu-irq.c
> index 1fbe4c3de5c1592bfcf2334a83776c25d5ca7a3f..a53ed2040c0cf7065474d681b2eb933a15877380 100644
> --- a/drivers/firmware/imx/imx-scu-irq.c
> +++ b/drivers/firmware/imx/imx-scu-irq.c
> @@ -214,6 +214,8 @@ int imx_scu_enable_general_irq_channel(struct device *dev)
>  	cl->dev = dev;
>  	cl->rx_callback = imx_scu_irq_callback;
>
> +	INIT_WORK(&imx_sc_irq_work, imx_scu_irq_work_handler);
> +
>  	/* SCU general IRQ uses general interrupt channel 3 */
>  	ch = mbox_request_channel_byname(cl, "gip3");
>  	if (IS_ERR(ch)) {
> @@ -223,8 +225,6 @@ int imx_scu_enable_general_irq_channel(struct device *dev)
>  		return ret;
>  	}
>
> -	INIT_WORK(&imx_sc_irq_work, imx_scu_irq_work_handler);
> -
>  	if (!of_parse_phandle_with_args(dev->of_node, "mboxes",
>  				       "#mbox-cells", 0, &spec)) {
>  		i = of_alias_get_id(spec.np, "mu");
>
> --
> 2.37.1
>

Re: [PATCH 4/8] firmware: imx: scu-irq: Set mu_resource_id before get handle

[Frank Li]

On Tue, Oct 14, 2025 at 12:54:41PM +0800, Peng Fan wrote:
> mu_resource_id is referenced in imx_scu_irq_get_status() and
> imx_scu_irq_group_enable() which could be used by other modules, so
> need to set correct value before using imx_sc_irq_ipc_handle in
> SCU API call.
>
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---

Reviewed-by: Frank Li <Frank.Li@nxp.com>

>  drivers/firmware/imx/imx-scu-irq.c | 24 ++++++++++++------------
>  1 file changed, 12 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/firmware/imx/imx-scu-irq.c b/drivers/firmware/imx/imx-scu-irq.c
> index a53ed2040c0cf7065474d681b2eb933a15877380..1346b75596293892ccd90a856d46f52171d88734 100644
> --- a/drivers/firmware/imx/imx-scu-irq.c
> +++ b/drivers/firmware/imx/imx-scu-irq.c
> @@ -203,6 +203,18 @@ int imx_scu_enable_general_irq_channel(struct device *dev)
>  	struct mbox_chan *ch;
>  	int ret = 0, i = 0;
>
> +	if (!of_parse_phandle_with_args(dev->of_node, "mboxes",
> +				       "#mbox-cells", 0, &spec)) {
> +		i = of_alias_get_id(spec.np, "mu");
> +		of_node_put(spec.np);
> +	}
> +
> +	/* use mu1 as general mu irq channel if failed */
> +	if (i < 0)
> +		i = 1;
> +
> +	mu_resource_id = IMX_SC_R_MU_0A + i;
> +
>  	ret = imx_scu_get_handle(&imx_sc_irq_ipc_handle);
>  	if (ret)
>  		return ret;
> @@ -225,18 +237,6 @@ int imx_scu_enable_general_irq_channel(struct device *dev)
>  		return ret;
>  	}
>
> -	if (!of_parse_phandle_with_args(dev->of_node, "mboxes",
> -				       "#mbox-cells", 0, &spec)) {
> -		i = of_alias_get_id(spec.np, "mu");
> -		of_node_put(spec.np);
> -	}
> -
> -	/* use mu1 as general mu irq channel if failed */
> -	if (i < 0)
> -		i = 1;
> -
> -	mu_resource_id = IMX_SC_R_MU_0A + i;
> -
>  	/* Create directory under /sysfs/firmware */
>  	wakeup_obj = kobject_create_and_add("scu_wakeup_source", firmware_kobj);
>  	if (!wakeup_obj) {
>
> --
> 2.37.1
>

Re: [PATCH 5/8] firmware: imx: scu-irq: Remove unused export of imx_scu_enable_general_irq_channel

[Frank Li]

On Tue, Oct 14, 2025 at 12:54:42PM +0800, Peng Fan wrote:
> Since its introduction, this symbol has not been used by any loadable
> modules. It remains only referenced within imx-scu.c, which is always built
> together with imx-scu-irq.c

imx_scu_enable_general_irq_channel is only referenced within imx-scu.c,
which always built together with imx-scu-irq.c.

>
> As such, exporting imx_scu_enable_general_irq_channel is unnecessary, so
> remove the export.
>
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---

Reviewed-by: Frank Li <Frank.Li@nxp.com>

>  drivers/firmware/imx/imx-scu-irq.c | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/drivers/firmware/imx/imx-scu-irq.c b/drivers/firmware/imx/imx-scu-irq.c
> index 1346b75596293892ccd90a856d46f52171d88734..0be9c4c75d826a641e7078a265d30f146e8eb14d 100644
> --- a/drivers/firmware/imx/imx-scu-irq.c
> +++ b/drivers/firmware/imx/imx-scu-irq.c
> @@ -259,4 +259,3 @@ int imx_scu_enable_general_irq_channel(struct device *dev)
>
>  	return ret;
>  }
> -EXPORT_SYMBOL(imx_scu_enable_general_irq_channel);
>
> --
> 2.37.1
>

Re: [PATCH 6/8] firmware: imx: scu: Update error code

[Frank Li]

On Tue, Oct 14, 2025 at 12:54:43PM +0800, Peng Fan wrote:
> IMX_SC_ERR_NOTFOUND should map with -ENOENT, not -EEXIST. -ENODEV makes
> more sense for IMX_SC_ERR_NOPOWER, and -ECOMM makes more sense for
> IMX_SC_ERR_IPC.
>
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---

Reviewed-by: Frank Li <Frank.Li@nxp.com>

>  drivers/firmware/imx/imx-scu.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/firmware/imx/imx-scu.c b/drivers/firmware/imx/imx-scu.c
> index 8c28e25ddc8a650d2d191c40193cf1fb5e70bc35..6046156bc3c9abcd8f62b46f04571b1a9decc0eb 100644
> --- a/drivers/firmware/imx/imx-scu.c
> +++ b/drivers/firmware/imx/imx-scu.c
> @@ -73,9 +73,9 @@ static int imx_sc_linux_errmap[IMX_SC_ERR_LAST] = {
>  	-EACCES, /* IMX_SC_ERR_NOACCESS */
>  	-EACCES, /* IMX_SC_ERR_LOCKED */
>  	-ERANGE, /* IMX_SC_ERR_UNAVAILABLE */
> -	-EEXIST, /* IMX_SC_ERR_NOTFOUND */
> -	-EPERM,	 /* IMX_SC_ERR_NOPOWER */
> -	-EPIPE,	 /* IMX_SC_ERR_IPC */
> +	-ENOENT, /* IMX_SC_ERR_NOTFOUND */
> +	-ENODEV, /* IMX_SC_ERR_NOPOWER */
> +	-ECOMM,	 /* IMX_SC_ERR_IPC */
>  	-EBUSY,	 /* IMX_SC_ERR_BUSY */
>  	-EIO,	 /* IMX_SC_ERR_FAIL */
>  };
>
> --
> 2.37.1
>

Re: [PATCH 7/8] firmware: imx: scu: Suppress bind attrs

[Frank Li]

On Tue, Oct 14, 2025 at 12:54:44PM +0800, Peng Fan wrote:
> The SCU driver is critical for system working properly, it should
> never be removed and binded again. So suppress the bind attrs
>
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---

Reviewed-by: Frank Li <Frank.Li@nxp.com>

>  drivers/firmware/imx/imx-scu.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/firmware/imx/imx-scu.c b/drivers/firmware/imx/imx-scu.c
> index 6046156bc3c9abcd8f62b46f04571b1a9decc0eb..630e3dba4db15961ae4d77273af6248be614145e 100644
> --- a/drivers/firmware/imx/imx-scu.c
> +++ b/drivers/firmware/imx/imx-scu.c
> @@ -352,6 +352,7 @@ static struct platform_driver imx_scu_driver = {
>  	.driver = {
>  		.name = "imx-scu",
>  		.of_match_table = imx_scu_match,
> +		.suppress_bind_attrs = true,
>  	},
>  	.probe = imx_scu_probe,
>  };
>
> --
> 2.37.1
>

Re: [PATCH 8/8] firmware: imx: scu: Use devm_mutex_init

[Frank Li]

On Tue, Oct 14, 2025 at 12:54:45PM +0800, Peng Fan wrote:
> In normal case, there is no need to invoke mutex_destroy in error path,
> but it is useful when CONFIG_DEBUG_MUTEXES, so use devm_mutex_init().
>
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---

Reviewed-by: Frank Li <Frank.Li@nxp.com>

>  drivers/firmware/imx/imx-scu.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/firmware/imx/imx-scu.c b/drivers/firmware/imx/imx-scu.c
> index 630e3dba4db15961ae4d77273af6248be614145e..67b267a7408a12deed77d2c8f52d5f64b239a408 100644
> --- a/drivers/firmware/imx/imx-scu.c
> +++ b/drivers/firmware/imx/imx-scu.c
> @@ -324,7 +324,9 @@ static int imx_scu_probe(struct platform_device *pdev)
>  	}
>
>  	sc_ipc->dev = dev;
> -	mutex_init(&sc_ipc->lock);
> +	ret = devm_mutex_init(dev, &sc_ipc->lock);
> +	if (ret)
> +		return ret;
>  	init_completion(&sc_ipc->done);
>
>  	imx_sc_ipc_handle = sc_ipc;
>
> --
> 2.37.1
>

Re: [PATCH 2/8] firmware: imx: scu-irq: Free mailbox client on failure

[Peng Fan]

On Tue, Oct 14, 2025 at 11:54:36AM -0400, Frank Li wrote:
>On Tue, Oct 14, 2025 at 12:54:39PM +0800, Peng Fan wrote:
>> With mailbox channel freed, it is pointless to keep mailbox client.
>> So free the mailbox client in err path.
>>
>> Signed-off-by: Peng Fan <peng.fan@nxp.com>
>> ---
>>  drivers/firmware/imx/imx-scu-irq.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/firmware/imx/imx-scu-irq.c b/drivers/firmware/imx/imx-scu-irq.c
>> index f2b902e95b738fae90af9cbe54da4f488219906f..1fbe4c3de5c1592bfcf2334a83776c25d5ca7a3f 100644
>> --- a/drivers/firmware/imx/imx-scu-irq.c
>> +++ b/drivers/firmware/imx/imx-scu-irq.c
>> @@ -255,6 +255,7 @@ int imx_scu_enable_general_irq_channel(struct device *dev)
>>
>>  free_ch:
>>  	mbox_free_channel(ch);
>> +	devm_kfree(dev, cl);
>
>
>you use devm_kmalloc(), when return failure, framework will auto free cl.
>
>Avoid mixing manual free and management free code.
>
>So I think this patch is not neccesary.

Actually in imx-scu.c, there is only a warning message if this API call returns
error. So need to free here.

Thanks
Peng

>
>Frank
>>
>>  	return ret;
>>  }
>>
>> --
>> 2.37.1
>>

Re: [PATCH 2/8] firmware: imx: scu-irq: Free mailbox client on failure

[Frank Li]

On Wed, Oct 15, 2025 at 09:55:03PM +0800, Peng Fan wrote:
> On Tue, Oct 14, 2025 at 11:54:36AM -0400, Frank Li wrote:
> >On Tue, Oct 14, 2025 at 12:54:39PM +0800, Peng Fan wrote:
> >> With mailbox channel freed, it is pointless to keep mailbox client.
> >> So free the mailbox client in err path.
> >>
> >> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> >> ---
> >>  drivers/firmware/imx/imx-scu-irq.c | 1 +
> >>  1 file changed, 1 insertion(+)
> >>
> >> diff --git a/drivers/firmware/imx/imx-scu-irq.c b/drivers/firmware/imx/imx-scu-irq.c
> >> index f2b902e95b738fae90af9cbe54da4f488219906f..1fbe4c3de5c1592bfcf2334a83776c25d5ca7a3f 100644
> >> --- a/drivers/firmware/imx/imx-scu-irq.c
> >> +++ b/drivers/firmware/imx/imx-scu-irq.c
> >> @@ -255,6 +255,7 @@ int imx_scu_enable_general_irq_channel(struct device *dev)
> >>
> >>  free_ch:
> >>  	mbox_free_channel(ch);
> >> +	devm_kfree(dev, cl);
> >
> >
> >you use devm_kmalloc(), when return failure, framework will auto free cl.
> >
> >Avoid mixing manual free and management free code.
> >
> >So I think this patch is not neccesary.
>
> Actually in imx-scu.c, there is only a warning message if this API call returns
> error. So need to free here.

what's warning?

Frank
>
> Thanks
> Peng
>
> >
> >Frank
> >>
> >>  	return ret;
> >>  }
> >>
> >> --
> >> 2.37.1
> >>

Re: [PATCH 2/8] firmware: imx: scu-irq: Free mailbox client on failure

[Peng Fan]

On Wed, Oct 15, 2025 at 10:32:35AM -0400, Frank Li wrote:
>On Wed, Oct 15, 2025 at 09:55:03PM +0800, Peng Fan wrote:
>> On Tue, Oct 14, 2025 at 11:54:36AM -0400, Frank Li wrote:
>> >On Tue, Oct 14, 2025 at 12:54:39PM +0800, Peng Fan wrote:
>> >> With mailbox channel freed, it is pointless to keep mailbox client.
>> >> So free the mailbox client in err path.
>> >>
>> >> Signed-off-by: Peng Fan <peng.fan@nxp.com>
>> >> ---
>> >>  drivers/firmware/imx/imx-scu-irq.c | 1 +
>> >>  1 file changed, 1 insertion(+)
>> >>
>> >> diff --git a/drivers/firmware/imx/imx-scu-irq.c b/drivers/firmware/imx/imx-scu-irq.c
>> >> index f2b902e95b738fae90af9cbe54da4f488219906f..1fbe4c3de5c1592bfcf2334a83776c25d5ca7a3f 100644
>> >> --- a/drivers/firmware/imx/imx-scu-irq.c
>> >> +++ b/drivers/firmware/imx/imx-scu-irq.c
>> >> @@ -255,6 +255,7 @@ int imx_scu_enable_general_irq_channel(struct device *dev)
>> >>
>> >>  free_ch:
>> >>  	mbox_free_channel(ch);
>> >> +	devm_kfree(dev, cl);
>> >
>> >
>> >you use devm_kmalloc(), when return failure, framework will auto free cl.
>> >
>> >Avoid mixing manual free and management free code.
>> >
>> >So I think this patch is not neccesary.
>>
>> Actually in imx-scu.c, there is only a warning message if this API call returns
>> error. So need to free here.
>
>what's warning?

When imx_scu_enable_general_irq_channel() fails, there is only a warning
printed out as below, the probe continues.

        ret = imx_scu_enable_general_irq_channel(dev);
        if (ret)
                dev_warn(dev,
                        "failed to enable general irq channel: %d\n", ret);

        dev_info(dev, "NXP i.MX SCU Initialized\n");

        return devm_of_platform_populate(dev);

Thanks,
Peng

>
>Frank
>>
>> Thanks
>> Peng
>>
>> >
>> >Frank
>> >>
>> >>  	return ret;
>> >>  }
>> >>
>> >> --
>> >> 2.37.1
>> >>

Re: [PATCH 2/8] firmware: imx: scu-irq: Free mailbox client on failure

[Frank Li]

On Thu, Oct 16, 2025 at 10:14:59AM +0800, Peng Fan wrote:
> On Wed, Oct 15, 2025 at 10:32:35AM -0400, Frank Li wrote:
> >On Wed, Oct 15, 2025 at 09:55:03PM +0800, Peng Fan wrote:
> >> On Tue, Oct 14, 2025 at 11:54:36AM -0400, Frank Li wrote:
> >> >On Tue, Oct 14, 2025 at 12:54:39PM +0800, Peng Fan wrote:
> >> >> With mailbox channel freed, it is pointless to keep mailbox client.
> >> >> So free the mailbox client in err path.
> >> >>
> >> >> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> >> >> ---
> >> >>  drivers/firmware/imx/imx-scu-irq.c | 1 +
> >> >>  1 file changed, 1 insertion(+)
> >> >>
> >> >> diff --git a/drivers/firmware/imx/imx-scu-irq.c b/drivers/firmware/imx/imx-scu-irq.c
> >> >> index f2b902e95b738fae90af9cbe54da4f488219906f..1fbe4c3de5c1592bfcf2334a83776c25d5ca7a3f 100644
> >> >> --- a/drivers/firmware/imx/imx-scu-irq.c
> >> >> +++ b/drivers/firmware/imx/imx-scu-irq.c
> >> >> @@ -255,6 +255,7 @@ int imx_scu_enable_general_irq_channel(struct device *dev)
> >> >>
> >> >>  free_ch:
> >> >>  	mbox_free_channel(ch);
> >> >> +	devm_kfree(dev, cl);
> >> >
> >> >
> >> >you use devm_kmalloc(), when return failure, framework will auto free cl.
> >> >
> >> >Avoid mixing manual free and management free code.
> >> >
> >> >So I think this patch is not neccesary.
> >>
> >> Actually in imx-scu.c, there is only a warning message if this API call returns
> >> error. So need to free here.
> >
> >what's warning?
>
> When imx_scu_enable_general_irq_channel() fails, there is only a warning
> printed out as below, the probe continues.
>
>         ret = imx_scu_enable_general_irq_channel(dev);
>         if (ret)
>                 dev_warn(dev,
>                         "failed to enable general irq channel: %d\n", ret);
>
>         dev_info(dev, "NXP i.MX SCU Initialized\n");
>
>         return devm_of_platform_populate(dev);
>
> Thanks,
> Peng

It make sense. commit message need be improved to make more clear.

scu-irq: Free mailbox client on failure at imx_scu_enable_general_irq_channel()

The IRQ mailbox is an optional channel and does not need to be kept until
driver removal when an error occurs. Free the allocated memory in the
error path.

There are

 ch = mbox_request_channel_byname(cl, "gip3");
        if (IS_ERR(ch)) {
                ret = PTR_ERR(ch);
                dev_err(dev, "failed to request mbox chan gip3, ret %d\n", ret);
                devm_kfree(dev, cl);
		^^^
                return ret;
        }

Here should goto free_ch also. keep free at one place.

Frank

>
> >
> >Frank
> >>
> >> Thanks
> >> Peng
> >>
> >> >
> >> >Frank
> >> >>
> >> >>  	return ret;
> >> >>  }
> >> >>
> >> >> --
> >> >> 2.37.1
> >> >>