by Matti Vaittinen

[permalink] [raw]

Subject: Re: [PATCH v6 0/7] Support ROHM BU27034 ALS sensor

On 3/27/23 14:27, Matti Vaittinen wrote:
> Support ROHM BU27034 ALS sensor
>
> The patch 1/7 introduces the helpers for creating/dropping a test device
> for devm-tests. It can be applied alone.

Sorry folks. The wrapper is 3/7 not 1/7

> The patch 4/7 (IIO GTS tests) also depends on the patch 1/7 (and also
> other patches in the series).
>
> Rest of the series should be Ok to be applied with/without the patches
> 1/7 and 4/7 - although the 4/7 (which depends on 1/7) would be "nice to
> have" together with the rest of the series for the testability reasons.
>

snip.

>
> Matti Vaittinen (7):
> dt-bindings: iio: light: Support ROHM BU27034
> iio: light: Add gain-time-scale helpers
> kunit: Add kunit wrappers for (root) device creation

Here.

> iio: test: test gain-time-scale helpers
> MAINTAINERS: Add IIO gain-time-scale helpers
> iio: light: ROHM BU27034 Ambient Light Sensor
> MAINTAINERS: Add ROHM BU27034
>
Yours,
-- Matti

--
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~

2023-03-27 11:51:27

by Matti Vaittinen

[permalink] [raw]

Subject: [PATCH v6 3/7] kunit: Add kunit wrappers for (root) device creation

A few tests need to have a valid struct device. One such example is
tests which want to be testing devm-managed interfaces.

Add kunit wrapper for root_device_[un]register(), which create a root
device and also add a kunit managed clean-up routine for the device
destruction upon test exit.

Special note: In some cases the device reference-count does not reach
zero and devm-unwinding is not done if device is not sitting on a bus.
The root_device_[un]register() are dealing with such devices and thus
this interface may not be usable by all in its current form. More
information can be found from:
https://lore.kernel.org/dri-devel/20221117165311.vovrc7usy4efiytl@houat/

The use of root-devices in the kunit helpers is intended to be an
intermediate solution to allow tests which do not require device to sit
on a bus avoid directly abusing the root_device_[un]register() while
proper kunit device solution is being worked on. Related discussion can be
found from:
https://lore.kernel.org/lkml/CABVgOSmx3A4Vwos2_8xO-XQrQAw5gvY0nc5zLpLmcJ7FtA-dTQ@mail.gmail.com/

Signed-off-by: Matti Vaittinen <[email protected]>

---
Change history:
v5 => v6:
- Kunit resource-managed root_device creation wrapper (new patch)

Please note: This patch uses root-devices (as was suggested) until there
is a proper dummy device creation mechanism added in kunit. The root
devices are embedded in kunit wrappers to simplify replacing the
root-devices with proper solution when it is available.

David Gow has sent out an RFC[1] which should implement these helpers
using not-yet-in-tree deferring API. This RFC aims to support
kunit_device which should be _the right thing to do_. I added this
implementation here because it may (or may not) take a while for the David's
RFC to make it's way in-kernel. So, in order to not delay this series I
added these helpers which use the existing kunit resource management for
clean-up while the new deferring kunit API is not yet in place.

[1] https://lore.kernel.org/linux-kselftest/[email protected]/T/#mf797239a8bce11630875fdf60aab9ed627add1f0

---
include/kunit/device.h | 18 ++++++++++++++++++
lib/kunit/Makefile | 3 ++-
lib/kunit/device.c | 36 ++++++++++++++++++++++++++++++++++++
3 files changed, 56 insertions(+), 1 deletion(-)
create mode 100644 include/kunit/device.h
create mode 100644 lib/kunit/device.c

