[lm-sensors] [PATCH v2] hwmon, fam15h_power: Tweak runavg_range on resume

[lm-sensors] [PATCH v2] hwmon, fam15h_power: Tweak runavg_range on resume

[Andreas Herrmann]

The quirk introduced with commit
00250ec90963b7ef6678438888f3244985ecde14 (hwmon: fam15h_power: fix
bogus values with current BIOSes) is not only required during driver
load but also when system resumes from suspend. The BIOS might set the
previously recommended (but unsuitable) initilization value for the
running average range register during resume.

Signed-off-by: Andreas Herrmann <andreas.herrmann3@amd.com>
---
 drivers/hwmon/fam15h_power.c |   15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

Hi Jean,

Thanks for spotting the issues.
Here's a new patch version, esp. to fix the section mismatch.

Please apply.


Thanks,

Andreas

diff --git a/drivers/hwmon/fam15h_power.c b/drivers/hwmon/fam15h_power.c
index 2764b78a784b..af69073b3fe8 100644
--- a/drivers/hwmon/fam15h_power.c
+++ b/drivers/hwmon/fam15h_power.c
@@ -129,12 +129,12 @@ static bool __devinit fam15h_power_is_internal_node0(struct pci_dev *f4)
  * counter saturations resulting in bogus power readings.
  * We correct this value ourselves to cope with older BIOSes.
  */
-static DEFINE_PCI_DEVICE_TABLE(affected_device) = {
+static const struct pci_device_id affected_device[] = {
 	{ PCI_VDEVICE(AMD, PCI_DEVICE_ID_AMD_15H_NB_F4) },
 	{ 0 }
 };
 
-static void __devinit tweak_runavg_range(struct pci_dev *pdev)
+static void tweak_runavg_range(struct pci_dev *pdev)
 {
 	u32 val;
 
@@ -158,6 +158,16 @@ static void __devinit tweak_runavg_range(struct pci_dev *pdev)
 		REG_TDP_RUNNING_AVERAGE, val);
 }
 
+#ifdef CONFIG_PM
+static int fam15h_power_resume(struct pci_dev *pdev)
+{
+	tweak_runavg_range(pdev);
+	return 0;
+}
+#else
+#define fam15h_power_resume NULL
+#endif
+
 static void __devinit fam15h_power_init_data(struct pci_dev *f4,
 					     struct fam15h_power_data *data)
 {
@@ -256,6 +266,7 @@ static struct pci_driver fam15h_power_driver = {
 	.id_table = fam15h_power_id_table,
 	.probe = fam15h_power_probe,
 	.remove = __devexit_p(fam15h_power_remove),
+	.resume = fam15h_power_resume,
 };
 
 module_pci_driver(fam15h_power_driver);
-- 
1.7.10.4



_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH v2] hwmon, fam15h_power: Tweak runavg_range on resume

[Andreas Hartmann]

Hello Andreas,

Andreas Herrmann wrote:
> 
> The quirk introduced with commit
> 00250ec90963b7ef6678438888f3244985ecde14 (hwmon: fam15h_power: fix
> bogus values with current BIOSes) is not only required during driver
> load but also when system resumes from suspend. The BIOS might set the
> previously recommended (but unsuitable) initilization value for the
> running average range register during resume.
> 
> Signed-off-by: Andreas Herrmann <andreas.herrmann3@amd.com>


I could test your patch successfully with kernel 3.4.11. The reported
power consumption

fam15h_power-pci-00c4
Adapter: PCI adapter
power1:       28.45 W

works now as expected.


Thanks,
kind regards,
Andreas Hartmann


_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH v2] hwmon, fam15h_power: Tweak runavg_range on resume

[Guenter Roeck]

