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?
