Re: [PATCH v2] usb: typec: hd3ss3220: Fix NULL pointer crash

[Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx>]

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()?

If it is, would something like this help:

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 role sw */
-                       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