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@xxxxxxxxx> 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@xxxxxxxxx> 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@xxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx
> > > > Signed-off-by: Sailesh Nandanavanam <saileshnandanavanam@xxxxxxxxx>
> > > > ---
> > >
> > > [...]
> > >
> > > > -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