diff mbox

[v1,06/10] device property: switch to use UUID API

Message ID 1455711448-124103-7-git-send-email-andriy.shevchenko@linux.intel.com (mailing list archive)
State Not Applicable
Headers show

Commit Message

Andy Shevchenko Feb. 17, 2016, 12:17 p.m. UTC
Switch to use a generic UUID API instead of custom approach. It allows to
define UUIDs, compare them, and validate.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/acpi/property.c | 18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)

Comments

Rafael J. Wysocki Feb. 18, 2016, 12:03 a.m. UTC | #1
On Wednesday, February 17, 2016 02:17:24 PM Andy Shevchenko wrote:
> Switch to use a generic UUID API instead of custom approach. It allows to
> define UUIDs, compare them, and validate.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/acpi/property.c | 18 ++++++++----------
>  1 file changed, 8 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
> index 2aee416..1e75305 100644
> --- a/drivers/acpi/property.c
> +++ b/drivers/acpi/property.c
> @@ -25,15 +25,13 @@ static int acpi_data_get_property_array(struct acpi_device_data *data,
>  					const union acpi_object **obj);
>  
>  /* ACPI _DSD device properties UUID: daffd814-6eba-4d8c-8a91-bc9bbf4aa301 */
> -static const u8 prp_uuid[16] = {
> -	0x14, 0xd8, 0xff, 0xda, 0xba, 0x6e, 0x8c, 0x4d,
> -	0x8a, 0x91, 0xbc, 0x9b, 0xbf, 0x4a, 0xa3, 0x01
> -};
> +static const uuid_le prp_uuid =
> +	UUID_LE(0xdaffd814, 0x6eba, 0x4d8c,
> +		0x8a, 0x91, 0xbc, 0x9b, 0xbf, 0x4a, 0xa3, 0x01);
>  /* ACPI _DSD data subnodes UUID: dbb8e3e6-5886-4ba6-8795-1319f52a966b */
> -static const u8 ads_uuid[16] = {
> -	0xe6, 0xe3, 0xb8, 0xdb, 0x86, 0x58, 0xa6, 0x4b,
> -	0x87, 0x95, 0x13, 0x19, 0xf5, 0x2a, 0x96, 0x6b
> -};
> +static const uuid_le ads_uuid =
> +	UUID_LE(0xdbb8e3e6, 0x5886, 0x4ba6,
> +		0x87, 0x95, 0x13, 0x19, 0xf5, 0x2a, 0x96, 0x6b);
>  
>  static bool acpi_enumerate_nondev_subnodes(acpi_handle scope,
>  					   const union acpi_object *desc,
> @@ -138,7 +136,7 @@ static bool acpi_enumerate_nondev_subnodes(acpi_handle scope,
>  		    || links->type != ACPI_TYPE_PACKAGE)
>  			break;
>  
> -		if (memcmp(uuid->buffer.pointer, ads_uuid, sizeof(ads_uuid)))
> +		if (uuid_le_cmp(*(uuid_le *)uuid->buffer.pointer, ads_uuid))

Maybe it's too late, but I don't quite understand the pointer manipulations here.

I can see why you need a type conversion (although it looks ugly), but why do you
need to dereference it too?

Thanks,
Rafael
Mika Westerberg Feb. 18, 2016, 11:07 a.m. UTC | #2
On Wed, Feb 17, 2016 at 02:17:24PM +0200, Andy Shevchenko wrote:
> Switch to use a generic UUID API instead of custom approach. It allows to
> define UUIDs, compare them, and validate.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/acpi/property.c | 18 ++++++++----------
>  1 file changed, 8 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
> index 2aee416..1e75305 100644
> --- a/drivers/acpi/property.c
> +++ b/drivers/acpi/property.c
> @@ -25,15 +25,13 @@ static int acpi_data_get_property_array(struct acpi_device_data *data,
>  					const union acpi_object **obj);
>  
>  /* ACPI _DSD device properties UUID: daffd814-6eba-4d8c-8a91-bc9bbf4aa301 */
> -static const u8 prp_uuid[16] = {
> -	0x14, 0xd8, 0xff, 0xda, 0xba, 0x6e, 0x8c, 0x4d,
> -	0x8a, 0x91, 0xbc, 0x9b, 0xbf, 0x4a, 0xa3, 0x01
> -};
> +static const uuid_le prp_uuid =
> +	UUID_LE(0xdaffd814, 0x6eba, 0x4d8c,
> +		0x8a, 0x91, 0xbc, 0x9b, 0xbf, 0x4a, 0xa3, 0x01);
>  /* ACPI _DSD data subnodes UUID: dbb8e3e6-5886-4ba6-8795-1319f52a966b */
> -static const u8 ads_uuid[16] = {
> -	0xe6, 0xe3, 0xb8, 0xdb, 0x86, 0x58, 0xa6, 0x4b,
> -	0x87, 0x95, 0x13, 0x19, 0xf5, 0x2a, 0x96, 0x6b
> -};
> +static const uuid_le ads_uuid =
> +	UUID_LE(0xdbb8e3e6, 0x5886, 0x4ba6,
> +		0x87, 0x95, 0x13, 0x19, 0xf5, 0x2a, 0x96, 0x6b);

This looks better than the original but it is still not too readable.

I think it would be better to have uuid as string like:

static const char *ads_uuid = "dbb8e3e6-5886-4ba6-8795-1319f52a966b";

and then convert that to the right byte presentation when first time
used. That would match the uuid format in the ACPI (and device
properties) spec. and thus make it easier to read and understand.
Andy Shevchenko Feb. 26, 2016, 2:11 p.m. UTC | #3
On Thu, 2016-02-18 at 01:03 +0100, Rafael J. Wysocki wrote:
> On Wednesday, February 17, 2016 02:17:24 PM Andy Shevchenko wrote:
> > Switch to use a generic UUID API instead of custom approach. It
> > allows to
> > define UUIDs, compare them, and validate.

[]

> > +static const uuid_le ads_uuid =
> > +	UUID_LE(0xdbb8e3e6, 0x5886, 0x4ba6,
> > +		0x87, 0x95, 0x13, 0x19, 0xf5, 0x2a, 0x96, 0x6b);
> >  
> >  static bool acpi_enumerate_nondev_subnodes(acpi_handle scope,
> >  					   const union acpi_object
> > *desc,
> > @@ -138,7 +136,7 @@ static bool
> > acpi_enumerate_nondev_subnodes(acpi_handle scope,
> >  		    || links->type != ACPI_TYPE_PACKAGE)
> >  			break;
> >  
> > -		if (memcmp(uuid->buffer.pointer, ads_uuid,
> > sizeof(ads_uuid)))
> > +		if (uuid_le_cmp(*(uuid_le *)uuid->buffer.pointer,
> > ads_uuid))
> 
> Maybe it's too late, but I don't quite understand the pointer
> manipulations here.
> 
> I can see why you need a type conversion (although it looks ugly),
> but why do you
> need to dereference it too?

The function takes that kind of type on input. The other variants are
not compiled.
Perhaps we better change uuid_{lb}e_cmp() first to take normal
pointers, though I think the initial idea was to get type checking at
compile time.
Andy Shevchenko Feb. 26, 2016, 2:28 p.m. UTC | #4
On Thu, 2016-02-18 at 13:07 +0200, Mika Westerberg wrote:
> On Wed, Feb 17, 2016 at 02:17:24PM +0200, Andy Shevchenko wrote:
> > Switch to use a generic UUID API instead of custom approach. It
> > allows to
> > define UUIDs, compare them, and validate.
> > 
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > ---
> >  drivers/acpi/property.c | 18 ++++++++----------
> >  1 file changed, 8 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
> > index 2aee416..1e75305 100644
> > --- a/drivers/acpi/property.c
> > +++ b/drivers/acpi/property.c
> > @@ -25,15 +25,13 @@ static int acpi_data_get_property_array(struct
> > acpi_device_data *data,
> >  					const union acpi_object
> > **obj);
> >  
> >  /* ACPI _DSD device properties UUID: daffd814-6eba-4d8c-8a91-
> > bc9bbf4aa301 */
> > -static const u8 prp_uuid[16] = {
> > -	0x14, 0xd8, 0xff, 0xda, 0xba, 0x6e, 0x8c, 0x4d,
> > -	0x8a, 0x91, 0xbc, 0x9b, 0xbf, 0x4a, 0xa3, 0x01
> > -};
> > +static const uuid_le prp_uuid =
> > +	UUID_LE(0xdaffd814, 0x6eba, 0x4d8c,
> > +		0x8a, 0x91, 0xbc, 0x9b, 0xbf, 0x4a, 0xa3, 0x01);
> >  /* ACPI _DSD data subnodes UUID: dbb8e3e6-5886-4ba6-8795-
> > 1319f52a966b */
> > -static const u8 ads_uuid[16] = {
> > -	0xe6, 0xe3, 0xb8, 0xdb, 0x86, 0x58, 0xa6, 0x4b,
> > -	0x87, 0x95, 0x13, 0x19, 0xf5, 0x2a, 0x96, 0x6b
> > -};
> > +static const uuid_le ads_uuid =
> > +	UUID_LE(0xdbb8e3e6, 0x5886, 0x4ba6,
> > +		0x87, 0x95, 0x13, 0x19, 0xf5, 0x2a, 0x96, 0x6b);
> 
> This looks better than the original but it is still not too readable.
> 
> I think it would be better to have uuid as string like:
> 
> static const char *ads_uuid = "dbb8e3e6-5886-4ba6-8795-1319f52a966b";
> 
> and then convert that to the right byte presentation when first time
> used. That would match the uuid format in the ACPI (and device
> properties) spec. and thus make it easier to read and understand.

Yes we can do this on slow paths. Or in cases where we know that
conversion happens only once.
Andy Shevchenko April 7, 2016, 4:41 p.m. UTC | #5
On Fri, 2016-02-26 at 16:11 +0200, Andy Shevchenko wrote:
> On Thu, 2016-02-18 at 01:03 +0100, Rafael J. Wysocki wrote:
> > 
> > On Wednesday, February 17, 2016 02:17:24 PM Andy Shevchenko wrote:
> > > 
> > > Switch to use a generic UUID API instead of custom approach. It
> > > allows to
> > > define UUIDs, compare them, and validate.
> []
> 

Summon initial author of the UUID library.

Summary: the API of comparison functions is rather strange. What the
point to not take pointers directly? (Moreover I hope compiler too
clever not to make a copy of constant arguments there)

I could only imagine the case you are trying to avoid temporary
variables for constants like NULL_UUID.

Issue with this is the ugliness in the users of that, in particularly
present in ACPI (drivers/acpi/apei/ghes.c).

I would like to have more clear interface for that. Perhaps we may add
something like

cmp_p(pointer, non-pointer);
cmp_pp(pointer, pointer);

to not break existing API for now.

It would be useful for many cases in the kernel.

> > 
> > > 
> > > +static const uuid_le ads_uuid =
> > > +	UUID_LE(0xdbb8e3e6, 0x5886, 0x4ba6,
> > > +		0x87, 0x95, 0x13, 0x19, 0xf5, 0x2a, 0x96, 0x6b);
> > >  
> > >  static bool acpi_enumerate_nondev_subnodes(acpi_handle scope,
> > >  					   const union
> > > acpi_object
> > > *desc,
> > > @@ -138,7 +136,7 @@ static bool
> > > acpi_enumerate_nondev_subnodes(acpi_handle scope,
> > >  		    || links->type != ACPI_TYPE_PACKAGE)
> > >  			break;
> > >  
> > > -		if (memcmp(uuid->buffer.pointer, ads_uuid,
> > > sizeof(ads_uuid)))
> > > +		if (uuid_le_cmp(*(uuid_le *)uuid->buffer.pointer,
> > > ads_uuid))
> > Maybe it's too late, but I don't quite understand the pointer
> > manipulations here.
> > 
> > I can see why you need a type conversion (although it looks ugly),
> > but why do you
> > need to dereference it too?
> The function takes that kind of type on input. The other variants are
> not compiled.
> Perhaps we better change uuid_{lb}e_cmp() first to take normal
> pointers, though I think the initial idea was to get type checking at
> compile time.
>
Huang, Ying April 8, 2016, 1:27 a.m. UTC | #6
Andy Shevchenko <andriy.shevchenko@linux.intel.com> writes:

> On Fri, 2016-02-26 at 16:11 +0200, Andy Shevchenko wrote:
>> On Thu, 2016-02-18 at 01:03 +0100, Rafael J. Wysocki wrote:
>> > 
>> > On Wednesday, February 17, 2016 02:17:24 PM Andy Shevchenko wrote:
>> > > 
>> > > Switch to use a generic UUID API instead of custom approach. It
>> > > allows to
>> > > define UUIDs, compare them, and validate.
>> []
>> 
>
> Summon initial author of the UUID library.
>
> Summary: the API of comparison functions is rather strange. What the
> point to not take pointers directly? (Moreover I hope compiler too
> clever not to make a copy of constant arguments there)
>
> I could only imagine the case you are trying to avoid temporary
> variables for constants like NULL_UUID.
>
> Issue with this is the ugliness in the users of that, in particularly
> present in ACPI (drivers/acpi/apei/ghes.c).
>
> I would like to have more clear interface for that. Perhaps we may add
> something like
>
> cmp_p(pointer, non-pointer);
> cmp_pp(pointer, pointer);
>
> to not break existing API for now.
>
> It would be useful for many cases in the kernel.

You can take a look at the drivers/acpi/apei/erst.c for uuid_le_cmp
usage.

#define CPER_CREATOR_PSTORE                                             \
        UUID_LE(0x75a574e3, 0x5052, 0x4b29, 0x8a, 0x8e, 0xbe, 0x2c,     \
                0x64, 0x90, 0xb8, 0x9d)

        if (uuid_le_cmp(rcd->hdr.creator_id, CPER_CREATOR_PSTORE) != 0)
                goto skip;

Looks better?

This is the typical use case in mind when I write the uuid.h.

As for uuid_le_cmp usage in drivers/acpi/apei/ghes.c,

		if (!uuid_le_cmp(*(uuid_le *)gdata->section_type,
				 CPER_SEC_PLATFORM_MEM)) {

The code looks not good mainly because acpi_hest_generic_data is not
defined with uuid_le in mind.

struct acpi_hest_generic_data {
	u8 section_type[16];
	u32 error_severity;
	u16 revision;
	u8 validation_bits;
	u8 flags;
	u32 error_data_length;
	u8 fru_id[16];
	u8 fru_text[20];
};

If section_type was defined as uuid_le instead of u8[16], the
uuid_le_cmp usage would look better.  So I suggest to use uuid_le/be in
data structure definition in new code if possible.

Best Regards,
Huang, Ying

>> > 
>> > > 
>> > > +static const uuid_le ads_uuid =
>> > > +	UUID_LE(0xdbb8e3e6, 0x5886, 0x4ba6,
>> > > +		0x87, 0x95, 0x13, 0x19, 0xf5, 0x2a, 0x96, 0x6b);
>> > >  
>> > >  static bool acpi_enumerate_nondev_subnodes(acpi_handle scope,
>> > >  					   const union
>> > > acpi_object
>> > > *desc,
>> > > @@ -138,7 +136,7 @@ static bool
>> > > acpi_enumerate_nondev_subnodes(acpi_handle scope,
>> > >  		    || links->type != ACPI_TYPE_PACKAGE)
>> > >  			break;
>> > >  
>> > > -		if (memcmp(uuid->buffer.pointer, ads_uuid,
>> > > sizeof(ads_uuid)))
>> > > +		if (uuid_le_cmp(*(uuid_le *)uuid->buffer.pointer,
>> > > ads_uuid))
>> > Maybe it's too late, but I don't quite understand the pointer
>> > manipulations here.
>> > 
>> > I can see why you need a type conversion (although it looks ugly),
>> > but why do you
>> > need to dereference it too?
>> The function takes that kind of type on input. The other variants are
>> not compiled.
>> Perhaps we better change uuid_{lb}e_cmp() first to take normal
>> pointers, though I think the initial idea was to get type checking at
>> compile time.
>>
Andy Shevchenko April 8, 2016, 10 a.m. UTC | #7
On Fri, 2016-04-08 at 09:27 +0800, Huang, Ying wrote:
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> writes:
> 
> > 
> > On Fri, 2016-02-26 at 16:11 +0200, Andy Shevchenko wrote:
> > > 
> > > On Thu, 2016-02-18 at 01:03 +0100, Rafael J. Wysocki wrote:
> > > > 
> > > > 
> > > > On Wednesday, February 17, 2016 02:17:24 PM Andy Shevchenko
> > > > wrote:
> > > > > 
> > > > > 
> > > > > Switch to use a generic UUID API instead of custom approach.
> > > > > It
> > > > > allows to
> > > > > define UUIDs, compare them, and validate.
> > > []
> > > 
> > Summon initial author of the UUID library.
> > 
> > Summary: the API of comparison functions is rather strange. What the
> > point to not take pointers directly? (Moreover I hope compiler too
> > clever not to make a copy of constant arguments there)
> > 
> > I could only imagine the case you are trying to avoid temporary
> > variables for constants like NULL_UUID.
> > 
> > Issue with this is the ugliness in the users of that, in
> > particularly
> > present in ACPI (drivers/acpi/apei/ghes.c).
> > 
> > I would like to have more clear interface for that. Perhaps we may
> > add
> > something like
> > 
> > cmp_p(pointer, non-pointer);
> > cmp_pp(pointer, pointer);
> > 
> > to not break existing API for now.
> > 
> > It would be useful for many cases in the kernel.
> You can take a look at the drivers/acpi/apei/erst.c for uuid_le_cmp
> usage.
> 
> #define
> CPER_CREATOR_PSTORE                                             \
>         UUID_LE(0x75a574e3, 0x5052, 0x4b29, 0x8a, 0x8e, 0xbe,
> 0x2c,     \
>                 0x64, 0x90, 0xb8, 0x9d)
> 
>         if (uuid_le_cmp(rcd->hdr.creator_id, CPER_CREATOR_PSTORE) !=
> 0)
>                 goto skip;
> 
> Looks better?

I don't quite understand the issues with 

if (uuid_le_cmp(&rcd->hdr.creator_id, &CPER_CREATOR_PSTORE) != 0)

or, like I mentioned previously, we may introduce _cmp_p() and use like

if (uuid_le_cmp_p(&rcd->hdr.creator_id, CPER_CREATOR_PSTORE) != 0)

if it looks better (again, I don't know if compiler is going to copy the last argument).

> 
> This is the typical use case in mind when I write the uuid.h.
> 
> As for uuid_le_cmp usage in drivers/acpi/apei/ghes.c,
> 
> 		if (!uuid_le_cmp(*(uuid_le *)gdata->section_type,
> 				 CPER_SEC_PLATFORM_MEM)) {

Ditto

if (!uuid_le_cmp_p((uuid_le *)gdata->section_type,
CPER_SEC_PLATFORM_MEM)) {

> 
> The code looks not good mainly because acpi_hest_generic_data is not
> defined with uuid_le in mind.
> 
> struct acpi_hest_generic_data {
> 	u8 section_type[16];
> 	u32 error_severity;
> 	u16 revision;
> 	u8 validation_bits;
> 	u8 flags;
> 	u32 error_data_length;
> 	u8 fru_id[16];
> 	u8 fru_text[20];
> };
> 
> If section_type was defined as uuid_le instead of u8[16], the
> uuid_le_cmp usage would look better.  So I suggest to use uuid_le/be
> in
> data structure definition in new code if possible.

This is understandable for such structures, but we might get a UUID from
a buffer which is pointer to u8. It's not possible to convert to uuid_*
since it's too generic stuff and might require to introduce
ACPI_TYPE_UUID with standardization and all necessary work. Apparently
not the shortest way.

> 
> Best Regards,
> Huang, Ying
> 
> > 
> > > 
> > > > 
> > > > 
> > > > > 
> > > > > 
> > > > > +static const uuid_le ads_uuid =
> > > > > +	UUID_LE(0xdbb8e3e6, 0x5886, 0x4ba6,
> > > > > +		0x87, 0x95, 0x13, 0x19, 0xf5, 0x2a, 0x96,
> > > > > 0x6b);
> > > > >  
> > > > >  static bool acpi_enumerate_nondev_subnodes(acpi_handle scope,
> > > > >  					   const union
> > > > > acpi_object
> > > > > *desc,
> > > > > @@ -138,7 +136,7 @@ static bool
> > > > > acpi_enumerate_nondev_subnodes(acpi_handle scope,
> > > > >  		    || links->type != ACPI_TYPE_PACKAGE)
> > > > >  			break;
> > > > >  
> > > > > -		if (memcmp(uuid->buffer.pointer, ads_uuid,
> > > > > sizeof(ads_uuid)))
> > > > > +		if (uuid_le_cmp(*(uuid_le *)uuid-
> > > > > >buffer.pointer,
> > > > > ads_uuid))
> > > > Maybe it's too late, but I don't quite understand the pointer
> > > > manipulations here.
> > > > 
> > > > I can see why you need a type conversion (although it looks
> > > > ugly),
> > > > but why do you
> > > > need to dereference it too?
> > > The function takes that kind of type on input. The other variants
> > > are
> > > not compiled.
> > > Perhaps we better change uuid_{lb}e_cmp() first to take normal
> > > pointers, though I think the initial idea was to get type checking
> > > at
> > > compile time.
> > >
huang ying April 8, 2016, 11:46 p.m. UTC | #8
On Fri, Apr 8, 2016 at 6:00 PM, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> On Fri, 2016-04-08 at 09:27 +0800, Huang, Ying wrote:
>> Andy Shevchenko <andriy.shevchenko@linux.intel.com> writes:
>>
>> >
>> > On Fri, 2016-02-26 at 16:11 +0200, Andy Shevchenko wrote:
>> > >
>> > > On Thu, 2016-02-18 at 01:03 +0100, Rafael J. Wysocki wrote:
>> > > >
>> > > >
>> > > > On Wednesday, February 17, 2016 02:17:24 PM Andy Shevchenko
>> > > > wrote:
>> > > > >
>> > > > >
>> > > > > Switch to use a generic UUID API instead of custom approach.
>> > > > > It
>> > > > > allows to
>> > > > > define UUIDs, compare them, and validate.
>> > > []
>> > >
>> > Summon initial author of the UUID library.
>> >
>> > Summary: the API of comparison functions is rather strange. What the
>> > point to not take pointers directly? (Moreover I hope compiler too
>> > clever not to make a copy of constant arguments there)
>> >
>> > I could only imagine the case you are trying to avoid temporary
>> > variables for constants like NULL_UUID.
>> >
>> > Issue with this is the ugliness in the users of that, in
>> > particularly
>> > present in ACPI (drivers/acpi/apei/ghes.c).
>> >
>> > I would like to have more clear interface for that. Perhaps we may
>> > add
>> > something like
>> >
>> > cmp_p(pointer, non-pointer);
>> > cmp_pp(pointer, pointer);
>> >
>> > to not break existing API for now.
>> >
>> > It would be useful for many cases in the kernel.
>> You can take a look at the drivers/acpi/apei/erst.c for uuid_le_cmp
>> usage.
>>
>> #define
>> CPER_CREATOR_PSTORE                                             \
>>         UUID_LE(0x75a574e3, 0x5052, 0x4b29, 0x8a, 0x8e, 0xbe,
>> 0x2c,     \
>>                 0x64, 0x90, 0xb8, 0x9d)
>>
>>         if (uuid_le_cmp(rcd->hdr.creator_id, CPER_CREATOR_PSTORE) !=
>> 0)
>>                 goto skip;
>>
>> Looks better?
>
> I don't quite understand the issues with
>
> if (uuid_le_cmp(&rcd->hdr.creator_id, &CPER_CREATOR_PSTORE) != 0)

I tried to make uuid_le looks like a primitive data type and UUID
constant looks like primitive type constants if possible.  If we can
define data as uuid_le/be, then it will look just like that.  But if
there are too many places we cannot use uuid_le/be directly, I am OK
to convert the interface to use pointer instead.

> or, like I mentioned previously, we may introduce _cmp_p() and use like
>
> if (uuid_le_cmp_p(&rcd->hdr.creator_id, CPER_CREATOR_PSTORE) != 0)

Personally, I don't like this interface. It is better for two
parameters to have same data type.

> if it looks better (again, I don't know if compiler is going to copy the last argument).
>
>>
>> This is the typical use case in mind when I write the uuid.h.
>>
>> As for uuid_le_cmp usage in drivers/acpi/apei/ghes.c,
>>
>>               if (!uuid_le_cmp(*(uuid_le *)gdata->section_type,
>>                                CPER_SEC_PLATFORM_MEM)) {
>
> Ditto
>
> if (!uuid_le_cmp_p((uuid_le *)gdata->section_type,
> CPER_SEC_PLATFORM_MEM)) {
>
>>
>> The code looks not good mainly because acpi_hest_generic_data is not
>> defined with uuid_le in mind.
>>
>> struct acpi_hest_generic_data {
>>       u8 section_type[16];
>>       u32 error_severity;
>>       u16 revision;
>>       u8 validation_bits;
>>       u8 flags;
>>       u32 error_data_length;
>>       u8 fru_id[16];
>>       u8 fru_text[20];
>> };
>>
>> If section_type was defined as uuid_le instead of u8[16], the
>> uuid_le_cmp usage would look better.  So I suggest to use uuid_le/be
>> in
>> data structure definition in new code if possible.
>
> This is understandable for such structures, but we might get a UUID from
> a buffer which is pointer to u8. It's not possible to convert to uuid_*
> since it's too generic stuff and might require to introduce
> ACPI_TYPE_UUID with standardization and all necessary work. Apparently
> not the shortest way.

If this is just a special case that happens seldom, we can just work
around it with *(uuid_le/be *)buf.  If it is common, we can change the
interface or add a new interface.

Best Regards,
Huang, YIng

>> >
>> > >
>> > > >
>> > > >
>> > > > >
>> > > > >
>> > > > > +static const uuid_le ads_uuid =
>> > > > > +     UUID_LE(0xdbb8e3e6, 0x5886, 0x4ba6,
>> > > > > +             0x87, 0x95, 0x13, 0x19, 0xf5, 0x2a, 0x96,
>> > > > > 0x6b);
>> > > > >
>> > > > >  static bool acpi_enumerate_nondev_subnodes(acpi_handle scope,
>> > > > >                                          const union
>> > > > > acpi_object
>> > > > > *desc,
>> > > > > @@ -138,7 +136,7 @@ static bool
>> > > > > acpi_enumerate_nondev_subnodes(acpi_handle scope,
>> > > > >                   || links->type != ACPI_TYPE_PACKAGE)
>> > > > >                       break;
>> > > > >
>> > > > > -             if (memcmp(uuid->buffer.pointer, ads_uuid,
>> > > > > sizeof(ads_uuid)))
>> > > > > +             if (uuid_le_cmp(*(uuid_le *)uuid-
>> > > > > >buffer.pointer,
>> > > > > ads_uuid))
>> > > > Maybe it's too late, but I don't quite understand the pointer
>> > > > manipulations here.
>> > > >
>> > > > I can see why you need a type conversion (although it looks
>> > > > ugly),
>> > > > but why do you
>> > > > need to dereference it too?
>> > > The function takes that kind of type on input. The other variants
>> > > are
>> > > not compiled.
>> > > Perhaps we better change uuid_{lb}e_cmp() first to take normal
>> > > pointers, though I think the initial idea was to get type checking
>> > > at
>> > > compile time.
>> > >
>
> --
> Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Intel Finland Oy
>
diff mbox

Patch

diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
index 2aee416..1e75305 100644
--- a/drivers/acpi/property.c
+++ b/drivers/acpi/property.c
@@ -25,15 +25,13 @@  static int acpi_data_get_property_array(struct acpi_device_data *data,
 					const union acpi_object **obj);
 
 /* ACPI _DSD device properties UUID: daffd814-6eba-4d8c-8a91-bc9bbf4aa301 */
-static const u8 prp_uuid[16] = {
-	0x14, 0xd8, 0xff, 0xda, 0xba, 0x6e, 0x8c, 0x4d,
-	0x8a, 0x91, 0xbc, 0x9b, 0xbf, 0x4a, 0xa3, 0x01
-};
+static const uuid_le prp_uuid =
+	UUID_LE(0xdaffd814, 0x6eba, 0x4d8c,
+		0x8a, 0x91, 0xbc, 0x9b, 0xbf, 0x4a, 0xa3, 0x01);
 /* ACPI _DSD data subnodes UUID: dbb8e3e6-5886-4ba6-8795-1319f52a966b */
-static const u8 ads_uuid[16] = {
-	0xe6, 0xe3, 0xb8, 0xdb, 0x86, 0x58, 0xa6, 0x4b,
-	0x87, 0x95, 0x13, 0x19, 0xf5, 0x2a, 0x96, 0x6b
-};
+static const uuid_le ads_uuid =
+	UUID_LE(0xdbb8e3e6, 0x5886, 0x4ba6,
+		0x87, 0x95, 0x13, 0x19, 0xf5, 0x2a, 0x96, 0x6b);
 
 static bool acpi_enumerate_nondev_subnodes(acpi_handle scope,
 					   const union acpi_object *desc,
@@ -138,7 +136,7 @@  static bool acpi_enumerate_nondev_subnodes(acpi_handle scope,
 		    || links->type != ACPI_TYPE_PACKAGE)
 			break;
 
-		if (memcmp(uuid->buffer.pointer, ads_uuid, sizeof(ads_uuid)))
+		if (uuid_le_cmp(*(uuid_le *)uuid->buffer.pointer, ads_uuid))
 			continue;
 
 		return acpi_add_nondev_subnodes(scope, links, &data->subnodes);
@@ -245,7 +243,7 @@  static bool acpi_extract_properties(const union acpi_object *desc,
 		    || properties->type != ACPI_TYPE_PACKAGE)
 			break;
 
-		if (memcmp(uuid->buffer.pointer, prp_uuid, sizeof(prp_uuid)))
+		if (uuid_le_cmp(*(uuid_le *)uuid->buffer.pointer, prp_uuid))
 			continue;
 
 		/*