Re: [flashrom] [PATCH 3/3] Warn if running on laptops

[Carl-Daniel Hailfinger]

Missing signoff. Will comment anyway.

On 24.02.2010 14:28, Michael Karcher wrote:
> diff --git a/dmi.c b/dmi.c
> index 100767f..ad023e0 100644
> --- a/dmi.c
> +++ b/dmi.c
> @@ -96,6 +96,7 @@ static char *get_dmi_string(const char * string_name)
>  void dmi_init(void)
>  {
>    int i;
> char * chassis_type;

char *chassis_type;
> has_dmi_support = 1;
>    for (i = 0; i < DMI_ID_INVALID; i++) {
>    	dmistrings[i] = get_dmi_string(dmidecode_names[i]);
> @@ -104,6 +105,14 @@ void dmi_init(void)
>    		break;
>    	}
>    }
> chassis_type = get_dmi_string("chassis-type");
> if (chassis_type && !strcmp(chassis_type, "Notebook"))
> {
> printf_debug("Laptop detected via DMI");
> is_laptop = 1;
> }
> free (chassis_type);

Not sure about the most common coding style. free(object) or free
(object). Would be nice to have uniform usage.
> }
>
> /**
> diff --git a/flash.h b/flash.h
> index ff2004c..0ad6212 100644
> --- a/flash.h
> +++ b/flash.h
> @@ -382,6 +382,7 @@ struct pci_dev *pci_card_find(uint16_t vendor, uint16_t device,
>  void get_io_perms(void);
>  void release_io_perms(void);
>  #if INTERNAL_SUPPORT == 1
> +extern int is_laptop;
>  void probe_superio(void);
>  int internal_init(void);
>  int internal_shutdown(void);
> diff --git a/internal.c b/internal.c
> index 12dae39..d557738 100644
> --- a/internal.c
> +++ b/internal.c
> @@ -110,6 +110,8 @@ void probe_superio(void)
>  #endif
>  }
>
> +int is_laptop;
>
> int internal_init(void)
>  {
>    int ret = 0;
> @@ -131,6 +133,16 @@ int internal_init(void)
>    /* Probe for the SuperI/O chip and fill global struct superio. */
>    probe_superio();
> /* Warn if a laptop is detected */
> if (is_laptop)
> printf("========================================================================\n"
>        "WARNING! You seem to be running flashrom on a 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"
>        "See http://www.flashrom.org/Supported_hardware#Unsupported_Laptops.2FNotebooks.2FNetbooks\n"

Should we point people to http://www.flashrom.org/Laptops instead?
>        "========================================================================\n");
> /* try to enable it. Failure IS an option, since not all motherboards
> really need this to be done, etc., etc.
> */

Looks good otherwise. Fix or explain the issues above, and this is
Acked-by: Carl-Daniel Hailfinger

Regards,
Carl-Daniel

[PATCH 3/3] Warn if running on laptops

[Michael Karcher]

Am Mittwoch, den 24.02.2010, 16:39 +0100 schrieb Carl-Daniel Hailfinger:
> > char * chassis_type;
>
> char *chassis_type;

Changed.
> > free (chassis_type);
>
> Not sure about the most common coding style. free(object) or free
> (object). Would be nice to have uniform usage.

Changed. You have the space only after keywords.
> >        "See http://www.flashrom.org/Supported_hardware#Unsupported_Laptops.2FNotebooks.2FNetbooks\n"
>
> Should we point people to http://www.flashrom.org/Laptops instead?

Yes, we should.
> Acked-by: Carl-Daniel Hailfinger

Updated patch included, thanks for the ack. Not committed, as it depends
on the unacked "[PATCH 2/3] Factor out DMI string reading into
subfunction"

Regards,
  Michael Karcher

[PATCH] Warn if running on laptops

[Michael Karcher]

Extra email, as I forgot to include the patch

Signed-off-by: Michael Karcher 
Acked-by: Carl-Daniel Hailfinger 
---
 dmi.c      |    8 ++++++++
 flash.h    |    1 +
 internal.c |   12 ++++++++++++
 3 files changed, 21 insertions(+), 0 deletions(-)

diff --git a/dmi.c b/dmi.c
index 90d4b30..6f45199 100644
--- a/dmi.c
+++ b/dmi.c
@@ -93,6 +93,7 @@ static char *get_dmi_string(const char *string_name)
 void dmi_init(void)
 {
    int i;
+	char *chassis_type;
    has_dmi_support = 1;
    for (i = 0; i < DMI_ID_INVALID; i++) {
    	dmistrings[i] = get_dmi_string(dmidecode_names[i]);
@@ -101,6 +102,13 @@ void dmi_init(void)
    		break;
    	}
    }
+
+	chassis_type = get_dmi_string("chassis-type");
+	if (chassis_type && !strcmp(chassis_type, "Notebook")) {
+		printf_debug("Laptop detected via DMI");
+		is_laptop = 1;
+	}
+	free(chassis_type);
 }

/**
diff --git a/flash.h b/flash.h
index ff2004c..0ad6212 100644
--- a/flash.h
+++ b/flash.h
@@ -382,6 +382,7 @@ struct pci_dev *pci_card_find(uint16_t vendor, uint16_t device,
 void get_io_perms(void);
 void release_io_perms(void);
 #if INTERNAL_SUPPORT == 1
+extern int is_laptop;
 void probe_superio(void);
 int internal_init(void);
 int internal_shutdown(void);
diff --git a/internal.c b/internal.c
index 12dae39..d38ae4b 100644
--- a/internal.c
+++ b/internal.c
@@ -110,6 +110,8 @@ void probe_superio(void)
 #endif
 }

+int is_laptop;
+
 int internal_init(void)
 {
    int ret = 0;
@@ -131,6 +133,16 @@ int internal_init(void)
    /* Probe for the SuperI/O chip and fill global struct superio. */
    probe_superio();

+	/* Warn if a laptop is detected */
+	if (is_laptop)
+		printf("========================================================================\n"
+		       "WARNING! You seem to be running flashrom on a 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"
+		       "See http://www.flashrom.org/Laptops%5Cn"
+		       "========================================================================\n");
+
    /* try to enable it. Failure IS an option, since not all motherboards
     * really need this to be done, etc., etc.
     */

[PATCH 3/3] Warn if running on laptops

[Michael Karcher]

Am Mittwoch, den 24.02.2010, 23:48 +0100 schrieb Michael Karcher:
> > Acked-by: Carl-Daniel Hailfinger
>
> Updated patch included, thanks for the ack. Not committed, as it depends
> on the unacked "[PATCH 2/3] Factor out DMI string reading into
> subfunction"

Committed as r916, thanks.

Regards,
  Michael Karcher