[PATCH v2] scmi: reset: validate number of reset domains

[PATCH v2] scmi: reset: validate number of reset domains

[Artem Shimko]

Add validation to reject zero reset domains during protocol initialization.

The fix adds an explicit check for zero domains in
scmi_reset_protocol_init(), returning -EINVAL early during protocol
initialization. This prevents the driver from proceeding with a
non-functional state and avoids potential kernel panics in functions
like scmi_reset_domain_reset() and scmi_reset_notify_supported() that
assume dom_info is always valid.

The change is minimal and safe, affecting only the error case while
preserving all existing functionality for valid configurations.
The existing -ENOMEM handling for memory allocation remains unchanged
and sufficient.

This change ensures early failure with -EINVAL during protocol
initialization, preventing silent failures and maintaining system
stability. The existing -ENOMEM handling for memory allocation remains
unchanged and sufficient.

Signed-off-by: Artem Shimko <a.shimko.dev@gmail.com>
---
Dear SCMI Maintainers,

This patch addresses an issue in the SCMI reset protocol initialization
where a zero value for num_domains could lead to a non-functional state
or potential NULL pointer dereferences.

Currently, if the platform reports zero reset domains, the driver
continues initialization but creates an inconsistent state:

    ret = scmi_reset_attributes_get(ph, pinfo);
    if (ret)
        return ret;

    /* When num_domains == 0: */
    pinfo->dom_info = devm_kcalloc(ph->dev, pinfo->num_domains,  /* 0 */
          sizeof(*pinfo->dom_info), GFP_KERNEL);
    /* Returns ZERO_SIZE_PTR (not NULL) */
    
    if (!pinfo->dom_info)  /* ZERO_SIZE_PTR != NULL, condition fails */
        return -ENOMEM;

    /* Execution continues! */
    return ph->set_priv(ph, pinfo, version);  /* Returns SUCCESS (0)! */

However, subsequent reset operations crash when accessing dom_info:

    static int scmi_reset_domain_reset(const struct scmi_protocol_handle *ph,
              u32 domain_id)
    {
        struct scmi_reset_info *pi = ph->get_priv(ph);
        struct reset_dom_info *dom = pi->dom_info + domain_id;  
        /* ZERO_SIZE_PTR + domain_id = INVALID POINTER! */
        /* KERNEL PANIC on dom-> access */
    }

The protocol appears to initialize successfully but is actually non-functional
and will crash on first usage.

The patch adds validation to reject zero domains during initialization,
ensuring fail-fast behavior and preventing hidden failures. This approach
maintains system stability by catching invalid configurations early.

Testing confirmed normal operation with positive num_domains values and
proper error handling with zero domains. The change is minimal and safe,
affecting only the error case while preserving all existing functionality
for valid configurations.

This patch fixes a potential crash scenario while maintaining full
backward compatibility with properly configured systems.

If this is a working case, I will check and supplement other protocols such as
sensor and power domain.
--
Best regards,
Artem Shimko

ChangeLog:
  v2: Change commit message

 drivers/firmware/arm_scmi/reset.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/firmware/arm_scmi/reset.c b/drivers/firmware/arm_scmi/reset.c
index 0aa82b96f41b..458b75fcc858 100644
--- a/drivers/firmware/arm_scmi/reset.c
+++ b/drivers/firmware/arm_scmi/reset.c
@@ -358,6 +358,9 @@ static int scmi_reset_protocol_init(const struct scmi_protocol_handle *ph)
 	if (ret)
 		return ret;
 
+	if (!pinfo->num_domains)
+		return -EINVAL;
+
 	pinfo->dom_info = devm_kcalloc(ph->dev, pinfo->num_domains,
 				       sizeof(*pinfo->dom_info), GFP_KERNEL);
 	if (!pinfo->dom_info)
-- 
2.43.0

Re: [PATCH v2] scmi: reset: validate number of reset domains

[Cristian Marussi]

On Mon, Nov 03, 2025 at 07:10:43PM +0300, Artem Shimko wrote:
> Add validation to reject zero reset domains during protocol initialization.
> 

Hi Artem,

> The fix adds an explicit check for zero domains in
> scmi_reset_protocol_init(), returning -EINVAL early during protocol
> initialization. This prevents the driver from proceeding with a
> non-functional state and avoids potential kernel panics in functions
> like scmi_reset_domain_reset() and scmi_reset_notify_supported() that
> assume dom_info is always valid.

Indeed, this was alreay spotted/reported/fixed in other protocols, but
the preferred solution is NOT to bail-out when there are ZERO domains,
but to carry-on WITHOUT crashing of course: the reason for this is
testing scenarios in which you can have a platform/FW reply with ZERO
domains.

> 
> The change is minimal and safe, affecting only the error case while
> preserving all existing functionality for valid configurations.
> The existing -ENOMEM handling for memory allocation remains unchanged
> and sufficient.
>

