[PATCH 0/6] platform/x86: acerhdf: new BIOS string, better modparam handling

[PATCH 0/6] platform/x86: acerhdf: new BIOS string, better modparam handling

[Paul Gortmaker]

I came across this older netbook over the xmas holidays, and noticed the
acerhdf driver wouldn't load.  Turns out the BIOS string was too new,
and not listed in the driver.  There were module params for overrides,
but I found their use isn't quite clear without reading the source.

Here I clarify some of the module parameter usage, and then add the new
BIOS string, so they won't be needed for this particular platform.  Some
probe code is also shifted out to be __init to reduce the driver size a
bit.  Further details can be found in the respective commit logs.

Since it was months ago when I created these patches, I retested on top
of tree to ensure things still work, as can be seen below.

root@gw:~# cat /proc/version 
Linux version 4.19.0-rc4+ (paul@host) (gcc version 5.4.0 20160609 (Ubuntu 5.4.0-6ubuntu1~16.04.10)) #1 SMP Thu Sep 20 18:47:15 EDT 2018
root@gw:~# modprobe acerhdf
root@gw:~# dmesg|tail -n3
acerhdf: Acer Aspire One Fan driver, v.0.7.0
acerhdf: Fan control off, to enable do:
acerhdf: echo -n "enabled" > /sys/class/thermal/thermal_zoneN/mode # N=0,1,2...
root@gw:~# lsmod
Module                  Size  Used by
acerhdf                16384  0 
root@gw:~# 

Paul.
---

Cc: Peter Feuerer <peter@piie.net>
Cc: Darren Hart <dvhart@infradead.org>
Cc: Andy Shevchenko <andy@infradead.org>

Paul Gortmaker (6):
  platform/x86: acerhdf: clarify modinfo messages for BIOS override
  platform/x86: acerhdf: Enable ability to list supported systems
  platform/x86: acerhdf: Remove cut-and-paste trap from instructions
  platform/x86: acerhdf: Add BIOS entry for Gateway LT31 v1.3307
  platform/x86: acerhdf: mark appropriate content with __init prefix
  platform/x86: acerhdf: restructure to allow large BIOS table be __initconst

 drivers/platform/x86/Kconfig   |  5 +++-
 drivers/platform/x86/acerhdf.c | 68 ++++++++++++++++++++++++++++++------------
 2 files changed, 53 insertions(+), 20 deletions(-)

-- 
2.15.0

[PATCH 1/6] platform/x86: acerhdf: clarify modinfo messages for BIOS override

[Paul Gortmaker]

Normally, a module parameter for a BIOS check override implies "pretend
you support this version" (and the user will enter their local version).

However, this driver uses the model/BIOS module parameters in a way that
is "pretend my system is the supported model XYZ with BIOS version ABC."
which is less common.

Since the help strings don't make such a distinction, one gets this
somewhat frustrating scenario, where the user sees the error, enters
*their* BIOS version and then gets the same error:

  root@gw:~# modprobe acerhdf
  acerhdf: Acer Aspire One Fan driver, v.0.7.0
  acerhdf: unknown (unsupported) BIOS version Gateway  /LT31   /v1.3307 , please report, aborting!
  modprobe: ERROR: could not insert 'acerhdf': Invalid argument

  root@gw:~# modprobe acerhdf force_bios=v1.3307
  acerhdf: Acer Aspire One Fan driver, v.0.7.0
  acerhdf: forcing BIOS version: v1.3307
  acerhdf: unknown (unsupported) BIOS version Gateway  /LT31   /v1.3307 , please report, aborting!
  modprobe: ERROR: could not insert 'acerhdf': Invalid argument

Clarify the module param help text to make it clear that the driver
expects a choice from existing supported models/versions.

Cc: Peter Feuerer <peter@piie.net>
Cc: Darren Hart <dvhart@infradead.org>
Cc: Andy Shevchenko <andy@infradead.org>
Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>
---
 drivers/platform/x86/acerhdf.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/platform/x86/acerhdf.c b/drivers/platform/x86/acerhdf.c