On Thu, Sep 20, 2012 at 12:34:08PM +0200, Andreas Herrmann wrote:
> 
> The quirk introduced with commit
> 00250ec90963b7ef6678438888f3244985ecde14 (hwmon: fam15h_power: fix
> bogus values with current BIOSes) is not only required during driver
> load but also when system resumes from suspend. The BIOS might set the
> previously recommended (but unsuitable) initilization value for the
> running average range register during resume.
> 
> Signed-off-by: Andreas Herrmann <andreas.herrmann3@amd.com>
> ---
>  drivers/hwmon/fam15h_power.c |   15 +++++++++++++--
>  1 file changed, 13 insertions(+), 2 deletions(-)
> 
> Hi Jean,
> 
> Thanks for spotting the issues.
> Here's a new patch version, esp. to fix the section mismatch.
> 
> Please apply.
> 
> 
> Thanks,
> 
> Andreas
> 
> diff --git a/drivers/hwmon/fam15h_power.c b/drivers/hwmon/fam15h_power.c
> index 2764b78a784b..af69073b3fe8 100644
> --- a/drivers/hwmon/fam15h_power.c
> +++ b/drivers/hwmon/fam15h_power.c
> @@ -129,12 +129,12 @@ static bool __devinit fam15h_power_is_internal_node0(struct pci_dev *f4)
>   * counter saturations resulting in bogus power readings.
>   * We correct this value ourselves to cope with older BIOSes.
>   */
> -static DEFINE_PCI_DEVICE_TABLE(affected_device) = {
> +static const struct pci_device_id affected_device[] = {

I would prefer if we could keep DEFINE_PCI_DEVICE_TABLE to avoid the
checkpatch warning. Should be easy to implement, by moving pci_match_id
into the probe function and only call tweak_runavg_range() if it returns true.
You could keep the response in fam15h_power_data, as a flag such as needs_tweak,
for use in fam15h_power_resume().

Just my $0.02, though. I'll leave it up to Jean to decide if the patch is ok
as-is. 

Guenter

>  	{ PCI_VDEVICE(AMD, PCI_DEVICE_ID_AMD_15H_NB_F4) },
>  	{ 0 }
>  };
>  
> -static void __devinit tweak_runavg_range(struct pci_dev *pdev)
> +static void tweak_runavg_range(struct pci_dev *pdev)
>  {
>  	u32 val;
>  
> @@ -158,6 +158,16 @@ static void __devinit tweak_runavg_range(struct pci_dev *pdev)
>  		REG_TDP_RUNNING_AVERAGE, val);
>  }
>  
> +#ifdef CONFIG_PM
> +static int fam15h_power_resume(struct pci_dev *pdev)
> +{
> +	tweak_runavg_range(pdev);
> +	return 0;
> +}
> +#else
> +#define fam15h_power_resume NULL
> +#endif
> +
>  static void __devinit fam15h_power_init_data(struct pci_dev *f4,
>  					     struct fam15h_power_data *data)
>  {
> @@ -256,6 +266,7 @@ static struct pci_driver fam15h_power_driver = {
>  	.id_table = fam15h_power_id_table,
>  	.probe = fam15h_power_probe,
>  	.remove = __devexit_p(fam15h_power_remove),
> +	.resume = fam15h_power_resume,
>  };
>  
>  module_pci_driver(fam15h_power_driver);
> -- 
> 1.7.10.4
> 
> 
> 

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH v2] hwmon, fam15h_power: Tweak runavg_range on resume

[Jean Delvare]

On Thu, 20 Sep 2012 09:35:39 -0700, Guenter Roeck wrote:
> On Thu, Sep 20, 2012 at 12:34:08PM +0200, Andreas Herrmann wrote:
> > 
> > The quirk introduced with commit
> > 00250ec90963b7ef6678438888f3244985ecde14 (hwmon: fam15h_power: fix
> > bogus values with current BIOSes) is not only required during driver
> > load but also when system resumes from suspend. The BIOS might set the
> > previously recommended (but unsuitable) initilization value for the
> > running average range register during resume.
> > 
> > Signed-off-by: Andreas Herrmann <andreas.herrmann3@amd.com>
> > ---
> >  drivers/hwmon/fam15h_power.c |   15 +++++++++++++--
> >  1 file changed, 13 insertions(+), 2 deletions(-)
> > 
> > Hi Jean,
> > 
> > Thanks for spotting the issues.
> > Here's a new patch version, esp. to fix the section mismatch.
> > 
> > Please apply.
> > 
> > 
> > Thanks,
> > 
> > Andreas
> > 
> > diff --git a/drivers/hwmon/fam15h_power.c b/drivers/hwmon/fam15h_power.c
> > index 2764b78a784b..af69073b3fe8 100644
> > --- a/drivers/hwmon/fam15h_power.c
> > +++ b/drivers/hwmon/fam15h_power.c
> > @@ -129,12 +129,12 @@ static bool __devinit fam15h_power_is_internal_node0(struct pci_dev *f4)
> >   * counter saturations resulting in bogus power readings.
> >   * We correct this value ourselves to cope with older BIOSes.
> >   */
> > -static DEFINE_PCI_DEVICE_TABLE(affected_device) = {
> > +static const struct pci_device_id affected_device[] = {
> 
> I would prefer if we could keep DEFINE_PCI_DEVICE_TABLE to avoid the
> checkpatch warning. Should be easy to implement, by moving pci_match_id
> into the probe function and only call tweak_runavg_range() if it returns true.
> You could keep the response in fam15h_power_data, as a flag such as needs_tweak,
> for use in fam15h_power_resume().
> 
> Just my $0.02, though. I'll leave it up to Jean to decide if the patch is ok
> as-is. 

I'll apply the patch as is. The warning from checkpatch is a false
positive, it happens sometimes and that's no reason for changing
working, tested code. Especially for a code change I'll push upstream
right before a release, and which should go to stable kernels, keeping
the patch simple is a must.

-- 
Jean Delvare

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH v2] hwmon, fam15h_power: Tweak runavg_range on resume

[Jean Delvare]

Hi Satoshi,

On Wed, 17 Jul 2013 21:35:43 +0000 (UTC), Satoshi Imamura wrote:
> Hello Mr. Andreas Herrmann,

I don't think Andreas is still reading the lm-sensors list, so I am
adding him to Cc.

> I'm trying to measure power consumption of AMD Opteron 6272 using fam15h_power.
> Linux kernel version is 3.9.4.
> Even when all cores are idle, fam15h_power shows about 85W.
> However, the power consumption decreases to about 74W
>  when a process is executed with all cores.
> 
> For Linux kernel 3.9.4, the patch for fam15h_power is already applied.
> Moreover, "RunAvgRange" (p. 460 of BKDG for Family 15h) is 
>  appropriately set to "0x9" as following:
> 
> % setpci -s 00:18.5 0xe0.l
> % 00372c99
> 
> Why is the power consumption so high (85W) when all cores are idle?
> Why does it decrease when a process is executed with all cores?
> I would appreciate if you could give me a advice.
> 
> Sincerely yours,
> 
> Satoshi

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH v2] hwmon, fam15h_power: Tweak runavg_range on resume

[Satoshi Imamura]

Hi Jean,

Thank you very much for your cooperation.

Regards,

Satoshi

-- 
Satoshi Imamura
Visiting Researcher
School of Electronics, Electrical Engineering and Computer Science
Queen's University of Belfast
Email: s.imamura@qub.ac.uk

On Jul 18, 2013, at 3:47 PM, Jean Delvare <khali@linux-fr.org> wrote:

> Hi Satoshi,
> 
> On Wed, 17 Jul 2013 21:35:43 +0000 (UTC), Satoshi Imamura wrote:
>> Hello Mr. Andreas Herrmann,
> 
> I don't think Andreas is still reading the lm-sensors list, so I am
> adding him to Cc.
> 
>> I'm trying to measure power consumption of AMD Opteron 6272 using fam15h_power.
>> Linux kernel version is 3.9.4.
>> Even when all cores are idle, fam15h_power shows about 85W.
>> However, the power consumption decreases to about 74W
>> when a process is executed with all cores.
>> 
>> For Linux kernel 3.9.4, the patch for fam15h_power is already applied.
>> Moreover, "RunAvgRange" (p. 460 of BKDG for Family 15h) is 
>> appropriately set to "0x9" as following:
>> 
>> % setpci -s 00:18.5 0xe0.l
>> % 00372c99
>> 
>> Why is the power consumption so high (85W) when all cores are idle?
>> Why does it decrease when a process is executed with all cores?
>> I would appreciate if you could give me a advice.
>> 
>> Sincerely yours,
>> 
>> Satoshi


_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors