Patchwork [1/2] ACPI / video: Refactor and fix dmi_is_desktop

login
register
mail settings
Submitter Hans de Goede
Date Jan. 7, 2019, 4:08 p.m.
Message ID <20190107160821.26961-1-hdegoede@redhat.com>
Download mbox | patch
Permalink /patch/693997/
State New
Headers show

Comments

Hans de Goede - Jan. 7, 2019, 4:08 p.m.
This commit refactors the chassis-type detection introduced by
commit 53fa1f6e8a59 ("ACPI / video: Only default only_lcd to true on
Win8-ready _desktops_") (where desktop means anything without a builtin
screen).

The DMI chassis_type is an unsigned integer, so rather then doing a
whole bunch of string-compares on it, convert it to an int and feed
the result to a switch case.

Note the switch case uses hex values, this is done because the spec
uses hex values too. This changes the check for "Main Server Chassis"
from checking for 11 decimal to 11 hexadecimal, this is a bug fix,
the original check for 11 decimal was wrong.

Fixes: 53fa1f6e8a59 ("ACPI / video: Only default only_lcd to true ...")
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/acpi/acpi_video.c | 20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)
Rafael J. Wysocki - Jan. 14, 2019, 10:52 a.m.
On Mon, Jan 7, 2019 at 5:08 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> This commit refactors the chassis-type detection introduced by
> commit 53fa1f6e8a59 ("ACPI / video: Only default only_lcd to true on
> Win8-ready _desktops_") (where desktop means anything without a builtin
> screen).
>
> The DMI chassis_type is an unsigned integer, so rather then doing a
> whole bunch of string-compares on it, convert it to an int and feed
> the result to a switch case.
>
> Note the switch case uses hex values, this is done because the spec
> uses hex values too. This changes the check for "Main Server Chassis"
> from checking for 11 decimal to 11 hexadecimal, this is a bug fix,
> the original check for 11 decimal was wrong.
>
> Fixes: 53fa1f6e8a59 ("ACPI / video: Only default only_lcd to true ...")
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/acpi/acpi_video.c | 20 +++++++++++++-------
>  1 file changed, 13 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/acpi/acpi_video.c b/drivers/acpi/acpi_video.c
> index f0b52266b3ac..ba5c2f70babe 100644
> --- a/drivers/acpi/acpi_video.c
> +++ b/drivers/acpi/acpi_video.c
> @@ -2124,21 +2124,27 @@ static int __init intel_opregion_present(void)
>         return opregion;
>  }
>
> +/* Check if the chassis-type indicates there is no builtin LCD panel */
>  static bool dmi_is_desktop(void)
>  {
>         const char *chassis_type;
> +       unsigned long type;
>
>         chassis_type = dmi_get_system_info(DMI_CHASSIS_TYPE);
>         if (!chassis_type)
>                 return false;
>
> -       if (!strcmp(chassis_type, "3") || /*  3: Desktop */
> -           !strcmp(chassis_type, "4") || /*  4: Low Profile Desktop */
> -           !strcmp(chassis_type, "5") || /*  5: Pizza Box */
> -           !strcmp(chassis_type, "6") || /*  6: Mini Tower */
> -           !strcmp(chassis_type, "7") || /*  7: Tower */
> -           !strcmp(chassis_type, "11"))  /* 11: Main Server Chassis */
> -               return true;
> +       if (kstrtoul(chassis_type, 10, &type) != 0)
> +               return false;
> +

Why don't you write the code below as a single line:

return (type >= 0x03 && type <= 0x07) || type == 0x11;

> +       switch (type) {
> +       case 0x03: return true; /* Desktop */
> +       case 0x04: return true; /* Low Profile Desktop */
> +       case 0x05: return true; /* Pizza Box */
> +       case 0x06: return true; /* Mini Tower */
> +       case 0x07: return true; /* Tower */
> +       case 0x11: return true; /* Main Server Chassis */
> +       }
>
>         return false;
>  }
> --

And you can add a comment like "Return true for the Desktop (0x03),
Low Profile Desktop (0x04), ... Main Server Chassis (0x11) form
factors." next to it to describe the meaning of the values.
Hans de Goede - Jan. 14, 2019, 11:13 a.m.
Hi,

