Re: [PATCH] usb: typec: fusb302: Switch to threaded IRQ handler

[Hans de Goede]

Hi,

On 12-Mar-26 13:04, Sebastian Andrzej Siewior wrote:
> On 2026-03-12 11:49:30 [+0100], Hans de Goede wrote:
>> Using a threaded interrupt handler should be ok, yes. This should
>> also fix the issue this patch tries to fix:
>>
>> https://lore.kernel.org/linux-usb/20260103083232.9510-4-linux.amoon@xxxxxxxxx
> 
> This issue went away with commit a7fb84ea70aae ("usb: typec: fusb302:
> Remove IRQF_ONESHOT").
> 
>> Normally an i2c device like this would use a threaded interrupt handler to
>> do all the work since I2C transfers can sleep combined with disabling the IRQ
>> on suspend to avoid the interrupt handler running while the parent i2c-adapter
>> may be suspended.
>>
>> The problem with the fusb302 is that it can be a wakeup source so we cannot
>> disable the IRQ. I worked around this in commit 207338ec5a2 ("usb: typec: fusb302:
>> Improve suspend/resume handling") by moving the actual work to a workqueue
>> and have a hard (non threaded) interrupt handler which disables the IRQ and
>> queues the work, with the work re-enabling the IRQ when done + special
>> handling for the suspended case. Basically our own manual oneshot.
>>
>> If we move the IRQ disabling to a threaded handler, which appears to be
>> necessary for some IRQ controllers (arguably a IRQ controller driver issue,
>> but this seems to be a re-occuring issue), then I wonder if we need
>> the ONESHOT flag again to avoid a level type IRQ re-triggering before
>> the threaded handler gets a chance to disable it (with the workqueue
>> item eventually re-enabling it).
>>
>> I think we need to re-add the ONESHOT flag, but maybe that is the default
>> with a primary NULL handler ?
>>
>> Sebastian Siewior I think you now the IRQ subsystem better then me,
>> any advice / remarks ?

Sebastian, Thank you for your input.

> You could do
> 	request_threaded_irq(chip->gpio_int_n_irq, NULL, fusb302_irq_intn,
> 			     IRQF_ONESHOT | IRQF_TRIGGER_LOW, "fsc_interrupt_int_n", chip);

Ok, that is good to know.

> which would ensure that the handler runs as a thread and the interrupt
> line is disable while it is active.

That is what we want, thank you.

> Then you could let fusb302_irq_intn() do what fusb302_irq_work() does.
> Since it is a thread, mutex_lock() works here.

Right, but the resume handler needs to also schedule the work when the
IRQ is initially ignored if the IRQ triggers before the i2c_client's
resume-handler is called to ensure that the parent i2c-adapter is
ready when the IRQ handling code does i2c accesses.

> Last step would be to replace fusb302_chip::irq_suspended with
> disable_irq() in fusb302_pm_suspend() and enable_irq() in
> fusb302_pm_resume().

That unfortunately is not possible because the fusb302 maybe
a wake-up source so it cannot be disabled unconditionally
and without the disable_irq() / enable_irq() pair the IRQ
may trigger before the parent i2c-adapter is resumed.

This is why the IRQ handling in this driver is as convoluted
as it is in the first place. With the IRQ handler setting
an irq_while_suspended flag if the IRQ runs before the
i2c_client resume and then with resume checking that flag
+ queuing the work do to the actual IRQ handling once the
parent i2c-adapter is ready (if we hit this race).

So as far as I can see the current state of fusb302 code
is good as is.

Except for the problem on case the IRQ line is connected to
the mentioned i2c GPIO expander in this email thread.

I think that proper handling of the sync mechanism for IRQ
chips attached to busses where IO to the IRQ chip may sleep
should fix this. But this seems to keep coming up, so I'm
tempted to just move the IRQ handler to a thread, to avoid
problems with disable_irq_nosync() not working from
"hard" IRQ handlers with some IRQ chip drivers.

TL;DR:

I think doing this:

 	request_threaded_irq(chip->gpio_int_n_irq, NULL, fusb302_irq_intn,
 			     IRQF_ONESHOT | IRQF_TRIGGER_LOW, "fsc_interrupt_int_n", chip);

as you suggest is the best way forward here.

This does what the original patch in this thread suggested,
with the modification that it re-adds the IRQF_ONESHOT flag.

Regards,

Hans