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.rudo...@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.rudo...@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
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
