[v2,2/6] ACPI: bus: update acpi_dev_uid_match() to support multiple types

  According to ACPI specification, a _UID object can evaluate to either
a numeric value or a string. Update acpi_dev_uid_match() helper to
support _UID matching for both integer and string types.

Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Suggested-by: Mika Westerberg <mika.westerberg@linux.intel.com>
Suggested-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Signed-off-by: Raag Jadav <raag.jadav@intel.com>
---
 drivers/acpi/utils.c    | 19 -------------------
 include/acpi/acpi_bus.h | 35 ++++++++++++++++++++++++++++++++++-
 include/linux/acpi.h    |  8 +++-----
 3 files changed, 37 insertions(+), 25 deletions(-)
  

On Tue, Nov 21, 2023 at 04:08:25PM +0530, Raag Jadav wrote:
> According to ACPI specification, a _UID object can evaluate to either
> a numeric value or a string. Update acpi_dev_uid_match() helper to
> support _UID matching for both integer and string types.
> 
> Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Suggested-by: Mika Westerberg <mika.westerberg@linux.intel.com>

Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>
  

On Tue, Nov 21, 2023 at 11:38 AM Raag Jadav <raag.jadav@intel.com> wrote:
>
> According to ACPI specification, a _UID object can evaluate to either
> a numeric value or a string. Update acpi_dev_uid_match() helper to
> support _UID matching for both integer and string types.
>
> Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Suggested-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> Suggested-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

You need to be careful with using this.  There are some things below
that go beyond what I have suggested.

> Signed-off-by: Raag Jadav <raag.jadav@intel.com>
> ---
>  drivers/acpi/utils.c    | 19 -------------------
>  include/acpi/acpi_bus.h | 35 ++++++++++++++++++++++++++++++++++-
>  include/linux/acpi.h    |  8 +++-----
>  3 files changed, 37 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/acpi/utils.c b/drivers/acpi/utils.c
> index 28c75242fca9..fe7e850c6479 100644
> --- a/drivers/acpi/utils.c
> +++ b/drivers/acpi/utils.c
> @@ -824,25 +824,6 @@ bool acpi_check_dsm(acpi_handle handle, const guid_t *guid, u64 rev, u64 funcs)
>  }
>  EXPORT_SYMBOL(acpi_check_dsm);
>
> -/**
> - * acpi_dev_uid_match - Match device by supplied UID
> - * @adev: ACPI device to match.
> - * @uid2: Unique ID of the device.
> - *
> - * Matches UID in @adev with given @uid2.
> - *
> - * Returns:
> - *  - %true if matches.
> - *  - %false otherwise.
> - */
> -bool acpi_dev_uid_match(struct acpi_device *adev, const char *uid2)
> -{
> -       const char *uid1 = acpi_device_uid(adev);
> -
> -       return uid1 && uid2 && !strcmp(uid1, uid2);
> -}
> -EXPORT_SYMBOL_GPL(acpi_dev_uid_match);
> -
>  /**
>   * acpi_dev_hid_uid_match - Match device by supplied HID and UID
>   * @adev: ACPI device to match.
> diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
> index ec6a673dcb95..bcd78939bab4 100644
> --- a/include/acpi/acpi_bus.h
> +++ b/include/acpi/acpi_bus.h
> @@ -9,6 +9,7 @@
>  #ifndef __ACPI_BUS_H__
>  #define __ACPI_BUS_H__
>
> +#include <linux/compiler.h>
>  #include <linux/device.h>
>  #include <linux/property.h>
>
> @@ -857,10 +858,42 @@ static inline bool acpi_device_can_poweroff(struct acpi_device *adev)
>                 adev->power.states[ACPI_STATE_D3_HOT].flags.explicit_set);
>  }
>
> -bool acpi_dev_uid_match(struct acpi_device *adev, const char *uid2);
>  bool acpi_dev_hid_uid_match(struct acpi_device *adev, const char *hid2, const char *uid2);
>  int acpi_dev_uid_to_integer(struct acpi_device *adev, u64 *integer);
>
> +static inline bool acpi_str_uid_match(struct acpi_device *adev, const char *uid2)
> +{
> +       const char *uid1 = acpi_device_uid(adev);
> +
> +       return uid1 && uid2 && !strcmp(uid1, uid2);
> +}
> +
> +static inline bool acpi_int_uid_match(struct acpi_device *adev, u64 uid2)
> +{
> +       u64 uid1;
> +
> +       return !acpi_dev_uid_to_integer(adev, &uid1) && uid1 == uid2;
> +}
> +

Up to this point it is all fine IMV.

> +/**
> + * acpi_dev_uid_match - Match device by supplied UID
> + * @adev: ACPI device to match.
> + * @uid2: Unique ID of the device.
> + *
> + * Matches UID in @adev with given @uid2.
> + *
> + * Returns: %true if matches, %false otherwise.
> + */
> +
> +/* Treat uid as a string for array and pointer types, treat as an integer otherwise */
> +#define get_uid_type(x) \
> +       (__builtin_choose_expr(is_array_or_pointer_type(x), (const char *)0, (u64)0))

