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

[Peter Zijlstra <peterz@infradead.org>]

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?