[PATCH] thermal/drivers/intel_powerclamp: Fix module param for duration

[PATCH] thermal/drivers/intel_powerclamp: Fix module param for duration

[Srinivas Pandruvada]

After switch to use powercap/idle-inject framework in the Intel
powerclamp driver, the idle "duration" is changed to micro seconds.

But the module parameter for idle "duration" is in milli seconds. So,
convert to micro seconds in the set callback and convert back to milli
seconds by adding a get callback.

While here also use mutex protection for setting and getting "duration".
The other uses of "duration" is already protected by mutex.

Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
---
This patch is for linux-pm bleeding-edge branch. Can be after patch:
"
commit 8526eb7fc75abcd09d8bd061610215baf0ca948a
Author: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
Date:   Wed Feb 1 10:28:53 2023 -0800

    thermal: intel: powerclamp: Use powercap idle-inject feature
"
Can't add Fixes tag as commit ID will change once merged to mainline.
Or wait till the above patch is merged to mainline.

 drivers/thermal/intel/intel_powerclamp.c | 24 ++++++++++++++++++------
 1 file changed, 18 insertions(+), 6 deletions(-)

diff --git a/drivers/thermal/intel/intel_powerclamp.c b/drivers/thermal/intel/intel_powerclamp.c
index 1390748706a6..038acccc0509 100644
--- a/drivers/thermal/intel/intel_powerclamp.c
+++ b/drivers/thermal/intel/intel_powerclamp.c
@@ -74,6 +74,7 @@ static struct thermal_cooling_device *cooling_dev;
 
 static DEFINE_MUTEX(powerclamp_lock);
 
+/* This duration is in micro seconds */
 static unsigned int duration;
 static unsigned int pkg_cstate_ratio_cur;
 static unsigned int window_size;
@@ -90,23 +91,34 @@ static int duration_set(const char *arg, const struct kernel_param *kp)
 		pr_err("Out of recommended range %lu, between 6-25ms\n",
 			new_duration);
 		ret = -EINVAL;
+		goto exit;
 	}
 
-	duration = clamp(new_duration, 6ul, 25ul);
-	smp_mb();
-
+	mutex_lock(&powerclamp_lock);
+	duration = clamp(new_duration, 6ul, 25ul) * 1000;
+	mutex_unlock(&powerclamp_lock);
 exit:
 
 	return ret;
 }
 
+static int duration_get(char *buf, const struct kernel_param *kp)
+{
+	int ret;
+
+	mutex_lock(&powerclamp_lock);
+	ret = sysfs_emit(buf, "%d\n", duration / 1000);
+	mutex_unlock(&powerclamp_lock);
+
+	return ret;
+}
+
 static const struct kernel_param_ops duration_ops = {
 	.set = duration_set,
-	.get = param_get_int,
+	.get = duration_get,
 };
 
-
-module_param_cb(duration, &duration_ops, &duration, 0644);
+module_param_cb(duration, &duration_ops, NULL, 0644);
 MODULE_PARM_DESC(duration, "forced idle time for each attempt in msec.");
 
 struct powerclamp_calibration_data {
-- 
2.39.1

Re: [PATCH] thermal/drivers/intel_powerclamp: Fix module param for duration

[Rafael J. Wysocki]

On Wed, Feb 8, 2023 at 8:09 AM Srinivas Pandruvada
<srinivas.pandruvada@linux.intel.com> wrote:
>
> After switch to use powercap/idle-inject framework in the Intel
> powerclamp driver, the idle "duration" is changed to micro seconds.
>
> But the module parameter for idle "duration" is in milli seconds. So,
> convert to micro seconds in the set callback and convert back to milli
> seconds by adding a get callback.
>
> While here also use mutex protection for setting and getting "duration".
> The other uses of "duration" is already protected by mutex.
>
> Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
> ---
> This patch is for linux-pm bleeding-edge branch. Can be after patch:
> "
> commit 8526eb7fc75abcd09d8bd061610215baf0ca948a
> Author: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
> Date:   Wed Feb 1 10:28:53 2023 -0800
>
>     thermal: intel: powerclamp: Use powercap idle-inject feature
> "
> Can't add Fixes tag as commit ID will change once merged to mainline.
> Or wait till the above patch is merged to mainline.
>
>  drivers/thermal/intel/intel_powerclamp.c | 24 ++++++++++++++++++------
>  1 file changed, 18 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/thermal/intel/intel_powerclamp.c b/drivers/thermal/intel/intel_powerclamp.c
> index 1390748706a6..038acccc0509 100644
> --- a/drivers/thermal/intel/intel_powerclamp.c
> +++ b/drivers/thermal/intel/intel_powerclamp.c
> @@ -74,6 +74,7 @@ static struct thermal_cooling_device *cooling_dev;
>
>  static DEFINE_MUTEX(powerclamp_lock);
>
> +/* This duration is in micro seconds */
>  static unsigned int duration;
>  static unsigned int pkg_cstate_ratio_cur;
>  static unsigned int window_size;
> @@ -90,23 +91,34 @@ static int duration_set(const char *arg, const struct kernel_param *kp)
>                 pr_err("Out of recommended range %lu, between 6-25ms\n",
>                         new_duration);
>                 ret = -EINVAL;
> +               goto exit;
>         }
>
> -       duration = clamp(new_duration, 6ul, 25ul);
> -       smp_mb();
> -
> +       mutex_lock(&powerclamp_lock);
> +       duration = clamp(new_duration, 6ul, 25ul) * 1000;
> +       mutex_unlock(&powerclamp_lock);
>  exit:
>
>         return ret;
>  }
>
> +static int duration_get(char *buf, const struct kernel_param *kp)
> +{
> +       int ret;
> +
> +       mutex_lock(&powerclamp_lock);
> +       ret = sysfs_emit(buf, "%d\n", duration / 1000);
> +       mutex_unlock(&powerclamp_lock);
> +
> +       return ret;
> +}
> +
>  static const struct kernel_param_ops duration_ops = {
>         .set = duration_set,
> -       .get = param_get_int,
> +       .get = duration_get,
>  };
>
> -
> -module_param_cb(duration, &duration_ops, &duration, 0644);
> +module_param_cb(duration, &duration_ops, NULL, 0644);
>  MODULE_PARM_DESC(duration, "forced idle time for each attempt in msec.");
>
>  struct powerclamp_calibration_data {
> --

Applied with some edits in the subject and changelog, thanks!