+ Shimoda-San
-----Original Message----- From: Heikki Krogerus heikki.krogerus@linux.intel.com Sent: 13 December 2022 08:37 To: Biju Das biju.das.jz@bp.renesas.com Cc: Greg Kroah-Hartman gregkh@linuxfoundation.org; Biju Das biju.das@bp.renesas.com; linux-usb@vger.kernel.org; Geert Uytterhoeven geert+renesas@glider.be; Fabrizio Castro fabrizio.castro.jz@renesas.com; linux-renesas-soc@vger.kernel.org; stable@vger.kernel.org Subject: Re: [PATCH v2] usb: typec: hd3ss3220: Fix NULL pointer crash
Hi,
On Mon, Dec 12, 2022 at 10:54:25AM +0000, Biju Das wrote:
Looks It is a bug in renesas_usb3.c rather than this driver.
But how we will prevent hd3ss3220_set_role being called after usb_role_switch_unregister(usb3->role_sw) from renesas_usb3.c driver??
Normally that should not be a problem. When you get a reference to the role switch, also the reference count of the switch driver module (on top of the device) is incremented.
From where is usb_role_switch_unregister() being called in this case - is it renesas_usb3_probe()?
Yes, that os correct.
If it is, would something like this help:
Shimoda-San,
What is your thoughts on Heikki's proposal as below? It looks good to me.
diff --git a/drivers/usb/gadget/udc/renesas_usb3.c b/drivers/usb/gadget/udc/renesas_usb3.c index 615ba0a6fbee1..d2e01f7cfef11 100644 --- a/drivers/usb/gadget/udc/renesas_usb3.c +++ b/drivers/usb/gadget/udc/renesas_usb3.c @@ -2907,18 +2907,13 @@ static int renesas_usb3_probe(struct platform_device *pdev) renesas_usb3_role_switch_desc.driver_data = usb3;
INIT_WORK(&usb3->role_work, renesas_usb3_role_work);
usb3->role_sw = usb_role_switch_register(&pdev->dev,&renesas_usb3_role_switch_desc);if (!IS_ERR(usb3->role_sw)) {usb3->host_dev = usb_of_get_companion_dev(&pdev->dev);if (!usb3->host_dev) {/* If not found, this driver will not use a rolesw */
usb_role_switch_unregister(usb3->role_sw);usb3->role_sw = NULL;}} else {
usb3->host_dev = usb_of_get_companion_dev(&pdev->dev);if (usb3->host_dev)usb3->role_sw = usb_role_switch_register(&pdev->dev,&renesas_usb3_role_switch_desc);
if (IS_ERR(usb3->role_sw)) usb3->role_sw = NULL;
} usb3->workaround_for_vbus = priv->workaround_for_vbus;Do we need to add additional check for "fwnode_usb_role_switch_get" and "usb_role_switch_get" to return error if there is no registered role_switch device Like the scenario above??
No. The switch is always an optional resource.
Error means that there is a switch that you can control, but you can't get a handle to it for some reason.
NULL means you don't need to worry about it - there is no switch on your platform that you could control.
thanks,
-- heikki