This series refines _UID references across kernel by:
- Extracting _UID matching functionality from acpi_dev_hid_uid_match()
helper and introducing it as a separate acpi_dev_uid_match() helper.
- Converting manual _UID references to use standard ACPI helpers.
Raag Jadav (8):
ACPI: utils: Introduce acpi_dev_uid_match() for matching _UID
pinctrl: intel: use acpi_dev_uid_match() for matching _UID
ACPI: sysfs: use acpi_device_uid() for fetching _UID
ACPI: utils: use acpi_dev_uid_match() for matching _UID
ACPI: x86: use acpi_dev_uid_match() for matching _UID
perf: qcom: use acpi_device_uid() for fetching _UID
hwmon: nct6775: use acpi_dev_hid_uid_match() for matching _HID and
_UID
perf: arm_cspmu: use acpi_dev_hid_uid_match() for matching _HID and
_UID
drivers/acpi/device_sysfs.c | 6 ++--
drivers/acpi/utils.c | 43 +++++++++++++++++++++------
drivers/acpi/x86/utils.c | 3 +-
drivers/hwmon/nct6775-platform.c | 4 +--
drivers/perf/arm_cspmu/arm_cspmu.c | 8 ++---
drivers/perf/qcom_l3_pmu.c | 4 +--
drivers/pinctrl/intel/pinctrl-intel.c | 2 +-
include/acpi/acpi_bus.h | 1 +
include/linux/acpi.h | 5 ++++
9 files changed, 51 insertions(+), 25 deletions(-)
base-commit: 9bc2fb9a7e41542a193658deff3df572fa24cb68
--
2.17.1
Convert manual _UID references to use standard ACPI helpers.
Signed-off-by: Raag Jadav <[email protected]>
Reviewed-by: Andy Shevchenko <[email protected]>
---
drivers/pinctrl/intel/pinctrl-intel.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/pinctrl/intel/pinctrl-intel.c b/drivers/pinctrl/intel/pinctrl-intel.c
index 3be04ab760d3..999f453344d2 100644
--- a/drivers/pinctrl/intel/pinctrl-intel.c
+++ b/drivers/pinctrl/intel/pinctrl-intel.c
@@ -1694,7 +1694,7 @@ const struct intel_pinctrl_soc_data *intel_pinctrl_get_soc_data(struct platform_
unsigned int i;
for (i = 0; table[i]; i++) {
- if (!strcmp(adev->pnp.unique_id, table[i]->uid)) {
+ if (acpi_dev_uid_match(adev, table[i]->uid)) {
data = table[i];
break;
}
--
2.17.1
Convert manual _UID references to use standard ACPI helpers.
Signed-off-by: Raag Jadav <[email protected]>
---
drivers/acpi/x86/utils.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/acpi/x86/utils.c b/drivers/acpi/x86/utils.c
index 63d834dd3811..bc65ebfcdf76 100644
--- a/drivers/acpi/x86/utils.c
+++ b/drivers/acpi/x86/utils.c
@@ -184,8 +184,7 @@ bool acpi_device_override_status(struct acpi_device *adev, unsigned long long *s
if (acpi_match_device_ids(adev, override_status_ids[i].hid))
continue;
- if (!adev->pnp.unique_id ||
- strcmp(adev->pnp.unique_id, override_status_ids[i].uid))
+ if (!acpi_dev_uid_match(adev, override_status_ids[i].uid))
continue;
}
--
2.17.1
Convert manual _UID references to use standard ACPI helpers.
Signed-off-by: Raag Jadav <[email protected]>
---
drivers/perf/qcom_l3_pmu.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/perf/qcom_l3_pmu.c b/drivers/perf/qcom_l3_pmu.c
index 2887edb4eb0b..f16783d03db7 100644
--- a/drivers/perf/qcom_l3_pmu.c
+++ b/drivers/perf/qcom_l3_pmu.c
@@ -742,8 +742,8 @@ static int qcom_l3_cache_pmu_probe(struct platform_device *pdev)
l3pmu = devm_kzalloc(&pdev->dev, sizeof(*l3pmu), GFP_KERNEL);
name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "l3cache_%s_%s",
- acpi_dev_parent(acpi_dev)->pnp.unique_id,
- acpi_dev->pnp.unique_id);
+ acpi_device_uid(acpi_dev_parent(acpi_dev)),
+ acpi_device_uid(acpi_dev));
if (!l3pmu || !name)
return -ENOMEM;
--
2.17.1
Convert manual _UID references to use standard ACPI helpers.
Signed-off-by: Raag Jadav <[email protected]>
---
drivers/perf/arm_cspmu/arm_cspmu.c | 8 +++-----
1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/drivers/perf/arm_cspmu/arm_cspmu.c b/drivers/perf/arm_cspmu/arm_cspmu.c
index e2b7827c4563..f0e6d14281d6 100644
--- a/drivers/perf/arm_cspmu/arm_cspmu.c
+++ b/drivers/perf/arm_cspmu/arm_cspmu.c
@@ -1061,7 +1061,7 @@ static int arm_cspmu_request_irq(struct arm_cspmu *cspmu)
static inline int arm_cspmu_find_cpu_container(int cpu, u32 container_uid)
{
- u32 acpi_uid;
+ u64 acpi_uid;
struct device *cpu_dev;
struct acpi_device *acpi_dev;
@@ -1071,10 +1071,8 @@ static inline int arm_cspmu_find_cpu_container(int cpu, u32 container_uid)
acpi_dev = ACPI_COMPANION(cpu_dev);
while (acpi_dev) {
- if (!strcmp(acpi_device_hid(acpi_dev),
- ACPI_PROCESSOR_CONTAINER_HID) &&
- !kstrtouint(acpi_device_uid(acpi_dev), 0, &acpi_uid) &&
- acpi_uid == container_uid)
+ 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)
return 0;
acpi_dev = acpi_dev_parent(acpi_dev);
--
2.17.1
Introduce acpi_dev_uid_match() helper that matches the device with
supplied _UID string.
Suggested-by: Andy Shevchenko <[email protected]>
Signed-off-by: Raag Jadav <[email protected]>
Reviewed-by: Andy Shevchenko <[email protected]>
---
drivers/acpi/utils.c | 40 +++++++++++++++++++++++++++++++++-------
include/acpi/acpi_bus.h | 1 +
include/linux/acpi.h | 5 +++++
3 files changed, 39 insertions(+), 7 deletions(-)
diff --git a/drivers/acpi/utils.c b/drivers/acpi/utils.c
index 2ea14648a661..664b9890b625 100644
--- a/drivers/acpi/utils.c
+++ b/drivers/acpi/utils.c
@@ -768,28 +768,54 @@ 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, pass NULL to not check _UID.
+ *
+ * Matches UID in @adev with given @uid2. Absence of @uid2 will be treated as
+ * a match. If user wants to validate @uid2, it should be done before calling
+ * this function. This behaviour is as needed by most of its current users.
+ *
+ * Returns:
+ * - %true if matches or @uid2 is NULL.
+ * - %false otherwise.
+ */
+bool acpi_dev_uid_match(struct acpi_device *adev, const char *uid2)
+{
+ const char *uid1 = acpi_device_uid(adev);
+
+ if (!uid2)
+ return true;
+
+ return uid1 && !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.
* @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.
- * Returns true if matches.
+ * 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. This behaviour is as needed by most of
+ * its current users.
+ *
+ * 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);
- const char *uid1 = acpi_device_uid(adev);
if (strcmp(hid1, hid2))
return false;
- if (!uid2)
- return true;
-
- return uid1 && !strcmp(uid1, uid2);
+ return acpi_dev_uid_match(adev, uid2);
}
EXPORT_SYMBOL(acpi_dev_hid_uid_match);
diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
index 254685085c82..d1fe6446ffe0 100644
--- a/include/acpi/acpi_bus.h
+++ b/include/acpi/acpi_bus.h
@@ -760,6 +760,7 @@ 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);
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index afd94c9b8b8a..db3a33e19c97 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -787,6 +787,11 @@ 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
Convert manual _UID references to use standard ACPI helpers.
Signed-off-by: Raag Jadav <[email protected]>
---
drivers/hwmon/nct6775-platform.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/hwmon/nct6775-platform.c b/drivers/hwmon/nct6775-platform.c
index 81bf03dad6bb..0adeeab7ee03 100644
--- a/drivers/hwmon/nct6775-platform.c
+++ b/drivers/hwmon/nct6775-platform.c
@@ -1465,10 +1465,8 @@ static const char * const asus_msi_boards[] = {
static int nct6775_asuswmi_device_match(struct device *dev, void *data)
{
struct acpi_device *adev = to_acpi_device(dev);
- const char *uid = acpi_device_uid(adev);
- const char *hid = acpi_device_hid(adev);
- if (hid && !strcmp(hid, ASUSWMI_DEVICE_HID) && uid && !strcmp(uid, data)) {
+ if (acpi_dev_hid_uid_match(adev, ASUSWMI_DEVICE_HID, data)) {
asus_acpi_dev = adev;
return 1;
}
--
2.17.1
Convert manual _UID references to use standard ACPI helpers.
Signed-off-by: Raag Jadav <[email protected]>
---
drivers/acpi/utils.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/acpi/utils.c b/drivers/acpi/utils.c
index 664b9890b625..e3ba835e6590 100644
--- a/drivers/acpi/utils.c
+++ b/drivers/acpi/utils.c
@@ -889,8 +889,7 @@ static int acpi_dev_match_cb(struct device *dev, const void *data)
if (acpi_match_device_ids(adev, match->hid))
return 0;
- if (match->uid && (!adev->pnp.unique_id ||
- strcmp(adev->pnp.unique_id, match->uid)))
+ if (!acpi_dev_uid_match(adev, match->uid))
return 0;
if (match->hrv == -1)
--
2.17.1
On Fri, Oct 20, 2023 at 02:17:25PM +0530, Raag Jadav wrote:
> Introduce acpi_dev_uid_match() helper that matches the device with
> supplied _UID string.
...
> +/**
> + * acpi_dev_uid_match - Match device by supplied UID
> + * @adev: ACPI device to match.
> + * @uid2: Unique ID of the device, pass NULL to not check _UID.
> + *
> + * Matches UID in @adev with given @uid2. Absence of @uid2 will be treated as
> + * a match. If user wants to validate @uid2, it should be done before calling
> + * this function. This behaviour is as needed by most of its current users.
The "current" internally I applied to the result of the series. So reading this
again I think the better wording is "potential".
> + *
> + * Returns:
> + * - %true if matches or @uid2 is NULL.
> + * - %false otherwise.
> + */
...
> /**
> * 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.
> - * Returns true if matches.
> + * 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. This behaviour is as needed by most of
> + * its current users.
Ditto.
> + * Returns:
> + * - %true if matches or @uid2 is NULL.
> + * - %false otherwise.
> */
--
With Best Regards,
Andy Shevchenko
On Fri, Oct 20, 2023 at 02:17:28PM +0530, Raag Jadav wrote:
> Convert manual _UID references to use standard ACPI helpers.
Yes, while not so obvious this is the correct replacement.
Reviewed-by: Andy Shevchenko <[email protected]>
--
With Best Regards,
Andy Shevchenko
On Fri, Oct 20, 2023 at 02:17:29PM +0530, Raag Jadav wrote:
> Convert manual _UID references to use standard ACPI helpers.
Reviewed-by: Andy Shevchenko <[email protected]>
...
> - if (!adev->pnp.unique_id ||
> - strcmp(adev->pnp.unique_id, override_status_ids[i].uid))
> + if (!acpi_dev_uid_match(adev, override_status_ids[i].uid))
The check for NULL argument inside that API does not affect the behaviour as
otherwise it will be a crash with the current implementation.
> continue;
--
With Best Regards,
Andy Shevchenko
On Fri, Oct 20, 2023 at 01:36:27PM +0300, Andy Shevchenko wrote:
> On Fri, Oct 20, 2023 at 02:17:28PM +0530, Raag Jadav wrote:
> > Convert manual _UID references to use standard ACPI helpers.
>
> Yes, while not so obvious this is the correct replacement.
> Reviewed-by: Andy Shevchenko <[email protected]>
I think this is the only case which would suffer from the more obvious
behaviour, i.e.
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);
}
That said, we can't be particularly sure about it's potential future users,
especially when the usage will not be limited to just ACPI core since we're
exporting it.
Raag
On Fri, Oct 20, 2023 at 02:38:06PM +0300, Raag Jadav wrote:
> On Fri, Oct 20, 2023 at 01:36:27PM +0300, Andy Shevchenko wrote:
> > On Fri, Oct 20, 2023 at 02:17:28PM +0530, Raag Jadav wrote:
> > > Convert manual _UID references to use standard ACPI helpers.
> >
> > Yes, while not so obvious this is the correct replacement.
> > Reviewed-by: Andy Shevchenko <[email protected]>
>
> I think this is the only case which would suffer from the more obvious
> behaviour, i.e.
No, that's not true. The same with override CPU in the other patch, where the
check is simply absent, but the result will be the same. So, all with negation
will suffer from the "obvious" implementation.
> 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);
> }
>
> That said, we can't be particularly sure about it's potential future users,
> especially when the usage will not be limited to just ACPI core since we're
> exporting it.
--
With Best Regards,
Andy Shevchenko
On Fri, Oct 20, 2023 at 04:42:08PM +0300, Andy Shevchenko wrote:
> On Fri, Oct 20, 2023 at 02:38:06PM +0300, Raag Jadav wrote:
> > On Fri, Oct 20, 2023 at 01:36:27PM +0300, Andy Shevchenko wrote:
> > > On Fri, Oct 20, 2023 at 02:17:28PM +0530, Raag Jadav wrote:
> > > > Convert manual _UID references to use standard ACPI helpers.
> > >
> > > Yes, while not so obvious this is the correct replacement.
> > > Reviewed-by: Andy Shevchenko <[email protected]>
> >
> > I think this is the only case which would suffer from the more obvious
> > behaviour, i.e.
>
> No, that's not true. The same with override CPU in the other patch, where the
> check is simply absent, but the result will be the same. So, all with negation
> will suffer from the "obvious" implementation.
Forgot to add, we don't need to change the original acpi_dev_hid_uid_match()
behaviour, i.e.
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);
}
I'm fine with both, this just makes more sense to me.
Raag
On Fri, 20 Oct 2023 14:17:25 +0530
Raag Jadav <[email protected]> wrote:
> Introduce acpi_dev_uid_match() helper that matches the device with
> supplied _UID string.
>
> Suggested-by: Andy Shevchenko <[email protected]>
> Signed-off-by: Raag Jadav <[email protected]>
> Reviewed-by: Andy Shevchenko <[email protected]>
> /**
> * 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.
> - * Returns true if matches.
> + * 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. This behaviour is as needed by most of
> + * its current users.
If there are other other users that need different behavior are they
buggy? Also what behavior is this referring to?
I'd just drop the at last sentence as confusing and not adding much.
> + *
> + * 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);
> - const char *uid1 = acpi_device_uid(adev);
>
> if (strcmp(hid1, hid2))
> return false;
>
> - if (!uid2)
> - return true;
> -
> - return uid1 && !strcmp(uid1, uid2);
> + return acpi_dev_uid_match(adev, uid2);
> }
> EXPORT_SYMBOL(acpi_dev_hid_uid_match);
>
> diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
> index 254685085c82..d1fe6446ffe0 100644
> --- a/include/acpi/acpi_bus.h
> +++ b/include/acpi/acpi_bus.h
> @@ -760,6 +760,7 @@ 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);
>
> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
> index afd94c9b8b8a..db3a33e19c97 100644
> --- a/include/linux/acpi.h
> +++ b/include/linux/acpi.h
> @@ -787,6 +787,11 @@ 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 Fri, Oct 20, 2023 at 12:38 PM Andy Shevchenko
<[email protected]> wrote:
>
> On Fri, Oct 20, 2023 at 02:17:29PM +0530, Raag Jadav wrote:
> > Convert manual _UID references to use standard ACPI helpers.
>
> Reviewed-by: Andy Shevchenko <[email protected]>
>
> ...
>
> > - if (!adev->pnp.unique_id ||
> > - strcmp(adev->pnp.unique_id, override_status_ids[i].uid))
> > + if (!acpi_dev_uid_match(adev, override_status_ids[i].uid))
>
> The check for NULL argument inside that API does not affect the behaviour as
> otherwise it will be a crash with the current implementation.
What current implementation do you mean, exactly?
On Fri, Oct 20, 2023 at 1:38 PM Raag Jadav <[email protected]> wrote:
>
> On Fri, Oct 20, 2023 at 01:36:27PM +0300, Andy Shevchenko wrote:
> > On Fri, Oct 20, 2023 at 02:17:28PM +0530, Raag Jadav wrote:
> > > Convert manual _UID references to use standard ACPI helpers.
> >
> > Yes, while not so obvious this is the correct replacement.
> > Reviewed-by: Andy Shevchenko <[email protected]>
>
> I think this is the only case which would suffer from the more obvious
> behaviour, i.e.
>
> 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);
> }
>
> That said, we can't be particularly sure about it's potential future users,
> especially when the usage will not be limited to just ACPI core since we're
> exporting it.
I actually agree with this, so please switch over to the above.
The above is straightforward and easy to understand and if somebody
wants to treat uid2 == NULL as a match, let them check that explicitly
before calling this function.
On Fri, Oct 20, 2023 at 10:48 AM Raag Jadav <[email protected]> wrote:
>
> Convert manual _UID references to use standard ACPI helpers.
>
> Signed-off-by: Raag Jadav <[email protected]>
> ---
> drivers/perf/qcom_l3_pmu.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/perf/qcom_l3_pmu.c b/drivers/perf/qcom_l3_pmu.c
> index 2887edb4eb0b..f16783d03db7 100644
> --- a/drivers/perf/qcom_l3_pmu.c
> +++ b/drivers/perf/qcom_l3_pmu.c
> @@ -742,8 +742,8 @@ static int qcom_l3_cache_pmu_probe(struct platform_device *pdev)
>
> l3pmu = devm_kzalloc(&pdev->dev, sizeof(*l3pmu), GFP_KERNEL);
> name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "l3cache_%s_%s",
> - acpi_dev_parent(acpi_dev)->pnp.unique_id,
> - acpi_dev->pnp.unique_id);
> + acpi_device_uid(acpi_dev_parent(acpi_dev)),
> + acpi_device_uid(acpi_dev));
> if (!l3pmu || !name)
> return -ENOMEM;
>
> --
Applied as 6.7 material.
QCom perf maintainers, if you'd rather take this yourselves, please let me know.
Thanks!
On Fri, Oct 20, 2023 at 07:11:53PM +0200, Rafael J. Wysocki wrote:
> On Fri, Oct 20, 2023 at 1:38 PM Raag Jadav <[email protected]> wrote:
> >
> > On Fri, Oct 20, 2023 at 01:36:27PM +0300, Andy Shevchenko wrote:
> > > On Fri, Oct 20, 2023 at 02:17:28PM +0530, Raag Jadav wrote:
> > > > Convert manual _UID references to use standard ACPI helpers.
> > >
> > > Yes, while not so obvious this is the correct replacement.
> > > Reviewed-by: Andy Shevchenko <[email protected]>
> >
> > I think this is the only case which would suffer from the more obvious
> > behaviour, i.e.
> >
> > 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);
> > }
> >
> > That said, we can't be particularly sure about it's potential future users,
> > especially when the usage will not be limited to just ACPI core since we're
> > exporting it.
>
> I actually agree with this, so please switch over to the above.
Will send out a v2, thanks.
Andy, can I add your review for this?
Raag
On Fri, Oct 20, 2023 at 07:05:37PM +0200, Rafael J. Wysocki wrote:
> On Fri, Oct 20, 2023 at 12:38 PM Andy Shevchenko
> <[email protected]> wrote:
> > On Fri, Oct 20, 2023 at 02:17:29PM +0530, Raag Jadav wrote:
...
> > > - if (!adev->pnp.unique_id ||
> > > - strcmp(adev->pnp.unique_id, override_status_ids[i].uid))
> > > + if (!acpi_dev_uid_match(adev, override_status_ids[i].uid))
> >
> > The check for NULL argument inside that API does not affect the behaviour as
> > otherwise it will be a crash with the current implementation.
>
> What current implementation do you mean, exactly?
strcmp() against NULL will crash the system.
--
With Best Regards,
Andy Shevchenko
On Fri, Oct 20, 2023 at 09:11:56PM +0300, Raag Jadav wrote:
> On Fri, Oct 20, 2023 at 07:11:53PM +0200, Rafael J. Wysocki wrote:
> > On Fri, Oct 20, 2023 at 1:38 PM Raag Jadav <[email protected]> wrote:
> > >
> > > On Fri, Oct 20, 2023 at 01:36:27PM +0300, Andy Shevchenko wrote:
> > > > On Fri, Oct 20, 2023 at 02:17:28PM +0530, Raag Jadav wrote:
> > > > > Convert manual _UID references to use standard ACPI helpers.
> > > >
> > > > Yes, while not so obvious this is the correct replacement.
> > > > Reviewed-by: Andy Shevchenko <[email protected]>
> > >
> > > I think this is the only case which would suffer from the more obvious
> > > behaviour, i.e.
> > >
> > > 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);
> > > }
> > >
> > > That said, we can't be particularly sure about it's potential future users,
> > > especially when the usage will not be limited to just ACPI core since we're
> > > exporting it.
> >
> > I actually agree with this, so please switch over to the above.
>
> Will send out a v2, thanks.
>
> Andy, can I add your review for this?
IIUC you agree with the usage format, but not the actual helper.
So I'm gonna drop it from the first patch and keep it for the rest,
except this one.
Let me know if I'm doing this wrong.
Raag