linux-kernel - Re: [PATCH] usb: typec: fusb302: Revert incorrect threaded irq fix

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <aKbIZSCIK5mOB3Vz@kuha.fi.intel.com>
Date: Thu, 21 Aug 2025 10:19:01 +0300
From: Heikki Krogerus <heikki.krogerus@...ux.intel.com>
To: Sebastian Reichel <sebastian.reichel@...labora.com>
Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Yongbo Zhang <giraffesnn123@...il.com>,
	Hans de Goede <hansg@...nel.org>, linux-usb@...r.kernel.org,
	linux-kernel@...r.kernel.org, kernel@...labora.com
Subject: Re: [PATCH] usb: typec: fusb302: Revert incorrect threaded irq fix

On Mon, Aug 18, 2025 at 06:50:19PM +0200, Sebastian Reichel wrote:
> The fusb302 irq handler has been carefully optimized by Hans de Goede in
> commit 207338ec5a27 ("usb: typec: fusb302: Improve suspend/resume
> handling"). A recent 'fix' undid most of that work to avoid a virtio-gpio
> driver bug.
> 
> This reverts the incorrect fix, since it is of very low quality. It
> reverts the quirks from Hans change (and thus reintroduces the problems
> fixed by Hans) while keeping the overhead from the original change.
> 
> The proper fix to support using fusb302 with an interrupt line provided
> by virtio-gpio must be implemented in the virtio driver instead, which
> should support disabling the IRQ from the fusb302 interrupt routine.
> 
> Cc: Hans de Goede <hansg@...nel.org>
> Cc: Yongbo Zhang <giraffesnn123@...il.com>
> Fixes: 1c2d81bded19 ("usb: typec: fusb302: fix scheduling while atomic when using virtio-gpio")
> Signed-off-by: Sebastian Reichel <sebastian.reichel@...labora.com>

Reviewed-by: Heikki Krogerus <heikki.krogerus@...ux.intel.com>

> ---
> Note, that the referenced patch is in 6.17-rc, but not yet in any
> released kernel. It breaks boot for Radxa ROCK 5B when fusb302 is
> enabled in its device tree (strictly speaking that's not a regression,
> since upstream DT does not yet describe fusb302).
> ---
>  drivers/usb/typec/tcpm/fusb302.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/usb/typec/tcpm/fusb302.c b/drivers/usb/typec/tcpm/fusb302.c
> index a4ff2403ddd66fb7aaa27fd890533c7aff1dc163..870a71f953f6cd8dfc618caea56f72782e40ee1c 100644
> --- a/drivers/usb/typec/tcpm/fusb302.c
> +++ b/drivers/usb/typec/tcpm/fusb302.c
> @@ -1485,6 +1485,9 @@ 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;
> @@ -1627,6 +1630,7 @@ 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)
> @@ -1751,10 +1755,9 @@ static int fusb302_probe(struct i2c_client *client)
>  		goto destroy_workqueue;
>  	}
>  
> -	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);
> +	ret = request_irq(chip->gpio_int_n_irq, 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;
> @@ -1779,6 +1782,7 @@ 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);

-- 
heikki