Re: [PATCH] usb: typec: fusb302: fix scheduling while atomic when using virtio-gpio

[Hans de Goede <hansg@kernel.org>]

Hi,

On 4-Aug-25 8:00 PM, Sebastian Reichel wrote:
> +CC Hans de Goede
> 
> Hi,
> 
> On Wed, May 28, 2025 at 01:23:01PM +0300, Heikki Krogerus wrote:
>> On Mon, May 26, 2025 at 12:34:33PM +0800, Yongbo Zhang wrote:
>>> When the gpio irqchip connected to a slow bus(e.g., i2c bus or virtio
>>> bus), calling disable_irq_nosync() in top-half ISR handler will trigger
>>> the following kernel BUG:
>>>
>>> BUG: scheduling while atomic: RenderEngine/253/0x00010002
>>> ...
>>> Call trace:
>>>  dump_backtrace+0x0/0x1c8
>>>  show_stack+0x1c/0x2c
>>>  dump_stack_lvl+0xdc/0x12c
>>>  dump_stack+0x1c/0x64
>>>  __schedule_bug+0x64/0x80
>>>  schedule_debug+0x98/0x118
>>>  __schedule+0x68/0x704
>>>  schedule+0xa0/0xe8
>>>  schedule_timeout+0x38/0x124
>>>  wait_for_common+0xa4/0x134
>>>  wait_for_completion+0x1c/0x2c
>>>  _virtio_gpio_req+0xf8/0x198
>>>  virtio_gpio_irq_bus_sync_unlock+0x94/0xf0
>>>  __irq_put_desc_unlock+0x50/0x54
>>>  disable_irq_nosync+0x64/0x94
>>>  fusb302_irq_intn+0x24/0x84
>>>  __handle_irq_event_percpu+0x84/0x278
>>>  handle_irq_event+0x64/0x14c
>>>  handle_level_irq+0x134/0x1d4
>>>  generic_handle_domain_irq+0x40/0x68
>>>  virtio_gpio_event_vq+0xb0/0x130
>>>  vring_interrupt+0x7c/0x90
>>>  vm_interrupt+0x88/0xd8
>>>  __handle_irq_event_percpu+0x84/0x278
>>>  handle_irq_event+0x64/0x14c
>>>  handle_fasteoi_irq+0x110/0x210
>>>  __handle_domain_irq+0x80/0xd0
>>>  gic_handle_irq+0x78/0x154
>>>  el0_irq_naked+0x60/0x6c
>>>
>>> This patch replaces request_irq() with devm_request_threaded_irq() to
>>> avoid the use of disable_irq_nosync().
>>>
>>> Signed-off-by: Yongbo Zhang <giraffesnn123@gmail.com>
>>
>> Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
>>
>>> ---
> 
> I'm currently investigating a potential "regression" (quotes,
> because USB-C support is not yet enabled in the upstream board
> devicetree) with the Radxa ROCK 5B USB-C support after rebasing
> to latest master branch. I'm not yet sure, if this patch is at
> fault or totally unrelated, but please be aware that it undoes
> previous work from Hans de Goede to NOT use threaded IRQs:
> 
> 207338ec5a27 ("usb: typec: fusb302: Improve suspend/resume handling")
> 
> At the same time the fix from Yongbo Zhang misses cleaning up the
> now useless fusb302_irq_work() split, which had been introduced by
> Hans patch to have the hard IRQ as short as possible. With the
> interrupt handler being a thread itself, the code can just be called
> directly.

Yes the mentioned "usb: typec: fusb302: fix scheduling while atomic
when using virtio-gpio" change looks broken to me.

Calling disable_irq_nosync() from an interrupt handler is
perfectly fine and if the virtio GPIO controller cannot handle
that, then that is a bug inside the virtio GPIO controller not
in the the fusb302 driver.

The patch switches to using threaded interrupt handling, but that
does not fix the interrupt storm when level IRQs are used since
all the now threaded interrupt handler does is queue the work
which does the actual interrupt handling. So when the threaded
interrupt handler is done the interrupt source is not yet
cleared and the interrupt will immediately re-fire.

So IMHO the "usb: typec: fusb302: fix scheduling while atomic when
using virtio-gpio" change should be reverted.

Please submit a revert and please Cc me on future changes to fusb302
interrupt handling.

Regards,

Hans









> 
> Greetings,
> 
> -- Sebastian
> 
>>>  drivers/usb/typec/tcpm/fusb302.c | 12 ++++--------
>>>  1 file changed, 4 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/drivers/usb/typec/tcpm/fusb302.c b/drivers/usb/typec/tcpm/fusb302.c
>>> index f15c63d3a8f4..f2801279c4b5 100644
>>> --- a/drivers/usb/typec/tcpm/fusb302.c
>>> +++ b/drivers/usb/typec/tcpm/fusb302.c
>>> @@ -1477,9 +1477,6 @@ static irqreturn_t fusb302_irq_intn(int irq, void *dev_id)
>>>  	struct fusb302_chip *chip = dev_id;
>>>  	unsigned long flags;
>>>
>>> -	/* Disable our level triggered IRQ until our irq_work has cleared it */
>>> -	disable_irq_nosync(chip->gpio_int_n_irq);
>>> -
>>>  	spin_lock_irqsave(&chip->irq_lock, flags);
>>>  	if (chip->irq_suspended)
>>>  		chip->irq_while_suspended = true;
>>> @@ -1622,7 +1619,6 @@ static void fusb302_irq_work(struct work_struct *work)
>>>  	}
>>>  done:
>>>  	mutex_unlock(&chip->lock);
>>> -	enable_irq(chip->gpio_int_n_irq);
>>>  }
>>>
>>>  static int init_gpio(struct fusb302_chip *chip)
>>> @@ -1747,9 +1743,10 @@ static int fusb302_probe(struct i2c_client *client)
>>>  		goto destroy_workqueue;
>>>  	}
>>>
>>> -	ret = request_irq(chip->gpio_int_n_irq, fusb302_irq_intn,
>>> -			  IRQF_ONESHOT | IRQF_TRIGGER_LOW,
>>> -			  "fsc_interrupt_int_n", chip);
>>> +	ret = devm_request_threaded_irq(dev, chip->gpio_int_n_irq,
>>> +					NULL, fusb302_irq_intn,
>>> +					IRQF_ONESHOT | 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;
>>> @@ -1774,7 +1771,6 @@ static void fusb302_remove(struct i2c_client *client)
>>>  	struct fusb302_chip *chip = i2c_get_clientdata(client);
>>>
>>>  	disable_irq_wake(chip->gpio_int_n_irq);
>>> -	free_irq(chip->gpio_int_n_irq, chip);
>>>  	cancel_work_sync(&chip->irq_work);
>>>  	cancel_delayed_work_sync(&chip->bc_lvl_handler);
>>>  	tcpm_unregister_port(chip->tcpm_port);
>>> --
>>> 2.49.0
>>
>> -- 
>> heikki
>>