[RFC,v1,07/10] iio: light: opt3001: add roadtest

Add a regression test for the problem fixed by the following patch,
which would require specific environmental conditions to be able to be
reproduced and regression-tested on real hardware:

 iio: light: opt3001: Fixed timeout error when 0 lux
 https://lore.kernel.org/lkml/20210920125351.6569-1-valek@2n.cz/

No other aspects of the driver are tested.

Signed-off-by: Vincent Whitchurch <vincent.whitchurch@axis.com>
---
 .../roadtest/roadtest/tests/iio/__init__.py   |  0
 .../roadtest/roadtest/tests/iio/config        |  1 +
 .../roadtest/tests/iio/light/__init__.py      |  0
 .../roadtest/roadtest/tests/iio/light/config  |  1 +
 .../roadtest/tests/iio/light/test_opt3001.py  | 95 +++++++++++++++++++
 5 files changed, 97 insertions(+)
 create mode 100644 tools/testing/roadtest/roadtest/tests/iio/__init__.py
 create mode 100644 tools/testing/roadtest/roadtest/tests/iio/config
 create mode 100644 tools/testing/roadtest/roadtest/tests/iio/light/__init__.py
 create mode 100644 tools/testing/roadtest/roadtest/tests/iio/light/config
 create mode 100644 tools/testing/roadtest/roadtest/tests/iio/light/test_opt3001.py

On Tue, Mar 15, 2022 at 12:11:50AM +0100, Brendan Higgins wrote:
> On Fri, Mar 11, 2022 at 11:24 AM Vincent Whitchurch
> <vincent.whitchurch@axis.com> wrote:
> > +class TestOPT3001(UMLTestCase):
> 
> I am partial to starting with UML since there are a lot of nice easy
> things about starting there; however, I imagine people will eventually
> want to use this on other architectures (speaking from experience).
> How difficult do you think it would be to extend this to support
> manipulating fake devices in say QEMU?

It should be possible, but upstream QEMU doesn't have everything that we
need so some work is needed there.  Also, of course work is need to
provide user space for running the tests and communicating between the
virtual machine and the backend:

- We need user space, so build scripts would need to be provided to
  cross-compile busybox and Python (and whatever libraries it needs) for
  the target architecture.

- We also use UML's hostfs feature to make things transparent to the
  user and to avoid having to set up things like networking for
  communication between the host and the backend.  I think QEMU's 9pfs
  support can be used as a rootfs too but it's not something I've
  personally tested.

- We use virtio-i2c and virtio-gpio and use virtio-uml which uses the
  vhost-user API to communicate from UML to the backend.  The latest
  version of QEMU has support for vhost-user-i2c, but vhost-user-gpio
  doesn't seem to have been merged yet, so work is needed on the QEMU
  side.  This will also be true for other buses in the future, if they
  are implemented with new virtio devices.

- For MMIO, UML has virtio-mmio which allows implementing any PCIe
  device (and by extension any platform device) outside of UML, but last
  I checked, upstream QEMU did not have something similar.

