On Tue, Feb 25, 2025 at 8:25 AM Jonathan Cameron
<Jonathan.Cameron@xxxxxxxxxx> wrote:
> On Sat, 22 Feb 2025 22:58:00 +0200
> Andy Shevchenko <andy.shevchenko@xxxxxxxxx> wrote:
> > 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:
...
> > > > > +/*
> > > > > + * 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)
> > >
> > > 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.
Oh...
> 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.
Me neither. I haven't looked at the sparse, I don't know how it works.
> 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.
FWIW, Torvalds recently uploaded a new PoC branch in his sparse tree,
perhaps he can do something about it at the end of the day. I would Cc
him to the most needed sparse updates, like __cond_acquires().
> 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.
Right, so we have a trylock semantics for time being...
> > > 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
