diff mbox series

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

Message ID 20231025053833.16014-1-raag.jadav@intel.com
State New
Headers
Series [v1] ACPI: LPSS: use acpi_dev_uid_match() for matching _UID |

Commit Message

Raag Jadav Oct. 25, 2023, 5:38 a.m. UTC 
  Use acpi_dev_uid_match() for matching _UID instead of treating it
as an integer.

Signed-off-by: Raag Jadav <raag.jadav@intel.com>
---
 drivers/acpi/acpi_lpss.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)


base-commit: 72d54941cd56ac3fedca6f7ae00a300b33ead29e

Comments

Mika Westerberg Oct. 25, 2023, 5:53 a.m. UTC | #1 
On Wed, Oct 25, 2023 at 11:08:33AM +0530, Raag Jadav wrote:
> Use acpi_dev_uid_match() for matching _UID 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>
Rafael J. Wysocki Oct. 25, 2023, 6:04 p.m. UTC | #2 
On Wed, Oct 25, 2023 at 7:53 AM Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
>
> On Wed, Oct 25, 2023 at 11:08:33AM +0530, Raag Jadav wrote:
> > Use acpi_dev_uid_match() for matching _UID 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>

I was about to apply this, but then I realized that it might change
the behavior in a subtle way, because what if the _UID string is
something like "01"?
Raag Jadav Oct. 25, 2023, 6:21 p.m. UTC | #3 
On Wed, Oct 25, 2023 at 08:04:44PM +0200, Rafael J. Wysocki wrote:
> On Wed, Oct 25, 2023 at 7:53 AM Mika Westerberg
> <mika.westerberg@linux.intel.com> wrote:
> >
> > On Wed, Oct 25, 2023 at 11:08:33AM +0530, Raag Jadav wrote:
> > > Use acpi_dev_uid_match() for matching _UID 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>
> 
> I was about to apply this, but then I realized that it might change
> the behavior in a subtle way, because what if the _UID string is
> something like "01"?

I checked the git history and found below.

commit 2a036e489eb1571810126d6fa47bd8af1e237c08
Author: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Date:   Tue Sep 13 19:31:41 2022 +0300

    ACPI: LPSS: Refactor _UID handling to use acpi_dev_uid_to_integer()

    ACPI utils provide acpi_dev_uid_to_integer() helper to extract _UID as
    an integer. Use it instead of custom approach.

    Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
    Reviewed-by: Hans de Goede <hdegoede@redhat.com>
    Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

