Messages in this thread | | | Date | Wed, 10 Sep 2014 16:23:35 -0500 | From | Aravind Gopalakrishnan <> | Subject | Re: [PATCH] hwmon, fam15h_power: Add support for two more processors |
| |
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 >>>>
| |