| Message ID | 20231121103829.10027-3-raag.jadav@intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | Support _UID matching for integer types | expand |
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!
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; +} + +/** + * 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)) + +#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) + 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) {
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(-)