On 04/19/11 19:00, Hennerich, Michael wrote:
> Jonathan Cameron wrote on 2011-04-19:
>> On 04/19/11 16:22, Hennerich, Michael wrote:
>>> Hi Jonathan,
>>>
>>> The AD7606 ring buffer doesn't use the thread, and installs only the
>>> hard handler.
>>>
>>> indio_dev->pollfunc->h = &ad7606_trigger_handler_th;
>>> indio_dev->pollfunc->thread = NULL;
>>> This crashes the system in handle_nested_irq (null pointer
>>> action->thread_fn) called from iio_trigger_poll_chained().
>> I knew that wouldn't work, but didn't realize it wouldn't just fail
>> with an error...
>>
>> The only thing I can think to do is to actually set both h and thread
>> to ad7606_trigger_handler_th.
>>
>> As it returns IRQ_HANDLED, if it is called via irq_trigger_poll, it
>> will happen in interrupt context and thread will never run.
>>
>> If it is called via irq_trigger_poll_handler (e.g. for non interrupt
>> context) it'll happen outside interrupt context. Given timing is never
>> going to be that tight for userspace triggers, this probably isn't a
>> problem.
>>
>> Can you try that out and see if it works?
>
> I know that setting the thread function will effectively avoid the crash.
> However I actually haven't traced if it's actually being called once the
> Hard handler returned IRQ_HANDLED.
It certainly shouldn't be. Feel free to check, but given handle_irq_event_percpu
(which is called from handle_simple_irq -> handle_irq_event)
contains
switch (res) {
case IRQ_WAKE_THREAD:
/*
* Set result to handled so the spurious check
* does not trigger.
*/
res = IRQ_HANDLED;
/*
* Catch drivers which return WAKE_THREAD but
* did not set up a thread function
*/
if (unlikely(!action->thread_fn)) {
warn_no_thread(irq, action);
break;
}
irq_wake_thread(desc, action);
/* Fall through to add to randomness */
case IRQ_HANDLED:
random |= action->flags;
break;
default:
break;
}
We should be completely safe in the hard irq case.
I'm not convinced what we have in the sysfs trig is the best
way of doing this, but haven't found a better one and only
reply to querying this said, 'that'll work' which whilst
encouraging doesn't say if it is the best plan.
The problem there is that whilst, the handle_irq code does a
sanity check and warn on no thread for the irq, handle_nested_irq
doesn't (which is reasonable considering it would pretty odd to call
this for an irq that doesn't have one!.
>
> I'll have try.
>
> -Michael
--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