In fact if you look at the code there are already a lot of places in
reset.c where the code path is anyway guarded by num_domains so it is
NOT problematic.

There are, though, other places where the dom-> dereference is NOT
protected and those could be probelematic.

Have you seen any crash related to this for real when zero num_domains
are reported ?

Anyway, it would be good to harden the protocol code as already done
a bit in other protocols in the past, but I advise you to lookup in
perf.c the scmi_perf_domain_lookup() helper as an example and see
how it used across perf to address a similar scenario and adopt the
same solution for reset in order to harden the code while preserving
the possibility to initialize the protocol even with ZERO domains for
testing purposes.

Thanks,
Cristian

[PATCH v3] firmware: arm_scmi: refactor reset domain handling

[Artem Shimko]

Improve the SCMI reset protocol implementation by centralizing domain
validation and enhancing error handling.

Add scmi_reset_domain_lookup() helper function to validate
domain ids and return appropriate error codes. Refactor all
domain-specific functions to use this helper, ensuring consistent
error handling throughout the reset protocol.

Signed-off-by: Artem Shimko <a.shimko.dev@gmail.com>
--

Hi Cristian,

Thank you for suggesting! If it works for you I'll try to improve the others protocols.

It was tested on my board in QEMU and on real hardware.
The functionality didn't break, and peripherals that are reseted via SCMI work correctly.

As an example, I can provide DMA test results (as one of the processor units dependent on SCMI reset).

[    2.559040] dw_axi_dmac_platform 1000nda0.nda_dma_0: DesignWare AXI DMA Controller, nda channels
[    2.569265] dw_axi_dmac_platform 1000nda0.nda_dma_1: DesignWare AXI DMA Controller, nda channels
[    2.580601] dw_axi_dmac_platform 1000nda0.nda_dma_2: DesignWare AXI DMA Controller, nda channels

# echo 2000 > /sys/module/dmatest/parameters/timeout
# echo 1000 > /sys/module/dmatest/parameters/iterations
# echo dma0chan0 > /sys/module/dmatest/parameters/channel
[   95.917504] dmatest: Added 1 threads using dma0chan0
# echo 1 > /sys/module/dmatest/parameters/run
[  101.931372] dmatest: Started 1 threads using dma0chan0
[  102.731009] dmatest: dma0chan0-copy0: summary 1000 tests, 0 failures 2508.42 iops 19706 KB/s (0)

And vice versa:

1. Confirmed that the DMA module is non-functional without a proper reset via SCMI.
Disabling the reset logic causes a system crash on first access to its registers.

2. Requesting a non-existent reset domain:

[    2.463400] dw_axi_dmac_platform 1000nda0.nda_dma_0: error -EINVAL: Failed to deassert resets
[    2.472091] dw_axi_dmac_platform 1000nda0.nda_dma_0: probe with driver dw_axi_dmac_platform failed with error -22
[    2.482911] dw_axi_dmac_platform 1000nda0.nda_dma_1: error -EINVAL: Failed to deassert resets
[    2.491553] dw_axi_dmac_platform 1000nda0.nda_dma_1: probe with driver dw_axi_dmac_platform failed with error -22
[    2.502256] dw_axi_dmac_platform 1000nda0.nda_dma_2: error -EINVAL: Failed to deassert resets
[    2.510735] dw_axi_dmac_platform 1000nda0.nda_dma_2: probe with driver dw_axi_dmac_platform failed with error -22

--
Regards,
Artem

 drivers/firmware/arm_scmi/reset.c | 59 ++++++++++++++++++++-----------
 1 file changed, 39 insertions(+), 20 deletions(-)

