Message-ID: <20250507204026.11a260ef@jic23-huawei>
Date: Wed, 7 May 2025 20:40:26 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: Gyeyoung Baek <gye976@...il.com>
Cc: David Lechner <dlechner@...libre.com>, Nuno Sá
 <nuno.sa@...log.com>, Andy Shevchenko <andy@...nel.org>,
 linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] iio: trigger: Add validation to reject devices
 requiring top half

On Wed, 7 May 2025 00:55:27 +0900
Gyeyoung Baek <gye976@...il.com> wrote:

> Hello Jonathan, thank you for the review.
> I would appreciate it if you could review my additional comments.
> 
> On Sun, May 4, 2025 at 11:24 PM Jonathan Cameron <jic23@...nel.org> wrote:
> >
> > On Sun,  4 May 2025 04:00:43 +0900
> > Gyeyoung Baek <gye976@...il.com> wrote:
> >  
> > > Some device drivers implement top-half handler,
> > > which is not compatible with threaded handler trigger.
> > > This patch adds a validation function to reject such devices,
> > > allowing only iio_pollfunc_store_time().  
> >
> > This needs more reasoning.  What makes it not work?
> > + what do we mean by not compatible?
> > I'd expect at least a reference to it using iio_trigger_poll_nested()
> > directly.  
> 
> Of course, even if the IIO device registers a top-half,
> `iio_trigger_poll_nested()` ignores the top-half and only calls the
> bottom-half, so it works properly.
> What I misunderstood here is that I thought there were other IIO
> devices implementing a top-half other than
> `iio_pollfunc_store_time()`. So I assumed that the TODO was to block
> those.
> I had confused it with the IIO trigger's top-half handler, apologies
> for the confusion.

There were...  I'm rather surprised that there are none any more!

> 
> ---
> 
> > It's unfortunately hard to tell whether a top half handler is
> > actually needed or not.  As a follow up question, what cases do we have
> > of top half / interrupt context handlers other than iio_pollfunc_store_time()?  
> 
> No, it seems that `iio_pollfunc_store_time()` is the only top-half
> handler for IIO devices.
Excellent.

> 
> ---
> 
> > Maybe we don't need this code to be this complex any more at all
> > (i.e. could it become a flag to say whether the timestamp is useful or not)
> > rather than registering the callback.  
> 
> my new understanding of TODO is as follows:
>     - Since `iio_loop_thread()` can only call
> `iio_trigger_poll_nested()` and not `iio_trigger_poll()`,
>       if the connected IIO device expects a top half such as
> `iio_pollfunc_store_time()`,
>       then `iio_loop_thread()` needs to directly call
> `iio_pollfunc_store_time().`

I'd take a different approach (slightly) though it's more effort.

Step 1. Tidy up current situation.

Patch to convert all existing calls to devm_iio_triggered_buffer_setup()
and iio_triggered_buffer_setup() to not take a top half function but replace
that variable with a bool early_timestamp or something along those lines.
Replace the h in struct iio_poll_func with a similarly named bool.
Bunch of plumbing to make that all get filled in correctly.

Then in iio_trigger_attach_pollfunc() check that bool and if appropriate
pass iio_pollfunc_store_time() it to request_threaded_irq()

Step 2. Make what you want work cleanly now we only have that one handler.

In iio_trigger_poll_nested() we can't know if that flag is set and I'm not
really keen on trying to get to this from elsewhere. We have previously considered
solving this case via whether the timestamp is set or not in the threaded
handler. I've never like that much as in theory timestamp 0 is valid (was
a while ago). The rpr0521 light sensor has handling for this.

I wonder if the following would work.

In iio_trigger_attach_poll_func() we have access to the trigger and
the pollfunc.  So if the pollfunc flag for wanting an early timestamp is set and
we know the trigger is going to use iio_poll_trigger_nested() then we could
wrap the registered handler in a local one that calls the iio_pollfunc_store_time()

The additional magic needed here is that today we don't know that about the trigger.
So we'd need to add a bool to the struct iio_trig to indicate it and set that
for all drivers that use iio_trigger_poll_nested()   bool nested; will do.

It's not perfect as there are driver that do iio_trigger_poll() and iio_trigger_poll_nested()
depending on path. To handle those we'd need a flag to say don't overwrite my timestamp.
at91-sama5d2-adc.c is the first one I found.

There are ways to make even that work but lets skip that for now as they'd
slightly complicate things. That driver won't call the timestamp capture in
some paths but it doesn't today so we are no worse off.

Jonathan


> 
> Would my understanding be correct?
> 
> ---
> 
> > >
> > > +/*
> > > + * Protect against connection of devices that 'need' the top half
> > > + * handler.
> > > + */
> > > +static int iio_loop_trigger_validate_device(struct iio_trigger *trig,
> > > +                                             struct iio_dev *indio_dev)
> > > +{
> > > +     struct iio_poll_func *pf = indio_dev->pollfunc;
> > > +
> > > +     /* Only iio timestamp grabbing is allowed. */
> > > +     if (pf->h && pf->h != iio_pollfunc_store_time)  
> >
> > Why is iio_pollfunc_store_time useable here?  It's not going to store the
> > time if we don't call it...  We could special case it probably but we'd
> > need to ensure the call is actually made.  
> 
> Yes, If my new understanding is correct, `iio_loop_thread()` needs to
> call `iio_pollfunc_store_time()` directly,
> depending on whether the IIO device's top-half is NULL or
> `iio_pollfunc_store_time()`.
Yes. But it doesn't have direct access to the required pollfunc.


> 
> --
> Regards,
> Gyeyoung
> 

Powered by blists - more mailing lists