linux-kernel - Re: [lm-sensors] [PATCH 1/4] kernel.h: add find

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] [day] [month] [year] [list]

Message-ID: <20150226001313.GA14592@roeck-us.net>
Date:	Wed, 25 Feb 2015 16:13:13 -0800
From:	Guenter Roeck <linux@...ck-us.net>
To:	Bartosz Golaszewski <bgolaszewski@...libre.com>
Cc:	Phil Pokorny <ppokorny@...guincomputing.com>,
	LKML <linux-kernel@...r.kernel.org>,
	lm-sensors <lm-sensors@...sensors.org>
Subject: Re: [lm-sensors] [PATCH 1/4] kernel.h: add find_closest() macro

On Wed, Feb 25, 2015 at 11:59:51AM +0100, Bartosz Golaszewski wrote:
> 2015-02-24 21:51 GMT+01:00 Guenter Roeck <linux@...ck-us.net>:
> > I think the lm85 conversion actually introduces a bug with such an
> > off-by-one mistake. And if it doesn't, there is still a unexplained
> > and not easy to understand '-1' in one of the calls to find_closest().
> >
> > So the question is if the new code really improves the situation in that
> > respect.
> 
> Yes, it's a mistake. I couldn't test this one and missed this '-1'.
> Sorry for that.
> 
> As for the size comparisons - at first glance it seems as if it adds bloat:
> 
> ina2xx:
> add/remove: 0/0 grow/shrink: 1/0 up/down: 24/0 (24)
> function                                     old     new   delta
> ina226_set_interval                          296     320     +24
> 
> lm85:
> add/remove: 0/0 grow/shrink: 3/0 up/down: 72/0 (72)
> function                                     old     new   delta
> set_temp_auto_temp_min                       364     388     +24
> set_temp_auto_temp_max                       336     360     +24
> set_pwm_freq                                 284     308     +24
> 
> w83795:
> add/remove: 0/0 grow/shrink: 1/0 up/down: 16/0 (16)
> function                                     old     new   delta
> store_pwm                                    496     512     +16
> 
> But this actually comes from DIV_ROUND_CLOSEST() since replacing it
> with a simple '/ 2' gives different results:
> 
> ina2xx.ko:
> add/remove: 0/0 grow/shrink: 1/0 up/down: 6/0 (6)
> function                                     old     new   delta
> __UNIQUE_ID_vermagic0                         73      79      +6
> 
> lm85.ko:
> add/remove: 0/0 grow/shrink: 1/0 up/down: 6/0 (6)
> function                                     old     new   delta
> __UNIQUE_ID_vermagic0                         73      79      +6
> 
> w83795.ko:
> add/remove: 0/0 grow/shrink: 2/0 up/down: 14/0 (14)
> function                                     old     new   delta
> store_pwm                                    496     504      +8
> __UNIQUE_ID_vermagic0                         73      79      +6
> 
> The idea however was to remove duplicated operations from source files
> and prevent off-by-one bugs (how ironic the lm85 patch...), not really
> to reduce size.
> 
I don't know, seems to me it can cause more trouble and potential for getting
something wrong than it is worth. I'll definitely want to see feedback (and
acceptance) from more senior maintainers than myself.

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/