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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



+ 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




[Index of Archives]     [Linux Kernel]     [Kernel Development Newbies]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Hiking]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux