| Message ID | 20220619185839.1363503-1-jic23@kernel.org (mailing list archive) |
|---|---|
| Headers | show |
| Series | staging/iio: Clean up AD7746 CDC driver and move from staging. | expand |
On Sun, Jun 19, 2022 at 8:58 PM Jonathan Cameron <jic23@kernel.org> wrote: > Vincent: I'm getting some instability with roadtest after forwards porting to > current mainline. Tests run 'sometimes'. Other times I get a crash > in um_set_signal. Seems unrelated to the test this series adds. > > Changes since v1: Thanks to Andy and Peter for reviews. > - Use sizeof(data) for be24 buffer data[3] > - Make the precision patch fall back to old maths if overflow would > have occured. > > This started out as an experiment with Vincent Whitchurch's roadtest > testing framework [1] and that worked well so I carried on cleaning up the > driver. > > Mostly this is standard tidy up, move to new interfaces, then move the driver > out of staging, but there are a few other things in here. > > 1) Precision improvement for IIO_VAL_FRACTIONAL_LOG2. > The ad7746 is a 24 bit sensor and this highlighted that 9 decimal > places wasn't enough to keep the error introduced by > _raw * _scale from growing too large over the whole range (I couldn't > write a sensible test for it). So I've proposed increasing the precision > of the calculation used to provide the sysfs attributes with this value > type and printing a few more decimal places (12), but only if overflow > will not occur due to val[0] > ULLONG_MAX / PICO > 2) _inputoffset ABI addition. This driver had an odd use of _offset for > the case of differential channels that applied the offset to both > of the differential pair (hence usespace shouldn't not apply it when userspace? > converting to the base units. That isn't inline with the existing > documentation for _offset and it wasn't clear to me that it made sense > at all. To avoid confusion I've added this new ABI (_inputoffset) for t > 3) roadtest file - note this is not a complete test set for the driver and > mainly focused on the main channel reads and places I thought I might > have broken things whilst working on the driver. > > My conclusion on roadtest - Very useful indeed. I'd encourage others to > consider developing some basic sanity tests for drivers they are working on. > Hopefully my python code isn't too hideous to understand at least! > Vincent, it might be worth thinking about some generic code to handle the > 'variants' on correct ABI like I introduce here because I switched from > a shared by type scale to an individual one per channel for the voltages. > Both were ABI compliant so that sort of change is fine most of the time > though we have to be careful with it. > > All comments welcome. Note there may be changes that make more sense > to do after moving this out of staging as long as there are no ABI changes involved > etc. Feel free to highlight those sorts of changes as well as anything more > significant. Overall it looks good to me. One Q though, do you plan to switch it to regmap APIs? ... > staging: iio: cdc: ad7746: Drop usused i2c_set_clientdata() unused
> > Overall it looks good to me. One Q though, do you plan to switch it to > regmap APIs? Probably not unless doing some more significant work on the driver for some other reason. It's definitely not yet a requirement that IIO drivers use regmap, even though it is of course desirable where it fits well. Given it should be fine to do the conversion and rely on roadtest to see if anything isn't broken, converting this one might be a good first patch for someone who is interested in writing modern drivers. Thanks, Jonathan
On Mon, Jun 20, 2022 at 01:23:39AM +0200, Andy Shevchenko wrote: > On Sun, Jun 19, 2022 at 8:58 PM Jonathan Cameron <jic23@kernel.org> wrote: > > > Vincent: I'm getting some instability with roadtest after forwards porting to > > current mainline. Tests run 'sometimes'. Other times I get a crash > > in um_set_signal. Seems unrelated to the test this series adds. > > > > Changes since v1: Thanks to Andy and Peter for reviews. > > - Use sizeof(data) for be24 buffer data[3] > > - Make the precision patch fall back to old maths if overflow would > > have occured. > > > > This started out as an experiment with Vincent Whitchurch's roadtest > > testing framework [1] and that worked well so I carried on cleaning up the > > driver. > > > > Mostly this is standard tidy up, move to new interfaces, then move the driver > > out of staging, but there are a few other things in here. > > > > 1) Precision improvement for IIO_VAL_FRACTIONAL_LOG2. > > The ad7746 is a 24 bit sensor and this highlighted that 9 decimal > > places wasn't enough to keep the error introduced by > > _raw * _scale from growing too large over the whole range (I couldn't > > write a sensible test for it). So I've proposed increasing the precision > > of the calculation used to provide the sysfs attributes with this value > > type and printing a few more decimal places (12), but only if overflow > > will not occur due to val[0] > ULLONG_MAX / PICO > > 2) _inputoffset ABI addition. This driver had an odd use of _offset for > > the case of differential channels that applied the offset to both > > of the differential pair (hence usespace shouldn't not apply it when > > userspace? > Also, there's a confusing double negative. Either "should" or "should not". Thanks, Joe
On Sun, Jun 19, 2022 at 08:58:22PM +0200, Jonathan Cameron wrote: > Vincent: I'm getting some instability with roadtest after forwards porting to > current mainline. Tests run 'sometimes'. Other times I get a crash > in um_set_signal. Seems unrelated to the test this series adds. My local work-in-progress version has a lot of changes since the RFC, but yes, I'm seeing a splat too (below) if I use the RFC version on v5.19-rc3. The problem goes away if I enable CONFIG_UML_RANDOM=y, as I have done in my local branch: diff --git a/tools/testing/roadtest/roadtest/tests/base/config b/tools/testing/roadtest/roadtest/tests/base/config index c1952d047c8e..74b201c48d42 100644 --- a/tools/testing/roadtest/roadtest/tests/base/config +++ b/tools/testing/roadtest/roadtest/tests/base/config @@ -4,6 +4,7 @@ CONFIG_LOG_BUF_SHIFT=14 CONFIG_EXPERT=y CONFIG_HOSTFS=y CONFIG_UML_TIME_TRAVEL_SUPPORT=y +CONFIG_UML_RANDOM=y CONFIG_NULL_CHAN=y CONFIG_PORT_CHAN=y CONFIG_PTY_CHAN=y [ 547.130000][ T21] BUG: failure at arch/um/kernel/time.c:415/time_travel_update_time()! [ 547.130000][ T21] Kernel panic - not syncing: BUG! [ 547.130000][ T21] CPU: 0 PID: 21 Comm: python3 Not tainted 5.19.0-rc3-00018-gcb2e63986f7f #77 [ 547.130000][ T21] Stack: [ 547.130000][ T21] 605860dd 60290b07 60037130 00000000 [ 547.130000][ T21] 605860dd 00000000 607a39d0 60426253 [ 547.130000][ T21] 6064d1a8 00000000 604106ad 60557d16 [ 547.130000][ T21] Call Trace: [ 547.130000][ T21] [<60290b07>] ? dump_stack_print_info+0xd7/0xf0 [ 547.130000][ T21] [<60037130>] ? um_set_signals+0x0/0x50 [ 547.130000][ T21] [<60426253>] ? dump_stack_lvl+0x5b/0x7a [ 547.130000][ T21] [<604106ad>] ? _printk+0x0/0x9b [ 547.130000][ T21] [<60025440>] ? time_travel_del_event+0x0/0xc0 [ 547.130000][ T21] [<6040b016>] ? panic+0x1a8/0x372 [ 547.130000][ T21] [<6040ae6e>] ? panic+0x0/0x372 [ 547.130000][ T21] [<604106ad>] ? _printk+0x0/0x9b [ 547.130000][ T21] [<60037130>] ? um_set_signals+0x0/0x50 [ 547.130000][ T21] [<60037174>] ? um_set_signals+0x44/0x50 [ 547.130000][ T21] [<60025440>] ? time_travel_del_event+0x0/0xc0 [ 547.130000][ T21] [<60409c35>] ? time_travel_update_time.cold+0x6c/0x133 [ 547.130000][ T21] [<6043b5f0>] ? __schedule+0x780/0x880 [ 547.130000][ T21] [<6043ae70>] ? __schedule+0x0/0x880 [ 547.130000][ T21] [<600bd220>] ? random_get_entropy_fallback+0x0/0x30 [ 547.130000][ T21] [<6003be10>] ? get_fp_registers+0x0/0x80 [ 547.130000][ T21] [<600258fd>] ? timer_read+0xbd/0xf0 [ 547.130000][ T21] [<60305fb0>] ? mix_pool_bytes+0x0/0x60 [ 547.130000][ T21] [<60305fb0>] ? mix_pool_bytes+0x0/0x60 [ 547.130000][ T21] [<60430d32>] ? try_to_generate_entropy+0x14d/0x164 [ 547.130000][ T21] [<60430f98>] ? entropy_timer+0x0/0x48 [ 547.130000][ T21] [<60431049>] ? urandom_read_iter.cold+0xc/0x11 [ 547.130000][ T21] [<60176b52>] ? new_sync_read+0xe2/0x150 [ 547.130000][ T21] [<60178dc2>] ? vfs_read+0xf2/0x200 [ 547.130000][ T21] [<601a51d5>] ? __fdget_pos+0x15/0x60 [ 547.130000][ T21] [<601793a1>] ? ksys_read+0x61/0xf0 [ 547.130000][ T21] [<600280ca>] ? handle_syscall+0xaa/0xf0 [ 547.130000][ T21] [<60039236>] ? userspace+0x346/0x570 [ 547.130000][ T21] [<60037130>] ? um_set_signals+0x0/0x50
On Wed, 22 Jun 2022 15:07:54 +0200 Vincent Whitchurch <vincent.whitchurch@axis.com> wrote: > On Sun, Jun 19, 2022 at 08:58:22PM +0200, Jonathan Cameron wrote: > > Vincent: I'm getting some instability with roadtest after forwards porting to > > current mainline. Tests run 'sometimes'. Other times I get a crash > > in um_set_signal. Seems unrelated to the test this series adds. > > My local work-in-progress version has a lot of changes since the RFC, > but yes, I'm seeing a splat too (below) if I use the RFC version on > v5.19-rc3. The problem goes away if I enable CONFIG_UML_RANDOM=y, as I > have done in my local branch: Thanks, that's indeed effective though I guess papering over a real problem. Works for me though :) Jonathan > > diff --git a/tools/testing/roadtest/roadtest/tests/base/config b/tools/testing/roadtest/roadtest/tests/base/config > index c1952d047c8e..74b201c48d42 100644 > --- a/tools/testing/roadtest/roadtest/tests/base/config > +++ b/tools/testing/roadtest/roadtest/tests/base/config > @@ -4,6 +4,7 @@ CONFIG_LOG_BUF_SHIFT=14 > CONFIG_EXPERT=y > CONFIG_HOSTFS=y > CONFIG_UML_TIME_TRAVEL_SUPPORT=y > +CONFIG_UML_RANDOM=y > CONFIG_NULL_CHAN=y > CONFIG_PORT_CHAN=y > CONFIG_PTY_CHAN=y > > [ 547.130000][ T21] BUG: failure at arch/um/kernel/time.c:415/time_travel_update_time()! > [ 547.130000][ T21] Kernel panic - not syncing: BUG! > [ 547.130000][ T21] CPU: 0 PID: 21 Comm: python3 Not tainted 5.19.0-rc3-00018-gcb2e63986f7f #77 > [ 547.130000][ T21] Stack: > [ 547.130000][ T21] 605860dd 60290b07 60037130 00000000 > [ 547.130000][ T21] 605860dd 00000000 607a39d0 60426253 > [ 547.130000][ T21] 6064d1a8 00000000 604106ad 60557d16 > [ 547.130000][ T21] Call Trace: > [ 547.130000][ T21] [<60290b07>] ? dump_stack_print_info+0xd7/0xf0 > [ 547.130000][ T21] [<60037130>] ? um_set_signals+0x0/0x50 > [ 547.130000][ T21] [<60426253>] ? dump_stack_lvl+0x5b/0x7a > [ 547.130000][ T21] [<604106ad>] ? _printk+0x0/0x9b > [ 547.130000][ T21] [<60025440>] ? time_travel_del_event+0x0/0xc0 > [ 547.130000][ T21] [<6040b016>] ? panic+0x1a8/0x372 > [ 547.130000][ T21] [<6040ae6e>] ? panic+0x0/0x372 > [ 547.130000][ T21] [<604106ad>] ? _printk+0x0/0x9b > [ 547.130000][ T21] [<60037130>] ? um_set_signals+0x0/0x50 > [ 547.130000][ T21] [<60037174>] ? um_set_signals+0x44/0x50 > [ 547.130000][ T21] [<60025440>] ? time_travel_del_event+0x0/0xc0 > [ 547.130000][ T21] [<60409c35>] ? time_travel_update_time.cold+0x6c/0x133 > [ 547.130000][ T21] [<6043b5f0>] ? __schedule+0x780/0x880 > [ 547.130000][ T21] [<6043ae70>] ? __schedule+0x0/0x880 > [ 547.130000][ T21] [<600bd220>] ? random_get_entropy_fallback+0x0/0x30 > [ 547.130000][ T21] [<6003be10>] ? get_fp_registers+0x0/0x80 > [ 547.130000][ T21] [<600258fd>] ? timer_read+0xbd/0xf0 > [ 547.130000][ T21] [<60305fb0>] ? mix_pool_bytes+0x0/0x60 > [ 547.130000][ T21] [<60305fb0>] ? mix_pool_bytes+0x0/0x60 > [ 547.130000][ T21] [<60430d32>] ? try_to_generate_entropy+0x14d/0x164 > [ 547.130000][ T21] [<60430f98>] ? entropy_timer+0x0/0x48 > [ 547.130000][ T21] [<60431049>] ? urandom_read_iter.cold+0xc/0x11 > [ 547.130000][ T21] [<60176b52>] ? new_sync_read+0xe2/0x150 > [ 547.130000][ T21] [<60178dc2>] ? vfs_read+0xf2/0x200 > [ 547.130000][ T21] [<601a51d5>] ? __fdget_pos+0x15/0x60 > [ 547.130000][ T21] [<601793a1>] ? ksys_read+0x61/0xf0 > [ 547.130000][ T21] [<600280ca>] ? handle_syscall+0xaa/0xf0 > [ 547.130000][ T21] [<60039236>] ? userspace+0x346/0x570 > [ 547.130000][ T21] [<60037130>] ? um_set_signals+0x0/0x50 >
From: Jonathan Cameron <Jonathan.Cameron@huawei.com> Vincent: I'm getting some instability with roadtest after forwards porting to current mainline. Tests run 'sometimes'. Other times I get a crash in um_set_signal. Seems unrelated to the test this series adds. Changes since v1: Thanks to Andy and Peter for reviews. - Use sizeof(data) for be24 buffer data[3] - Make the precision patch fall back to old maths if overflow would have occured. This started out as an experiment with Vincent Whitchurch's roadtest testing framework [1] and that worked well so I carried on cleaning up the driver. Mostly this is standard tidy up, move to new interfaces, then move the driver out of staging, but there are a few other things in here. 1) Precision improvement for IIO_VAL_FRACTIONAL_LOG2. The ad7746 is a 24 bit sensor and this highlighted that 9 decimal places wasn't enough to keep the error introduced by _raw * _scale from growing too large over the whole range (I couldn't write a sensible test for it). So I've proposed increasing the precision of the calculation used to provide the sysfs attributes with this value type and printing a few more decimal places (12), but only if overflow will not occur due to val[0] > ULLONG_MAX / PICO 2) _inputoffset ABI addition. This driver had an odd use of _offset for the case of differential channels that applied the offset to both of the differential pair (hence usespace shouldn't not apply it when converting to the base units. That isn't inline with the existing documentation for _offset and it wasn't clear to me that it made sense at all. To avoid confusion I've added this new ABI (_inputoffset) for t 3) roadtest file - note this is not a complete test set for the driver and mainly focused on the main channel reads and places I thought I might have broken things whilst working on the driver. My conclusion on roadtest - Very useful indeed. I'd encourage others to consider developing some basic sanity tests for drivers they are working on. Hopefully my python code isn't too hideous to understand at least! Vincent, it might be worth thinking about some generic code to handle the 'variants' on correct ABI like I introduce here because I switched from a shared by type scale to an individual one per channel for the voltages. Both were ABI compliant so that sort of change is fine most of the time though we have to be careful with it. All comments welcome. Note there may be changes that make more sense to do after moving this out of staging as long as there are no ABI changes involved etc. Feel free to highlight those sorts of changes as well as anything more significant. [1] https://lore.kernel.org/all/20220311162445.346685-9-vincent.whitchurch@axis.com/ Jonathan Cameron (17): iio: core: Increase precision of IIO_VAL_FRACTIONAL_LOG2 when possible iio: ABI: Fix wrong format of differential capacitance channel ABI. staging: iio: cdc: ad7746: Use explicit be24 handling. staging: iio: cdc: ad7746: Push handling of supply voltage scale to userspace. staging: iio: cdc: ad7746: Use local buffer for multi byte reads. staging: iio: cdc: ad7746: Factor out ad7746_read_channel() staging: iio: cdc: ad7764: Push locking down into case statements in read/write_raw staging: iio: cdc: ad7746: Break up use of chan->address and use FIELD_PREP etc staging: iio: cdc: ad7746: Drop usused i2c_set_clientdata() staging: iio: cdc: ad7746: Use _raw and _scale for temperature channels. iio: core: Introduce _inputoffset for differential channels staging: iio: cdc: ad7746: Switch from _offset to _inputoffset for differential channels. staging: iio: cdc: ad7746: Use read_avail() rather than opencoding. staging: iio: ad7746: White space cleanup iio: cdc: ad7746: Add device specific ABI documentation. iio: cdc: ad7746: Move driver out of staging. RFC: iio: cdc: ad7746: Add roadtest Documentation/ABI/testing/sysfs-bus-iio | 15 +- .../ABI/testing/sysfs-bus-iio-cdc-ad7746 | 11 + drivers/iio/cdc/Kconfig | 10 + drivers/iio/cdc/Makefile | 1 + drivers/iio/cdc/ad7746.c | 818 ++++++++++++++++++ drivers/iio/industrialio-core.c | 31 +- drivers/staging/iio/cdc/ad7746.c | 767 ---------------- include/linux/iio/types.h | 1 + .../roadtest/tests/iio/cdc/__init__.py | 0 .../roadtest/roadtest/tests/iio/cdc/config | 1 + .../roadtest/tests/iio/cdc/test_ad7746.py | 323 +++++++ 11 files changed, 1202 insertions(+), 776 deletions(-) create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-cdc-ad7746 create mode 100644 drivers/iio/cdc/ad7746.c delete mode 100644 drivers/staging/iio/cdc/ad7746.c create mode 100644 tools/testing/roadtest/roadtest/tests/iio/cdc/__init__.py create mode 100644 tools/testing/roadtest/roadtest/tests/iio/cdc/config create mode 100644 tools/testing/roadtest/roadtest/tests/iio/cdc/test_ad7746.py