Re: [RFT PATCH 1/2] xhci: Reconfigure endpoint 0 max packet size only during endpoint reset

[Kuen-Han Tsai]

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
>