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 <[email protected]>
Cc: Darren Hart <[email protected]>
Cc: Andy Shevchenko <[email protected]>
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
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 <[email protected]>
Cc: Darren Hart <[email protected]>
Cc: Andy Shevchenko <[email protected]>
Signed-off-by: Paul Gortmaker <[email protected]>
---
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
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 <[email protected]>
Cc: Darren Hart <[email protected]>
Cc: Andy Shevchenko <[email protected]>
Signed-off-by: Paul Gortmaker <[email protected]>
---
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
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 <[email protected]>
Cc: Darren Hart <[email protected]>
Cc: Andy Shevchenko <[email protected]>
Signed-off-by: Paul Gortmaker <[email protected]>
---
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
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 <[email protected]>
Cc: Darren Hart <[email protected]>
Cc: Andy Shevchenko <[email protected]>
Signed-off-by: Paul Gortmaker <[email protected]>
---
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
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 <[email protected]>
Cc: Darren Hart <[email protected]>
Cc: Andy Shevchenko <[email protected]>
Signed-off-by: Paul Gortmaker <[email protected]>
---
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
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 <[email protected]>
Cc: Darren Hart <[email protected]>
Cc: Andy Shevchenko <[email protected]>
Signed-off-by: Paul Gortmaker <[email protected]>
---
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
Hi,
21. September 2018 03:45, "Paul Gortmaker" <[email protected]> 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 <[email protected]>
Please also add CC: Boris Petkov <[email protected]> for review.
[...]
--
kind regards,
--peter;
On Fri, Sep 21, 2018 at 9:20 AM Peter Feuerer <[email protected]> wrote:
>
> Hi,
>
> 21. September 2018 03:45, "Paul Gortmaker" <[email protected]> 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 <[email protected]>
>
> Please also add CC: Boris Petkov <[email protected]> for review.
Shall I wait for Boris' tags?
>
> [...]
>
> --
> kind regards,
> --peter;
--
With Best Regards,
Andy Shevchenko
On Wed, Sep 26, 2018 at 2:09 PM Andy Shevchenko
<[email protected]> wrote:
>
> On Fri, Sep 21, 2018 at 9:20 AM Peter Feuerer <[email protected]> wrote:
> >
> > Hi,
> >
> > 21. September 2018 03:45, "Paul Gortmaker" <[email protected]> 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 <[email protected]>
> >
> > Please also add CC: Boris Petkov <[email protected]> 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] On 26/09/2018 (Wed 14:13) Andy Shevchenko wrote:
> On Wed, Sep 26, 2018 at 2:09 PM Andy Shevchenko
> <[email protected]> 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.