| Message ID | 20231217173548.112701-1-jic23@kernel.org (mailing list archive) |
|---|---|
| Headers | show |
| Series | IIO: Use the new cleanup.h magic | expand |
On Sun, Dec 17, 2023 at 11:36 AM Jonathan Cameron <jic23@kernel.org> wrote: > > From: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > A lot of the advantages of the automated cleanup added for locks and similar > are not that useful in IIO unless we also deal with the > iio_device_claim_direct_mode() / iio_device_release_direct_mode() > calls that prevent IIO device drivers from transitioning into buffered > mode whilst calls are in flight + prevent sysfs reads and writes from > interfering with buffered capture if it is enabled. > > Relies on Peter Zilstra's conditional cleanup handling which is queued > up for the merge window in the tip tree. This series is based on > a merge of tip/master into iio/togreg. > > All comments welcome. If this looks positive I'll make use of it in a > lot more drivers, but hopefully these give an idea of how it will work. > > The need to always handle what happens after > iio_device_claim_direct_scoped() {} is a little irritating but the > compiler will warn if you don't do it and it's not obvious how to > let the compiler know the magic loop (hidden in the cleanup.h macros) > always runs once. Example: > > iio_device_claim_direct_scoped(return -EBUSY, indio_dev) { > return 42; > } > /* Can't actually get here, but compiler moans if no return val */ > return -EINVAL; Maybe better would be? unreachable(); > }
On Sun, 17 Dec 2023 19:10:48 -0600 David Lechner <dlechner@baylibre.com> wrote: > On Sun, Dec 17, 2023 at 11:36 AM Jonathan Cameron <jic23@kernel.org> wrote: > > > > From: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > > > A lot of the advantages of the automated cleanup added for locks and similar > > are not that useful in IIO unless we also deal with the > > iio_device_claim_direct_mode() / iio_device_release_direct_mode() > > calls that prevent IIO device drivers from transitioning into buffered > > mode whilst calls are in flight + prevent sysfs reads and writes from > > interfering with buffered capture if it is enabled. > > > > Relies on Peter Zilstra's conditional cleanup handling which is queued > > up for the merge window in the tip tree. This series is based on > > a merge of tip/master into iio/togreg. > > > > All comments welcome. If this looks positive I'll make use of it in a > > lot more drivers, but hopefully these give an idea of how it will work. > > > > The need to always handle what happens after > > iio_device_claim_direct_scoped() {} is a little irritating but the > > compiler will warn if you don't do it and it's not obvious how to > > let the compiler know the magic loop (hidden in the cleanup.h macros) > > always runs once. Example: > > > > iio_device_claim_direct_scoped(return -EBUSY, indio_dev) { > > return 42; > > } > > /* Can't actually get here, but compiler moans if no return val */ > > return -EINVAL; > > Maybe better would be? > > unreachable(); Interesting thought, but there is very little precedence for using that in the kernel. + I think it's a C23 feature so we'd be relying on whether gcc and clang happened to implement it rather than being sure it was available. Jonathan > > > }
On Sun, 14 Jan 2024 17:33:36 +0000 Jonathan Cameron <jic23@kernel.org> wrote: > On Sun, 17 Dec 2023 19:10:48 -0600 > David Lechner <dlechner@baylibre.com> wrote: > > > On Sun, Dec 17, 2023 at 11:36 AM Jonathan Cameron <jic23@kernel.org> wrote: > > > > > > From: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > > > > > A lot of the advantages of the automated cleanup added for locks and similar > > > are not that useful in IIO unless we also deal with the > > > iio_device_claim_direct_mode() / iio_device_release_direct_mode() > > > calls that prevent IIO device drivers from transitioning into buffered > > > mode whilst calls are in flight + prevent sysfs reads and writes from > > > interfering with buffered capture if it is enabled. > > > > > > Relies on Peter Zilstra's conditional cleanup handling which is queued > > > up for the merge window in the tip tree. This series is based on > > > a merge of tip/master into iio/togreg. > > > > > > All comments welcome. If this looks positive I'll make use of it in a > > > lot more drivers, but hopefully these give an idea of how it will work. > > > > > > The need to always handle what happens after > > > iio_device_claim_direct_scoped() {} is a little irritating but the > > > compiler will warn if you don't do it and it's not obvious how to > > > let the compiler know the magic loop (hidden in the cleanup.h macros) > > > always runs once. Example: > > > > > > iio_device_claim_direct_scoped(return -EBUSY, indio_dev) { > > > return 42; > > > } > > > /* Can't actually get here, but compiler moans if no return val */ > > > return -EINVAL; > > > > Maybe better would be? > > > > unreachable(); > > Interesting thought, but there is very little precedence for using that in the kernel. > + I think it's a C23 feature so we'd be relying on whether gcc and clang happened > to implement it rather than being sure it was available. Ah. I'd missed the default implementation in compiler.h. So let us fall back on the first argument of limited precedence. J > > Jonathan > > > > > > > } >
On Sun, Jan 14, 2024 at 11:39 AM Jonathan Cameron <jic23@kernel.org> wrote: > > On Sun, 14 Jan 2024 17:33:36 +0000 > Jonathan Cameron <jic23@kernel.org> wrote: > > > On Sun, 17 Dec 2023 19:10:48 -0600 > > David Lechner <dlechner@baylibre.com> wrote: > > > > > On Sun, Dec 17, 2023 at 11:36 AM Jonathan Cameron <jic23@kernel.org> wrote: > > > > > > > > From: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > > > > > > > A lot of the advantages of the automated cleanup added for locks and similar > > > > are not that useful in IIO unless we also deal with the > > > > iio_device_claim_direct_mode() / iio_device_release_direct_mode() > > > > calls that prevent IIO device drivers from transitioning into buffered > > > > mode whilst calls are in flight + prevent sysfs reads and writes from > > > > interfering with buffered capture if it is enabled. > > > > > > > > Relies on Peter Zilstra's conditional cleanup handling which is queued > > > > up for the merge window in the tip tree. This series is based on > > > > a merge of tip/master into iio/togreg. > > > > > > > > All comments welcome. If this looks positive I'll make use of it in a > > > > lot more drivers, but hopefully these give an idea of how it will work. > > > > > > > > The need to always handle what happens after > > > > iio_device_claim_direct_scoped() {} is a little irritating but the > > > > compiler will warn if you don't do it and it's not obvious how to > > > > let the compiler know the magic loop (hidden in the cleanup.h macros) > > > > always runs once. Example: > > > > > > > > iio_device_claim_direct_scoped(return -EBUSY, indio_dev) { > > > > return 42; > > > > } > > > > /* Can't actually get here, but compiler moans if no return val */ > > > > return -EINVAL; > > > > > > Maybe better would be? > > > > > > unreachable(); > > > > Interesting thought, but there is very little precedence for using that in the kernel. > > + I think it's a C23 feature so we'd be relying on whether gcc and clang happened > > to implement it rather than being sure it was available. > > Ah. I'd missed the default implementation in compiler.h. > So let us fall back on the first argument of limited precedence. Couldn't the same be said about limited precedence for cleanup.h? This seems like a new sort of problem we don't usually have in kernel code so requiring an uncommon solution doesn't seem entirely out of place. It just seems to me like the natural solution here. It's self documenting and should help the compiler do a better job optimizing code.
From: Jonathan Cameron <Jonathan.Cameron@huawei.com> A lot of the advantages of the automated cleanup added for locks and similar are not that useful in IIO unless we also deal with the iio_device_claim_direct_mode() / iio_device_release_direct_mode() calls that prevent IIO device drivers from transitioning into buffered mode whilst calls are in flight + prevent sysfs reads and writes from interfering with buffered capture if it is enabled. Relies on Peter Zilstra's conditional cleanup handling which is queued up for the merge window in the tip tree. This series is based on a merge of tip/master into iio/togreg. All comments welcome. If this looks positive I'll make use of it in a lot more drivers, but hopefully these give an idea of how it will work. The need to always handle what happens after iio_device_claim_direct_scoped() {} is a little irritating but the compiler will warn if you don't do it and it's not obvious how to let the compiler know the magic loop (hidden in the cleanup.h macros) always runs once. Example: iio_device_claim_direct_scoped(return -EBUSY, indio_dev) { return 42; } /* Can't actually get here, but compiler moans if no return val */ return -EINVAL; } Assuming no show stoppers, I'll post a non RFC version next cycle and if I get sufficiently bored over the holidays it may include a few more users. Jonathan Cameron (10): iio: locking: introduce __cleanup() based direct mode claiming infrastructure iio: dummy: Use automatic lock and direct mode cleanup. iio: accel: adxl367: Use automated cleanup for locks and iio direct mode. iio: imu: bmi323: Use cleanup handling for iio_device_claim_direct_mode() iio: adc: max1363: Use automatic cleanup for locks and iio mode claiming. iio: proximity: sx9360: Use automated cleanup for locks and IIO mode claiming. iio: proximity: sx9324: Use automated cleanup for locks and IIO mode claiming. iio: proximity: sx9310: Use automated cleanup for locks and IIO mode claiming. iio: adc: ad4130: Use automatic cleanup of locks and direct mode. iio: adc: ad7091r-base: Use auto cleanup of locks. drivers/iio/accel/adxl367.c | 261 +++++++++++---------------- drivers/iio/adc/ad4130.c | 128 ++++++------- drivers/iio/adc/ad7091r-base.c | 25 +-- drivers/iio/adc/max1363.c | 149 +++++++-------- drivers/iio/dummy/iio_simple_dummy.c | 189 ++++++++++--------- drivers/iio/imu/bmi323/bmi323_core.c | 53 ++---- drivers/iio/industrialio-core.c | 4 + drivers/iio/proximity/sx9310.c | 114 ++++-------- drivers/iio/proximity/sx9324.c | 107 ++++------- drivers/iio/proximity/sx9360.c | 111 ++++-------- include/linux/iio/iio.h | 22 +++ 11 files changed, 481 insertions(+), 682 deletions(-)