Re: [RFC][PATCH] irq: support IRQ_NESTED_THREAD with non-threaded interrupt handlers

[Esben Haabendal]

On Sat, Jun 5, 2010 at 10:14 PM, Thomas Gleixner <tglx@xxxxxxxxxxxxx> wrote:
>> The phy interrupt handler calls disable_irq, which does not work
>> with an i2c irq controller (ie. the genirc buslock mutex), and I specifically
>> got rid of the buslock for pca9535 driver (powerpc only, though).
>
> And that's the fundamentally wrong approach.

Well, given you haven't seen what I have done there, I don't see how you
can be so sure ;-)

The only reason for the buslock in the pca9535 driver is to set the
direction (ie. input) for interrupt pins.  On powerpc, I do this in the map()
irq_chip function. So I don't see the need for buslock on powerpc.

>> This is really a drawback of the genirq buslock IMHO.
>
> No, it avoids races even on UP. It's there for a reason and there was
> a huge discussion about this last year.

I am not talking about it in general, but for the pca953x driver in particular.
The only thing that is done in the unlock is to set direction, and I don't see
the point in not doing this in earlier.  In most situations, it will
likely be set
even eariler, given that default direction is input, and that it is not uncommon
to set the direction appropriately early in the board init phase.

> Just because you have not hit the problem yet does not make it non
> existant.

Which particular problem should I be on the lookout for in pca953x
driver?

>> So until all handlers are rewritten to run threaded, I believe
>> something like the proposed patch will give value to people
>> with i2c irq controllers. Hopefully, not to many people are unlucky
>> enough to have this, but anyway...
>
> No it does not, as it just encourages people to disable buslocks and
> do simliar shitty crap.

Hold your horses, no reason to call it shitty when you haven't even seen
it. I didn't try to start any kind of general duscission for/against genirq
buslock.

Anyway,

>
>> >> Unless all interrupt handlers should be rewritten to be able to in both
>> >> thread and interrupt context, I fail to se the conflict between the patch
>> >> proposed and the work being done on request_any_context_irq().
>> >
>> > It's not a question of conflict. It's a question of semantics.
>> >
>> > We had and still have enough surprises in preempt-rt where we force
>> > thread all handlers which were caused by various assumptions in the
>> > handler code. I really prefer that the system yells in such a case
>> > instead of letting run people into hard to debug problems silently.
>>
>> I fully appreciate that.  And for exactly that reason (combined with
>> a tight timeschedule), I really just need to have the phy interrupt handler
>> running unchanged with the i2c irq controller.
>
> Yeah, and at the same time you rip out the buslock. Why are you not
> sending this patch as well ? Simply because you know that it is
> utterly wrong and a horrible hack.

No, it was send at the same time, but to the linuxppc-dev.  I do not
see it as utterly wrong, and I hope you will give it a look with an open
mind, not just assuming that it is shitty crap, utterly wrong or horrible
hack even before reading it, thanks ;-)

http://patchwork.ozlabs.org/patch/54717/

It is a longer patch, and I expect that it could be improved quite a
bit, but I really don't see it as a fundanmentally shitty.

> But at the same time you want to sell me a patch which rips out the
> prevention mechanism for your hack.

Which patch are you refering to?

The patch we are discussing here does not rip out any thing,

It simply adds support for using existing dirvers together
with handle_nested_irq().

> The time you spent to fiddle with the generic code and ripping out the
> buslock is probably more than it had taken to make the PHY driver
> thread safe.

Maybe, but I am not so sure.

The phy driver calls disable_irq_nosync(), which is documented as
"This function may be called from IRQ context."
And with disable_irq_nosync() calling chip_bus_lock() and
chip_bus_sync_unlock(), which definitely is not meant
for IRQ context, I got the understanding that it was not
the phy driver that needed fixing, but mroe the generic
buslock stuff, which I did not have the guts to touch, as
I understand it would be easy to get wrong.

So maybe you can help me out here, is disable_irq_nosync()
to be improved here?  It does not seem to be fair to
document that it can be called in IRQ context, and then
have it designed to blow up if done so in combination
with a genirq buslock driver.

> The buslock is there for a reason and if you can't use the code with
> the disable/enable_irq() in the atomic context, then this needs to be
> fixed there if you need to run the irq handler in thread context.

How can I disable_irq in interrupt context, with the interrupt handled
by a genirq buslock driver?

/Esben
-- 
Esben Haabendal, Senior Software Consultant
DoréDevelopment ApS, Ved Stranden 1, 9560 Hadsund, DK-Denmark
Phone: +45 51 92 53 93, E-mail: eha@xxxxxxxxxxxxxxxxxx
WWW: http://www.doredevelopment.dk
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/