| Message ID | 20221004134909.1692021-14-nuno.sa@analog.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Make 'mlock' really private | expand |
On Tue, Oct 4, 2022 at 4:49 PM Nuno Sá <nuno.sa@analog.com> wrote: > > These APIs are analogous to iio_device_claim_direct_mode() and > iio_device_release_direct_mode() but, as the name suggests, with the > logic flipped. While this looks odd enough, it will have at least two > users (in following changes) and it will be important to move the iio > mlock to the private struct. ... > +int iio_device_claim_buffer_mode(struct iio_dev *indio_dev) > +{ > + mutex_lock(&indio_dev->mlock); > + > + if (iio_buffer_enabled(indio_dev)) Do you need to annotate these two APIs to make sparse happy about locking balance? (Try to run `make W=1 C=1 ...` with your patches and look if any new warnings appear.) > + return 0; > + > + mutex_unlock(&indio_dev->mlock); > + return -EBUSY; > +} ... > +void iio_device_release_buffer_mode(struct iio_dev *indio_dev) > +{ > + mutex_unlock(&indio_dev->mlock); > +}
On Tue, 2022-10-04 at 17:08 +0300, Andy Shevchenko wrote: > On Tue, Oct 4, 2022 at 4:49 PM Nuno Sá <nuno.sa@analog.com> wrote: > > > > These APIs are analogous to iio_device_claim_direct_mode() and > > iio_device_release_direct_mode() but, as the name suggests, with > > the > > logic flipped. While this looks odd enough, it will have at least > > two > > users (in following changes) and it will be important to move the > > iio > > mlock to the private struct. > > ... > > > +int iio_device_claim_buffer_mode(struct iio_dev *indio_dev) > > +{ > > + mutex_lock(&indio_dev->mlock); > > + > > + if (iio_buffer_enabled(indio_dev)) > > Do you need to annotate these two APIs to make sparse happy about > locking balance? > > (Try to run `make W=1 C=1 ...` with your patches and look if any new > warnings appear.) make W=1 C=1 drivers/iio/industrialio-core.o # UPD include/config/kernel.release UPD include/generated/utsrelease.h CALL scripts/checksyscalls.sh CALL scripts/atomic/check-atomics.sh CC drivers/iio/industrialio-core.o CHECK drivers/iio/industrialio-core.c drivers/iio/industrialio-core.c: note: in included file (through include/linux/bitops.h, include/linux/kernel.h): ./include/asm-generic/bitops/generic-non-atomic.h:30:9: warning: unreplaced symbol 'mask' ./include/asm-generic/bitops/generic-non-atomic.h:31:9: warning: unreplaced symbol 'p' ./include/asm-generic/bitops/generic-non-atomic.h:33:10: warning: unreplaced symbol 'p' ./include/asm-generic/bitops/generic-non-atomic.h:33:16: warning: unreplaced symbol 'mask' ./include/asm-generic/bitops/generic-non-atomic.h:28:1: warning: unreplaced symbol 'return' ./include/asm-generic/bitops/generic-non-atomic.h:39:9: warning: unreplaced symbol 'mask' ./include/asm-generic/bitops/generic-non-atomic.h:40:9: warning: unreplaced symbol 'p' ./include/asm-generic/bitops/generic-non-atomic.h:42:10: warning: unreplaced symbol 'p' ./include/asm-generic/bitops/generic-non-atomic.h:42:16: warning: unreplaced symbol 'mask' ./include/asm-generic/bitops/generic-non-atomic.h:37:1: warning: unreplaced symbol 'return' ./include/asm-generic/bitops/generic-non-atomic.h:57:9: warning: unreplaced symbol 'mask' ./include/asm-generic/bitops/generic-non-atomic.h:58:9: warning: unreplaced symbol 'p' ./include/asm-generic/bitops/generic-non-atomic.h:60:10: warning: unreplaced symbol 'p' ./include/asm-generic/bitops/generic-non-atomic.h:60:15: warning: unreplaced symbol 'mask' ./include/asm-generic/bitops/generic-non-atomic.h:55:1: warning: unreplaced symbol 'return' ./include/asm-generic/bitops/generic-non-atomic.h:75:9: warning: unreplaced symbol 'mask' ./include/asm-generic/bitops/generic-non-atomic.h:76:9: warning: unreplaced symbol 'p' ./include/asm-generic/bitops/generic-non-atomic.h:77:9: warning: unreplaced symbol 'old' ./include/asm-generic/bitops/generic-non-atomic.h:79:10: warning: unreplaced symbol 'p' ./include/asm-generic/bitops/generic-non-atomic.h:79:14: warning: unreplaced symbol 'old' ./include/asm-generic/bitops/generic-non-atomic.h:79:20: warning: unreplaced symbol 'mask' ./include/asm-generic/bitops/generic-non-atomic.h:80:17: warning: unreplaced symbol 'old' ./include/asm-generic/bitops/generic-non-atomic.h:80:23: warning: unreplaced symbol 'mask' ./include/asm-generic/bitops/generic-non-atomic.h:80:9: warning: unreplaced symbol 'return' ./include/asm-generic/bitops/generic-non-atomic.h:73:1: warning: unreplaced symbol 'return' ./include/asm-generic/bitops/generic-non-atomic.h:95:9: warning: unreplaced symbol 'mask' ./include/asm-generic/bitops/generic-non-atomic.h:96:9: warning: unreplaced symbol 'p' ./include/asm-generic/bitops/generic-non-atomic.h:97:9: warning: unreplaced symbol 'old' ./include/asm-generic/bitops/generic-non-atomic.h:99:10: warning: unreplaced symbol 'p' ./include/asm-generic/bitops/generic-non-atomic.h:99:14: warning: unreplaced symbol 'old' ./include/asm-generic/bitops/generic-non-atomic.h:99:21: warning: unreplaced symbol 'mask' ./include/asm-generic/bitops/generic-non-atomic.h:100:17: warning: unreplaced symbol 'old' ./include/asm-generic/bitops/generic-non-atomic.h:100:23: warning: unreplaced symbol 'mask' ./include/asm-generic/bitops/generic-non-atomic.h:100:9: warning: unreplaced symbol 'return' ./include/asm-generic/bitops/generic-non-atomic.h:93:1: warning: unreplaced symbol 'return' ./include/asm-generic/bitops/generic-non-atomic.h:107:9: warning: unreplaced symbol 'mask' ./include/asm-generic/bitops/generic-non-atomic.h:108:9: warning: unreplaced symbol 'p' ./include/asm-generic/bitops/generic-non-atomic.h:109:9: warning: unreplaced symbol 'old' ./include/asm-generic/bitops/generic-non-atomic.h:111:10: warning: unreplaced symbol 'p' ./include/asm-generic/bitops/generic-non-atomic.h:111:14: warning: unreplaced symbol 'old' ./include/asm-generic/bitops/generic-non-atomic.h:111:20: warning: unreplaced symbol 'mask' ./include/asm-generic/bitops/generic-non-atomic.h:112:17: warning: unreplaced symbol 'old' ./include/asm-generic/bitops/generic-non-atomic.h:112:23: warning: unreplaced symbol 'mask' ./include/asm-generic/bitops/generic-non-atomic.h:112:9: warning: unreplaced symbol 'return' ./include/asm-generic/bitops/generic-non-atomic.h:105:1: warning: unreplaced symbol 'return' ./include/asm-generic/bitops/generic-non-atomic.h:128:9: warning: unreplaced symbol 'return' ./include/asm-generic/bitops/generic-non-atomic.h:121:1: warning: unreplaced symbol 'return' ./include/asm-generic/bitops/generic-non-atomic.h:168:9: warning: unreplaced symbol 'p' ./include/asm-generic/bitops/generic-non-atomic.h:169:9: warning: unreplaced symbol 'mask' ./include/asm-generic/bitops/generic-non-atomic.h:170:9: warning: unreplaced symbol 'val' ./include/asm-generic/bitops/generic-non-atomic.h:172:19: warning: unreplaced symbol 'val' ./include/asm-generic/bitops/generic-non-atomic.h:172:25: warning: unreplaced symbol 'mask' ./include/asm-generic/bitops/generic-non-atomic.h:172:9: warning: unreplaced symbol 'return' ./include/asm-generic/bitops/generic-non-atomic.h:166:1: warning: unreplaced symbol 'return' drivers/iio/industrialio-core.c:2100: warning: expecting prototype for iio_device_claim_buffered_mode(). Prototype was for iio_device_claim_buffer_mode() instead Don't really see anything odd in here... Am I missing something? Anyways, I guess you mean annotations as __acquires() and __releases()... Well, this API is pretty much analogous to iio_device_claim_direct_mode() which also don't have such annotations. Thus, I would say to add them (if we are going too) in a future patch to both APIs... Also fine with adding them now if Jonathan feels it's necessary. - Nuno Sá
On Wed, 05 Oct 2022 10:37:39 +0200 Nuno Sá <noname.nuno@gmail.com> wrote: > On Tue, 2022-10-04 at 17:08 +0300, Andy Shevchenko wrote: > > On Tue, Oct 4, 2022 at 4:49 PM Nuno Sá <nuno.sa@analog.com> wrote: > > > > > > These APIs are analogous to iio_device_claim_direct_mode() and > > > iio_device_release_direct_mode() but, as the name suggests, with > > > the > > > logic flipped. While this looks odd enough, it will have at least > > > two > > > users (in following changes) and it will be important to move the > > > iio > > > mlock to the private struct. > > > > ... > > > > > +int iio_device_claim_buffer_mode(struct iio_dev *indio_dev) > > > +{ > > > + mutex_lock(&indio_dev->mlock); > > > + > > > + if (iio_buffer_enabled(indio_dev)) > > > > Do you need to annotate these two APIs to make sparse happy about > > locking balance? > > > > (Try to run `make W=1 C=1 ...` with your patches and look if any new > > warnings appear.) > > make W=1 C=1 drivers/iio/industrialio-core.o > # UPD include/config/kernel.release ... > drivers/iio/industrialio-core.c:2100: warning: expecting prototype for > iio_device_claim_buffered_mode(). Prototype was for > iio_device_claim_buffer_mode() instead That one wants fixing as this patch introduces it. > > Don't really see anything odd in here... Am I missing something? > > Anyways, I guess you mean annotations as __acquires() and > __releases()... Well, this API is pretty much analogous to > iio_device_claim_direct_mode() which also don't have such annotations. > Thus, I would say to add them (if we are going too) in a future patch > to both APIs... > > Also fine with adding them now if Jonathan feels it's necessary. I've wondered for a while why we don't get reports as a result of those not being annotated. However, follow up patch probably makes sense rather than rolling it into this series. Jonathan >
On Sun, 2022-10-09 at 12:41 +0100, Jonathan Cameron wrote: > On Wed, 05 Oct 2022 10:37:39 +0200 > Nuno Sá <noname.nuno@gmail.com> wrote: > > > On Tue, 2022-10-04 at 17:08 +0300, Andy Shevchenko wrote: > > > On Tue, Oct 4, 2022 at 4:49 PM Nuno Sá <nuno.sa@analog.com> > > > wrote: > > > > > > > > These APIs are analogous to iio_device_claim_direct_mode() and > > > > iio_device_release_direct_mode() but, as the name suggests, > > > > with > > > > the > > > > logic flipped. While this looks odd enough, it will have at > > > > least > > > > two > > > > users (in following changes) and it will be important to move > > > > the > > > > iio > > > > mlock to the private struct. > > > > > > ... > > > > > > > +int iio_device_claim_buffer_mode(struct iio_dev *indio_dev) > > > > +{ > > > > + mutex_lock(&indio_dev->mlock); > > > > + > > > > + if (iio_buffer_enabled(indio_dev)) > > > > > > Do you need to annotate these two APIs to make sparse happy about > > > locking balance? > > > > > > (Try to run `make W=1 C=1 ...` with your patches and look if any > > > new > > > warnings appear.) > > > > make W=1 C=1 drivers/iio/industrialio-core.o > > # UPD include/config/kernel.release > > ... > > > drivers/iio/industrialio-core.c:2100: warning: expecting prototype > > for > > iio_device_claim_buffered_mode(). Prototype was for > > iio_device_claim_buffer_mode() instead > > That one wants fixing as this patch introduces it. > Bah, That's why another pair of eyes is useful... I looked for that warning without seeing what it was complaining about. Now, I could finally see it :) - Nuno Sá >
diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c index 151ff3993354..c23d3abb33c5 100644 --- a/drivers/iio/industrialio-core.c +++ b/drivers/iio/industrialio-core.c @@ -2083,6 +2083,44 @@ void iio_device_release_direct_mode(struct iio_dev *indio_dev) } EXPORT_SYMBOL_GPL(iio_device_release_direct_mode); +/** + * iio_device_claim_buffered_mode - Keep device in buffer mode + * @indio_dev: the iio_dev associated with the device + * + * If the device is in buffer mode it is guaranteed to stay + * that way until iio_device_release_buffer_mode() is called. + * + * Use with iio_device_release_buffer_mode() + * + * Returns: 0 on success, -EBUSY on failure + */ +int iio_device_claim_buffer_mode(struct iio_dev *indio_dev) +{ + mutex_lock(&indio_dev->mlock); + + if (iio_buffer_enabled(indio_dev)) + return 0; + + mutex_unlock(&indio_dev->mlock); + return -EBUSY; +} +EXPORT_SYMBOL_GPL(iio_device_claim_buffer_mode); + +/** + * iio_device_release_buffer_mode - releases claim on buffer mode + * @indio_dev: the iio_dev associated with the device + * + * Release the claim. Device is no longer guaranteed to stay + * in buffer mode. + * + * Use with iio_device_claim_buffer_mode() + */ +void iio_device_release_buffer_mode(struct iio_dev *indio_dev) +{ + mutex_unlock(&indio_dev->mlock); +} +EXPORT_SYMBOL_GPL(iio_device_release_buffer_mode); + /** * iio_device_get_current_mode() - helper function providing read-only access to * the opaque @currentmode variable diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h index f0ec8a5e5a7a..9d3bd6379eb8 100644 --- a/include/linux/iio/iio.h +++ b/include/linux/iio/iio.h @@ -629,6 +629,8 @@ int __devm_iio_device_register(struct device *dev, struct iio_dev *indio_dev, int iio_push_event(struct iio_dev *indio_dev, u64 ev_code, s64 timestamp); int iio_device_claim_direct_mode(struct iio_dev *indio_dev); void iio_device_release_direct_mode(struct iio_dev *indio_dev); +int iio_device_claim_buffer_mode(struct iio_dev *indio_dev); +void iio_device_release_buffer_mode(struct iio_dev *indio_dev); extern struct bus_type iio_bus_type;
These APIs are analogous to iio_device_claim_direct_mode() and iio_device_release_direct_mode() but, as the name suggests, with the logic flipped. While this looks odd enough, it will have at least two users (in following changes) and it will be important to move the iio mlock to the private struct. Signed-off-by: Nuno Sá <nuno.sa@analog.com> --- drivers/iio/industrialio-core.c | 38 +++++++++++++++++++++++++++++++++ include/linux/iio/iio.h | 2 ++ 2 files changed, 40 insertions(+)