> I also have some colleagues inside of Google that worked on some
> projects to simulate simple devices on an FPGA to test software and
> adjacent devices in a conceptually similar way; one of these teams
> built a Domain Specific Language kind of like roadtest to implement
> the tests and the environment for the tests. The main reason I mention
> this here is I am thinking about maybe one day having an API you can
> implement so you can run your roadtests on UML, QEMU, or on any
> emulator or hardware testbed that implements the appropriate API.
> 
> I'll try to dig up some people who might be interested and add them here.
> 
> > +    dts = DtFragment(
> > +        src="""
> > +&i2c {
> > +    light-sensor@$addr$ {
> > +        compatible = "ti,opt3001";
> > +        reg = <0x$addr$>;
> > +    };
> > +};
> > +        """,
> > +        variables={
> > +            "addr": DtVar.I2C_ADDR,
> > +        },
> > +    )
> > +
> > +    @classmethod
> > +    def setUpClass(cls) -> None:
> > +        insmod("opt3001")
> > +
> > +    @classmethod
> > +    def tearDownClass(cls) -> None:
> > +        rmmod("opt3001")
> > +
> > +    def setUp(self) -> None:
> > +        self.driver = I2CDriver("opt3001")
> > +        self.hw = Hardware("i2c")
> > +        self.hw.load_model(OPT3001)
> > +
> > +    def tearDown(self) -> None:
> > +        self.hw.close()
> > +
> > +    def test_illuminance(self) -> None:
> > +        data = [
> > +            # Some values from datasheet, and 0
> > +            (0b_0000_0000_0000_0000, 0),
> > +            (0b_0000_0000_0000_0001, 0.01),
> > +            (0b_0011_0100_0101_0110, 88.80),
> > +            (0b_0111_1000_1001_1010, 2818.56),
> > +        ]
> > +        with self.driver.bind(self.dts["addr"]) as dev:
> > +            luxfile = dev.path / "iio:device0/in_illuminance_input"
> > +
> > +            for regval, lux in data:
> > +                self.hw.reg_write(REG_RESULT, regval)
> > +                self.assertEqual(read_float(luxfile), lux)
> 
> I love the framework; this looks very easy to use.
> 
> One nit about this test; it seems like you cover just one test case
> here - the happy path. Can you cover some other one? Particularly some
> error paths?
> 
> Sorry, I am not trying to be cheeky here; it looks like this driver
> actually should probably be fully (or very close to fully) testable
> via roadtest as I understand it. It only looks like there are a
> handful of cases to cover for the driver: the device is busy, the
> device returned something invalid, the user requested something
> invalid, and several SMBus read/write failures - it really only looks
> like there are a handful of paths and I think they are all accessible
> via the I2C interface (except for maybe the user requesting something
> invalid).

Yes, there are more things that could be tested in this driver.
However, as the commit message says, I only indented this particular
test to serve as a regression test for the specific bug fix, which would
need an environment where the chip detects 0 lux to be able to test on
real hardware.  There are a few reasons for this:

 - Unlike the other drivers being tested in this series, I don't have
   access to boards with this chip so my interest in this particular
   piece of hardware is limited.

 - I actually started writing more tests for this driver earlier on
   (specifically, testing the configuration which uses interrupts), but
   I quickly discovered that this driver has race conditions which
   result in unbalanced mutex locking (in brief: the ok_to_ignore_lock
   stuff is broken).  This shows the value of the test framework, but I
   also didn't want to write non-trivial fixes for drivers where I
   didn't have real hardware to test.

 - Also, some paths in this driver needs a modification to be tested
   under roadtest.  It uses wait_event_timeout() with a fixed value, but
   we cannot guarantee that this constraint is met in the test
   environment since it depends on things like CPU load on the host.

   (Also, we use UML's "time travel" feature which essentially
   fast-forwards through idle time, so the constraint can never be met
   in practice.)
   
   So the timeout parameter would have to be made adjustable via say a
   module parameter, to be able to make it infinite (to test the normal
   case) and not (to be able to test timeout handling).  I think this
   could be done fairly cleanly with a one- or two-liner patch to the
   driver and by hiding the details in a header file behind a
   roadtest-specific config option, but I wanted to avoid having to
   patch the kernel proper for the initial version of the framework.

For vcnl4000, I have actually inherited some out-of-tree patches which
are in need of mainlining so the tests are a bit more complete since I'm
hoping to send some patches to that driver soon.  The period mode busy
handling is not tested there either though, I can try to add that.

As for I2C API failures, I have not added tests for them in any of the
drivers.  There's not much the test cases could assert, other than
perhaps error propagation, so it's unclear if there is enough value
compared to the effort required to implement test cases to make sure
that every I2C transaction's failure path is tested.

