[PATCH] iio: st_sensors: request any context IRQ

[PATCH] iio: st_sensors: request any context IRQ

[Linus Walleij]

It really doesn't matter if the poll is triggered from a
fastpath or threaded IRQ, the IIO core runs its own interrupt
thread anyway. Sometimes this is connected to a hard IRQ line,
sometimes to something on an I2C expander that needs to
run from a thread, so request any context IRQ.

Cc: Denis Ciocca <denis.ciocca@st.com>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/iio/common/st_sensors/st_sensors_trigger.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/iio/common/st_sensors/st_sensors_trigger.c b/drivers/iio/common/st_sensors/st_sensors_trigger.c
index 3c0aa17d753f..e33796cdd607 100644
--- a/drivers/iio/common/st_sensors/st_sensors_trigger.c
+++ b/drivers/iio/common/st_sensors/st_sensors_trigger.c
@@ -32,9 +32,8 @@ int st_sensors_allocate_trigger(struct iio_dev *indio_dev,
 		goto iio_trigger_alloc_error;
 	}
 
-	err = request_threaded_irq(sdata->get_irq_data_ready(indio_dev),
+	err = request_any_context_irq(sdata->get_irq_data_ready(indio_dev),
 			iio_trigger_generic_data_rdy_poll,
-			NULL,
 			IRQF_TRIGGER_RISING,
 			sdata->trig->name,
 			sdata->trig);
-- 
2.4.3

Re: [PATCH] iio: st_sensors: request any context IRQ

[Jonathan Cameron]

On 12/11/15 13:12, Linus Walleij wrote:
> It really doesn't matter if the poll is triggered from a
> fastpath or threaded IRQ, the IIO core runs its own interrupt
> thread anyway. Sometimes this is connected to a hard IRQ line,
> sometimes to something on an I2C expander that needs to
> run from a thread, so request any context IRQ.
> 
> Cc: Denis Ciocca <denis.ciocca@st.com>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
Hi Linus,

Note first that there is nothing stopping any random device
hanging off the trigger - so we can't make any device specific
assumptions.

>From the start the way it was built up pretty much assumed
we had a real hardware interrupt to drive this.  Interesting
question is does it matter - we jump through hoops to drop
back into interrupt context at times. Perhaps we don't need to
- I can't honestly remember why we did this and it was pre
threaded interrupts so perhaps the assumptions are no longer
valid.

My knowledge of the irq handling is perhaps not
deep enough but I can follow through code ;)  So here goes.

The result of this as you've identified is that we may call the 
function iio_trigger_generic_data_ready_poll from either a thread
context or a interrupt context.

