Re: [flashrom] [PATCH] be more refined regarding DMI chassis types

[Carl-Daniel Hailfinger]

Am 04.06.2011 10:55 schrieb Stefan Tauner:
> we had broken laptops in the past that were not detected as such because their DMI chassis-type was either undefined/out-of-spec, or set to 'other' or 'unknown'.
>
> this patch tries to mitigate this problem as follows:
> if the DMI chassis-type clearly identifies the system as laptop/notebook/mobile platform then nothing changes: the user gets the laptop warning without a hint to the force switch.
> if the DMI chassis-type is not specific enough, we warn the user similarly, but tell them the switch.
>
> to reduce the number of false positives i have added a few new chassis types that we have encountered in the last months to the list.
> Signed-off-by: Stefan Tauner
>
> dmi.c      |   37 ++++++++++++++++++++++++++-----------
>  internal.c |   21 +++++++++++++++------
>  2 files changed, 41 insertions(+), 17 deletions(-)
>
> diff --git a/dmi.c b/dmi.c
> index cda6656..5d37b76 100644
> --- a/dmi.c
> +++ b/dmi.c
> @@ -54,7 +54,13 @@ static const char *dmidecode_names[] = {
>    "baseboard-version",
>  };
>
> -/* A full list of chassis types can be found in the System Management BIOS
> +/* This list is used to identify supposed laptops. The is_laptop field has the
> following meaning:
> 0: in all likelihood not a laptop
> 1: in all likelihood a laptop
> 2: chassis-type is not specific enough (these do not need to be listed
>     in the table, because it is the default anyway)
> A full list of chassis types can be found in the System Management BIOS
> (SMBIOS) Reference Specification 2.7.0 section 7.4.1 "Chassis Types" at
> http://www.dmtf.org/sites/default/files/standards/documents/DSP0134_2.7.0.pd...
> The types below are the most common ones.
>
> @@ -64,14 +70,16 @@ static const struct {
>    unsigned char is_laptop;
>    const char *name;
>  } dmi_chassis_types[] = {
> {0x01, 0, "Other"},
> {0x02, 0, "Unknown"},

Why remove those lines? Use 2 for undecided cases. Removing these lines
will make it harder later to integrate the DMI decoder (we currently
rely on external dmidecode and that fails quite often due to dmidecode
not being installed etc.). The DMI decoder integration patch was posted
some time ago to the list, but it needs to be redone/cleaned up
significantly before we can merge it.
> {0x03, 0, "Desktop",},
> {0x06, 0, "Mini Tower"},
> {0x07, 0, "Tower"},
> {0x08, 1, "Portable"},
> {0x09, 1, "Laptop"},
> {0x0a, 1, "Notebook"},
> {0x0b, 1, "Hand Held"},
> {0x0e, 1, "Sub Notebook"},
> {0x11, 0, "Main Server Chassis"},
> {0x17, 0, "Rack Mount Chassis"},
>
> };
>
> #define DMI_COMMAND_LEN_MAX 260
> @@ -152,16 +160,23 @@ void dmi_init(void)
>    }
>
> chassis_type = get_dmi_string("chassis-type");
> if (chassis_type) {
> for (i = 0; i < ARRAY_SIZE(dmi_chassis_types); i++) {
> 	if (!strcasecmp(chassis_type,
> 			dmi_chassis_types[i].name) &&
> 	    dmi_chassis_types[i].is_laptop) {
> 		msg_pdbg("Laptop detected via DMI\n");
> 		is_laptop = 1;
> 	}
> if (chassis_type == NULL)
> return;

is_laptop = 2;
> for (i = 0; i < ARRAY_SIZE(dmi_chassis_types); i++) {
> if (strcasecmp(chassis_type, dmi_chassis_types[i].name) == 0) {
> 	is_laptop = dmi_chassis_types[i].is_laptop;

break;
> 	if (is_laptop == 1) {
> 		msg_pdbg("Laptop detected via DMI.\n");
> 		free(chassis_type);
> 		return;
> 	} else
> 		break; /* first match breaks loop */

Can you kill the whole inner if statement?
> }
>
> }
> msg_pdbg("DMI chassis-type is not specific enough.\n");

Please use this instead (my whitespace is broken, I know):
switch (is_laptop) {
case 1:
msg_pdbg("Laptop detected via DMI.\n");
break;
case 2:
msg_pdbg("DMI chassis-type is not specific enough.\n");
break;
}
> is_laptop = 2;

Kill the assignment above.
> free(chassis_type);
>  }
>
> diff --git a/internal.c b/internal.c
> index c9f62c1..bbcdc5b 100644
> --- a/internal.c
> +++ b/internal.c
> @@ -124,7 +124,7 @@ int register_superio(struct superio s)
>
> #endif
>
> -int is_laptop = 0;
> +int is_laptop = -1;

