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@xxxxxxxxx> 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@xxxxxxxxxxxxxxx>
> > Suggested-by: Mika Westerberg <mika.westerberg@xxxxxxxxxxxxxxx>
> > Suggested-by: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
>
> 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@xxxxxxxxx>
> > ---
> > 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