diff --git a/drivers/acpi/acpi_lpss.c b/drivers/acpi/acpi_lpss.c
index c4d4d21391d7..4d415e210c32 100644
--- a/drivers/acpi/acpi_lpss.c
+++ b/drivers/acpi/acpi_lpss.c
@@ -167,10 +167,10 @@ static struct pwm_lookup byt_pwm_lookup[] = {

 static void byt_pwm_setup(struct lpss_private_data *pdata)
 {
-       struct acpi_device *adev = pdata->adev;
+       u64 uid;

        /* Only call pwm_add_table for the first PWM controller */
-       if (!adev->pnp.unique_id || strcmp(adev->pnp.unique_id, "1"))
+       if (acpi_dev_uid_to_integer(pdata->adev, &uid) || uid != 1)
                return;

        pwm_add_table(byt_pwm_lookup, ARRAY_SIZE(byt_pwm_lookup));

So if we consider the original logic with strcmp, which is more inline
with acpi_dev_uid_match(), "01" should not be the case here in my opinion.

Thanks for sharing your concern though.

Raag
Rafael J. Wysocki Oct. 25, 2023, 6:33 p.m. UTC | #4 
On Wed, Oct 25, 2023 at 8:21 PM Raag Jadav <raag.jadav@intel.com> wrote:
>
> On Wed, Oct 25, 2023 at 08:04:44PM +0200, Rafael J. Wysocki wrote:
> > On Wed, Oct 25, 2023 at 7:53 AM Mika Westerberg
> > <mika.westerberg@linux.intel.com> wrote:
> > >
> > > On Wed, Oct 25, 2023 at 11:08:33AM +0530, Raag Jadav wrote:
> > > > Use acpi_dev_uid_match() for matching _UID 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>
> >
> > I was about to apply this, but then I realized that it might change
> > the behavior in a subtle way, because what if the _UID string is
> > something like "01"?
>
> I checked the git history and found below.
>
> commit 2a036e489eb1571810126d6fa47bd8af1e237c08
> Author: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Date:   Tue Sep 13 19:31:41 2022 +0300
>
>     ACPI: LPSS: Refactor _UID handling to use acpi_dev_uid_to_integer()
>
>     ACPI utils provide acpi_dev_uid_to_integer() helper to extract _UID as
>     an integer. Use it instead of custom approach.
>
>     Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>     Reviewed-by: Hans de Goede <hdegoede@redhat.com>
>     Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> diff --git a/drivers/acpi/acpi_lpss.c b/drivers/acpi/acpi_lpss.c
> index c4d4d21391d7..4d415e210c32 100644
> --- a/drivers/acpi/acpi_lpss.c
> +++ b/drivers/acpi/acpi_lpss.c
> @@ -167,10 +167,10 @@ static struct pwm_lookup byt_pwm_lookup[] = {
>
>  static void byt_pwm_setup(struct lpss_private_data *pdata)
>  {
> -       struct acpi_device *adev = pdata->adev;
> +       u64 uid;
>
>         /* Only call pwm_add_table for the first PWM controller */
> -       if (!adev->pnp.unique_id || strcmp(adev->pnp.unique_id, "1"))
> +       if (acpi_dev_uid_to_integer(pdata->adev, &uid) || uid != 1)
>                 return;
>
>         pwm_add_table(byt_pwm_lookup, ARRAY_SIZE(byt_pwm_lookup));
>
> So if we consider the original logic with strcmp, which is more inline
> with acpi_dev_uid_match(), "01" should not be the case here in my opinion.
>
> Thanks for sharing your concern though.

Well, this means that what the patch really does is to effectively
revert commit 2a036e489eb1571810126d6fa47bd8af1e237c08 and use the new
helper instead of the open-coded check that was there before, so all
of this information should be present in the changelog.
Andy Shevchenko Oct. 25, 2023, 8:33 p.m. UTC | #5 
On Wed, Oct 25, 2023 at 11:08:33AM +0530, Raag Jadav wrote:
> Use acpi_dev_uid_match() for matching _UID instead of treating it
> as an integer.

NAK. See below why.

...

>  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"))

_UID by specification is a type of _string_. Yet, that string may represent an
integer number. Now, how many variants of the strings can you imagine that may
be interpreted as integer 1? I can tell about dozens.

With your change you restricted the all possible spectre of the 1
representations to a single one. Have you checked ALL of the DSDTs
for these platforms to say 'hey, all current tables uses "1" and
this is not an issue'? Further question, will you guarantee that new
platforms on this chip won't use something different? (Not that big
issue, but will require to actively revert your change in the future).

>  		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;
Raag Jadav Oct. 26, 2023, 3 a.m. UTC | #6 
On Wed, Oct 25, 2023 at 11:33:40PM +0300, Andy Shevchenko wrote:
> On Wed, Oct 25, 2023 at 11:08:33AM +0530, Raag Jadav wrote:
> > Use acpi_dev_uid_match() for matching _UID instead of treating it
> > as an integer.
> 
> NAK. See below why.
> 
> ...
> 
> >  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"))
> 
> _UID by specification is a type of _string_. Yet, that string may represent an
> integer number. Now, how many variants of the strings can you imagine that may
> be interpreted as integer 1? I can tell about dozens.
> 
> With your change you restricted the all possible spectre of the 1
> representations to a single one. Have you checked ALL of the DSDTs
> for these platforms to say 'hey, all current tables uses "1" and
> this is not an issue'?

I'm not sure if I'm following you, this would basically invalidate every
usage of acpi_dev_hid_uid_match() helper across the driver.

Raag
Andy Shevchenko Oct. 26, 2023, 12:06 p.m. UTC | #7 
On Thu, Oct 26, 2023 at 06:00:25AM +0300, Raag Jadav wrote:
> On Wed, Oct 25, 2023 at 11:33:40PM +0300, Andy Shevchenko wrote:
> > On Wed, Oct 25, 2023 at 11:08:33AM +0530, Raag Jadav wrote:

...

> > >  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"))
> > 
> > _UID by specification is a type of _string_. Yet, that string may represent an
> > integer number. Now, how many variants of the strings can you imagine that may
> > be interpreted as integer 1? I can tell about dozens.
> > 
> > With your change you restricted the all possible spectre of the 1
> > representations to a single one. Have you checked ALL of the DSDTs
> > for these platforms to say 'hey, all current tables uses "1" and
> > this is not an issue'?
> 
> I'm not sure if I'm following you, this would basically invalidate every
> usage of acpi_dev_hid_uid_match() helper across the driver.

It depends.
diff mbox series

Patch

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));