linux-kernel - Re: [PATCH v2] hwmon: Add driver for AMD family 15h processor power information

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 6 Apr 2011 17:35:43 +0200
From:	Jean Delvare <khali@...ux-fr.org>
To:	Andreas Herrmann <herrmann.der.user@...glemail.com>,
	Guenter Roeck <guenter.roeck@...csson.com>
Cc:	Thomas Renninger <trenn@...e.de>, lm-sensors@...sensors.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] hwmon: Add driver for AMD family 15h processor power
  information

On Wed, 6 Apr 2011 17:19:01 +0200, Andreas Herrmann wrote:
> On Wed, Apr 06, 2011 at 04:14:01PM +0200, Jean Delvare wrote:
> > On Tue, 5 Apr 2011 16:45:36 +0200, Andreas Herrmann wrote:
> > > +static ssize_t show_power(struct device *dev,
> > > +			  struct device_attribute *attr, char *buf)
> > > +{
> > > +	u32 val, btdp, tdpl, tdp2w, arange;
> > > +	s32 acap;
> > > +	u64 ctdp;
> > 
> > These variable names aren't easy to understand.
> 
> Just random names which eventually map to the spec:
> 
> btdp - base_tdp
> tdpl - tdp_limit
> tdp2w - tdp_to_watt
> acap - average_accumulator_capture (or even worse how about "processor_tdp_running_average_accumulator":(
> arange - average_range

avg_cap and avg_range would do, respectively, for the last two.

> I don't think that changing the names make it much easier to
> reconstruct the calculation but if you insist in changing it I'll
> adapt it.

I do prefer the "extended" names, really. Sure, this doesn't change the
calculations, but it helps the reader understand what's going on. Which
will be useful if one ever has to fix a bug in the code or extend it
for a different CPU family.

But maybe this is just me. Guenter, do you have an opinion?

> > > (...)
> > > +	dev_set_drvdata(&pdev->dev, hwmon_dev);
> > 
> > pci_set_drvdata()
> 
> Are you sure?

Yes, I'm sure. This function exists, so there is no reason not to use
it.

> No single hwmon driver is using this wrapper around dev_set_drvdata() so far. (Or I just missed it.)

$ grep i2c_set_clientdata drivers/hwmon/*.c | wc -l
67
$ grep spi_set_drvdata drivers/hwmon/*.c | wc -l
2
$ grep platform_set_drvdata drivers/hwmon/*.c | wc -l
66
$

> At the moment only dev_set_drvdata() is used by
> 
>  drivers/hwmon/adcxx.c
>  drivers/hwmon/lm70.c
>  drivers/hwmon/ultra45_env.c
>  drivers/hwmon/ibmpex.c
>  drivers/hwmon/k10temp.c
>  drivers/hwmon/ibmaem.c
>  drivers/hwmon/k8temp.c

Most of these (at least spi and pci drivers) should be fixed. I'll send
a patch later.

-- 
Jean Delvare
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/