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

[Sebastian Andrzej Siewior]

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 ?

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);

which would ensure that the handler runs as a thread and the interrupt
line is disable while it is active.
Then you could let fusb302_irq_intn() do what fusb302_irq_work() does.
Since it is a thread, mutex_lock() works here.
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 could do the trick.
 
> Regards,
> 
> Hans

Sebastian