| Message ID | 20231121103829.10027-1-raag.jadav@intel.com |
|---|---|
| Headers | show |
| Series | Support _UID matching for integer types | expand |
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 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.