diff --git a/drivers/firmware/arm_scmi/reset.c b/drivers/firmware/arm_scmi/reset.c
index 0aa82b96f41b..9854a3016d13 100644
--- a/drivers/firmware/arm_scmi/reset.c
+++ b/drivers/firmware/arm_scmi/reset.c
@@ -100,6 +100,7 @@ static int scmi_reset_attributes_get(const struct scmi_protocol_handle *ph,
 
 static int
 scmi_reset_domain_attributes_get(const struct scmi_protocol_handle *ph,
+				 struct reset_dom_info *dom_info,
 				 struct scmi_reset_info *pinfo,
 				 u32 domain, u32 version)
 {
@@ -107,7 +108,6 @@ scmi_reset_domain_attributes_get(const struct scmi_protocol_handle *ph,
 	u32 attributes;
 	struct scmi_xfer *t;
 	struct scmi_msg_resp_reset_domain_attributes *attr;
-	struct reset_dom_info *dom_info = pinfo->dom_info + domain;
 
 	ret = ph->xops->xfer_get_init(ph, RESET_DOMAIN_ATTRIBUTES,
 				      sizeof(domain), sizeof(*attr), &t);
@@ -153,23 +153,39 @@ static int scmi_reset_num_domains_get(const struct scmi_protocol_handle *ph)
 	return pi->num_domains;
 }
 
+static inline struct reset_dom_info *
+scmi_reset_domain_lookup(const struct scmi_protocol_handle *ph, u32 domain)
+{
+	struct scmi_reset_info *pi = ph->get_priv(ph);
+
+	if (domain >= pi->num_domains)
+		return ERR_PTR(-EINVAL);
+
+	return pi->dom_info + domain;
+}
+
 static const char *
 scmi_reset_name_get(const struct scmi_protocol_handle *ph, u32 domain)
 {
-	struct scmi_reset_info *pi = ph->get_priv(ph);
+	struct reset_dom_info *dom_info;
 
-	struct reset_dom_info *dom = pi->dom_info + domain;
+	dom_info = scmi_reset_domain_lookup(ph, domain);
+	if (IS_ERR(dom_info))
+		return "unknown";
 
-	return dom->name;
+	return dom_info->name;
 }
 
 static int scmi_reset_latency_get(const struct scmi_protocol_handle *ph,
 				  u32 domain)
 {
-	struct scmi_reset_info *pi = ph->get_priv(ph);
-	struct reset_dom_info *dom = pi->dom_info + domain;
+	struct reset_dom_info *dom_info;
 
-	return dom->latency_us;
+	dom_info = scmi_reset_domain_lookup(ph, domain);
+	if (IS_ERR(dom_info))
+		return PTR_ERR(dom_info);
+
+	return dom_info->latency_us;
 }
 
 static int scmi_domain_reset(const struct scmi_protocol_handle *ph, u32 domain,
@@ -178,14 +194,13 @@ static int scmi_domain_reset(const struct scmi_protocol_handle *ph, u32 domain,
 	int ret;
 	struct scmi_xfer *t;
 	struct scmi_msg_reset_domain_reset *dom;
-	struct scmi_reset_info *pi = ph->get_priv(ph);
-	struct reset_dom_info *rdom;
+	struct reset_dom_info *dom_info;
 
-	if (domain >= pi->num_domains)
-		return -EINVAL;
+	dom_info = scmi_reset_domain_lookup(ph, domain);
+	if (IS_ERR(dom_info))
+		return PTR_ERR(dom_info);
 
-	rdom = pi->dom_info + domain;
-	if (rdom->async_reset && flags & AUTONOMOUS_RESET)
+	if (dom_info->async_reset && flags & AUTONOMOUS_RESET)
 		flags |= ASYNCHRONOUS_RESET;
 
 	ret = ph->xops->xfer_get_init(ph, RESET, sizeof(*dom), 0, &t);
@@ -238,15 +253,16 @@ static const struct scmi_reset_proto_ops reset_proto_ops = {
 static bool scmi_reset_notify_supported(const struct scmi_protocol_handle *ph,
 					u8 evt_id, u32 src_id)
 {
-	struct reset_dom_info *dom;
-	struct scmi_reset_info *pi = ph->get_priv(ph);
+	struct reset_dom_info *dom_info;
 
-	if (evt_id != SCMI_EVENT_RESET_ISSUED || src_id >= pi->num_domains)
+	if (evt_id != SCMI_EVENT_RESET_ISSUED)
 		return false;
 
-	dom = pi->dom_info + src_id;
+	dom_info = scmi_reset_domain_lookup(ph, src_id);
+	if (IS_ERR(dom_info))
+		return false;
 
-	return dom->reset_notify;
+	return dom_info->reset_notify;
 }
 
 static int scmi_reset_notify(const struct scmi_protocol_handle *ph,
@@ -363,8 +379,11 @@ static int scmi_reset_protocol_init(const struct scmi_protocol_handle *ph)
 	if (!pinfo->dom_info)
 		return -ENOMEM;
 
-	for (domain = 0; domain < pinfo->num_domains; domain++)
-		scmi_reset_domain_attributes_get(ph, pinfo, domain, version);
+	for (domain = 0; domain < pinfo->num_domains; domain++) {
+		struct reset_dom_info *dom_info = pinfo->dom_info + domain;
+
+		scmi_reset_domain_attributes_get(ph, dom_info, pinfo, domain, version);
+	}
 
 	pinfo->version = version;
 	return ph->set_priv(ph, pinfo, version);
-- 
2.43.0

[PATCH v4] firmware: arm_scmi: refactor reset domain handling

[Artem Shimko]

Improve the SCMI reset protocol implementation by centralizing domain
validation and enhancing error handling.

Add scmi_reset_domain_lookup() helper function to validate
domain ids and return appropriate error codes. Refactor all
domain-specific functions to use this helper, ensuring consistent
error handling throughout the reset protocol.

Suggested-by: Cristian Marussi <cristian.marussi@arm.com>
Signed-off-by: Artem Shimko <a.shimko.dev@gmail.com>
---

Hi Cristian,

Thank you for suggesting! If it works for you I'll try to improve the others protocols.

It was tested on my board in QEMU and on real hardware.
The functionality didn't break, and peripherals that are reseted via SCMI work correctly.

As an example, I can provide DMA test results (as one of the processor units dependent on SCMI reset).

[    2.559040] dw_axi_dmac_platform 1000nda0.nda_dma_0: DesignWare AXI DMA Controller, nda channels
[    2.569265] dw_axi_dmac_platform 1000nda0.nda_dma_1: DesignWare AXI DMA Controller, nda channels
[    2.580601] dw_axi_dmac_platform 1000nda0.nda_dma_2: DesignWare AXI DMA Controller, nda channels

# echo 2000 > /sys/module/dmatest/parameters/timeout
# echo 1000 > /sys/module/dmatest/parameters/iterations
# echo dma0chan0 > /sys/module/dmatest/parameters/channel
[   95.917504] dmatest: Added 1 threads using dma0chan0
# echo 1 > /sys/module/dmatest/parameters/run
[  101.931372] dmatest: Started 1 threads using dma0chan0
[  102.731009] dmatest: dma0chan0-copy0: summary 1000 tests, 0 failures 2508.42 iops 19706 KB/s (0)

And vice versa:

1. Confirmed that the DMA module is non-functional without a proper reset via SCMI.
Disabling the reset logic causes a system crash on first access to its registers.

2. Requesting a non-existent reset domain:

[    2.463400] dw_axi_dmac_platform 1000nda0.nda_dma_0: error -EINVAL: Failed to deassert resets
[    2.472091] dw_axi_dmac_platform 1000nda0.nda_dma_0: probe with driver dw_axi_dmac_platform failed with error -22
[    2.482911] dw_axi_dmac_platform 1000nda0.nda_dma_1: error -EINVAL: Failed to deassert resets
[    2.491553] dw_axi_dmac_platform 1000nda0.nda_dma_1: probe with driver dw_axi_dmac_platform failed with error -22
[    2.502256] dw_axi_dmac_platform 1000nda0.nda_dma_2: error -EINVAL: Failed to deassert resets
[    2.510735] dw_axi_dmac_platform 1000nda0.nda_dma_2: probe with driver dw_axi_dmac_platform failed with error -22

--
Regards,
Artem

---
ChangeLog:
  v4: Add Suggested-by tag

 drivers/firmware/arm_scmi/reset.c | 59 ++++++++++++++++++++-----------
 1 file changed, 39 insertions(+), 20 deletions(-)

diff --git a/drivers/firmware/arm_scmi/reset.c b/drivers/firmware/arm_scmi/reset.c
index 0aa82b96f41b..9854a3016d13 100644
--- a/drivers/firmware/arm_scmi/reset.c
+++ b/drivers/firmware/arm_scmi/reset.c
@@ -100,6 +100,7 @@ static int scmi_reset_attributes_get(const struct scmi_protocol_handle *ph,
 
 static int
 scmi_reset_domain_attributes_get(const struct scmi_protocol_handle *ph,
+				 struct reset_dom_info *dom_info,
 				 struct scmi_reset_info *pinfo,
 				 u32 domain, u32 version)
 {
@@ -107,7 +108,6 @@ scmi_reset_domain_attributes_get(const struct scmi_protocol_handle *ph,
 	u32 attributes;
 	struct scmi_xfer *t;
 	struct scmi_msg_resp_reset_domain_attributes *attr;
-	struct reset_dom_info *dom_info = pinfo->dom_info + domain;
 
 	ret = ph->xops->xfer_get_init(ph, RESET_DOMAIN_ATTRIBUTES,
 				      sizeof(domain), sizeof(*attr), &t);
@@ -153,23 +153,39 @@ static int scmi_reset_num_domains_get(const struct scmi_protocol_handle *ph)
 	return pi->num_domains;
 }
 
+static inline struct reset_dom_info *
+scmi_reset_domain_lookup(const struct scmi_protocol_handle *ph, u32 domain)
+{
+	struct scmi_reset_info *pi = ph->get_priv(ph);
+
+	if (domain >= pi->num_domains)
+		return ERR_PTR(-EINVAL);
+
+	return pi->dom_info + domain;
+}
+
 static const char *
 scmi_reset_name_get(const struct scmi_protocol_handle *ph, u32 domain)
 {
-	struct scmi_reset_info *pi = ph->get_priv(ph);
+	struct reset_dom_info *dom_info;
 
-	struct reset_dom_info *dom = pi->dom_info + domain;
+	dom_info = scmi_reset_domain_lookup(ph, domain);
+	if (IS_ERR(dom_info))
+		return "unknown";
 
-	return dom->name;
+	return dom_info->name;
 }
 
 static int scmi_reset_latency_get(const struct scmi_protocol_handle *ph,
 				  u32 domain)
 {
-	struct scmi_reset_info *pi = ph->get_priv(ph);
-	struct reset_dom_info *dom = pi->dom_info + domain;
+	struct reset_dom_info *dom_info;
 
-	return dom->latency_us;
+	dom_info = scmi_reset_domain_lookup(ph, domain);
+	if (IS_ERR(dom_info))
+		return PTR_ERR(dom_info);
+
+	return dom_info->latency_us;
 }
 
 static int scmi_domain_reset(const struct scmi_protocol_handle *ph, u32 domain,
@@ -178,14 +194,13 @@ static int scmi_domain_reset(const struct scmi_protocol_handle *ph, u32 domain,
 	int ret;
 	struct scmi_xfer *t;
 	struct scmi_msg_reset_domain_reset *dom;
-	struct scmi_reset_info *pi = ph->get_priv(ph);
-	struct reset_dom_info *rdom;
+	struct reset_dom_info *dom_info;
 
-	if (domain >= pi->num_domains)
-		return -EINVAL;
+	dom_info = scmi_reset_domain_lookup(ph, domain);
+	if (IS_ERR(dom_info))
+		return PTR_ERR(dom_info);
 
-	rdom = pi->dom_info + domain;
-	if (rdom->async_reset && flags & AUTONOMOUS_RESET)
+	if (dom_info->async_reset && flags & AUTONOMOUS_RESET)
 		flags |= ASYNCHRONOUS_RESET;
 
 	ret = ph->xops->xfer_get_init(ph, RESET, sizeof(*dom), 0, &t);
@@ -238,15 +253,16 @@ static const struct scmi_reset_proto_ops reset_proto_ops = {
 static bool scmi_reset_notify_supported(const struct scmi_protocol_handle *ph,
 					u8 evt_id, u32 src_id)
 {
-	struct reset_dom_info *dom;
-	struct scmi_reset_info *pi = ph->get_priv(ph);
+	struct reset_dom_info *dom_info;
 
-	if (evt_id != SCMI_EVENT_RESET_ISSUED || src_id >= pi->num_domains)
+	if (evt_id != SCMI_EVENT_RESET_ISSUED)
 		return false;
 
-	dom = pi->dom_info + src_id;
+	dom_info = scmi_reset_domain_lookup(ph, src_id);
+	if (IS_ERR(dom_info))
+		return false;
 
-	return dom->reset_notify;
+	return dom_info->reset_notify;
 }
 
 static int scmi_reset_notify(const struct scmi_protocol_handle *ph,
@@ -363,8 +379,11 @@ static int scmi_reset_protocol_init(const struct scmi_protocol_handle *ph)
 	if (!pinfo->dom_info)
 		return -ENOMEM;
 
-	for (domain = 0; domain < pinfo->num_domains; domain++)
-		scmi_reset_domain_attributes_get(ph, pinfo, domain, version);
+	for (domain = 0; domain < pinfo->num_domains; domain++) {
+		struct reset_dom_info *dom_info = pinfo->dom_info + domain;
+
+		scmi_reset_domain_attributes_get(ph, dom_info, pinfo, domain, version);
+	}
 
 	pinfo->version = version;
 	return ph->set_priv(ph, pinfo, version);
-- 
2.43.0

Re: [PATCH v4] firmware: arm_scmi: refactor reset domain handling

[Sudeep Holla]

On Sun, Nov 23, 2025 at 07:35:57PM +0300, Artem Shimko wrote:
> Improve the SCMI reset protocol implementation by centralizing domain
> validation and enhancing error handling.
> 
> Add scmi_reset_domain_lookup() helper function to validate
> domain ids and return appropriate error codes. Refactor all
> domain-specific functions to use this helper, ensuring consistent
> error handling throughout the reset protocol.
> 
> Suggested-by: Cristian Marussi <cristian.marussi@arm.com>
> Signed-off-by: Artem Shimko <a.shimko.dev@gmail.com>
> ---
> 
> Hi Cristian,
> 
> Thank you for suggesting! If it works for you I'll try to improve the others protocols.
> 
> It was tested on my board in QEMU and on real hardware.
> The functionality didn't break, and peripherals that are reseted via SCMI work correctly.
> 
> As an example, I can provide DMA test results (as one of the processor units dependent on SCMI reset).
> 
> [    2.559040] dw_axi_dmac_platform 1000nda0.nda_dma_0: DesignWare AXI DMA Controller, nda channels
> [    2.569265] dw_axi_dmac_platform 1000nda0.nda_dma_1: DesignWare AXI DMA Controller, nda channels
> [    2.580601] dw_axi_dmac_platform 1000nda0.nda_dma_2: DesignWare AXI DMA Controller, nda channels
> 
> # echo 2000 > /sys/module/dmatest/parameters/timeout
> # echo 1000 > /sys/module/dmatest/parameters/iterations
> # echo dma0chan0 > /sys/module/dmatest/parameters/channel
> [   95.917504] dmatest: Added 1 threads using dma0chan0
> # echo 1 > /sys/module/dmatest/parameters/run
> [  101.931372] dmatest: Started 1 threads using dma0chan0
> [  102.731009] dmatest: dma0chan0-copy0: summary 1000 tests, 0 failures 2508.42 iops 19706 KB/s (0)
> 
> And vice versa:
> 
> 1. Confirmed that the DMA module is non-functional without a proper reset via SCMI.
> Disabling the reset logic causes a system crash on first access to its registers.
> 
> 2. Requesting a non-existent reset domain:
> 
> [    2.463400] dw_axi_dmac_platform 1000nda0.nda_dma_0: error -EINVAL: Failed to deassert resets
> [    2.472091] dw_axi_dmac_platform 1000nda0.nda_dma_0: probe with driver dw_axi_dmac_platform failed with error -22
> [    2.482911] dw_axi_dmac_platform 1000nda0.nda_dma_1: error -EINVAL: Failed to deassert resets
> [    2.491553] dw_axi_dmac_platform 1000nda0.nda_dma_1: probe with driver dw_axi_dmac_platform failed with error -22
> [    2.502256] dw_axi_dmac_platform 1000nda0.nda_dma_2: error -EINVAL: Failed to deassert resets
> [    2.510735] dw_axi_dmac_platform 1000nda0.nda_dma_2: probe with driver dw_axi_dmac_platform failed with error -22
> 
> --
> Regards,
> Artem
> 
> ---
> ChangeLog:
>   v4: Add Suggested-by tag
> 
>  drivers/firmware/arm_scmi/reset.c | 59 ++++++++++++++++++++-----------
>  1 file changed, 39 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/firmware/arm_scmi/reset.c b/drivers/firmware/arm_scmi/reset.c
> index 0aa82b96f41b..9854a3016d13 100644
> --- a/drivers/firmware/arm_scmi/reset.c
> +++ b/drivers/firmware/arm_scmi/reset.c
> @@ -100,6 +100,7 @@ static int scmi_reset_attributes_get(const struct scmi_protocol_handle *ph,
>  
>  static int
>  scmi_reset_domain_attributes_get(const struct scmi_protocol_handle *ph,
> +				 struct reset_dom_info *dom_info,

Instead of changing this, ...

>  				 struct scmi_reset_info *pinfo,
>  				 u32 domain, u32 version)
>  {
> @@ -107,7 +108,6 @@ scmi_reset_domain_attributes_get(const struct scmi_protocol_handle *ph,
>  	u32 attributes;
>  	struct scmi_xfer *t;
>  	struct scmi_msg_resp_reset_domain_attributes *attr;
> -	struct reset_dom_info *dom_info = pinfo->dom_info + domain;
>  

... You can just follow the pattern you have done everywhere else in this
change like:

struct reset_dom_info *dom_info; 

dom_info = scmi_reset_domain_lookup(ph, domain);
if (IS_ERR(dom_info))
	return PTR_ERR(dom_info);

Any particular reason for not doing that ?

-- 
Regards,
Sudeep

Re: [PATCH v4] firmware: arm_scmi: refactor reset domain handling

[Artem Shimko]

On Thu, Dec 4, 2025 at 7:26 PM Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> On Sun, Nov 23, 2025 at 07:35:57PM +0300, Artem Shimko wrote:
> > Improve the SCMI reset protocol implementation by centralizing domain
> > validation and enhancing error handling.
> >
> > Add scmi_reset_domain_lookup() helper function to validate
> > domain ids and return appropriate error codes. Refactor all
> > domain-specific functions to use this helper, ensuring consistent
> > error handling throughout the reset protocol.
> >
> > Suggested-by: Cristian Marussi <cristian.marussi@arm.com>
> > Signed-off-by: Artem Shimko <a.shimko.dev@gmail.com>
> > ---
> >
> > Hi Cristian,
> >
> > Thank you for suggesting! If it works for you I'll try to improve the others protocols.
> >
> > It was tested on my board in QEMU and on real hardware.
> > The functionality didn't break, and peripherals that are reseted via SCMI work correctly.
> >
> > As an example, I can provide DMA test results (as one of the processor units dependent on SCMI reset).
> >
> > [    2.559040] dw_axi_dmac_platform 1000nda0.nda_dma_0: DesignWare AXI DMA Controller, nda channels
> > [    2.569265] dw_axi_dmac_platform 1000nda0.nda_dma_1: DesignWare AXI DMA Controller, nda channels
> > [    2.580601] dw_axi_dmac_platform 1000nda0.nda_dma_2: DesignWare AXI DMA Controller, nda channels
> >
> > # echo 2000 > /sys/module/dmatest/parameters/timeout
> > # echo 1000 > /sys/module/dmatest/parameters/iterations
> > # echo dma0chan0 > /sys/module/dmatest/parameters/channel
> > [   95.917504] dmatest: Added 1 threads using dma0chan0
> > # echo 1 > /sys/module/dmatest/parameters/run
> > [  101.931372] dmatest: Started 1 threads using dma0chan0
> > [  102.731009] dmatest: dma0chan0-copy0: summary 1000 tests, 0 failures 2508.42 iops 19706 KB/s (0)
> >
> > And vice versa:
> >
> > 1. Confirmed that the DMA module is non-functional without a proper reset via SCMI.
> > Disabling the reset logic causes a system crash on first access to its registers.
> >
> > 2. Requesting a non-existent reset domain:
> >
> > [    2.463400] dw_axi_dmac_platform 1000nda0.nda_dma_0: error -EINVAL: Failed to deassert resets
> > [    2.472091] dw_axi_dmac_platform 1000nda0.nda_dma_0: probe with driver dw_axi_dmac_platform failed with error -22
> > [    2.482911] dw_axi_dmac_platform 1000nda0.nda_dma_1: error -EINVAL: Failed to deassert resets
> > [    2.491553] dw_axi_dmac_platform 1000nda0.nda_dma_1: probe with driver dw_axi_dmac_platform failed with error -22
> > [    2.502256] dw_axi_dmac_platform 1000nda0.nda_dma_2: error -EINVAL: Failed to deassert resets
> > [    2.510735] dw_axi_dmac_platform 1000nda0.nda_dma_2: probe with driver dw_axi_dmac_platform failed with error -22
> >
> > --
> > Regards,
> > Artem
> >
> > ---
> > ChangeLog:
> >   v4: Add Suggested-by tag
> >
> >  drivers/firmware/arm_scmi/reset.c | 59 ++++++++++++++++++++-----------
> >  1 file changed, 39 insertions(+), 20 deletions(-)
> >
> > diff --git a/drivers/firmware/arm_scmi/reset.c b/drivers/firmware/arm_scmi/reset.c
> > index 0aa82b96f41b..9854a3016d13 100644
> > --- a/drivers/firmware/arm_scmi/reset.c
> > +++ b/drivers/firmware/arm_scmi/reset.c
> > @@ -100,6 +100,7 @@ static int scmi_reset_attributes_get(const struct scmi_protocol_handle *ph,
> >
> >  static int
> >  scmi_reset_domain_attributes_get(const struct scmi_protocol_handle *ph,
> > +                              struct reset_dom_info *dom_info,
>
> Instead of changing this, ...
>
> >                                struct scmi_reset_info *pinfo,
> >                                u32 domain, u32 version)
> >  {
> > @@ -107,7 +108,6 @@ scmi_reset_domain_attributes_get(const struct scmi_protocol_handle *ph,
> >       u32 attributes;
> >       struct scmi_xfer *t;
> >       struct scmi_msg_resp_reset_domain_attributes *attr;
> > -     struct reset_dom_info *dom_info = pinfo->dom_info + domain;
> >
>
> ... You can just follow the pattern you have done everywhere else in this
> change like:
>
> struct reset_dom_info *dom_info;
>
> dom_info = scmi_reset_domain_lookup(ph, domain);
> if (IS_ERR(dom_info))
>         return PTR_ERR(dom_info);
>
> Any particular reason for not doing that ?

Hi Sudeep,

Based on perf.c, I added this pattern only to functions that can be
called externally with an invalid domain value.
Therefore, I decided it would be better to change the signature of
scmi_reset_domain_attributes_get (similar to
scmi_perf_domain_attributes_get from pref.c)
to ensure consistency across drivers when working with domain numbers.

If you believe this pattern should be used in
scmi_reset_domain_attributes_get, I'll send you the next version of
the patch.

Thank you very much for reviewing my changes =)

Regards,
Artem

Re: [PATCH v4] firmware: arm_scmi: refactor reset domain handling

[Sudeep Holla]

On Fri, Dec 05, 2025 at 01:36:23PM +0300, Artem Shimko wrote:
> On Thu, Dec 4, 2025 at 7:26 PM Sudeep Holla <sudeep.holla@arm.com> wrote:
> >
> > On Sun, Nov 23, 2025 at 07:35:57PM +0300, Artem Shimko wrote:
> > > Improve the SCMI reset protocol implementation by centralizing domain
> > > validation and enhancing error handling.
> > >
> > > Add scmi_reset_domain_lookup() helper function to validate
> > > domain ids and return appropriate error codes. Refactor all
> > > domain-specific functions to use this helper, ensuring consistent
> > > error handling throughout the reset protocol.
> > >
> > > Suggested-by: Cristian Marussi <cristian.marussi@arm.com>
> > > Signed-off-by: Artem Shimko <a.shimko.dev@gmail.com>
> > > ---
> > >
> > > Hi Cristian,
> > >
> > > Thank you for suggesting! If it works for you I'll try to improve the others protocols.
> > >
> > > It was tested on my board in QEMU and on real hardware.
> > > The functionality didn't break, and peripherals that are reseted via SCMI work correctly.
> > >
> > > As an example, I can provide DMA test results (as one of the processor units dependent on SCMI reset).
> > >
> > > [    2.559040] dw_axi_dmac_platform 1000nda0.nda_dma_0: DesignWare AXI DMA Controller, nda channels
> > > [    2.569265] dw_axi_dmac_platform 1000nda0.nda_dma_1: DesignWare AXI DMA Controller, nda channels
> > > [    2.580601] dw_axi_dmac_platform 1000nda0.nda_dma_2: DesignWare AXI DMA Controller, nda channels
> > >
> > > # echo 2000 > /sys/module/dmatest/parameters/timeout
> > > # echo 1000 > /sys/module/dmatest/parameters/iterations
> > > # echo dma0chan0 > /sys/module/dmatest/parameters/channel
> > > [   95.917504] dmatest: Added 1 threads using dma0chan0
> > > # echo 1 > /sys/module/dmatest/parameters/run
> > > [  101.931372] dmatest: Started 1 threads using dma0chan0
> > > [  102.731009] dmatest: dma0chan0-copy0: summary 1000 tests, 0 failures 2508.42 iops 19706 KB/s (0)
> > >
> > > And vice versa:
> > >
> > > 1. Confirmed that the DMA module is non-functional without a proper reset via SCMI.
> > > Disabling the reset logic causes a system crash on first access to its registers.
> > >
> > > 2. Requesting a non-existent reset domain:
> > >
> > > [    2.463400] dw_axi_dmac_platform 1000nda0.nda_dma_0: error -EINVAL: Failed to deassert resets
> > > [    2.472091] dw_axi_dmac_platform 1000nda0.nda_dma_0: probe with driver dw_axi_dmac_platform failed with error -22
> > > [    2.482911] dw_axi_dmac_platform 1000nda0.nda_dma_1: error -EINVAL: Failed to deassert resets
> > > [    2.491553] dw_axi_dmac_platform 1000nda0.nda_dma_1: probe with driver dw_axi_dmac_platform failed with error -22
> > > [    2.502256] dw_axi_dmac_platform 1000nda0.nda_dma_2: error -EINVAL: Failed to deassert resets
> > > [    2.510735] dw_axi_dmac_platform 1000nda0.nda_dma_2: probe with driver dw_axi_dmac_platform failed with error -22
> > >
> > > --
> > > Regards,
> > > Artem
> > >
> > > ---
> > > ChangeLog:
> > >   v4: Add Suggested-by tag
> > >
> > >  drivers/firmware/arm_scmi/reset.c | 59 ++++++++++++++++++++-----------
> > >  1 file changed, 39 insertions(+), 20 deletions(-)
> > >
> > > diff --git a/drivers/firmware/arm_scmi/reset.c b/drivers/firmware/arm_scmi/reset.c
> > > index 0aa82b96f41b..9854a3016d13 100644
> > > --- a/drivers/firmware/arm_scmi/reset.c
> > > +++ b/drivers/firmware/arm_scmi/reset.c
> > > @@ -100,6 +100,7 @@ static int scmi_reset_attributes_get(const struct scmi_protocol_handle *ph,
> > >
> > >  static int
> > >  scmi_reset_domain_attributes_get(const struct scmi_protocol_handle *ph,
> > > +                              struct reset_dom_info *dom_info,
> >
> > Instead of changing this, ...
> >
> > >                                struct scmi_reset_info *pinfo,
> > >                                u32 domain, u32 version)
> > >  {
> > > @@ -107,7 +108,6 @@ scmi_reset_domain_attributes_get(const struct scmi_protocol_handle *ph,
> > >       u32 attributes;
> > >       struct scmi_xfer *t;
> > >       struct scmi_msg_resp_reset_domain_attributes *attr;
> > > -     struct reset_dom_info *dom_info = pinfo->dom_info + domain;
> > >
> >
> > ... You can just follow the pattern you have done everywhere else in this
> > change like:
> >
> > struct reset_dom_info *dom_info;
> >
> > dom_info = scmi_reset_domain_lookup(ph, domain);
> > if (IS_ERR(dom_info))
> >         return PTR_ERR(dom_info);
> >
> > Any particular reason for not doing that ?
> 
> Hi Sudeep,
> 
> Based on perf.c, I added this pattern only to functions that can be
> called externally with an invalid domain value.
> Therefore, I decided it would be better to change the signature of
> scmi_reset_domain_attributes_get (similar to
> scmi_perf_domain_attributes_get from pref.c)
> to ensure consistency across drivers when working with domain numbers.
> 
> If you believe this pattern should be used in
> scmi_reset_domain_attributes_get, I'll send you the next version of
> the patch.
>

No need to post yet, I fixed it up here[1], let's see if Cristian agrees
with that. Let me know if you are happy too with that change. I will rebase
at -rc1 and apply it officially then.

-- 
Regards,
Sudeep

[1] https://git.kernel.org/sudeep.holla/c/8bc55f21a345

Re: [PATCH v4] firmware: arm_scmi: refactor reset domain handling

[Artem Shimko]

On Fri, Dec 5, 2025 at 3:14 PM Sudeep Holla <sudeep.holla@arm.com> wrote:

> Let me know if you are happy too with that change.
Yes, sure, I'm happy with the change. Thank you!

--
Regards,
Artem