* [PATCH 0/2] iio: flow: slf3s: fix and test the vdd supply balance
@ 2026-09-04 19:24 Wadim Mueller
2026-09-04 19:24 ` [PATCH 1/2] iio: flow: slf3s: keep the vdd supply balanced across suspend/resume Wadim Mueller
2026-09-04 19:24 ` [PATCH 2/2] iio: flow: slf3s: add KUnit tests for the PM regulator balance Wadim Mueller
0 siblings, 2 replies; 5+ messages in thread
From: Wadim Mueller @ 2026-09-04 19:24 UTC (permalink / raw)
To: Maxwell Doose, Jonathan Cameron, David Lechner, Nuno Sá,
Andy Shevchenko
Cc: Jonathan Cameron, linux-iio, linux-kernel, Li Youhong, Li Youhong,
Brendan Higgins, David Gow, Rae Moar, linux-kselftest, kunit-dev,
Wadim Mueller
Li Youhong reported that slf3s_resume() leaves vdd enabled when the
measurement restart fails [1]. The report is right, but the patch and
my own Reviewed-by on it were both too narrow.
Andy asked whether the regulator refcount can go sideways here. It
cannot. device_suspend() sets power.is_suspended only when the callback
returned 0, and device_resume() returns before the callback when the
flag is clear. So the two alternate and the count stays in {0, 1}.
Please drop my Reviewed-by, the reasoning in it is wrong.
Three things do go wrong though:
- resume enables vdd, start_meas() fails, the supply stays on.
- the part is then powered but idle, so it NACKs the stop command.
suspend() returns early on that error and never disables vdd, so the
system cannot suspend any more until rebind. This one hurts most.
- if regulator_enable() fails in resume, the next suspend disables a
supply that was never enabled: "unbalanced disables" in the regulator
core, and -EIO. Unbind hits the same case.
The original patch fixes the first one only.
Patch 1 tracks the supply state and drives enable/disable from it.
Patch 2 adds KUnit tests with a fake i2c adapter and a counting
regulator, so no hardware is needed:
./tools/testing/kunit/kunit.py run --kunitconfig=drivers/iio/flow
Four of the eight tests fail without patch 1.
[1] https://lore.kernel.org/linux-iio/20260901025010.356735-1-dayou5941@163.com/
Signed-off-by: Wadim Mueller <wafgo01@gmail.com>
---
Wadim Mueller (2):
iio: flow: slf3s: keep the vdd supply balanced across suspend/resume
iio: flow: slf3s: add KUnit tests for the PM regulator balance
MAINTAINERS | 2 +
drivers/iio/flow/.kunitconfig | 6 +
drivers/iio/flow/Kconfig | 15 +
drivers/iio/flow/Makefile | 1 +
drivers/iio/flow/slf3s-kunit.c | 734 +++++++++++++++++++++++++++++++++++++++++
drivers/iio/flow/slf3s.c | 35 +-
6 files changed, 787 insertions(+), 6 deletions(-)
---
base-commit: bc35965f6940a9bf834d54187b6088b8eb09206d
change-id: 20260904-iio-slf3s-pm-regulator-kunit-43ee079b36f0
Best regards,
--
Wadim Mueller <wafgo01@gmail.com>
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH 1/2] iio: flow: slf3s: keep the vdd supply balanced across suspend/resume
2026-09-04 19:24 [PATCH 0/2] iio: flow: slf3s: fix and test the vdd supply balance Wadim Mueller
@ 2026-09-04 19:24 ` Wadim Mueller
2026-09-05 1:13 ` Jonathan Cameron
2026-09-04 19:24 ` [PATCH 2/2] iio: flow: slf3s: add KUnit tests for the PM regulator balance Wadim Mueller
1 sibling, 1 reply; 5+ messages in thread
From: Wadim Mueller @ 2026-09-04 19:24 UTC (permalink / raw)
To: Maxwell Doose, Jonathan Cameron, David Lechner, Nuno Sá,
Andy Shevchenko
Cc: Jonathan Cameron, linux-iio, linux-kernel, Li Youhong, Li Youhong,
Brendan Higgins, David Gow, Rae Moar, linux-kselftest, kunit-dev,
Wadim Mueller
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.
- 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 <liyouhong@kylinos.cn>
Closes: https://lore.kernel.org/linux-iio/20260901025010.356735-1-dayou5941@163.com/
Fixes: d240b0b8a1ce ("iio: flow: add Sensirion SLF3S liquid flow sensor driver")
Signed-off-by: Wadim Mueller <wafgo01@gmail.com>
---
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. */
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);
}
@@ -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);
+ }
+
+ return ret;
}
static DEFINE_SIMPLE_DEV_PM_OPS(slf3s_pm_ops, slf3s_suspend, slf3s_resume);
--
2.55.0
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH 2/2] iio: flow: slf3s: add KUnit tests for the PM regulator balance
2026-09-04 19:24 [PATCH 0/2] iio: flow: slf3s: fix and test the vdd supply balance Wadim Mueller
2026-09-04 19:24 ` [PATCH 1/2] iio: flow: slf3s: keep the vdd supply balanced across suspend/resume Wadim Mueller
@ 2026-09-04 19:24 ` Wadim Mueller
2026-09-05 1:31 ` Jonathan Cameron
1 sibling, 1 reply; 5+ messages in thread
From: Wadim Mueller @ 2026-09-04 19:24 UTC (permalink / raw)
To: Maxwell Doose, Jonathan Cameron, David Lechner, Nuno Sá,
Andy Shevchenko
Cc: Jonathan Cameron, linux-iio, linux-kernel, Li Youhong, Li Youhong,
Brendan Higgins, David Gow, Rae Moar, linux-kselftest, kunit-dev,
Wadim Mueller
Bind the driver to an emulated SLF3S on a fake I2C adapter, back it with
a regulator whose enable and disable callbacks are counted, and check
that the two stay paired across suspend and resume - including the cycles
in which the sensor NACKs a command or the supply refuses to come up.
The suspend/resume helpers reproduce the PM core's rule that a resume
callback only runs after a suspend callback that returned 0, so that the
tests cannot construct an ordering the core never produces. One of the
tests relies on it: it cycles the device ten times with rotating failure
injection and asserts that the driver's enable count never climbs above
one.
Signed-off-by: Wadim Mueller <wafgo01@gmail.com>
---
MAINTAINERS | 2 +
drivers/iio/flow/.kunitconfig | 6 +
drivers/iio/flow/Kconfig | 15 +
drivers/iio/flow/Makefile | 1 +
drivers/iio/flow/slf3s-kunit.c | 734 +++++++++++++++++++++++++++++++++++++++++
5 files changed, 758 insertions(+)
diff --git a/MAINTAINERS b/MAINTAINERS
index 627595e245f3..5f87c0542700 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -24835,6 +24835,8 @@ R: Maxwell Doose <maxwell@maxwelld.cc>
L: linux-iio@vger.kernel.org
S: Maintained
F: Documentation/devicetree/bindings/iio/flow/sensirion,slf3s.yaml
+F: drivers/iio/flow/.kunitconfig
+F: drivers/iio/flow/slf3s-kunit.c
F: drivers/iio/flow/slf3s.c
SENSIRION SPS30 AIR POLLUTION SENSOR DRIVER
diff --git a/drivers/iio/flow/.kunitconfig b/drivers/iio/flow/.kunitconfig
new file mode 100644
index 000000000000..20b177a241d0
--- /dev/null
+++ b/drivers/iio/flow/.kunitconfig
@@ -0,0 +1,6 @@
+CONFIG_KUNIT=y
+CONFIG_I2C=y
+CONFIG_REGULATOR=y
+CONFIG_IIO=y
+CONFIG_SENSIRION_SLF3S=y
+CONFIG_SENSIRION_SLF3S_KUNIT_TEST=y
diff --git a/drivers/iio/flow/Kconfig b/drivers/iio/flow/Kconfig
index e0e1a8e3654a..cb56f9d8395e 100644
--- a/drivers/iio/flow/Kconfig
+++ b/drivers/iio/flow/Kconfig
@@ -19,4 +19,19 @@ config SENSIRION_SLF3S
To compile this driver as a module, choose M here: the module
will be called slf3s.
+config SENSIRION_SLF3S_KUNIT_TEST
+ tristate "KUnit tests for the Sensirion SLF3S driver" if !KUNIT_ALL_TESTS
+ depends on KUNIT && SENSIRION_SLF3S && REGULATOR
+ default KUNIT_ALL_TESTS
+ help
+ Build KUnit tests for the Sensirion SLF3S driver. The tests bind
+ the driver to an emulated sensor and check that its suspend and
+ resume callbacks keep the supply regulator balanced, including on
+ the error paths.
+
+ For more information on KUnit and unit tests in general, please
+ refer to the KUnit documentation in Documentation/dev-tools/kunit/.
+
+ If unsure, say N.
+
endmenu
diff --git a/drivers/iio/flow/Makefile b/drivers/iio/flow/Makefile
index 3cf4ab95c69c..70c9054da4b6 100644
--- a/drivers/iio/flow/Makefile
+++ b/drivers/iio/flow/Makefile
@@ -5,3 +5,4 @@
# When adding new entries keep the list in alphabetical order
obj-$(CONFIG_SENSIRION_SLF3S) += slf3s.o
+obj-$(CONFIG_SENSIRION_SLF3S_KUNIT_TEST) += slf3s-kunit.o
diff --git a/drivers/iio/flow/slf3s-kunit.c b/drivers/iio/flow/slf3s-kunit.c
new file mode 100644
index 000000000000..d5e2db855838
--- /dev/null
+++ b/drivers/iio/flow/slf3s-kunit.c
@@ -0,0 +1,734 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * KUnit tests for the Sensirion SLF3S driver's power management.
+ *
+ * The tests bind the real driver to a fake I2C device backed by a
+ * programmable regulator, so that every regulator_enable() and
+ * regulator_disable() the driver issues can be counted. The point of the
+ * suite is the balance of those calls across suspend/resume cycles,
+ * including the cycles in which the sensor or the supply fails.
+ *
+ * Copyright (c) 2026
+ */
+
+#include <kunit/device.h>
+#include <kunit/resource.h>
+#include <kunit/test.h>
+
+#include <linux/cleanup.h>
+#include <linux/crc8.h>
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/i2c.h>
+#include <linux/mutex.h>
+#include <linux/pm.h>
+#include <linux/regulator/driver.h>
+#include <linux/regulator/machine.h>
+#include <linux/sprintf.h>
+#include <linux/string.h>
+
+#define SLF3S_TEST_ADDR 0x08
+#define SLF3S_TEST_CRC8_POLY 0x31
+#define SLF3S_TEST_CRC8_INIT 0xff
+#define SLF3S_TEST_PID_LEN 18
+/* "%d-%04x" for an adapter number and a 16-bit address, plus the nul. */
+#define SLF3S_TEST_CONSUMER_LEN 16
+
+/*
+ * Family byte 0x03 and sub-type byte 0x03 make slf3s_detect_variant()
+ * settle on the SLF3S-0600F. The bytes sit at offsets 1 and 3 of the
+ * product-info block, i.e. in the low half of the first word and the high
+ * half of the second one.
+ */
+#define SLF3S_TEST_PID_WORD0 0x0003
+#define SLF3S_TEST_PID_WORD1 0x0300
+
+enum slf3s_fake_state {
+ SLF3S_FAKE_OFF,
+ SLF3S_FAKE_IDLE,
+ SLF3S_FAKE_MEASURING,
+};
+
+enum slf3s_fake_cmd {
+ SLF3S_FAKE_CMD_NONE,
+ SLF3S_FAKE_CMD_PREP_PID,
+ SLF3S_FAKE_CMD_READ_PID,
+ SLF3S_FAKE_CMD_START_WATER,
+ SLF3S_FAKE_CMD_START_IPA,
+ SLF3S_FAKE_CMD_STOP,
+ SLF3S_FAKE_CMD_UNKNOWN,
+ SLF3S_FAKE_CMD_COUNT,
+};
+
+/**
+ * struct slf3s_fake - protocol-level emulation of an SLF3S sensor
+ * @adap: I2C adapter the emulated sensor answers on
+ * @lock: serialises state against the driver's transfers
+ * @state: power/measurement state of the emulated part
+ * @pid_armed: a product-id read command was accepted, a read may follow
+ * @fail_cmd: command whose next occurrence is rejected
+ * @fail_err: error returned for @fail_cmd
+ * @n_cmd: per-command counters, indexed by enum slf3s_fake_cmd
+ * @crc_table: CRC-8 table used to sign the emulated responses
+ */
+struct slf3s_fake {
+ struct i2c_adapter adap;
+ struct mutex lock; /* serialises state against the driver's transfers */
+ enum slf3s_fake_state state;
+ bool pid_armed;
+ enum slf3s_fake_cmd fail_cmd;
+ int fail_err;
+ unsigned int n_cmd[SLF3S_FAKE_CMD_COUNT];
+ u8 crc_table[CRC8_TABLE_SIZE];
+};
+
+/**
+ * struct slf3s_test_reg - regulator that counts what the driver does to it
+ * @rdev: registered regulator device
+ * @lock: protects the counters against concurrent callbacks
+ * @n_enable: successful regulator_enable() calls seen
+ * @n_disable: successful regulator_disable() calls seen
+ * @enabled: current state of the emulated supply
+ * @fail_enable: error returned by the next enable, then cleared
+ */
+struct slf3s_test_reg {
+ struct regulator_dev *rdev;
+ struct mutex lock; /* protects the counters against concurrent callbacks */
+ unsigned int n_enable;
+ unsigned int n_disable;
+ bool enabled;
+ int fail_enable;
+};
+
+/**
+ * struct slf3s_test_ctx - per-test fixture
+ * @fake: emulated sensor
+ * @reg: emulated supply
+ * @client: I2C client the driver is bound to
+ * @suspended: mirrors dev->power.is_suspended for the PM sequencer
+ */
+struct slf3s_test_ctx {
+ struct slf3s_fake fake;
+ struct slf3s_test_reg reg;
+ struct i2c_client *client;
+ bool suspended;
+};
+
+/* --- emulated sensor ---------------------------------------------------- */
+
+static enum slf3s_fake_cmd slf3s_fake_decode(const u8 *buf)
+{
+ static const struct {
+ u8 bytes[2];
+ enum slf3s_fake_cmd cmd;
+ } cmds[] = {
+ { { 0x36, 0x7c }, SLF3S_FAKE_CMD_PREP_PID },
+ { { 0xe1, 0x02 }, SLF3S_FAKE_CMD_READ_PID },
+ { { 0x36, 0x08 }, SLF3S_FAKE_CMD_START_WATER },
+ { { 0x36, 0x15 }, SLF3S_FAKE_CMD_START_IPA },
+ { { 0x3f, 0xf9 }, SLF3S_FAKE_CMD_STOP },
+ };
+
+ for (unsigned int i = 0; i < ARRAY_SIZE(cmds); i++) {
+ if (!memcmp(buf, cmds[i].bytes, sizeof(cmds[i].bytes)))
+ return cmds[i].cmd;
+ }
+
+ return SLF3S_FAKE_CMD_UNKNOWN;
+}
+
+static int slf3s_fake_write(struct slf3s_fake *f, struct i2c_msg *msg)
+{
+ enum slf3s_fake_cmd cmd;
+
+ if (msg->len != 2)
+ return -EIO;
+
+ cmd = slf3s_fake_decode(msg->buf);
+ f->n_cmd[cmd]++;
+
+ if (f->fail_cmd == cmd) {
+ f->fail_cmd = SLF3S_FAKE_CMD_NONE;
+ return f->fail_err;
+ }
+
+ switch (cmd) {
+ case SLF3S_FAKE_CMD_PREP_PID:
+ case SLF3S_FAKE_CMD_READ_PID:
+ if (f->state != SLF3S_FAKE_IDLE)
+ return -ENXIO;
+ f->pid_armed = cmd == SLF3S_FAKE_CMD_READ_PID;
+ return 0;
+ case SLF3S_FAKE_CMD_START_WATER:
+ case SLF3S_FAKE_CMD_START_IPA:
+ if (f->state != SLF3S_FAKE_IDLE)
+ return -ENXIO;
+ f->state = SLF3S_FAKE_MEASURING;
+ return 0;
+ case SLF3S_FAKE_CMD_STOP:
+ /* The real part NACKs a stop when it is already idle. */
+ if (f->state != SLF3S_FAKE_MEASURING)
+ return -ENXIO;
+ f->state = SLF3S_FAKE_IDLE;
+ return 0;
+ default:
+ return -ENXIO;
+ }
+}
+
+static void slf3s_fake_put_word(struct slf3s_fake *f, u8 *dst, u16 val)
+{
+ dst[0] = val >> 8;
+ dst[1] = val & 0xff;
+ dst[2] = crc8(f->crc_table, dst, 2, SLF3S_TEST_CRC8_INIT);
+}
+
+/*
+ * Only the product-info block is emulated. Sample reads are not part of
+ * these tests, and the driver never issues one on its own.
+ */
+static int slf3s_fake_read(struct slf3s_fake *f, struct i2c_msg *msg)
+{
+ u8 buf[SLF3S_TEST_PID_LEN];
+
+ if (!f->pid_armed)
+ return -ENXIO;
+
+ f->pid_armed = false;
+
+ if (msg->len != sizeof(buf))
+ return -EIO;
+
+ slf3s_fake_put_word(f, &buf[0], SLF3S_TEST_PID_WORD0);
+ slf3s_fake_put_word(f, &buf[3], SLF3S_TEST_PID_WORD1);
+ for (unsigned int i = 6; i < sizeof(buf); i += 3)
+ slf3s_fake_put_word(f, &buf[i], 0);
+
+ memcpy(msg->buf, buf, sizeof(buf));
+
+ return 0;
+}
+
+static int slf3s_fake_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
+ int num)
+{
+ struct slf3s_fake *f = i2c_get_adapdata(adap);
+
+ guard(mutex)(&f->lock);
+
+ for (int i = 0; i < num; i++) {
+ int ret;
+
+ if (msgs[i].addr != SLF3S_TEST_ADDR)
+ return -ENXIO;
+
+ if (f->state == SLF3S_FAKE_OFF)
+ return -ENXIO;
+
+ if (msgs[i].flags & I2C_M_RD)
+ ret = slf3s_fake_read(f, &msgs[i]);
+ else
+ ret = slf3s_fake_write(f, &msgs[i]);
+
+ if (ret)
+ return ret;
+ }
+
+ return num;
+}
+
+static u32 slf3s_fake_func(struct i2c_adapter *adap)
+{
+ return I2C_FUNC_I2C;
+}
+
+static const struct i2c_algorithm slf3s_fake_algo = {
+ .xfer = slf3s_fake_xfer,
+ .functionality = slf3s_fake_func,
+};
+
+static void slf3s_fake_fail_next(struct slf3s_fake *f,
+ enum slf3s_fake_cmd cmd, int err)
+{
+ guard(mutex)(&f->lock);
+
+ f->fail_cmd = cmd;
+ f->fail_err = err;
+}
+
+static enum slf3s_fake_state slf3s_fake_state(struct slf3s_fake *f)
+{
+ guard(mutex)(&f->lock);
+
+ return f->state;
+}
+
+/* --- emulated supply ---------------------------------------------------- */
+
+static int slf3s_test_reg_enable(struct regulator_dev *rdev)
+{
+ struct slf3s_test_reg *reg = rdev_get_drvdata(rdev);
+
+ guard(mutex)(®->lock);
+
+ if (reg->fail_enable) {
+ int err = reg->fail_enable;
+
+ reg->fail_enable = 0;
+ return err;
+ }
+
+ reg->n_enable++;
+ reg->enabled = true;
+
+ return 0;
+}
+
+static int slf3s_test_reg_disable(struct regulator_dev *rdev)
+{
+ struct slf3s_test_reg *reg = rdev_get_drvdata(rdev);
+
+ guard(mutex)(®->lock);
+
+ reg->n_disable++;
+ reg->enabled = false;
+
+ return 0;
+}
+
+static int slf3s_test_reg_is_enabled(struct regulator_dev *rdev)
+{
+ struct slf3s_test_reg *reg = rdev_get_drvdata(rdev);
+
+ guard(mutex)(®->lock);
+
+ return reg->enabled;
+}
+
+static void slf3s_test_reg_fail_enable(struct slf3s_test_reg *reg, int err)
+{
+ guard(mutex)(®->lock);
+
+ reg->fail_enable = err;
+}
+
+static const struct regulator_ops slf3s_test_reg_ops = {
+ .enable = slf3s_test_reg_enable,
+ .disable = slf3s_test_reg_disable,
+ .is_enabled = slf3s_test_reg_is_enabled,
+};
+
+static const struct regulator_desc slf3s_test_reg_desc = {
+ .name = "slf3s-test-vdd",
+ .id = -1,
+ .type = REGULATOR_VOLTAGE,
+ .owner = THIS_MODULE,
+ .ops = &slf3s_test_reg_ops,
+};
+
+/* --- PM sequencer ------------------------------------------------------- */
+
+/*
+ * device_suspend() sets dev->power.is_suspended only when the callback
+ * returned 0, and device_resume() bails out before running any callback
+ * when the flag is clear. The resume callback therefore runs if and only
+ * if the immediately preceding suspend callback succeeded. The helpers
+ * below reproduce that rule so the tests cannot construct a sequence the
+ * PM core never produces.
+ */
+static int slf3s_test_suspend(struct kunit *test, struct slf3s_test_ctx *ctx)
+{
+ struct device *dev = &ctx->client->dev;
+ int ret;
+
+ KUNIT_ASSERT_FALSE_MSG(test, ctx->suspended,
+ "suspend called twice without a resume");
+ KUNIT_ASSERT_NOT_NULL(test, dev->driver);
+ KUNIT_ASSERT_NOT_NULL(test, dev->driver->pm);
+ KUNIT_ASSERT_NOT_NULL(test, dev->driver->pm->suspend);
+
+ ret = dev->driver->pm->suspend(dev);
+ ctx->suspended = ret == 0;
+
+ return ret;
+}
+
+static int slf3s_test_resume(struct kunit *test, struct slf3s_test_ctx *ctx)
+{
+ struct device *dev = &ctx->client->dev;
+
+ KUNIT_ASSERT_TRUE_MSG(test, ctx->suspended,
+ "resume after a failed suspend: the PM core does not do this");
+ KUNIT_ASSERT_NOT_NULL(test, dev->driver->pm->resume);
+
+ ctx->suspended = false;
+
+ return dev->driver->pm->resume(dev);
+}
+
+/*
+ * After a resume whose start command failed, the sensor is powered but
+ * idle. A test that wants to continue cycling has to put it back into a
+ * measuring state; doing so through the fake rather than through the
+ * driver keeps the driver's call counts untouched.
+ */
+static void slf3s_test_resume_sensor(struct kunit *test,
+ struct slf3s_test_ctx *ctx)
+{
+ guard(mutex)(&ctx->fake.lock);
+
+ if (ctx->fake.state == SLF3S_FAKE_IDLE)
+ ctx->fake.state = SLF3S_FAKE_MEASURING;
+}
+
+#define KUNIT_EXPECT_REG_BALANCED(test, ctx) do { \
+ KUNIT_EXPECT_EQ((test), (ctx)->reg.n_enable, \
+ (ctx)->reg.n_disable); \
+ KUNIT_EXPECT_FALSE((test), (ctx)->reg.enabled); \
+} while (0)
+
+/* --- fixture ------------------------------------------------------------ */
+
+static void slf3s_test_del_adapter(void *ptr)
+{
+ i2c_del_adapter(ptr);
+}
+
+static void slf3s_test_unregister_reg(void *ptr)
+{
+ regulator_unregister(ptr);
+}
+
+static void slf3s_test_unregister_client(void *ptr)
+{
+ i2c_unregister_device(ptr);
+}
+
+static void slf3s_test_destroy_mutex(void *data)
+{
+ mutex_destroy(data);
+}
+
+static int slf3s_test_init(struct kunit *test)
+{
+ struct regulator_consumer_supply *supply;
+ struct regulator_init_data *init_data;
+ struct i2c_board_info info = { };
+ struct regulator_config config = { };
+ struct slf3s_test_ctx *ctx;
+ struct device *parent;
+ char *consumer;
+ int ret;
+
+ ctx = kunit_kzalloc(test, sizeof(*ctx), GFP_KERNEL);
+ KUNIT_ASSERT_NOT_NULL(test, ctx);
+ test->priv = ctx;
+
+ /* Actions run in reverse, so destroy after the unbind takes the locks. */
+ mutex_init(&ctx->fake.lock);
+ ret = kunit_add_action_or_reset(test, slf3s_test_destroy_mutex,
+ &ctx->fake.lock);
+ KUNIT_ASSERT_EQ(test, ret, 0);
+
+ mutex_init(&ctx->reg.lock);
+ ret = kunit_add_action_or_reset(test, slf3s_test_destroy_mutex,
+ &ctx->reg.lock);
+ KUNIT_ASSERT_EQ(test, ret, 0);
+
+ crc8_populate_msb(ctx->fake.crc_table, SLF3S_TEST_CRC8_POLY);
+ ctx->fake.state = SLF3S_FAKE_OFF;
+
+ parent = kunit_device_register(test, "slf3s-test");
+ KUNIT_ASSERT_NOT_ERR_OR_NULL(test, parent);
+
+ strscpy(ctx->fake.adap.name, "slf3s-test-adapter",
+ sizeof(ctx->fake.adap.name));
+ ctx->fake.adap.owner = THIS_MODULE;
+ ctx->fake.adap.algo = &slf3s_fake_algo;
+ ctx->fake.adap.dev.parent = parent;
+ i2c_set_adapdata(&ctx->fake.adap, &ctx->fake);
+
+ ret = i2c_add_adapter(&ctx->fake.adap);
+ KUNIT_ASSERT_EQ(test, ret, 0);
+ ret = kunit_add_action_or_reset(test, slf3s_test_del_adapter,
+ &ctx->fake.adap);
+ KUNIT_ASSERT_EQ(test, ret, 0);
+
+ /*
+ * The supply has to be resolvable by the time the client probes, and
+ * without a device tree that means a consumer map keyed on the client
+ * name. The name is "<bus>-<addr>", so the adapter has to exist
+ * first.
+ */
+ consumer = kunit_kzalloc(test, SLF3S_TEST_CONSUMER_LEN, GFP_KERNEL);
+ KUNIT_ASSERT_NOT_NULL(test, consumer);
+ scnprintf(consumer, SLF3S_TEST_CONSUMER_LEN, "%d-%04x",
+ ctx->fake.adap.nr, SLF3S_TEST_ADDR);
+
+ supply = kunit_kzalloc(test, sizeof(*supply), GFP_KERNEL);
+ KUNIT_ASSERT_NOT_NULL(test, supply);
+ supply->supply = "vdd";
+ supply->dev_name = consumer;
+
+ init_data = kunit_kzalloc(test, sizeof(*init_data), GFP_KERNEL);
+ KUNIT_ASSERT_NOT_NULL(test, init_data);
+ init_data->constraints.valid_ops_mask = REGULATOR_CHANGE_STATUS;
+ init_data->num_consumer_supplies = 1;
+ init_data->consumer_supplies = supply;
+
+ config.dev = parent;
+ config.init_data = init_data;
+ config.driver_data = &ctx->reg;
+
+ ctx->reg.rdev = regulator_register(parent, &slf3s_test_reg_desc,
+ &config);
+ KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ctx->reg.rdev);
+ ret = kunit_add_action_or_reset(test, slf3s_test_unregister_reg,
+ ctx->reg.rdev);
+ KUNIT_ASSERT_EQ(test, ret, 0);
+
+ /* Powered but idle, like a part that has just been given its supply. */
+ ctx->fake.state = SLF3S_FAKE_IDLE;
+
+ strscpy(info.type, "slf3s-0600f", sizeof(info.type));
+ info.addr = SLF3S_TEST_ADDR;
+
+ ctx->client = i2c_new_client_device(&ctx->fake.adap, &info);
+ KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ctx->client);
+ ret = kunit_add_action_or_reset(test, slf3s_test_unregister_client,
+ ctx->client);
+ KUNIT_ASSERT_EQ(test, ret, 0);
+
+ KUNIT_ASSERT_NOT_NULL_MSG(test, ctx->client->dev.driver,
+ "the slf3s driver did not bind");
+ /* One enable proves we got this regulator, not the dummy one. */
+ KUNIT_ASSERT_EQ_MSG(test, ctx->reg.n_enable, 1,
+ "driver did not enable the test regulator");
+
+ return 0;
+}
+
+/* --- tests -------------------------------------------------------------- */
+
+/* T1: a clean cycle pairs every enable with a disable. */
+static void slf3s_test_cycle_balanced(struct kunit *test)
+{
+ struct slf3s_test_ctx *ctx = test->priv;
+
+ KUNIT_EXPECT_EQ(test, slf3s_fake_state(&ctx->fake),
+ SLF3S_FAKE_MEASURING);
+
+ KUNIT_EXPECT_EQ(test, slf3s_test_suspend(test, ctx), 0);
+ KUNIT_EXPECT_EQ(test, slf3s_fake_state(&ctx->fake), SLF3S_FAKE_IDLE);
+ KUNIT_EXPECT_FALSE(test, ctx->reg.enabled);
+ KUNIT_EXPECT_EQ(test, ctx->reg.n_disable, 1);
+
+ KUNIT_EXPECT_EQ(test, slf3s_test_resume(test, ctx), 0);
+ KUNIT_EXPECT_EQ(test, slf3s_fake_state(&ctx->fake),
+ SLF3S_FAKE_MEASURING);
+ KUNIT_EXPECT_TRUE(test, ctx->reg.enabled);
+ KUNIT_EXPECT_EQ(test, ctx->reg.n_enable, 2);
+
+ kunit_release_action(test, slf3s_test_unregister_client, ctx->client);
+
+ KUNIT_EXPECT_EQ(test, ctx->reg.n_enable, 2);
+ KUNIT_EXPECT_EQ(test, ctx->reg.n_disable, 2);
+ KUNIT_EXPECT_REG_BALANCED(test, ctx);
+}
+
+/*
+ * T5: the proof for the question raised on the list. The PM core never
+ * runs two resumes in a row, so the driver's enable count can never climb
+ * above one, no matter how often the sensor fails. The sequencer refuses
+ * illegal orderings, so this loop is the legal worst case.
+ */
+static void slf3s_test_cycles_never_leak(struct kunit *test)
+{
+ struct slf3s_test_ctx *ctx = test->priv;
+
+ for (unsigned int i = 0; i < 10; i++) {
+ int ret;
+
+ switch (i % 3) {
+ case 1:
+ slf3s_fake_fail_next(&ctx->fake,
+ SLF3S_FAKE_CMD_START_WATER,
+ -ENXIO);
+ break;
+ case 2:
+ slf3s_fake_fail_next(&ctx->fake, SLF3S_FAKE_CMD_STOP,
+ -ENXIO);
+ break;
+ default:
+ break;
+ }
+
+ ret = slf3s_test_suspend(test, ctx);
+ KUNIT_EXPECT_LE_MSG(test, ctx->reg.n_enable - ctx->reg.n_disable,
+ 1, "supply enabled more than once, cycle %u",
+ i);
+ if (ret)
+ continue;
+
+ slf3s_test_resume(test, ctx);
+ KUNIT_EXPECT_LE_MSG(test, ctx->reg.n_enable - ctx->reg.n_disable,
+ 1, "supply enabled more than once, cycle %u",
+ i);
+
+ /* Put the sensor back into a measuring state for the next round. */
+ if (slf3s_fake_state(&ctx->fake) != SLF3S_FAKE_MEASURING &&
+ !ctx->suspended)
+ slf3s_test_resume_sensor(test, ctx);
+ }
+}
+
+/* T6: unbinding after a failed resume must not leave the supply on. */
+static void slf3s_test_unbind_after_failed_resume(struct kunit *test)
+{
+ struct slf3s_test_ctx *ctx = test->priv;
+
+ KUNIT_EXPECT_EQ(test, slf3s_test_suspend(test, ctx), 0);
+
+ slf3s_fake_fail_next(&ctx->fake, SLF3S_FAKE_CMD_START_WATER, -ENXIO);
+ KUNIT_EXPECT_LT(test, slf3s_test_resume(test, ctx), 0);
+
+ kunit_release_action(test, slf3s_test_unregister_client, ctx->client);
+
+ KUNIT_EXPECT_REG_BALANCED(test, ctx);
+}
+
+/* T7: a probe that fails must not leave the supply enabled. */
+static void slf3s_test_probe_failure(struct kunit *test)
+{
+ struct slf3s_test_ctx *ctx = test->priv;
+ struct i2c_board_info info = { };
+ struct i2c_client *client;
+
+ /* Start from a clean slate: drop the client the fixture bound. */
+ kunit_release_action(test, slf3s_test_unregister_client, ctx->client);
+ ctx->client = NULL;
+ KUNIT_ASSERT_FALSE(test, ctx->reg.enabled);
+
+ slf3s_fake_fail_next(&ctx->fake, SLF3S_FAKE_CMD_PREP_PID, -ENXIO);
+
+ strscpy(info.type, "slf3s-0600f", sizeof(info.type));
+ info.addr = SLF3S_TEST_ADDR;
+
+ client = i2c_new_client_device(&ctx->fake.adap, &info);
+ KUNIT_ASSERT_NOT_ERR_OR_NULL(test, client);
+
+ KUNIT_EXPECT_NULL_MSG(test, client->dev.driver,
+ "probe was expected to fail");
+ KUNIT_EXPECT_REG_BALANCED(test, ctx);
+
+ i2c_unregister_device(client);
+}
+
+/* T2: a resume whose start command fails must switch the supply back off. */
+static void slf3s_test_resume_start_failure(struct kunit *test)
+{
+ struct slf3s_test_ctx *ctx = test->priv;
+
+ KUNIT_ASSERT_EQ(test, slf3s_test_suspend(test, ctx), 0);
+
+ slf3s_fake_fail_next(&ctx->fake, SLF3S_FAKE_CMD_START_WATER, -ENXIO);
+ KUNIT_EXPECT_LT(test, slf3s_test_resume(test, ctx), 0);
+
+ KUNIT_EXPECT_FALSE_MSG(test, ctx->reg.enabled,
+ "supply left on after a failed resume");
+ KUNIT_EXPECT_REG_BALANCED(test, ctx);
+}
+
+/* T3: a stop command that fails is no reason to keep the supply on. */
+static void slf3s_test_suspend_stop_failure(struct kunit *test)
+{
+ struct slf3s_test_ctx *ctx = test->priv;
+
+ slf3s_fake_fail_next(&ctx->fake, SLF3S_FAKE_CMD_STOP, -ENXIO);
+ slf3s_test_suspend(test, ctx);
+
+ KUNIT_EXPECT_FALSE_MSG(test, ctx->reg.enabled,
+ "supply left on after a failed stop command");
+ KUNIT_EXPECT_REG_BALANCED(test, ctx);
+}
+
+/*
+ * T4: if the supply itself refuses to come up, the next suspend must not
+ * call regulator_disable() on a supply that was never enabled. Doing so
+ * trips "unbalanced disables" in the regulator core and aborts the whole
+ * system suspend with -EIO.
+ */
+static void slf3s_test_resume_enable_failure(struct kunit *test)
+{
+ struct slf3s_test_ctx *ctx = test->priv;
+ unsigned int n_disable;
+
+ KUNIT_ASSERT_EQ(test, slf3s_test_suspend(test, ctx), 0);
+ n_disable = ctx->reg.n_disable;
+
+ slf3s_test_reg_fail_enable(&ctx->reg, -EIO);
+ KUNIT_EXPECT_EQ(test, slf3s_test_resume(test, ctx), -EIO);
+ KUNIT_EXPECT_FALSE(test, ctx->reg.enabled);
+
+ /* The sensor answers even though the driver's supply never came up. */
+ slf3s_test_resume_sensor(test, ctx);
+
+ KUNIT_EXPECT_EQ_MSG(test, slf3s_test_suspend(test, ctx), 0,
+ "suspend failed after a resume that could not enable the supply");
+ KUNIT_EXPECT_EQ_MSG(test, ctx->reg.n_disable, n_disable,
+ "driver disabled a supply it never enabled");
+ KUNIT_EXPECT_REG_BALANCED(test, ctx);
+}
+
+/*
+ * T8: the devm cleanup must not disable a supply that is already off.
+ *
+ * An unbalanced regulator_disable() WARNs and returns before the fake's
+ * disable op runs, so n_disable cannot tell buggy from correct here.
+ * Count the WARN() instead.
+ */
+static void slf3s_test_unbind_after_enable_failure(struct kunit *test)
+{
+ struct slf3s_test_ctx *ctx = test->priv;
+ unsigned int n_disable;
+
+ KUNIT_ASSERT_EQ(test, slf3s_test_suspend(test, ctx), 0);
+ n_disable = ctx->reg.n_disable;
+
+ slf3s_test_reg_fail_enable(&ctx->reg, -EIO);
+ KUNIT_EXPECT_EQ(test, slf3s_test_resume(test, ctx), -EIO);
+
+ kunit_warning_suppress(test) {
+ kunit_release_action(test, slf3s_test_unregister_client,
+ ctx->client);
+ KUNIT_EXPECT_EQ_MSG(test, KUNIT_SUPPRESSED_WARNING_COUNT(), 0,
+ "unbind triggered \"unbalanced disables\" for a supply that was already off");
+ }
+
+ KUNIT_EXPECT_EQ_MSG(test, ctx->reg.n_disable, n_disable,
+ "unbind disabled a supply that was already off");
+ KUNIT_EXPECT_REG_BALANCED(test, ctx);
+}
+
+static struct kunit_case slf3s_test_cases[] = {
+ KUNIT_CASE(slf3s_test_cycle_balanced),
+ KUNIT_CASE(slf3s_test_cycles_never_leak),
+ KUNIT_CASE(slf3s_test_unbind_after_failed_resume),
+ KUNIT_CASE(slf3s_test_probe_failure),
+ KUNIT_CASE(slf3s_test_resume_start_failure),
+ KUNIT_CASE(slf3s_test_suspend_stop_failure),
+ KUNIT_CASE(slf3s_test_resume_enable_failure),
+ KUNIT_CASE(slf3s_test_unbind_after_enable_failure),
+ { }
+};
+
+static struct kunit_suite slf3s_test_suite = {
+ .name = "slf3s-pm",
+ .init = slf3s_test_init,
+ .test_cases = slf3s_test_cases,
+};
+
+kunit_test_suite(slf3s_test_suite);
+
+MODULE_DESCRIPTION("KUnit tests for the Sensirion SLF3S driver");
+MODULE_LICENSE("GPL");
--
2.55.0
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 1/2] iio: flow: slf3s: keep the vdd supply balanced across suspend/resume
2026-09-04 19:24 ` [PATCH 1/2] iio: flow: slf3s: keep the vdd supply balanced across suspend/resume Wadim Mueller
@ 2026-09-05 1:13 ` Jonathan Cameron
0 siblings, 0 replies; 5+ messages in thread
From: Jonathan Cameron @ 2026-09-05 1:13 UTC (permalink / raw)
To: Wadim Mueller
Cc: Maxwell Doose, David Lechner, Nuno Sá, Andy Shevchenko,
Jonathan Cameron, linux-iio, linux-kernel, Li Youhong, Li Youhong,
Brendan Higgins, David Gow, Rae Moar, linux-kselftest, kunit-dev
On Fri, 04 Sep 2026 21:24:25 +0200
Wadim Mueller <wafgo01@gmail.com> 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 <liyouhong@kylinos.cn>
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/20260901025010.356735-1-dayou5941@163.com/
> Fixes: d240b0b8a1ce ("iio: flow: add Sensirion SLF3S liquid flow sensor driver")
> Signed-off-by: Wadim Mueller <wafgo01@gmail.com>
> ---
> 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);
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 2/2] iio: flow: slf3s: add KUnit tests for the PM regulator balance
2026-09-04 19:24 ` [PATCH 2/2] iio: flow: slf3s: add KUnit tests for the PM regulator balance Wadim Mueller
@ 2026-09-05 1:31 ` Jonathan Cameron
0 siblings, 0 replies; 5+ messages in thread
From: Jonathan Cameron @ 2026-09-05 1:31 UTC (permalink / raw)
To: Wadim Mueller
Cc: Maxwell Doose, David Lechner, Nuno Sá, Andy Shevchenko,
Jonathan Cameron, linux-iio, linux-kernel, Li Youhong, Li Youhong,
Brendan Higgins, David Gow, Rae Moar, linux-kselftest, kunit-dev
On Fri, 04 Sep 2026 21:24:26 +0200
Wadim Mueller <wafgo01@gmail.com> wrote:
> Bind the driver to an emulated SLF3S on a fake I2C adapter, back it with
> a regulator whose enable and disable callbacks are counted, and check
> that the two stay paired across suspend and resume - including the cycles
> in which the sensor NACKs a command or the supply refuses to come up.
>
> The suspend/resume helpers reproduce the PM core's rule that a resume
> callback only runs after a suspend callback that returned 0, so that the
> tests cannot construct an ordering the core never produces. One of the
> tests relies on it: it cycles the device ten times with rotating failure
> injection and asserts that the driver's enable count never climbs above
> one.
>
> Signed-off-by: Wadim Mueller <wafgo01@gmail.com>
Interesting approach to testing the flows.
I'm not sure we want to carry this level of testing for a driver in the
tree. Would like to hear the views of others on this.
There is some really nice stuff in here. Makes me wonder if we should
build a specific test device + driver that would hit all the widest possible
set of corner cases that aren't device specific. The one slightly unusual
thing for this device is the error on telling to stop when it is already stopped.
A couple of things I noted whilst taking a first look.
I'll want to take some time when I'm a lot more awake than I am today
to look at this closely.
Thanks!
Jonathan
> diff --git a/drivers/iio/flow/slf3s-kunit.c b/drivers/iio/flow/slf3s-kunit.c
> new file mode 100644
> index 000000000000..d5e2db855838
> --- /dev/null
> +++ b/drivers/iio/flow/slf3s-kunit.c
> @@ -0,0 +1,734 @@
> +
> +static void slf3s_fake_put_word(struct slf3s_fake *f, u8 *dst, u16 val)
> +{
> + dst[0] = val >> 8;
> + dst[1] = val & 0xff;
Use a put_unaligned_be16() for this.
> + dst[2] = crc8(f->crc_table, dst, 2, SLF3S_TEST_CRC8_INIT);
> +}
> +static int slf3s_test_init(struct kunit *test)
> +{
> + struct regulator_consumer_supply *supply;
> + struct regulator_init_data *init_data;
> + struct i2c_board_info info = { };
> + struct regulator_config config = { };
> + struct slf3s_test_ctx *ctx;
> + struct device *parent;
> + char *consumer;
> + int ret;
> +
> + ctx = kunit_kzalloc(test, sizeof(*ctx), GFP_KERNEL);
> + KUNIT_ASSERT_NOT_NULL(test, ctx);
> + test->priv = ctx;
> +
> + /* Actions run in reverse, so destroy after the unbind takes the locks. */
> + mutex_init(&ctx->fake.lock);
> + ret = kunit_add_action_or_reset(test, slf3s_test_destroy_mutex,
> + &ctx->fake.lock);
> + KUNIT_ASSERT_EQ(test, ret, 0);
> +
> + mutex_init(&ctx->reg.lock);
> + ret = kunit_add_action_or_reset(test, slf3s_test_destroy_mutex,
> + &ctx->reg.lock);
> + KUNIT_ASSERT_EQ(test, ret, 0);
Given those are deep in the emulation which is not what we are testing, do
we need to bother with the mutex destroy calls? That is only there
for some debugging of locks that isn't relevant here I think.
> +
> + crc8_populate_msb(ctx->fake.crc_table, SLF3S_TEST_CRC8_POLY);
> + ctx->fake.state = SLF3S_FAKE_OFF;
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2026-09-05 1:31 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-09-04 19:24 [PATCH 0/2] iio: flow: slf3s: fix and test the vdd supply balance Wadim Mueller
2026-09-04 19:24 ` [PATCH 1/2] iio: flow: slf3s: keep the vdd supply balanced across suspend/resume Wadim Mueller
2026-09-05 1:13 ` Jonathan Cameron
2026-09-04 19:24 ` [PATCH 2/2] iio: flow: slf3s: add KUnit tests for the PM regulator balance Wadim Mueller
2026-09-05 1:31 ` Jonathan Cameron
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox