| Message ID | 20220128223603.2362621-1-aford173@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | usb: gadget: udc: renesas_usb3: Fix host to USB_ROLE_NONE transition | expand |
Hi Adam-san, Thank you for the patch! > From: Adam Ford, Sent: Saturday, January 29, 2022 7:36 AM > > The support the external role switch a variety of situations were > addressed, but the transition from USB_ROLE_HOST to USB_ROLE_NONE > leaves the host up which can cause some error messages when > switching from host to none, to gadget, to none, and then back > to host again. I think so. An external role switch in this driver is possible to enter the role to USB_ROLE_NONE. (Just a record for me: in other words, the "internal" role switch didn't enter the role to USB_ROLE_NONE.) > xhci-hcd ee000000.usb: Abort failed to stop command ring: -110 > xhci-hcd ee000000.usb: xHCI host controller not responding, assume dead > xhci-hcd ee000000.usb: HC died; cleaning up > usb 4-1: device not accepting address 6, error -108 > usb usb4-port1: couldn't allocate usb_device > > After this happens it will not act as a host again. > Fix this by releasing the host mode when transitioning to USB_ROLE_NONE. > > Fixes: 0604160d8c0b ("usb: gadget: udc: renesas_usb3: Enhance role switch support") > Signed-off-by: Adam Ford <aford173@gmail.com> > > diff --git a/drivers/usb/gadget/udc/renesas_usb3.c b/drivers/usb/gadget/udc/renesas_usb3.c > index 57d417a7c3e0..601829a6b4ba 100644 > --- a/drivers/usb/gadget/udc/renesas_usb3.c > +++ b/drivers/usb/gadget/udc/renesas_usb3.c > @@ -2378,6 +2378,8 @@ static void handle_ext_role_switch_states(struct device *dev, > switch (role) { > case USB_ROLE_NONE: > usb3->connection_state = USB_ROLE_NONE; > + if (cur_role == USB_ROLE_HOST) > + device_release_driver(host); The handle_ext_role_switch_states() with role = USB_ROLE_NONE seems to be called multiple times. So, we have to avoid multiple calling of device_release_driver() somehow. Best regards, Yoshihiro Shimoda > if (usb3->driver) > usb3_disconnect(usb3); > usb3_vbus_out(usb3, false); > -- > 2.32.0
On Sun, Jan 30, 2022 at 6:23 PM Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> wrote: > > Hi Adam-san, > > Thank you for the patch! > > > From: Adam Ford, Sent: Saturday, January 29, 2022 7:36 AM > > > > The support the external role switch a variety of situations were > > addressed, but the transition from USB_ROLE_HOST to USB_ROLE_NONE > > leaves the host up which can cause some error messages when > > switching from host to none, to gadget, to none, and then back > > to host again. > > I think so. An external role switch in this driver is possible to > enter the role to USB_ROLE_NONE. (Just a record for me: in other words, > the "internal" role switch didn't enter the role to USB_ROLE_NONE.) > > > xhci-hcd ee000000.usb: Abort failed to stop command ring: -110 > > xhci-hcd ee000000.usb: xHCI host controller not responding, assume dead > > xhci-hcd ee000000.usb: HC died; cleaning up > > usb 4-1: device not accepting address 6, error -108 > > usb usb4-port1: couldn't allocate usb_device > > > > After this happens it will not act as a host again. > > Fix this by releasing the host mode when transitioning to USB_ROLE_NONE. > > > > Fixes: 0604160d8c0b ("usb: gadget: udc: renesas_usb3: Enhance role switch support") > > Signed-off-by: Adam Ford <aford173@gmail.com> > > > > diff --git a/drivers/usb/gadget/udc/renesas_usb3.c b/drivers/usb/gadget/udc/renesas_usb3.c > > index 57d417a7c3e0..601829a6b4ba 100644 > > --- a/drivers/usb/gadget/udc/renesas_usb3.c > > +++ b/drivers/usb/gadget/udc/renesas_usb3.c > > @@ -2378,6 +2378,8 @@ static void handle_ext_role_switch_states(struct device *dev, > > switch (role) { > > case USB_ROLE_NONE: > > usb3->connection_state = USB_ROLE_NONE; > > + if (cur_role == USB_ROLE_HOST) > > + device_release_driver(host); > > The handle_ext_role_switch_states() with role = USB_ROLE_NONE seems > to be called multiple times. So, we have to avoid multiple calling of > device_release_driver() somehow. From what I can tell, it appears that device_release_driver ultimately calls __device_release_driver which checks for a NULL, and during the release sets it to NULL, so subsequent releases should just get ignored. Maybe I am missing something. adam > > Best regards, > Yoshihiro Shimoda > > > if (usb3->driver) > > usb3_disconnect(usb3); > > usb3_vbus_out(usb3, false); > > -- > > 2.32.0 >
Hi Adam-san, > From: Adam Ford, Sent: Monday, January 31, 2022 10:01 AM > > On Sun, Jan 30, 2022 at 6:23 PM Yoshihiro Shimoda > <yoshihiro.shimoda.uh@renesas.com> wrote: <snip> > > > diff --git a/drivers/usb/gadget/udc/renesas_usb3.c b/drivers/usb/gadget/udc/renesas_usb3.c > > > index 57d417a7c3e0..601829a6b4ba 100644 > > > --- a/drivers/usb/gadget/udc/renesas_usb3.c > > > +++ b/drivers/usb/gadget/udc/renesas_usb3.c > > > @@ -2378,6 +2378,8 @@ static void handle_ext_role_switch_states(struct device *dev, > > > switch (role) { > > > case USB_ROLE_NONE: > > > usb3->connection_state = USB_ROLE_NONE; > > > + if (cur_role == USB_ROLE_HOST) > > > + device_release_driver(host); > > > > The handle_ext_role_switch_states() with role = USB_ROLE_NONE seems > > to be called multiple times. So, we have to avoid multiple calling of > > device_release_driver() somehow. > > From what I can tell, it appears that device_release_driver ultimately > calls __device_release_driver which checks for a NULL, and during the > release sets it to NULL, so subsequent releases should just get > ignored. Maybe I am missing something. Thank you for your comment. You're correct. I'm sorry, I didn't check the __device_release_driver() behavior. This patch looks good to me. So, Reviewed-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> Best regards, Yoshihiro Shimoda
diff --git a/drivers/usb/gadget/udc/renesas_usb3.c b/drivers/usb/gadget/udc/renesas_usb3.c index 57d417a7c3e0..601829a6b4ba 100644 --- a/drivers/usb/gadget/udc/renesas_usb3.c +++ b/drivers/usb/gadget/udc/renesas_usb3.c @@ -2378,6 +2378,8 @@ static void handle_ext_role_switch_states(struct device *dev, switch (role) { case USB_ROLE_NONE: usb3->connection_state = USB_ROLE_NONE; + if (cur_role == USB_ROLE_HOST) + device_release_driver(host); if (usb3->driver) usb3_disconnect(usb3); usb3_vbus_out(usb3, false);
The support the external role switch a variety of situations were addressed, but the transition from USB_ROLE_HOST to USB_ROLE_NONE leaves the host up which can cause some error messages when switching from host to none, to gadget, to none, and then back to host again. xhci-hcd ee000000.usb: Abort failed to stop command ring: -110 xhci-hcd ee000000.usb: xHCI host controller not responding, assume dead xhci-hcd ee000000.usb: HC died; cleaning up usb 4-1: device not accepting address 6, error -108 usb usb4-port1: couldn't allocate usb_device After this happens it will not act as a host again. Fix this by releasing the host mode when transitioning to USB_ROLE_NONE. Fixes: 0604160d8c0b ("usb: gadget: udc: renesas_usb3: Enhance role switch support") Signed-off-by: Adam Ford <aford173@gmail.com>