[PATCH 1/2] platform/x86/drivers/acerhdf: Use module_param_cb to set/get polling interval

[PATCH 1/2] platform/x86/drivers/acerhdf: Use module_param_cb to set/get polling interval

[Daniel Lezcano]

The module parameter can be set by using ops to get and set the
values. The change will allow to check the correctness of the interval
value everytime it is changed instead of checking in the get_temp
function.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 drivers/platform/x86/acerhdf.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/platform/x86/acerhdf.c b/drivers/platform/x86/acerhdf.c
index 44b6bfbd32df..19fc8ff2225c 100644
--- a/drivers/platform/x86/acerhdf.c
+++ b/drivers/platform/x86/acerhdf.c
@@ -84,8 +84,6 @@ static struct platform_device *acerhdf_dev;
 
 module_param(kernelmode, uint, 0);
 MODULE_PARM_DESC(kernelmode, "Kernel mode fan control on / off");
-module_param(interval, uint, 0600);
-MODULE_PARM_DESC(interval, "Polling interval of temperature check");
 module_param(fanon, uint, 0600);
 MODULE_PARM_DESC(fanon, "Turn the fan on above this temperature");
 module_param(fanoff, uint, 0600);
@@ -824,3 +822,11 @@ MODULE_ALIAS("dmi:*:*Acer*:pnExtensa*5420*:");
 
 module_init(acerhdf_init);
 module_exit(acerhdf_exit);
+
+static const struct kernel_param_ops interval_ops = {
+	.set = param_set_uint,
+	.get = param_get_uint,
+};
+
+module_param_cb(interval, &interval_ops, &interval, 0600);
+MODULE_PARM_DESC(interval, "Polling interval of temperature check");
-- 
2.25.1

[PATCH 2/2] platform/x86/drivers/acerhdf: Check the interval value when it is set

[Daniel Lezcano]

Currently the code checks the interval value when the temperature is
read which is bad for two reasons:

 - checking and setting the interval in the get_temp callback is
   inaccurate and awful, that can be done when changing the value.

 - Changing the thermal zone structure internals is an abuse of the
   exported structure, moreover no lock is taken here.

The goal of this patch is to solve the first item by using the 'set'
function called when changing the interval. The check is done there
and removed from the get_temp function. If the thermal zone was not
initialized yet, the interval is not updated in this case as that will
happen in the init function when registering the thermal zone device.

I don't have any hardware to test the changes.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 drivers/platform/x86/acerhdf.c | 20 ++++++++++++++++----
 1 file changed, 16 insertions(+), 4 deletions(-)

diff --git a/drivers/platform/x86/acerhdf.c b/drivers/platform/x86/acerhdf.c
index 19fc8ff2225c..084005841d56 100644
--- a/drivers/platform/x86/acerhdf.c
+++ b/drivers/platform/x86/acerhdf.c
@@ -334,7 +334,10 @@ static void acerhdf_check_param(struct thermal_zone_device *thermal)
 		}
 		if (verbose)
 			pr_notice("interval changed to: %d\n", interval);
-		thermal->polling_delay = interval*1000;
+
+		if (thermal)
+			thermal->polling_delay = interval*1000;
+
 		prev_interval = interval;
 	}
 }
@@ -349,8 +352,6 @@ static int acerhdf_get_ec_temp(struct thermal_zone_device *thermal, int *t)
 {
 	int temp, err = 0;
 
-	acerhdf_check_param(thermal);
-
 	err = acerhdf_get_temp(&temp);
 	if (err)
 		return err;
@@ -823,8 +824,19 @@ MODULE_ALIAS("dmi:*:*Acer*:pnExtensa*5420*:");
 module_init(acerhdf_init);
 module_exit(acerhdf_exit);
 
+int interval_set_uint(const char *val, const struct kernel_param *kp)
+{
+	ret = param_set_uint(val, kp);
+	if (ret)
+		return ret;
+
+	acerhdf_check_param(thz_dev);
+
+	return 0;
+}
+
 static const struct kernel_param_ops interval_ops = {
-	.set = param_set_uint,
+	.set = interval_set_uint,
 	.get = param_get_uint,
 };
 
-- 
2.25.1

Re: [PATCH 2/2] platform/x86/drivers/acerhdf: Check the interval value when it is set

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 2379 bytes --]

Hi Daniel,

I love your patch! Perhaps something to improve:

[auto build test WARNING on linus/master]
[also build test WARNING on v5.10-rc6 next-20201201]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Daniel-Lezcano/platform-x86-drivers-acerhdf-Use-module_param_cb-to-set-get-polling-interval/20201203-062450
base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 3bb61aa61828499a7d0f5e560051625fd02ae7e4
config: x86_64-randconfig-a004-20201202 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
reproduce (this is a W=1 build):
        # https://github.com/0day-ci/linux/commit/40fa9177042a71ae3df7c776d693d27bce63fce8
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Daniel-Lezcano/platform-x86-drivers-acerhdf-Use-module_param_cb-to-set-get-polling-interval/20201203-062450
        git checkout 40fa9177042a71ae3df7c776d693d27bce63fce8
        # save the attached .config to linux build tree
        make W=1 ARCH=x86_64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> drivers/platform/x86/acerhdf.c:827:5: warning: no previous prototype for 'interval_set_uint' [-Wmissing-prototypes]
     827 | int interval_set_uint(const char *val, const struct kernel_param *kp)
         |     ^~~~~~~~~~~~~~~~~
   drivers/platform/x86/acerhdf.c: In function 'interval_set_uint':
   drivers/platform/x86/acerhdf.c:829:2: error: 'ret' undeclared (first use in this function)
     829 |  ret = param_set_uint(val, kp);
         |  ^~~
   drivers/platform/x86/acerhdf.c:829:2: note: each undeclared identifier is reported only once for each function it appears in

vim +/interval_set_uint +827 drivers/platform/x86/acerhdf.c

   826	
 > 827	int interval_set_uint(const char *val, const struct kernel_param *kp)
   828	{
   829		ret = param_set_uint(val, kp);
   830		if (ret)
   831			return ret;
   832	
   833		acerhdf_check_param(thz_dev);
   834	
   835		return 0;
   836	}
   837	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 38066 bytes --]

Re: [PATCH 2/2] platform/x86/drivers/acerhdf: Check the interval value when it is set

[Peter Kästle]

2. Dezember 2020 23:22, "Daniel Lezcano" <daniel.lezcano@linaro.org> schrieb:

> Currently the code checks the interval value when the temperature is
> read which is bad for two reasons:
> 
> - checking and setting the interval in the get_temp callback is
> inaccurate and awful, that can be done when changing the value.
> 
> - Changing the thermal zone structure internals is an abuse of the
> exported structure, moreover no lock is taken here.
> 
> The goal of this patch is to solve the first item by using the 'set'
> function called when changing the interval. The check is done there
> and removed from the get_temp function. If the thermal zone was not
> initialized yet, the interval is not updated in this case as that will
> happen in the init function when registering the thermal zone device.
> 
> I don't have any hardware to test the changes.

Thanks for this patch, I'll test it in upcoming days.