public inbox for stable@vger.kernel.org
 help / color / mirror / Atom feed
* [PATCH] usb: typec: fusb302: Switch to threaded IRQ handler
@ 2026-03-11 14:57 Alexey Charkov
  2026-03-12  9:10 ` Heikki Krogerus
  0 siblings, 1 reply; 7+ messages in thread
From: Alexey Charkov @ 2026-03-11 14:57 UTC (permalink / raw)
  To: Heikki Krogerus, Greg Kroah-Hartman, Sebastian Reichel
  Cc: linux-usb, linux-kernel, stable, Alexey Charkov

FUSB302 fails to probe with -EINVAL if its interrupt line is connected via
an I2C GPIO expander, such as TI TCA6416.

Switch the interrupt handler to a threaded one, which also works behind
such GPIO expanders.

Cc: stable@vger.kernel.org
Fixes: 309b6341d557 ("usb: typec: fusb302: Revert incorrect threaded irq fix")
Signed-off-by: Alexey Charkov <alchark@flipper.net>
---
 drivers/usb/typec/tcpm/fusb302.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/typec/tcpm/fusb302.c b/drivers/usb/typec/tcpm/fusb302.c
index 19ff8217818e..4f1f24737051 100644
--- a/drivers/usb/typec/tcpm/fusb302.c
+++ b/drivers/usb/typec/tcpm/fusb302.c
@@ -1755,8 +1755,8 @@ static int fusb302_probe(struct i2c_client *client)
 		goto destroy_workqueue;
 	}
 
-	ret = request_irq(chip->gpio_int_n_irq, fusb302_irq_intn,
-			  IRQF_TRIGGER_LOW, "fsc_interrupt_int_n", chip);
+	ret = request_threaded_irq(chip->gpio_int_n_irq, NULL, fusb302_irq_intn,
+				   IRQF_TRIGGER_LOW, "fsc_interrupt_int_n", chip);
 	if (ret < 0) {
 		dev_err(dev, "cannot request IRQ for GPIO Int_N, ret=%d", ret);
 		goto tcpm_unregister_port;

---
base-commit: 7109a2155340cc7b21f27e832ece6df03592f2e8
change-id: 20260311-fusb302-irq-316834765871

Best regards,
-- 
Alexey Charkov <alchark@flipper.net>


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH] usb: typec: fusb302: Switch to threaded IRQ handler
  2026-03-11 14:57 [PATCH] usb: typec: fusb302: Switch to threaded IRQ handler Alexey Charkov
@ 2026-03-12  9:10 ` Heikki Krogerus
  2026-03-12 10:49   ` Hans de Goede
  0 siblings, 1 reply; 7+ messages in thread
From: Heikki Krogerus @ 2026-03-12  9:10 UTC (permalink / raw)
  To: Alexey Charkov
  Cc: Greg Kroah-Hartman, Sebastian Reichel, Hans de Goede,
	Yongbo Zhang, linux-usb, linux-kernel, stable

Adding Hans and Yongbo,

Wed, Mar 11, 2026 at 06:57:12PM +0400, Alexey Charkov wrote:
> FUSB302 fails to probe with -EINVAL if its interrupt line is connected via
> an I2C GPIO expander, such as TI TCA6416.
> 
> Switch the interrupt handler to a threaded one, which also works behind
> such GPIO expanders.
> 
> Cc: stable@vger.kernel.org
> Fixes: 309b6341d557 ("usb: typec: fusb302: Revert incorrect threaded irq fix")
> Signed-off-by: Alexey Charkov <alchark@flipper.net>
> ---
>  drivers/usb/typec/tcpm/fusb302.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/typec/tcpm/fusb302.c b/drivers/usb/typec/tcpm/fusb302.c
> index 19ff8217818e..4f1f24737051 100644
> --- a/drivers/usb/typec/tcpm/fusb302.c
> +++ b/drivers/usb/typec/tcpm/fusb302.c
> @@ -1755,8 +1755,8 @@ static int fusb302_probe(struct i2c_client *client)
>  		goto destroy_workqueue;
>  	}
>  
> -	ret = request_irq(chip->gpio_int_n_irq, fusb302_irq_intn,
> -			  IRQF_TRIGGER_LOW, "fsc_interrupt_int_n", chip);
> +	ret = request_threaded_irq(chip->gpio_int_n_irq, NULL, fusb302_irq_intn,
> +				   IRQF_TRIGGER_LOW, "fsc_interrupt_int_n", chip);
>  	if (ret < 0) {
>  		dev_err(dev, "cannot request IRQ for GPIO Int_N, ret=%d", ret);
>  		goto tcpm_unregister_port;

While this looks okay to me, since this is now being changed back and
forth, let's look at it a bit more carefully.

Sebastian, Yongbo and Hans. I would really appreciate if you could
check this, and give you ack if it's okay.

Br,

-- 
heikki

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] usb: typec: fusb302: Switch to threaded IRQ handler
  2026-03-12  9:10 ` Heikki Krogerus