But I think that we do want to at least make sure the error paths are
executed, to check that drivers don't crash or deadlock due to faulty
cleanups and the like.  A way to solve this could be to implement fault
injection support in the I2C framework.  The fail-nth feature could be
used to systemically trigger each and every I2C transaction failure and
check that the driver doesn't blow up, while using the roadtest as a
means to hit the various code paths in the driver during each of the
iterations of fail-nth.  Fault injection support would also be helpful
when testing on real hardware.

On Fri, 2022-03-18 at 16:49 +0100, Vincent Whitchurch wrote:
> 
> It should be possible, but upstream QEMU doesn't have everything that we
> need so some work is needed there.  Also, of course work is need to
> provide user space for running the tests and communicating between the
> virtual machine and the backend:
> 
> - We need user space, so build scripts would need to be provided to
>   cross-compile busybox and Python (and whatever libraries it needs) for
>   the target architecture.

You could possibly use some nix recipes for all of this, but that's a
fairly arcane thing (we use it, but ...)

> - We also use UML's hostfs feature to make things transparent to the
>   user and to avoid having to set up things like networking for
>   communication between the host and the backend.  I think QEMU's 9pfs
>   support can be used as a rootfs too but it's not something I've
>   personally tested.

That works just fine, yes. We used to do exactly this in the wireless
test suite before we switched to UML, but the switch to UML was due to
the "time-travel" feature.

https://w1.fi/cgit/hostap/tree/tests/hwsim/vm

has support for both UML and qemu/kvm.

> - We use virtio-i2c and virtio-gpio and use virtio-uml which uses the
>   vhost-user API to communicate from UML to the backend.  The latest
>   version of QEMU has support for vhost-user-i2c, but vhost-user-gpio
>   doesn't seem to have been merged yet, so work is needed on the QEMU
>   side.  This will also be true for other buses in the future, if they
>   are implemented with new virtio devices.
> 
> - For MMIO, UML has virtio-mmio which allows implementing any PCIe
>   device (and by extension any platform device) outside of UML, but last
>   I checked, upstream QEMU did not have something similar.

I think you have this a bit fuzzy.

The virtio_uml[.c] you speak of is the "bus" driver for virtio in UML.
Obviously, qemu has support for virtio, so you don't need those bits.

Now, virtio_uml is actually the virtio (bus) driver inside the kernel,
like you'd have virtio-mmio/virtio-pci in qemu. However, virtio_uml
doesn't implement the devices in the hypervisor, where most qemu devices
are implemented, but uses vhost-user to run the device implementation in
a separate userspace. [1]

Now we're talking about vhost-user to talk to the device, and qemu
supports this as well, in fact the vhost-user spec is part of qemu:
https://git.qemu.org/?p=qemu.git;a=blob;f=docs/system/devices/vhost-user.rst;h=86128114fa3788a73679f0af38e141021087c828;hb=1d60bb4b14601e38ed17384277aa4c30c57925d3
https://www.qemu.org/docs/master/interop/vhost-user.html

The docs on how to use it are here:
https://www.qemu.org/docs/master/system/devices/vhost-user.html

So once you have a device implementation (regardless of whether it's for
use with any of the virtio-i2c, arch/um/drivers/virt-pci.c, virtio-gpio,
virtio-net, ... drivers) you can actually connect it to virtual machines
running as UML or in qemu.

(Actually, that's not strictly true today since it's
arch/um/drivers/virt-pci.c and I didn't get a proper device ID assigned
etc since it was for experimentation, I guess if we make this more
commonly used then we should move it to drivers/pci/controller/virtio-
pci.c and actually specify it in the OASIS virtio spec., at the very
least it'd have to be possible to compile this and lib/logic_iomem.c on
x86, but that's possible. Anyway I think PCI(e) is probably low on your
list of things ...)

>  - Also, some paths in this driver needs a modification to be tested
>    under roadtest.  It uses wait_event_timeout() with a fixed value, but
>    we cannot guarantee that this constraint is met in the test
>    environment since it depends on things like CPU load on the host.
> 
>    (Also, we use UML's "time travel" feature which essentially
>    fast-forwards through idle time, so the constraint can never be met
>    in practice.)