But I wouldn't use the above.

It is far more elaborate than needed for this use case and may not
actually work as expected.  For instance, why would a pointer to a
random struct type be a good candidate for a string?

> +
> +#define acpi_dev_uid_match(adev, uid2)                         \
> +       _Generic(get_uid_type(uid2),                            \
> +                const char *: acpi_str_uid_match,              \
> +                u64: acpi_int_uid_match)(adev, uid2)
> +

Personally, I would just do something like the following

#define acpi_dev_uid_match(adev, uid2) \
        _Generic((uid2), \
                const char *: acpi_str_uid_match, \
                char *: acpi_str_uid_match, \
                const void *: acpi_str_uid_match, \
                void *: acpi_str_uid_match, \
                default: acpi_int_uid_match)(adev, uid2)

which doesn't require compiler.h to be fiddled with and is rather
straightforward to follow.

If I'm to apply the patches, this is about the level of complexity you
need to target.

>  void acpi_dev_clear_dependencies(struct acpi_device *supplier);
>  bool acpi_dev_ready_for_enumeration(const struct acpi_device *device);
>  struct acpi_device *acpi_dev_get_next_consumer_dev(struct acpi_device *supplier,
> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
> index b63d7811c728..aae3a459d63c 100644
> --- a/include/linux/acpi.h
> +++ b/include/linux/acpi.h
> @@ -763,6 +763,9 @@ const char *acpi_get_subsystem_id(acpi_handle handle);
>  #define ACPI_HANDLE(dev)               (NULL)
>  #define ACPI_HANDLE_FWNODE(fwnode)     (NULL)
>
> +/* Get rid of the -Wunused-variable for adev */
> +#define acpi_dev_uid_match(adev, uid2)                 (adev && false)
> +
>  #include <acpi/acpi_numa.h>
>
>  struct fwnode_handle;
> @@ -779,11 +782,6 @@ static inline bool acpi_dev_present(const char *hid, const char *uid, s64 hrv)
>
>  struct acpi_device;
>
> -static inline bool acpi_dev_uid_match(struct acpi_device *adev, const char *uid2)
> -{
> -       return false;
> -}
> -
>  static inline bool
>  acpi_dev_hid_uid_match(struct acpi_device *adev, const char *hid2, const char *uid2)
>  {
> --
  

On Tue, Nov 21, 2023 at 08:25:20PM +0100, Rafael J. Wysocki wrote:
> On Tue, Nov 21, 2023 at 11:38 AM Raag Jadav <raag.jadav@intel.com> wrote:
> >
> > According to ACPI specification, a _UID object can evaluate to either
> > a numeric value or a string. Update acpi_dev_uid_match() helper to
> > support _UID matching for both integer and string types.
> >
> > Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > Suggested-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> > Suggested-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> You need to be careful with using this.  There are some things below
> that go beyond what I have suggested.

I think we all suggested some bits and pieces so I included everyone.
We can drop if there are any objections.

> > Signed-off-by: Raag Jadav <raag.jadav@intel.com>
> > ---
> >  drivers/acpi/utils.c    | 19 -------------------
> >  include/acpi/acpi_bus.h | 35 ++++++++++++++++++++++++++++++++++-
> >  include/linux/acpi.h    |  8 +++-----
> >  3 files changed, 37 insertions(+), 25 deletions(-)
> >
> > diff --git a/drivers/acpi/utils.c b/drivers/acpi/utils.c
> > index 28c75242fca9..fe7e850c6479 100644
> > --- a/drivers/acpi/utils.c
> > +++ b/drivers/acpi/utils.c
> > @@ -824,25 +824,6 @@ bool acpi_check_dsm(acpi_handle handle, const guid_t *guid, u64 rev, u64 funcs)
> >  }
> >  EXPORT_SYMBOL(acpi_check_dsm);
> >
> > -/**
> > - * acpi_dev_uid_match - Match device by supplied UID
> > - * @adev: ACPI device to match.
> > - * @uid2: Unique ID of the device.
> > - *
> > - * Matches UID in @adev with given @uid2.
> > - *
> > - * Returns:
> > - *  - %true if matches.
> > - *  - %false otherwise.
> > - */
> > -bool acpi_dev_uid_match(struct acpi_device *adev, const char *uid2)
> > -{
> > -       const char *uid1 = acpi_device_uid(adev);
> > -
> > -       return uid1 && uid2 && !strcmp(uid1, uid2);
> > -}
> > -EXPORT_SYMBOL_GPL(acpi_dev_uid_match);
> > -
> >  /**
> >   * acpi_dev_hid_uid_match - Match device by supplied HID and UID
> >   * @adev: ACPI device to match.
> > diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
> > index ec6a673dcb95..bcd78939bab4 100644
> > --- a/include/acpi/acpi_bus.h
> > +++ b/include/acpi/acpi_bus.h
> > @@ -9,6 +9,7 @@
> >  #ifndef __ACPI_BUS_H__
> >  #define __ACPI_BUS_H__
> >
> > +#include <linux/compiler.h>
> >  #include <linux/device.h>
> >  #include <linux/property.h>
> >
> > @@ -857,10 +858,42 @@ static inline bool acpi_device_can_poweroff(struct acpi_device *adev)
> >                 adev->power.states[ACPI_STATE_D3_HOT].flags.explicit_set);
> >  }
> >
> > -bool acpi_dev_uid_match(struct acpi_device *adev, const char *uid2);
> >  bool acpi_dev_hid_uid_match(struct acpi_device *adev, const char *hid2, const char *uid2);
> >  int acpi_dev_uid_to_integer(struct acpi_device *adev, u64 *integer);
> >
> > +static inline bool acpi_str_uid_match(struct acpi_device *adev, const char *uid2)
> > +{
> > +       const char *uid1 = acpi_device_uid(adev);
> > +
> > +       return uid1 && uid2 && !strcmp(uid1, uid2);
> > +}
> > +
> > +static inline bool acpi_int_uid_match(struct acpi_device *adev, u64 uid2)
> > +{
> > +       u64 uid1;
> > +
> > +       return !acpi_dev_uid_to_integer(adev, &uid1) && uid1 == uid2;
> > +}
> > +
> 
> Up to this point it is all fine IMV.
> 
> > +/**
> > + * acpi_dev_uid_match - Match device by supplied UID
> > + * @adev: ACPI device to match.
> > + * @uid2: Unique ID of the device.
> > + *
> > + * Matches UID in @adev with given @uid2.
> > + *
> > + * Returns: %true if matches, %false otherwise.
> > + */
> > +
> > +/* Treat uid as a string for array and pointer types, treat as an integer otherwise */
> > +#define get_uid_type(x) \
> > +       (__builtin_choose_expr(is_array_or_pointer_type(x), (const char *)0, (u64)0))
> 
> But I wouldn't use the above.
> 
> It is far more elaborate than needed for this use case and may not
> actually work as expected.  For instance, why would a pointer to a
> random struct type be a good candidate for a string?

Such case will not compile, since its data type will not match with
acpi_str_uid_match() prototype. The compiler does a very good job of
qualifing only the compatible string types here, which is exactly what
we want.

error: passing argument 2 of 'acpi_str_uid_match' from incompatible pointer type [-Werror=incompatible-pointer-types]
    if (acpi_dev_uid_match(adev, adev)) {
                                 ^
./include/acpi/acpi_bus.h:870:20: note: expected 'const char *' but argument is of type 'struct acpi_device *'
 static inline bool acpi_str_uid_match(struct acpi_device *adev, const char *uid2)

> > +
> > +#define acpi_dev_uid_match(adev, uid2)                         \
> > +       _Generic(get_uid_type(uid2),                            \
> > +                const char *: acpi_str_uid_match,              \
> > +                u64: acpi_int_uid_match)(adev, uid2)
> > +
> 
> Personally, I would just do something like the following
> 
> #define acpi_dev_uid_match(adev, uid2) \
>         _Generic((uid2), \
>                 const char *: acpi_str_uid_match, \
>                 char *: acpi_str_uid_match, \
>                 const void *: acpi_str_uid_match, \
>                 void *: acpi_str_uid_match, \
>                 default: acpi_int_uid_match)(adev, uid2)
> 
> which doesn't require compiler.h to be fiddled with and is rather
> straightforward to follow.
> 
> If I'm to apply the patches, this is about the level of complexity you
> need to target.

Understood, however this will limit the type support to only a handful
of types and will not satisfy a few of the existing users, which, for
example are passing signed or unsigned pointer or an array of u8.

Listing every possible type manually for _Generic() looks a bit verbose
for something that can be simply achieved by __builtin functions in my
opinion.

I can still send out a v3 to see if it really works. However, I prefer the
v2 approach, as it covers all possible scenarios without any corner cases.

Raag
  

On Tue, Nov 21, 2023 at 08:25:20PM +0100, Rafael J. Wysocki wrote:
> On Tue, Nov 21, 2023 at 11:38 AM Raag Jadav <raag.jadav@intel.com> wrote:
> >
> > According to ACPI specification, a _UID object can evaluate to either
> > a numeric value or a string. Update acpi_dev_uid_match() helper to
> > support _UID matching for both integer and string types.
> >
> > Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > Suggested-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> > Suggested-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> You need to be careful with using this.  There are some things below
> that go beyond what I have suggested.

Same to me and actually I've asked several times to remove this tag of mine!
  

On Wed, Nov 22, 2023 at 5:58 AM Raag Jadav <raag.jadav@intel.com> wrote:
>
> On Tue, Nov 21, 2023 at 08:25:20PM +0100, Rafael J. Wysocki wrote:
> > On Tue, Nov 21, 2023 at 11:38 AM Raag Jadav <raag.jadav@intel.com> wrote:
> > >
> > > According to ACPI specification, a _UID object can evaluate to either
> > > a numeric value or a string. Update acpi_dev_uid_match() helper to
> > > support _UID matching for both integer and string types.
> > >
> > > Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > > Suggested-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> > > Suggested-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > You need to be careful with using this.  There are some things below
> > that go beyond what I have suggested.
>
> I think we all suggested some bits and pieces so I included everyone.
> We can drop if there are any objections.

There are, from me and from Andy.

[cut]

> > Up to this point it is all fine IMV.
> >
> > > +/**
> > > + * acpi_dev_uid_match - Match device by supplied UID
> > > + * @adev: ACPI device to match.
> > > + * @uid2: Unique ID of the device.
> > > + *
> > > + * Matches UID in @adev with given @uid2.
> > > + *
> > > + * Returns: %true if matches, %false otherwise.
> > > + */
> > > +
> > > +/* Treat uid as a string for array and pointer types, treat as an integer otherwise */
> > > +#define get_uid_type(x) \
> > > +       (__builtin_choose_expr(is_array_or_pointer_type(x), (const char *)0, (u64)0))
> >
> > But I wouldn't use the above.
> >
> > It is far more elaborate than needed for this use case and may not
> > actually work as expected.  For instance, why would a pointer to a
> > random struct type be a good candidate for a string?
>
> Such case will not compile, since its data type will not match with
> acpi_str_uid_match() prototype. The compiler does a very good job of
> qualifing only the compatible string types here, which is exactly what
> we want.
>
> error: passing argument 2 of 'acpi_str_uid_match' from incompatible pointer type [-Werror=incompatible-pointer-types]
>     if (acpi_dev_uid_match(adev, adev)) {
>                                  ^
> ./include/acpi/acpi_bus.h:870:20: note: expected 'const char *' but argument is of type 'struct acpi_device *'
>  static inline bool acpi_str_uid_match(struct acpi_device *adev, const char *uid2)

You are right, it won't compile, but that's not my point.  Why would
it be matched with acpi_str_uid_match() in the first place?

> > > +
> > > +#define acpi_dev_uid_match(adev, uid2)                         \
> > > +       _Generic(get_uid_type(uid2),                            \
> > > +                const char *: acpi_str_uid_match,              \
> > > +                u64: acpi_int_uid_match)(adev, uid2)
> > > +
> >
> > Personally, I would just do something like the following
> >
> > #define acpi_dev_uid_match(adev, uid2) \
> >         _Generic((uid2), \
> >                 const char *: acpi_str_uid_match, \
> >                 char *: acpi_str_uid_match, \
> >                 const void *: acpi_str_uid_match, \
> >                 void *: acpi_str_uid_match, \
> >                 default: acpi_int_uid_match)(adev, uid2)
> >
> > which doesn't require compiler.h to be fiddled with and is rather
> > straightforward to follow.
> >
> > If I'm to apply the patches, this is about the level of complexity you
> > need to target.
>
> Understood, however this will limit the type support to only a handful
> of types,

Indeed.

> and will not satisfy a few of the existing users, which, for
> example are passing signed or unsigned pointer or an array of u8.

Fair enough, so those types would need to be added to the list.

> Listing every possible type manually for _Generic() looks a bit verbose
> for something that can be simply achieved by __builtin functions in my
> opinion.

But then you don't even need _Generic(), do you?

Wouldn't something like the below work?

#define acpi_dev_uid_match(adev, uid2) \
        (__builtin_choose_expr(is_array_or_pointer_type((uid2)),acpi_str_uid_match(adev,
uid2), acpi_int_uid_match(adev, uid2))

In any case, I'm not particularly convinced about the
is_array_or_pointer_type() thing and so I'm not going to apply the
series as is.