index ea22591ee66f..3d5c4e183111 100644
--- a/drivers/platform/x86/acerhdf.c
+++ b/drivers/platform/x86/acerhdf.c
@@ -105,9 +105,9 @@ MODULE_PARM_DESC(fanoff, "Turn the fan off below this temperature");
 module_param(verbose, uint, 0600);
 MODULE_PARM_DESC(verbose, "Enable verbose dmesg output");
 module_param_string(force_bios, force_bios, 16, 0);
-MODULE_PARM_DESC(force_bios, "Force BIOS version and omit BIOS check");
+MODULE_PARM_DESC(force_bios, "Pretend system has this known supported BIOS version");
 module_param_string(force_product, force_product, 16, 0);
-MODULE_PARM_DESC(force_product, "Force BIOS product and omit BIOS check");
+MODULE_PARM_DESC(force_product, "Pretend system is this known supported model");
 
 /*
  * cmd_off: to switch the fan completely off and check if the fan is off
-- 
2.15.0

[PATCH 2/6] platform/x86: acerhdf: Enable ability to list supported systems

[Paul Gortmaker]

This driver has two module parameters that allow an override of the
checks for matching model and BIOS version.  However, both parameters
expect you to choose an entry from the existing list of supported
systems, encoded within the driver itself.

Without the source, such as in a binary distribution, the end user
does not have access to this information, thus rendering the two
module parameters essentially useless.

Add a module parameter that allows the end user to dump the list
of make/model/versions so that they can then pick one that most
closely matches their own system.

Cc: Peter Feuerer <peter@piie.net>
Cc: Darren Hart <dvhart@infradead.org>
Cc: Andy Shevchenko <andy@infradead.org>
Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>
---
 drivers/platform/x86/acerhdf.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/platform/x86/acerhdf.c b/drivers/platform/x86/acerhdf.c
index 3d5c4e183111..2735815c73c5 100644
--- a/drivers/platform/x86/acerhdf.c
+++ b/drivers/platform/x86/acerhdf.c
@@ -86,6 +86,7 @@ static unsigned int interval = 10;
 static unsigned int fanon = 60000;
 static unsigned int fanoff = 53000;
 static unsigned int verbose;
+static unsigned int list_supported;
 static unsigned int fanstate = ACERHDF_FAN_AUTO;
 static char force_bios[16];
 static char force_product[16];
@@ -104,6 +105,8 @@ module_param(fanoff, uint, 0600);
 MODULE_PARM_DESC(fanoff, "Turn the fan off below this temperature");
 module_param(verbose, uint, 0600);
 MODULE_PARM_DESC(verbose, "Enable verbose dmesg output");
+module_param(list_supported, uint, 0600);
+MODULE_PARM_DESC(list_supported, "List supported models and BIOS versions");
 module_param_string(force_bios, force_bios, 16, 0);
 MODULE_PARM_DESC(force_bios, "Pretend system has this known supported BIOS version");
 module_param_string(force_product, force_product, 16, 0);
@@ -632,6 +635,17 @@ static int acerhdf_check_hardware(void)
 
 	pr_info("Acer Aspire One Fan driver, v.%s\n", DRV_VER);
 
+	if (list_supported) {
+		pr_info("List of supported Manufacturer/Model/BIOS:\n");
+		pr_info("---------------------------------------------------\n");
+		for (bt = bios_tbl; bt->vendor[0]; bt++) {
+			pr_info("%-13s | %-17s | %-10s\n", bt->vendor,
+				bt->product, bt->version);
+		}
+		pr_info("---------------------------------------------------\n");
+		return -ECANCELED;
+	}
+
 	if (force_bios[0]) {
 		version = force_bios;
 		pr_info("forcing BIOS version: %s\n", version);
-- 
2.15.0

[PATCH 3/6] platform/x86: acerhdf: Remove cut-and-paste trap from instructions

[Paul Gortmaker]

Just like we avoid specifying actual block devices like sda for fdisk
and dd examples, we should not specify specific thermal zones here.

On the platform I was testing on, zone0 was acpitz, and zone1 was for
this acerhdf driver.  Make the printk such that it won't work with a
blind cut-and-paste, and force the user to determine which zone is
correct for this driver.

Cc: Peter Feuerer <peter@piie.net>
Cc: Darren Hart <dvhart@infradead.org>
Cc: Andy Shevchenko <andy@infradead.org>
Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>
---
 drivers/platform/x86/Kconfig   | 5 ++++-
 drivers/platform/x86/acerhdf.c | 2 +-
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index 0c1aa6c314f5..1fca33c97e8a 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -60,7 +60,10 @@ config ACERHDF
 
 	  After loading this driver the BIOS is still in control of the fan.
 	  To let the kernel handle the fan, do:
-	  echo -n enabled > /sys/class/thermal/thermal_zone0/mode
+	  echo -n enabled > /sys/class/thermal/thermal_zoneN/mode
+	  where N=0,1,2... depending on the number of thermal nodes and the
+	  detection order of your particular system.  The "type" parameter
+	  in the same node directory will tell you if it is "acerhdf".
 
 	  For more information about this driver see
 	  <http://piie.net/files/acerhdf_README.txt>
diff --git a/drivers/platform/x86/acerhdf.c b/drivers/platform/x86/acerhdf.c
index 2735815c73c5..fef3b727bc24 100644
--- a/drivers/platform/x86/acerhdf.c
+++ b/drivers/platform/x86/acerhdf.c
@@ -688,7 +688,7 @@ static int acerhdf_check_hardware(void)
 	 */
 	if (!kernelmode) {
 		pr_notice("Fan control off, to enable do:\n");
-		pr_notice("echo -n \"enabled\" > /sys/class/thermal/thermal_zone0/mode\n");
+		pr_notice("echo -n \"enabled\" > /sys/class/thermal/thermal_zoneN/mode # N=0,1,2...\n");
 	}
 
 	return 0;
-- 
2.15.0

[PATCH 4/6] platform/x86: acerhdf: Add BIOS entry for Gateway LT31 v1.3307

[Paul Gortmaker]

To fix:

  acerhdf: unknown (unsupported) BIOS version Gateway  /LT31   /v1.3307 , please report, aborting!

As can be seen in the context, the BIOS registers haven't changed in
the previous versions, so the assumption is they won't have changed
in this last update for this somewhat older platform either.

Cc: Peter Feuerer <peter@piie.net>
Cc: Darren Hart <dvhart@infradead.org>
Cc: Andy Shevchenko <andy@infradead.org>
Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>
---
 drivers/platform/x86/acerhdf.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/platform/x86/acerhdf.c b/drivers/platform/x86/acerhdf.c
index fef3b727bc24..eddcd8e94a88 100644
--- a/drivers/platform/x86/acerhdf.c
+++ b/drivers/platform/x86/acerhdf.c
@@ -236,6 +236,7 @@ static const struct bios_settings bios_tbl[] = {
 	{"Gateway", "LT31",   "v1.3201",  0x55, 0x58, {0x9e, 0x00}, 0},
 	{"Gateway", "LT31",   "v1.3302",  0x55, 0x58, {0x9e, 0x00}, 0},
 	{"Gateway", "LT31",   "v1.3303t", 0x55, 0x58, {0x9e, 0x00}, 0},
+	{"Gateway", "LT31",   "v1.3307",  0x55, 0x58, {0x9e, 0x00}, 0},
 	/* Packard Bell */
 	{"Packard Bell", "DOA150",  "v0.3104",  0x55, 0x58, {0x21, 0x00}, 0},
 	{"Packard Bell", "DOA150",  "v0.3105",  0x55, 0x58, {0x20, 0x00}, 0},
-- 
2.15.0

[PATCH 5/6] platform/x86: acerhdf: mark appropriate content with __init prefix

[Paul Gortmaker]

These three functions are only called from the probe code which is
already marked __init and hence these can be __init as well.

Cc: Peter Feuerer <peter@piie.net>
Cc: Darren Hart <dvhart@infradead.org>
Cc: Andy Shevchenko <andy@infradead.org>
Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>
---
 drivers/platform/x86/acerhdf.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/platform/x86/acerhdf.c b/drivers/platform/x86/acerhdf.c
index eddcd8e94a88..5f579fcde315 100644
--- a/drivers/platform/x86/acerhdf.c
+++ b/drivers/platform/x86/acerhdf.c
@@ -619,7 +619,7 @@ static int str_starts_with(const char *str, const char *start)
 }
 
 /* check hardware */