Wohoo! This makes me very happy, finally somebody else who uses it :-)



[1] As an aside, you might be interested in usfstl (which you can find
at https://github.com/linux-test-project/usfstl) which is one way you
could implement the device side - though the focus here is on making a
device implementation easy while under "time-travel" mode.

If you ever want to use time-travel with multiple machines or actually
with virtio devices, it also contains the necessary controller program
to glue the entire simulation together. We use this very successfully to
test the (real but compiled for x86) wifi firmware for iwlwifi together
with the real driver actually seeing a PCIe device in UML, under time-
travel :)

johannes

On Fri, Mar 18, 2022 at 09:09:02PM +0100, Johannes Berg wrote:
> On Fri, 2022-03-18 at 16:49 +0100, Vincent Whitchurch wrote:
> > - We use virtio-i2c and virtio-gpio and use virtio-uml which uses the
> >   vhost-user API to communicate from UML to the backend.  The latest
> >   version of QEMU has support for vhost-user-i2c, but vhost-user-gpio
> >   doesn't seem to have been merged yet, so work is needed on the QEMU
> >   side.  This will also be true for other buses in the future, if they
> >   are implemented with new virtio devices.
> > 
> > - For MMIO, UML has virtio-mmio which allows implementing any PCIe
> >   device (and by extension any platform device) outside of UML, but last
> >   I checked, upstream QEMU did not have something similar.
> 
> I think you have this a bit fuzzy.
> 
> The virtio_uml[.c] you speak of is the "bus" driver for virtio in UML.
> Obviously, qemu has support for virtio, so you don't need those bits.
> 
> Now, virtio_uml is actually the virtio (bus) driver inside the kernel,
> like you'd have virtio-mmio/virtio-pci in qemu. However, virtio_uml
> doesn't implement the devices in the hypervisor, where most qemu devices
> are implemented, but uses vhost-user to run the device implementation in
> a separate userspace. [1]
> 
> Now we're talking about vhost-user to talk to the device, and qemu
> supports this as well, in fact the vhost-user spec is part of qemu:
> https://git.qemu.org/?p=qemu.git;a=blob;f=docs/system/devices/vhost-user.rst;h=86128114fa3788a73679f0af38e141021087c828;hb=1d60bb4b14601e38ed17384277aa4c30c57925d3
> https://www.qemu.org/docs/master/interop/vhost-user.html
> 
> The docs on how to use it are here:
> https://www.qemu.org/docs/master/system/devices/vhost-user.html
> 
> So once you have a device implementation (regardless of whether it's for
> use with any of the virtio-i2c, arch/um/drivers/virt-pci.c, virtio-gpio,
> virtio-net, ... drivers) you can actually connect it to virtual machines
> running as UML or in qemu.

I'm aware of vhost-user, but AFAICS QEMU needs glue for each device type
to be able to actually hook up vhost-user implementations to the devices
it exposes to the guest via the virtio PCI device.  See e.g.
hw/virtio/vhost-user-i2c-pci.c and hw/virtio/vhost-user-i2c.c in QEMU.

That is what I meant was missing for virtio-gpio, there seems to be an
in-progress patch set for that here though:
 https://lore.kernel.org/all/cover.1641987128.git.viresh.kumar@linaro.org/

Similarly, glue for something like arch/um/drivers/virt-pci.c does not
exist in QEMU.

Or perhaps you are implying that hw/virtio/vhost-user-i2c* in QEMU are
not strictly needed?

> (Actually, that's not strictly true today since it's
> arch/um/drivers/virt-pci.c and I didn't get a proper device ID assigned
> etc since it was for experimentation, I guess if we make this more
> commonly used then we should move it to drivers/pci/controller/virtio-
> pci.c and actually specify it in the OASIS virtio spec., at the very
> least it'd have to be possible to compile this and lib/logic_iomem.c on
> x86, but that's possible. Anyway I think PCI(e) is probably low on your
> list of things ...)

