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

[Jonathan Cameron <Jonathan.Cameron@huawei.com>]

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

> On Sat, Feb 22, 2025 at 7:24 PM Jonathan Cameron <jic23@kernel.org> wrote:
> > 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.  
> 
> Hmm... You mean that the following (still as a macro)
> 
> static inline int ...
> {
>   int ret;
>   ...
>   if (ret)
>     return ret;
> 
>   __acquire_lock(...);
>   return 0;
> }
> 
> triggers the sparse warning?

Doing it in the ternary form of the same thing did trigger issues on fairly
simple cases. I don't recall if I tried this precise combination. The motivation
for this form originally being the __cond_acquires() markings (See later).
However, I 'think' based on other false positives including the smaller
set that required refactors to avoid triggering in this series, is that
sparse isn't coping well with more complex control flows.  So if we
assign a local integer variable and then check it, it seems to loose
track of what is going on in more cases than if we can do

	if (!trylock())
		return;

I'm not sure on what is going wrong.  However it seems sparse is effectively
unmaintained at the moment so even if we could rectify things without the
code upstream it gets us nowhere.  Hence my motivation to make this 'look
like' existing stuff sparse is checking. The nearest being trylock.
It makes me have more warm fuzzy feelings to be the same style of code
as the other cases using the same infrastructure, than doing something new.

Ideally sparse will eventually wake up again and we can have the __cond_acquires()
markings that we had in v1 and not have to have the implementation in the
header. That currently requires trylock type boolean returns.

So overall I think this direction makes sense.  Also can't complain that it
saves 1 line of code for every instance and removes false pretense that
this thing returned a useful error code.

Jonathan
> 
> > 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.  
> 
> > > > +}  
>