grub-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v2 1/7] grub-core/bus/usb: Parse SuperSpeed companion descrip


From: Glenn Washburn
Subject: Re: [PATCH v2 1/7] grub-core/bus/usb: Parse SuperSpeed companion descriptors
Date: Thu, 18 Mar 2021 20:06:58 -0500

Hi Patrick,

I noticed some style issues while rebasing this code. Also this patch
series now looks like it'll need a touch up when rebasing on to the
release candidate (you might want to wait until the actual release
comes out to send the next version).

On Mon,  7 Dec 2020 08:41:21 +0100
Patrick Rudolph <patrick.rudolph@9elements.com> wrote:

> Parse the SS_ENDPOINT_COMPANION descriptor, which is only present on
> USB 3.0 capable devices and xHCI controllers. Make the descendp an
> array of pointers to the endpoint descriptor as it's no longer an
> continous array.
> 
> Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com>
> ---
>  grub-core/bus/usb/serial/common.c |  2 +-
>  grub-core/bus/usb/usb.c           | 44
> +++++++++++++++++++------------ grub-core/bus/usb/usbhub.c        |
> 22 ++++++++++++---- grub-core/commands/usbtest.c      |  2 +-
>  grub-core/disk/usbms.c            |  2 +-
>  grub-core/term/usb_keyboard.c     |  2 +-
>  include/grub/usb.h                |  2 +-
>  include/grub/usbdesc.h            | 11 +++++++-
>  8 files changed, 59 insertions(+), 28 deletions(-)
> 
> diff --git a/grub-core/bus/usb/serial/common.c
> b/grub-core/bus/usb/serial/common.c index 8e94c7dc0..63e64cc0a 100644
> --- a/grub-core/bus/usb/serial/common.c
> +++ b/grub-core/bus/usb/serial/common.c
> @@ -72,7 +72,7 @@ grub_usbserial_attach (grub_usb_device_t usbdev,
> int configno, int interfno, for (j = 0; j < interf->endpointcnt; j++)
>      {
>        struct grub_usb_desc_endp *endp;
> -      endp = &usbdev->config[0].interf[interfno].descendp[j];
> +      endp = usbdev->config[0].interf[interfno].descendp[j];
>  
>        if ((endp->endp_addr & 128) && (endp->attrib & 3) == 2
>         && (in_endp == GRUB_USB_SERIAL_ENDPOINT_LAST_MATCHING
> diff --git a/grub-core/bus/usb/usb.c b/grub-core/bus/usb/usb.c
> index 8da5e4c74..6b32bbd93 100644
> --- a/grub-core/bus/usb/usb.c
> +++ b/grub-core/bus/usb/usb.c
> @@ -115,7 +115,7 @@ grub_usb_device_initialize (grub_usb_device_t dev)
>    struct grub_usb_desc_device *descdev;
>    struct grub_usb_desc_config config;
>    grub_usb_err_t err;
> -  int i;
> +  int i, j;
>  
>    /* First we have to read first 8 bytes only and determine
>     * max. size of packet */
> @@ -149,6 +149,7 @@ grub_usb_device_initialize (grub_usb_device_t dev)
>        int currif;
>        char *data;
>        struct grub_usb_desc *desc;
> +      struct grub_usb_desc_endp *endp;
>  
>        /* First just read the first 4 bytes of the configuration
>        descriptor, after that it is known how many bytes really
> have @@ -192,24 +193,27 @@ grub_usb_device_initialize
> (grub_usb_device_t dev) = (struct grub_usb_desc_if *) &data[pos];
>         pos += dev->config[i].interf[currif].descif->length;
>  
> +    dev->config[i].interf[currif].descendp = grub_malloc (
> +            dev->config[i].interf[currif].descif->endpointcnt *
> +            sizeof(struct grub_usb_desc_endp));
> +
> +    j = 0;
>         while (pos < config.totallen)
>              {
>                desc = (struct grub_usb_desc *)&data[pos];
> -              if (desc->type == GRUB_USB_DESCRIPTOR_ENDPOINT)
> -                break;
> -              if (!desc->length)
> -                {
> -                  err = GRUB_USB_ERR_BADDEVICE;
> -                  goto fail;
> -                }
> -              pos += desc->length;
> -            }
> -
> -       /* Point to the first endpoint.  */
> -       dev->config[i].interf[currif].descendp
> -         = (struct grub_usb_desc_endp *) &data[pos];
> -       pos += (sizeof (struct grub_usb_desc_endp)
> -               *
> dev->config[i].interf[currif].descif->endpointcnt);
> +              if (desc->type == GRUB_USB_DESCRIPTOR_ENDPOINT) {
> +                endp = (struct grub_usb_desc_endp *) &data[pos];
> +                dev->config[i].interf[currif].descendp[j++] = endp;
> +                pos += desc->length;
> +              } else {

The above brackets don't follow GRUB convention. I haven't reviewed the
other patches, but there may be similar issues.

> +                if (!desc->length)
> +                  {
> +                    err = GRUB_USB_ERR_BADDEVICE;
> +                    goto fail;
> +                  }

They should be formatted like these.

> +                pos += desc->length;
> +             }
> +       }
>       }
>      }
>  
> @@ -217,8 +221,14 @@ grub_usb_device_initialize (grub_usb_device_t
> dev) 
>   fail:
>  
> -  for (i = 0; i < 8; i++)
> +  for (i = 0; i < 8; i++) {
> +    int currif;
> +
> +    for (currif = 0; currif < dev->config[i].descconf->numif;
> currif++)
> +      grub_free (dev->config[i].interf[currif].descendp);
> +
>      grub_free (dev->config[i].descconf);
> +  }

Ditto on the brackets. And many more, I think you get the idea.

>  
>    return err;
>  }
> diff --git a/grub-core/bus/usb/usbhub.c b/grub-core/bus/usb/usbhub.c
> index a06cce302..136bf6ee4 100644
> --- a/grub-core/bus/usb/usbhub.c
> +++ b/grub-core/bus/usb/usbhub.c
> @@ -82,8 +82,14 @@ grub_usb_hub_add_dev (grub_usb_controller_t
> controller, if (i == GRUB_USBHUB_MAX_DEVICES)
>      {
>        grub_error (GRUB_ERR_IO, "can't assign address to USB device");
> -      for (i = 0; i < 8; i++)
> -        grub_free (dev->config[i].descconf);
> +      for (i = 0; i < 8; i++) {
> +     int currif;
> +
> +     for (currif = 0; currif < dev->config[i].descconf->numif;
> currif++)
> +       grub_free (dev->config[i].interf[currif].descendp);
> +
> +     grub_free (dev->config[i].descconf);
> +      }
>        grub_free (dev);
>        return NULL;
>      }
> @@ -96,8 +102,14 @@ grub_usb_hub_add_dev (grub_usb_controller_t
> controller, i, 0, 0, NULL);
>    if (err)
>      {
> -      for (i = 0; i < 8; i++)
> -        grub_free (dev->config[i].descconf);
> +      for (i = 0; i < 8; i++) {
> +     int currif;
> +
> +     for (currif = 0; currif < dev->config[i].descconf->numif;
> currif++)
> +       grub_free (dev->config[i].interf[currif].descendp);
> +
> +     grub_free (dev->config[i].descconf);
> +      }
>        grub_free (dev);
>        return NULL;
>      }
> @@ -176,7 +188,7 @@ grub_usb_add_hub (grub_usb_device_t dev)
>         i++)
>      {
>        struct grub_usb_desc_endp *endp = NULL;
> -      endp = &dev->config[0].interf[0].descendp[i];
> +      endp = dev->config[0].interf[0].descendp[i];
>  
>        if ((endp->endp_addr & 128) && grub_usb_get_ep_type(endp)
>         == GRUB_USB_EP_INTERRUPT)
> diff --git a/grub-core/commands/usbtest.c
> b/grub-core/commands/usbtest.c index 2c6d93fe6..55a657635 100644
> --- a/grub-core/commands/usbtest.c
> +++ b/grub-core/commands/usbtest.c
> @@ -185,7 +185,7 @@ usb_iterate (grub_usb_device_t dev, void *data
> __attribute__ ((unused))) for (j = 0; j < interf->endpointcnt; j++)
>       {
>         struct grub_usb_desc_endp *endp;
> -       endp = &dev->config[0].interf[i].descendp[j];
> +       endp = dev->config[0].interf[i].descendp[j];
>  
>         grub_printf ("Endpoint #%d: %s, max packed size: %d,
> transfer type: %s, latency: %d\n", endp->endp_addr & 15,
> diff --git a/grub-core/disk/usbms.c b/grub-core/disk/usbms.c
> index 380ca4c4c..64e1be8ff 100644
> --- a/grub-core/disk/usbms.c
> +++ b/grub-core/disk/usbms.c
> @@ -184,7 +184,7 @@ grub_usbms_attach (grub_usb_device_t usbdev, int
> configno, int interfno) for (j = 0; j < interf->endpointcnt; j++)
>      {
>        struct grub_usb_desc_endp *endp;
> -      endp = &usbdev->config[0].interf[interfno].descendp[j];
> +      endp = usbdev->config[0].interf[interfno].descendp[j];
>  
>        if ((endp->endp_addr & 128) && (endp->attrib & 3) == 2)
>       /* Bulk IN endpoint.  */
> diff --git a/grub-core/term/usb_keyboard.c
> b/grub-core/term/usb_keyboard.c index e67b8f785..d65e3474d 100644
> --- a/grub-core/term/usb_keyboard.c
> +++ b/grub-core/term/usb_keyboard.c
> @@ -175,7 +175,7 @@ grub_usb_keyboard_attach (grub_usb_device_t
> usbdev, int configno, int interfno) for (j = 0; j <
> usbdev->config[configno].interf[interfno].descif->endpointcnt; j++)
>      {
> -      endp = &usbdev->config[configno].interf[interfno].descendp[j];
> +      endp = usbdev->config[configno].interf[interfno].descendp[j];
>  
>        if ((endp->endp_addr & 128) && grub_usb_get_ep_type(endp)
>         == GRUB_USB_EP_INTERRUPT)
> diff --git a/include/grub/usb.h b/include/grub/usb.h
> index 512ae1dd0..0527a3e2b 100644
> --- a/include/grub/usb.h
> +++ b/include/grub/usb.h
> @@ -149,7 +149,7 @@ struct grub_usb_interface
>  {
>    struct grub_usb_desc_if *descif;
>  
> -  struct grub_usb_desc_endp *descendp;
> +  struct grub_usb_desc_endp **descendp;
>  
>    /* A driver is handling this interface. Do we need to support
> multiple drivers for single interface?
> diff --git a/include/grub/usbdesc.h b/include/grub/usbdesc.h
> index aac5ab05a..bb2ab2e27 100644
> --- a/include/grub/usbdesc.h
> +++ b/include/grub/usbdesc.h
> @@ -29,7 +29,8 @@ typedef enum {
>    GRUB_USB_DESCRIPTOR_INTERFACE,
>    GRUB_USB_DESCRIPTOR_ENDPOINT,
>    GRUB_USB_DESCRIPTOR_DEBUG = 10,
> -  GRUB_USB_DESCRIPTOR_HUB = 0x29
> +  GRUB_USB_DESCRIPTOR_HUB = 0x29,
> +  GRUB_USB_DESCRIPTOR_SS_ENDPOINT_COMPANION = 0x30
>  } grub_usb_descriptor_t;
>  
>  struct grub_usb_desc
> @@ -105,6 +106,14 @@ struct grub_usb_desc_endp
>    grub_uint8_t interval;
>  } GRUB_PACKED;
>  
> +struct grub_usb_desc_ssep {
> +  grub_uint8_t  length;
> +  grub_uint8_t  type;
> +  grub_uint8_t  maxburst;
> +  grub_uint8_t  attrib;
> +  grub_uint16_t interval;
> +} GRUB_PACKED;
> +
>  struct grub_usb_desc_str
>  {
>    grub_uint8_t length;

Cheers,
Glenn



reply via email to

[Prev in Thread] Current Thread [Next in Thread]