diff --git a/include/kunit/device.h b/include/kunit/device.h
new file mode 100644
index 000000000000..f02740b7583b
--- /dev/null
+++ b/include/kunit/device.h
@@ -0,0 +1,18 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#ifndef __KUNIT_DEVICE_H__
+#define __KUNIT_DEVICE_H__
+
+#include <kunit/test.h>
+
+struct device;
+
+/* Register a new device against a KUnit test. */
+struct device *kunit_device_register(struct kunit *test, const char *name);
+/*
+ * Unregister a device created by kunit_device_register() early (i.e.,
+ * before test cleanup).
+ */
+void kunit_device_unregister(struct kunit *test, struct device *dev);
+
+#endif
diff --git a/lib/kunit/Makefile b/lib/kunit/Makefile
index cb417f504996..64449549b990 100644
--- a/lib/kunit/Makefile
+++ b/lib/kunit/Makefile
@@ -6,7 +6,8 @@ kunit-objs += test.o \
string-stream.o \
assert.o \
try-catch.o \
- executor.o
+ executor.o \
+ device.o

ifeq ($(CONFIG_KUNIT_DEBUGFS),y)
kunit-objs += debugfs.o
diff --git a/lib/kunit/device.c b/lib/kunit/device.c
new file mode 100644
index 000000000000..425f6d62ebd7
--- /dev/null
+++ b/lib/kunit/device.c
@@ -0,0 +1,36 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <kunit/device.h>
+#include <kunit/test.h>
+
+#include <linux/device.h>
+
+static void kunit_device_drop(struct kunit_resource *res)
+{
+ root_device_unregister(res->data);
+}
+
+struct device *kunit_device_register(struct kunit *test, const char *name)
+{
+ struct device *dev;
+
+ dev = root_device_register(name);
+ if (IS_ERR_OR_NULL(dev))
+ return dev;
+
+ return kunit_alloc_resource(test, NULL, kunit_device_drop, GFP_KERNEL,
+ dev);
+}
+EXPORT_SYMBOL_GPL(kunit_device_register);
+
+static bool kunit_device_match(struct kunit *test, struct kunit_resource *res,
+ void *match_data)
+{
+ return res->data == match_data;
+}
+
+void kunit_device_unregister(struct kunit *test, struct device *dev)
+{
+ kunit_destroy_resource(test, kunit_device_match, dev);
+}
+EXPORT_SYMBOL_GPL(kunit_device_unregister);
--
2.39.2

--
Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND

~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~
Simon says - in Latin please.
~~~ "non cogito me" dixit Rene Descarte, deinde evanescavit ~~~
Thanks to Simon Glass for the translation =]

Attachments:

(No filename) (4.80 kB)
signature.asc (499.00 B)
Download all attachments

2023-03-27 11:51:27

Thanks, Gred and Matti.

On Mon, 27 Mar 2023 at 20:38, Greg Kroah-Hartman
<[email protected]> wrote:
>
> On Mon, Mar 27, 2023 at 03:20:06PM +0300, Matti Vaittinen wrote:
> > On 3/27/23 15:01, Greg Kroah-Hartman wrote:
> > > On Mon, Mar 27, 2023 at 02:34:02PM +0300, Matti Vaittinen wrote:
> > > > A few tests need to have a valid struct device. One such example is
> > > > tests which want to be testing devm-managed interfaces.
> > > >
> > > > Add kunit wrapper for root_device_[un]register(), which create a root
> > > > device and also add a kunit managed clean-up routine for the device
> > > > destruction upon test exit.
> > >
> > > I really do not like this as a "root device" is a horrible hack and
> > > should only be used if you have to hang other devices off of it and you
> > > don't have a real device to tie those devices to.
> > >
> > > Here you are abusing it and attempting to treat it as a real device,
> > > which it is not at all, because:
> > >

There's a tradeoff here in that we want to pull in as little code (and
complexity) as possible into unit tests, both to make them as easy as
possible to write, and to make them as targeted as possible. This
leads to a lot of tests manually filling out structures with the bare
minimum to get the code being tested to run, and creating "root
devices" seems to have been a convenient way of doing that while only
registering _one_ thing in a big global list per test. So having a
"real device" is not something I'd consider a _necessity_ in designing
these sorts of helpers: a convincing enough fake is sometimes better.

That being said, now that I've got a bit of a better understanding of
the device model, I agree that "root devices" are not an ideal
solution, even if they are a convenient one. I'm still not thrilled by
the prospect of having to register extra things like drivers to get
these simple tests to work, but when wrapped behind helpers, it's a
nice solution in practice.

> > > > Special note: In some cases the device reference-count does not reach
> > > > zero and devm-unwinding is not done if device is not sitting on a bus.
> > > > The root_device_[un]register() are dealing with such devices and thus
> > > > this interface may not be usable by all in its current form. More
> > > > information can be found from:
> > > > https://lore.kernel.org/dri-devel/20221117165311.vovrc7usy4efiytl@houat/
> > >
> > > See, not a real device, doesn't follow normal "struct device" rules and
> > > lifetimes, don't try to use it for a test as it will only cause problems
> > > and you will be forced to work around that in a test.

I think there's still some confusion around exactly what these issues
are, and if they're indeed bugs or expected behaviour. I think it
hangs off the question of what uses of a device with no driver
attached are valid. My initial reading through of the various bits of
the devres implementation seemed to imply that using it with such an
unattached device was supported, but I'm less certain now. In any
case, Maxime's tests in the other thread are a good starting point to
clarify this behaviour, and if we use the bus-based KUnit helpers, it
won't matter either way.

> >
> > Ok. I understood using the root-device has been a work-around in some other
> > tests. Thus continuing use it for tests where we don't need the bus until we
> > have a proper alternative was suggested by David.
> >
> > > Do the right thing here, create a fake bus and add devices to it.
> > >
> > > Heck, I'll even write that code if you want it, what's the requirement,
> > > something like:
> > > struct device *kunit_device_create(struct kunit *test, const char *name);
> > > void kunit_device_destroy(struct device *dev);
> >
> > Thanks for the offer Greg. This, however, is being already worked on by
> > David. I don't want to step on his toes by writing the same thing, nor do I
> > think I should be pushing him to rush on his work.
>
> Ok, David, my offer stands, if you want me to write this I will be glad
> to do so.

I'm happy to keep working on this, but would definitely appreciate
your feedback.

I've put my work-in-progress code here:
https://kunit.googlesource.com/linux/+/refs/heads/kunit/device-helpers%5E%21/#F0

