* [PATCH] usb: typec: tcpci: Don't skip cleanup in .remove() on error
@ 2022-05-02 8:04 Uwe Kleine-König
2022-05-02 12:27 ` Heikki Krogerus
` (2 more replies)
0 siblings, 3 replies; 4+ messages in thread
From: Uwe Kleine-König @ 2022-05-02 8:04 UTC (permalink / raw)
To: Guenter Roeck, Heikki Krogerus
Cc: Jun Li, Greg Kroah-Hartman, linux-usb, kernel
Returning an error value in an i2c remove callback results in an error
message being emitted by the i2c core, but otherwise it doesn't make a
difference. The device goes away anyhow and the devm cleanups are
called.
In this case the remove callback even returns early without stopping the
tcpm worker thread and various timers. A work scheduled on the work
queue, or a firing timer after tcpci_remove() returned probably results
in a use-after-free situation because the regmap and driver data were
freed. So better make sure that tcpci_unregister_port() is called even
if disabling the irq failed.
Also emit a more specific error message instead of the i2c core's
"remove failed (EIO), will be ignored" and return 0 to suppress the
core's warning.
This patch is (also) a preparation for making i2c remove callbacks
return void.
Fixes: 3ba76256fc4e ("usb: typec: tcpci: mask event interrupts when remove driver")
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
drivers/usb/typec/tcpm/tcpci.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/usb/typec/tcpm/tcpci.c b/drivers/usb/typec/tcpm/tcpci.c
index e07d26a3cd8e..f33e08eb7670 100644
--- a/drivers/usb/typec/tcpm/tcpci.c
+++ b/drivers/usb/typec/tcpm/tcpci.c
@@ -877,7 +877,7 @@ static int tcpci_remove(struct i2c_client *client)
/* Disable chip interrupts before unregistering port */
err = tcpci_write16(chip->tcpci, TCPC_ALERT_MASK, 0);
if (err < 0)
- return err;
+ dev_warn(&client->dev, "Failed to disable irqs (%pe)\n", ERR_PTR(err));
tcpci_unregister_port(chip->tcpci);
base-commit: 3123109284176b1532874591f7c81f3837bbdc17
--
2.35.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] usb: typec: tcpci: Don't skip cleanup in .remove() on error
2022-05-02 8:04 [PATCH] usb: typec: tcpci: Don't skip cleanup in .remove() on error Uwe Kleine-König
@ 2022-05-02 12:27 ` Heikki Krogerus
2022-05-02 13:30 ` Guenter Roeck
2022-05-05 21:26 ` Guenter Roeck
2 siblings, 0 replies; 4+ messages in thread
From: Heikki Krogerus @ 2022-05-02 12:27 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Guenter Roeck, Jun Li, Greg Kroah-Hartman, linux-usb, kernel
On Mon, May 02, 2022 at 10:04:56AM +0200, Uwe Kleine-König wrote:
> Returning an error value in an i2c remove callback results in an error
> message being emitted by the i2c core, but otherwise it doesn't make a
> difference. The device goes away anyhow and the devm cleanups are
> called.
>
> In this case the remove callback even returns early without stopping the
> tcpm worker thread and various timers. A work scheduled on the work
> queue, or a firing timer after tcpci_remove() returned probably results
> in a use-after-free situation because the regmap and driver data were
> freed. So better make sure that tcpci_unregister_port() is called even
> if disabling the irq failed.
>
> Also emit a more specific error message instead of the i2c core's
> "remove failed (EIO), will be ignored" and return 0 to suppress the
> core's warning.
>
> This patch is (also) a preparation for making i2c remove callbacks
> return void.
>
> Fixes: 3ba76256fc4e ("usb: typec: tcpci: mask event interrupts when remove driver")
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
Acked-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> ---
> drivers/usb/typec/tcpm/tcpci.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/usb/typec/tcpm/tcpci.c b/drivers/usb/typec/tcpm/tcpci.c
> index e07d26a3cd8e..f33e08eb7670 100644
> --- a/drivers/usb/typec/tcpm/tcpci.c
> +++ b/drivers/usb/typec/tcpm/tcpci.c
> @@ -877,7 +877,7 @@ static int tcpci_remove(struct i2c_client *client)
> /* Disable chip interrupts before unregistering port */
> err = tcpci_write16(chip->tcpci, TCPC_ALERT_MASK, 0);
> if (err < 0)
> - return err;
> + dev_warn(&client->dev, "Failed to disable irqs (%pe)\n", ERR_PTR(err));
>
> tcpci_unregister_port(chip->tcpci);
>
>
> base-commit: 3123109284176b1532874591f7c81f3837bbdc17
> --
> 2.35.1
thanks,
--
heikki
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] usb: typec: tcpci: Don't skip cleanup in .remove() on error
2022-05-02 8:04 [PATCH] usb: typec: tcpci: Don't skip cleanup in .remove() on error Uwe Kleine-König
2022-05-02 12:27 ` Heikki Krogerus
@ 2022-05-02 13:30 ` Guenter Roeck
2022-05-05 21:26 ` Guenter Roeck
2 siblings, 0 replies; 4+ messages in thread
From: Guenter Roeck @ 2022-05-02 13:30 UTC (permalink / raw)
To: Uwe Kleine-König, Heikki Krogerus
Cc: Jun Li, Greg Kroah-Hartman, linux-usb, kernel
On 5/2/22 01:04, Uwe Kleine-König wrote:
> Returning an error value in an i2c remove callback results in an error
> message being emitted by the i2c core, but otherwise it doesn't make a
> difference. The device goes away anyhow and the devm cleanups are
> called.
>
> In this case the remove callback even returns early without stopping the
> tcpm worker thread and various timers. A work scheduled on the work
> queue, or a firing timer after tcpci_remove() returned probably results
> in a use-after-free situation because the regmap and driver data were
> freed. So better make sure that tcpci_unregister_port() is called even
> if disabling the irq failed.
>
> Also emit a more specific error message instead of the i2c core's
> "remove failed (EIO), will be ignored" and return 0 to suppress the
> core's warning.
>
> This patch is (also) a preparation for making i2c remove callbacks
> return void.
>
> Fixes: 3ba76256fc4e ("usb: typec: tcpci: mask event interrupts when remove driver")
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
Reviewed-by: Guenter Roeck <linux@roeck-us.net>
> ---
> drivers/usb/typec/tcpm/tcpci.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/usb/typec/tcpm/tcpci.c b/drivers/usb/typec/tcpm/tcpci.c
> index e07d26a3cd8e..f33e08eb7670 100644
> --- a/drivers/usb/typec/tcpm/tcpci.c
> +++ b/drivers/usb/typec/tcpm/tcpci.c
> @@ -877,7 +877,7 @@ static int tcpci_remove(struct i2c_client *client)
> /* Disable chip interrupts before unregistering port */
> err = tcpci_write16(chip->tcpci, TCPC_ALERT_MASK, 0);
> if (err < 0)
> - return err;
> + dev_warn(&client->dev, "Failed to disable irqs (%pe)\n", ERR_PTR(err));
>
> tcpci_unregister_port(chip->tcpci);
>
>
> base-commit: 3123109284176b1532874591f7c81f3837bbdc17
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] usb: typec: tcpci: Don't skip cleanup in .remove() on error
2022-05-02 8:04 [PATCH] usb: typec: tcpci: Don't skip cleanup in .remove() on error Uwe Kleine-König
2022-05-02 12:27 ` Heikki Krogerus
2022-05-02 13:30 ` Guenter Roeck
@ 2022-05-05 21:26 ` Guenter Roeck
2 siblings, 0 replies; 4+ messages in thread
From: Guenter Roeck @ 2022-05-05 21:26 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Heikki Krogerus, Jun Li, Greg Kroah-Hartman, linux-usb, kernel
On Mon, May 02, 2022 at 10:04:56AM +0200, Uwe Kleine-König wrote:
> Returning an error value in an i2c remove callback results in an error
> message being emitted by the i2c core, but otherwise it doesn't make a
> difference. The device goes away anyhow and the devm cleanups are
> called.
>
> In this case the remove callback even returns early without stopping the
> tcpm worker thread and various timers. A work scheduled on the work
> queue, or a firing timer after tcpci_remove() returned probably results
> in a use-after-free situation because the regmap and driver data were
> freed. So better make sure that tcpci_unregister_port() is called even
> if disabling the irq failed.
>
> Also emit a more specific error message instead of the i2c core's
> "remove failed (EIO), will be ignored" and return 0 to suppress the
> core's warning.
>
> This patch is (also) a preparation for making i2c remove callbacks
> return void.
>
> Fixes: 3ba76256fc4e ("usb: typec: tcpci: mask event interrupts when remove driver")
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
Reviewed-by: Guenter Roeck <linux@roeck-us.net>
> ---
> drivers/usb/typec/tcpm/tcpci.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/usb/typec/tcpm/tcpci.c b/drivers/usb/typec/tcpm/tcpci.c
> index e07d26a3cd8e..f33e08eb7670 100644
> --- a/drivers/usb/typec/tcpm/tcpci.c
> +++ b/drivers/usb/typec/tcpm/tcpci.c
> @@ -877,7 +877,7 @@ static int tcpci_remove(struct i2c_client *client)
> /* Disable chip interrupts before unregistering port */
> err = tcpci_write16(chip->tcpci, TCPC_ALERT_MASK, 0);
> if (err < 0)
> - return err;
> + dev_warn(&client->dev, "Failed to disable irqs (%pe)\n", ERR_PTR(err));
>
> tcpci_unregister_port(chip->tcpci);
>
>
> base-commit: 3123109284176b1532874591f7c81f3837bbdc17
> --
> 2.35.1
>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2022-05-05 21:26 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-05-02 8:04 [PATCH] usb: typec: tcpci: Don't skip cleanup in .remove() on error Uwe Kleine-König
2022-05-02 12:27 ` Heikki Krogerus
2022-05-02 13:30 ` Guenter Roeck
2022-05-05 21:26 ` Guenter Roeck
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).