+ Shimoda-San
> -----Original Message-----
> From: Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx>
> Sent: 13 December 2022 08:37
> To: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>; Biju Das
> <biju.das@xxxxxxxxxxxxxx>; linux-usb@xxxxxxxxxxxxxxx; Geert Uytterhoeven
> <geert+renesas@xxxxxxxxx>; Fabrizio Castro
> <fabrizio.castro.jz@xxxxxxxxxxx>; linux-renesas-soc@xxxxxxxxxxxxxxx;
> stable@xxxxxxxxxxxxxxx
> 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 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