It creates a "kunit" bus, and adds a few helpers to create both
devices and drivers on that bus, and clean them up when the test
exits. It seems to work on all of the tests which used
root_device_register so far (except those -- only
test_iio_find_closest_gain_low so far -- which create multiple devices
with the same name, as the driver name won't be unique), and the drm
tests work fine when ported to it as well.

There's still a lot of cleanup to do and questions which need
answering, including:
- Working out how best to provide an owning module (it's currently
just kunit, but probably should be the module which contains the
actual tests)
- Improving the API around drivers: probably exposing the helper to
create a driver, and add a way of cleaning it up early.
- Properly testing it with modules, not just with kunit.py (including
looking at what actually appears in sysfs)
- Experimenting with using probe, etc, callbacks.
- Cleaning up lots of ugly, still experimental code, documenting, testing, etc.

The thought of trying to expand the match function to support, e.g.,
devicetree occurred to me, but I _think_ that devicetree-based tests
are probably still better off using a platform_device. Regardless, it
can probably wait to a follow-up

In any case, does this seem like the right way forward?

Cheers,
-- David

Attachments:

smime.p7s (3.91 kB)
S/MIME Cryptographic Signature

2023-03-28 13:29:04

by Matti Vaittinen

[permalink] [raw]

Subject: Re: [PATCH v6 3/7] kunit: Add kunit wrappers for (root) device creation

Hi David & Greg and thanks for working with this!

On 3/28/23 15:45, David Gow wrote:
> Thanks, Gred and Matti.
>
> On Mon, 27 Mar 2023 at 20:38, Greg Kroah-Hartman
> <[email protected]> wrote:
>>
>> On Mon, Mar 27, 2023 at 03:20:06PM +0300, Matti Vaittinen wrote:
>>> On 3/27/23 15:01, Greg Kroah-Hartman wrote:
>>>> On Mon, Mar 27, 2023 at 02:34:02PM +0300, Matti Vaittinen wrote:
>
> I'm happy to keep working on this, but would definitely appreciate
> your feedback.
>
> I've put my work-in-progress code here:
> https://kunit.googlesource.com/linux/+/refs/heads/kunit/device-helpers%5E%21/#F0
>
> It creates a "kunit" bus, and adds a few helpers to create both
> devices and drivers on that bus, and clean them up when the test
> exits. It seems to work on all of the tests which used
> root_device_register so far (except those -- only
> test_iio_find_closest_gain_low so far -- which create multiple devices
> with the same name, as the driver name won't be unique),

I wouldn't worry about it for as long as it's just because an iio-gts
test does something silly. Those tests are currently only in my personal
playground and changing those tests should be pretty trivial.

And right after saying that - the test_iio_find_closest_gain_low test does

a) register a 'test' device
b) perform test on devm_ API
c) unregister the 'test' device

d) register a 'test' device (same name as at step a)
e) perform test on devm_ API
f) unregister the 'test' device

My assumption is that the test device would be gone after step c)
because there should be no references to it anywhere. Hence, I wonder
why registering at step d) fails? (Or did I misunderstand something?)

> and the drm
> tests work fine when ported to it as well.
>
> There's still a lot of cleanup to do and questions which need
> answering, including:
> - Working out how best to provide an owning module (it's currently
> just kunit, but probably should be the module which contains the
> actual tests)

Maybe there is something I am not seeing but how about wrapping the
kunit_device_register() in a macro and getting the THIS_MODULE in
caller's context?

> In any case, does this seem like the right way forward?

I am by no means an expert on this but this does look good to me. I
would keep this as clean, lean and simple as possible in order to keep
understanding / debugging the problems exposed by the tests as simple as
possible. At some point someone is wondering why a test fails, and ends
up looking through these helpers to ensure problem is no lurking
there... Hence, I'd kept the code there in minimum - meaning, I might
not add kunit class or even a driver until tests require that. (Even if
it would not look as good in the sysfs - as far as I understand the
kunit sysfs entries are a 'test feature' which should not be present in
'production systems'. This is not an excuse to make things bad - but (in
my opinion) this is a good reason to prioritize simplicity.

Anyways, thanks for the work!

Yours,
-- Matti

--
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~

2023-03-28 13:44:38

On 3/27/23 14:34, Matti Vaittinen wrote:
> ROHM BU27034 is an ambient light sensor with 3 channels and 3 photo diodes
> capable of detecting a very wide range of illuminance. Typical application
> is adjusting LCD and backlight power of TVs and mobile phones.
>
> Add initial support for the ROHM BU27034 ambient light sensor.
>
> NOTE:
> - Driver exposes 4 channels. One IIO_LIGHT channel providing the
> calculated lux values based on measured data from diodes #0 and
> #1. In addition, 3 IIO_INTENSITY channels are emitting the raw
> register data from all diodes for more intense user-space
> computations.
> - Sensor has GAIN values that can be adjusted from 1x to 4096x.
> - Sensor has adjustible measurement times of 5, 55, 100, 200 and
> 400 mS. Driver does not support 5 mS which has special
> limitations.
> - Driver exposes standard 'scale' adjustment which is
> implemented by:
> 1) Trying to adjust only the GAIN
> 2) If GAIN adjustment alone can't provide requested
> scale, adjusting both the time and the gain is
> attempted.
> - Driver exposes writable INT_TIME property that can be used
> for adjusting the measurement time. Time adjustment will also
> cause the driver to try to adjust the GAIN so that the
> overall scale is kept as close to the original as possible.
>
> Signed-off-by: Matti Vaittinen <[email protected]>
>
> ---

