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 <[email protected]> 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 > <[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 PM 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 and > > > > active regulators.") > > > > Cc: [email protected] > > > > Signed-off-by: Sailesh Nandanavanam <[email protected]> > > > > --- > > > > > > [...] > > > > > > > -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
