Re: [PATCH 2/3] hwmon (coretemp): Add support for dynamic tjmax

Re: [PATCH 2/3] hwmon (coretemp): Add support for dynamic tjmax – Guenter Roeck

From: Guenter Roeck <linux@roeck-us.net>
To: Zhang Rui <rui.zhang@intel.com>
Cc: jdelvare@suse.com, fenghua.yu@intel.com,
	linux-hwmon@vger.kernel.org, srinivas.pandruvada@linux.intel.com
Subject: Re: [PATCH 2/3] hwmon (coretemp): Add support for dynamic tjmax
Date: Fri, 11 Nov 2022 13:26:45 -0800	[thread overview]
Message-ID: <20221111212645.GA1059539@roeck-us.net> (raw)
In-Reply-To: <20221108075051.5139-3-rui.zhang@intel.com>

On Tue, Nov 08, 2022 at 03:50:50PM +0800, Zhang Rui wrote:
> Tjmax value retrieved from MSR_IA32_TEMPERATURE_TARGET can be changed at
> runtime when the Intel SST-PP (Intel Speed Select Technology -
> Performance Profile) level is changed.
> 
> Improve the code to always use updated tjmax when it can be retrieved
> from MSR_IA32_TEMPERATURE_TARGET.
> 
> When tjmax can not be retrieved from MSR_IA32_TEMPERATURE_TARGET, still
> follow the previous logic and always use a static tjmax value.
> 
> Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> ---
>  drivers/hwmon/coretemp.c | 40 ++++++++++++++++++++++++++++++----------
>  1 file changed, 30 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/hwmon/coretemp.c b/drivers/hwmon/coretemp.c
> index ec35ada68455..5292f7844860 100644
> --- a/drivers/hwmon/coretemp.c
> +++ b/drivers/hwmon/coretemp.c
> @@ -55,6 +55,8 @@ MODULE_PARM_DESC(tjmax, "TjMax value in degrees Celsius");
>  
>  /*
>   * Per-Core Temperature Data
> + * @tjmax: The static tjmax value when tjmax cannot be retrieved from
> + *		IA32_TEMPERATURE_TARGET MSR.
>   * @last_updated: The time when the current temperature value was updated
>   *		earlier (in jiffies).
>   * @cpu_core_id: The CPU Core from which temperature values should be read
> @@ -93,6 +95,8 @@ struct platform_data {
>  	struct device_attribute name_attr;
>  };
>  
> +static int get_tjmax(struct temp_data *tdata, struct device *dev);
> +

Please rearrange the code to avoid the forward declaration.

>  /* Keep track of how many zone pointers we allocated in init() */
>  static int max_zones __read_mostly;
>  /* Array of zone pointers. Serialized by cpu hotplug lock */
> @@ -131,8 +135,14 @@ static ssize_t show_tjmax(struct device *dev,
>  {
>  	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
>  	struct platform_data *pdata = dev_get_drvdata(dev);
> +	struct temp_data *tdata = pdata->core_data[attr->index];
> +	int tjmax;
> +
> +	mutex_lock(&tdata->update_lock);
> +	tjmax = get_tjmax(tdata, dev);
> +	mutex_unlock(&tdata->update_lock);
>  
> -	return sprintf(buf, "%d\n", pdata->core_data[attr->index]->tjmax);
> +	return sprintf(buf, "%d\n", tjmax);
>  }
>  
>  static ssize_t show_ttarget(struct device *dev,
> @@ -151,9 +161,11 @@ static ssize_t show_temp(struct device *dev,
>  	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
>  	struct platform_data *pdata = dev_get_drvdata(dev);
>  	struct temp_data *tdata = pdata->core_data[attr->index];
> +	int tjmax;
>  
>  	mutex_lock(&tdata->update_lock);
>  
> +	tjmax = get_tjmax(tdata, dev);
>  	/* Check whether the time interval has elapsed */
>  	if (time_after(jiffies, tdata->last_updated + HZ)) {
>  		rdmsr_on_cpu(tdata->cpu, tdata->status_reg, &eax, &edx);
> @@ -163,7 +175,7 @@ static ssize_t show_temp(struct device *dev,
>  		 * Return it instead of reporting an error which doesn't
>  		 * really help at all.
>  		 */
> -		tdata->temp = tdata->tjmax - ((eax >> 16) & 0x7f) * 1000;
> +		tdata->temp = tjmax - ((eax >> 16) & 0x7f) * 1000;
>  		tdata->last_updated = jiffies;
>  	}
>  
> @@ -334,20 +346,25 @@ static bool cpu_has_tjmax(struct cpuinfo_x86 *c)
>  	       model != 0x36;
>  }
>  
> -static int get_tjmax(struct cpuinfo_x86 *c, u32 id, struct device *dev)
> +static int get_tjmax(struct temp_data *tdata, struct device *dev)
>  {
> +	struct cpuinfo_x86 *c = &cpu_data(tdata->cpu);
>  	int err;
>  	u32 eax, edx;
>  	u32 val;
>  
> +	/* use static tjmax once it is set */
> +	if (tdata->tjmax)
> +		return tdata->tjmax;
> +
>  	/*
>  	 * A new feature of current Intel(R) processors, the
>  	 * IA32_TEMPERATURE_TARGET contains the TjMax value
>  	 */
> -	err = rdmsr_safe_on_cpu(id, MSR_IA32_TEMPERATURE_TARGET, &eax, &edx);
> +	err = rdmsr_safe_on_cpu(tdata->cpu, MSR_IA32_TEMPERATURE_TARGET, &eax, &edx);
>  	if (err) {
>  		if (cpu_has_tjmax(c))
> -			dev_warn(dev, "Unable to read TjMax from CPU %u\n", id);
> +			dev_warn(dev, "Unable to read TjMax from CPU %u\n", tdata->cpu);
>  	} else {
>  		val = (eax >> 16) & 0xff;
>  		/*
> @@ -363,14 +380,17 @@ static int get_tjmax(struct cpuinfo_x86 *c, u32 id, struct device *dev)
>  	if (force_tjmax) {
>  		dev_notice(dev, "TjMax forced to %d degrees C by user\n",
>  			   force_tjmax);
> -		return force_tjmax * 1000;
> +		tdata->tjmax = force_tjmax * 1000;
> +		goto end;

This added goto doesn't add any value and is just a manual replacement
for if/else. Please drop and return immediately, or use if/else.

	if (force_tjmax) {
		...
		tdata->tjmax = force_tjmax * 1000;
	} else {
		/*
		 * An assumption is made for early CPUs and unreadable MSR.
		 * NOTE: the calculated value may not be correct.
		 */
		tdata->tjmax = adjust_tjmax(c, tdata->cpu, dev);
	}
	return tdata->tjmax;

Thanks,
Guenter

>  	}
>  
>  	/*
>  	 * An assumption is made for early CPUs and unreadable MSR.
>  	 * NOTE: the calculated value may not be correct.
>  	 */
> -	return adjust_tjmax(c, id, dev);
> +	tdata->tjmax = adjust_tjmax(c, tdata->cpu, dev);
> +end:
> +	return tdata->tjmax;
>  }
>  
>  static int create_core_attrs(struct temp_data *tdata, struct device *dev,
> @@ -450,7 +470,7 @@ static int create_core_data(struct platform_device *pdev, unsigned int cpu,
>  	struct platform_data *pdata = platform_get_drvdata(pdev);
>  	struct cpuinfo_x86 *c = &cpu_data(cpu);
>  	u32 eax, edx;
> -	int err, index, attr_no;
> +	int err, index, attr_no, tjmax;
>  
>  	/*
>  	 * Find attr number for sysfs:
> @@ -485,7 +505,7 @@ static int create_core_data(struct platform_device *pdev, unsigned int cpu,
>  		goto exit_free;
>  
>  	/* We can access status register. Get Critical Temperature */
> -	tdata->tjmax = get_tjmax(c, cpu, &pdev->dev);
> +	tjmax = get_tjmax(tdata, &pdev->dev);
>  
>  	/*
>  	 * Read the still undocumented bits 8:15 of IA32_TEMPERATURE_TARGET.
> @@ -497,7 +517,7 @@ static int create_core_data(struct platform_device *pdev, unsigned int cpu,
>  					&eax, &edx);
>  		if (!err) {
>  			tdata->ttarget
> -			  = tdata->tjmax - ((eax >> 8) & 0xff) * 1000;
> +			  = tjmax - ((eax >> 8) & 0xff) * 1000;
>  			tdata->attr_size++;
>  		}
>  	}

next prev parent reply	other threads:[~2022-11-11 21:28 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-11-08  7:50 [PATCH 0/3] hwmon (coretemp): Add support for dynamic tjmax/ttarget Zhang Rui
2022-11-08  7:50 ` [PATCH 1/3] hwmon (coretemp): Remove obsolete temp_data->valid Zhang Rui
2022-11-11 21:14   ` Guenter Roeck
2022-11-08  7:50 ` [PATCH 2/3] hwmon (coretemp): Add support for dynamic tjmax Zhang Rui
2022-11-11 21:26   ` Guenter Roeck [this message]
2022-11-08  7:50 ` [PATCH 3/3] hwmon (coretemp): Add support for dynamic ttarget Zhang Rui
2022-11-11 21:34   ` Guenter Roeck
2022-11-12  8:37     ` Zhang Rui
2022-11-12 15:10       ` Guenter Roeck

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20221111212645.GA1059539@roeck-us.net \
    --to=linux@roeck-us.net \
    --cc=fenghua.yu@intel.com \
    --cc=jdelvare@suse.com \
    --cc=linux-hwmon@vger.kernel.org \
    --cc=rui.zhang@intel.com \
    --cc=srinivas.pandruvada@linux.intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Be sure your reply has a Subject: header at the top and a blank line
before the message body.


This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.

Read more here: Source link