...

> +/*
> + * We try to change the time in such way that the scale is maintained for
> + * given channels by adjusting gain so that it compensates the time change.
> + */
> +static int bu27034_try_set_int_time(struct bu27034_data *data, int time_us)
> +{
> + struct bu27034_gain_check gains[] = {
> + { .chan = BU27034_CHAN_DATA0 },
> + { .chan = BU27034_CHAN_DATA1 },
> + };
> + int numg = ARRAY_SIZE(gains);
> + int ret, int_time_old, i;
> +
> + mutex_lock(&data->mutex);
> + ret = bu27034_get_int_time(data);
> + if (ret < 0)
> + goto unlock_out;
> +
> + int_time_old = ret;
> +
> + if (!iio_gts_valid_time(&data->gts, time_us)) {
> + dev_err(data->dev, "Unsupported integration time %u\n",
> + time_us);
> + ret = -EINVAL;
> +
> + goto unlock_out;
> + }
> +
> + if (time_us == int_time_old) {
> + ret = 0;
> + goto unlock_out;
> + }
> +
> + for (i = 0; i < numg; i++) {
> + ret = bu27034_get_gain(data, gains[i].chan, &gains[i].old_gain);
> + if (ret)
> + goto unlock_out;
> +
> + ret = iio_gts_find_new_gain_by_old_gain_time(&data->gts,
> + gains[i].old_gain,
> + int_time_old, time_us,
> + &gains[i].new_gain);
> + if (ret) {

As mentioned in my comment to the helper patch, we should have different
returnvalue here depending on if the given times were invalid (and new
gain was not computed) or if the new_gain was computed but not
supported. I plan to use -ERANGE to denote "new gain computed but not
supported" and add...

> + int scale1, scale2;
> + bool ok;
> +
> + _bu27034_get_scale(data, gains[i].chan, &scale1, &scale2);
> + dev_dbg(data->dev,
> + "chan %u, can't support time %u with scale %u %u\n",
> + gains[i].chan, time_us, scale1, scale2);

+ if (ret != -ERANGE)
+ goto unlock_out;

... here at v7.

> +
> + /*
> + * If caller requests for integration time change and we
> + * can't support the scale - then the caller should be
> + * prepared to 'pick up the pieces and deal with the
> + * fact that the scale changed'.
> + */
> + ret = iio_find_closest_gain_low(&data->gts,
> + gains[i].new_gain, &ok);
> +
> + if (!ok) {
> + dev_dbg(data->dev,
> + "optimal gain out of range for chan %u\n",
> + gains[i].chan);
> + }
> + if (ret < 0) {
> + dev_dbg(data->dev,
> + "Total gain increase. Risk of saturation");
> + ret = iio_gts_get_min_gain(&data->gts);
> + if (ret < 0)
> + goto unlock_out;
> + }
> + dev_dbg(data->dev, "chan %u scale changed\n",
> + gains[i].chan);
> + gains[i].new_gain = ret;
> + dev_dbg(data->dev, "chan %u new gain %u\n",
> + gains[i].chan, gains[i].new_gain);
> + }
> + }
> +
> + for (i = 0; i < numg; i++) {
> + ret = bu27034_set_gain(data, gains[i].chan, gains[i].new_gain);
> + if (ret)
> + goto unlock_out;
> + }
> +
> + ret = bu27034_set_int_time(data, time_us);
> +
> +unlock_out:
> + mutex_unlock(&data->mutex);
> +
> + return ret;
> +}
> +

Yours,
-- Matti

--
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~