Re: [PATCH v1] platform/x86: wmi: Switch to use new generic UUID API
On Fri, 2018-07-13 at 15:53 +0300, Andy Shevchenko wrote: > There are new types and helpers that are supposed to be used in new > code. > > As a preparation to get rid of legacy types and API functions do > the conversion here. > > While here, update Copyright to reflect this change along with > previous > one for the topic. Oops, sorry for the noise, wrong patch from old archive. -- Andy Shevchenko Intel Finland Oy
Re: [PATCH v1] platform/x86: wmi: Switch to use new generic UUID API
On Fri, Sep 1, 2017 at 1:23 AM, Christoph Hellwig wrote: > On Wed, Aug 30, 2017 at 01:01:28PM -0700, Andy Lutomirski wrote: >> What I'm saying is: I agree that "RFC4122 UUID" and "wintel GUID" are >> different, but the new structs aren't called "RFC4122 UUID" and >> "wintel GUID" - they're called "uuid" and "guid". I think the latter >> is very far from intuitive. I read the wmi patches several times >> before I figured out that they were even potentially correct. > > uuid_t is the classic type for the RFC4122 UUID, userspace libuuid > which exists on just about any platforms calls it that, and in > the kernel XFS has been since before uuid_be was introduces. > Now > for the Wintel GUID we'd have to call it "GUID" to stay close to > the roots, but I think guid_t is a good enough Linux-ish approximation > for that. Why? I guess I agree that the string "guid" of whatever case should be there, and, if the world actually thought that "guid" meant "little-endian Wintel monstrosity", I'd be fine with that. But it doesn't. I just Googled the string guid and the entire first page of results either directly states that guid and uuid are synonymous or merely fails to acknowledge the difference. Even the link that actually points to Microsoft: https://msdn.microsoft.com/en-us/library/system.guid(v=vs.110).aspx says nothing about endianness. It mentions a constructor that accepts a 16 byte array and *does not document* what it means. For good measure, I Binged it, too. Same results. So please, please, PLEASE give them names that actually explain something. How about "guid_le"? That uses the Microsoft-preferred "g" *and* conveys the important bit, which is that the endianness is screwed up. FWIW, just plain "guid' is also problematic even if the reader somehow realizes that it specifically means the Wintel one. The actual Windows definition of GUID is: typedef struct _GUID { DWORD Data1; WORD Data2; WORD Data3; BYTE Data4[8]; } GUID; Which is not a valid substitute for the one in Linus' tree: typedef struct { __u8 b[16]; } guid_t; MS's GUID has its endianness depend on the system endianness. The shiny one in uapi/uuid.h is always little-endian. So if you really want to stick to MS's terminology, the new struct is wrong. Please just rename it "guid_le_t" or "guid_le". Until/unless something that happens, I'm not going to ack the changes to the wmi driver, since I think they're entirely a step backwards and they take code that looks correct and make it look wrong. > >> 1. Make them totally separate. Have a function to convert a string to >> a uuid_le (or a guid_le or whatever you want to call it, as long as >> "le" or perhaps "wintel" is involved so it's obvious.) Have another >> function to convert back. Teach printk to understand %pULE. > > They are entirely separate - one uses the uuid_* functions, one uses > guid_* functions. Keeping them separate is fine with me. My objection is not to the API structure -- it's to the fact that neither the obvious reading (guid == uuid) nor the Wintel-centric reading (struct GUID) actually match what the code does.
Re: [PATCH v1] platform/x86: wmi: Switch to use new generic UUID API
On Wed, Aug 30, 2017 at 01:01:28PM -0700, Andy Lutomirski wrote: > What I'm saying is: I agree that "RFC4122 UUID" and "wintel GUID" are > different, but the new structs aren't called "RFC4122 UUID" and > "wintel GUID" - they're called "uuid" and "guid". I think the latter > is very far from intuitive. I read the wmi patches several times > before I figured out that they were even potentially correct. uuid_t is the classic type for the RFC4122 UUID, userspace libuuid which exists on just about any platforms calls it that, and in the kernel XFS has been since before uuid_be was introduces. Now for the Wintel GUID we'd have to call it "GUID" to stay close to the roots, but I think guid_t is a good enough Linux-ish approximation for that. > 1. Make them totally separate. Have a function to convert a string to > a uuid_le (or a guid_le or whatever you want to call it, as long as > "le" or perhaps "wintel" is involved so it's obvious.) Have another > function to convert back. Teach printk to understand %pULE. They are entirely separate - one uses the uuid_* functions, one uses guid_* functions. > 2. Have a function to convert back and forth so that kernel code uses > the real RFC4122 UUID for internal representations and keep just %pU. 99% of the uses are in interfaces we don't control - on disk, on the wire, firmware, hypervisor. So there really is no point in converting between them.
Re: [PATCH v1] platform/x86: wmi: Switch to use new generic UUID API
On Wed, Aug 30, 2017 at 5:25 AM, Christoph Hellwig wrote: > On Sun, Aug 13, 2017 at 08:34:56AM -0700, Andy Lutomirski wrote: >> > This specification defines a Uniform Resource Name namespace for >> > UUIDs (Universally Unique IDentifier), also known as GUIDs (Globally >> > Unique IDentifier). >> >> No, that still matches what I thought I knew: "UUID" and "GUID" are synonyms. > > Well, in practice they aren't - wintel GUID are big endian, and > RFC4122 clearly states it is big endian, although it uses the term > "network byte order": What I'm saying is: I agree that "RFC4122 UUID" and "wintel GUID" are different, but the new structs aren't called "RFC4122 UUID" and "wintel GUID" - they're called "uuid" and "guid". I think the latter is very far from intuitive. I read the wmi patches several times before I figured out that they were even potentially correct. >> >> typedef whatever uuid_t; >> typedef something_different uuid_le; /* which already existed */ >> >> extern void uuid_le_to_uuid(uuid_t *out, uuid_le *in); >> extern void uuid_to_uuid_le(...); > > What's the point of converting between a RFC4122 UUID and a Wintel > GUID? They are used for entirely different things. I can see at least two clean ways to design the API: 1. Make them totally separate. Have a function to convert a string to a uuid_le (or a guid_le or whatever you want to call it, as long as "le" or perhaps "wintel" is involved so it's obvious.) Have another function to convert back. Teach printk to understand %pULE. 2. Have a function to convert back and forth so that kernel code uses the real RFC4122 UUID for internal representations and keep just %pU. --Andy
Re: [PATCH v1] platform/x86: wmi: Switch to use new generic UUID API
On Sun, Aug 13, 2017 at 08:34:56AM -0700, Andy Lutomirski wrote: > > This specification defines a Uniform Resource Name namespace for > > UUIDs (Universally Unique IDentifier), also known as GUIDs (Globally > > Unique IDentifier). > > No, that still matches what I thought I knew: "UUID" and "GUID" are synonyms. Well, in practice they aren't - wintel GUID are big endian, and RFC4122 clearly states it is big endian, although it uses the term "network byte order": In the absence of explicit application or presentation protocol specification to the contrary, a UUID is encoded as a 128-bit object, as follows: The fields are encoded as 16 octets, with the sizes and order of the fields defined above, and with each field encoded with the Most Significant Byte first (known as network byte order). Note that the field names, particularly for multiplexed fields, follow historical practice. So yes, it is big endian, and it is defined an a sequence of octets. > > typedef whatever uuid_t; > typedef something_different uuid_le; /* which already existed */ > > extern void uuid_le_to_uuid(uuid_t *out, uuid_le *in); > extern void uuid_to_uuid_le(...); What's the point of converting between a RFC4122 UUID and a Wintel GUID? They are used for entirely different things.
Re: [PATCH v1] platform/x86: wmi: Switch to use new generic UUID API
On Sun, Aug 13, 2017 at 8:23 AM, Andy Lutomirski wrote: > On Sun, Aug 13, 2017 at 7:15 AM, Andy Shevchenko > wrote: >> On Sun, 2017-08-06 at 08:43 -0700, Andy Lutomirski wrote: >>> On Fri, Aug 4, 2017 at 8:27 AM, Andy Shevchenko >>> wrote: >>> > On Fri, Aug 4, 2017 at 6:01 PM, Andy Lutomirski >>> > wrote: >>> > > > On Aug 2, 2017, at 9:28 AM, Andy Shevchenko >> > > > inux.intel.com> wrote: >> >>> > > NAK. guid_block is a firmware interface, so opaque kernel types >>> > > don't >>> > > belong in it. >>> > >>> > I f we leave this, what do you think about everything else? >>> >>> Assuming it works, it's fine with me. I'd be happy to test. >> >> Just sent v2. >> >>> Keep in mind that this beast is a *little-endian* GUID abomination, >>> and I don't see generic conversion helpers. Something might need to >>> be added. >> >> Do you mean something like char16_to_guid() / char16_to_uuid() ? > > No, I mean something like uuid_le_to_guid(). There's an existing > helper uuid_le_to_bin() which, for all that the implementation and > signature is ridiculous, does the right thing. Oh, crikey. I just read the changelog here: commit f9727a17db9bab71ddae91f74f11a8a2f9a0ece6 Author: Christoph Hellwig Date: Wed May 17 10:02:48 2017 +0200 uuid: rename uuid types Our "little endian" UUID really is a Wintel GUID, so rename it and its helpers such (guid_t). The big endian UUID is the only true one, so give it the name uuid_t. The uuid_le and uuid_be names are retained for now, but will hopefully go away soon. The exception to that are the _cmp helpers that will be replaced by better primitives ASAP and thus don't get the new names. Andy and Christoph, can you rethink this? As a driver writer who doesn't want to resort to reading changelog entries to explain that the names of structures in header files don't actually match their accepted usage, this is awful. Here's the actual sructure definition in Linus' tree: typedef struct { __u8 b[UUID_SIZE]; } uuid_t; No comments. Maybe I'll try something authoritative like RFC 4122: > This specification defines a Uniform Resource Name namespace for > UUIDs (Universally Unique IDentifier), also known as GUIDs (Globally > Unique IDentifier). No, that still matches what I thought I knew: "UUID" and "GUID" are synonyms. As far as I know, there are exactly three representations of UUIID/GUID: the sane big-endian one, the totally nuts Wintel little-endian one, and text. Why not make the Linux helpers self-explanatory: typedef whatever uuid_t; typedef something_different uuid_le; /* which already existed */ extern void uuid_le_to_uuid(uuid_t *out, uuid_le *in); extern void uuid_to_uuid_le(...); and then no one has to read changelogs or nasty constants in a library file somewhere to figure out what's going on. In any event, from my perspective as a wmi driver sometimes maintainer, NAK to this whole patch. It takes somewhat awkward but reasonably clear code and updates it to make it look actively incorrect. If you can find a way to rework the header and helpers to make the driver code more readable, that would be great. In the mean time, I think this is purely a regression. --Andy
Re: [PATCH v1] platform/x86: wmi: Switch to use new generic UUID API
On Sun, Aug 13, 2017 at 7:15 AM, Andy Shevchenko wrote: > On Sun, 2017-08-06 at 08:43 -0700, Andy Lutomirski wrote: >> On Fri, Aug 4, 2017 at 8:27 AM, Andy Shevchenko >> wrote: >> > On Fri, Aug 4, 2017 at 6:01 PM, Andy Lutomirski >> > wrote: >> > > > On Aug 2, 2017, at 9:28 AM, Andy Shevchenko > > > > inux.intel.com> wrote: > >> > > NAK. guid_block is a firmware interface, so opaque kernel types >> > > don't >> > > belong in it. >> > >> > I f we leave this, what do you think about everything else? >> >> Assuming it works, it's fine with me. I'd be happy to test. > > Just sent v2. > >> Keep in mind that this beast is a *little-endian* GUID abomination, >> and I don't see generic conversion helpers. Something might need to >> be added. > > Do you mean something like char16_to_guid() / char16_to_uuid() ? No, I mean something like uuid_le_to_guid(). There's an existing helper uuid_le_to_bin() which, for all that the implementation and signature is ridiculous, does the right thing.
Re: [PATCH v1] platform/x86: wmi: Switch to use new generic UUID API
On Sun, 2017-08-06 at 08:43 -0700, Andy Lutomirski wrote: > On Fri, Aug 4, 2017 at 8:27 AM, Andy Shevchenko > wrote: > > On Fri, Aug 4, 2017 at 6:01 PM, Andy Lutomirski > > wrote: > > > > On Aug 2, 2017, at 9:28 AM, Andy Shevchenko > > > inux.intel.com> wrote: > > > NAK. guid_block is a firmware interface, so opaque kernel types > > > don't > > > belong in it. > > > > I f we leave this, what do you think about everything else? > > Assuming it works, it's fine with me. I'd be happy to test. Just sent v2. > Keep in mind that this beast is a *little-endian* GUID abomination, > and I don't see generic conversion helpers. Something might need to > be added. Do you mean something like char16_to_guid() / char16_to_uuid() ? -- Andy Shevchenko Intel Finland Oy
Re: [PATCH v1] platform/x86: wmi: Switch to use new generic UUID API
On Fri, Aug 4, 2017 at 8:27 AM, Andy Shevchenko wrote: > On Fri, Aug 4, 2017 at 6:01 PM, Andy Lutomirski wrote: >>> On Aug 2, 2017, at 9:28 AM, Andy Shevchenko >>> wrote: >>> >>> There are new types and helpers that are supposed to be used in new code. >>> >>> As a preparation to get rid of legacy types and API functions do >>> the conversion here. >>> >>> While here, update Copyright to reflect this change along with previous >>> one for the topic. > >>> struct guid_block { >>> -char guid[16]; >>> +guid_t guid; >> >> NAK. guid_block is a firmware interface, so opaque kernel types don't >> belong in it. > > I f we leave this, what do you think about everything else? Assuming it works, it's fine with me. I'd be happy to test. Keep in mind that this beast is a *little-endian* GUID abomination, and I don't see generic conversion helpers. Something might need to be added. --Andy
Re: [PATCH v1] platform/x86: wmi: Switch to use new generic UUID API
On Fri, Aug 4, 2017 at 6:01 PM, Andy Lutomirski wrote: >> On Aug 2, 2017, at 9:28 AM, Andy Shevchenko >> wrote: >> >> There are new types and helpers that are supposed to be used in new code. >> >> As a preparation to get rid of legacy types and API functions do >> the conversion here. >> >> While here, update Copyright to reflect this change along with previous >> one for the topic. >> struct guid_block { >> -char guid[16]; >> +guid_t guid; > > NAK. guid_block is a firmware interface, so opaque kernel types don't > belong in it. I f we leave this, what do you think about everything else? -- With Best Regards, Andy Shevchenko
Re: [PATCH v1] platform/x86: wmi: Switch to use new generic UUID API
> On Aug 2, 2017, at 9:28 AM, Andy Shevchenko > wrote: > > There are new types and helpers that are supposed to be used in new code. > > As a preparation to get rid of legacy types and API functions do > the conversion here. > > While here, update Copyright to reflect this change along with previous > one for the topic. > > Signed-off-by: Andy Shevchenko > --- > drivers/platform/x86/wmi.c | 56 -- > 1 file changed, 29 insertions(+), 27 deletions(-) > > diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c > index e32ba575e8d9..7b8fac7067dc 100644 > --- a/drivers/platform/x86/wmi.c > +++ b/drivers/platform/x86/wmi.c > @@ -7,6 +7,8 @@ > * Copyright (C) 2001,2002 Richard Russon > * Copyright (c) 2001-2007 Anton Altaparmakov > * Copyright (C) 2001,2002 Jakob Kemi > + * ...replaced by generic UUID API: > + * Copyright (C) 2016,2017 Intel Corporation. > * > * WMI bus infrastructure by Andrew Lutomirski and Darren Hart: > *Copyright (C) 2015 Andrew Lutomirski > @@ -53,7 +55,7 @@ MODULE_LICENSE("GPL"); > static LIST_HEAD(wmi_block_list); > > struct guid_block { > -char guid[16]; > +guid_t guid; NAK. guid_block is a firmware interface, so opaque kernel types don't belong in it.