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

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=E2=80=AFAM Sailesh Nandanavanam
<[email protected]> 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=E2=80=AFPM Konrad Dybcio
> <[email protected]> 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 a=
nd active regulators.")
> > > Cc: [email protected]
> > > Signed-off-by: Sailesh Nandanavanam <[email protected]>
> > > ---
> >
> > [...]
> >
> > > -err:
> > > -     for (; i >=3D 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