On Fri, 04 Sep 2026 21:24:25 +0200 Wadim Mueller <[email protected]> wrote:
> slf3s_resume() enables vdd and then restarts the measurement. When the > restart fails it returns an error with the supply still on, and > slf3s_suspend() bails out before regulator_disable() when the stop > command fails. Both paths leave the driver's idea of the power state and > the regulator use count out of sync. > > The PM core only runs the resume callback when the preceding suspend > callback returned 0 (device_suspend() sets power.is_suspended on success > only, and device_resume() bails out when it is clear), so the enable > count cannot climb above one. The damage is different: > > - After a failed restart the sensor stays powered but idle. Every > following suspend then fails too, because the idle part NACKs the > stop command and slf3s_suspend() returns early - the system can no > longer suspend at all until the driver is rebound. That resume errors don't stop the state transition is rather annoying and leads to all this complexity being needed. Given how few drivers go to this level of complexity I suspect it is mostly luck if any recover from a failure in these callbacks. All bets are pretty much off if your power supplies are returning errors so I guess that is kind of fair enough! > > - If regulator_enable() itself fails during resume, the next suspend is > free to run. Should the stop command succeed there, the driver calls > regulator_disable() on a supply it never enabled, which trips > "unbalanced disables" in the regulator core and aborts the system > suspend with -EIO. The devm cleanup has the same problem on unbind. > > Track the supply state in the driver and drive regulator_enable() and > regulator_disable() from that state only. A stop command that fails no > longer keeps the supply on: it is cut right afterwards anyway. > > Reported-by: Li Youhong <[email protected]> Given Li Youhong has been proposing patches for this I'd definitely like their input on this one. > Closes: > https://lore.kernel.org/linux-iio/[email protected]/ > Fixes: d240b0b8a1ce ("iio: flow: add Sensirion SLF3S liquid flow sensor > driver") > Signed-off-by: Wadim Mueller <[email protected]> > --- > drivers/iio/flow/slf3s.c | 35 +++++++++++++++++++++++++++++------ > 1 file changed, 29 insertions(+), 6 deletions(-) > > diff --git a/drivers/iio/flow/slf3s.c b/drivers/iio/flow/slf3s.c > index dfa7c1409045..37f40685e9d9 100644 > --- a/drivers/iio/flow/slf3s.c > +++ b/drivers/iio/flow/slf3s.c > @@ -110,6 +110,7 @@ static const struct slf3s_variant slf3s_variants[] = { > * @vdd: supply regulator, disabled while suspended > * @variant: pointer into @slf3s_variants for the detected device > * @medium: currently active calibration medium > + * @vdd_on: tracks whether @vdd is currently enabled by this driver > * @lock: serialises the multi-step command/response exchanges > * @crc_table: pre-computed CRC-8 lookup table for SLF3S_CRC8_POLY > */ > @@ -118,6 +119,7 @@ struct slf3s_data { > struct regulator *vdd; > const struct slf3s_variant *variant; > enum slf3s_medium medium; > + bool vdd_on; > struct mutex lock; > u8 crc_table[CRC8_TABLE_SIZE]; > }; > @@ -382,6 +384,12 @@ static void slf3s_disable_vdd(void *data) > { > struct slf3s_data *sf = data; > > + guard(mutex)(&sf->lock); > + > + if (!sf->vdd_on) > + return; > + > + sf->vdd_on = false; > regulator_disable(sf->vdd); > } > > @@ -416,6 +424,8 @@ static int slf3s_probe(struct i2c_client *client) > if (ret) > return dev_err_probe(dev, ret, "failed to enable vdd supply\n"); > > + sf->vdd_on = true; > + > ret = devm_add_action_or_reset(dev, slf3s_disable_vdd, sf); > if (ret) > return ret; > @@ -453,10 +463,8 @@ static int slf3s_probe(struct i2c_client *client) > } > > /* > - * The sensor has no low-power state of its own, so stop the measurement > - * and cut the supply while suspended. Resume powers it back up, waits > - * out the power-up time and restarts with the medium that was active > - * before. > + * The sensor has no low-power state, so stop measuring and cut the supply. > + * Resume powers it up again and restarts the previous medium. > */ > static int slf3s_suspend(struct device *dev) > { > @@ -466,9 +474,16 @@ static int slf3s_suspend(struct device *dev) > > guard(mutex)(&sf->lock); > > + /* A failed resume may have left the supply off, nothing to do then. */ > + if (!sf->vdd_on) > + return 0; > + > + /* The supply goes away below anyway, so a failed stop is not fatal. */ Not true. This consumer of the power supply says I don't need it any more so maybe if no one else is using it and the power supply even supports being controlled will the power turn off. So this remains an error that should be reported. > ret = slf3s_send_cmd(sf->client, slf3s_cmd_stop_meas); > if (ret) > - return ret; > + dev_warn(dev, "failed to stop measurement: %d\n", ret); > + > + sf->vdd_on = false; > > return regulator_disable(sf->vdd); Sashiko calls out that we have no idea if this succeeds. So vdd_on may end up out of sync. > } > @@ -485,9 +500,17 @@ static int slf3s_resume(struct device *dev) > if (ret) > return ret; > > + sf->vdd_on = true; > + > fsleep(SLF3S_POWER_UP_DELAY_US); > > - return slf3s_start_meas(sf, sf->medium); > + ret = slf3s_start_meas(sf, sf->medium); > + if (ret) { > + sf->vdd_on = false; > + regulator_disable(sf->vdd); This could also leave us out of sync if that disable fails. > + } > + > + return ret; > } > > static DEFINE_SIMPLE_DEV_PM_OPS(slf3s_pm_ops, slf3s_suspend, slf3s_resume); >
