Re: [PATCH v1] platform/x86: wmi: Switch to use new generic UUID API

[Andy Shevchenko]

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

[Andy Shevchenko]

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

[Andy Lutomirski]

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

[Andy Lutomirski]

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

[Christoph Hellwig]

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

[Christoph Hellwig]

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

[Andy Lutomirski]

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

[Andy Lutomirski]

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

[Christoph Hellwig]

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

[Christoph Hellwig]

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

[Andy Lutomirski]

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

[Andy Lutomirski]

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

[Andy Lutomirski]

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

[Andy Lutomirski]

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

[Andy Shevchenko]

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

[Andy Shevchenko]

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

[Andy Lutomirski]

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

[Andy Lutomirski]

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

[Andy Shevchenko]

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

[Andy Shevchenko]

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

[Andy Lutomirski]

> 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.

Re: [PATCH v1] platform/x86: wmi: Switch to use new generic UUID API

[Andy Lutomirski]

> 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.