On 14-01-19 11:52, Rafael J. Wysocki wrote:
> On Mon, Jan 7, 2019 at 5:08 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> This commit refactors the chassis-type detection introduced by
>> commit 53fa1f6e8a59 ("ACPI / video: Only default only_lcd to true on
>> Win8-ready _desktops_") (where desktop means anything without a builtin
>> screen).
>>
>> The DMI chassis_type is an unsigned integer, so rather then doing a
>> whole bunch of string-compares on it, convert it to an int and feed
>> the result to a switch case.
>>
>> Note the switch case uses hex values, this is done because the spec
>> uses hex values too. This changes the check for "Main Server Chassis"
>> from checking for 11 decimal to 11 hexadecimal, this is a bug fix,
>> the original check for 11 decimal was wrong.
>>
>> Fixes: 53fa1f6e8a59 ("ACPI / video: Only default only_lcd to true ...")
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>   drivers/acpi/acpi_video.c | 20 +++++++++++++-------
>>   1 file changed, 13 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/acpi/acpi_video.c b/drivers/acpi/acpi_video.c
>> index f0b52266b3ac..ba5c2f70babe 100644
>> --- a/drivers/acpi/acpi_video.c
>> +++ b/drivers/acpi/acpi_video.c
>> @@ -2124,21 +2124,27 @@ static int __init intel_opregion_present(void)
>>          return opregion;
>>   }
>>
>> +/* Check if the chassis-type indicates there is no builtin LCD panel */
>>   static bool dmi_is_desktop(void)
>>   {
>>          const char *chassis_type;
>> +       unsigned long type;
>>
>>          chassis_type = dmi_get_system_info(DMI_CHASSIS_TYPE);
>>          if (!chassis_type)
>>                  return false;
>>
>> -       if (!strcmp(chassis_type, "3") || /*  3: Desktop */
>> -           !strcmp(chassis_type, "4") || /*  4: Low Profile Desktop */
>> -           !strcmp(chassis_type, "5") || /*  5: Pizza Box */
>> -           !strcmp(chassis_type, "6") || /*  6: Mini Tower */
>> -           !strcmp(chassis_type, "7") || /*  7: Tower */
>> -           !strcmp(chassis_type, "11"))  /* 11: Main Server Chassis */
>> -               return true;
>> +       if (kstrtoul(chassis_type, 10, &type) != 0)
>> +               return false;
>> +
> 
> Why don't you write the code below as a single line:
> 
> return (type >= 0x03 && type <= 0x07) || type == 0x11;

If you look at the second patch in the set it adds a check for type == 0x10, so
then this would become:

return (type >= 0x03 && type <= 0x07) || type == 0x10 || type == 0x11;

I'm afraid that in the future if we want to add a check for one or
two more values this will become messy. Also the switch-case +
per line comment makes it much clearer which value is what then the
comment you suggest IMHO.

So I would prefer to keep the switch case, but if you want me to
rewrite this as you suggest then I can post a v2 with that
changed.

Regards,

Hans


> 
>> +       switch (type) {
>> +       case 0x03: return true; /* Desktop */
>> +       case 0x04: return true; /* Low Profile Desktop */
>> +       case 0x05: return true; /* Pizza Box */
>> +       case 0x06: return true; /* Mini Tower */
>> +       case 0x07: return true; /* Tower */
>> +       case 0x11: return true; /* Main Server Chassis */
>> +       }
>>
>>          return false;
>>   }
>> --
> 
> And you can add a comment like "Return true for the Desktop (0x03),
> Low Profile Desktop (0x04), ... Main Server Chassis (0x11) form
> factors." next to it to describe the meaning of the values.
>
Rafael J. Wysocki - Jan. 14, 2019, 11:24 a.m.
On Mon, Jan 14, 2019 at 12:13 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi,
>
> On 14-01-19 11:52, Rafael J. Wysocki wrote:
> > On Mon, Jan 7, 2019 at 5:08 PM Hans de Goede <hdegoede@redhat.com> wrote:
> >>
> >> This commit refactors the chassis-type detection introduced by
> >> commit 53fa1f6e8a59 ("ACPI / video: Only default only_lcd to true on
> >> Win8-ready _desktops_") (where desktop means anything without a builtin
> >> screen).
> >>
> >> The DMI chassis_type is an unsigned integer, so rather then doing a
> >> whole bunch of string-compares on it, convert it to an int and feed
> >> the result to a switch case.
> >>
> >> Note the switch case uses hex values, this is done because the spec
> >> uses hex values too. This changes the check for "Main Server Chassis"
> >> from checking for 11 decimal to 11 hexadecimal, this is a bug fix,
> >> the original check for 11 decimal was wrong.
> >>
> >> Fixes: 53fa1f6e8a59 ("ACPI / video: Only default only_lcd to true ...")
> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> >> ---
> >>   drivers/acpi/acpi_video.c | 20 +++++++++++++-------
> >>   1 file changed, 13 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/drivers/acpi/acpi_video.c b/drivers/acpi/acpi_video.c
> >> index f0b52266b3ac..ba5c2f70babe 100644
> >> --- a/drivers/acpi/acpi_video.c
> >> +++ b/drivers/acpi/acpi_video.c
> >> @@ -2124,21 +2124,27 @@ static int __init intel_opregion_present(void)
> >>          return opregion;
> >>   }
> >>
> >> +/* Check if the chassis-type indicates there is no builtin LCD panel */
> >>   static bool dmi_is_desktop(void)
> >>   {
> >>          const char *chassis_type;
> >> +       unsigned long type;
> >>
> >>          chassis_type = dmi_get_system_info(DMI_CHASSIS_TYPE);
> >>          if (!chassis_type)
> >>                  return false;
> >>
> >> -       if (!strcmp(chassis_type, "3") || /*  3: Desktop */
> >> -           !strcmp(chassis_type, "4") || /*  4: Low Profile Desktop */
> >> -           !strcmp(chassis_type, "5") || /*  5: Pizza Box */
> >> -           !strcmp(chassis_type, "6") || /*  6: Mini Tower */
> >> -           !strcmp(chassis_type, "7") || /*  7: Tower */
> >> -           !strcmp(chassis_type, "11"))  /* 11: Main Server Chassis */
> >> -               return true;
> >> +       if (kstrtoul(chassis_type, 10, &type) != 0)
> >> +               return false;
> >> +
> >
> > Why don't you write the code below as a single line:
> >
> > return (type >= 0x03 && type <= 0x07) || type == 0x11;
>
> If you look at the second patch in the set it adds a check for type == 0x10, so
> then this would become:
>
> return (type >= 0x03 && type <= 0x07) || type == 0x10 || type == 0x11;

That you could write as

return (type >= 0x03 && type <= 0x07) || (type & 0x10);

too. ;-)