@ 2026-03-12 10:49   ` Hans de Goede
  2026-03-12 12:04     ` Sebastian Andrzej Siewior
  0 siblings, 1 reply; 7+ messages in thread
From: Hans de Goede @ 2026-03-12 10:49 UTC (permalink / raw)
  To: Heikki Krogerus, Alexey Charkov, Sebastian Andrzej Siewior
  Cc: Greg Kroah-Hartman, Sebastian Reichel, Yongbo Zhang, linux-usb,
	linux-kernel, stable

Hi,

+ Sebastian Siewior <bigeasy>

On 12-Mar-26 10:10, Heikki Krogerus wrote:
> Adding Hans and Yongbo,
> 
> Wed, Mar 11, 2026 at 06:57:12PM +0400, Alexey Charkov wrote:
>> FUSB302 fails to probe with -EINVAL if its interrupt line is connected via
>> an I2C GPIO expander, such as TI TCA6416.
>>
>> Switch the interrupt handler to a threaded one, which also works behind
>> such GPIO expanders.
>>
>> Cc: stable@vger.kernel.org
>> Fixes: 309b6341d557 ("usb: typec: fusb302: Revert incorrect threaded irq fix")
>> Signed-off-by: Alexey Charkov <alchark@flipper.net>
>> ---
>>  drivers/usb/typec/tcpm/fusb302.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/usb/typec/tcpm/fusb302.c b/drivers/usb/typec/tcpm/fusb302.c
>> index 19ff8217818e..4f1f24737051 100644
>> --- a/drivers/usb/typec/tcpm/fusb302.c
>> +++ b/drivers/usb/typec/tcpm/fusb302.c
>> @@ -1755,8 +1755,8 @@ static int fusb302_probe(struct i2c_client *client)
>>  		goto destroy_workqueue;
>>  	}
>>  
>> -	ret = request_irq(chip->gpio_int_n_irq, fusb302_irq_intn,
>> -			  IRQF_TRIGGER_LOW, "fsc_interrupt_int_n", chip);
>> +	ret = request_threaded_irq(chip->gpio_int_n_irq, NULL, fusb302_irq_intn,
>> +				   IRQF_TRIGGER_LOW, "fsc_interrupt_int_n", chip);
>>  	if (ret < 0) {
>>  		dev_err(dev, "cannot request IRQ for GPIO Int_N, ret=%d", ret);
>>  		goto tcpm_unregister_port;
> 
> While this looks okay to me, since this is now being changed back and
> forth, let's look at it a bit more carefully.
> 
> Sebastian, Yongbo and Hans. I would really appreciate if you could
> check this, and give you ack if it's okay.

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@gmail.com

which I did ack after some discussion.

But I wonder if we need to re-add the ONESHOT flag Sebastian Siewior <bigeasy>
added then ?

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 ?

Regards,

Hans







> 
> Br,
> 


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] usb: typec: fusb302: Switch to threaded IRQ handler
  2026-03-12 10:49   ` Hans de Goede
@ 2026-03-12 12:04     ` Sebastian Andrzej Siewior
  2026-03-13 16:21       ` Hans de Goede
  0 siblings, 1 reply; 7+ messages in thread
From: Sebastian Andrzej Siewior @ 2026-03-12 12:04 UTC (permalink / raw)
  To: Hans de Goede
  Cc: Heikki Krogerus, Alexey Charkov, Greg Kroah-Hartman,
	Sebastian Reichel, Yongbo Zhang, linux-usb, linux-kernel, stable

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@gmail.com

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] usb: typec: fusb302: Switch to threaded IRQ handler
  2026-03-12 12:04     ` Sebastian Andrzej Siewior
@ 2026-03-13 16:21       ` Hans de Goede
  2026-03-17 17:12         ` Sebastian Reichel
  0 siblings, 1 reply; 7+ messages in thread
From: Hans de Goede @ 2026-03-13 16:21 UTC (permalink / raw)
  To: Sebastian Andrzej Siewior
  Cc: Heikki Krogerus, Alexey Charkov, Greg Kroah-Hartman,
	Sebastian Reichel, Yongbo Zhang, linux-usb, linux-kernel, stable

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@gmail.com
> 
> 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



