Nouveau - May 2017 - [PATCH v1] ACPI: Switch to use generic UUID API

On Thu, May 4, 2017 at 2:21 AM, Andy Shevchenko
<andriy.shevchenko at linux.intel.com> 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 at rjwysocki.net>
> Cc: Mika Westerberg <mika.westerberg at linux.intel.com>
> Cc: Borislav Petkov <bp at suse.de>
> Cc: Dan Williams <dan.j.williams at intel.com>
> Cc: Amir Goldstein <amir73il at gmail.com>
> Cc: Jarkko Sakkinen <jarkko.sakkinen at linux.intel.com>
> Cc: Jani Nikula <jani.nikula at linux.intel.com>
> Cc: Ben Skeggs <bskeggs at redhat.com>
> Cc: Benjamin Tissoires <benjamin.tissoires at redhat.com>
> Cc: Joerg Roedel <joro at 8bytes.org>
> Cc: Adrian Hunter <adrian.hunter at intel.com>
> Cc: Yisen Zhuang <yisen.zhuang at huawei.com>
> Cc: Bjorn Helgaas <bhelgaas at google.com>
> Cc: Zhang Rui <rui.zhang at intel.com>
> Cc: Felipe Balbi <balbi at kernel.org>
> Cc: Mathias Nyman <mathias.nyman at intel.com>
> Cc: Heikki Krogerus <heikki.krogerus at linux.intel.com>
> Cc: Liam Girdwood <lgirdwood at gmail.com>
> Cc: Mark Brown <broonie at kernel.org>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko at linux.intel.com>
[..]
> 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?

Other than that, looks ok to me.

On Fri, May 05, 2017 at 10:06:06AM +0300, Amir Goldstein
wrote:
> 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).
Yeah.
> 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.
It's not jut neutral, it's the right thing to to.  The Apollo / DCE
uuids (later also specified in RFC 4122) are big endian, so we should
use the name there.
> Christoph also suggested a similar treatment to typedef guid_t to
> struct uuid_le.
Exactly.  The whole idea of "little endian UUIDs" comes from the
Wintel world, and if you look at the relevant specs they are almost
exclusively referred to as GUIDs.

The magic XFS and AFS types for specific interpretations of one of
the RFC4122 formats don't really help, but I'll just send a patch
to kill them off for XFS ASAP to at least get that out, and we
probably should revert at least

"afs: Move UUID struct to linux/uuid.h"

That moved the AFS mess to common code as a start, and then
also clean up the way we generate random UUIDs, where we currently
have le helper, a be helper and then also generate_random_uuid
just to confuse the heck out of people.  With no description of
either of them.

On Fri, 2017-05-05 at 10:06 +0300, Amir Goldstein wrote:
> On Fri, May 5, 2017 at 9:20 AM, Dan Williams <dan.j.williams at
intel.com
> > wrote:
> > On Thu, May 4, 2017 at 2:21 AM, Andy Shevchenko
> > <andriy.shevchenko at linux.intel.com> wrote:
> > >         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?
Dan, it's a typo. In this patch it should be just ..._cmp(), which is
already a part of API.
> > 
> 
> I second that.
> 
> Andy,
Amir, just to be clear. This patch can be applied without any addons to
an existing API. Above is just a typo due to rebase in my tree. I will
replace it to just uuid_le_cmp().
> 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.
We may go this way. But I wouldn't prevent current users of uuid_le to
continue using it without conversion (it may be done case by case after
we settle an API)

So, summarize what Christoph said it will look like

typedef uuid_be uuid_t;
typedef uuid_le guid_t

uuid_cmp() / uuid_copy() / uuid_to_bin() / etc
guid_cmp() / guid_copy() / guid_to_bin() / etc

Correct? Christoph?

-- 
Andy Shevchenko <andriy.shevchenko at linux.intel.com>
Intel Finland Oy