| Message ID | 20170802132834.8223-1-andriy.shevchenko@linux.intel.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
> On Aug 2, 2017, at 9:28 AM, Andy Shevchenko <andriy.shevchenko@linux.intel.com> 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 <andriy.shevchenko@linux.intel.com> > --- > 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 <ldm@flatcap.org> > * Copyright (c) 2001-2007 Anton Altaparmakov > * Copyright (C) 2001,2002 Jakob Kemi <jakob.kemi@telia.com> > + * ...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.
On Fri, Aug 4, 2017 at 6:01 PM, Andy Lutomirski <luto@kernel.org> wrote: >> On Aug 2, 2017, at 9:28 AM, Andy Shevchenko <andriy.shevchenko@linux.intel.com> 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?
On Fri, Aug 4, 2017 at 8:27 AM, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Fri, Aug 4, 2017 at 6:01 PM, Andy Lutomirski <luto@kernel.org> wrote: >>> On Aug 2, 2017, at 9:28 AM, Andy Shevchenko <andriy.shevchenko@linux.intel.com> 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
On Sun, 2017-08-06 at 08:43 -0700, Andy Lutomirski wrote: > On Fri, Aug 4, 2017 at 8:27 AM, Andy Shevchenko > <andy.shevchenko@gmail.com> wrote: > > On Fri, Aug 4, 2017 at 6:01 PM, Andy Lutomirski <luto@kernel.org> > > wrote: > > > > On Aug 2, 2017, at 9:28 AM, Andy Shevchenko <andriy.shevchenko@l > > > > 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() ?
On Sun, Aug 13, 2017 at 7:15 AM, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > On Sun, 2017-08-06 at 08:43 -0700, Andy Lutomirski wrote: >> On Fri, Aug 4, 2017 at 8:27 AM, Andy Shevchenko >> <andy.shevchenko@gmail.com> wrote: >> > On Fri, Aug 4, 2017 at 6:01 PM, Andy Lutomirski <luto@kernel.org> >> > wrote: >> > > > On Aug 2, 2017, at 9:28 AM, Andy Shevchenko <andriy.shevchenko@l >> > > > 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.
On Sun, Aug 13, 2017 at 8:23 AM, Andy Lutomirski <luto@kernel.org> wrote: > On Sun, Aug 13, 2017 at 7:15 AM, Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: >> On Sun, 2017-08-06 at 08:43 -0700, Andy Lutomirski wrote: >>> On Fri, Aug 4, 2017 at 8:27 AM, Andy Shevchenko >>> <andy.shevchenko@gmail.com> wrote: >>> > On Fri, Aug 4, 2017 at 6:01 PM, Andy Lutomirski <luto@kernel.org> >>> > wrote: >>> > > > On Aug 2, 2017, at 9:28 AM, Andy Shevchenko <andriy.shevchenko@l >>> > > > 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 <hch@lst.de> 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
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.
On Wed, Aug 30, 2017 at 5:25 AM, Christoph Hellwig <hch@lst.de> 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
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.
On Fri, Sep 1, 2017 at 1:23 AM, Christoph Hellwig <hch@lst.de> 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.
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 <ldm@flatcap.org> * Copyright (c) 2001-2007 Anton Altaparmakov * Copyright (C) 2001,2002 Jakob Kemi <jakob.kemi@telia.com> + * ...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; union { char object_id[2]; struct { @@ -121,19 +123,19 @@ static struct platform_driver acpi_wmi_driver = { static bool find_guid(const char *guid_string, struct wmi_block **out) { - uuid_le guid_input; + guid_t guid_input; struct wmi_block *wblock; struct guid_block *block; struct list_head *p; - if (uuid_le_to_bin(guid_string, &guid_input)) + if (guid_parse(guid_string, &guid_input)) return false; list_for_each(p, &wmi_block_list) { wblock = list_entry(p, struct wmi_block, list); block = &wblock->gblock; - if (memcmp(block->guid, &guid_input, 16) == 0) { + if (guid_equal(&block->guid, &guid_input)) { if (out) *out = wblock; return true; @@ -420,7 +422,7 @@ EXPORT_SYMBOL_GPL(wmi_set_block); static void wmi_dump_wdg(const struct guid_block *g) { - pr_info("%pUL:\n", g->guid); + pr_info("%pUL:\n", &g->guid); if (g->flags & ACPI_WMI_EVENT) pr_info("\tnotify_id: 0x%02X\n", g->notify_id); else @@ -480,6 +482,7 @@ static void wmi_notify_debug(u32 value, void *context) /** * wmi_install_notify_handler - Register handler for WMI events + * @guid: 36 char string of the form fa50ff2b-f2e8-45de-83fa-65417f2f49ba * @handler: Function to handle notifications * @data: Data to be returned to handler when event is fired * @@ -490,20 +493,20 @@ wmi_notify_handler handler, void *data) { struct wmi_block *block; acpi_status status = AE_NOT_EXIST; - uuid_le guid_input; + guid_t guid_input; struct list_head *p; if (!guid || !handler) return AE_BAD_PARAMETER; - if (uuid_le_to_bin(guid, &guid_input)) + if (guid_parse(guid, &guid_input)) return AE_BAD_PARAMETER; list_for_each(p, &wmi_block_list) { acpi_status wmi_status; block = list_entry(p, struct wmi_block, list); - if (memcmp(block->gblock.guid, &guid_input, 16) == 0) { + if (guid_equal(&block->gblock.guid, &guid_input)) { if (block->handler && block->handler != wmi_notify_debug) return AE_ALREADY_ACQUIRED; @@ -524,6 +527,7 @@ EXPORT_SYMBOL_GPL(wmi_install_notify_handler); /** * wmi_uninstall_notify_handler - Unregister handler for WMI events + * @guid: 36 char string of the form fa50ff2b-f2e8-45de-83fa-65417f2f49ba * * Unregister handler for events sent to the ACPI-WMI mapper device. */ @@ -531,20 +535,20 @@ acpi_status wmi_remove_notify_handler(const char *guid) { struct wmi_block *block; acpi_status status = AE_NOT_EXIST; - uuid_le guid_input; + guid_t guid_input; struct list_head *p; if (!guid) return AE_BAD_PARAMETER; - if (uuid_le_to_bin(guid, &guid_input)) + if (guid_parse(guid, &guid_input)) return AE_BAD_PARAMETER; list_for_each(p, &wmi_block_list) { acpi_status wmi_status; block = list_entry(p, struct wmi_block, list); - if (memcmp(block->gblock.guid, &guid_input, 16) == 0) { + if (guid_equal(&block->gblock.guid, &guid_input)) { if (!block->handler || block->handler == wmi_notify_debug) return AE_NULL_ENTRY; @@ -633,7 +637,7 @@ static ssize_t modalias_show(struct device *dev, struct device_attribute *attr, { struct wmi_block *wblock = dev_to_wblock(dev); - return sprintf(buf, "wmi:%pUL\n", wblock->gblock.guid); + return sprintf(buf, "wmi:%pUL\n", &wblock->gblock.guid); } static DEVICE_ATTR_RO(modalias); @@ -642,7 +646,7 @@ static ssize_t guid_show(struct device *dev, struct device_attribute *attr, { struct wmi_block *wblock = dev_to_wblock(dev); - return sprintf(buf, "%pUL\n", wblock->gblock.guid); + return sprintf(buf, "%pUL\n", &wblock->gblock.guid); } static DEVICE_ATTR_RO(guid); @@ -725,10 +729,10 @@ static int wmi_dev_uevent(struct device *dev, struct kobj_uevent_env *env) { struct wmi_block *wblock = dev_to_wblock(dev); - if (add_uevent_var(env, "MODALIAS=wmi:%pUL", wblock->gblock.guid)) + if (add_uevent_var(env, "MODALIAS=wmi:%pUL", &wblock->gblock.guid)) return -ENOMEM; - if (add_uevent_var(env, "WMI_GUID=%pUL", wblock->gblock.guid)) + if (add_uevent_var(env, "WMI_GUID=%pUL", &wblock->gblock.guid)) return -ENOMEM; return 0; @@ -749,11 +753,11 @@ static int wmi_dev_match(struct device *dev, struct device_driver *driver) const struct wmi_device_id *id = wmi_driver->id_table; while (id->guid_string) { - uuid_le driver_guid; + guid_t driver_guid; - if (WARN_ON(uuid_le_to_bin(id->guid_string, &driver_guid))) + if (WARN_ON(guid_parse(id->guid_string, &driver_guid))) continue; - if (!memcmp(&driver_guid, wblock->gblock.guid, 16)) + if (guid_equal(&driver_guid, &wblock->gblock.guid)) return 1; id++; @@ -891,7 +895,7 @@ static int wmi_create_device(struct device *wmi_bus_dev, wblock->dev.dev.bus = &wmi_bus_type; wblock->dev.dev.parent = wmi_bus_dev; - dev_set_name(&wblock->dev.dev, "%pUL", gblock->guid); + dev_set_name(&wblock->dev.dev, "%pUL", &gblock->guid); device_initialize(&wblock->dev.dev); @@ -912,12 +916,12 @@ static void wmi_free_devices(struct acpi_device *device) } static bool guid_already_parsed(struct acpi_device *device, - const u8 *guid) + const guid_t *guid) { struct wmi_block *wblock; list_for_each_entry(wblock, &wmi_block_list, list) { - if (memcmp(wblock->gblock.guid, guid, 16) == 0) { + if (guid_equal(&wblock->gblock.guid, guid)) { /* * Because we historically didn't track the relationship * between GUIDs and ACPI nodes, we don't know whether @@ -972,7 +976,7 @@ static int parse_wdg(struct device *wmi_bus_dev, struct acpi_device *device) * case yet, so for now, we'll just ignore the duplicate * for device creation. */ - if (guid_already_parsed(device, gblock[i].guid)) + if (guid_already_parsed(device, &gblock[i].guid)) continue; wblock = kzalloc(sizeof(struct wmi_block), GFP_KERNEL); @@ -1009,7 +1013,7 @@ static int parse_wdg(struct device *wmi_bus_dev, struct acpi_device *device) retval = device_add(&wblock->dev.dev); if (retval) { dev_err(wmi_bus_dev, "failed to register %pULL\n", - wblock->gblock.guid); + &wblock->gblock.guid); if (debug_event) wmi_method_enable(wblock, 0); list_del(&wblock->list); @@ -1124,10 +1128,8 @@ static void acpi_wmi_notify_handler(acpi_handle handle, u32 event, wblock->handler(event, wblock->handler_data); } - if (debug_event) { - pr_info("DEBUG Event GUID: %pUL\n", - wblock->gblock.guid); - } + if (debug_event) + pr_info("DEBUG Event GUID: %pUL\n", &wblock->gblock.guid); acpi_bus_generate_netlink_event( wblock->acpi_device->pnp.device_class,
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 <andriy.shevchenko@linux.intel.com> --- drivers/platform/x86/wmi.c | 56 ++++++++++++++++++++++++---------------------- 1 file changed, 29 insertions(+), 27 deletions(-)