Re: [RFC PATCH 1/8] iio: locking: introduce __cleanup() based direct mode claiming infrastructure

[Jonathan Cameron <jic23@kernel.org>]

On Tue, 24 Oct 2023 17:28:00 +0200
Peter Zijlstra <peterz@infradead.org> wrote:

> On Tue, Oct 24, 2023 at 05:11:23PM +0200, Peter Zijlstra wrote:
> > On Mon, Oct 23, 2023 at 10:55:56AM +0200, Nuno Sá wrote:
> >   
> > > > > +/*
> > > > > + * Auto cleanup version of iio_device_claim_direct_mode,
> > > > > + *
> > > > > + *     CLASS(iio_claim_direct, claimed_dev)(indio_dev);
> > > > > + *     if (IS_ERR(claimed_dev))
> > > > > + *             return PTR_ERR(claimed_dev);
> > > > > + *
> > > > > + *     st = iio_priv(claimed_dev);
> > > > > + *     ....
> > > > > + */
> > > > > +DEFINE_CLASS(iio_claim_direct, struct iio_dev *,
> > > > > +            iio_device_release_direct_mode(_T),
> > > > > +            ({
> > > > > +                       struct iio_dev *dev;
> > > > > +                       int d = iio_device_claim_direct_mode(_T);
> > > > > +
> > > > > +                       if (d < 0)
> > > > > +                               dev = ERR_PTR(d);
> > > > > +                       else
> > > > > +                               dev = _T;
> > > > > +                       dev;
> > > > > +            }),
> > > > > +            struct iio_dev *_T);
> > > > > +  
> >   
> > > I don't really have a very strong opinion on this but what I really don't like
> > > much is the pattern:
> > > 
> > > CLASS(type, ret), where the return value is an argument of the macro... It would
> > > be nice if we could just make it like:
> > > 
> > > ret = guard(type)(...); //or any other variation of the guard() macro
> > > if (ret) 
> > > 	return ret;
> > > 
> > > the above could also be an error pointer or even have one variation of each. but
> > > yeah, that likely means changing the cleanup.h file and that might be out of
> > > scope for Jonathan's patch series.   
> > 
> > So I have a version for trylocks that I've not managed to send out.. it
> > doesn't deal with arbitrary error codes, and as someone else down-thread
> > noted, the guard() thing is not really suited for tricks like this.
> > 
> > Notably I have a patch that converts ptrace_attach() to have a loop like:
> > 
> > 	scoped_guard (mutex_intr, &task->signal->cred_guard_mutex) {
> > 
> > 		goto success;
> > 	}
> > 	return -ERESTARTNOINTR;
> > 
> > success:
> > 	...
> > 	return 0;
> > 
> > 
> > And another patch that does something like:
> > 
> > 	scoped_cond_guard (rwsem_read_intr, no_lock,
> > 			   task ? &task->signal->exec_update_lock : NULL) {
> > 
> > 		if (0) {
> > no_lock:
> > 			if (task)
> > 				return -EINTR;
> > 		}
> > 		
> > 		...
> > 	}
> >   
> 
> Hmm, looking at:
> 
> +       case IIO_CHAN_INFO_RAW: { /* magic value - channel value read */
> +               CLASS(iio_claim_direct, claimed_dev)(indio_dev);
> +               if (IS_ERR(claimed_dev))
> +                       return PTR_ERR(claimed_dev);
> +               guard(mutex)(&st->lock);
> +
>                 switch (chan->type) {
>                 case IIO_VOLTAGE:
>                         if (chan->output) {
>                                 /* Set integer part to cached value */
>                                 *val = st->dac_val;
> +                               return IIO_VAL_INT;
>                         } else if (chan->differential) {
>                                 if (chan->channel == 1)
>                                         *val = st->differential_adc_val[0];
>                                 else
>                                         *val = st->differential_adc_val[1];
> +                               return IIO_VAL_INT;
>                         } else {
>                                 *val = st->single_ended_adc_val;
> +                               return IIO_VAL_INT;
>                         }
> +
>                 case IIO_ACCEL:
>                         *val = st->accel_val;
> +                       return IIO_VAL_INT;
>                 default:
> +                       return -EINVAL;
>                 }
> +       }
> 
> 
> And your iio_device_claim_direct_mode(), that is basically a trylock,
> either it succeeds (and returns 0) or fails with -EBUSY.
> 
> Which means you could write your thing above like:
> 
> DEFINE_CLASS(iio_claim_direct, struct iio_dev *,
>             iio_device_release_direct_mode(_T),
>             ({
>                        struct iio_dev *dev;
>                        int d = iio_device_claim_direct_mode(_T);
> 
>                        if (d < 0)
>                                dev = NULL;
>                        else
>                                dev = _T;
>                        dev;
>             }),
>             struct iio_dev *_T);
> 
> static inline void *
> class_iio_claim_direct_lock_ptr(class_iio_claim_direct_t *_T)
> { return *_T; }
> 
> 
> 
> 	case IIO_CHAN_INFO_RAW: /* magic value - channel value read */
> 		scoped_guard (iio_device_claim, indio_dev) {
> 			guard(mutex)(&st->lock);
> 			switch (chan->type) {
> 			case ..:
> 				return IIO_VAL_INT;
> 			default:
> 				return -EINVAL;
> 			}
> 		}
> 		return -EBUSY;
> 
> and it would all just work, no?
With the scoped_guard change you mention in previous
email, this should work.

I'm not that keen on it from a readability point of view as it puts
the 'good' path inline and if we do have anything to do after the
scoped_guard() we need to engage in setting a condition variable to
indicate we'd been in the loop.

Maybe we can extend your other case though for cases where
an early exit fits or make it more general to incorporate this?

scoped_cond_guard_ret(mutex_intr, &mutex, -EINTR)?
or (naming needs work
scoped_cond_guard_call(mutex_intr, &mutex, ({ return -EINTR; }));

#define scoped_cond_guard_call(_name, _call, args...) \
+	for (CLASS(_name, scope)(args), \
+	     *done = NULL; !done; done = (void *)1) \
+		if (!__guard_ptr(_name)(&scope)) _call; \
+		else

Totally untested but hopefully conveys the idea (which is a tiny
extension of your scoped_cond_guard)

Jonathan