On Tue, Nov 28, 2023 at 10:00 PM Mathias Nyman
<mathias.nyman@xxxxxxxxxxxxxxx> wrote:
>
> The max packet size for full speed control endpoint 0 may vary. It is
> defined in the device descriptor, which is read using the same endpoint.
> Usb core sets a temporary max packet size value until the real value is
> read.
>
> xhci driver needs to reconfigure the endpoint context seen by the
> controller if the max packet size changes.
> It makes more sense to do this reconfiguration in xhci_endpoint_reset()
> instead of urb enqueue as usb core will call endpoint reset during
> enumeration if the max packet values differ.
> Max packet size adjustment for endpoinr 0 can only happen once per
> device enumeration.
s/endpoinr/endpoint
>
> Previously the max packet size was checked during every urb enqueue.
> This is an additional check for every enqueued urb, and also turned out
> to have locking issues as urbs may be queued in any context while xhci
> max packet size reconfiguration requires memory allocation, locking, and
> sleeping.
>
> Tested with a full speed device using both old and new scheme enumeration
> and a intentionally incorrect preliminary max packet size value.
s/a intentionally/an intentionally
>
> Signed-off-by: Mathias Nyman <mathias.nyman@xxxxxxxxxxxxxxx>
> ---
> drivers/usb/host/xhci.c | 85 ++++++++++++++++++++---------------------
> 1 file changed, 42 insertions(+), 43 deletions(-)
>
> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> index 884b0898d9c9..df31d44498d6 100644
> --- a/drivers/usb/host/xhci.c
> +++ b/drivers/usb/host/xhci.c
> @@ -1438,10 +1438,8 @@ static int xhci_configure_endpoint(struct xhci_hcd *xhci,
> * descriptor. If the usb_device's max packet size changes after that point,
> * we need to issue an evaluate context command and wait on it.
> */
> -static int xhci_check_maxpacket(struct xhci_hcd *xhci, unsigned int slot_id,
> - unsigned int ep_index, struct urb *urb, gfp_t mem_flags)
> +static int xhci_check_ep0_maxpacket(struct xhci_hcd *xhci, struct xhci_virt_device *vdev)
> {
> - struct xhci_container_ctx *out_ctx;
> struct xhci_input_control_ctx *ctrl_ctx;
> struct xhci_ep_ctx *ep_ctx;
> struct xhci_command *command;
> @@ -1449,11 +1447,15 @@ static int xhci_check_maxpacket(struct xhci_hcd *xhci, unsigned int slot_id,
> int hw_max_packet_size;
> int ret = 0;
>
> - out_ctx = xhci->devs[slot_id]->out_ctx;
> - ep_ctx = xhci_get_ep_ctx(xhci, out_ctx, ep_index);
> + ep_ctx = xhci_get_ep_ctx(xhci, vdev->out_ctx, 0);
> hw_max_packet_size = MAX_PACKET_DECODED(le32_to_cpu(ep_ctx->ep_info2));
> - max_packet_size = usb_endpoint_maxp(&urb->dev->ep0.desc);
> - if (hw_max_packet_size != max_packet_size) {
> + max_packet_size = usb_endpoint_maxp(&vdev->udev->ep0.desc);
> +
> + if (hw_max_packet_size == max_packet_size)
> + return 0;
> +
> + switch (max_packet_size) {
> + case 8: case 16: case 32: case 64: case 9:
> xhci_dbg_trace(xhci, trace_xhci_dbg_context_change,
> "Max Packet Size for ep 0 changed.");
> xhci_dbg_trace(xhci, trace_xhci_dbg_context_change,
> @@ -1465,28 +1467,22 @@ static int xhci_check_maxpacket(struct xhci_hcd *xhci, unsigned int slot_id,
> xhci_dbg_trace(xhci, trace_xhci_dbg_context_change,
> "Issuing evaluate context command.");
>
> - /* Set up the input context flags for the command */
> - /* FIXME: This won't work if a non-default control endpoint
> - * changes max packet sizes.
> - */
> -
> - command = xhci_alloc_command(xhci, true, mem_flags);
> + command = xhci_alloc_command(xhci, true, GFP_KERNEL);
> if (!command)
> return -ENOMEM;
>
> - command->in_ctx = xhci->devs[slot_id]->in_ctx;
> + command->in_ctx = vdev->in_ctx;
> ctrl_ctx = xhci_get_input_control_ctx(command->in_ctx);
> if (!ctrl_ctx) {
> xhci_warn(xhci, "%s: Could not get input context, bad type.\n",
> __func__);
> ret = -ENOMEM;
> - goto command_cleanup;
> + break;
> }
> /* Set up the modified control endpoint 0 */
> - xhci_endpoint_copy(xhci, xhci->devs[slot_id]->in_ctx,
> - xhci->devs[slot_id]->out_ctx, ep_index);
> + xhci_endpoint_copy(xhci, vdev->in_ctx, vdev->out_ctx, 0);
>
> - ep_ctx = xhci_get_ep_ctx(xhci, command->in_ctx, ep_index);
> + ep_ctx = xhci_get_ep_ctx(xhci, command->in_ctx, 0);
> ep_ctx->ep_info &= cpu_to_le32(~EP_STATE_MASK);/* must clear */
> ep_ctx->ep_info2 &= cpu_to_le32(~MAX_PACKET_MASK);
> ep_ctx->ep_info2 |= cpu_to_le32(MAX_PACKET(max_packet_size));
> @@ -1494,17 +1490,20 @@ static int xhci_check_maxpacket(struct xhci_hcd *xhci, unsigned int slot_id,
> ctrl_ctx->add_flags = cpu_to_le32(EP0_FLAG);
> ctrl_ctx->drop_flags = 0;
>
> - ret = xhci_configure_endpoint(xhci, urb->dev, command,
> - true, false);
> -
> - /* Clean up the input context for later use by bandwidth
> - * functions.
> - */
> + ret = xhci_configure_endpoint(xhci, vdev->udev, command,
> + true, false);
> + /* Clean up the input context for later use by bandwidth functions */
> ctrl_ctx->add_flags = cpu_to_le32(SLOT_FLAG);
> -command_cleanup:
> - kfree(command->completion);
> - kfree(command);
> + break;
> + default:
> + dev_dbg(&vdev->udev->dev, "incorrect max packet size %d for ep0\n",
> + max_packet_size);
> + return -EINVAL;
> }
> +
> + kfree(command->completion);
> + kfree(command);
> +
> return ret;
> }
>
> @@ -1561,21 +1560,6 @@ static int xhci_urb_enqueue(struct usb_hcd *hcd, struct urb *urb, gfp_t mem_flag
>
> trace_xhci_urb_enqueue(urb);
>
> - if (usb_endpoint_xfer_control(&urb->ep->desc)) {
> - /* Check to see if the max packet size for the default control
> - * endpoint changed during FS device enumeration
> - */
> - if (urb->dev->speed == USB_SPEED_FULL) {
> - ret = xhci_check_maxpacket(xhci, slot_id,
> - ep_index, urb, mem_flags);
> - if (ret < 0) {
> - xhci_urb_free_priv(urb_priv);
> - urb->hcpriv = NULL;
> - return ret;
> - }
> - }
> - }
> -
> spin_lock_irqsave(&xhci->lock, flags);
>
> if (xhci->xhc_state & XHCI_STATE_DYING) {
> @@ -3104,6 +3088,21 @@ static void xhci_endpoint_reset(struct usb_hcd *hcd,
> int err;
>
> xhci = hcd_to_xhci(hcd);
> + ep_index = xhci_get_endpoint_index(&host_ep->desc);
> +
> + /*
> + * Usb core assumes a max packet value for ep0 on FS devices until the
> + * real value is read from the descriptor. Core resets Ep0 if values
> + * mismatch. Reconfigure the xhci ep0 endpoint context here in that case
> + */
> + if (usb_endpoint_xfer_control(&host_ep->desc) && ep_index == 0) {
> + udev = container_of(host_ep, struct usb_device, ep0);
> + if (udev->speed == USB_SPEED_FULL)
> + xhci_check_ep0_maxpacket(xhci, xhci->devs[udev->slot_id]);
> + /* Nothing else should be done here for ep0 during ep reset */
> + return;
> + }
> +
Could there be a race condition between the xhci_endpoint_reset() and
xhci_free_dev() functions, resulting in the xhci->devs[udev->slot_id]
becoming null?
If so, a null pointer dereference will happen in
xhci_check_ep0_maxpacket() when accessing vdev->out_ctx.
> if (!host_ep->hcpriv)
> return;
> udev = (struct usb_device *) host_ep->hcpriv;
> @@ -3116,7 +3115,7 @@ static void xhci_endpoint_reset(struct usb_hcd *hcd,
> */
> if (!udev->slot_id || !vdev)
> return;
> - ep_index = xhci_get_endpoint_index(&host_ep->desc);
> +
> ep = &vdev->eps[ep_index];
>
> /* Bail out if toggle is already being cleared by a endpoint reset */
> --
> 2.25.1
>