> I'm afraid that in the future if we want to add a check for one or
> two more values this will become messy. Also the switch-case +
> per line comment makes it much clearer which value is what then the
> comment you suggest IMHO.

Well, fair enough I guess, but can you make the switch a fall-through one?

The multiple 'return true;' statements really look odd to me.

Patch

diff --git a/drivers/acpi/acpi_video.c b/drivers/acpi/acpi_video.c
index f0b52266b3ac..ba5c2f70babe 100644
--- a/drivers/acpi/acpi_video.c
+++ b/drivers/acpi/acpi_video.c
@@ -2124,21 +2124,27 @@  static int __init intel_opregion_present(void)
 	return opregion;
 }
 
+/* Check if the chassis-type indicates there is no builtin LCD panel */
 static bool dmi_is_desktop(void)
 {
 	const char *chassis_type;
+	unsigned long type;
 
 	chassis_type = dmi_get_system_info(DMI_CHASSIS_TYPE);
 	if (!chassis_type)
 		return false;
 
-	if (!strcmp(chassis_type, "3") || /*  3: Desktop */
-	    !strcmp(chassis_type, "4") || /*  4: Low Profile Desktop */
-	    !strcmp(chassis_type, "5") || /*  5: Pizza Box */
-	    !strcmp(chassis_type, "6") || /*  6: Mini Tower */
-	    !strcmp(chassis_type, "7") || /*  7: Tower */
-	    !strcmp(chassis_type, "11"))  /* 11: Main Server Chassis */
-		return true;
+	if (kstrtoul(chassis_type, 10, &type) != 0)
+		return false;
+
+	switch (type) {
+	case 0x03: return true; /* Desktop */
+	case 0x04: return true; /* Low Profile Desktop */
+	case 0x05: return true; /* Pizza Box */
+	case 0x06: return true; /* Mini Tower */
+	case 0x07: return true; /* Tower */
+	case 0x11: return true; /* Main Server Chassis */
+	}
 
 	return false;
 }