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@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx>
> ---
> This patch is for linux-pm bleeding-edge branch. Can be after patch:
> "
> commit 8526eb7fc75abcd09d8bd061610215baf0ca948a
> Author: Srinivas Pandruvada <srinivas.pandruvada@xxxxxxxxxxxxxxx>
> 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!