remoteproc: qcom_q6v5_mss: Fix off-by-one error in regulator error cleanup

PATCH In review Sailesh Nandanavanam 4 participants 9 messages Jul 10 – Sep 21

Subsystems (3)

Stable branch Arm Qualcomm mailing list Remote processor (remoteproc)

lore

Share & tools

lore

Sailesh Nandanavanam

author

2026-07-10 19:47 UTC #0 lore raw

PATCH 1/1 remoteproc: qcom_q6v5_mss: Fix off-by-one error in regulator error cleanup

In q6v5_regulator_enable(), when any operation fails for regulator at index 'i', the error cleanup path unconditionally calls regulator_disable() starting from index 'i'. However, regulator 'i' was never successfully enabled at this point, resulting in an unbalanced disable.

There are three distinct failure points:
- regulator_set_voltage() failure: voltage was never set, load was never set, regulator was never enabled.
- regulator_set_load() failure: voltage was set, but regulator was never enabled.
- regulator_enable() failure: voltage and load were set, but regulator was never enabled.

Fix this by introducing three separate error labels to handle each failure point correctly. For the failing regulator at index 'i', only reset the resources that were actually configured, without calling regulator_disable(). Then roll back all previously enabled regulators using 'i--' in the for loop initializer to skip the never-enabled regulator.

Fixes 19f902b53b47 ("remoteproc: qcom: Initialize and enable proxy and active regulators.")

Cc [email protected]

Signed-off-by Sailesh Nandanavanam <[email protected]>

---
 drivers/remoteproc/qcom_q6v5_mss.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

drivers/remoteproc/qcom_q6v5_mss.c +11 -5

index ae78f5c7c1b6..9a17aa065f50 100644
@@ -311,7 +311,7 @@ static int q6v5_regulator_enable(struct q6v5 *qproc,
 				dev_err(qproc->dev,
 					"Failed to request voltage for %d.\n",
 						i);
-				goto err;
+				goto err_set_voltage;
 			}
 		}

@@ -321,20 +321,26 @@ static int q6v5_regulator_enable(struct q6v5 *qproc,
 			if (ret < 0) {
 				dev_err(qproc->dev,
 					"Failed to set regulator mode\n");
-				goto err;
+				goto err_set_load;
 			}
 		}
 
 		ret = regulator_enable(regs[i].reg);
 		if (ret) {
 			dev_err(qproc->dev, "Regulator enable failed\n");
-			goto err;
+			goto err_enable;
 		}
 	}
 
 	return 0;
