Re: [PATCH 5/6] hwmon: (k10temp): Don't show Tdie for all Zen/Zen2/Zen3 CPU/APU

Re: [PATCH 5/6] hwmon: (k10temp): Don't show Tdie for all Zen/Zen2/Zen3 CPU/APU

[Guenter Roeck]

On Thu, Aug 26, 2021 at 01:40:56PM -0500, Mario Limonciello wrote:
> Tdie is an offset calculation that should only be shown when temp_offset
> is actually put into a table.  This is useless to show for all CPU/APU.
> Show it only when necessary.
> 
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>

Applied.

I won't apply the other patches of the series because they either have issues
or need approval from x86/pci maintainers. No reason to hold up this one
or the first patch of the series, though.

Side note: I accepted this patch because it _seems_ like hwinfo64
does something similar. I do hope, though, that the assertion made
in the patch description is correct and that this doesn't miss CPUs
where there is a (logical or real) difference between Tctl and Tdie.

Guenter

> ---
>  drivers/hwmon/k10temp.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/hwmon/k10temp.c b/drivers/hwmon/k10temp.c
> index 41d9c0c0a1f1..e8ec0e36fc3b 100644
> --- a/drivers/hwmon/k10temp.c
> +++ b/drivers/hwmon/k10temp.c
> @@ -435,7 +435,6 @@ static int k10temp_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  	} else if (boot_cpu_data.x86 == 0x17 || boot_cpu_data.x86 == 0x18) {
>  		data->temp_adjust_mask = ZEN_CUR_TEMP_RANGE_SEL_MASK;
>  		data->read_tempreg = read_tempreg_nb_zen;
> -		data->show_temp |= BIT(TDIE_BIT);	/* show Tdie */
>  		data->is_zen = true;
>  
>  		switch (boot_cpu_data.x86_model) {
> @@ -457,7 +456,6 @@ static int k10temp_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  	} else if (boot_cpu_data.x86 == 0x19) {
>  		data->temp_adjust_mask = ZEN_CUR_TEMP_RANGE_SEL_MASK;
>  		data->read_tempreg = read_tempreg_nb_zen;
> -		data->show_temp |= BIT(TDIE_BIT);
>  		data->is_zen = true;
>  
>  		switch (boot_cpu_data.x86_model) {
> @@ -478,6 +476,7 @@ static int k10temp_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  
>  		if (boot_cpu_data.x86 == entry->model &&
>  		    strstr(boot_cpu_data.x86_model_id, entry->id)) {
> +			data->show_temp |= BIT(TDIE_BIT);	/* show Tdie */
>  			data->temp_offset = entry->offset;
>  			break;
>  		}
> -- 
> 2.25.1
> 

Re: [PATCH 5/6] hwmon: (k10temp): Don't show Tdie for all Zen/Zen2/Zen3 CPU/APU

[Limonciello, Mario]

On 8/27/2021 11:48, Guenter Roeck wrote:
> On Thu, Aug 26, 2021 at 01:40:56PM -0500, Mario Limonciello wrote:
>> Tdie is an offset calculation that should only be shown when temp_offset
>> is actually put into a table.  This is useless to show for all CPU/APU.
>> Show it only when necessary.
>>
>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> 
> Applied.

Really appreciate the expedient review.

> 
> I won't apply the other patches of the series because they either have issues
> or need approval from x86/pci maintainers. No reason to hold up this one
> or the first patch of the series, though.

Thanks.  I've sent v2 to accommodate suggested changes and to loop in 
other maintainers on the applicable patches since as you point out some 
files cross subsystems.

> 
> Side note: I accepted this patch because it _seems_ like hwinfo64
> does something similar. I do hope, though, that the assertion made
> in the patch description is correct and that this doesn't miss CPUs
> where there is a (logical or real) difference between Tctl and Tdie.
> 

If there does turn out to be a system that these are different, then 
showing both as the same value will certainly send a confusing message. 
  If such a case is discovered we should be adding to the table (which 
will bring back Tdie for those products).

> Guenter
> 
>> ---
>>   drivers/hwmon/k10temp.c | 3 +--
>>   1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/drivers/hwmon/k10temp.c b/drivers/hwmon/k10temp.c
>> index 41d9c0c0a1f1..e8ec0e36fc3b 100644
>> --- a/drivers/hwmon/k10temp.c
>> +++ b/drivers/hwmon/k10temp.c
>> @@ -435,7 +435,6 @@ static int k10temp_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>>   	} else if (boot_cpu_data.x86 == 0x17 || boot_cpu_data.x86 == 0x18) {
>>   		data->temp_adjust_mask = ZEN_CUR_TEMP_RANGE_SEL_MASK;
>>   		data->read_tempreg = read_tempreg_nb_zen;
>> -		data->show_temp |= BIT(TDIE_BIT);	/* show Tdie */
>>   		data->is_zen = true;
>>   
>>   		switch (boot_cpu_data.x86_model) {
>> @@ -457,7 +456,6 @@ static int k10temp_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>>   	} else if (boot_cpu_data.x86 == 0x19) {
>>   		data->temp_adjust_mask = ZEN_CUR_TEMP_RANGE_SEL_MASK;
>>   		data->read_tempreg = read_tempreg_nb_zen;
>> -		data->show_temp |= BIT(TDIE_BIT);
>>   		data->is_zen = true;
>>   
>>   		switch (boot_cpu_data.x86_model) {
>> @@ -478,6 +476,7 @@ static int k10temp_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>>   
>>   		if (boot_cpu_data.x86 == entry->model &&
>>   		    strstr(boot_cpu_data.x86_model_id, entry->id)) {
>> +			data->show_temp |= BIT(TDIE_BIT);	/* show Tdie */
>>   			data->temp_offset = entry->offset;
>>   			break;
>>   		}
>> -- 
>> 2.25.1
>>

[PATCH 0/6] Add k10temp support for more client APUs

[Mario Limonciello]

Extend existing code to cover more missing client APUs.  Some logic changes
and failure handling will make this more scalable for other future APUs as
well.

Mario Limonciello (6):
  hwmon: (k10temp): Add additional missing Zen2 and Zen3 APUs
  x86/amd_nb: Rename PCI_DEVICE_ID_AMD_19H_DF_F3 to match specific model
  hwmon: (k10temp): Rework the temperature offset calculation
  hwmon: (k10temp): Show errors failing to read
  hwmon: (k10temp): Don't show Tdie for all Zen/Zen2/Zen3 CPU/APU
  hwmon: (k10temp): Add support for yellow carp

 arch/x86/kernel/amd_nb.c |  7 ++++++-
 drivers/hwmon/k10temp.c  | 44 ++++++++++++++++++++++++++++------------
 include/linux/pci_ids.h  |  3 ++-
 3 files changed, 39 insertions(+), 15 deletions(-)

-- 
2.25.1

[PATCH 5/6] hwmon: (k10temp): Don't show Tdie for all Zen/Zen2/Zen3 CPU/APU

[Mario Limonciello]

Tdie is an offset calculation that should only be shown when temp_offset
is actually put into a table.  This is useless to show for all CPU/APU.
Show it only when necessary.

Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/hwmon/k10temp.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/hwmon/k10temp.c b/drivers/hwmon/k10temp.c
index 41d9c0c0a1f1..e8ec0e36fc3b 100644
--- a/drivers/hwmon/k10temp.c
+++ b/drivers/hwmon/k10temp.c
@@ -435,7 +435,6 @@ static int k10temp_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	} else if (boot_cpu_data.x86 == 0x17 || boot_cpu_data.x86 == 0x18) {
 		data->temp_adjust_mask = ZEN_CUR_TEMP_RANGE_SEL_MASK;
 		data->read_tempreg = read_tempreg_nb_zen;
-		data->show_temp |= BIT(TDIE_BIT);	/* show Tdie */
 		data->is_zen = true;
 
 		switch (boot_cpu_data.x86_model) {
@@ -457,7 +456,6 @@ static int k10temp_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	} else if (boot_cpu_data.x86 == 0x19) {
 		data->temp_adjust_mask = ZEN_CUR_TEMP_RANGE_SEL_MASK;
 		data->read_tempreg = read_tempreg_nb_zen;
-		data->show_temp |= BIT(TDIE_BIT);
 		data->is_zen = true;
 
 		switch (boot_cpu_data.x86_model) {
@@ -478,6 +476,7 @@ static int k10temp_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 
 		if (boot_cpu_data.x86 == entry->model &&
 		    strstr(boot_cpu_data.x86_model_id, entry->id)) {
+			data->show_temp |= BIT(TDIE_BIT);	/* show Tdie */
 			data->temp_offset = entry->offset;
 			break;
 		}
-- 
2.25.1