On Wed, Oct 25, 2023 at 07:16:05PM +0300, Sergey Shtylyov wrote: > On 10/25/23 7:00 PM, Sergey Shtylyov wrote: > > [...] > > Sorry to be PITA but... (-: > > I just had to speak up after Alan's ACK. :-) No problem, Thanks for the feedback. I agreed with many of them > > >> This patch introduces a new USB quirk, USB_QUIRK_SHORT_DEVICE_ADDR_TIMEOUT, > >> which modifies the timeout value for the 'set_address' command. The > > > > This is called a request, not a command by the spec. And the USB spec > > names the requests in all uppercase, e.g. SET_ADDRESS... > > > >> standard timeout for this command is 5000 ms, as recommended in the USB > >> 3.2 specification (section 9.2.6.1). > > > > This section in the USB specs 1.1/2.0/3.0 talks about _all_ requests. > > I don't have USB 3.2 but It believe it has the same wording. > > The patch modifies both xhci and hub.c, and the keywords 'command' come from the xhci driver. I will change 'command' to '(all)request' and newly added 'address_device' to 'SET_ADDRESS' in hub.c. That sounds better for hub.c > > [...] > > > >> Signed-off-by: Hardik Gajjar <hgajjar@xxxxxxxxxxxxxx> > [...] > > >> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c > >> index e1b1b64a0723..0c610a853aef 100644 > >> --- a/drivers/usb/host/xhci.c > >> +++ b/drivers/usb/host/xhci.c > >> @@ -3998,11 +3998,17 @@ int xhci_alloc_dev(struct usb_hcd *hcd, struct usb_device *udev) > >> } > >> > >> /* > > > > You seem to be converting the existing comment to a kernel-doc one > > but you miss changing from /* /** at the start and adding colons after > > From /* to /**, I meant to type... > > > the param names below... > > This comment update also looks like a meterial for a separate patch... > I think this is acceptable in the patch since it modifies the function arguments as well. I improved the existing comments while adding information about the new argument. However will update /* to /** > >> - * Issue an Address Device command and optionally send a corresponding > >> - * SetAddress request to the device. > >> + * xhci_setup_device - issues an Address Device command to assign a unique > >> + * USB bus address. > >> + * @hcd USB host controller data structure. > >> + * @udev USB dev structure representing the connected device. > >> + * @setup Enum specifying setup mode: address only or with context. > >> + * @timeout_ms Max wait time (ms) for the command operation to complete. > >> + * > >> + * Return: 0 if successful; otherwise, negative error code. > >> */ > >> static int xhci_setup_device(struct usb_hcd *hcd, struct usb_device *udev, > >> - enum xhci_setup_dev setup) > >> + enum xhci_setup_dev setup, unsigned int timeout_ms) > > [...] > > MBR, Sergey Thanks, Hardik
