On Wed, Aug 23, 2023, Alan Stern wrote: > On Wed, Aug 23, 2023 at 04:30:23AM +0200, Andrey Konovalov wrote: > > I started looking into reworking the UDC drivers to drop the special > > case for USB_GADGET_DELAYED_STATUS, but this seems more complicated. > > > > First, I noticed that some of the UDC drivers only expect to handle a > > delayed Status stage for SET_CONFIGURATION requests. (Which is Just want to clarify that dwc3 would expect a delayed status for any request returned with USB_GADGET_DELAYED_STATUS. The issue is that dwc3 assumes the _first_ delayed request would be SET_CONFIGURATION. Any subsequence control request with delayed request, it assumes the device is already in configured state. > > That expectation is wrong; gadget drivers can also want to delay the > Status stage for a SET_INTERFACE request. And in theory they might want > to delay any control-OUT transfer. Agree. Thanks Andrey and Alan for looking into dwc3. Regarding SET_INTERFACE, it should be fine because it should be done while it's already in configured state, which is after SET_CONFIGURATION. But it's true that dwc3 needs to fix this assumption here. > > > reasonable, as they were developed assuming that only the composite > > framework might request to delay the Status stage.) In particular, > > dwc3, cdns2, and cdns3 set the gadget state to USB_STATE_CONFIGURED > > when handling a delayed Status stage: > > > > dwc3/ep0.c:136: usb_gadget_set_state(dwc->gadget, USB_STATE_CONFIGURED); > > cdns3/cdns3-ep0.c:739: usb_gadget_set_state(&priv_dev->gadget, > > USB_STATE_CONFIGURED); > > gadget/udc/cdns2/cdns2-ep0.c:572: usb_gadget_set_state(&pdev->gadget, > > USB_STATE_CONFIGURED); > > This is also wrong. SET_CONFIGURATION can tell a gadget to install > config 0, in which case the state should be changed to > USB_STATE_ADDRESS. Added on my TODO list. > > For that matter, a gadget can undergo many state changes other than the > change into the CONFIGURED state. It doesn't look like many of the UDC > drivers are careful about reporting them. > > > So I believe an additional check for whether the request was indeed > > SET_CONFIGURATION is required. (cdns2 and cdns3 also do other things > > besides setting the state to USB_STATE_CONFIGURED, but it should be > > possible to hide that under the same check.) > > > > I also looked into how other UDC drivers change the gadget state to > > USB_STATE_CONFIGURED: > > > > 1. isp1760, mtu3, and bdc immediately set USB_STATE_CONFIGURED once > > they receive a SET_CONFIGURATION request, before calling ->setup() for > > the gadget driver; > > 2. gr and mv_u3d do that after the ->setup() call; > > 3. tegra does it after the first non-control endpoint is enabled; > > 4. dwc3, cdns2, and cdns3 appear to not set USB_STATE_CONFIGURED if > > the Status stage is not delayed; > > 5. dwc2, cdnsp, and all other UDCs don't set USB_STATE_CONFIGURED at all. > > > > I'm guessing the UDCs in #4 and #5 expect the gadget driver to set > > USB_STATE_CONFIGURED. > > > > I see that the composite framework sets the gadget state to > > USB_STATE_CONFIGURED even if some of the functions request a delayed > > Status stage via USB_GADGET_DELAYED_STATUS. And GadgetFS also sets the > > state to USB_STATE_CONFIGURED before delegating the SET_CONFIGURATION > > request to userspace. However, Raw Gadget expects the userspace to > > issue an ioctl that sets USB_STATE_CONFIGURED before completing the > > delayed SET_CONFIGURATION request. > > > > So I am wondering: when is proper time to set USB_STATE_CONFIGURED? > > And should this be handled by the UDC driver or the gadget driver? > > The proper time isn't really well defined. As far as the gadget driver > is concerned, it's when the configuration change is completed (when it > tells the composite framework to stop delaying the status stage). But > as far as the host is concerned, it's when the Status stage completes > successfully. > > If the Status stage of the control transfer gets corrupted, it's > possible to end up in a situation where the gadget believes it is > configured and the host believes it isn't. Luckily this doesn't > happen very often, and if it does then the host should reissue the > transfer. > > All the other state changes are (or should be) handled by the UDC > drivers. I guess they can handle the changes to/from the CONFIGURED > state as well, although they will have to be more careful about it than > they are now. > The dwc3 tries to handle these state changes. However, as pointed out, it needs to be audited. We got a little over reliant on USB CV tests to audit these state changes for us. Thanks, Thinh
