This series updates the standard ACPI helpers to support _UID matching
for both integer and string types, and uses them in a couple of places.
Changes since v1:
- Fix build errors
Raag Jadav (6):
compiler.h: Introduce helpers for identifying array and pointer types
ACPI: bus: update acpi_dev_uid_match() to support multiple types
ACPI: bus: update acpi_dev_hid_uid_match() to support multiple types
ACPI: LPSS: use acpi_dev_uid_match() for matching _UID
efi: dev-path-parser: use acpi_dev_uid_match() for matching _UID
perf: arm_cspmu: drop redundant acpi_dev_uid_to_integer()
drivers/acpi/acpi_lpss.c | 16 ++-----
drivers/acpi/utils.c | 48 ---------------------
drivers/firmware/efi/dev-path-parser.c | 7 +--
drivers/perf/arm_cspmu/arm_cspmu.c | 4 +-
include/acpi/acpi_bus.h | 59 +++++++++++++++++++++++++-
include/linux/acpi.h | 15 ++-----
include/linux/compiler.h | 5 +++
7 files changed, 73 insertions(+), 81 deletions(-)
base-commit: f437a8d1debff5412e36a1c9454adee193b31950
--
2.17.1
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 <[email protected]>
Suggested-by: Mika Westerberg <[email protected]>
Suggested-by: Rafael J. Wysocki <[email protected]>
Signed-off-by: Raag Jadav <[email protected]>
---
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;
+}
+
+/**
+ * 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)
{
--
2.17.1
Now that we have _UID matching support for both integer and string types,
we can support them into acpi_dev_hid_uid_match() helper as well.
Signed-off-by: Raag Jadav <[email protected]>
---
drivers/acpi/utils.c | 29 -----------------------------
include/acpi/acpi_bus.h | 24 +++++++++++++++++++++++-
include/linux/acpi.h | 7 +------
3 files changed, 24 insertions(+), 36 deletions(-)
diff --git a/drivers/acpi/utils.c b/drivers/acpi/utils.c
index fe7e850c6479..03f6de9a0807 100644
--- a/drivers/acpi/utils.c
+++ b/drivers/acpi/utils.c
@@ -824,35 +824,6 @@ bool acpi_check_dsm(acpi_handle handle, const guid_t *guid, u64 rev, u64 funcs)
}
EXPORT_SYMBOL(acpi_check_dsm);
-/**
- * acpi_dev_hid_uid_match - Match device by supplied HID and UID
- * @adev: ACPI device to match.
- * @hid2: Hardware ID of the device.
- * @uid2: Unique ID of the device, pass NULL to not check _UID.
- *
- * Matches HID and UID in @adev with given @hid2 and @uid2. Absence of @uid2
- * will be treated as a match. If user wants to validate @uid2, it should be
- * done before calling this function.
- *
- * Returns:
- * - %true if matches or @uid2 is NULL.
- * - %false otherwise.
- */
-bool acpi_dev_hid_uid_match(struct acpi_device *adev,
- const char *hid2, const char *uid2)
-{
- const char *hid1 = acpi_device_hid(adev);
-
- if (strcmp(hid1, hid2))
- return false;
-
- if (!uid2)
- return true;
-
- return acpi_dev_uid_match(adev, uid2);
-}
-EXPORT_SYMBOL(acpi_dev_hid_uid_match);
-
/**
* acpi_dev_uid_to_integer - treat ACPI device _UID as integer
* @adev: ACPI device to get _UID from
diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
index bcd78939bab4..6f10c3de9488 100644
--- a/include/acpi/acpi_bus.h
+++ b/include/acpi/acpi_bus.h
@@ -858,9 +858,15 @@ static inline bool acpi_device_can_poweroff(struct acpi_device *adev)
adev->power.states[ACPI_STATE_D3_HOT].flags.explicit_set);
}
-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_dev_hid_match(struct acpi_device *adev, const char *hid2)
+{
+ const char *hid1 = acpi_device_hid(adev);
+
+ return hid1 && hid2 && !strcmp(hid1, hid2);
+}
+
static inline bool acpi_str_uid_match(struct acpi_device *adev, const char *uid2)
{
const char *uid1 = acpi_device_uid(adev);
@@ -894,6 +900,22 @@ static inline bool acpi_int_uid_match(struct acpi_device *adev, u64 uid2)
const char *: acpi_str_uid_match, \
u64: acpi_int_uid_match)(adev, uid2)
+/**
+ * acpi_dev_hid_uid_match - Match device by supplied HID and UID
+ * @adev: ACPI device to match.
+ * @hid2: Hardware ID of the device.
+ * @uid2: Unique ID of the device, pass 0 or NULL to not check _UID.
+ *
+ * Matches HID and UID in @adev with given @hid2 and @uid2. Absence of @uid2
+ * will be treated as a match. If user wants to validate @uid2, it should be
+ * done before calling this function.
+ *
+ * Returns: %true if matches or @uid2 is 0 or NULL, %false otherwise.
+ */
+#define acpi_dev_hid_uid_match(adev, hid2, uid2) \
+ (acpi_dev_hid_match(adev, hid2) && \
+ (!(uid2) || acpi_dev_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 aae3a459d63c..fda188ff3dcf 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -765,6 +765,7 @@ const char *acpi_get_subsystem_id(acpi_handle handle);
/* Get rid of the -Wunused-variable for adev */
#define acpi_dev_uid_match(adev, uid2) (adev && false)
+#define acpi_dev_hid_uid_match(adev, hid2, uid2) (adev && false)
#include <acpi/acpi_numa.h>
@@ -782,12 +783,6 @@ static inline bool acpi_dev_present(const char *hid, const char *uid, s64 hrv)
struct acpi_device;
-static inline bool
-acpi_dev_hid_uid_match(struct acpi_device *adev, const char *hid2, const char *uid2)
-{
- return false;
-}
-
static inline int acpi_dev_uid_to_integer(struct acpi_device *adev, u64 *integer)
{
return -ENODEV;
--
2.17.1
Now that we have _UID matching support for integer types, we can use
acpi_dev_uid_match() for it.
Signed-off-by: Raag Jadav <[email protected]>
---
drivers/acpi/acpi_lpss.c | 16 ++++------------
1 file changed, 4 insertions(+), 12 deletions(-)
diff --git a/drivers/acpi/acpi_lpss.c b/drivers/acpi/acpi_lpss.c
index 875de44961bf..79f4fc7d6871 100644
--- a/drivers/acpi/acpi_lpss.c
+++ b/drivers/acpi/acpi_lpss.c
@@ -167,13 +167,9 @@ static struct pwm_lookup byt_pwm_lookup[] = {
static void byt_pwm_setup(struct lpss_private_data *pdata)
{
- u64 uid;
-
/* Only call pwm_add_table for the first PWM controller */
- if (acpi_dev_uid_to_integer(pdata->adev, &uid) || uid != 1)
- return;
-
- pwm_add_table(byt_pwm_lookup, ARRAY_SIZE(byt_pwm_lookup));
+ if (acpi_dev_uid_match(pdata->adev, 1))
+ pwm_add_table(byt_pwm_lookup, ARRAY_SIZE(byt_pwm_lookup));
}
#define LPSS_I2C_ENABLE 0x6c
@@ -218,13 +214,9 @@ static struct pwm_lookup bsw_pwm_lookup[] = {
static void bsw_pwm_setup(struct lpss_private_data *pdata)
{
- u64 uid;
-
/* Only call pwm_add_table for the first PWM controller */
- if (acpi_dev_uid_to_integer(pdata->adev, &uid) || uid != 1)
- return;
-
- pwm_add_table(bsw_pwm_lookup, ARRAY_SIZE(bsw_pwm_lookup));
+ if (acpi_dev_uid_match(pdata->adev, 1))
+ pwm_add_table(bsw_pwm_lookup, ARRAY_SIZE(bsw_pwm_lookup));
}
static const struct property_entry lpt_spi_properties[] = {
--
2.17.1
Introduce is_array_type() and is_pointer_type() helpers, which compare the
data type of provided argument against the enumeration values defined in
typeclass.h using __builtin_classify_type() function and identify array
and pointer types respectively.
Signed-off-by: Raag Jadav <[email protected]>
---
include/linux/compiler.h | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/include/linux/compiler.h b/include/linux/compiler.h
index bb1339c7057b..b4f656002c0f 100644
--- a/include/linux/compiler.h
+++ b/include/linux/compiler.h
@@ -242,6 +242,11 @@ static inline void *offset_to_ptr(const int *off)
#define is_signed_type(type) (((type)(-1)) < (__force type)1)
#define is_unsigned_type(type) (!is_signed_type(type))
+/* Classify data type based on enum values in typeclass.h */
+#define is_array_type(x) (__builtin_classify_type(x) == 14)
+#define is_pointer_type(x) (__builtin_classify_type(x) == 5)
+#define is_array_or_pointer_type(x) (is_array_type(x) || is_pointer_type(x))
+
/*
* This is needed in functions which generate the stack canary, see
* arch/x86/kernel/smpboot.c::start_secondary() for an example.
--
2.17.1
Now that we have _UID matching support for integer types, we can use
acpi_dev_uid_match() for it.
Signed-off-by: Raag Jadav <[email protected]>
---
drivers/firmware/efi/dev-path-parser.c | 7 ++-----
1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/drivers/firmware/efi/dev-path-parser.c b/drivers/firmware/efi/dev-path-parser.c
index f80d87c199c3..937be269fee8 100644
--- a/drivers/firmware/efi/dev-path-parser.c
+++ b/drivers/firmware/efi/dev-path-parser.c
@@ -18,8 +18,6 @@ static long __init parse_acpi_path(const struct efi_dev_path *node,
struct acpi_device *adev;
struct device *phys_dev;
char hid[ACPI_ID_LEN];
- u64 uid;
- int ret;
if (node->header.length != 12)
return -EINVAL;
@@ -31,10 +29,9 @@ static long __init parse_acpi_path(const struct efi_dev_path *node,
node->acpi.hid >> 16);
for_each_acpi_dev_match(adev, hid, NULL, -1) {
- ret = acpi_dev_uid_to_integer(adev, &uid);
- if (ret == 0 && node->acpi.uid == uid)
+ if (acpi_dev_uid_match(adev, node->acpi.uid))
break;
- if (ret == -ENODATA && node->acpi.uid == 0)
+ if (!acpi_device_uid(adev) && node->acpi.uid == 0)
break;
}
if (!adev)
--
2.17.1
Now that we have _UID matching support for integer types, we can use
acpi_dev_hid_uid_match() for it.
Signed-off-by: Raag Jadav <[email protected]>
---
drivers/perf/arm_cspmu/arm_cspmu.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/perf/arm_cspmu/arm_cspmu.c b/drivers/perf/arm_cspmu/arm_cspmu.c
index 2cc35dded007..50b89b989ce7 100644
--- a/drivers/perf/arm_cspmu/arm_cspmu.c
+++ b/drivers/perf/arm_cspmu/arm_cspmu.c
@@ -1108,7 +1108,6 @@ static int arm_cspmu_request_irq(struct arm_cspmu *cspmu)
static inline int arm_cspmu_find_cpu_container(int cpu, u32 container_uid)
{
- u64 acpi_uid;
struct device *cpu_dev;
struct acpi_device *acpi_dev;
@@ -1118,8 +1117,7 @@ static inline int arm_cspmu_find_cpu_container(int cpu, u32 container_uid)
acpi_dev = ACPI_COMPANION(cpu_dev);
while (acpi_dev) {
- if (acpi_dev_hid_uid_match(acpi_dev, ACPI_PROCESSOR_CONTAINER_HID, NULL) &&
- !acpi_dev_uid_to_integer(acpi_dev, &acpi_uid) && acpi_uid == container_uid)
+ if (acpi_dev_hid_uid_match(acpi_dev, ACPI_PROCESSOR_CONTAINER_HID, container_uid))
return 0;
acpi_dev = acpi_dev_parent(acpi_dev);
--
2.17.1
On Tue, Nov 21, 2023 at 04:08:29PM +0530, Raag Jadav wrote:
> Now that we have _UID matching support for integer types, we can use
> acpi_dev_hid_uid_match() for it.
>
> Signed-off-by: Raag Jadav <[email protected]>
> ---
> drivers/perf/arm_cspmu/arm_cspmu.c | 4 +---
> 1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/drivers/perf/arm_cspmu/arm_cspmu.c b/drivers/perf/arm_cspmu/arm_cspmu.c
> index 2cc35dded007..50b89b989ce7 100644
> --- a/drivers/perf/arm_cspmu/arm_cspmu.c
> +++ b/drivers/perf/arm_cspmu/arm_cspmu.c
> @@ -1108,7 +1108,6 @@ static int arm_cspmu_request_irq(struct arm_cspmu *cspmu)
>
> static inline int arm_cspmu_find_cpu_container(int cpu, u32 container_uid)
> {
> - u64 acpi_uid;
> struct device *cpu_dev;
> struct acpi_device *acpi_dev;
>
> @@ -1118,8 +1117,7 @@ static inline int arm_cspmu_find_cpu_container(int cpu, u32 container_uid)
>
> acpi_dev = ACPI_COMPANION(cpu_dev);
> while (acpi_dev) {
> - if (acpi_dev_hid_uid_match(acpi_dev, ACPI_PROCESSOR_CONTAINER_HID, NULL) &&
> - !acpi_dev_uid_to_integer(acpi_dev, &acpi_uid) && acpi_uid == container_uid)
> + if (acpi_dev_hid_uid_match(acpi_dev, ACPI_PROCESSOR_CONTAINER_HID, container_uid))
> return 0;
>
> acpi_dev = acpi_dev_parent(acpi_dev);
> --
> 2.17.1
Acked-by: Will Deacon <[email protected]>
Will
On Tue, 21 Nov 2023 at 05:39, Raag Jadav <[email protected]> wrote:
>
> Now that we have _UID matching support for integer types, we can use
> acpi_dev_uid_match() for it.
>
> Signed-off-by: Raag Jadav <[email protected]>
Reviewed-by: Ard Biesheuvel <[email protected]>
> ---
> drivers/firmware/efi/dev-path-parser.c | 7 ++-----
> 1 file changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/firmware/efi/dev-path-parser.c b/drivers/firmware/efi/dev-path-parser.c
> index f80d87c199c3..937be269fee8 100644
> --- a/drivers/firmware/efi/dev-path-parser.c
> +++ b/drivers/firmware/efi/dev-path-parser.c
> @@ -18,8 +18,6 @@ static long __init parse_acpi_path(const struct efi_dev_path *node,
> struct acpi_device *adev;
> struct device *phys_dev;
> char hid[ACPI_ID_LEN];
> - u64 uid;
> - int ret;
>
> if (node->header.length != 12)
> return -EINVAL;
> @@ -31,10 +29,9 @@ static long __init parse_acpi_path(const struct efi_dev_path *node,
> node->acpi.hid >> 16);
>
> for_each_acpi_dev_match(adev, hid, NULL, -1) {
> - ret = acpi_dev_uid_to_integer(adev, &uid);
> - if (ret == 0 && node->acpi.uid == uid)
> + if (acpi_dev_uid_match(adev, node->acpi.uid))
> break;
> - if (ret == -ENODATA && node->acpi.uid == 0)
> + if (!acpi_device_uid(adev) && node->acpi.uid == 0)
> break;
> }
> if (!adev)
> --
> 2.17.1
>
On Tue, Nov 21, 2023 at 04:08:26PM +0530, Raag Jadav wrote:
> Now that we have _UID matching support for both integer and string types,
> we can support them into acpi_dev_hid_uid_match() helper as well.
>
> Signed-off-by: Raag Jadav <[email protected]>
Reviewed-by: Mika Westerberg <[email protected]>
On Tue, Nov 21, 2023 at 04:08:27PM +0530, Raag Jadav wrote:
> Now that we have _UID matching support for integer types, we can use
> acpi_dev_uid_match() for it.
>
> Signed-off-by: Raag Jadav <[email protected]>
Acked-by: Mika Westerberg <[email protected]>
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 <[email protected]>
> Suggested-by: Mika Westerberg <[email protected]>
Reviewed-by: Mika Westerberg <[email protected]>
On Tue, Nov 21, 2023 at 11:38 AM Raag Jadav <[email protected]> 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 <[email protected]>
> Suggested-by: Mika Westerberg <[email protected]>
> Suggested-by: Rafael J. Wysocki <[email protected]>
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 <[email protected]>
> ---
> 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 <[email protected]> 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 <[email protected]>
> > Suggested-by: Mika Westerberg <[email protected]>
> > Suggested-by: Rafael J. Wysocki <[email protected]>
>
> 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 <[email protected]>
> > ---
> > 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 <[email protected]> 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 <[email protected]>
> > Suggested-by: Mika Westerberg <[email protected]>
> > Suggested-by: Rafael J. Wysocki <[email protected]>
>
> 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!
--
With Best Regards,
Andy Shevchenko
On Wed, Nov 22, 2023 at 5:58 AM Raag Jadav <[email protected]> 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 <[email protected]> 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 <[email protected]>
> > > Suggested-by: Mika Westerberg <[email protected]>
> > > Suggested-by: Rafael J. Wysocki <[email protected]>
> >
> > 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.