[v2] ACPI: LPSS: use acpi_dev_uid_match() for matching _UID

  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
  

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