Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.35 as permitted sender) client-ip=23.128.96.35;
Date:   Wed, 22 Nov 2023 06:58:10 +0200
From:   Raag Jadav <raag.jadav@intel.com>
To:     "Rafael J. Wysocki" <rafael@kernel.org>
Cc:     mika.westerberg@linux.intel.com, andriy.shevchenko@linux.intel.com,
        lenb@kernel.org, robert.moore@intel.com, ardb@kernel.org,
        will@kernel.org, mark.rutland@arm.com, linux-acpi@vger.kernel.org,
        linux-kernel@vger.kernel.org,
        acpica-devel@lists.linuxfoundation.org, linux-efi@vger.kernel.org,
        linux-arm-kernel@lists.infradead.org,
        mallikarjunappa.sangannavar@intel.com, bala.senthil@intel.com
Subject: Re: [PATCH v2 2/6] ACPI: bus: update acpi_dev_uid_match() to support
 multiple types
Message-ID: <ZV2KYqah4FHH4tnz@black.fi.intel.com>
References: <20231121103829.10027-1-raag.jadav@intel.com>
 <20231121103829.10027-3-raag.jadav@intel.com>
 <CAJZ5v0jmaRQWfO_mM4GZ8mEFftuSNgt36=tJ5vC2Uw7MAcpYJg@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <CAJZ5v0jmaRQWfO_mM4GZ8mEFftuSNgt36=tJ5vC2Uw7MAcpYJg@mail.gmail.com>
Precedence: bulk

On Tue, Nov 21, 2023 at 08:25:20PM +0100, Rafael J. Wysocki wrote:
> On Tue, Nov 21, 2023 at 11:38 AM Raag Jadav <raag.jadav@intel.com> wrote:
> >
> > According to ACPI specification, a _UID object can evaluate to either
> > a numeric value or a string. Update acpi_dev_uid_match() helper to
> > support _UID matching for both integer and string types.
> >
> > Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > Suggested-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> > Suggested-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> You need to be careful with using this.  There are some things below
> that go beyond what I have suggested.

I think we all suggested some bits and pieces so I included everyone.
We can drop if there are any objections.

