Re: [PATCH v1] ACPI: Switch to use generic UUID API

[Amir Goldstein]

On Fri, May 5, 2017 at 9:20 AM, Dan Williams <dan.j.williams@xxxxxxxxx> wrote:
> On Thu, May 4, 2017 at 2:21 AM, Andy Shevchenko
> <andriy.shevchenko@xxxxxxxxxxxxxxx> wrote:
>> acpi_evaluate_dsm() and friends take a pointer to a raw buffer of 16
>> bytes. Instead we convert them to use uuid_le type. At the same time we
>> convert current users.
>>
>> acpi_str_to_uuid() becomes useless after the conversion and it's safe to
>> get rid of it.
>>
>> The conversion fixes a potential bug in int340x_thermal as well since
>> we have to use memcmp() on binary data.
>>
>> Cc: Rafael J. Wysocki <rjw@xxxxxxxxxxxxx>
>> Cc: Mika Westerberg <mika.westerberg@xxxxxxxxxxxxxxx>
>> Cc: Borislav Petkov <bp@xxxxxxx>
>> Cc: Dan Williams <dan.j.williams@xxxxxxxxx>
>> Cc: Amir Goldstein <amir73il@xxxxxxxxx>
>> Cc: Jarkko Sakkinen <jarkko.sakkinen@xxxxxxxxxxxxxxx>
>> Cc: Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx>
>> Cc: Ben Skeggs <bskeggs@xxxxxxxxxx>
>> Cc: Benjamin Tissoires <benjamin.tissoires@xxxxxxxxxx>
>> Cc: Joerg Roedel <joro@xxxxxxxxxx>
>> Cc: Adrian Hunter <adrian.hunter@xxxxxxxxx>
>> Cc: Yisen Zhuang <yisen.zhuang@xxxxxxxxxx>
>> Cc: Bjorn Helgaas <bhelgaas@xxxxxxxxxx>
>> Cc: Zhang Rui <rui.zhang@xxxxxxxxx>
>> Cc: Felipe Balbi <balbi@xxxxxxxxxx>
>> Cc: Mathias Nyman <mathias.nyman@xxxxxxxxx>
>> Cc: Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx>
>> Cc: Liam Girdwood <lgirdwood@xxxxxxxxx>
>> Cc: Mark Brown <broonie@xxxxxxxxxx>
>> Signed-off-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
> [..]
>> diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
>> index 0f7982a1caaf..bd3e45ede056 100644
>> --- a/drivers/acpi/nfit/core.c
>> +++ b/drivers/acpi/nfit/core.c
>> @@ -74,11 +74,11 @@ struct nfit_table_prev {
>>         struct list_head flushes;
>>  };
>>
>> -static u8 nfit_uuid[NFIT_UUID_MAX][16];
>> +static uuid_le nfit_uuid[NFIT_UUID_MAX];
>>
>> -const u8 *to_nfit_uuid(enum nfit_uuids id)
>> +const uuid_le *to_nfit_uuid(enum nfit_uuids id)
>>  {
>> -       return nfit_uuid[id];
>> +       return &nfit_uuid[id];
>>  }
>>  EXPORT_SYMBOL(to_nfit_uuid);
>>
>> @@ -207,7 +207,7 @@ int acpi_nfit_ctl(struct nvdimm_bus_descriptor *nd_desc, struct nvdimm *nvdimm,
>>         u32 offset, fw_status = 0;
>>         acpi_handle handle;
>>         unsigned int func;
>> -       const u8 *uuid;
>> +       const uuid_le *uuid;
>>         int rc, i;
>>
>>         func = cmd;
>> @@ -394,7 +394,7 @@ int nfit_spa_type(struct acpi_nfit_system_address *spa)
>>         int i;
>>
>>         for (i = 0; i < NFIT_UUID_MAX; i++)
>> -               if (memcmp(to_nfit_uuid(i), spa->range_guid, 16) == 0)
>> +               if (!uuid_le_cmp_pp(to_nfit_uuid(i), (uuid_le *)spa->range_guid))
>
> What is _cmp_pp? Why not _compare?
>

I second that.

Andy,

I much rather that you sort out uuid helpers in a way that will
satisfy the filesystem
needs (just provide the helpers don't need to convert filesystems code).

The only reason I took a swing at hoisting the xfs uuid helpers is
because it didn't
seem like your proposal was going to be posted soon or wasn't going to satisfy
the filesystems use case.

My opinion now, is that your suggestion is probably much closer to the real deal
than mine.

IMO, you should acknowledge that the common use case for filesystems is
to handle an opaque char[16] which most likely holds a uuid_be and you
should provide 'neutral' helpers to satisfy this use case.

The simplest would be to typedef uuid_t to struct uuid_be and to name 'neutral'
helpers' uuid_cmp/uuid_copy(uuid_t *, uuid_t *), similar to my proposal.

I think with this semantic change, our proposals can reach common grounds
and satisfy a wider group of users (i.e. filesystem developers).

Christoph also suggested a similar treatment to typedef guid_t to
struct uuid_le.
I don't know the use cases enough to comment on that.

Cheers,
Amir.