On Sat, Feb 22, 2025 at 7:24 PM Jonathan Cameron <jic23@xxxxxxxxxx> wrote:
> On Sat, 22 Feb 2025 17:51:02 +0200
> Andy Shevchenko <andy.shevchenko@xxxxxxxxx> wrote:
> > Sun, Feb 09, 2025 at 06:05:58PM +0000, Jonathan Cameron kirjoitti:
> > > From: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx>
> > >
> > > 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?
> 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.
> > > +}
--
With Best Regards,
Andy Shevchenko
