Re: Linux irq subsys i2c interaction question

[Hans de Goede <hdegoede@redhat.com>]

Hi,

On 07-03-17 10:18, Thomas Gleixner wrote:
> Hans,
>
> On Tue, 7 Mar 2017, Hans de Goede wrote:
>
>> I've an "interesting" irq problem. I've an i2c pmic (Intel Cherry Trail
>> Whiskey Cove) which itself contains an i2c controller (adapter in Linux
>> terms) and has a pin dedicated for raising irqs by the external battery
>> charger ic attached to its i2c-adapter.
>>
>> To be able to use the irq for the external-charger, the driver for the
>> PMIC needs to implement an irqchip and here things get interesting. This
>> irqchip can NOT use handle_nested_irq, because the i2c-client driver's
>> irq-handler will want to read/write to the external-charger which uses
>> the i2c-controller embedded in the PMIC which requires handling of new
>> (not arrived when started) PMIC irqs, which cannot be done if the client
>> irq-handler is running in handle_nested_irq, because then the PMIC's irq
>> handler is already / still running and blocked in the i2c-client's
>> irq-handler which is waiting for the new interrupt(s) to get processed to
>> signal completion of the i2c-transaction(s) it is doing.
>
> Cute circular dependency.
>
>> I've solved this the following way, which works but
>> I wonder if it is the right way to solve this ?
>>
>> Note this sits inside the threaded interrupt handler
>> for the PMIC irq (after reading and acking the irqs):
>>
>>         /*
>>          * Do NOT use handle_nested_irq here, the client irq handler will
>>          * likely want to do i2c transfers and the i2c controller uses this
>>          * interrupt handler as well, so running the client irq handler from
>>          * this thread will cause things to lock up.
>>          */
>>         if (reg & CHT_WC_EXTCHGRIRQ_CLIENT_IRQ) {
>>                 /*
>>                  * generic_handle_irq expects local irqs to be disabled
>>                  * as normally it is called from interrupt context.
>>                  */
>>                 local_irq_disable();
>>                 generic_handle_irq(adap->client_irq);
>>                 local_irq_enable();
>>         }
>>
>> Not really pretty, but it works well enough. I can
>> live with this if you can live with it too :)
>
> I'm a bit worried about this being hardcoded for that particular use
> case. That also means that you cannot use the generic regmap irq handling
> stuff and need to have your own irq magic there.

Yeah, although the amount of irqchip code needed is pretty minimal
for this (otherwise) simple device.

> Wouldn't it be smarter to
> mark that interrupt in some way and have that as a generic option?

Such a generic option is what I was looking for before going this
route, so yeah +1 for that. I'm not familiar enough with the
irq code to feel comfortable adding that, but if you can provide
a patch, I can test it.

> Can you point me at the relevant drivers/patches?

Here is my current wip stuff (I've pushed this to a for-irq-discussion
branch so that the commits won't go away when I rebase my wip
branch).

mfd patches:
https://github.com/jwrdegoede/linux-sunxi/commit/a0b9ee4ba7b8206880415351a53d5fbbd5969ba6
https://github.com/jwrdegoede/linux-sunxi/commit/a2bff50785c31239ec699a8b43e827c40089ee1f
https://github.com/jwrdegoede/linux-sunxi/commit/708985e8f33471976359c9ff52e2db9964e3ddf4

i2c-adapter patch:
https://github.com/jwrdegoede/linux-sunxi/commit/88a671b3543ffd1f5f4251f4028e4a5d67d88fda

Note the mfd driver has its own irqchip using the regmap-irq.c stuff (second commit)
and the trouble some irqchip driver is nested from that in the i2c-adapter driver
(the last commit).

Regards,

Hans