diff mbox series

[v7,1/2] usb: xhci: Add timeout argument in address_device USB HCD callback

Message ID 20231027152029.104363-1-hgajjar@de.adit-jv.com
State New
Headers
Series [v7,1/2] usb: xhci: Add timeout argument in address_device USB HCD callback |

Commit Message

Hardik Gajjar Oct. 27, 2023, 3:20 p.m. UTC 
  - The HCD address_device callback now accepts a user-defined timeout value
  in milliseconds, providing better control over command execution times.
- The default timeout value for the address_device command has been set
  to 5000 ms, aligning with the USB 3.2 specification. However, this
  timeout can be adjusted as needed.
- The xhci_setup_device function has been updated to accept the timeout
  value, allowing it to specify the maximum wait time for the command
  operation to complete.
- The hub driver has also been updated to accommodate the newly added
  timeout parameter during the SET_ADDRESS request.

Signed-off-by: Hardik Gajjar <hgajjar@de.adit-jv.com>
---
Changes from version 1 to 6:
        - The changes were part of the large patch until v6
          Link to v6:https://lore.kernel.org/linux-usb/20231026101551.36551-1-hgajjar@de.adit-jv.com/
          This new patch was created by splitting patch v6 into two separate patches.
---
 drivers/usb/core/hub.c       |  2 +-
 drivers/usb/host/xhci-mem.c  |  2 ++
 drivers/usb/host/xhci-ring.c | 11 ++++++-----
 drivers/usb/host/xhci.c      | 23 ++++++++++++++++-------
 drivers/usb/host/xhci.h      |  9 +++++++--
 include/linux/usb/hcd.h      |  5 +++--
 6 files changed, 35 insertions(+), 17 deletions(-)

Comments

Mathias Nyman Oct. 30, 2023, 10:45 a.m. UTC | #1 
On 27.10.2023 18.20, Hardik Gajjar wrote:
> - The HCD address_device callback now accepts a user-defined timeout value
>    in milliseconds, providing better control over command execution times.
> - The default timeout value for the address_device command has been set
>    to 5000 ms, aligning with the USB 3.2 specification. However, this
>    timeout can be adjusted as needed.
> - The xhci_setup_device function has been updated to accept the timeout
>    value, allowing it to specify the maximum wait time for the command
>    operation to complete.
> - The hub driver has also been updated to accommodate the newly added
>    timeout parameter during the SET_ADDRESS request.
> 
> Signed-off-by: Hardik Gajjar <hgajjar@de.adit-jv.com>

For the xhci parts

Reviewed-by: Mathias Nyman <mathias.nyman@linux.intel.com>
Hardik Gajjar Nov. 3, 2023, 3:18 p.m. UTC | #2 
On Mon, Oct 30, 2023 at 12:45:54PM +0200, Mathias Nyman wrote:
> On 27.10.2023 18.20, Hardik Gajjar wrote:
> > - The HCD address_device callback now accepts a user-defined timeout value
> >    in milliseconds, providing better control over command execution times.
> > - The default timeout value for the address_device command has been set
> >    to 5000 ms, aligning with the USB 3.2 specification. However, this
> >    timeout can be adjusted as needed.
> > - The xhci_setup_device function has been updated to accept the timeout
> >    value, allowing it to specify the maximum wait time for the command
> >    operation to complete.
> > - The hub driver has also been updated to accommodate the newly added
> >    timeout parameter during the SET_ADDRESS request.
> > 
> > Signed-off-by: Hardik Gajjar <hgajjar@de.adit-jv.com>
> 
> For the xhci parts
> 
> Reviewed-by: Mathias Nyman <mathias.nyman@linux.intel.com>
> 
>

@Greg KH, Friendly reminder.

