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

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

[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: stable@vger.kernel.org
Signed-off-by: Sailesh Nandanavanam <saileshnandanavanam@gmail.com>
---
 drivers/remoteproc/qcom_q6v5_mss.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/drivers/remoteproc/qcom_q6v5_mss.c b/drivers/remoteproc/qcom_q6v5_mss.c
index ae78f5c7c1b6..9a17aa065f50 100644
--- a/drivers/remoteproc/qcom_q6v5_mss.c
+++ b/drivers/remoteproc/qcom_q6v5_mss.c
@@ -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

Re: [PATCH] remoteproc: qcom_q6v5_mss: Fix off-by-one error in regulator error cleanup

[Konrad Dybcio]

On 7/10/26 9:46 PM, Sailesh Nandanavanam wrote:
> 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: stable@vger.kernel.org
> Signed-off-by: Sailesh Nandanavanam <saileshnandanavanam@gmail.com>
> ---

[...]

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

Re: [PATCH] remoteproc: qcom_q6v5_mss: Fix off-by-one error in regulator error cleanup

[Sailesh Nandanavanam]

On 7/17/26 3:01 PM, Konrad Dybcio wrote:
> 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


On Fri, Jul 17, 2026 at 3:01 PM Konrad Dybcio
<konrad.dybcio@oss.qualcomm.com> wrote:
>
> On 7/10/26 9:46 PM, Sailesh Nandanavanam wrote:
> > 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: stable@vger.kernel.org
> > Signed-off-by: Sailesh Nandanavanam <saileshnandanavanam@gmail.com>
> > ---
>
> [...]
>
> > -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

Re: [PATCH] remoteproc: qcom_q6v5_mss: Fix off-by-one error in regulator error cleanup

[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

On Sat, Jul 18, 2026 at 1:27 AM Sailesh Nandanavanam
<saileshnandanavanam@gmail.com> wrote:
>
> On 7/17/26 3:01 PM, Konrad Dybcio wrote:
> > 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
>
>
> On Fri, Jul 17, 2026 at 3:01 PM Konrad Dybcio
> <konrad.dybcio@oss.qualcomm.com> wrote:
> >
> > On 7/10/26 9:46 PM, Sailesh Nandanavanam wrote:
> > > 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: stable@vger.kernel.org
> > > Signed-off-by: Sailesh Nandanavanam <saileshnandanavanam@gmail.com>
> > > ---
> >
> > [...]
> >
> > > -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

Re: [PATCH] remoteproc: qcom_q6v5_mss: Fix off-by-one error in regulator error cleanup

[Sailesh Nandanavanam]

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

On Sun, Aug 2, 2026 at 3:12 PM Sailesh Nandanavanam
<saileshnandanavanam@gmail.com> wrote:
>
> 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
>
> On Sat, Jul 18, 2026 at 1:27 AM Sailesh Nandanavanam
> <saileshnandanavanam@gmail.com> wrote:
> >
> > On 7/17/26 3:01 PM, Konrad Dybcio wrote:
> > > 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
> >
> >
> > On Fri, Jul 17, 2026 at 3:01 PM Konrad Dybcio
> > <konrad.dybcio@oss.qualcomm.com> wrote:
> > >
> > > On 7/10/26 9:46 PM, Sailesh Nandanavanam wrote:
> > > > 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: stable@vger.kernel.org
> > > > Signed-off-by: Sailesh Nandanavanam <saileshnandanavanam@gmail.com>
> > > > ---
> > >
> > > [...]
> > >
> > > > -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

Re: [PATCH] remoteproc: qcom_q6v5_mss: Fix off-by-one error in regulator error cleanup

[Dmitry Baryshkov]

On Fri, Jul 17, 2026 at 11:31:02AM +0200, Konrad Dybcio wrote:
> On 7/10/26 9:46 PM, Sailesh Nandanavanam wrote:
> > 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: stable@vger.kernel.org
> > Signed-off-by: Sailesh Nandanavanam <saileshnandanavanam@gmail.com>
> > ---
> 
> [...]
> 
> > -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.

> 
> Konrad

-- 
With best wishes
Dmitry

Re: [PATCH] remoteproc: qcom_q6v5_mss: Fix off-by-one error in regulator error cleanup

[Dmitry Baryshkov]

On Sat, Jul 11, 2026 at 01:16:38AM +0530, Sailesh Nandanavanam wrote:
> 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: stable@vger.kernel.org
> Signed-off-by: Sailesh Nandanavanam <saileshnandanavanam@gmail.com>
> ---
>  drivers/remoteproc/qcom_q6v5_mss.c | 16 +++++++++++-----
>  1 file changed, 11 insertions(+), 5 deletions(-)
> 

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>


-- 
With best wishes
Dmitry

Re: [PATCH] remoteproc: qcom_q6v5_mss: Fix off-by-one error in regulator error cleanup

[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


On Sat, Jul 11, 2026 at 1:17 AM Sailesh Nandanavanam
<saileshnandanavanam@gmail.com> wrote:
>
> 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: stable@vger.kernel.org
> Signed-off-by: Sailesh Nandanavanam <saileshnandanavanam@gmail.com>
> ---
>  drivers/remoteproc/qcom_q6v5_mss.c | 16 +++++++++++-----
>  1 file changed, 11 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/remoteproc/qcom_q6v5_mss.c b/drivers/remoteproc/qcom_q6v5_mss.c
> index ae78f5c7c1b6..9a17aa065f50 100644
> --- a/drivers/remoteproc/qcom_q6v5_mss.c
> +++ b/drivers/remoteproc/qcom_q6v5_mss.c
> @@ -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
>

Re: [PATCH] remoteproc: qcom_q6v5_mss: Fix off-by-one error in regulator error cleanup

[Anup Vishwakarma]

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