-err:
-	for (; i >= 0; i--) {
+err_enable:
+	if (regs[i].uA > 0)
+		regulator_set_load(regs[i].reg, 0);
+err_set_load:
+	if (regs[i].uV > 0)
+		regulator_set_voltage(regs[i].reg, 0, INT_MAX);
+err_set_voltage:
+	for (i--; i >= 0; i--) {
 		if (regs[i].uV > 0)
 			regulator_set_voltage(regs[i].reg, 0, INT_MAX);

Signature · 1 line

2.34.1

Konrad Dybcio

2026-07-17 09:31 UTC #1 lore raw

Quoted 25 lines · Sailesh Nandanavanam

In q6v5_regulator_enable(), when any operation fails for regulator at index 'i', the error cleanup path unconditionally calls regulator_disable() starting from index 'i'. However, regulator 'i' was never successfully enabled at this point, resulting in an unbalanced disable.
There are three distinct failure points:
- regulator_set_voltage() failure: voltage was never set, load was never set, regulator was never enabled.
- regulator_set_load() failure: voltage was set, but regulator was never enabled.
- regulator_enable() failure: voltage and load were set, but regulator was never enabled.
Fix this by introducing three separate error labels to handle each failure point correctly. For the failing regulator at index 'i', only reset the resources that were actually configured, without calling regulator_disable(). Then roll back all previously enabled regulators using 'i--' in the for loop initializer to skip the never-enabled regulator.
Fixes 19f902b53b47 ("remoteproc: qcom: Initialize and enable proxy and active regulators.")
Cc [email protected]
Signed-off-by Sailesh Nandanavanam <[email protected]>
---

[...]

drivers/remoteproc/qcom_q6v5_mss.c static int q6v5_regulator_enable(struct q6v5 *qproc, · line 341

Quoted 8 lines

drivers/remoteproc/qcom_q6v5_mss.c +6 -2

-err:
-	for (; i >= 0; i--) {
+err_enable:
+	if (regs[i].uA > 0)
+		regulator_set_load(regs[i].reg, 0);
+err_set_load:
+	if (regs[i].uV > 0)
+		regulator_set_voltage(regs[i].reg, 0, INT_MAX);

The first two labels only unwind a single regulator

Konrad

Sailesh Nandanavanam

author

2026-07-17 19:57 UTC #2 lore raw

Quoted 1 line · Konrad Dybcio

The first two labels only unwind a single regulator

Thanks for taking a look. Could you clarify whether this is a correctness concern (e.g. the fallthrough from err_enable/err_set_load into err_set_voltage not doing what you'd expect), or more a structural/style preference (e.g. avoiding three chained labels in favor of a different approach)? Happy to send a v2 once I understand what you'd like changed.

Thanks,
Sailesh

Quoted 42 lines · Konrad Dybcio

Quoted 25 lines · Sailesh Nandanavanam
In q6v5_regulator_enable(), when any operation fails for regulator at index 'i', the error cleanup path unconditionally calls regulator_disable() starting from index 'i'. However, regulator 'i' was never successfully enabled at this point, resulting in an unbalanced disable.
There are three distinct failure points:
- regulator_set_voltage() failure: voltage was never set, load was never set, regulator was never enabled.
- regulator_set_load() failure: voltage was set, but regulator was never enabled.
- regulator_enable() failure: voltage and load were set, but regulator was never enabled.
Fix this by introducing three separate error labels to handle each failure point correctly. For the failing regulator at index 'i', only reset the resources that were actually configured, without calling regulator_disable(). Then roll back all previously enabled regulators using 'i--' in the for loop initializer to skip the never-enabled regulator.
Fixes 19f902b53b47 ("remoteproc: qcom: Initialize and enable proxy and active regulators.")
Cc [email protected]
Signed-off-by Sailesh Nandanavanam <[email protected]>
---
[...]
drivers/remoteproc/qcom_q6v5_mss.c static int q6v5_regulator_enable(struct q6v5 *qproc, · line 341
Quoted 8 lines
drivers/remoteproc/qcom_q6v5_mss.c +6 -2
-err:
-     for (; i >= 0; i--) {
+err_enable:
+     if (regs[i].uA > 0)
+             regulator_set_load(regs[i].reg, 0);
+err_set_load:
+     if (regs[i].uV > 0)
+             regulator_set_voltage(regs[i].reg, 0, INT_MAX);
The first two labels only unwind a single regulator
Konrad

Sailesh Nandanavanam

author

2026-08-02 09:42 UTC #3 lore raw

Hi Konrad,

Just following up on this - wanted to check if you had a chance to look at my question above. Happy to send a v2 as soon as I know whether the concern is about correctness or the label structure.

Thanks,
Sailesh Nandanavanam

Quoted 59 lines · Sailesh Nandanavanam

Quoted 1 line · Konrad Dybcio
The first two labels only unwind a single regulator
Thanks for taking a look. Could you clarify whether this is a correctness concern (e.g. the fallthrough from err_enable/err_set_load into err_set_voltage not doing what you'd expect), or more a structural/style preference (e.g. avoiding three chained labels in favor of a different approach)? Happy to send a v2 once I understand what you'd like changed.
Thanks,
Sailesh
Quoted 42 lines · Konrad Dybcio
Quoted 25 lines · Sailesh Nandanavanam
In q6v5_regulator_enable(), when any operation fails for regulator at index 'i', the error cleanup path unconditionally calls regulator_disable() starting from index 'i'. However, regulator 'i' was never successfully enabled at this point, resulting in an unbalanced disable.
There are three distinct failure points:
- regulator_set_voltage() failure: voltage was never set, load was never set, regulator was never enabled.
- regulator_set_load() failure: voltage was set, but regulator was never enabled.
- regulator_enable() failure: voltage and load were set, but regulator was never enabled.
Fix this by introducing three separate error labels to handle each failure point correctly. For the failing regulator at index 'i', only reset the resources that were actually configured, without calling regulator_disable(). Then roll back all previously enabled regulators using 'i--' in the for loop initializer to skip the never-enabled regulator.
Fixes 19f902b53b47 ("remoteproc: qcom: Initialize and enable proxy and active regulators.")
Cc [email protected]
Signed-off-by Sailesh Nandanavanam <[email protected]>
---
[...]
drivers/remoteproc/qcom_q6v5_mss.c static int q6v5_regulator_enable(struct q6v5 *qproc, · line 341
Quoted 8 lines
drivers/remoteproc/qcom_q6v5_mss.c +6 -2
-err:
-     for (; i >= 0; i--) {
+err_enable:
+     if (regs[i].uA > 0)
+             regulator_set_load(regs[i].reg, 0);
+err_set_load:
+     if (regs[i].uV > 0)
+             regulator_set_voltage(regs[i].reg, 0, INT_MAX);
The first two labels only unwind a single regulator
Konrad

Sailesh Nandanavanam

author

2026-08-17 18:08 UTC #4 lore raw

Hi Konrad,

Following up once more — still hoping to understand whether your comment was a correctness concern (the err_enable/err_set_load fallthrough into err_set_voltage) or a preference on the label structure itself, so I can send a v2 that addresses it correctly.

Happy to rework this either way once I know which direction you'd prefer.

Thanks,
Sailesh

Quoted 71 lines · Sailesh Nandanavanam

Hi Konrad,
Just following up on this - wanted to check if you had a chance to look at my question above. Happy to send a v2 as soon as I know whether the concern is about correctness or the label structure.
Thanks,
Sailesh Nandanavanam
Quoted 59 lines · Sailesh Nandanavanam
Quoted 1 line · Konrad Dybcio
The first two labels only unwind a single regulator
Thanks for taking a look. Could you clarify whether this is a correctness concern (e.g. the fallthrough from err_enable/err_set_load into err_set_voltage not doing what you'd expect), or more a structural/style preference (e.g. avoiding three chained labels in favor of a different approach)? Happy to send a v2 once I understand what you'd like changed.
Thanks,
Sailesh
Quoted 42 lines · Konrad Dybcio
Quoted 25 lines · Sailesh Nandanavanam
In q6v5_regulator_enable(), when any operation fails for regulator at index 'i', the error cleanup path unconditionally calls regulator_disable() starting from index 'i'. However, regulator 'i' was never successfully enabled at this point, resulting in an unbalanced disable.
There are three distinct failure points:
- regulator_set_voltage() failure: voltage was never set, load was never set, regulator was never enabled.
- regulator_set_load() failure: voltage was set, but regulator was never enabled.
- regulator_enable() failure: voltage and load were set, but regulator was never enabled.
Fix this by introducing three separate error labels to handle each failure point correctly. For the failing regulator at index 'i', only reset the resources that were actually configured, without calling regulator_disable(). Then roll back all previously enabled regulators using 'i--' in the for loop initializer to skip the never-enabled regulator.
Fixes 19f902b53b47 ("remoteproc: qcom: Initialize and enable proxy and active regulators.")
Cc [email protected]
Signed-off-by Sailesh Nandanavanam <[email protected]>
---
[...]
drivers/remoteproc/qcom_q6v5_mss.c static int q6v5_regulator_enable(struct q6v5 *qproc, · line 341
Quoted 8 lines
drivers/remoteproc/qcom_q6v5_mss.c +6 -2
-err:
-     for (; i >= 0; i--) {
+err_enable:
+     if (regs[i].uA > 0)
+             regulator_set_load(regs[i].reg, 0);
+err_set_load:
+     if (regs[i].uV > 0)
+             regulator_set_voltage(regs[i].reg, 0, INT_MAX);
The first two labels only unwind a single regulator
Konrad

Dmitry Baryshkov

2026-08-18 05:06 UTC #5 lore raw

In reply to Konrad Dybcio

Quoted 29 lines · Konrad Dybcio

Quoted 25 lines · Sailesh Nandanavanam
In q6v5_regulator_enable(), when any operation fails for regulator at index 'i', the error cleanup path unconditionally calls regulator_disable() starting from index 'i'. However, regulator 'i' was never successfully enabled at this point, resulting in an unbalanced disable.
There are three distinct failure points:
- regulator_set_voltage() failure: voltage was never set, load was never set, regulator was never enabled.
- regulator_set_load() failure: voltage was set, but regulator was never enabled.
- regulator_enable() failure: voltage and load were set, but regulator was never enabled.
Fix this by introducing three separate error labels to handle each failure point correctly. For the failing regulator at index 'i', only reset the resources that were actually configured, without calling regulator_disable(). Then roll back all previously enabled regulators using 'i--' in the for loop initializer to skip the never-enabled regulator.
Fixes 19f902b53b47 ("remoteproc: qcom: Initialize and enable proxy and active regulators.")
Cc [email protected]
Signed-off-by Sailesh Nandanavanam <[email protected]>
---
[...]

Quoted 10 lines

drivers/remoteproc/qcom_q6v5_mss.c static int q6v5_regulator_enable(struct q6v5 *qproc, · line 341
Quoted 8 lines
drivers/remoteproc/qcom_q6v5_mss.c +6 -2
-err:
-	for (; i >= 0; i--) {
+err_enable:
+	if (regs[i].uA > 0)
+		regulator_set_load(regs[i].reg, 0);
+err_set_load:
+	if (regs[i].uV > 0)
+		regulator_set_voltage(regs[i].reg, 0, INT_MAX);
The first two labels only unwind a single regulator

It is expected, the rest are unwound in the loop.

Quoted 2 lines

Konrad

Signature · 2 lines

With best wishes
Dmitry

Dmitry Baryshkov

Reviewed-by on v1 1/1

2026-08-18 05:07 UTC #6 lore raw

In reply to Sailesh Nandanavanam

Quoted 21 lines · Sailesh Nandanavanam

In q6v5_regulator_enable(), when any operation fails for regulator at index 'i', the error cleanup path unconditionally calls regulator_disable() starting from index 'i'. However, regulator 'i' was never successfully enabled at this point, resulting in an unbalanced disable.
There are three distinct failure points:
- regulator_set_voltage() failure: voltage was never set, load was never set, regulator was never enabled.
- regulator_set_load() failure: voltage was set, but regulator was never enabled.
- regulator_enable() failure: voltage and load were set, but regulator was never enabled.
Fix this by introducing three separate error labels to handle each failure point correctly. For the failing regulator at index 'i', only reset the resources that were actually configured, without calling regulator_disable(). Then roll back all previously enabled regulators using 'i--' in the for loop initializer to skip the never-enabled regulator.

Quoted 7 lines

Fixes 19f902b53b47 ("remoteproc: qcom: Initialize and enable proxy and active regulators.")
Cc [email protected]
Signed-off-by Sailesh Nandanavanam <[email protected]>
---
 drivers/remoteproc/qcom_q6v5_mss.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

Reviewed-by Dmitry Baryshkov <[email protected]>

Signature · 2 lines

With best wishes
Dmitry

Sailesh Nandanavanam

author

2026-09-21 11:18 UTC #7 lore raw

In reply to Sailesh Nandanavanam

Hi Bjorn, Mathieu,

Gentle ping on this patch. It has Dmitry's Reviewed-by from Aug 18, and Konrad's question in the thread was resolved.

I noticed Anup Vishwakarma posted a patch on Sep 16 for the same off-by-one in q6v5_regulator_enable(). Mine was posted earlier (Jul 10), but I'm fine with either being taken. Please let me know if anything else is needed from my side.

Thanks,
Sailesh

Quoted 76 lines · Sailesh Nandanavanam

Fixes 19f902b53b47 ("remoteproc: qcom: Initialize and enable proxy and active regulators.")

Cc [email protected]

Signed-off-by Sailesh Nandanavanam <[email protected]>

---
 drivers/remoteproc/qcom_q6v5_mss.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

drivers/remoteproc/qcom_q6v5_mss.c +11 -6

index ae78f5c7c1b6..9a17aa065f50 100644
@@ -311,7 +311,7 @@ static int q6v5_regulator_enable(struct q6v5 *qproc,
                                dev_err(qproc->dev,
                                        "Failed to request voltage for %d.\n",
                                                i);
-                               goto err;
+                               goto err_set_voltage;
                        }
                }

@@ -321,20 +321,26 @@ static int q6v5_regulator_enable(struct q6v5 *qproc,
                        if (ret < 0) {
                                dev_err(qproc->dev,
                                        "Failed to set regulator mode\n");
-                               goto err;
+                               goto err_set_load;
                        }
                }
 
                ret = regulator_enable(regs[i].reg);
                if (ret) {
                        dev_err(qproc->dev, "Regulator enable failed\n");
-                       goto err;
+                       goto err_enable;
                }
        }
 
        return 0;
-err:
-       for (; i >= 0; i--) {
+err_enable:
+       if (regs[i].uA > 0)
+               regulator_set_load(regs[i].reg, 0);
+err_set_load:
+       if (regs[i].uV > 0)
+               regulator_set_voltage(regs[i].reg, 0, INT_MAX);
+err_set_voltage:
+       for (i--; i >= 0; i--) {
                if (regs[i].uV > 0)
                        regulator_set_voltage(regs[i].reg, 0, INT_MAX);
 
--

2.34.1

Anup Vishwakarma

2026-09-21 17:16 UTC #8 lore raw

Hi Sailesh, Bjorn, Mathieu,

Thanks for the ping, Sailesh. I was not aware of your earlier patch when I submitted mine. Please take Sailesh's patch - it was posted first and has more reviews.

Regards,
Anup Vishwakarma