| Message ID | 20231011085011.89198-1-hgajjar@de.adit-jv.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v3] usb: core: hub: Add quirks for reducing device address timeout | expand |
On Wed, Oct 11, 2023 at 10:50:11AM +0200, Hardik Gajjar wrote: > Currently, the timeout for the set address command is fixed at > 5 seconds in the xhci driver. This means the host waits up to 5 > seconds to receive a response for the set_address command from > the device. > > In the automotive context, most smartphone enumerations, including > screen projection, should ideally complete within 3 seconds. > Achieving this is impossible in scenarios where the set_address is > not successful and waits for a timeout. > > The shortened address device timeout quirks provide the flexibility > to align with a 3-second time limit in the event of errors. > By swiftly triggering a failure response and swiftly initiating > retry procedures, these quirks ensure efficient and rapid recovery, > particularly in automotive contexts where rapid smartphone enumeration > and screen projection are vital. > > The quirk will set the timeout to 500 ms from 5 seconds. > > To use the quirk, please write "vendor_id:product_id:p" to > /sys/bus/usb/drivers/hub/module/parameter/quirks > > For example, > echo "0x2c48:0x0132:p" > /sys/bus/usb/drivers/hub/module/parameter/quirks" > > Signed-off-by: Hardik Gajjar <hgajjar@de.adit-jv.com> > --- > changes since version 1: > - implement quirk instead of new API in xhci driver > > changes since version 2: > - Add documentation for the new quirk. > - Define the timeout unit in milliseconds in variable names and function arguments. > - Change the xHCI command timeout from HZ (jiffies) to milliseconds. > - Add APTIV usb hub vendor and product ID in device quirk list > - Adding some other comments for clarity > --- > .../admin-guide/kernel-parameters.txt | 3 +++ > drivers/usb/core/hub.c | 13 ++++++++-- > drivers/usb/core/quirks.c | 6 +++++ > drivers/usb/host/xhci-mem.c | 2 ++ > drivers/usb/host/xhci-ring.c | 11 ++++---- > drivers/usb/host/xhci.c | 25 +++++++++++++------ > drivers/usb/host/xhci.h | 6 +++-- > include/linux/usb/hcd.h | 5 ++-- > include/linux/usb/quirks.h | 3 +++ > 9 files changed, 56 insertions(+), 18 deletions(-) > > diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt > index 0a1731a0f0ef..44732d179bce 100644 > --- a/Documentation/admin-guide/kernel-parameters.txt > +++ b/Documentation/admin-guide/kernel-parameters.txt > @@ -6817,6 +6817,9 @@ > pause after every control message); > o = USB_QUIRK_HUB_SLOW_RESET (Hub needs extra > delay after resetting its port); > + p = USB_QUIRK_SHORT_DEVICE_ADDR_TIMEOUT ( Timeout > + of set_address command reduce from 5000 ms > + to 500 ms No trailing ")" character? And no need for the extra space after the new "(" one, right? also, this should say it is "reducing", not "reduce"? > Example: quirks=0781:5580:bk,0a5c:5834:gij > > usbhid.mousepoll= > diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c > index 3c54b218301c..c0d727398cd1 100644 > --- a/drivers/usb/core/hub.c > +++ b/drivers/usb/core/hub.c > @@ -54,6 +54,9 @@ > #define USB_TP_TRANSMISSION_DELAY_MAX 65535 /* ns */ > #define USB_PING_RESPONSE_TIME 400 /* ns */ > > +#define USB_DEFAULT_ADDR_DEVICE_TIMEOUT_MS 5000 /* 5000ms */ This comes from the USB specification, right? If so, can you add the USB spec location for it in the comment? > +#define USB_SHORT_ADDR_DEVICE_TIMEOUT_MS 500 /* 500ms */ This is for "broken" devices, right? > + > /* Protect struct usb_device->state and ->children members > * Note: Both are also protected by ->dev.sem, except that ->state can > * change to USB_STATE_NOTATTACHED even when the semaphore isn't held. */ > @@ -4626,8 +4629,14 @@ EXPORT_SYMBOL_GPL(usb_ep0_reinit); > static int hub_set_address(struct usb_device *udev, int devnum) > { > int retval; > + unsigned int timeout_ms = USB_DEFAULT_ADDR_DEVICE_TIMEOUT_MS; > struct usb_hcd *hcd = bus_to_hcd(udev->bus); > > + struct usb_hub *hub = usb_hub_to_struct_hub(udev->parent); Did you run checkpatch.pl on your change? It should say the extra blank line you added here isn't needed (if not, it shouldn't be added anyway, that's not good kernel coding style.) > + > + if (hub->hdev->quirks & USB_QUIRK_SHORT_DEVICE_ADDR_TIMEOUT) > + timeout_ms = USB_SHORT_ADDR_DEVICE_TIMEOUT_MS; > + > /* > * The host controller will choose the device address, > * instead of the core having chosen it earlier > @@ -4639,11 +4648,11 @@ static int hub_set_address(struct usb_device *udev, int devnum) > if (udev->state != USB_STATE_DEFAULT) > return -EINVAL; > if (hcd->driver->address_device) > - retval = hcd->driver->address_device(hcd, udev); > + retval = hcd->driver->address_device(hcd, udev, timeout_ms); > else > retval = usb_control_msg(udev, usb_sndaddr0pipe(), > USB_REQ_SET_ADDRESS, 0, devnum, 0, > - NULL, 0, USB_CTRL_SET_TIMEOUT); > + NULL, 0, timeout_ms); > if (retval == 0) { > update_devnum(udev, devnum); > /* Device now using proper address. */ > diff --git a/drivers/usb/core/quirks.c b/drivers/usb/core/quirks.c > index 15e9bd180a1d..a1137740b496 100644 > --- a/drivers/usb/core/quirks.c > +++ b/drivers/usb/core/quirks.c > @@ -138,6 +138,9 @@ static int quirks_param_set(const char *value, const struct kernel_param *kp) > case 'o': > flags |= USB_QUIRK_HUB_SLOW_RESET; > break; > + case 'p': > + flags |= USB_QUIRK_SHORT_DEVICE_ADDR_TIMEOUT; > + break; > /* Ignore unrecognized flag characters */ > } > } > @@ -542,6 +545,9 @@ static const struct usb_device_id usb_quirk_list[] = { > /* INTEL VALUE SSD */ > { USB_DEVICE(0x8086, 0xf1a5), .driver_info = USB_QUIRK_RESET_RESUME }, > > + /* APTIV AUTOMOTIVE HUB */ > + { USB_DEVICE(0x2c48, 0x0132), .driver_info = USB_QUIRK_SHORT_DEVICE_ADDR_TIMEOUT }, > + > { } /* terminating entry must be last */ > }; > I miss where you add the timeout delay in the other host controller drivers. Why only xhci? What about uhci/ohci/dwc3/etc.? thanks, greg k-h
Hi Hardik, kernel test robot noticed the following build warnings: [auto build test WARNING on usb/usb-testing] [also build test WARNING on usb/usb-next usb/usb-linus linus/master v6.6-rc5 next-20231011] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Hardik-Gajjar/usb-core-hub-Add-quirks-for-reducing-device-address-timeout/20231011-165246 base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing patch link: https://lore.kernel.org/r/20231011085011.89198-1-hgajjar%40de.adit-jv.com patch subject: [PATCH v3] usb: core: hub: Add quirks for reducing device address timeout config: m68k-allyesconfig (https://download.01.org/0day-ci/archive/20231011/202310111851.fB7Flvj5-lkp@intel.com/config) compiler: m68k-linux-gcc (GCC) 13.2.0 reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231011/202310111851.fB7Flvj5-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202310111851.fB7Flvj5-lkp@intel.com/ All warnings (new ones prefixed by >>): drivers/usb/host/xhci.c:1318: warning: Function parameter or member 'desc' not described in 'xhci_get_endpoint_index' >> drivers/usb/host/xhci.c:4001: warning: This comment starts with '/**', but isn't a kernel-doc comment. Refer Documentation/doc-guide/kernel-doc.rst * This function issues an Address Device command to assign a unique USB bus vim +4001 drivers/usb/host/xhci.c 3999 4000 /** > 4001 * This function issues an Address Device command to assign a unique USB bus 4002 * address. Optionally, it sends a SetAddress request. 4003 * 4004 * @param hcd USB host controller data structure. 4005 * @param udev USB device structure representing the connected device. 4006 * @param setup Enum specifying setup mode: address only or with context. 4007 * @param timeout_ms Max wait time (ms) for the command operation to complete. 4008 * 4009 * @return Integer status code: 0 on success, negative on error. 4010 * 4011 */ 4012 static int xhci_setup_device(struct usb_hcd *hcd, struct usb_device *udev, 4013 enum xhci_setup_dev setup, unsigned int timeout_ms) 4014 { 4015 const char *act = setup == SETUP_CONTEXT_ONLY ? "context" : "address"; 4016 unsigned long flags; 4017 struct xhci_virt_device *virt_dev; 4018 int ret = 0; 4019 struct xhci_hcd *xhci = hcd_to_xhci(hcd); 4020 struct xhci_slot_ctx *slot_ctx; 4021 struct xhci_input_control_ctx *ctrl_ctx; 4022 u64 temp_64; 4023 struct xhci_command *command = NULL; 4024 4025 mutex_lock(&xhci->mutex); 4026 4027 if (xhci->xhc_state) { /* dying, removing or halted */ 4028 ret = -ESHUTDOWN; 4029 goto out; 4030 } 4031 4032 if (!udev->slot_id) { 4033 xhci_dbg_trace(xhci, trace_xhci_dbg_address, 4034 "Bad Slot ID %d", udev->slot_id); 4035 ret = -EINVAL; 4036 goto out; 4037 } 4038 4039 virt_dev = xhci->devs[udev->slot_id]; 4040 4041 if (WARN_ON(!virt_dev)) { 4042 /* 4043 * In plug/unplug torture test with an NEC controller, 4044 * a zero-dereference was observed once due to virt_dev = 0. 4045 * Print useful debug rather than crash if it is observed again! 4046 */ 4047 xhci_warn(xhci, "Virt dev invalid for slot_id 0x%x!\n", 4048 udev->slot_id); 4049 ret = -EINVAL; 4050 goto out; 4051 } 4052 slot_ctx = xhci_get_slot_ctx(xhci, virt_dev->out_ctx); 4053 trace_xhci_setup_device_slot(slot_ctx); 4054 4055 if (setup == SETUP_CONTEXT_ONLY) { 4056 if (GET_SLOT_STATE(le32_to_cpu(slot_ctx->dev_state)) == 4057 SLOT_STATE_DEFAULT) { 4058 xhci_dbg(xhci, "Slot already in default state\n"); 4059 goto out; 4060 } 4061 } 4062 4063 command = xhci_alloc_command(xhci, true, GFP_KERNEL); 4064 if (!command) { 4065 ret = -ENOMEM; 4066 goto out; 4067 } 4068 4069 command->in_ctx = virt_dev->in_ctx; 4070 command->timeout_ms = timeout_ms; 4071 4072 slot_ctx = xhci_get_slot_ctx(xhci, virt_dev->in_ctx); 4073 ctrl_ctx = xhci_get_input_control_ctx(virt_dev->in_ctx); 4074 if (!ctrl_ctx) { 4075 xhci_warn(xhci, "%s: Could not get input context, bad type.\n", 4076 __func__); 4077 ret = -EINVAL; 4078 goto out; 4079 } 4080 /* 4081 * If this is the first Set Address since device plug-in or 4082 * virt_device realloaction after a resume with an xHCI power loss, 4083 * then set up the slot context. 4084 */ 4085 if (!slot_ctx->dev_info) 4086 xhci_setup_addressable_virt_dev(xhci, udev); 4087 /* Otherwise, update the control endpoint ring enqueue pointer. */ 4088 else 4089 xhci_copy_ep0_dequeue_into_input_ctx(xhci, udev); 4090 ctrl_ctx->add_flags = cpu_to_le32(SLOT_FLAG | EP0_FLAG); 4091 ctrl_ctx->drop_flags = 0; 4092 4093 trace_xhci_address_ctx(xhci, virt_dev->in_ctx, 4094 le32_to_cpu(slot_ctx->dev_info) >> 27); 4095 4096 trace_xhci_address_ctrl_ctx(ctrl_ctx); 4097 spin_lock_irqsave(&xhci->lock, flags); 4098 trace_xhci_setup_device(virt_dev); 4099 ret = xhci_queue_address_device(xhci, command, virt_dev->in_ctx->dma, 4100 udev->slot_id, setup); 4101 if (ret) { 4102 spin_unlock_irqrestore(&xhci->lock, flags); 4103 xhci_dbg_trace(xhci, trace_xhci_dbg_address, 4104 "FIXME: allocate a command ring segment"); 4105 goto out; 4106 } 4107 xhci_ring_cmd_db(xhci); 4108 spin_unlock_irqrestore(&xhci->lock, flags); 4109 4110 /* ctrl tx can take up to 5 sec; XXX: need more time for xHC? */ 4111 wait_for_completion(command->completion); 4112 4113 /* FIXME: From section 4.3.4: "Software shall be responsible for timing 4114 * the SetAddress() "recovery interval" required by USB and aborting the 4115 * command on a timeout. 4116 */ 4117 switch (command->status) { 4118 case COMP_COMMAND_ABORTED: 4119 case COMP_COMMAND_RING_STOPPED: 4120 xhci_warn(xhci, "Timeout while waiting for setup device command\n"); 4121 ret = -ETIME; 4122 break; 4123 case COMP_CONTEXT_STATE_ERROR: 4124 case COMP_SLOT_NOT_ENABLED_ERROR: 4125 xhci_err(xhci, "Setup ERROR: setup %s command for slot %d.\n", 4126 act, udev->slot_id); 4127 ret = -EINVAL; 4128 break; 4129 case COMP_USB_TRANSACTION_ERROR: 4130 dev_warn(&udev->dev, "Device not responding to setup %s.\n", act); 4131 4132 mutex_unlock(&xhci->mutex); 4133 ret = xhci_disable_slot(xhci, udev->slot_id); 4134 xhci_free_virt_device(xhci, udev->slot_id); 4135 if (!ret) 4136 xhci_alloc_dev(hcd, udev); 4137 kfree(command->completion); 4138 kfree(command); 4139 return -EPROTO; 4140 case COMP_INCOMPATIBLE_DEVICE_ERROR: 4141 dev_warn(&udev->dev, 4142 "ERROR: Incompatible device for setup %s command\n", act); 4143 ret = -ENODEV; 4144 break; 4145 case COMP_SUCCESS: 4146 xhci_dbg_trace(xhci, trace_xhci_dbg_address, 4147 "Successful setup %s command", act); 4148 break; 4149 default: 4150 xhci_err(xhci, 4151 "ERROR: unexpected setup %s command completion code 0x%x.\n", 4152 act, command->status); 4153 trace_xhci_address_ctx(xhci, virt_dev->out_ctx, 1); 4154 ret = -EINVAL; 4155 break; 4156 } 4157 if (ret) 4158 goto out; 4159 temp_64 = xhci_read_64(xhci, &xhci->op_regs->dcbaa_ptr); 4160 xhci_dbg_trace(xhci, trace_xhci_dbg_address, 4161 "Op regs DCBAA ptr = %#016llx", temp_64); 4162 xhci_dbg_trace(xhci, trace_xhci_dbg_address, 4163 "Slot ID %d dcbaa entry @%p = %#016llx", 4164 udev->slot_id, 4165 &xhci->dcbaa->dev_context_ptrs[udev->slot_id], 4166 (unsigned long long) 4167 le64_to_cpu(xhci->dcbaa->dev_context_ptrs[udev->slot_id])); 4168 xhci_dbg_trace(xhci, trace_xhci_dbg_address, 4169 "Output Context DMA address = %#08llx", 4170 (unsigned long long)virt_dev->out_ctx->dma); 4171 trace_xhci_address_ctx(xhci, virt_dev->in_ctx, 4172 le32_to_cpu(slot_ctx->dev_info) >> 27); 4173 /* 4174 * USB core uses address 1 for the roothubs, so we add one to the 4175 * address given back to us by the HC. 4176 */ 4177 trace_xhci_address_ctx(xhci, virt_dev->out_ctx, 4178 le32_to_cpu(slot_ctx->dev_info) >> 27); 4179 /* Zero the input context control for later use */ 4180 ctrl_ctx->add_flags = 0; 4181 ctrl_ctx->drop_flags = 0; 4182 slot_ctx = xhci_get_slot_ctx(xhci, virt_dev->out_ctx); 4183 udev->devaddr = (u8)(le32_to_cpu(slot_ctx->dev_state) & DEV_ADDR_MASK); 4184 4185 xhci_dbg_trace(xhci, trace_xhci_dbg_address, 4186 "Internal device address = %d", 4187 le32_to_cpu(slot_ctx->dev_state) & DEV_ADDR_MASK); 4188 out: 4189 mutex_unlock(&xhci->mutex); 4190 if (command) { 4191 kfree(command->completion); 4192 kfree(command); 4193 } 4194 return ret; 4195 } 4196
On Wed, Oct 11, 2023 at 11:02:27AM +0200, Greg KH wrote: > On Wed, Oct 11, 2023 at 10:50:11AM +0200, Hardik Gajjar wrote: > > Currently, the timeout for the set address command is fixed at > > 5 seconds in the xhci driver. This means the host waits up to 5 > > seconds to receive a response for the set_address command from > > the device. > > > > In the automotive context, most smartphone enumerations, including > > screen projection, should ideally complete within 3 seconds. > > Achieving this is impossible in scenarios where the set_address is > > not successful and waits for a timeout. > > > > The shortened address device timeout quirks provide the flexibility > > to align with a 3-second time limit in the event of errors. > > By swiftly triggering a failure response and swiftly initiating > > retry procedures, these quirks ensure efficient and rapid recovery, > > particularly in automotive contexts where rapid smartphone enumeration > > and screen projection are vital. > > > > The quirk will set the timeout to 500 ms from 5 seconds. > > > > To use the quirk, please write "vendor_id:product_id:p" to > > /sys/bus/usb/drivers/hub/module/parameter/quirks > > > > For example, > > echo "0x2c48:0x0132:p" > /sys/bus/usb/drivers/hub/module/parameter/quirks" > > > > Signed-off-by: Hardik Gajjar <hgajjar@de.adit-jv.com> > > --- > > changes since version 1: > > - implement quirk instead of new API in xhci driver > > > > changes since version 2: > > - Add documentation for the new quirk. > > - Define the timeout unit in milliseconds in variable names and function arguments. > > - Change the xHCI command timeout from HZ (jiffies) to milliseconds. > > - Add APTIV usb hub vendor and product ID in device quirk list > > - Adding some other comments for clarity > > --- > > .../admin-guide/kernel-parameters.txt | 3 +++ > > drivers/usb/core/hub.c | 13 ++++++++-- > > drivers/usb/core/quirks.c | 6 +++++ > > drivers/usb/host/xhci-mem.c | 2 ++ > > drivers/usb/host/xhci-ring.c | 11 ++++---- > > drivers/usb/host/xhci.c | 25 +++++++++++++------ > > drivers/usb/host/xhci.h | 6 +++-- > > include/linux/usb/hcd.h | 5 ++-- > > include/linux/usb/quirks.h | 3 +++ > > 9 files changed, 56 insertions(+), 18 deletions(-) > > > > diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt > > index 0a1731a0f0ef..44732d179bce 100644 > > --- a/Documentation/admin-guide/kernel-parameters.txt > > +++ b/Documentation/admin-guide/kernel-parameters.txt > > @@ -6817,6 +6817,9 @@ > > pause after every control message); > > o = USB_QUIRK_HUB_SLOW_RESET (Hub needs extra > > delay after resetting its port); > > + p = USB_QUIRK_SHORT_DEVICE_ADDR_TIMEOUT ( Timeout > > + of set_address command reduce from 5000 ms > > + to 500 ms > > No trailing ")" character? And no need for the extra space after the > new "(" one, right? > Okay, update it. Interestingly, the 'scripts/checkpatch.pl' is not reporting such warnings > also, this should say it is "reducing", not "reduce"? > > > Example: quirks=0781:5580:bk,0a5c:5834:gij > > > > usbhid.mousepoll= > > diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c > > index 3c54b218301c..c0d727398cd1 100644 > > --- a/drivers/usb/core/hub.c > > +++ b/drivers/usb/core/hub.c > > @@ -54,6 +54,9 @@ > > #define USB_TP_TRANSMISSION_DELAY_MAX 65535 /* ns */ > > #define USB_PING_RESPONSE_TIME 400 /* ns */ > > > > +#define USB_DEFAULT_ADDR_DEVICE_TIMEOUT_MS 5000 /* 5000ms */ > > This comes from the USB specification, right? If so, can you add the > USB spec location for it in the comment? > > > +#define USB_SHORT_ADDR_DEVICE_TIMEOUT_MS 500 /* 500ms */ > > This is for "broken" devices, right? > > > + > > /* Protect struct usb_device->state and ->children members > > * Note: Both are also protected by ->dev.sem, except that ->state can > > * change to USB_STATE_NOTATTACHED even when the semaphore isn't held. */ > > @@ -4626,8 +4629,14 @@ EXPORT_SYMBOL_GPL(usb_ep0_reinit); > > static int hub_set_address(struct usb_device *udev, int devnum) > > { > > int retval; > > + unsigned int timeout_ms = USB_DEFAULT_ADDR_DEVICE_TIMEOUT_MS; > > struct usb_hcd *hcd = bus_to_hcd(udev->bus); > > > > + struct usb_hub *hub = usb_hub_to_struct_hub(udev->parent); > > Did you run https://urldefense.proofpoint.com/v2/url?u=http-3A__checkpatch.pl&d=DwICAg&c=euGZstcaTDllvimEN8b7jXrwqOf-v5A_CdpgnVfiiMM&r=SAhjP5GOmrADp1v_EE5jWoSuMlYCIt9gKduw-DCBPLs&m=6Am_63OPDGq3Jc3NvsSZFUkSZLQk4gIIiJUx4nh1LoSZtxv-gUlOsoQY-Ooe2uuX&s=gQ_CCUZ1pM8CXK5oamzWh365rtZOP3DiS3ne4Rcj__A&e= on your change? It should say the extra blank > line you added here isn't needed (if not, it shouldn't be added anyway, > that's not good kernel coding style.) Yes, I have executed 'scripts/checkpatch.pl' with the --strict option, but there is no such warning. Am I missing something? The following is the output of checkpatch.pl on my build machine: ------------------------------------------------------ scripts/checkpatch.pl --strict v3-0001-usb-core-hub-Add-quirks-for-reducing-device-addre.patch WARNING: function definition argument 'struct usb_hcd *' should also have an identifier name #285: FILE: include/linux/usb/hcd.h:376: + int (*address_device)(struct usb_hcd *, struct usb_device *udev, total: 0 errors, 1 warnings, 0 checks, 193 lines checked NOTE: For some of the reported defects, checkpatch may be able to mechanically convert to the typical style using --fix or --fix-inplace. v3-0001-usb-core-hub-Add-quirks-for-reducing-device-addre.patch has style problems, please review. NOTE: If any of the errors are false positives, please report them to the maintainer, see CHECKPATCH in MAINTAINERS. ------------------------------------------------------- > > > + > > + if (hub->hdev->quirks & USB_QUIRK_SHORT_DEVICE_ADDR_TIMEOUT) > > + timeout_ms = USB_SHORT_ADDR_DEVICE_TIMEOUT_MS; > > + > > /* > > * The host controller will choose the device address, > > * instead of the core having chosen it earlier > > @@ -4639,11 +4648,11 @@ static int hub_set_address(struct usb_device *udev, int devnum) > > if (udev->state != USB_STATE_DEFAULT) > > return -EINVAL; > > if (hcd->driver->address_device) > > - retval = hcd->driver->address_device(hcd, udev); > > + retval = hcd->driver->address_device(hcd, udev, timeout_ms); > > else > > retval = usb_control_msg(udev, usb_sndaddr0pipe(), > > USB_REQ_SET_ADDRESS, 0, devnum, 0, > > - NULL, 0, USB_CTRL_SET_TIMEOUT); > > + NULL, 0, timeout_ms); > > if (retval == 0) { > > update_devnum(udev, devnum); > > /* Device now using proper address. */ > > diff --git a/drivers/usb/core/quirks.c b/drivers/usb/core/quirks.c > > index 15e9bd180a1d..a1137740b496 100644 > > --- a/drivers/usb/core/quirks.c > > +++ b/drivers/usb/core/quirks.c > > @@ -138,6 +138,9 @@ static int quirks_param_set(const char *value, const struct kernel_param *kp) > > case 'o': > > flags |= USB_QUIRK_HUB_SLOW_RESET; > > break; > > + case 'p': > > + flags |= USB_QUIRK_SHORT_DEVICE_ADDR_TIMEOUT; > > + break; > > /* Ignore unrecognized flag characters */ > > } > > } > > @@ -542,6 +545,9 @@ static const struct usb_device_id usb_quirk_list[] = { > > /* INTEL VALUE SSD */ > > { USB_DEVICE(0x8086, 0xf1a5), .driver_info = USB_QUIRK_RESET_RESUME }, > > > > + /* APTIV AUTOMOTIVE HUB */ > > + { USB_DEVICE(0x2c48, 0x0132), .driver_info = USB_QUIRK_SHORT_DEVICE_ADDR_TIMEOUT }, > > + > > { } /* terminating entry must be last */ > > }; > > > > I miss where you add the timeout delay in the other host controller > drivers. Why only xhci? What about uhci/ohci/dwc3/etc.? > It is in hub.c file. Following is the code snippet of the patch. if (hcd->driver->address_device) - retval = hcd->driver->address_device(hcd, udev); + retval = hcd->driver->address_device(hcd, udev, timeout_ms); else retval = usb_control_msg(udev, usb_sndaddr0pipe(), USB_REQ_SET_ADDRESS, 0, devnum, 0, - NULL, 0, USB_CTRL_SET_TIMEOUT); + NULL, 0, timeout_ms); In other host drivers, the set address request is issued using the usb_control_msg() API within the 'else' condition. Other host drivers, except for xHCI, do not populate the 'address_device' function in the hc_driver structure when defining the structure in the host driver. For example, you can see the hc_driver structure of the Linux OHCI driver at the following link, and you'll notice that there is no 'address_device' function. https://github.com/torvalds/linux/blob/1c8b86a3799f7e5be903c3f49fcdaee29fd385b5/drivers/usb/host/ohci-hcd.c#L1183 > thanks, > > greg k-h Thanks, Hardik
On Wed, Oct 11, 2023 at 10:50:11AM +0200, Hardik Gajjar wrote: > --- a/drivers/usb/core/hub.c > +++ b/drivers/usb/core/hub.c > @@ -54,6 +54,9 @@ > #define USB_TP_TRANSMISSION_DELAY_MAX 65535 /* ns */ > #define USB_PING_RESPONSE_TIME 400 /* ns */ > > +#define USB_DEFAULT_ADDR_DEVICE_TIMEOUT_MS 5000 /* 5000ms */ > +#define USB_SHORT_ADDR_DEVICE_TIMEOUT_MS 500 /* 500ms */ You don't have to repeat the numbers in the comments. You can just write /* ms */ -- just like in the comments immediately above. > return -EINVAL; > if (hcd->driver->address_device) > - retval = hcd->driver->address_device(hcd, udev); > + retval = hcd->driver->address_device(hcd, udev, timeout_ms); > else > retval = usb_control_msg(udev, usb_sndaddr0pipe(), > --- a/drivers/usb/core/quirks.c > +++ b/drivers/usb/core/quirks.c > @@ -138,6 +138,9 @@ static int quirks_param_set(const char *value, const struct kernel_param *kp) > case 'o': > flags |= USB_QUIRK_HUB_SLOW_RESET; > break; > + case 'p': > + flags |= USB_QUIRK_SHORT_DEVICE_ADDR_TIMEOUT; > + break; > /* Ignore unrecognized flag characters */ > } > } > @@ -542,6 +545,9 @@ static const struct usb_device_id usb_quirk_list[] = { > /* INTEL VALUE SSD */ > { USB_DEVICE(0x8086, 0xf1a5), .driver_info = USB_QUIRK_RESET_RESUME }, > > + /* APTIV AUTOMOTIVE HUB */ > + { USB_DEVICE(0x2c48, 0x0132), .driver_info = USB_QUIRK_SHORT_DEVICE_ADDR_TIMEOUT }, This table is sorted by Vendor ID, then Product ID, then Class ID, as stated in the comment at the beginning of the definition. Your new entry is in the wrong position. Alan Stern
On Wed, Oct 11, 2023 at 02:05:35PM +0200, Hardik Gajjar wrote: > On Wed, Oct 11, 2023 at 11:02:27AM +0200, Greg KH wrote: > > On Wed, Oct 11, 2023 at 10:50:11AM +0200, Hardik Gajjar wrote: > > > Currently, the timeout for the set address command is fixed at > > > 5 seconds in the xhci driver. This means the host waits up to 5 > > > seconds to receive a response for the set_address command from > > > the device. > > > > > > In the automotive context, most smartphone enumerations, including > > > screen projection, should ideally complete within 3 seconds. > > > Achieving this is impossible in scenarios where the set_address is > > > not successful and waits for a timeout. > > > > > > The shortened address device timeout quirks provide the flexibility > > > to align with a 3-second time limit in the event of errors. > > > By swiftly triggering a failure response and swiftly initiating > > > retry procedures, these quirks ensure efficient and rapid recovery, > > > particularly in automotive contexts where rapid smartphone enumeration > > > and screen projection are vital. > > > > > > The quirk will set the timeout to 500 ms from 5 seconds. > > > > > > To use the quirk, please write "vendor_id:product_id:p" to > > > /sys/bus/usb/drivers/hub/module/parameter/quirks > > > > > > For example, > > > echo "0x2c48:0x0132:p" > /sys/bus/usb/drivers/hub/module/parameter/quirks" > > > > > > Signed-off-by: Hardik Gajjar <hgajjar@de.adit-jv.com> > > > --- > > > changes since version 1: > > > - implement quirk instead of new API in xhci driver > > > > > > changes since version 2: > > > - Add documentation for the new quirk. > > > - Define the timeout unit in milliseconds in variable names and function arguments. > > > - Change the xHCI command timeout from HZ (jiffies) to milliseconds. > > > - Add APTIV usb hub vendor and product ID in device quirk list > > > - Adding some other comments for clarity > > > --- > > > .../admin-guide/kernel-parameters.txt | 3 +++ > > > drivers/usb/core/hub.c | 13 ++++++++-- > > > drivers/usb/core/quirks.c | 6 +++++ > > > drivers/usb/host/xhci-mem.c | 2 ++ > > > drivers/usb/host/xhci-ring.c | 11 ++++---- > > > drivers/usb/host/xhci.c | 25 +++++++++++++------ > > > drivers/usb/host/xhci.h | 6 +++-- > > > include/linux/usb/hcd.h | 5 ++-- > > > include/linux/usb/quirks.h | 3 +++ > > > 9 files changed, 56 insertions(+), 18 deletions(-) > > > > > > diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt > > > index 0a1731a0f0ef..44732d179bce 100644 > > > --- a/Documentation/admin-guide/kernel-parameters.txt > > > +++ b/Documentation/admin-guide/kernel-parameters.txt > > > @@ -6817,6 +6817,9 @@ > > > pause after every control message); > > > o = USB_QUIRK_HUB_SLOW_RESET (Hub needs extra > > > delay after resetting its port); > > > + p = USB_QUIRK_SHORT_DEVICE_ADDR_TIMEOUT ( Timeout > > > + of set_address command reduce from 5000 ms > > > + to 500 ms > > > > No trailing ")" character? And no need for the extra space after the > > new "(" one, right? > > > > Okay, update it. Interestingly, the 'scripts/checkpatch.pl' is not reporting such warnings checkpatch doesn't parse documentation like this, so don't expect it to catch stuff here. thanks, greg k-h
diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt index 0a1731a0f0ef..44732d179bce 100644 --- a/Documentation/admin-guide/kernel-parameters.txt +++ b/Documentation/admin-guide/kernel-parameters.txt @@ -6817,6 +6817,9 @@ pause after every control message); o = USB_QUIRK_HUB_SLOW_RESET (Hub needs extra delay after resetting its port); + p = USB_QUIRK_SHORT_DEVICE_ADDR_TIMEOUT ( Timeout + of set_address command reduce from 5000 ms + to 500 ms Example: quirks=0781:5580:bk,0a5c:5834:gij usbhid.mousepoll= diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c index 3c54b218301c..c0d727398cd1 100644 --- a/drivers/usb/core/hub.c +++ b/drivers/usb/core/hub.c @@ -54,6 +54,9 @@ #define USB_TP_TRANSMISSION_DELAY_MAX 65535 /* ns */ #define USB_PING_RESPONSE_TIME 400 /* ns */ +#define USB_DEFAULT_ADDR_DEVICE_TIMEOUT_MS 5000 /* 5000ms */ +#define USB_SHORT_ADDR_DEVICE_TIMEOUT_MS 500 /* 500ms */ + /* Protect struct usb_device->state and ->children members * Note: Both are also protected by ->dev.sem, except that ->state can * change to USB_STATE_NOTATTACHED even when the semaphore isn't held. */ @@ -4626,8 +4629,14 @@ EXPORT_SYMBOL_GPL(usb_ep0_reinit); static int hub_set_address(struct usb_device *udev, int devnum) { int retval; + unsigned int timeout_ms = USB_DEFAULT_ADDR_DEVICE_TIMEOUT_MS; struct usb_hcd *hcd = bus_to_hcd(udev->bus); + struct usb_hub *hub = usb_hub_to_struct_hub(udev->parent); + + if (hub->hdev->quirks & USB_QUIRK_SHORT_DEVICE_ADDR_TIMEOUT) + timeout_ms = USB_SHORT_ADDR_DEVICE_TIMEOUT_MS; + /* * The host controller will choose the device address, * instead of the core having chosen it earlier @@ -4639,11 +4648,11 @@ static int hub_set_address(struct usb_device *udev, int devnum) if (udev->state != USB_STATE_DEFAULT) return -EINVAL; if (hcd->driver->address_device) - retval = hcd->driver->address_device(hcd, udev); + retval = hcd->driver->address_device(hcd, udev, timeout_ms); else retval = usb_control_msg(udev, usb_sndaddr0pipe(), USB_REQ_SET_ADDRESS, 0, devnum, 0, - NULL, 0, USB_CTRL_SET_TIMEOUT); + NULL, 0, timeout_ms); if (retval == 0) { update_devnum(udev, devnum); /* Device now using proper address. */ diff --git a/drivers/usb/core/quirks.c b/drivers/usb/core/quirks.c index 15e9bd180a1d..a1137740b496 100644 --- a/drivers/usb/core/quirks.c +++ b/drivers/usb/core/quirks.c @@ -138,6 +138,9 @@ static int quirks_param_set(const char *value, const struct kernel_param *kp) case 'o': flags |= USB_QUIRK_HUB_SLOW_RESET; break; + case 'p': + flags |= USB_QUIRK_SHORT_DEVICE_ADDR_TIMEOUT; + break; /* Ignore unrecognized flag characters */ } } @@ -542,6 +545,9 @@ static const struct usb_device_id usb_quirk_list[] = { /* INTEL VALUE SSD */ { USB_DEVICE(0x8086, 0xf1a5), .driver_info = USB_QUIRK_RESET_RESUME }, + /* APTIV AUTOMOTIVE HUB */ + { USB_DEVICE(0x2c48, 0x0132), .driver_info = USB_QUIRK_SHORT_DEVICE_ADDR_TIMEOUT }, + { } /* terminating entry must be last */ }; diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c index 8714ab5bf04d..4a286136d1a8 100644 --- a/drivers/usb/host/xhci-mem.c +++ b/drivers/usb/host/xhci-mem.c @@ -1729,6 +1729,8 @@ struct xhci_command *xhci_alloc_command(struct xhci_hcd *xhci, } command->status = 0; + /* set default timeout to 5000 ms */ + command->timeout_ms = XHCI_CMD_DEFAULT_TIMEOUT_MS; INIT_LIST_HEAD(&command->cmd_list); return command; } diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c index 1dde53f6eb31..8f36c2914938 100644 --- a/drivers/usb/host/xhci-ring.c +++ b/drivers/usb/host/xhci-ring.c @@ -366,9 +366,10 @@ void xhci_ring_cmd_db(struct xhci_hcd *xhci) readl(&xhci->dba->doorbell[0]); } -static bool xhci_mod_cmd_timer(struct xhci_hcd *xhci, unsigned long delay) +static bool xhci_mod_cmd_timer(struct xhci_hcd *xhci) { - return mod_delayed_work(system_wq, &xhci->cmd_timer, delay); + return mod_delayed_work(system_wq, &xhci->cmd_timer, + msecs_to_jiffies(xhci->current_cmd->timeout_ms)); } static struct xhci_command *xhci_next_queued_cmd(struct xhci_hcd *xhci) @@ -412,7 +413,7 @@ static void xhci_handle_stopped_cmd_ring(struct xhci_hcd *xhci, if ((xhci->cmd_ring->dequeue != xhci->cmd_ring->enqueue) && !(xhci->xhc_state & XHCI_STATE_DYING)) { xhci->current_cmd = cur_cmd; - xhci_mod_cmd_timer(xhci, XHCI_CMD_DEFAULT_TIMEOUT); + xhci_mod_cmd_timer(xhci); xhci_ring_cmd_db(xhci); } } @@ -1786,7 +1787,7 @@ static void handle_cmd_completion(struct xhci_hcd *xhci, if (!list_is_singular(&xhci->cmd_list)) { xhci->current_cmd = list_first_entry(&cmd->cmd_list, struct xhci_command, cmd_list); - xhci_mod_cmd_timer(xhci, XHCI_CMD_DEFAULT_TIMEOUT); + xhci_mod_cmd_timer(xhci); } else if (xhci->current_cmd == cmd) { xhci->current_cmd = NULL; } @@ -4301,7 +4302,7 @@ static int queue_command(struct xhci_hcd *xhci, struct xhci_command *cmd, /* if there are no other commands queued we start the timeout timer */ if (list_empty(&xhci->cmd_list)) { xhci->current_cmd = cmd; - xhci_mod_cmd_timer(xhci, XHCI_CMD_DEFAULT_TIMEOUT); + xhci_mod_cmd_timer(xhci); } list_add_tail(&cmd->cmd_list, &xhci->cmd_list); diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c index e1b1b64a0723..cd24f9222b1b 100644 --- a/drivers/usb/host/xhci.c +++ b/drivers/usb/host/xhci.c @@ -3997,12 +3997,20 @@ int xhci_alloc_dev(struct usb_hcd *hcd, struct usb_device *udev) return 0; } -/* - * Issue an Address Device command and optionally send a corresponding - * SetAddress request to the device. +/** + * This function issues an Address Device command to assign a unique USB bus + * address. Optionally, it sends a SetAddress request. + * + * @param hcd USB host controller data structure. + * @param udev USB device structure representing the connected device. + * @param setup Enum specifying setup mode: address only or with context. + * @param timeout_ms Max wait time (ms) for the command operation to complete. + * + * @return Integer status code: 0 on success, negative on error. + * */ 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) { const char *act = setup == SETUP_CONTEXT_ONLY ? "context" : "address"; unsigned long flags; @@ -4059,6 +4067,7 @@ static int xhci_setup_device(struct usb_hcd *hcd, struct usb_device *udev, } command->in_ctx = virt_dev->in_ctx; + command->timeout_ms = timeout_ms; slot_ctx = xhci_get_slot_ctx(xhci, virt_dev->in_ctx); ctrl_ctx = xhci_get_input_control_ctx(virt_dev->in_ctx); @@ -4185,14 +4194,16 @@ static int xhci_setup_device(struct usb_hcd *hcd, struct usb_device *udev, return ret; } -static int xhci_address_device(struct usb_hcd *hcd, struct usb_device *udev) +static int xhci_address_device(struct usb_hcd *hcd, struct usb_device *udev, + unsigned int timeout_ms) { - return xhci_setup_device(hcd, udev, SETUP_CONTEXT_ADDRESS); + return xhci_setup_device(hcd, udev, SETUP_CONTEXT_ADDRESS, timeout_ms); } static int xhci_enable_device(struct usb_hcd *hcd, struct usb_device *udev) { - return xhci_setup_device(hcd, udev, SETUP_CONTEXT_ONLY); + return xhci_setup_device(hcd, udev, SETUP_CONTEXT_ONLY, + XHCI_CMD_DEFAULT_TIMEOUT_MS); } /* diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h index 7e282b4522c0..1ac3caca6b6f 100644 --- a/drivers/usb/host/xhci.h +++ b/drivers/usb/host/xhci.h @@ -818,6 +818,8 @@ struct xhci_command { struct completion *completion; union xhci_trb *command_trb; struct list_head cmd_list; + /* xHCI command response timeout in milliseconds */ + unsigned int timeout_ms; }; /* drop context bitmasks */ @@ -1576,8 +1578,8 @@ struct xhci_td { unsigned int num_trbs; }; -/* xHCI command default timeout value */ -#define XHCI_CMD_DEFAULT_TIMEOUT (5 * HZ) +/* xHCI command default timeout value in milliseconds */ +#define XHCI_CMD_DEFAULT_TIMEOUT_MS 5000 /* command descriptor */ struct xhci_cd { diff --git a/include/linux/usb/hcd.h b/include/linux/usb/hcd.h index 61d4f0b793dc..d0e19ac3ba6c 100644 --- a/include/linux/usb/hcd.h +++ b/include/linux/usb/hcd.h @@ -372,8 +372,9 @@ struct hc_driver { * or bandwidth constraints. */ void (*reset_bandwidth)(struct usb_hcd *, struct usb_device *); - /* Returns the hardware-chosen device address */ - int (*address_device)(struct usb_hcd *, struct usb_device *udev); + /* Set the hardware-chosen device address */ + int (*address_device)(struct usb_hcd *, struct usb_device *udev, + unsigned int timeout_ms); /* prepares the hardware to send commands to the device */ int (*enable_device)(struct usb_hcd *, struct usb_device *udev); /* Notifies the HCD after a hub descriptor is fetched. diff --git a/include/linux/usb/quirks.h b/include/linux/usb/quirks.h index eeb7c2157c72..0cb464e3eaf4 100644 --- a/include/linux/usb/quirks.h +++ b/include/linux/usb/quirks.h @@ -72,4 +72,7 @@ /* device has endpoints that should be ignored */ #define USB_QUIRK_ENDPOINT_IGNORE BIT(15) +/* short device address timeout */ +#define USB_QUIRK_SHORT_DEVICE_ADDR_TIMEOUT BIT(16) + #endif /* __LINUX_USB_QUIRKS_H */
Currently, the timeout for the set address command is fixed at 5 seconds in the xhci driver. This means the host waits up to 5 seconds to receive a response for the set_address command from the device. In the automotive context, most smartphone enumerations, including screen projection, should ideally complete within 3 seconds. Achieving this is impossible in scenarios where the set_address is not successful and waits for a timeout. The shortened address device timeout quirks provide the flexibility to align with a 3-second time limit in the event of errors. By swiftly triggering a failure response and swiftly initiating retry procedures, these quirks ensure efficient and rapid recovery, particularly in automotive contexts where rapid smartphone enumeration and screen projection are vital. The quirk will set the timeout to 500 ms from 5 seconds. To use the quirk, please write "vendor_id:product_id:p" to /sys/bus/usb/drivers/hub/module/parameter/quirks For example, echo "0x2c48:0x0132:p" > /sys/bus/usb/drivers/hub/module/parameter/quirks" Signed-off-by: Hardik Gajjar <hgajjar@de.adit-jv.com> --- changes since version 1: - implement quirk instead of new API in xhci driver changes since version 2: - Add documentation for the new quirk. - Define the timeout unit in milliseconds in variable names and function arguments. - Change the xHCI command timeout from HZ (jiffies) to milliseconds. - Add APTIV usb hub vendor and product ID in device quirk list - Adding some other comments for clarity --- .../admin-guide/kernel-parameters.txt | 3 +++ drivers/usb/core/hub.c | 13 ++++++++-- drivers/usb/core/quirks.c | 6 +++++ drivers/usb/host/xhci-mem.c | 2 ++ drivers/usb/host/xhci-ring.c | 11 ++++---- drivers/usb/host/xhci.c | 25 +++++++++++++------ drivers/usb/host/xhci.h | 6 +++-- include/linux/usb/hcd.h | 5 ++-- include/linux/usb/quirks.h | 3 +++ 9 files changed, 56 insertions(+), 18 deletions(-)