Message-ID: <CAKbEznvr1kcyfkocba021zBLJv208GNDHGxW1JCkg=mxAFyQ=w@mail.gmail.com>
Date: Mon, 19 May 2025 23:34:58 +0900
From: Gyeyoung Baek <gye976@...il.com>
To: Jonathan Cameron <jic23@...nel.org>
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 Thu, May 15, 2025 at 4:49 PM Jonathan Cameron <jic23@...nel.org> wrote:
>
> On Mon, 12 May 2025 00:47:39 +0900
> Gyeyoung Baek <gye976@...il.com> wrote:
>
> > Hello Jonathan,
> > I’ve referred to your previous comments and implemented the ideas.
> > Thank you for your earlier feedback.
> > I now have a few follow-up questions and would appreciate your
> > thoughts on the below points.
> >
> > On Thu, May 8, 2025 at 4:40 AM Jonathan Cameron <jic23@...nel.org> wrote:
> > >
> > > On Wed, 7 May 2025 00:55:27 +0900
> > > Gyeyoung Baek <gye976@...il.com> wrote:
> >
> > > 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()
> >
> > Now we have both the existing `devm_iio_triggered_buffer_setup()`,
> > and a new version with the additional arguments of that.
> > Should these two coexist for compatibility, or should the before one
> > be replaced by the new one?
>
> So the aim will be to replace the existing function.
> How to get there is indeed an excellent question.
>
> I'd want to do it in one go, but as it affects a lot of drivers a
> single patch is probably not appropriate.
>
> So we'd introduce something like
> devm_iio_triggered_buffer_setup2() with new parameters
>
> Move everything over to that then a single patch to remove
> the old function and rename it all back to the original.
>
> Alternatively we could find a reasonable alternative name to avoid
> that 'rename all at the end' patch.
>
> devm_iio_triggered_buf_setup() perhaps?
>
> >
> > > 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.
> >
> > What I'm trying to do is a mechanism where device drivers can
> > automatically get timestamps without manually handling them — simply
> > by setting a argument to indicate whether to capture the timestamp in
> > the tophalf or bottomhalf.
> >
> > But there are cases like the rpr0521 where the driver sets the
> > timestamp manually within its own trigger.
> > Would it make sense to extend this to automatically set the timestamp
> > in cases where the driver uses its own trigger as well?
> > To that, I believe we would need a unified interface that can cover
> > all trigger types (e.g., interrupt, software trigger) that invoke
> > poll() or poll_nested().
> > Would it be the right direction?
> > Or would it be more appropriate to consider only the top/bottom of a
> > consumer device?
>
> I'd like to avoid grabbing the timestamp for particular drivers
> that never need it (as they want to grab it in the thread for
> reasons of how they work - typically the capture only starts
> when then write to the device).  Other than that I'm not against
> having it grabbed in drivers that 'sometimes' need it whether
> or not it turns out they do.
>
> The dance is that we can't see the right information in poll / poll_nested
> so the best we can do is see if someone already filled in the
> timestamp in the handler we are currently running.  For that we need
> a flag alongside the pollfunc timestamp.  Care will be needed to ensure
> there are no races though as we might clear that flag just after another
> top half interrupt has been taken on a different cpu core.
>
> I'm not sure yet exactly how this will work.  Needs some experimenting
> and thought.

I think it would be possible to avoid checking whether the timestamp
was overwritten, by explicitly specifying which of poll() or
poll_nested() is being called in the `iio_trigger` structure.
I have submitted an RFC patch on this, and I would greatly appreciate
it if you could review it whenever you have time. Thanks!

Best regards,
Gyeyoung

Powered by blists - more mailing lists