^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] usb: typec: fusb302: Switch to threaded IRQ handler
  2026-03-13 16:21       ` Hans de Goede
@ 2026-03-17 17:12         ` Sebastian Reichel
  2026-03-17 19:50           ` Hans de Goede
  0 siblings, 1 reply; 7+ messages in thread
From: Sebastian Reichel @ 2026-03-17 17:12 UTC (permalink / raw)
  To: Hans de Goede
  Cc: Sebastian Andrzej Siewior, Heikki Krogerus, Alexey Charkov,
	Greg Kroah-Hartman, Yongbo Zhang, linux-usb, linux-kernel, stable

[-- Attachment #1: Type: text/plain, Size: 4397 bytes --]

Hi,

On Fri, Mar 13, 2026 at 05:21:33PM +0100, Hans de Goede wrote:
> 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@gmail.com
> > 
> > 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).

After re-checking everything, I don't see anything special about the
fusb302 and wondering a bit why this is not an issue with other
devices like e.g. i2c-hid (which can also be wakeup sources, see
e.g. HID devices on your Qualcomm T14s). Does that have the same
issue and we are just not running into the race condition?

In that case it might be sensible to move the logic for "interrupt
function should only run after the device has been PM resumed" into
the IRQ core behind a flag and simplify the drivers?

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

I think with the threaded irq handler, the code could be simplified
to no longer use a worker thread. Instead the thread and the irq
handler can be merged. It seems sensible to do this in a separate
patch, though.

> [...]

Greetings,

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] usb: typec: fusb302: Switch to threaded IRQ handler
  2026-03-17 17:12         ` Sebastian Reichel
@ 2026-03-17 19:50           ` Hans de Goede
  0 siblings, 0 replies; 7+ messages in thread
From: Hans de Goede @ 2026-03-17 19:50 UTC (permalink / raw)
  To: Sebastian Reichel
  Cc: Sebastian Andrzej Siewior, Heikki Krogerus, Alexey Charkov,
	Greg Kroah-Hartman, Yongbo Zhang, linux-usb, linux-kernel, stable

Hi,

On 17-Mar-26 18:12, Sebastian Reichel wrote:
> Hi,
> 
> On Fri, Mar 13, 2026 at 05:21:33PM +0100, Hans de Goede wrote:
>> 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@gmail.com
>>>
>>> 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).
> 
> After re-checking everything, I don't see anything special about the
> fusb302 and wondering a bit why this is not an issue with other
> devices like e.g. i2c-hid (which can also be wakeup sources, see
> e.g. HID devices on your Qualcomm T14s). Does that have the same
> issue and we are just not running into the race condition?

drivers/hid/i2c-hid/i2c-hid-core.c: i2c_hid_core_suspend()
does a disable_irq() AFAIK that is not really allowed for
a wakeup source. The wakeup source thingie likely still
works on x86 because x86 hw has a whole separate event
system in the hw for wakeup events.

The i2c-hid-core.c drivers also seems to miss calls
to enable_irq_wake() so they never configure the IRQ
as a wakeup source, further suggesting the handling there
is incomplete.

But on pure s2idle systems where there is only the 1 IRQ
mechanism disable_irq() will result in no wake-ups AFAIK
(as well as enable_irq_wake() being necessary to avoid the
IRQ getting disabled in the final stages of suspend).

Note I'm not very familiar with all this, so the above
could be very wrong ...

> In that case it might be sensible to move the logic for "interrupt
> function should only run after the device has been PM resumed" into
> the IRQ core behind a flag and simplify the drivers?

I do believe that having some sort of helpers for this,
either in the IRQ core, or in some drivers/base code
or some such, would be useful yes.

>> So as far as I can see the current state of fusb302 code
>> is good as is.
> 
> I think with the threaded irq handler, the code could be simplified
> to no longer use a worker thread. Instead the thread and the irq
> handler can be merged. It seems sensible to do this in a separate
> patch, though.

No that cannot be done, because the work will get scheduled
from fsusb302_resume() if the IRQ ran (and set a flag) before
fsusb302_resume() run and thus before the i2c-adapter parent
was ready.

One might be tempted to replace the queuing of the work
in fsusb302_resume() with a direct call to the threaded
interrupt handler. But the code relies on the workqueue
mechanism to also ensure that the threaded interrupt
handler does not run in parallel with itself.

If we drop the disable_irq() + queue-work + work does
enable_irq() pair currently done and instead bail early if
not resumed for the threaded handler and still set the flag
to redo the interrupt handling from the resume handler
then we may end up with 2 threads running the interrupt
handling in parallel.

I guess we could take some mutex for the entire run
of the threaded handler to deal with that but I'm not
sure if that would be an improvement.

Regards,

Hans



^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2026-03-17 19:50 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-03-11 14:57 [PATCH] usb: typec: fusb302: Switch to threaded IRQ handler Alexey Charkov
2026-03-12  9:10 ` Heikki Krogerus
2026-03-12 10:49   ` Hans de Goede
2026-03-12 12:04     ` Sebastian Andrzej Siewior
2026-03-13 16:21       ` Hans de Goede
2026-03-17 17:12         ` Sebastian Reichel
2026-03-17 19:50           ` Hans de Goede

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox