| Message ID | 20220804151001.23612-4-johan+linaro@kernel.org (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | c06795f114a6c4a423b11c9d9bbeb77ecbfbaa8b |
| Headers | show |
| Series | usb: dwc3: qcom: fix wakeup implementation | expand |
Hi John, On 8/4/22 08:09, Johan Hovold wrote: > A recent change added a dependency to the USB host stack and broke > gadget-only builds of the driver. > > Fixes: 6895ea55c385 ("usb: dwc3: qcom: Configure wakeup interrupts during suspend") > Reported-by: Randy Dunlap <rdunlap@infradead.org> > Signed-off-by: Johan Hovold <johan+linaro@kernel.org> LGTM. Thanks. Reviewed-by: Randy Dunlap <rdunlap@infradead.org> > --- > > Changes in v2 > - new patch > > drivers/usb/dwc3/dwc3-qcom.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c > index be2e3dd36440..e9364141661b 100644 > --- a/drivers/usb/dwc3/dwc3-qcom.c > +++ b/drivers/usb/dwc3/dwc3-qcom.c > @@ -310,8 +310,11 @@ static enum usb_device_speed dwc3_qcom_read_usb2_speed(struct dwc3_qcom *qcom) > * currently supports only 1 port per controller. So > * this is sufficient. > */ > +#ifdef CONFIG_USB > udev = usb_hub_find_child(hcd->self.root_hub, 1); > - > +#else > + udev = NULL; > +#endif > if (!udev) > return USB_SPEED_UNKNOWN; >
On Thu, Aug 04, 2022 at 05:09:55PM +0200, Johan Hovold wrote: > A recent change added a dependency to the USB host stack and broke > gadget-only builds of the driver. > > Fixes: 6895ea55c385 ("usb: dwc3: qcom: Configure wakeup interrupts during suspend") > Reported-by: Randy Dunlap <rdunlap@infradead.org> > Signed-off-by: Johan Hovold <johan+linaro@kernel.org> > --`- > > Changes in v2 > - new patch > > drivers/usb/dwc3/dwc3-qcom.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c > index be2e3dd36440..e9364141661b 100644 > --- a/drivers/usb/dwc3/dwc3-qcom.c > +++ b/drivers/usb/dwc3/dwc3-qcom.c > @@ -310,8 +310,11 @@ static enum usb_device_speed dwc3_qcom_read_usb2_speed(struct dwc3_qcom *qcom) > * currently supports only 1 port per controller. So > * this is sufficient. > */ > +#ifdef CONFIG_USB > udev = usb_hub_find_child(hcd->self.root_hub, 1); > - > +#else > + udev = NULL; > +#endif Perhaps the check should be moved to the caller instead? This function still references "usb_hcd" struct and I don't think that's intended for gadget only mode. Thanks, Mani > if (!udev) > return USB_SPEED_UNKNOWN; > > -- > 2.35.1 >
On Sat, Aug 06, 2022 at 07:45:36PM +0530, Manivannan Sadhasivam wrote: > On Thu, Aug 04, 2022 at 05:09:55PM +0200, Johan Hovold wrote: > > A recent change added a dependency to the USB host stack and broke > > gadget-only builds of the driver. > > > > Fixes: 6895ea55c385 ("usb: dwc3: qcom: Configure wakeup interrupts during suspend") > > Reported-by: Randy Dunlap <rdunlap@infradead.org> > > Signed-off-by: Johan Hovold <johan+linaro@kernel.org> > > --`- > > > > Changes in v2 > > - new patch > > > > drivers/usb/dwc3/dwc3-qcom.c | 5 ++++- > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c > > index be2e3dd36440..e9364141661b 100644 > > --- a/drivers/usb/dwc3/dwc3-qcom.c > > +++ b/drivers/usb/dwc3/dwc3-qcom.c > > @@ -310,8 +310,11 @@ static enum usb_device_speed dwc3_qcom_read_usb2_speed(struct dwc3_qcom *qcom) > > * currently supports only 1 port per controller. So > > * this is sufficient. > > */ > > +#ifdef CONFIG_USB > > udev = usb_hub_find_child(hcd->self.root_hub, 1); > > - > > +#else > > + udev = NULL; > > +#endif > > Perhaps the check should be moved to the caller instead? This function still > references "usb_hcd" struct and I don't think that's intended for gadget only > mode. That wouldn't help with the build failure, which is what this patch is addressing. > > if (!udev) > > return USB_SPEED_UNKNOWN; > > > > -- > > 2.35.1 Johan
On Sat, Aug 06, 2022 at 06:04:21PM +0200, Johan Hovold wrote: > On Sat, Aug 06, 2022 at 07:45:36PM +0530, Manivannan Sadhasivam wrote: > > On Thu, Aug 04, 2022 at 05:09:55PM +0200, Johan Hovold wrote: > > > A recent change added a dependency to the USB host stack and broke > > > gadget-only builds of the driver. > > > > > > Fixes: 6895ea55c385 ("usb: dwc3: qcom: Configure wakeup interrupts during suspend") > > > Reported-by: Randy Dunlap <rdunlap@infradead.org> > > > Signed-off-by: Johan Hovold <johan+linaro@kernel.org> > > > --`- > > > > > > Changes in v2 > > > - new patch > > > > > > drivers/usb/dwc3/dwc3-qcom.c | 5 ++++- > > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c > > > index be2e3dd36440..e9364141661b 100644 > > > --- a/drivers/usb/dwc3/dwc3-qcom.c > > > +++ b/drivers/usb/dwc3/dwc3-qcom.c > > > @@ -310,8 +310,11 @@ static enum usb_device_speed dwc3_qcom_read_usb2_speed(struct dwc3_qcom *qcom) > > > * currently supports only 1 port per controller. So > > > * this is sufficient. > > > */ > > > +#ifdef CONFIG_USB > > > udev = usb_hub_find_child(hcd->self.root_hub, 1); > > > - > > > +#else > > > + udev = NULL; > > > +#endif > > > > Perhaps the check should be moved to the caller instead? This function still > > references "usb_hcd" struct and I don't think that's intended for gadget only > > mode. > > That wouldn't help with the build failure, which is what this patch is > addressing. > I should've put it clearly. You should guard the entire function and not just usb_hub_find_child(). This way it becomes clear that this whole function depends on the USB host functionality. Like, #ifdef CONFIG_USB static enum usb_device_speed dwc3_qcom_read_usb2_speed(struct dwc3_qcom *qcom) { ... } #elif static enum usb_device_speed dwc3_qcom_read_usb2_speed(struct dwc3_qcom *qcom) { return USB_SPEED_UNKNOWN } #endif Thanks, Mani > > > if (!udev) > > > return USB_SPEED_UNKNOWN; > > > > > > -- > > > 2.35.1 > > Johan
On Sat, Aug 06, 2022 at 10:12:50PM +0530, Manivannan Sadhasivam wrote: > On Sat, Aug 06, 2022 at 06:04:21PM +0200, Johan Hovold wrote: > > On Sat, Aug 06, 2022 at 07:45:36PM +0530, Manivannan Sadhasivam wrote: > > > On Thu, Aug 04, 2022 at 05:09:55PM +0200, Johan Hovold wrote: > > > > A recent change added a dependency to the USB host stack and broke > > > > gadget-only builds of the driver. > > > > > > > > Fixes: 6895ea55c385 ("usb: dwc3: qcom: Configure wakeup interrupts during suspend") > > > > Reported-by: Randy Dunlap <rdunlap@infradead.org> > > > > Signed-off-by: Johan Hovold <johan+linaro@kernel.org> > > > > --`- > > > > > > > > Changes in v2 > > > > - new patch > > > > > > > > drivers/usb/dwc3/dwc3-qcom.c | 5 ++++- > > > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c > > > > index be2e3dd36440..e9364141661b 100644 > > > > --- a/drivers/usb/dwc3/dwc3-qcom.c > > > > +++ b/drivers/usb/dwc3/dwc3-qcom.c > > > > @@ -310,8 +310,11 @@ static enum usb_device_speed dwc3_qcom_read_usb2_speed(struct dwc3_qcom *qcom) > > > > * currently supports only 1 port per controller. So > > > > * this is sufficient. > > > > */ > > > > +#ifdef CONFIG_USB > > > > udev = usb_hub_find_child(hcd->self.root_hub, 1); > > > > - > > > > +#else > > > > + udev = NULL; > > > > +#endif > > > > > > Perhaps the check should be moved to the caller instead? This function still > > > references "usb_hcd" struct and I don't think that's intended for gadget only > > > mode. > > > > That wouldn't help with the build failure, which is what this patch is > > addressing. > > > > I should've put it clearly. You should guard the entire function and not just > usb_hub_find_child(). This way it becomes clear that this whole function depends > on the USB host functionality. Like, > > #ifdef CONFIG_USB > static enum usb_device_speed dwc3_qcom_read_usb2_speed(struct dwc3_qcom *qcom) > { > ... > } > #elif > static enum usb_device_speed dwc3_qcom_read_usb2_speed(struct dwc3_qcom *qcom) > { > return USB_SPEED_UNKNOWN > } > #endif Yeah, that's what Krishna suggested but I wanted to keep the ifdeffery minimal when fixing the build failure (we generally try to avoid adding stub functions to implementation files). Non-host mode was clearly never considered when adding the function in question as the code blows up otherwise regardless of whether CONFIG_USB is enabled or not (and a later patch in the series addresses that). But I'll revisit this too in a couple of weeks. Thanks for reviewing. Johan
On Thu, Aug 04, 2022 at 05:09:55PM +0200, Johan Hovold wrote: > A recent change added a dependency to the USB host stack and broke > gadget-only builds of the driver. > > Fixes: 6895ea55c385 ("usb: dwc3: qcom: Configure wakeup interrupts during suspend") > Reported-by: Randy Dunlap <rdunlap@infradead.org> > Signed-off-by: Johan Hovold <johan+linaro@kernel.org> > --- > > Changes in v2 > - new patch > > drivers/usb/dwc3/dwc3-qcom.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c > index be2e3dd36440..e9364141661b 100644 > --- a/drivers/usb/dwc3/dwc3-qcom.c > +++ b/drivers/usb/dwc3/dwc3-qcom.c > @@ -310,8 +310,11 @@ static enum usb_device_speed dwc3_qcom_read_usb2_speed(struct dwc3_qcom *qcom) > * currently supports only 1 port per controller. So > * this is sufficient. > */ > +#ifdef CONFIG_USB > udev = usb_hub_find_child(hcd->self.root_hub, 1); If a gadget driver needs this for some reason, then the #ifdef should be put in a .h file, not in a .c file. But step back a minute and ask why a host-config-only function is being called when a device is in gadget-only mode? This feels like a design/logic issue in this file, NOT something to paper over with a #ifdef in a .c file This implies that if this device is NOT in a host configuration, then the suspend path of it is not configured properly at all, as why would it be checking or caring about this at all if this is in gadget-only mode? Something else is wrong here, let's fix the root problem please. Maybe this driver should just never be built in gadget-only mode, as it is never intended to support that option? thanks, greg k-h
On Mon, Aug 08, 2022 at 03:05:36PM +0200, Greg Kroah-Hartman wrote: > On Thu, Aug 04, 2022 at 05:09:55PM +0200, Johan Hovold wrote: > > A recent change added a dependency to the USB host stack and broke > > gadget-only builds of the driver. > > > > Fixes: 6895ea55c385 ("usb: dwc3: qcom: Configure wakeup interrupts during suspend") > > Reported-by: Randy Dunlap <rdunlap@infradead.org> > > Signed-off-by: Johan Hovold <johan+linaro@kernel.org> > > --- > > > > Changes in v2 > > - new patch > > > > drivers/usb/dwc3/dwc3-qcom.c | 5 ++++- > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c > > index be2e3dd36440..e9364141661b 100644 > > --- a/drivers/usb/dwc3/dwc3-qcom.c > > +++ b/drivers/usb/dwc3/dwc3-qcom.c > > @@ -310,8 +310,11 @@ static enum usb_device_speed dwc3_qcom_read_usb2_speed(struct dwc3_qcom *qcom) > > * currently supports only 1 port per controller. So > > * this is sufficient. > > */ > > +#ifdef CONFIG_USB > > udev = usb_hub_find_child(hcd->self.root_hub, 1); > > If a gadget driver needs this for some reason, then the #ifdef should be > put in a .h file, not in a .c file. Yeah, if we're keeping this long-term then yes, and possibly also otherwise. > But step back a minute and ask why a host-config-only function is being > called when a device is in gadget-only mode? This feels like a > design/logic issue in this file, NOT something to paper over with a > #ifdef in a .c file We're not as I'm fixing that bug in later in the series. I should probably have put this one after that fix, but figured fixing the build was more important than a harder-to-hit NULL-deref due to non-host mode not being considered when the offending series was merged. > This implies that if this device is NOT in a host configuration, then > the suspend path of it is not configured properly at all, as why would > it be checking or caring about this at all if this is in gadget-only > mode? Right, so see path 6/9 which addresses this by only calling this hack when in host mode: https://lore.kernel.org/all/20220804151001.23612-7-johan+linaro@kernel.org/ > Something else is wrong here, let's fix the root problem please. Maybe > this driver should just never be built in gadget-only mode, as it is > never intended to support that option? The problem is commit 6895ea55c385 ("usb: dwc3: qcom: Configure wakeup interrupts during suspend"), which I considered simply reverting but as that breaks suspend completely on some boards I decided to try and fix it up while we work on a proper long-term solution (i.e. for how the dwc/xhci layers should be communicating to implement this). Remember that it took two years and 21 revisions to get to the state we're at now after you merged the wakeup series in June. Johan
On Mon, Aug 08, 2022 at 03:34:10PM +0200, Johan Hovold wrote: > On Mon, Aug 08, 2022 at 03:05:36PM +0200, Greg Kroah-Hartman wrote: > > On Thu, Aug 04, 2022 at 05:09:55PM +0200, Johan Hovold wrote: > > > A recent change added a dependency to the USB host stack and broke > > > gadget-only builds of the driver. > > > > > > Fixes: 6895ea55c385 ("usb: dwc3: qcom: Configure wakeup interrupts during suspend") > > > Reported-by: Randy Dunlap <rdunlap@infradead.org> > > > Signed-off-by: Johan Hovold <johan+linaro@kernel.org> > > > --- > > > > > > Changes in v2 > > > - new patch > > > > > > drivers/usb/dwc3/dwc3-qcom.c | 5 ++++- > > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c > > > index be2e3dd36440..e9364141661b 100644 > > > --- a/drivers/usb/dwc3/dwc3-qcom.c > > > +++ b/drivers/usb/dwc3/dwc3-qcom.c > > > @@ -310,8 +310,11 @@ static enum usb_device_speed dwc3_qcom_read_usb2_speed(struct dwc3_qcom *qcom) > > > * currently supports only 1 port per controller. So > > > * this is sufficient. > > > */ > > > +#ifdef CONFIG_USB > > > udev = usb_hub_find_child(hcd->self.root_hub, 1); > > > > If a gadget driver needs this for some reason, then the #ifdef should be > > put in a .h file, not in a .c file. > > Yeah, if we're keeping this long-term then yes, and possibly also > otherwise. > > > But step back a minute and ask why a host-config-only function is being > > called when a device is in gadget-only mode? This feels like a > > design/logic issue in this file, NOT something to paper over with a > > #ifdef in a .c file > > We're not as I'm fixing that bug in later in the series. I should > probably have put this one after that fix, but figured fixing the build > was more important than a harder-to-hit NULL-deref due to non-host mode > not being considered when the offending series was merged. > > > This implies that if this device is NOT in a host configuration, then > > the suspend path of it is not configured properly at all, as why would > > it be checking or caring about this at all if this is in gadget-only > > mode? > > Right, so see path 6/9 which addresses this by only calling this hack > when in host mode: > > https://lore.kernel.org/all/20220804151001.23612-7-johan+linaro@kernel.org/ > > > Something else is wrong here, let's fix the root problem please. Maybe > > this driver should just never be built in gadget-only mode, as it is > > never intended to support that option? > > The problem is commit 6895ea55c385 ("usb: dwc3: qcom: Configure wakeup > interrupts during suspend"), which I considered simply reverting but as > that breaks suspend completely on some boards I decided to try and fix > it up while we work on a proper long-term solution (i.e. for how the > dwc/xhci layers should be communicating to implement this). > > Remember that it took two years and 21 revisions to get to the state > we're at now after you merged the wakeup series in June. Very good point. This is a mess, thanks for cleaning it up. I've applied this series now and will get it into 6.0-final, hopefully all should be good now. thanks for doing this work. greg k-h
diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c index be2e3dd36440..e9364141661b 100644 --- a/drivers/usb/dwc3/dwc3-qcom.c +++ b/drivers/usb/dwc3/dwc3-qcom.c @@ -310,8 +310,11 @@ static enum usb_device_speed dwc3_qcom_read_usb2_speed(struct dwc3_qcom *qcom) * currently supports only 1 port per controller. So * this is sufficient. */ +#ifdef CONFIG_USB udev = usb_hub_find_child(hcd->self.root_hub, 1); - +#else + udev = NULL; +#endif if (!udev) return USB_SPEED_UNKNOWN;
A recent change added a dependency to the USB host stack and broke gadget-only builds of the driver. Fixes: 6895ea55c385 ("usb: dwc3: qcom: Configure wakeup interrupts during suspend") Reported-by: Randy Dunlap <rdunlap@infradead.org> Signed-off-by: Johan Hovold <johan+linaro@kernel.org> --- Changes in v2 - new patch drivers/usb/dwc3/dwc3-qcom.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)