Thanks,
Hardik
Greg KH Nov. 3, 2023, 3:26 p.m. UTC | #3 
On Fri, Nov 03, 2023 at 04:18:22PM +0100, Hardik Gajjar wrote:
> On Mon, Oct 30, 2023 at 12:45:54PM +0200, Mathias Nyman wrote:
> > On 27.10.2023 18.20, Hardik Gajjar wrote:
> > > - The HCD address_device callback now accepts a user-defined timeout value
> > >    in milliseconds, providing better control over command execution times.
> > > - The default timeout value for the address_device command has been set
> > >    to 5000 ms, aligning with the USB 3.2 specification. However, this
> > >    timeout can be adjusted as needed.
> > > - The xhci_setup_device function has been updated to accept the timeout
> > >    value, allowing it to specify the maximum wait time for the command
> > >    operation to complete.
> > > - The hub driver has also been updated to accommodate the newly added
> > >    timeout parameter during the SET_ADDRESS request.
> > > 
> > > Signed-off-by: Hardik Gajjar <hgajjar@de.adit-jv.com>
> > 
> > For the xhci parts
> > 
> > Reviewed-by: Mathias Nyman <mathias.nyman@linux.intel.com>
> > 
> >
> 
> @Greg KH, Friendly reminder.

It is the m iddle of the merge window, my branches are closed for
obvious reasons until after -rc1 is out.  Please relax and wait for a
week or so _after_ -rc1 is out before worrying about anything.

thanks,

greg k-h
Greg KH Nov. 3, 2023, 3:27 p.m. UTC | #4 
On Fri, Nov 03, 2023 at 04:26:37PM +0100, Greg KH wrote:
> On Fri, Nov 03, 2023 at 04:18:22PM +0100, Hardik Gajjar wrote:
> > On Mon, Oct 30, 2023 at 12:45:54PM +0200, Mathias Nyman wrote:
> > > On 27.10.2023 18.20, Hardik Gajjar wrote:
> > > > - The HCD address_device callback now accepts a user-defined timeout value
> > > >    in milliseconds, providing better control over command execution times.
> > > > - The default timeout value for the address_device command has been set
> > > >    to 5000 ms, aligning with the USB 3.2 specification. However, this
> > > >    timeout can be adjusted as needed.
> > > > - The xhci_setup_device function has been updated to accept the timeout
> > > >    value, allowing it to specify the maximum wait time for the command
> > > >    operation to complete.
> > > > - The hub driver has also been updated to accommodate the newly added
> > > >    timeout parameter during the SET_ADDRESS request.
> > > > 
> > > > Signed-off-by: Hardik Gajjar <hgajjar@de.adit-jv.com>
> > > 
> > > For the xhci parts
> > > 
> > > Reviewed-by: Mathias Nyman <mathias.nyman@linux.intel.com>
> > > 
> > >
> > 
> > @Greg KH, Friendly reminder.
> 
> It is the m iddle of the merge window, my branches are closed for
> obvious reasons until after -rc1 is out.  Please relax and wait for a
> week or so _after_ -rc1 is out before worrying about anything.

In the meantime, to make things go faster, please help review patches
from others on the mailing list so that when the merge window does open
back up, my queue will be much smaller and lighter and yours will be
closer to the top.

thanks,

greg k-h
diff mbox series

Patch

diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index 3c54b218301c..376147af287f 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -4639,7 +4639,7 @@  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, USB_CTRL_SET_TIMEOUT);
 	else
 		retval = usb_control_msg(udev, usb_sndaddr0pipe(),
 				USB_REQ_SET_ADDRESS, 0, devnum, 0,
diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
index 8714ab5bf04d..4d6df03d255e 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;
 	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..7c972d5b475b 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -3997,12 +3997,18 @@  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.
+/**
+ * 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)
 {
 	const char *act = setup == SETUP_CONTEXT_ONLY ? "context" : "address";
 	unsigned long flags;
@@ -4059,6 +4065,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 +4192,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);
 }
 
 /*
diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index 7e282b4522c0..43193af562f4 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,11 @@  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.
+ * USB 3.2 spec, section 9.2.6.1
+ */
+#define XHCI_CMD_DEFAULT_TIMEOUT	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.