Re: [PATCH] hwmon, fam15h_power: Add support for two more processors

[Aravind Gopalakrishnan <aravind.gopalakrishnan@amd.com>]

On 9/10/2014 3:37 PM, Guenter Roeck wrote:
> On Wed, Sep 10, 2014 at 03:01:36PM -0500, Aravind Gopalakrishnan wrote:
>> On 9/10/2014 12:53 PM, Guenter Roeck wrote:
>>> On Wed, Sep 10, 2014 at 12:02:08PM -0500, Aravind Gopalakrishnan wrote:
>>>> Fam16h,M30h(Mullins) and Fam15hM30h(Kaveri) processors can
>>>> report 'power_crit' value. So, adding their respective device ids.
>>>>
>>>> Also, according to BKDGs, the 'TdpRunAvgAccCap' that show_power()
>>>> uses is valid only on Fam15h, Models 0x0-0xF. On all other processors
>>>> the field is 'Reserved'. So, return error if we are on any other family/model.
>>>>
>>>> Impact on lm-sensors is minimal. On such families, instead of reporting
>>>> Current power value as '0', we now have:
>>>> power1:           N/A
>>>>
>>> It will result in people complaining to us about it.
>>>
>>> It would be more appropriate to not create the attribute the first place
>>> if it is not supported. Sure, that is a bit more code, but it isn't that bad.
>>> You can simply return -ENODEV for unsupported CPUs from the probe function.
>>>
>>>
>>>> Signed-off-by: Aravind Gopalakrishnan <aravind.gopalakrishnan@amd.com>
>>>> ---
>>>>   drivers/hwmon/fam15h_power.c | 6 ++++++
>>>>   1 file changed, 6 insertions(+)
>>>>
>>>> diff --git a/drivers/hwmon/fam15h_power.c b/drivers/hwmon/fam15h_power.c
>>>> index 4a7cbfa..b69bf7d 100644
>>>> --- a/drivers/hwmon/fam15h_power.c
>>>> +++ b/drivers/hwmon/fam15h_power.c
>>>> @@ -57,6 +57,10 @@ static ssize_t show_power(struct device *dev,
>>>>   	struct fam15h_power_data *data = dev_get_drvdata(dev);
>>>>   	struct pci_dev *f4 = data->pdev;
>>>> +	/* The value TdpRunAvgAccCap is valid only on F15h, Models 0x0-0xF */
>>>> +	if (boot_cpu_data.x86 != 0x15 || boot_cpu_data.x86_model > 0x0)
>>> The comment does not match the code. The comment talks about accepting models
>>> F15h, models 0x0-0xF, but the code rejects anything but F15h model 0x0.
>> Ah. Yes, The condition should have been (..boot_cpu_data.x86_model > 0xf)
>>
>>> Now it may well be that the above describes identifies all F15h and F16h CPUs,
>>> but this is not clear from the comment. It rather looks as if anything but F15h,
>>> model 0x0 is rejected, including all F16h CPUs. But then why accept F16h CPUs
>>> in the first place ?
>> Yes, we want to reject anything but F15h, Models 00h-0fh.
>> The reason I included the newer processor IDs, (and let
>> PCI_DEVICE_ID_AMD_16H_NB_F4) remain
>> is because we can still obtain 'critical power value'. It is only
>> the 'current power' that is not exposed.
>>
> That is a behavioral change, though; previously the current power was
> reported for F16h chips with PCI ID PCI_DEVICE_ID_AMD_16H_NB_F4.
> Is this a bug, ie should the power value not have been reported
> for the F16h chips ?

That's right.

>> If we return -ENODEV in the probe function (or we can just remove
>> the listed PCI_DEVICE_ID), then we'd not get the critical power
>> values too.
>>
> If you want to make the actual power reporting conditional, you should
> introduce an is_visible function to the attribute group to ensure that
> power1_input is only reported if/when supported. If the actual power
> value is not really supported for F16h chips, you should actually provide
> two separate patches: One to make power1_input optional, to be reported for
> supported chips only, and another to add more chips. One is a bug fix,
> the other a functionality extension.
>

Ok, I'll do that and resend.

Thanks,
-Aravind.

>
>>>> +		return -ENOSYS;
>>>> +
>>>>   	pci_bus_read_config_dword(f4->bus, PCI_DEVFN(PCI_SLOT(f4->devfn), 5),
>>>>   				  REG_TDP_RUNNING_AVERAGE, &val);
>>>>   	running_avg_capture = (val >> 4) & 0x3fffff;
>>>> @@ -216,7 +220,9 @@ static int fam15h_power_probe(struct pci_dev *pdev,
>>>>   static const struct pci_device_id fam15h_power_id_table[] = {
>>>>   	{ PCI_VDEVICE(AMD, PCI_DEVICE_ID_AMD_15H_NB_F4) },
>>>> +	{ PCI_VDEVICE(AMD, PCI_DEVICE_ID_AMD_15H_M30H_NB_F4) },
>>>>   	{ PCI_VDEVICE(AMD, PCI_DEVICE_ID_AMD_16H_NB_F4) },
>>>> +	{ PCI_VDEVICE(AMD, PCI_DEVICE_ID_AMD_16H_M30H_NB_F3) },
>>>>   	{}
>>>>   };
>>>>   MODULE_DEVICE_TABLE(pci, fam15h_power_id_table);
>>>> -- 
>>>> 2.0.3
>>>>