> > Signed-off-by: Raag Jadav <raag.jadav@intel.com>
> > ---
> >  drivers/acpi/utils.c    | 19 -------------------
> >  include/acpi/acpi_bus.h | 35 ++++++++++++++++++++++++++++++++++-
> >  include/linux/acpi.h    |  8 +++-----
> >  3 files changed, 37 insertions(+), 25 deletions(-)
> >
> > diff --git a/drivers/acpi/utils.c b/drivers/acpi/utils.c
> > index 28c75242fca9..fe7e850c6479 100644
> > --- a/drivers/acpi/utils.c
> > +++ b/drivers/acpi/utils.c
> > @@ -824,25 +824,6 @@ bool acpi_check_dsm(acpi_handle handle, const guid_t *guid, u64 rev, u64 funcs)
> >  }
> >  EXPORT_SYMBOL(acpi_check_dsm);
> >
> > -/**
> > - * acpi_dev_uid_match - Match device by supplied UID
> > - * @adev: ACPI device to match.
> > - * @uid2: Unique ID of the device.
> > - *
> > - * Matches UID in @adev with given @uid2.
> > - *
> > - * Returns:
> > - *  - %true if matches.
> > - *  - %false otherwise.
> > - */
> > -bool acpi_dev_uid_match(struct acpi_device *adev, const char *uid2)
> > -{
> > -       const char *uid1 = acpi_device_uid(adev);
> > -
> > -       return uid1 && uid2 && !strcmp(uid1, uid2);
> > -}
> > -EXPORT_SYMBOL_GPL(acpi_dev_uid_match);
> > -
> >  /**
> >   * acpi_dev_hid_uid_match - Match device by supplied HID and UID
> >   * @adev: ACPI device to match.
> > diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
> > index ec6a673dcb95..bcd78939bab4 100644
> > --- a/include/acpi/acpi_bus.h
> > +++ b/include/acpi/acpi_bus.h
> > @@ -9,6 +9,7 @@
> >  #ifndef __ACPI_BUS_H__
> >  #define __ACPI_BUS_H__
> >
> > +#include <linux/compiler.h>
> >  #include <linux/device.h>
> >  #include <linux/property.h>
> >
> > @@ -857,10 +858,42 @@ static inline bool acpi_device_can_poweroff(struct acpi_device *adev)
> >                 adev->power.states[ACPI_STATE_D3_HOT].flags.explicit_set);
> >  }
> >
> > -bool acpi_dev_uid_match(struct acpi_device *adev, const char *uid2);
> >  bool acpi_dev_hid_uid_match(struct acpi_device *adev, const char *hid2, const char *uid2);
> >  int acpi_dev_uid_to_integer(struct acpi_device *adev, u64 *integer);
> >
> > +static inline bool acpi_str_uid_match(struct acpi_device *adev, const char *uid2)
> > +{
> > +       const char *uid1 = acpi_device_uid(adev);
> > +
> > +       return uid1 && uid2 && !strcmp(uid1, uid2);
> > +}
> > +
> > +static inline bool acpi_int_uid_match(struct acpi_device *adev, u64 uid2)
> > +{
> > +       u64 uid1;
> > +
> > +       return !acpi_dev_uid_to_integer(adev, &uid1) && uid1 == uid2;
> > +}
> > +
> 
> Up to this point it is all fine IMV.
> 
> > +/**
> > + * acpi_dev_uid_match - Match device by supplied UID
> > + * @adev: ACPI device to match.
> > + * @uid2: Unique ID of the device.
> > + *
> > + * Matches UID in @adev with given @uid2.
> > + *
> > + * Returns: %true if matches, %false otherwise.
> > + */
> > +
> > +/* Treat uid as a string for array and pointer types, treat as an integer otherwise */
> > +#define get_uid_type(x) \
> > +       (__builtin_choose_expr(is_array_or_pointer_type(x), (const char *)0, (u64)0))
> 
> But I wouldn't use the above.
> 
> It is far more elaborate than needed for this use case and may not
> actually work as expected.  For instance, why would a pointer to a
> random struct type be a good candidate for a string?

Such case will not compile, since its data type will not match with
acpi_str_uid_match() prototype. The compiler does a very good job of
qualifing only the compatible string types here, which is exactly what
we want.

error: passing argument 2 of 'acpi_str_uid_match' from incompatible pointer type [-Werror=incompatible-pointer-types]
    if (acpi_dev_uid_match(adev, adev)) {
                                 ^
./include/acpi/acpi_bus.h:870:20: note: expected 'const char *' but argument is of type 'struct acpi_device *'
 static inline bool acpi_str_uid_match(struct acpi_device *adev, const char *uid2)

> > +
> > +#define acpi_dev_uid_match(adev, uid2)                         \
> > +       _Generic(get_uid_type(uid2),                            \
> > +                const char *: acpi_str_uid_match,              \
> > +                u64: acpi_int_uid_match)(adev, uid2)
> > +
> 
> Personally, I would just do something like the following
> 
> #define acpi_dev_uid_match(adev, uid2) \
>         _Generic((uid2), \
>                 const char *: acpi_str_uid_match, \
>                 char *: acpi_str_uid_match, \
>                 const void *: acpi_str_uid_match, \
>                 void *: acpi_str_uid_match, \
>                 default: acpi_int_uid_match)(adev, uid2)
> 
> which doesn't require compiler.h to be fiddled with and is rather
> straightforward to follow.
> 
> If I'm to apply the patches, this is about the level of complexity you
> need to target.

Understood, however this will limit the type support to only a handful
of types and will not satisfy a few of the existing users, which, for
example are passing signed or unsigned pointer or an array of u8.

Listing every possible type manually for _Generic() looks a bit verbose
for something that can be simply achieved by __builtin functions in my
opinion.

I can still send out a v3 to see if it really works. However, I prefer the
v2 approach, as it covers all possible scenarios without any corner cases.

Raag