| Message ID | 20231026083335.12551-1-raag.jadav@intel.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | [v2] ACPI: LPSS: use acpi_dev_uid_match() for matching _UID | expand |
On Thu, Oct 26, 2023 at 02:03:35PM +0530, Raag Jadav wrote: > Now that we have a standard ACPI helper, we can use acpi_dev_uid_match() > for matching _UID as per the original logic before commit 2a036e489eb1 > ("ACPI: LPSS: Refactor _UID handling to use acpi_dev_uid_to_integer()"), > instead of treating it as an integer. > > Signed-off-by: Raag Jadav <raag.jadav@intel.com> > Acked-by: Mika Westerberg <mika.westerberg@linux.intel.com> The change still looks good to me, however I wonder if we could maybe improve acpi_dev_uid_match() to support both data types possible for _UID? This of course is separate patch (unless there are objections). There is the _Generic() thing and I think that can be used to make acpi_dev_uid_match() which takes either u64 (or maybe even unsigned int) or const char * and based on that picks the correct implementation. Not sure if that's possible, did not check but it would allow us to use one function everywhere instead of acpi_dev_uid_to_integer() and acpi_dev_uid_match().
On Fri, Oct 27, 2023 at 11:18:55AM +0300, Mika Westerberg wrote: > On Thu, Oct 26, 2023 at 02:03:35PM +0530, Raag Jadav wrote: > > Now that we have a standard ACPI helper, we can use acpi_dev_uid_match() > > for matching _UID as per the original logic before commit 2a036e489eb1 > > ("ACPI: LPSS: Refactor _UID handling to use acpi_dev_uid_to_integer()"), > > instead of treating it as an integer. > > > > Signed-off-by: Raag Jadav <raag.jadav@intel.com> > > Acked-by: Mika Westerberg <mika.westerberg@linux.intel.com> > > The change still looks good to me, however I wonder if we could maybe > improve acpi_dev_uid_match() to support both data types possible for > _UID? This of course is separate patch (unless there are objections). > > There is the _Generic() thing and I think that can be used to make > > acpi_dev_uid_match() > > which takes either u64 (or maybe even unsigned int) or const char * and > based on that picks the correct implementation. Not sure if that's > possible, did not check but it would allow us to use one function > everywhere instead of acpi_dev_uid_to_integer() and > acpi_dev_uid_match(). The way I see it, acpi_dev_uid_to_integer() is useful when drivers want to parse _UID and store it in their private data, so that it is available for making various decisions throughout the lifetime of the driver, as opposed to acpi_dev_uid_match() which is more useful for oneshot comparisons in my opinion. So I'm a bit conflicted about merging them into a single helper, unless ofcourse there is a way to serve both purposes. However, I do agree that we can improve acpi_dev_uid_match() by treating uids as integers underneath, instead of doing a raw string comparison. This would make it more aligned with the spec as suggested by Andy. Raag
On Fri, Oct 27, 2023 at 01:12:02PM +0300, Raag Jadav wrote: > On Fri, Oct 27, 2023 at 11:18:55AM +0300, Mika Westerberg wrote: > > On Thu, Oct 26, 2023 at 02:03:35PM +0530, Raag Jadav wrote: > > > Now that we have a standard ACPI helper, we can use acpi_dev_uid_match() > > > for matching _UID as per the original logic before commit 2a036e489eb1 > > > ("ACPI: LPSS: Refactor _UID handling to use acpi_dev_uid_to_integer()"), > > > instead of treating it as an integer. > > > > > > Signed-off-by: Raag Jadav <raag.jadav@intel.com> > > > Acked-by: Mika Westerberg <mika.westerberg@linux.intel.com> > > > > The change still looks good to me, however I wonder if we could maybe > > improve acpi_dev_uid_match() to support both data types possible for > > _UID? This of course is separate patch (unless there are objections). > > > > There is the _Generic() thing and I think that can be used to make > > > > acpi_dev_uid_match() > > > > which takes either u64 (or maybe even unsigned int) or const char * and > > based on that picks the correct implementation. Not sure if that's > > possible, did not check but it would allow us to use one function > > everywhere instead of acpi_dev_uid_to_integer() and > > acpi_dev_uid_match(). > > The way I see it, acpi_dev_uid_to_integer() is useful when drivers want to > parse _UID and store it in their private data, so that it is available for > making various decisions throughout the lifetime of the driver, as opposed > to acpi_dev_uid_match() which is more useful for oneshot comparisons in my > opinion. > > So I'm a bit conflicted about merging them into a single helper, unless > ofcourse there is a way to serve both purposes. Or perhaps something like, bool acpi_dev_uid_match(struct acpi_device *adev, const void *uid2, enum uid_type type) { u64 uid1_d, uid2_d; if (type == UID_TYPE_STR) { char *uid2_s = (char *)uid2; if (!(uid2_s && !kstrtou64(uid2_s, 0, &uid2_d))) return false; } else if (type == UID_TYPE_INT) { u64 *uid2_p; uid2_p = (u64 *)uid2; uid2_d = *uid2_p; } else { return false; } if (!acpi_dev_uid_to_integer(adev, &uid1_d) && uid1_d == uid2_d) return true; else return false; } Although this looks unnecessarily hideous. Raag
On Fri, Oct 27, 2023 at 05:17:12PM +0300, Raag Jadav wrote: > On Fri, Oct 27, 2023 at 01:12:02PM +0300, Raag Jadav wrote: > > On Fri, Oct 27, 2023 at 11:18:55AM +0300, Mika Westerberg wrote: > > > On Thu, Oct 26, 2023 at 02:03:35PM +0530, Raag Jadav wrote: > > > > Now that we have a standard ACPI helper, we can use acpi_dev_uid_match() > > > > for matching _UID as per the original logic before commit 2a036e489eb1 > > > > ("ACPI: LPSS: Refactor _UID handling to use acpi_dev_uid_to_integer()"), > > > > instead of treating it as an integer. > > > > > > > > Signed-off-by: Raag Jadav <raag.jadav@intel.com> > > > > Acked-by: Mika Westerberg <mika.westerberg@linux.intel.com> > > > > > > The change still looks good to me, however I wonder if we could maybe > > > improve acpi_dev_uid_match() to support both data types possible for > > > _UID? This of course is separate patch (unless there are objections). > > > > > > There is the _Generic() thing and I think that can be used to make > > > > > > acpi_dev_uid_match() > > > > > > which takes either u64 (or maybe even unsigned int) or const char * and > > > based on that picks the correct implementation. Not sure if that's > > > possible, did not check but it would allow us to use one function > > > everywhere instead of acpi_dev_uid_to_integer() and > > > acpi_dev_uid_match(). > > > > The way I see it, acpi_dev_uid_to_integer() is useful when drivers want to > > parse _UID and store it in their private data, so that it is available for > > making various decisions throughout the lifetime of the driver, as opposed > > to acpi_dev_uid_match() which is more useful for oneshot comparisons in my > > opinion. > > > > So I'm a bit conflicted about merging them into a single helper, unless > > ofcourse there is a way to serve both purposes. > > Or perhaps something like, > > bool acpi_dev_uid_match(struct acpi_device *adev, const void *uid2, enum uid_type type) > { > u64 uid1_d, uid2_d; > > if (type == UID_TYPE_STR) { > char *uid2_s = (char *)uid2; > if (!(uid2_s && !kstrtou64(uid2_s, 0, &uid2_d))) > return false; > } else if (type == UID_TYPE_INT) { > u64 *uid2_p; > uid2_p = (u64 *)uid2; > uid2_d = *uid2_p; > } else { > return false; > } > > if (!acpi_dev_uid_to_integer(adev, &uid1_d) && uid1_d == uid2_d) > return true; > else > return false; > } > > Although this looks unnecessarily hideous. Indeed, but using the _Generic() you should be able to have a single acpi_dev_uid_match() to work for either type so: acpi_dev_uid_match(adev, "1") and acpi_dev_uid_match(adev, 1) would both work with type checkings etc. Not sure if this is worth the trouble though.
On Fri, Oct 27, 2023 at 05:28:56PM +0300, Mika Westerberg wrote: > On Fri, Oct 27, 2023 at 05:17:12PM +0300, Raag Jadav wrote: > > Or perhaps something like, > > > > bool acpi_dev_uid_match(struct acpi_device *adev, const void *uid2, enum uid_type type) > > { > > u64 uid1_d, uid2_d; > > > > if (type == UID_TYPE_STR) { > > char *uid2_s = (char *)uid2; > > if (!(uid2_s && !kstrtou64(uid2_s, 0, &uid2_d))) > > return false; > > } else if (type == UID_TYPE_INT) { > > u64 *uid2_p; > > uid2_p = (u64 *)uid2; > > uid2_d = *uid2_p; > > } else { > > return false; > > } > > > > if (!acpi_dev_uid_to_integer(adev, &uid1_d) && uid1_d == uid2_d) > > return true; > > else > > return false; > > } > > > > Although this looks unnecessarily hideous. > > Indeed, but using the _Generic() you should be able to have > a single acpi_dev_uid_match() to work for either type so: > > acpi_dev_uid_match(adev, "1") > > and > > acpi_dev_uid_match(adev, 1) > > would both work with type checkings etc. > > Not sure if this is worth the trouble though. Well, in that case we can probably try both and hope for the best ;) bool acpi_dev_uid_match(struct acpi_device *adev, const char *uid2) { const char *uid1 = acpi_device_uid(adev); u64 uid1_d; return uid1 && uid2 && (!strcmp(uid1, uid2) || (!kstrtou64(uid1, 0, &uid1_d) && uid1_d == (u64)uid2)); } But I'm guessing the compiler wouldn't be very happy about this. Raag
On Fri, Oct 27, 2023 at 6:51 PM Raag Jadav <raag.jadav@intel.com> wrote: > > On Fri, Oct 27, 2023 at 05:28:56PM +0300, Mika Westerberg wrote: > > On Fri, Oct 27, 2023 at 05:17:12PM +0300, Raag Jadav wrote: > > > Or perhaps something like, > > > > > > bool acpi_dev_uid_match(struct acpi_device *adev, const void *uid2, enum uid_type type) > > > { > > > u64 uid1_d, uid2_d; > > > > > > if (type == UID_TYPE_STR) { > > > char *uid2_s = (char *)uid2; > > > if (!(uid2_s && !kstrtou64(uid2_s, 0, &uid2_d))) > > > return false; > > > } else if (type == UID_TYPE_INT) { > > > u64 *uid2_p; > > > uid2_p = (u64 *)uid2; > > > uid2_d = *uid2_p; > > > } else { > > > return false; > > > } > > > > > > if (!acpi_dev_uid_to_integer(adev, &uid1_d) && uid1_d == uid2_d) > > > return true; > > > else > > > return false; > > > } > > > > > > Although this looks unnecessarily hideous. > > > > Indeed, but using the _Generic() you should be able to have > > a single acpi_dev_uid_match() to work for either type so: > > > > acpi_dev_uid_match(adev, "1") > > > > and > > > > acpi_dev_uid_match(adev, 1) > > > > would both work with type checkings etc. > > > > Not sure if this is worth the trouble though. > > Well, in that case we can probably try both and hope for the best ;) > > bool acpi_dev_uid_match(struct acpi_device *adev, const char *uid2) > { > const char *uid1 = acpi_device_uid(adev); > u64 uid1_d; > > return uid1 && uid2 && (!strcmp(uid1, uid2) || > (!kstrtou64(uid1, 0, &uid1_d) && uid1_d == (u64)uid2)); > } > > But I'm guessing the compiler wouldn't be very happy about this. IMO it would be better to use the observation that if the uid2 string can be successfully cast to an int and the device's unique_id string can't be cast to an int (or the other way around), there is no match. If they both can be cast to an int, they match as long as the casting results are equal. If none of them can be cast to an int, they need to be compared as strings.
On Fri, Oct 27, 2023 at 7:19 PM Rafael J. Wysocki <rafael@kernel.org> wrote: > > On Fri, Oct 27, 2023 at 6:51 PM Raag Jadav <raag.jadav@intel.com> wrote: > > > > On Fri, Oct 27, 2023 at 05:28:56PM +0300, Mika Westerberg wrote: > > > On Fri, Oct 27, 2023 at 05:17:12PM +0300, Raag Jadav wrote: > > > > Or perhaps something like, > > > > > > > > bool acpi_dev_uid_match(struct acpi_device *adev, const void *uid2, enum uid_type type) > > > > { > > > > u64 uid1_d, uid2_d; > > > > > > > > if (type == UID_TYPE_STR) { > > > > char *uid2_s = (char *)uid2; > > > > if (!(uid2_s && !kstrtou64(uid2_s, 0, &uid2_d))) > > > > return false; > > > > } else if (type == UID_TYPE_INT) { > > > > u64 *uid2_p; > > > > uid2_p = (u64 *)uid2; > > > > uid2_d = *uid2_p; > > > > } else { > > > > return false; > > > > } > > > > > > > > if (!acpi_dev_uid_to_integer(adev, &uid1_d) && uid1_d == uid2_d) > > > > return true; > > > > else > > > > return false; > > > > } > > > > > > > > Although this looks unnecessarily hideous. > > > > > > Indeed, but using the _Generic() you should be able to have > > > a single acpi_dev_uid_match() to work for either type so: > > > > > > acpi_dev_uid_match(adev, "1") > > > > > > and > > > > > > acpi_dev_uid_match(adev, 1) > > > > > > would both work with type checkings etc. > > > > > > Not sure if this is worth the trouble though. > > > > Well, in that case we can probably try both and hope for the best ;) > > > > bool acpi_dev_uid_match(struct acpi_device *adev, const char *uid2) > > { > > const char *uid1 = acpi_device_uid(adev); > > u64 uid1_d; > > > > return uid1 && uid2 && (!strcmp(uid1, uid2) || > > (!kstrtou64(uid1, 0, &uid1_d) && uid1_d == (u64)uid2)); > > } > > > > But I'm guessing the compiler wouldn't be very happy about this. > > IMO it would be better to use the observation that if the uid2 string > can be successfully cast to an int and the device's unique_id string > can't be cast to an int (or the other way around), there is no match. > > If they both can be cast to an int, they match as long as the casting > results are equal. > > If none of them can be cast to an int, they need to be compared as strings. Or if the strings don't match literally, try to convert them both to ints and compare.
On Fri, Oct 27, 2023 at 07:40:38PM +0200, Rafael J. Wysocki wrote: > On Fri, Oct 27, 2023 at 7:19 PM Rafael J. Wysocki <rafael@kernel.org> wrote: > > > > On Fri, Oct 27, 2023 at 6:51 PM Raag Jadav <raag.jadav@intel.com> wrote: > > > > > > On Fri, Oct 27, 2023 at 05:28:56PM +0300, Mika Westerberg wrote: > > > > > > > > Indeed, but using the _Generic() you should be able to have > > > > a single acpi_dev_uid_match() to work for either type so: > > > > > > > > acpi_dev_uid_match(adev, "1") > > > > > > > > and > > > > > > > > acpi_dev_uid_match(adev, 1) > > > > > > > > would both work with type checkings etc. > > > > > > > > Not sure if this is worth the trouble though. > > > > > > Well, in that case we can probably try both and hope for the best ;) > > > > > > bool acpi_dev_uid_match(struct acpi_device *adev, const char *uid2) > > > { > > > const char *uid1 = acpi_device_uid(adev); > > > u64 uid1_d; > > > > > > return uid1 && uid2 && (!strcmp(uid1, uid2) || > > > (!kstrtou64(uid1, 0, &uid1_d) && uid1_d == (u64)uid2)); > > > } > > > > > > But I'm guessing the compiler wouldn't be very happy about this. > > > > IMO it would be better to use the observation that if the uid2 string > > can be successfully cast to an int and the device's unique_id string > > can't be cast to an int (or the other way around), there is no match. > > > > If they both can be cast to an int, they match as long as the casting > > results are equal. > > > > If none of them can be cast to an int, they need to be compared as strings. > > Or if the strings don't match literally, try to convert them both to > ints and compare. We'd probably end up with an oops trying to strcmp into a random address without knowing its type, so I think Mika's would be a better approach. #define acpi_dev_uid_match(adev, uid2) \ ({ \ const char *uid1 = acpi_device_uid(adev); \ u64 __uid1; \ \ _Generic(uid2, \ int: uid1 && !kstrtou64(uid1, 0, &__uid1) && (typeof(uid2))__uid1 == uid2, \ const char *: uid1 && uid2 && !strcmp(uid1, (const char *)uid2), \ default: false); \ \ }) This one I atleast got to compile, but I'm not very well versed with _Generic, so this could definitely use some comments. Raag
On Sat, Oct 28, 2023 at 11:58:12AM +0300, Raag Jadav wrote: > On Fri, Oct 27, 2023 at 07:40:38PM +0200, Rafael J. Wysocki wrote: ... > We'd probably end up with an oops trying to strcmp into a random address > without knowing its type, so I think Mika's would be a better approach. > > #define acpi_dev_uid_match(adev, uid2) \ > ({ \ > const char *uid1 = acpi_device_uid(adev); \ > u64 __uid1; \ > \ > _Generic(uid2, \ > int: uid1 && !kstrtou64(uid1, 0, &__uid1) && (typeof(uid2))__uid1 == uid2, \ > const char *: uid1 && uid2 && !strcmp(uid1, (const char *)uid2), \ > default: false); \ > \ > }) > > This one I atleast got to compile, but I'm not very well versed with _Generic, > so this could definitely use some comments. If you go this way, make _Generic() use simple in the macro with a help of two additional functions (per type). Also you need to take care about uid2 type to be _any_ unsigned integer. Or if you want to complicate things, then you need to distinguish signed and unsigned cases. P.S. All to me it seems way too overengineered w/o any potential prospective user.
On Mon, Oct 30, 2023 at 12:12:27PM +0200, Andy Shevchenko wrote: > On Sat, Oct 28, 2023 at 11:58:12AM +0300, Raag Jadav wrote: > > On Fri, Oct 27, 2023 at 07:40:38PM +0200, Rafael J. Wysocki wrote: > > ... > > > We'd probably end up with an oops trying to strcmp into a random address > > without knowing its type, so I think Mika's would be a better approach. > > > > #define acpi_dev_uid_match(adev, uid2) \ > > ({ \ > > const char *uid1 = acpi_device_uid(adev); \ > > u64 __uid1; \ > > \ > > _Generic(uid2, \ > > int: uid1 && !kstrtou64(uid1, 0, &__uid1) && (typeof(uid2))__uid1 == uid2, \ > > const char *: uid1 && uid2 && !strcmp(uid1, (const char *)uid2), \ > > default: false); \ > > \ > > }) > > > > This one I atleast got to compile, but I'm not very well versed with _Generic, > > so this could definitely use some comments. > > If you go this way, make _Generic() use simple in the macro with a help of two > additional functions (per type). Also you need to take care about uid2 type to > be _any_ unsigned integer. Or if you want to complicate things, then you need > to distinguish signed and unsigned cases. My initial thought was to have separate functions per type, but then I realized it would become an unnecessary inconvenience to maintain one per type. Having it inline with _Generic would make it relatively easier, but I'll leave it to the maintainers to decide. > P.S. > All to me it seems way too overengineered w/o any potential prospective user. I found a couple of acpi_dev_uid_to_integer() usages which could be simplified with this implementation, but let's see how everyone feels about this. Thanks for the comments, Raag
diff --git a/drivers/acpi/acpi_lpss.c b/drivers/acpi/acpi_lpss.c index 875de44961bf..6aaa6b066510 100644 --- a/drivers/acpi/acpi_lpss.c +++ b/drivers/acpi/acpi_lpss.c @@ -167,10 +167,8 @@ 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) + if (!acpi_dev_uid_match(pdata->adev, "1")) return; pwm_add_table(byt_pwm_lookup, ARRAY_SIZE(byt_pwm_lookup)); @@ -218,10 +216,8 @@ 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) + if (!acpi_dev_uid_match(pdata->adev, "1")) return; pwm_add_table(bsw_pwm_lookup, ARRAY_SIZE(bsw_pwm_lookup));
Now that we have a standard ACPI helper, we can use acpi_dev_uid_match() for matching _UID as per the original logic before commit 2a036e489eb1 ("ACPI: LPSS: Refactor _UID handling to use acpi_dev_uid_to_integer()"), instead of treating it as an integer. Signed-off-by: Raag Jadav <raag.jadav@intel.com> Acked-by: Mika Westerberg <mika.westerberg@linux.intel.com> --- Changes since v1: - Update commit message drivers/acpi/acpi_lpss.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) base-commit: f9c7f9d537da013471e5c7913a33b6489bdeb3cf