Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.38 as permitted sender) client-ip=23.128.96.38;
MIME-Version: 1.0
References: <20231121103829.10027-1-raag.jadav@intel.com> <20231121103829.10027-3-raag.jadav@intel.com>
In-Reply-To: <20231121103829.10027-3-raag.jadav@intel.com>
From:   "Rafael J. Wysocki" <rafael@kernel.org>
Date:   Tue, 21 Nov 2023 20:25:20 +0100
Message-ID: <CAJZ5v0jmaRQWfO_mM4GZ8mEFftuSNgt36=tJ5vC2Uw7MAcpYJg@mail.gmail.com>
Subject: Re: [PATCH v2 2/6] ACPI: bus: update acpi_dev_uid_match() to support
 multiple types
To:     Raag Jadav <raag.jadav@intel.com>
Cc:     mika.westerberg@linux.intel.com, andriy.shevchenko@linux.intel.com,
        rafael@kernel.org, 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
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8BIT
Precedence: bulk

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.

> 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?

> +
> +#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)
>  {
> --