This changes the logic. With this change, flashrom assumes that every
machine is a laptop, and that will bite us everywhere dmidecode is
unavailable.
Please review your changes below in light of that.
> int laptop_ok = 0;
>
> int internal_init(void)
> @@ -222,11 +222,19 @@ int internal_init(void)
>
> /* Warn if a non-whitelisted laptop is detected. */
>    if (is_laptop && !laptop_ok) {
> msg_perr("========================================================================\n"
> 	 "WARNING! You seem to be running flashrom on an unsupported laptop.\n"
> 	 "Laptops, notebooks and netbooks are difficult to support and we recommend\n"
> 	 "to use the vendor flashing utility. The embedded controller (EC) in these\n"
> 	 "machines often interacts badly with flashing.\n"
> msg_perr("========================================================================\n");
> if (is_laptop == 1) {
> 	msg_perr("WARNING! You seem to be running flashrom on an unsupported laptop.\n");
> } else {
> 	msg_perr("WARNING! You may be running flashrom on an unsupported laptop. We could\n"
> 		 "not detect this for sure because your vendor has not setup the SMBIOS\n"
> 		 "tables correctly. You can enforce execution by adding\n"
> 		 "'-p internal:laptop=force_I_want_a_brick' to the command line, but\n"
> 		 "please read the following warning if you are not sure.\n\n");
> }
> msg_perr("Laptops, notebooks and netbooks are difficult to support and we\n"
> 	 "recommend to use the vendor flashing utility. The embedded controller\n"
> 	 "(EC) in these machines often interacts badly with flashing.\n"
>  "See http://www.flashrom.org/Laptops for details.\n\n"
>  "If flash is shared with the EC, erase is guaranteed to brick your laptop\n"
>  "and write may brick your laptop.\n"
>
> @@ -234,6 +242,7 @@ int internal_init(void)
>    		 "failure and sudden poweroff.\n"
>    		 "You have been warned.\n"
>    		 "========================================================================\n");
> if (force_laptop) {
>   	msg_perr("Proceeding anyway because user specified "
>   		 "laptop=force_I_want_a_brick\n");

Regards,
Carl-Daniel

[PATCH] be more refined regarding DMI chassis types

[Stefan Tauner]

On Sat, 04 Jun 2011 11:37:27 +0200
Carl-Daniel Hailfinger  wrote:
> Am 04.06.2011 10:55 schrieb Stefan Tauner:
> > diff --git a/internal.c b/internal.c
> > index c9f62c1..bbcdc5b 100644
> > --- a/internal.c
> > +++ b/internal.c
> > @@ -124,7 +124,7 @@ int register_superio(struct superio s)
> >
> > #endif
> >
> > -int is_laptop = 0;
> > +int is_laptop = -1;
>
> This changes the logic. With this change, flashrom assumes that every
> machine is a laptop, and that will bite us everywhere dmidecode is
> unavailable.
> Please review your changes below in light of that.

yes. i forgot about the dependency on dmidecode.
your last sentence seems to indicate a problem with the hunk below, but
i dont see it (assuming that we leave is_laptop = 0 the default).
> > int laptop_ok = 0;
> >
> > int internal_init(void)
> > @@ -222,11 +222,19 @@ int internal_init(void)
> >
> > /* Warn if a non-whitelisted laptop is detected. */
> >    if (is_laptop && !laptop_ok) {
> > msg_perr("========================================================================\n"
> > 	 "WARNING! You seem to be running flashrom on an unsupported laptop.\n"
> > 	 "Laptops, notebooks and netbooks are difficult to support and we recommend\n"
> > 	 "to use the vendor flashing utility. The embedded controller (EC) in these\n"
> > 	 "machines often interacts badly with flashing.\n"
> > msg_perr("========================================================================\n");
> > if (is_laptop == 1) {
> > 	msg_perr("WARNING! You seem to be running flashrom on an unsupported laptop.\n");
> > } else {
> > 	msg_perr("WARNING! You may be running flashrom on an unsupported laptop. We could\n"
> > 		 "not detect this for sure because your vendor has not setup the SMBIOS\n"
> > 		 "tables correctly. You can enforce execution by adding\n"
> > 		 "'-p internal:laptop=force_I_want_a_brick' to the command line, but\n"
> > 		 "please read the following warning if you are not sure.\n\n");
> > }
> > msg_perr("Laptops, notebooks and netbooks are difficult to support and we\n"
> > 	 "recommend to use the vendor flashing utility. The embedded controller\n"
> > 	 "(EC) in these machines often interacts badly with flashing.\n"
> >  "See http://www.flashrom.org/Laptops for details.\n\n"
> >  "If flash is shared with the EC, erase is guaranteed to brick your laptop\n"
> >  "and write may brick your laptop.\n"
> >
> > @@ -234,6 +242,7 @@ int internal_init(void)
> >    		 "failure and sudden poweroff.\n"
> >    		 "You have been warned.\n"
> >    		 "========================================================================\n");
> > if (force_laptop) {
> >   	msg_perr("Proceeding anyway because user specified "
> >   		 "laptop=force_I_want_a_brick\n");

rest is clear and will be dealt with.
thanks for the quick review.

[PATCH] be more refined regarding DMI chassis types

[Carl-Daniel Hailfinger]

Am 04.06.2011 13:44 schrieb Stefan Tauner:
> On Sat, 04 Jun 2011 11:37:27 +0200
> Carl-Daniel Hailfinger  wrote:
> > Am 04.06.2011 10:55 schrieb Stefan Tauner:
> > > diff --git a/internal.c b/internal.c
> > > index c9f62c1..bbcdc5b 100644
> > > --- a/internal.c
> > > +++ b/internal.c
> > > @@ -124,7 +124,7 @@ int register_superio(struct superio s)
> > >
> > > #endif
> > >
> > > -int is_laptop = 0;
> > > +int is_laptop = -1;
> >
> > This changes the logic. With this change, flashrom assumes that every
> > machine is a laptop, and that will bite us everywhere dmidecode is
> > unavailable.
> > Please review your changes below in light of that.
>
> yes. i forgot about the dependency on dmidecode.
> your last sentence seems to indicate a problem with the hunk below, but
> i dont see it (assuming that we leave is_laptop = 0 the default).

IIRC the complaint was only about the side effect of is_laptop=-1. If
you keep is_laptop=0, the logic doesn't change that much.
> > > int laptop_ok = 0;
> > >
> > > int internal_init(void)
> > > @@ -222,11 +222,19 @@ int internal_init(void)
> > >
> > > /* Warn if a non-whitelisted laptop is detected. */
> > >    if (is_laptop && !laptop_ok) {
> > > msg_perr("========================================================================\n"
> > > 	 "WARNING! You seem to be running flashrom on an unsupported laptop.\n"
> > > 	 "Laptops, notebooks and netbooks are difficult to support and we recommend\n"
> > > 	 "to use the vendor flashing utility. The embedded controller (EC) in these\n"
> > > 	 "machines often interacts badly with flashing.\n"
> > > msg_perr("========================================================================\n");
> > > if (is_laptop == 1) {
> > > 	msg_perr("WARNING! You seem to be running flashrom on an unsupported laptop.\n");
> > > } else {
> > > 	msg_perr("WARNING! You may be running flashrom on an unsupported laptop. We could\n"
> > > 		 "not detect this for sure because your vendor has not setup the SMBIOS\n"
> > > 		 "tables correctly. You can enforce execution by adding\n"
> > > 		 "'-p internal:laptop=force_I_want_a_brick' to the command line, but\n"
> > > 		 "please read the following warning if you are not sure.\n\n");
> > > }
> > > msg_perr("Laptops, notebooks and netbooks are difficult to support and we\n"
> > > 	 "recommend to use the vendor flashing utility. The embedded controller\n"
> > > 	 "(EC) in these machines often interacts badly with flashing.\n"
> > >  "See http://www.flashrom.org/Laptops for details.\n\n"
> > >  "If flash is shared with the EC, erase is guaranteed to brick your laptop\n"
> > >  "and write may brick your laptop.\n"
> > >
> > > @@ -234,6 +242,7 @@ int internal_init(void)
> > >    		 "failure and sudden poweroff.\n"
> > >    		 "You have been warned.\n"
> > >    		 "========================================================================\n");
> > > if (force_laptop) {
> > >   	msg_perr("Proceeding anyway because user specified "
> > >   		 "laptop=force_I_want_a_brick\n");
>
> rest is clear and will be dealt with.
> thanks for the quick review.

Can you repost with the review addressed, please? Thanks.

Regards,
Carl-Daniel

[PATCH] be more refined regarding DMI chassis types

[Stefan Tauner]

we had broken laptops in the past that were not detected as such because
their DMI chassis-type was either undefined/out-of-spec, or set to
'other' or 'unknown'.

this patch tries to mitigate this problem as follows:
- if the DMI chassis-type clearly identifies the system as
  laptop/notebook/mobile platform then nothing changes: the user gets
  the laptop warning without a hint to the force switch.
- if the DMI chassis-type is not specific enough, we warn the user
  similarly, but tell them the switch.

to reduce the number of false positives i have added a few new
chassis types that we have encountered in the last months to the list.

Signed-off-by: Stefan Tauner 
---
 dmi.c      |   41 ++++++++++++++++++++++++++++++-----------
 internal.c |   19 ++++++++++++++-----
 2 files changed, 44 insertions(+), 16 deletions(-)

diff --git a/dmi.c b/dmi.c
index 836b38b..5cb6238 100644
--- a/dmi.c
+++ b/dmi.c
@@ -54,7 +54,12 @@ static const char *dmidecode_names[] = {
    "baseboard-version",
 };

-/* A full list of chassis types can be found in the System Management BIOS
+/* This list is used to identify supposed laptops. The is_laptop field has the
+ * following meaning:
+ * 	- 0: in all likelihood not a laptop
+ * 	- 1: in all likelihood a laptop
+ * 	- 2: chassis-type is not specific enough
+ * A full list of chassis types can be found in the System Management BIOS
  * (SMBIOS) Reference Specification 2.7.0 section 7.4.1 "Chassis Types" at
  * http://www.dmtf.org/sites/default/files/standards/documents/DSP0134_2.7.0.pd...
  * The types below are the most common ones.
@@ -64,14 +69,18 @@ static const struct {
    unsigned char is_laptop;
    const char *name;
 } dmi_chassis_types[] = {
-	{0x01, 0, "Other"},
-	{0x02, 0, "Unknown"},
+	{0x01, 2, "Other"},
+	{0x02, 2, "Unknown"},
    {0x03, 0, "Desktop",},
+	{0x06, 0, "Mini Tower"},
+	{0x07, 0, "Tower"},
    {0x08, 1, "Portable"},
    {0x09, 1, "Laptop"},
    {0x0a, 1, "Notebook"},
    {0x0b, 1, "Hand Held"},
    {0x0e, 1, "Sub Notebook"},
+	{0x11, 0, "Main Server Chassis"},
+	{0x17, 0, "Rack Mount Chassis"},
 };

#define DMI_COMMAND_LEN_MAX 260
@@ -152,16 +161,26 @@ void dmi_init(void)
    }

chassis_type = get_dmi_string("chassis-type");
-	if (chassis_type) {
-		for (i = 0; i < ARRAY_SIZE(dmi_chassis_types); i++) {
-			if (!strcasecmp(chassis_type,
-					dmi_chassis_types[i].name) &&
-			    dmi_chassis_types[i].is_laptop) {
-				msg_pdbg("Laptop detected via DMI\n");
-				is_laptop = 1;
-			}
+	if (chassis_type == NULL)
+		return;
+
+	is_laptop = 2;
+	for (i = 0; i < ARRAY_SIZE(dmi_chassis_types); i++) {
+		if (strcasecmp(chassis_type, dmi_chassis_types[i].name) == 0) {
+			is_laptop = dmi_chassis_types[i].is_laptop;
+			break;
    	}
    }
+
+	msg_pdbg("DMI chassis-type is not specific enough.\n");
+	switch (is_laptop) {
+	case 1:
+		msg_pdbg("Laptop detected via DMI.\n");
+		break;
+	case 2:
+		msg_pdbg("DMI chassis-type is not specific enough.\n");
+		break;
+	}
    free(chassis_type);
 }

diff --git a/internal.c b/internal.c
index c59209e..3938428 100644
--- a/internal.c
+++ b/internal.c
@@ -230,11 +230,19 @@ int internal_init(void)

/* Warn if a non-whitelisted laptop is detected. */
    if (is_laptop && !laptop_ok) {
-		msg_perr("========================================================================\n"
-			 "WARNING! You seem to be running flashrom on an unsupported laptop.\n"
-			 "Laptops, notebooks and netbooks are difficult to support and we recommend\n"
-			 "to use the vendor flashing utility. The embedded controller (EC) in these\n"
-			 "machines often interacts badly with flashing.\n"
+		msg_perr("========================================================================\n");
+		if (is_laptop == 1) {
+			msg_perr("WARNING! You seem to be running flashrom on an unsupported laptop.\n");
+		} else {
+			msg_perr("WARNING! You may be running flashrom on an unsupported laptop. We could\n"
+				 "not detect this for sure because your vendor has not setup the SMBIOS\n"
+				 "tables correctly. You can enforce execution by adding\n"
+				 "'-p internal:laptop=force_I_want_a_brick' to the command line, but\n"
+				 "please read the following warning if you are not sure.\n\n");
+		}
+		msg_perr("Laptops, notebooks and netbooks are difficult to support and we\n"
+			 "recommend to use the vendor flashing utility. The embedded controller\n"
+			 "(EC) in these machines often interacts badly with flashing.\n"
    		 "See http://www.flashrom.org/Laptops for details.\n\n"
    		 "If flash is shared with the EC, erase is guaranteed to brick your laptop\n"
    		 "and write may brick your laptop.\n"
@@ -242,6 +250,7 @@ int internal_init(void)
    		 "failure and sudden poweroff.\n"
    		 "You have been warned.\n"
    		 "========================================================================\n");
+
    	if (force_laptop) {
    		msg_perr("Proceeding anyway because user specified "
    			 "laptop=force_I_want_a_brick\n");