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

[PATCH v2 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 v2 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>
---
 V2:
   - Fixed static function annotation
---
 drivers/platform/x86/acerhdf.c | 22 ++++++++++++++++++----
 1 file changed, 18 insertions(+), 4 deletions(-)

diff --git a/drivers/platform/x86/acerhdf.c b/drivers/platform/x86/acerhdf.c
index 19fc8ff2225c..b6aa6e5514f4 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,21 @@ MODULE_ALIAS("dmi:*:*Acer*:pnExtensa*5420*:");
 module_init(acerhdf_init);
 module_exit(acerhdf_exit);
 
+static int interval_set_uint(const char *val, const struct kernel_param *kp)
+{
+	int ret;
+
+	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 v2 1/2] platform/x86/drivers/acerhdf: Use module_param_cb to set/get polling interval

[Peter Kästle]

3. Dezember 2020 08:17, "Daniel Lezcano" <daniel.lezcano@linaro.org> schrieb:

> 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>

Acked-by: Peter Kaestle <peter@piie.net>

> ---
> 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

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

[Peter Kästle]

3. Dezember 2020 08:17, "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.

Thanks for your effort.  This improves the code, good finding.

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

Tests successfully executed on my good old AOA110.


> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>

Acked-by: Peter Kaestle <peter@piie.net>


> ---
> V2:
> - Fixed static function annotation
> ---
> drivers/platform/x86/acerhdf.c | 22 ++++++++++++++++++----
> 1 file changed, 18 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/platform/x86/acerhdf.c b/drivers/platform/x86/acerhdf.c
> index 19fc8ff2225c..b6aa6e5514f4 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,21 @@ MODULE_ALIAS("dmi:*:*Acer*:pnExtensa*5420*:");
> module_init(acerhdf_init);
> module_exit(acerhdf_exit);
> 
> +static int interval_set_uint(const char *val, const struct kernel_param *kp)
> +{
> + int ret;
> +
> + 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 v2 2/2] platform/x86/drivers/acerhdf: Check the interval value when it is set

[Daniel Lezcano]

On 03/12/2020 22:22, Peter Kästle wrote:
> 3. Dezember 2020 08:17, "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.
> 
> Thanks for your effort.  This improves the code, good finding.
> 
>  
>> I don't have any hardware to test the changes.
> 
> Tests successfully executed on my good old AOA110.
> 
> 
>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> 
> Acked-by: Peter Kaestle <peter@piie.net>

Thanks for testing the changes.

Shall pick the patches through the thermal tree ?


-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

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

[Hans de Goede]

Hi,

On 12/4/20 12:43 PM, Daniel Lezcano wrote:
> On 03/12/2020 22:22, Peter Kästle wrote:
>> 3. Dezember 2020 08:17, "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.
>>
>> Thanks for your effort.  This improves the code, good finding.
>>
>>  
>>> I don't have any hardware to test the changes.
>>
>> Tests successfully executed on my good old AOA110.
>>
>>
>>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
>>
>> Acked-by: Peter Kaestle <peter@piie.net>
> 
> Thanks for testing the changes.
> 
> Shall pick the patches through the thermal tree ?

I can take them through the drivers/platform/x86 (pdx86) tree,
but if you prefer to take them upstream through the thermal tree,
then that is fine too...

Here is my ack (as pdx86 maintainer) for taking them through
the thermal tree:

Acked-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans

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

[Daniel Lezcano]

On 07/12/2020 15:54, Hans de Goede wrote:
> Hi,
> 
> On 12/4/20 12:43 PM, Daniel Lezcano wrote:
>> On 03/12/2020 22:22, Peter Kästle wrote:
>>> 3. Dezember 2020 08:17, "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.
>>>
>>> Thanks for your effort.  This improves the code, good finding.
>>>
>>>  
>>>> I don't have any hardware to test the changes.
>>>
>>> Tests successfully executed on my good old AOA110.
>>>
>>>
>>>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
>>>
>>> Acked-by: Peter Kaestle <peter@piie.net>
>>
>> Thanks for testing the changes.
>>
>> Shall pick the patches through the thermal tree ?
> 
> I can take them through the drivers/platform/x86 (pdx86) tree,
> but if you prefer to take them upstream through the thermal tree,
> then that is fine too...
> 
> Here is my ack (as pdx86 maintainer) for taking them through
> the thermal tree:
> 
> Acked-by: Hans de Goede <hdegoede@redhat.com>

Thanks. I'll take them through the thermal tree.

  -- Daniel


-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog