2023-01-25 14:34:07

by Andy Shevchenko

[permalink] [raw]
Subject: [PATCH v1 1/1] usb: gadget: Use correct APIs and data types for UUID handling

We have two types for UUIDs depending on the byte ordering.
Instead of explaining how bytes should go over the wire,
use dedicated APIs and data types. This removes a confusion
over the byte ordering.

Signed-off-by: Andy Shevchenko <[email protected]>
---
drivers/usb/gadget/composite.c | 4 ++--
include/linux/usb/webusb.h | 9 +++------
2 files changed, 5 insertions(+), 8 deletions(-)

diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
index 8e2603688016..fa7dd6cf014d 100644
--- a/drivers/usb/gadget/composite.c
+++ b/drivers/usb/gadget/composite.c
@@ -829,7 +829,7 @@ static int bos_desc(struct usb_composite_dev *cdev)
if (cdev->use_webusb) {
struct usb_plat_dev_cap_descriptor *webusb_cap;
struct usb_webusb_cap_data *webusb_cap_data;
- uuid_t webusb_uuid = WEBUSB_UUID;
+ guid_t webusb_uuid = WEBUSB_UUID;

webusb_cap = cdev->req->buf + le16_to_cpu(bos->wTotalLength);
webusb_cap_data = (struct usb_webusb_cap_data *) webusb_cap->CapabilityData;
@@ -841,7 +841,7 @@ static int bos_desc(struct usb_composite_dev *cdev)
webusb_cap->bDescriptorType = USB_DT_DEVICE_CAPABILITY;
webusb_cap->bDevCapabilityType = USB_PLAT_DEV_CAP_TYPE;
webusb_cap->bReserved = 0;
- export_uuid(webusb_cap->UUID, &webusb_uuid);
+ export_guid(webusb_cap->UUID, &webusb_uuid);

if (cdev->bcd_webusb_version != 0)
webusb_cap_data->bcdVersion = cpu_to_le16(cdev->bcd_webusb_version);
diff --git a/include/linux/usb/webusb.h b/include/linux/usb/webusb.h
index b430d84357f3..fe43020b4a48 100644
--- a/include/linux/usb/webusb.h
+++ b/include/linux/usb/webusb.h
@@ -11,15 +11,12 @@
#include "uapi/linux/usb/ch9.h"

/*
- * little endian PlatformCapablityUUID for WebUSB
+ * Little Endian PlatformCapablityUUID for WebUSB
* 3408b638-09a9-47a0-8bfd-a0768815b665
- * to identify Platform Device Capability descriptors as referring to WebUSB
- *
- * the UUID above MUST be sent over the wire as the byte sequence:
- * {0x38, 0xB6, 0x08, 0x34, 0xA9, 0x09, 0xA0, 0x47, 0x8B, 0xFD, 0xA0, 0x76, 0x88, 0x15, 0xB6, 0x65}.
+ * to identify Platform Device Capability descriptors as referring to WebUSB.
*/
#define WEBUSB_UUID \
- UUID_INIT(0x38b60834, 0xa909, 0xa047, 0x8b, 0xfd, 0xa0, 0x76, 0x88, 0x15, 0xb6, 0x65)
+ GUID_INIT(0x3408b638, 0x09a9, 0x47a0, 0x8b, 0xfd, 0xa0, 0x76, 0x88, 0x15, 0xb6, 0x65)

/*
* WebUSB Platform Capability data
--
2.39.0



2023-01-25 17:31:54

by Jó Ágila Bitsch

[permalink] [raw]
Subject: Re: [PATCH v1 1/1] usb: gadget: Use correct APIs and data types for UUID handling

On Wed, Jan 25, 2023 at 3:34 PM Andy Shevchenko
<[email protected]> wrote:
>
> We have two types for UUIDs depending on the byte ordering.
> Instead of explaining how bytes should go over the wire,
> use dedicated APIs and data types. This removes a confusion
> over the byte ordering.

Thanks for pointing this out. I was unaware of the exact UUID
functions, as I'm still quite a newbie here.

I compiled and tested your patch in my test setup and it works perfectly.

>
> Signed-off-by: Andy Shevchenko <[email protected]>
> ---
> drivers/usb/gadget/composite.c | 4 ++--
> include/linux/usb/webusb.h | 9 +++------
> 2 files changed, 5 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
> index 8e2603688016..fa7dd6cf014d 100644
> --- a/drivers/usb/gadget/composite.c
> +++ b/drivers/usb/gadget/composite.c
> @@ -829,7 +829,7 @@ static int bos_desc(struct usb_composite_dev *cdev)
> if (cdev->use_webusb) {
> struct usb_plat_dev_cap_descriptor *webusb_cap;
> struct usb_webusb_cap_data *webusb_cap_data;
> - uuid_t webusb_uuid = WEBUSB_UUID;
> + guid_t webusb_uuid = WEBUSB_UUID;
>
> webusb_cap = cdev->req->buf + le16_to_cpu(bos->wTotalLength);
> webusb_cap_data = (struct usb_webusb_cap_data *) webusb_cap->CapabilityData;
> @@ -841,7 +841,7 @@ static int bos_desc(struct usb_composite_dev *cdev)
> webusb_cap->bDescriptorType = USB_DT_DEVICE_CAPABILITY;
> webusb_cap->bDevCapabilityType = USB_PLAT_DEV_CAP_TYPE;
> webusb_cap->bReserved = 0;
> - export_uuid(webusb_cap->UUID, &webusb_uuid);
> + export_guid(webusb_cap->UUID, &webusb_uuid);
>
> if (cdev->bcd_webusb_version != 0)
> webusb_cap_data->bcdVersion = cpu_to_le16(cdev->bcd_webusb_version);
> diff --git a/include/linux/usb/webusb.h b/include/linux/usb/webusb.h
> index b430d84357f3..fe43020b4a48 100644
> --- a/include/linux/usb/webusb.h
> +++ b/include/linux/usb/webusb.h
> @@ -11,15 +11,12 @@
> #include "uapi/linux/usb/ch9.h"
>
> /*
> - * little endian PlatformCapablityUUID for WebUSB
> + * Little Endian PlatformCapablityUUID for WebUSB
> * 3408b638-09a9-47a0-8bfd-a0768815b665
> - * to identify Platform Device Capability descriptors as referring to WebUSB
> - *
> - * the UUID above MUST be sent over the wire as the byte sequence:
> - * {0x38, 0xB6, 0x08, 0x34, 0xA9, 0x09, 0xA0, 0x47, 0x8B, 0xFD, 0xA0, 0x76, 0x88, 0x15, 0xB6, 0x65}.

This is actually explicitly spelled out this way in the specification:
https://wicg.github.io/webusb/#webusb-platform-capability-descriptor

But I agree, the way you rewrote it is much clearer!

> + * to identify Platform Device Capability descriptors as referring to WebUSB.
> */
> #define WEBUSB_UUID \
> - UUID_INIT(0x38b60834, 0xa909, 0xa047, 0x8b, 0xfd, 0xa0, 0x76, 0x88, 0x15, 0xb6, 0x65)
> + GUID_INIT(0x3408b638, 0x09a9, 0x47a0, 0x8b, 0xfd, 0xa0, 0x76, 0x88, 0x15, 0xb6, 0x65)

Yes, this is definitely more readable.

>
> /*
> * WebUSB Platform Capability data
> --
> 2.39.0
>

2023-01-25 20:01:25

by Andy Shevchenko

[permalink] [raw]
Subject: Re: [PATCH v1 1/1] usb: gadget: Use correct APIs and data types for UUID handling

On Wed, Jan 25, 2023 at 06:31:36PM +0100, J? ?gila Bitsch wrote:
> On Wed, Jan 25, 2023 at 3:34 PM Andy Shevchenko
> <[email protected]> wrote:
> >
> > We have two types for UUIDs depending on the byte ordering.
> > Instead of explaining how bytes should go over the wire,
> > use dedicated APIs and data types. This removes a confusion
> > over the byte ordering.
>
> Thanks for pointing this out. I was unaware of the exact UUID
> functions, as I'm still quite a newbie here.
>
> I compiled and tested your patch in my test setup and it works perfectly.

Thanks for the testing. According to Submitting Patches documentation
you can provide a formal Tested-by tag.

--
With Best Regards,
Andy Shevchenko



2023-01-25 21:56:54

by Jó Ágila Bitsch

[permalink] [raw]
Subject: Re: [PATCH v1 1/1] usb: gadget: Use correct APIs and data types for UUID handling

On Wed, Jan 25, 2023 at 9:01 PM Andy Shevchenko
<[email protected]> wrote:
>
> On Wed, Jan 25, 2023 at 06:31:36PM +0100, Jó Ágila Bitsch wrote:
> > On Wed, Jan 25, 2023 at 3:34 PM Andy Shevchenko
> > <[email protected]> wrote:
> > >
> > > We have two types for UUIDs depending on the byte ordering.
> > > Instead of explaining how bytes should go over the wire,
> > > use dedicated APIs and data types. This removes a confusion
> > > over the byte ordering.
> >
> > Thanks for pointing this out. I was unaware of the exact UUID
> > functions, as I'm still quite a newbie here.
> >
> > I compiled and tested your patch in my test setup and it works perfectly.
>
> Thanks for the testing. According to Submitting Patches documentation
> you can provide a formal Tested-by tag.

Thanks for pointing this out to me.

I'm not really sure how to do that though.
On https://docs.kernel.org/process/submitting-patches.html#reviewer-s-statement-of-oversight,
it says:
> Both Tested-by and Reviewed-by tags, once received on mailing list from tester or reviewer, should be added by author to the applicable patches when sending next versions.

So I guess you could do that at your convenience on any next version.
Or is it already ok, if I just add the following line in my comment?

Tested-By: Jó Ágila Bitsch <[email protected]>

I'm still quite a newbie in the kernel development community, so
thanks for bearing with my ignorance :-)

Best regards and thanks a lot,



>
> --
> With Best Regards,
> Andy Shevchenko
>
>

2023-01-26 10:15:47

by Andy Shevchenko

[permalink] [raw]
Subject: Re: [PATCH v1 1/1] usb: gadget: Use correct APIs and data types for UUID handling

On Wed, Jan 25, 2023 at 10:54:58PM +0100, J? ?gila Bitsch wrote:
> On Wed, Jan 25, 2023 at 9:01 PM Andy Shevchenko
> <[email protected]> wrote:
> > On Wed, Jan 25, 2023 at 06:31:36PM +0100, J? ?gila Bitsch wrote:
> > > On Wed, Jan 25, 2023 at 3:34 PM Andy Shevchenko
> > > <[email protected]> wrote:

...

> > > I compiled and tested your patch in my test setup and it works perfectly.
> >
> > Thanks for the testing. According to Submitting Patches documentation
> > you can provide a formal Tested-by tag.
>
> Thanks for pointing this out to me.
>
> I'm not really sure how to do that though.
> On https://docs.kernel.org/process/submitting-patches.html#reviewer-s-statement-of-oversight,
> it says:
> > Both Tested-by and Reviewed-by tags, once received on mailing list from tester or reviewer, should be added by author to the applicable patches when sending next versions.
>
> So I guess you could do that at your convenience on any next version.

In mine I can only do if you give me that tag. Till this
line the tag is not given, but...

> Or is it already ok, if I just add the following line in my comment?

...this is what is expected to have.

> Tested-By: J? ?gila Bitsch <[email protected]>

And now _if_ I need a new version I will bear this with it. Otherwise
maintainer, who picks up the patch, takes care of gathering these all
together.

> I'm still quite a newbie in the kernel development community, so
> thanks for bearing with my ignorance :-)

So far you are doing good, thanks!

--
With Best Regards,
Andy Shevchenko