-static int acerhdf_check_hardware(void)
+static int __init acerhdf_check_hardware(void)
 {
 	char const *vendor, *version, *product;
 	const struct bios_settings *bt = NULL;
@@ -695,7 +695,7 @@ static int acerhdf_check_hardware(void)
 	return 0;
 }
 
-static int acerhdf_register_platform(void)
+static int __init acerhdf_register_platform(void)
 {
 	int err = 0;
 
@@ -727,7 +727,7 @@ static void acerhdf_unregister_platform(void)
 	platform_driver_unregister(&acerhdf_driver);
 }
 
-static int acerhdf_register_thermal(void)
+static int __init acerhdf_register_thermal(void)
 {
 	cl_dev = thermal_cooling_device_register("acerhdf-fan", NULL,
 						 &acerhdf_cooling_ops);
-- 
2.15.0

[PATCH 6/6] platform/x86: acerhdf: restructure to allow large BIOS table be __initconst

[Paul Gortmaker]

There is a table of Vendor/Model/Version {control data} in this driver,
but outside of the initial probe, the V/M/V is never used again, and
neither are any of the entries for platforms other than the one which
matches the running target.

By simply storing the {control data} for the matched platform, we can
mark the large table __initconst, which reduces the loaded driver size
by 20 percent.

Before:
	root@gw:~/git/linux-head# lsmod
	Module                  Size  Used by
	acerhdf                20480  0
	root@gw:~/git/linux-head#

After:
	root@gw:~/git/linux-head# lsmod
	Module                  Size  Used by
	acerhdf                16384  0
	root@gw:~/git/linux-head#

Cc: Peter Feuerer <peter@piie.net>
Cc: Darren Hart <dvhart@infradead.org>
Cc: Andy Shevchenko <andy@infradead.org>
Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>
---
 drivers/platform/x86/acerhdf.c | 41 ++++++++++++++++++++++++++++-------------
 1 file changed, 28 insertions(+), 13 deletions(-)

diff --git a/drivers/platform/x86/acerhdf.c b/drivers/platform/x86/acerhdf.c
index 5f579fcde315..505224225378 100644
--- a/drivers/platform/x86/acerhdf.c
+++ b/drivers/platform/x86/acerhdf.c
@@ -133,7 +133,7 @@ static const struct manualcmd mcmd = {
 	.moff = 0xff,
 };
 
-/* BIOS settings */
+/* BIOS settings - only used during probe */
 struct bios_settings {
 	const char *vendor;
 	const char *product;
@@ -144,8 +144,18 @@ struct bios_settings {
 	int mcmd_enable;
 };
 
+/* This could be a daughter struct in the above, but not worth the redirect */
+struct ctrl_settings {
+	u8 fanreg;
+	u8 tempreg;
+	struct fancmd cmd;
+	int mcmd_enable;
+};
+
+static struct ctrl_settings ctrl_cfg __read_mostly;
+
 /* Register addresses and values for different BIOS versions */
-static const struct bios_settings bios_tbl[] = {
+static const struct bios_settings bios_tbl[] __initconst = {
 	/* AOA110 */
 	{"Acer", "AOA110", "v0.3109", 0x55, 0x58, {0x1f, 0x00}, 0},
 	{"Acer", "AOA110", "v0.3114", 0x55, 0x58, {0x1f, 0x00}, 0},
@@ -260,8 +270,6 @@ static const struct bios_settings bios_tbl[] = {
 	{"", "", "", 0, 0, {0, 0}, 0}
 };
 
-static const struct bios_settings *bios_cfg __read_mostly;
-
 /*
  * this struct is used to instruct thermal layer to use bang_bang instead of
  * default governor for acerhdf
@@ -274,7 +282,7 @@ static int acerhdf_get_temp(int *temp)
 {
 	u8 read_temp;
 
-	if (ec_read(bios_cfg->tempreg, &read_temp))
+	if (ec_read(ctrl_cfg.tempreg, &read_temp))
 		return -EINVAL;
 
 	*temp = read_temp * 1000;
@@ -286,10 +294,10 @@ static int acerhdf_get_fanstate(int *state)
 {
 	u8 fan;
 
-	if (ec_read(bios_cfg->fanreg, &fan))
+	if (ec_read(ctrl_cfg.fanreg, &fan))
 		return -EINVAL;
 
-	if (fan != bios_cfg->cmd.cmd_off)
+	if (fan != ctrl_cfg.cmd.cmd_off)
 		*state = ACERHDF_FAN_AUTO;
 	else
 		*state = ACERHDF_FAN_OFF;
@@ -310,13 +318,13 @@ static void acerhdf_change_fanstate(int state)
 		state = ACERHDF_FAN_AUTO;
 	}
 
-	cmd = (state == ACERHDF_FAN_OFF) ? bios_cfg->cmd.cmd_off
-					 : bios_cfg->cmd.cmd_auto;
+	cmd = (state == ACERHDF_FAN_OFF) ? ctrl_cfg.cmd.cmd_off
+					 : ctrl_cfg.cmd.cmd_auto;
 	fanstate = state;
 
-	ec_write(bios_cfg->fanreg, cmd);
+	ec_write(ctrl_cfg.fanreg, cmd);
 
-	if (bios_cfg->mcmd_enable && state == ACERHDF_FAN_OFF) {
+	if (ctrl_cfg.mcmd_enable && state == ACERHDF_FAN_OFF) {
 		if (verbose)
 			pr_notice("turning off fan manually\n");
 		ec_write(mcmd.mreg, mcmd.moff);
@@ -623,6 +631,7 @@ static int __init acerhdf_check_hardware(void)
 {
 	char const *vendor, *version, *product;
 	const struct bios_settings *bt = NULL;
+	int found = 0;
 
 	/* get BIOS data */
 	vendor  = dmi_get_system_info(DMI_SYS_VENDOR);
@@ -672,17 +681,23 @@ static int __init acerhdf_check_hardware(void)
 		if (str_starts_with(vendor, bt->vendor) &&
 				str_starts_with(product, bt->product) &&
 				str_starts_with(version, bt->version)) {
-			bios_cfg = bt;
+			found = 1;
 			break;
 		}
 	}
 
-	if (!bios_cfg) {
+	if (!found) {
 		pr_err("unknown (unsupported) BIOS version %s/%s/%s, please report, aborting!\n",
 		       vendor, product, version);
 		return -EINVAL;
 	}
 
+	/* Copy control settings from BIOS table before we free it. */
+	ctrl_cfg.fanreg = bt->fanreg;
+	ctrl_cfg.tempreg = bt->tempreg;
+	memcpy(&ctrl_cfg.cmd, &bt->cmd, sizeof(struct fancmd));
+	ctrl_cfg.mcmd_enable = bt->mcmd_enable;
+
 	/*
 	 * if started with kernel mode off, prevent the kernel from switching
 	 * off the fan
-- 
2.15.0

Re: [PATCH 0/6] platform/x86: acerhdf: new BIOS string, better modparam handling

[Peter Feuerer]

Hi,

21. September 2018 03:45, "Paul Gortmaker" <paul.gortmaker@windriver.com> schrieb:

> I came across this older netbook over the xmas holidays, and noticed the
> acerhdf driver wouldn't load. Turns out the BIOS string was too new,
> and not listed in the driver. There were module params for overrides,
> but I found their use isn't quite clear without reading the source.
> 
> Here I clarify some of the module parameter usage, and then add the new
> BIOS string, so they won't be needed for this particular platform. Some
> probe code is also shifted out to be __init to reduce the driver size a
> bit. Further details can be found in the respective commit logs.
> 
> Since it was months ago when I created these patches, I retested on top
> of tree to ensure things still work, as can be seen below.

Thanks a lot for your work and this set of changes.  This improves quality.
I reviewed all 6 patches.

Reviewed-by: Peter Feuerer <peter@piie.net>

Please also add CC: Boris Petkov <bp@suse.de> for review.

[...]

-- 
kind regards,
--peter;

Re: [PATCH 0/6] platform/x86: acerhdf: new BIOS string, better modparam handling

[Andy Shevchenko]

On Fri, Sep 21, 2018 at 9:20 AM Peter Feuerer <peter@piie.net> wrote:
>
> Hi,
>
> 21. September 2018 03:45, "Paul Gortmaker" <paul.gortmaker@windriver.com> schrieb:
>
> > I came across this older netbook over the xmas holidays, and noticed the
> > acerhdf driver wouldn't load. Turns out the BIOS string was too new,
> > and not listed in the driver. There were module params for overrides,
> > but I found their use isn't quite clear without reading the source.
> >
> > Here I clarify some of the module parameter usage, and then add the new
> > BIOS string, so they won't be needed for this particular platform. Some
> > probe code is also shifted out to be __init to reduce the driver size a
> > bit. Further details can be found in the respective commit logs.
> >
> > Since it was months ago when I created these patches, I retested on top
> > of tree to ensure things still work, as can be seen below.
>
> Thanks a lot for your work and this set of changes.  This improves quality.
> I reviewed all 6 patches.
>
> Reviewed-by: Peter Feuerer <peter@piie.net>
>
> Please also add CC: Boris Petkov <bp@suse.de> for review.

Shall I wait for Boris'  tags?

>
> [...]
>
> --
> kind regards,
> --peter;



-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 0/6] platform/x86: acerhdf: new BIOS string, better modparam handling

[Andy Shevchenko]

On Wed, Sep 26, 2018 at 2:09 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Fri, Sep 21, 2018 at 9:20 AM Peter Feuerer <peter@piie.net> wrote:
> >
> > Hi,
> >
> > 21. September 2018 03:45, "Paul Gortmaker" <paul.gortmaker@windriver.com> schrieb:
> >
> > > I came across this older netbook over the xmas holidays, and noticed the
> > > acerhdf driver wouldn't load. Turns out the BIOS string was too new,
> > > and not listed in the driver. There were module params for overrides,
> > > but I found their use isn't quite clear without reading the source.
> > >
> > > Here I clarify some of the module parameter usage, and then add the new
> > > BIOS string, so they won't be needed for this particular platform. Some
> > > probe code is also shifted out to be __init to reduce the driver size a
> > > bit. Further details can be found in the respective commit logs.
> > >
> > > Since it was months ago when I created these patches, I retested on top
> > > of tree to ensure things still work, as can be seen below.
> >
> > Thanks a lot for your work and this set of changes.  This improves quality.
> > I reviewed all 6 patches.
> >
> > Reviewed-by: Peter Feuerer <peter@piie.net>
> >
> > Please also add CC: Boris Petkov <bp@suse.de> for review.
>
> Shall I wait for Boris'  tags?

Anyway, all suggested changes look good to me, I go ahead and push
them to my review and testing queue.
If nothing happens during let's say a week, I will move them forward.

Thanks!

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 0/6] platform/x86: acerhdf: new BIOS string, better modparam handling

[Paul Gortmaker]

[Re: [PATCH 0/6] platform/x86: acerhdf: new BIOS string, better modparam handling] On 26/09/2018 (Wed 14:13) Andy Shevchenko wrote:

> On Wed, Sep 26, 2018 at 2:09 PM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:

[...]

> 
> Anyway, all suggested changes look good to me, I go ahead and push
> them to my review and testing queue.
> If nothing happens during let's say a week, I will move them forward.

Sounds good to me -- thanks!

P.