This then calls generic_handle_irq for each of devices hanging
off the trigger and ultimately desc->handle_irq and on
to handle_simple_irq (fun this isn't it ;) This calls onto
handle_irq_event and after a few more jumps ends up at
handle_irq_event_percpu in kernel/irq/handle.c

This calls the top half interrupt just fine an then if it
gets an IRQ_WAKE_THREAD (most of the time)
we call __irq_wake_thread.  So the only remaining question
is there anything that actually requires the thread to be woken
from interrupt context...

Ah, this is where it come unstuck...
__irq_wake_thread explicitly makes the point that it it is running
only on one cpu at a time and in interrupt context.

Now, we could restrict the st_sensors trigger to use by
the device supplying it - we could then operate the
entire trigger always in a state where sleeping is safe
(and run it as a chained irq - entirely in a thread).

We could perhaps do some 'magic' to allow this on the irq
side.  As the interrupt is never threaded anyway (though the
stuff hanging off it almost always is) we could do something
nefarious such as:

Call request_any_context_irq.  If this returns IRQC_IS_HARDIRQ
continue as normal.  If it returns IRQ_C_IS_NESTED we could
note this fact and handle any child interrupts hanging off the
device differently. In the first instance we could refuse
to let any pollfuncs with a top half interrupt bind to the trigger.
The trigger would then call iio_trigger_poll_chained to
call the nested interrupts (fine I think?)

Am I missing something here?  This turned out to be a rather
deeper rabbit hole than I thought it would be when I started!

Jonathan

> ---
>  drivers/iio/common/st_sensors/st_sensors_trigger.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/common/st_sensors/st_sensors_trigger.c b/drivers/iio/common/st_sensors/st_sensors_trigger.c
> index 3c0aa17d753f..e33796cdd607 100644
> --- a/drivers/iio/common/st_sensors/st_sensors_trigger.c
> +++ b/drivers/iio/common/st_sensors/st_sensors_trigger.c
> @@ -32,9 +32,8 @@ int st_sensors_allocate_trigger(struct iio_dev *indio_dev,
>  		goto iio_trigger_alloc_error;
>  	}
>  
> -	err = request_threaded_irq(sdata->get_irq_data_ready(indio_dev),
> +	err = request_any_context_irq(sdata->get_irq_data_ready(indio_dev),
>  			iio_trigger_generic_data_rdy_poll,
> -			NULL,
>  			IRQF_TRIGGER_RISING,
>  			sdata->trig->name,
>  			sdata->trig);
> 

Re: [PATCH] iio: st_sensors: request any context IRQ

[Linus Walleij]

On Sat, Nov 14, 2015 at 7:36 PM, Jonathan Cameron <jic23@kernel.org> wrote:

> The result of this as you've identified is that we may call the
> function iio_trigger_generic_data_ready_poll from either a thread
> context or a interrupt context.

Yeah. Somebody connected an ST accelerometer to an IRQ
line that sits on and I2C expander in my design, meaning it needs
to have a threaded interrupt handler, as it needs to talk I2C
to handle the IRQ.

So somehow I need to handle any context of IRQs on the
ST MEMS drivers.

> This then calls generic_handle_irq for each of devices hanging
> off the trigger and ultimately desc->handle_irq and on
> to handle_simple_irq (fun this isn't it ;) This calls onto
> handle_irq_event and after a few more jumps ends up at
> handle_irq_event_percpu in kernel/irq/handle.c

I start to get the feeling tglx should review how IIO triggers handle
IRQs... maybe he already has. This brings into the question how
that special trigger IRQ thread is created (haven't looked into
that).

> Now, we could restrict the st_sensors trigger to use by
> the device supplying it - we could then operate the
> entire trigger always in a state where sleeping is safe
> (and run it as a chained irq - entirely in a thread).
(...)
> Call request_any_context_irq.  If this returns IRQC_IS_HARDIRQ
> continue as normal.  If it returns IRQ_C_IS_NESTED we could
> note this fact and handle any child interrupts hanging off the
> device differently. In the first instance we could refuse
> to let any pollfuncs with a top half interrupt bind to the trigger.
> The trigger would then call iio_trigger_poll_chained to
> call the nested interrupts (fine I think?)
>
> Am I missing something here?  This turned out to be a rather
> deeper rabbit hole than I thought it would be when I started!

Well. Yeah. Well. Maybe the *real* solution in that case is
that all drivers should be creating a threaded IRQ handler and
call from their thread and get rid of this extra thread from
the trigger.

But I'm confused, becaus as of today what I am actually
changing:

> -       err = request_threaded_irq(sdata->get_irq_data_ready(indio_dev),
> +       err = request_any_context_irq(sdata->get_irq_data_ready(indio_dev),

It *used* to be threaded all the time, but what I hear you saying
is that the IIO core was actually pretty happy with the hardirq/fastpath.
So I don't quite see why this was requested threaded in the first
place.

Yours,
Linus Walleij

Re: [PATCH] iio: st_sensors: request any context IRQ

[Jonathan Cameron]

On 15/11/15 16:22, Linus Walleij wrote:
> Yeah. Somebody connected an ST accelerometer to an IRQ
> line that sits on and I2C expander in my design, meaning it needs
> to have a threaded interrupt handler, as it needs to talk I2C
> to handle the IRQ.
> 
> So somehow I need to handle any context of IRQs on the
> ST MEMS drivers.
Sure, clearly need some way around it and I've known it would come
up at some point for a while!

> 
>> > This then calls generic_handle_irq for each of devices hanging
>> > off the trigger and ultimately desc->handle_irq and on
>> > to handle_simple_irq (fun this isn't it ;) This calls onto
>> > handle_irq_event and after a few more jumps ends up at
>> > handle_irq_event_percpu in kernel/irq/handle.c

> I start to get the feeling tglx should review how IIO triggers handle
> IRQs... maybe he already has. 

Thomas did help us work it out via discussions about the design
back in 2011 when we moved to threaded interrupts.

Google fed me:
http://marc.info/?l=linux-kernel&m=129917247506111&w=2
which lines up with my local email threads from the time.

I'm having fun actually tracking down the archive for the email
threads related to the patches themselves.  The series is this one
http://www.spinics.net/lists/linux-iio/msg01563.html

(though there was definitely a v3 as the patch is:
d96d1337e339521a2bd56dc9d51fef140c1a49ee 
staging:iio: Add infrastructure for irq_chip based triggers

but I seem to have lost it locally (probably sat on some dead
PC in my old lab somewhere).

I remember a lot of this stuff came after a very detailed review
Arnd Bergman did which led to tidying up of how we did this and
other aspects of the subsystem (was very very helpful at that stage
as other reviews were all from guys working in IIO and not many of
us were very experienced!).

Not sure anything has fundamentally changed since then.
If tglx has time to take another look it would certainly be
welcome!  Any input on this particular problem also welcome.
(cc'd - Thomas, discussion is about the handling of interrupts
for IIO triggers where we split a single, typically real,
interrupt and feed it to any devices that are registered to
receive it - code is in
drivers/industrialio/industrialio-triggers.c.  Complexity
is that any trigger can be used by any device - including
stuff like timestamping, or capture pin wiggling which tends
to be in traditional irq top half.  In this case Linus
has an interrupt that is on a chip on an i2c bus so can't
drive that stuff...)

To all intents and purposes the IIO triggers are irq chips
so everything gets set up exactly the same way as if they were
hardware irqs.  Its very similar to what a lot of mfd devices
do though we effectively spread the interrupt out rather than
working out which device to send it to (as it needs to go to
all of them).

The nasty hoops are only jumped through when we
generate triggers in software (e.g. the sysfs trigger).  Fixing
it for your case would also allow us to hopefully drop some of
that complexity as well. 

>This brings into the question how
> that special trigger IRQ thread is created (haven't looked into
> that).
> 
The st sensors trigger never had a threaded part anyway so
I'm not sure why it was requesting it via the threaded_irq request.
That falls back to standard irq if no thread is provided though.

Jonathan

Re: [PATCH] iio: st_sensors: request any context IRQ

[Jonathan Cameron]

On 15 November 2015 17:32:50 GMT+00:00, Jonathan Cameron <jic23@kernel.org> wrote:
>On 15/11/15 16:22, Linus Walleij wrote:
>> Yeah. Somebody connected an ST accelerometer to an IRQ
>> line that sits on and I2C expander in my design, meaning it needs
>> to have a threaded interrupt handler, as it needs to talk I2C
>> to handle the IRQ.
>> 
>> So somehow I need to handle any context of IRQs on the
>> ST MEMS drivers.
>Sure, clearly need some way around it and I've known it would come
>up at some point for a while!
>
This time I will actually write a reply.. Sorry about that.

Anyhow, looking into this further. The reason we use traditional handlers is to
 drive time critical stuff nearer the interrupt time. If we have a nested
 interrupt there is not much we can do on that front.

The pollfunc stuff could I think easily query if it has a hard IRQ or not as long as we pass the origin IRQ into the trigger 
alloc, query it there. If not I don't think anything stops us explicitly calling the
 hard IRQ handler then the chained interrupt stuff to run the thread function.

We might need to have a means of specifying a particular driver needs a hard
 IRQ as well and fail to attach to the triggers that don't provide them.


>> 
>>> > This then calls generic_handle_irq for each of devices hanging
>>> > off the trigger and ultimately desc->handle_irq and on
>>> > to handle_simple_irq (fun this isn't it ;) This calls onto
>>> > handle_irq_event and after a few more jumps ends up at
>>> > handle_irq_event_percpu in kernel/irq/handle.c
>
>> I start to get the feeling tglx should review how IIO triggers handle
>> IRQs... maybe he already has. 
>
>Thomas did help us work it out via discussions about the design
>back in 2011 when we moved to threaded interrupts.
>
>Google fed me:
>http://marc.info/?l=linux-kernel&m=129917247506111&w=2
>which lines up with my local email threads from the time.
>
>I'm having fun actually tracking down the archive for the email
>threads related to the patches themselves.  The series is this one
>http://www.spinics.net/lists/linux-iio/msg01563.html
>
>(though there was definitely a v3 as the patch is:
>d96d1337e339521a2bd56dc9d51fef140c1a49ee 
>staging:iio: Add infrastructure for irq_chip based triggers
>
>but I seem to have lost it locally (probably sat on some dead
>PC in my old lab somewhere).
>
>I remember a lot of this stuff came after a very detailed review
>Arnd Bergman did which led to tidying up of how we did this and
>other aspects of the subsystem (was very very helpful at that stage
>as other reviews were all from guys working in IIO and not many of
>us were very experienced!).
>
>Not sure anything has fundamentally changed since then.
>If tglx has time to take another look it would certainly be
>welcome!  Any input on this particular problem also welcome.
>(cc'd - Thomas, discussion is about the handling of interrupts
>for IIO triggers where we split a single, typically real,
>interrupt and feed it to any devices that are registered to
>receive it - code is in
>drivers/industrialio/industrialio-triggers.c.  Complexity
>is that any trigger can be used by any device - including
>stuff like timestamping, or capture pin wiggling which tends
>to be in traditional irq top half.  In this case Linus
>has an interrupt that is on a chip on an i2c bus so can't
>drive that stuff...)
>
>To all intents and purposes the IIO triggers are irq chips
>so everything gets set up exactly the same way as if they were
>hardware irqs.  Its very similar to what a lot of mfd devices
>do though we effectively spread the interrupt out rather than
>working out which device to send it to (as it needs to go to
>all of them).
>
>The nasty hoops are only jumped through when we
>generate triggers in software (e.g. the sysfs trigger).  Fixing
>it for your case would also allow us to hopefully drop some of
>that complexity as well. 
>
>>This brings into the question how
>> that special trigger IRQ thread is created (haven't looked into
>> that).
>> 
>The st sensors trigger never had a threaded part anyway so
>I'm not sure why it was requesting it via the threaded_irq request.
>That falls back to standard irq if no thread is provided though.
>
>Jonathan
>--
>To unsubscribe from this list: send the line "unsubscribe linux-iio" in
>the body of a message to majordomo@vger.kernel.org
>More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.