Re: [PATCH v2 01/27] iio: core: Rework claim and release of direct mode to work with sparse.

[Jonathan Cameron <jic23@kernel.org>]

On Sat, 22 Feb 2025 17:51:02 +0200
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> Sun, Feb 09, 2025 at 06:05:58PM +0000, Jonathan Cameron kirjoitti:
> > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > 
> > Initial thought was to do something similar to __cond_lock()
> > 
> > 	do_iio_device_claim_direct_mode(iio_dev) ? : ({ __acquire(iio_dev); 0; })
> > + Appropriate static inline iio_device_release_direct_mode()
> > 
> > However with that, sparse generates false positives. E.g.
> > 
> > drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c:1811:17: warning: context imbalance in 'st_lsm6dsx_read_raw' - unexpected unlock
> > 
> > So instead, this patch rethinks the return type and makes it more
> > 'conditional lock like' (which is part of what is going on under the hood
> > anyway) and return a boolean - true for successfully acquired, false for
> > did not acquire.
> > 
> > To allow a migration path given the rework is now non trivial, take a leaf
> > out of the naming of the conditional guard we currently have for IIO
> > device direct mode and drop the _mode postfix from the new functions giving
> > iio_device_claim_direct() and iio_device_release_direct()
> > 
> > Whilst the kernel supports __cond_acquires() upstream sparse does not
> > yet do so.  Hence rely on sparse expanding a static inline wrapper
> > to explicitly see whether __acquire() is called.
> > 
> > Note that even with the solution here, sparse sometimes gives false
> > positives. However in the few cases seen they were complex code
> > structures that benefited from simplification anyway.  
> 
> ...
> 
> 
> > +/*
> > + * Helper functions that allow claim and release of direct mode
> > + * in a fashion that doesn't generate many false positives from sparse.
> > + * Note this must remain static inline in the header so that sparse
> > + * can see the __acquire() marking. Revisit when sparse supports
> > + * __cond_acquires()
> > + */
> > +static inline bool iio_device_claim_direct(struct iio_dev *indio_dev)
> > +{
> > +	int ret = iio_device_claim_direct_mode(indio_dev);
> > +
> > +	if (ret)
> > +		return false;
> > +
> > +	__acquire(iio_dev);
> > +
> > +	return true;  
> 
> While I understand the intention, I dislike the function return boolean and
> hide the actual error code, it calls user to misuse and replace boolean false
> by arbitrary error codes.
> 
> Can we rather return an error code, please?
> (as a side effect it reduces the churn in the followup changes)
> 
Hi Andy,

I tried - see above.  It plays badly with sparse which is the whole point of
this exercise. Note that iio_device_claim_direct_mode() only ever returns one
error code -EBUSY. So reality is it's a boolean and this is a lot close
to mutex_trylock() than anything else hence the switch to a boolean return.

At the end of the full series (not yet posted) is a patch that gets rid
of their being any pretence this isn't a yes / no question and can
return other error values. This intermediate step does leave it looking
more confusing.

Churn wise if we'd been able to do keep the error return and make sparse
work I could have just applied this to the original functions and made
no changes at all to the vast majority of drivers.  Sadly that wasn't
to be. End result of ending up with a trylock type approach is cleaner
and more compact even if it's not what we have gotten used to for this
particular function.

Jonathan


> > +}  
>