linux-kernel - Re: [PATCH/RFC] hwmon: Add support for W83667HG-B

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]

Message-ID: <20100702145404.GC14032@ericsson.com>
Date:	Fri, 2 Jul 2010 07:54:04 -0700
From:	Guenter Roeck <guenter.roeck@...csson.com>
To:	Jean Delvare <khali@...ux-fr.org>
CC:	Jim Cromie <jim.cromie@...il.com>,
	H Hartley Sweeten <hsweeten@...ionengravers.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Ethan Lawrence <e.law87@...oo.com>,
	"lm-sensors@...sensors.org" <lm-sensors@...sensors.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"lars4910@...mail.com" <lars4910@...mail.com>,
	"birdie@...monline.ru" <birdie@...monline.ru>,
	"jadcock@....net" <jadcock@....net>
Subject: Re: [PATCH/RFC] hwmon: Add support for W83667HG-B

On Fri, Jul 02, 2010 at 05:49:49AM -0400, Jean Delvare wrote:
> Hi Guenter,
> 
> On Fri, 2 Jul 2010 01:25:30 -0700, Guenter Roeck wrote:
> > On Fri, Jul 02, 2010 at 03:20:11AM -0400, Jean Delvare wrote:
> > > On Thu, 1 Jul 2010 15:02:15 -0700, Guenter Roeck wrote:
> > > > -static const u8 W83627EHF_REG_FAN_MAX_OUTPUT[] = { 0xff, 0x67, 0xff, 0x69 };
> > > > -static const u8 W83627EHF_REG_FAN_STEP_OUTPUT[] = { 0xff, 0x68, 0xff, 0x6a };
> > > > +
> > > > +static const u8 *W83627EHF_REG_FAN_MAX_OUTPUT;
> > > > +static const u8 *W83627EHF_REG_FAN_STEP_OUTPUT;
> > > > +
> > > > +static const u8 W83627EHF_REG_FAN_MAX_OUTPUT_COMMON[]
> > > > +						= { 0xff, 0x67, 0xff, 0x69 };
> > > > +static const u8 W83627EHF_REG_FAN_STEP_OUTPUT_COMMON[]
> > > > +						= { 0xff, 0x68, 0xff, 0x6a };
> > > > +
> > > > +static const u8 W83627EHF_REG_FAN_MAX_OUTPUT_W83667_B[] = { 0x67, 0x69, 0x6b };
> > > > +static const u8 W83627EHF_REG_FAN_STEP_OUTPUT_W83667_B[] = { 0x68, 0x6a, 0x6c };
> > > 
> > > Is it just me or these arrays aren't used anywhere?
> > > 
> > > I think I would just drop them. The "0xff" are suspicious in the
> > > original arrays, and the size difference between the common and
> > > W83667HG-B cases is tricky. Anyone willing to add support for this
> > > feature will need to read the datasheets anyway, so you don't add any
> > > value by including the register addresses here.
> > 
> > After removing the defines and trying to compile I remembered.
> > I _knew_ there was a reason for not removing them.
> > Guess it's too late (or early) here to do serious work.
> > 
> > The defines _are_ used, in:
> > 
> > fan_functions(fan_max_output, FAN_MAX_OUTPUT)
> > fan_functions(fan_step_output, FAN_STEP_OUTPUT)
> > 
> > which expands to W83627EHF_REG_FAN_MAX_OUTPUT and W83627EHF_REG_FAN_STEP_OUTPUT.
> > 
> > Tricky ... and that was also the reason why I retained the original 
> > global variables.
> 
> Tricky indeed. We normally don't accept code like this in the kernel.
> 
> > I'll move the pointers into per-device code as you suggested, but I'll
> > have to think about how to do that w/o having to change a lot of code.
> 
> If code changes are desirable, let's just do them. You can do that in a
> preliminary patch, and then your patch adding support for the
> W83667HG-B goes on top of it.
> 
Without the support for -B the changes are not really needed, so that patch
would not make much sense without it. Have you looked at v2 of the patch ?

> > As for the 0xff - that pretty much applies to all chips supported by this driver. 
> > I guess it is supposed to mean "not supported", and as a result the code will
> > write to a non-existing register. I don't really want to touch that.
> 
> I want you to touch that. Writing to non-existing registers is a bad
> idea. You never know what actually happens when you do that.
> 
Good point.

Clean fix would be not to provide the unsupported attributes. Simple workaround
would be to return an error if a write is attempted on a non-supported attribute.
I am sure it would be better to not provide the attribute, but would you accept
the workaround ?

> > The size difference (3 entries vs. 4) doesn't matter, since the chips are both 
> > configured to have only three pwm fan controllers (even though the W83667HG
> > is supposed to have four per its datasheet). So the 4th element of the arrays
> > will not be accessed by the code if W83667HG(-B) is detected.
> 
> OK.
> 
On a side note, any idea why the 4th pwm is disabled for the W83667HG ?

Guenter
--
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/