| Message ID | 1644923059-3619-1-git-send-email-fabrice.gasnier@foss.st.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] usb: dwc2: drd: fix soft connect when gadget is unconfigured | expand |
On 2/15/22 3:04 PM, Greg KH wrote: > On Tue, Feb 15, 2022 at 12:04:19PM +0100, Fabrice Gasnier wrote: >> When the gadget driver hasn't been (yet) configured, and the cable is >> connected to a HOST, the SFTDISCON gets cleared unconditionally, so the >> HOST tries to enumerate it. >> At the host side, this can result in a stuck USB port or worse. When >> getting lucky, some dmesg can be observed at the host side: >> new high-speed USB device number ... >> device descriptor read/64, error -110 >> >> Fix it in drd, by checking the enabled flag before calling >> dwc2_hsotg_core_connect(). It will be called later, once configured, >> by the normal flow: >> - udc_bind_to_driver >> - usb_gadget_connect >> - dwc2_hsotg_pullup >> - dwc2_hsotg_core_connect >> >> Fixes: 17f934024e84 ("usb: dwc2: override PHY input signals with usb role switch support") >> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@foss.st.com> >> --- >> Changes in v2: >> - Fix build error: 'struct dwc2_hsotg' has no member named 'enabled'; >> as reported by the kernel test robot. >> https://lore.kernel.org/all/202202112236.AwoOTtHO-lkp@intel.com/ >> Add dwc2_is_device_enabled() macro to handle this. >> --- >> drivers/usb/dwc2/core.h | 2 ++ >> drivers/usb/dwc2/drd.c | 6 ++++-- >> 2 files changed, 6 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h >> index 8a63da3..8a7751b 100644 >> --- a/drivers/usb/dwc2/core.h >> +++ b/drivers/usb/dwc2/core.h >> @@ -1418,6 +1418,7 @@ void dwc2_hsotg_core_connect(struct dwc2_hsotg *hsotg); >> void dwc2_hsotg_disconnect(struct dwc2_hsotg *dwc2); >> int dwc2_hsotg_set_test_mode(struct dwc2_hsotg *hsotg, int testmode); >> #define dwc2_is_device_connected(hsotg) (hsotg->connected) >> +#define dwc2_is_device_enabled(hsotg) ((hsotg)->enabled) > > Why the extra ()? dwc2_is_device_connected does not have it, so this > one probably should not either, right? Hi Greg, I was wondering the same, checkpatch complains without it: CHECK: Macro argument 'hsotg' may be better as '(hsotg)' to avoid precedence issues I can remove the extra () in a v3 if you wish ? Thanks for reviewing, Best Regards, Fabrice > > thanks, > > greg k-h >
On Tue, Feb 15, 2022 at 04:42:46PM +0100, Fabrice Gasnier wrote: > On 2/15/22 3:04 PM, Greg KH wrote: > > On Tue, Feb 15, 2022 at 12:04:19PM +0100, Fabrice Gasnier wrote: > >> When the gadget driver hasn't been (yet) configured, and the cable is > >> connected to a HOST, the SFTDISCON gets cleared unconditionally, so the > >> HOST tries to enumerate it. > >> At the host side, this can result in a stuck USB port or worse. When > >> getting lucky, some dmesg can be observed at the host side: > >> new high-speed USB device number ... > >> device descriptor read/64, error -110 > >> > >> Fix it in drd, by checking the enabled flag before calling > >> dwc2_hsotg_core_connect(). It will be called later, once configured, > >> by the normal flow: > >> - udc_bind_to_driver > >> - usb_gadget_connect > >> - dwc2_hsotg_pullup > >> - dwc2_hsotg_core_connect > >> > >> Fixes: 17f934024e84 ("usb: dwc2: override PHY input signals with usb role switch support") > >> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@foss.st.com> > >> --- > >> Changes in v2: > >> - Fix build error: 'struct dwc2_hsotg' has no member named 'enabled'; > >> as reported by the kernel test robot. > >> https://lore.kernel.org/all/202202112236.AwoOTtHO-lkp@intel.com/ > >> Add dwc2_is_device_enabled() macro to handle this. > >> --- > >> drivers/usb/dwc2/core.h | 2 ++ > >> drivers/usb/dwc2/drd.c | 6 ++++-- > >> 2 files changed, 6 insertions(+), 2 deletions(-) > >> > >> diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h > >> index 8a63da3..8a7751b 100644 > >> --- a/drivers/usb/dwc2/core.h > >> +++ b/drivers/usb/dwc2/core.h > >> @@ -1418,6 +1418,7 @@ void dwc2_hsotg_core_connect(struct dwc2_hsotg *hsotg); > >> void dwc2_hsotg_disconnect(struct dwc2_hsotg *dwc2); > >> int dwc2_hsotg_set_test_mode(struct dwc2_hsotg *hsotg, int testmode); > >> #define dwc2_is_device_connected(hsotg) (hsotg->connected) > >> +#define dwc2_is_device_enabled(hsotg) ((hsotg)->enabled) > > > > Why the extra ()? dwc2_is_device_connected does not have it, so this > > one probably should not either, right? > > Hi Greg, > > I was wondering the same, checkpatch complains without it: > > CHECK: Macro argument 'hsotg' may be better as '(hsotg)' to avoid > precedence issues checkpatch is wrong here, this is a structure pointer, not anything you could ever use that could be evaluated any other way. > I can remove the extra () in a v3 if you wish ? Please do. thanks, greg k-h
diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h index 8a63da3..8a7751b 100644 --- a/drivers/usb/dwc2/core.h +++ b/drivers/usb/dwc2/core.h @@ -1418,6 +1418,7 @@ void dwc2_hsotg_core_connect(struct dwc2_hsotg *hsotg); void dwc2_hsotg_disconnect(struct dwc2_hsotg *dwc2); int dwc2_hsotg_set_test_mode(struct dwc2_hsotg *hsotg, int testmode); #define dwc2_is_device_connected(hsotg) (hsotg->connected) +#define dwc2_is_device_enabled(hsotg) ((hsotg)->enabled) int dwc2_backup_device_registers(struct dwc2_hsotg *hsotg); int dwc2_restore_device_registers(struct dwc2_hsotg *hsotg, int remote_wakeup); int dwc2_gadget_enter_hibernation(struct dwc2_hsotg *hsotg); @@ -1454,6 +1455,7 @@ static inline int dwc2_hsotg_set_test_mode(struct dwc2_hsotg *hsotg, int testmode) { return 0; } #define dwc2_is_device_connected(hsotg) (0) +#define dwc2_is_device_enabled(hsotg) (0) static inline int dwc2_backup_device_registers(struct dwc2_hsotg *hsotg) { return 0; } static inline int dwc2_restore_device_registers(struct dwc2_hsotg *hsotg, diff --git a/drivers/usb/dwc2/drd.c b/drivers/usb/dwc2/drd.c index 1b39c47..d8d6493 100644 --- a/drivers/usb/dwc2/drd.c +++ b/drivers/usb/dwc2/drd.c @@ -130,8 +130,10 @@ static int dwc2_drd_role_sw_set(struct usb_role_switch *sw, enum usb_role role) already = dwc2_ovr_avalid(hsotg, true); } else if (role == USB_ROLE_DEVICE) { already = dwc2_ovr_bvalid(hsotg, true); - /* This clear DCTL.SFTDISCON bit */ - dwc2_hsotg_core_connect(hsotg); + if (dwc2_is_device_enabled(hsotg)) { + /* This clear DCTL.SFTDISCON bit */ + dwc2_hsotg_core_connect(hsotg); + } } else { if (dwc2_is_device_mode(hsotg)) { if (!dwc2_ovr_bvalid(hsotg, false))
When the gadget driver hasn't been (yet) configured, and the cable is connected to a HOST, the SFTDISCON gets cleared unconditionally, so the HOST tries to enumerate it. At the host side, this can result in a stuck USB port or worse. When getting lucky, some dmesg can be observed at the host side: new high-speed USB device number ... device descriptor read/64, error -110 Fix it in drd, by checking the enabled flag before calling dwc2_hsotg_core_connect(). It will be called later, once configured, by the normal flow: - udc_bind_to_driver - usb_gadget_connect - dwc2_hsotg_pullup - dwc2_hsotg_core_connect Fixes: 17f934024e84 ("usb: dwc2: override PHY input signals with usb role switch support") Signed-off-by: Fabrice Gasnier <fabrice.gasnier@foss.st.com> --- Changes in v2: - Fix build error: 'struct dwc2_hsotg' has no member named 'enabled'; as reported by the kernel test robot. https://lore.kernel.org/all/202202112236.AwoOTtHO-lkp@intel.com/ Add dwc2_is_device_enabled() macro to handle this. --- drivers/usb/dwc2/core.h | 2 ++ drivers/usb/dwc2/drd.c | 6 ++++-- 2 files changed, 6 insertions(+), 2 deletions(-)