PCI is not that interesting, no, but platform devices are.  I did some
experiments early on with arch/um/drivers/virt-pci.c and a corresponding
backend along with a simple PCI driver which probes all devicetree nodes
under it, and I was able to use this to get some platform drivers
working.

> 
> >  - Also, some paths in this driver needs a modification to be tested
> >    under roadtest.  It uses wait_event_timeout() with a fixed value, but
> >    we cannot guarantee that this constraint is met in the test
> >    environment since it depends on things like CPU load on the host.
> > 
> >    (Also, we use UML's "time travel" feature which essentially
> >    fast-forwards through idle time, so the constraint can never be met
> >    in practice.)
> 
> Wohoo! This makes me very happy, finally somebody else who uses it :-)

Yes, thanks for that feature, it works well to speed up tests and also
has a knack for triggering race conditions (the RTC use-after-free for
example).

Time travel however sometimes triggers some WARN_ONs from the core
timekeeping code. I haven't seen them when running the test suites, but
they show up if the system under UML is idle for several (wall time)
seconds.  I haven't had a chance to investigate it further though, but I
can dig up the splats if you are interested.

On Tue, 2022-03-29 at 16:43 +0200, Vincent Whitchurch wrote:
> 
> I'm aware of vhost-user, but AFAICS QEMU needs glue for each device type
> to be able to actually hook up vhost-user implementations to the devices
> it exposes to the guest via the virtio PCI device.  See e.g.
> hw/virtio/vhost-user-i2c-pci.c and hw/virtio/vhost-user-i2c.c in QEMU.

Oh, I wasn't aware of that.

> That is what I meant was missing for virtio-gpio, there seems to be an
> in-progress patch set for that here though:
>  https://lore.kernel.org/all/cover.1641987128.git.viresh.kumar@linaro.org/
> 
> Similarly, glue for something like arch/um/drivers/virt-pci.c does not
> exist in QEMU.
> 
> Or perhaps you are implying that hw/virtio/vhost-user-i2c* in QEMU are
> not strictly needed?

I _thought_ that was the case, but honestly, that was just from reading
about it, not looking at the code. Thinking about it though, I don't
need special glue in UML, just passing the device ID on the command
line? So not sure what they need the glue for. Looking at the code, it's
not really much though? Not sure, I guess you need somebody more
familiar with qemu here, sorry.

> > Wohoo! This makes me very happy, finally somebody else who uses it :-)
> 
> Yes, thanks for that feature, it works well to speed up tests and also
> has a knack for triggering race conditions (the RTC use-after-free for
> example).
> 
> Time travel however sometimes triggers some WARN_ONs from the core
> timekeeping code. I haven't seen them when running the test suites, but
> they show up if the system under UML is idle for several (wall time)
> seconds.  I haven't had a chance to investigate it further though, but I
> can dig up the splats if you are interested.

Oh, I haven't seen that, and I'm pretty sure I've had systems idle for
very long periods of time passing inside (think weeks) ...

So yeah, if you have some splats (ideally with corresponding kernel
configs), I'd be interested.

johannes

On Tue, 2022-03-29 at 16:50 +0200, Johannes Berg wrote:
> > Or perhaps you are implying that hw/virtio/vhost-user-i2c* in QEMU are
> > not strictly needed?
> 
> I _thought_ that was the case, but honestly, that was just from reading
> about it, not looking at the code. Thinking about it though, I don't
> need special glue in UML, just passing the device ID on the command
> line? So not sure what they need the glue for. Looking at the code, it's
> not really much though? Not sure, I guess you need somebody more
> familiar with qemu here, sorry.
> 

So here
https://www.qemu.org/docs/master/system/devices/vhost-user.html#vhost-user-device

the docs say:

   These are simple stub devices that ensure the VirtIO device is
   visible to the guest. The code is mostly boilerplate although each
   device has a chardev option which specifies the ID of the --chardev
   device that connects via a socket to the vhost-user daemon.

So maybe if the ID were specified via the command line too, you could
have a generic vhost-user stub in qemu?

johannes