Jean Delvare wrote:
> Hi Jim,
>
>
>>>> +static ssize_t show_in_max_alarm(struct device *dev, struct device_attribute *devattr, char *buf)
>>>>
>>> Line too long, please fold. Same for all other callbacks you introduced.
>>>
>> Ok. I was under impression that fn-sigs were the sole exception
>> to the line-wrap rule, in that having the full sig on a single line is
>> good for grepping.
>>
>
> No, there is no such exception. Line wrap indeed makes grep'ing more
> difficult in some cases, but Linus seems to care about the line length
> more than the grep'ability.
>
>
Ok, thanks.
>> I'll guess that folding existing functions should be a separate patch ?
>> (combined with any other cosmetic changes, later)
>>
>
> Yes, please.
>
>
ok - Im thinking a full cosmetics patch, wrapping all long fn-sigs,
dropping old comment,
>>> This callback is exactly the same as show_in_min_alarm above, so why
>>> don't you just reuse it? Same for show_therm_max_alarm below and
>>> show_in_max_alarm.
>>>
>>> Which made me check the current code, and it appears that
>>> show_therm_input, show_therm_min and show_therm_max are redundant with
>>> show_in_input, show_in_min and show_in_max, respectively, so the code
>>> could be simplified. And this is again true of "set" callbacks, unless
>>> I am overlooking something? The benefit is even bigger here as these
>>> callbacks are larger. Care to submit a patch?
>>>
>> I was planning to fold them together in a 2D patch, like so:
>>
>> static ssize_t show_temp(struct device *dev, struct device_attribute
>> *devattr, char *buf)
>> {
>> struct sensor_device_attribute_2 *attr = to_sensor_dev_attr_2(devattr);
>> struct pc87360_data *data = pc87360_update_device(dev);
>> int idx = attr->index;
>> unsigned res = -1;
>>
>> switch (attr->nr) {
>> case FN_TEMP_INPUT:
>> res = TEMP_FROM_REG(data->temp[idx]);
>> break;
>>
>> default:
>> dev_err(dev, "unknown attr fetch\n");
>> }
>> return sprintf(buf, "%u\n", res);
>>
>
> How do you handle negative temperature values with a %u?
>
>
Ahh, Id not noticed that b4..
To establish a true baseline, I went back to 2.6.13 (before I started
hacking on this driver).
All of show_(in|therm)_<foo> use %u, which I also do in the 2D callbacks.
All the show_temp_<foo>s use /"%d\n" /in their respective formats, which
I didnt do, but will.
FWIW show_temp_status also used %d rather than %u, the latter is better
since status is flags.
Id prefer to ignore that preexisting sub-optimality (and use userspace
bug-compatibility argument to support that)
rather than add a case-specific return sprintf() or fmt* indirection.
>> }
>>
>> This gets significant shrinkage..
>> 15902 5160 32 21094 5266
>> 19rc1mm1-1/drivers/hwmon/pc87360.ko --- this patch
>>
>
> What is "this patch" exactly?
>
the previously sent patch - adding separate alarms.
>
>> 14011 5160 32 19203 4b03
>> 19rc1mm1-2/drivers/hwmon/pc87360.ko --- 2D patch
>>
>> This folding doesnt rely on thermistor measurements being done with voltage
>> hardware, which is an ideosyncrasy of this particular part, and doesnt
>> work for the temp_ sensors (only the therm ones), so I think its clearer
>> and more general this way.
>>
>
> Actually, thermistor measurements are always being done with voltage
> hardware, by design. What may be surprising is that the chip doesn't
> attempt to abstract this to make temperatures look more like
> temperatures.
>
> As said before, I'm not a fan of this switch/case approach, as it looks
> suboptimal from a performance point of view (unless you can prove me
> wrong with actual data). I did not object for the vt1211 driver because
> it was a new driver, and the author prefered it that way, so why not,
> but I see little benefit in converting a perfectly working driver to a
> different approach when both approaches have drawbacks and benefits.
>
>
1st, the size arg:
[jimc at harpo hwmon-stuff]$ size 19rc1mm1-*/drivers/hwmon/pc87360.ko
text data bss dec hex filename
14474 3816 32 18322 4792 19rc1mm1-0/drivers/hwmon/pc87360.ko
15902 5160 32 21094 5266 19rc1mm1-1/drivers/hwmon/pc87360.ko
15046 5160 32 20238 4f0e
19rc1mm1-1-fold/drivers/hwmon/pc87360.ko
14011 5160 32 19203 4b03 19rc1mm1-2/drivers/hwmon/pc87360.ko
0 - virgin rc1-mm1
1 - separate alarms
1-fold - with in/therm redundancy folded out
2 - 2D callbacks
As is evident, 2D gives 2x the size reduction as in/therm folding,
and I think its clearer. (also refactoring the status touching cases,
and sharing
that across in/therm is possible, but of tiny benefit in size, and a
loss of clarity imho)
As to performance (ie cache performance), the rule-of-thumb is that
smaller is better
(consider that force_inline is needed, cuz gcc sometimes ignores inline,
cuz its often a lose).
Beyond that, the access pattern from userspace is to read all the attrs
from each sensor,
then move to next sensor. For separate callbacks, this is the worst
pattern - each separate
callback has opportunity to eject the previous from cache, whereas with
2D callbacks,
the same function is re-called (more likely to still be in cache), and
each case is likely to
have separate cache-lines (adjacent code --> adjacent cache-line)
I could derive some value from trying to prove this (and learn to wield
oprofile well,
or cachegrind - but Im not set up to run a UML/xen-guest kernel, esp on
my 586-266 8-)
but GregKH has dismissed the cache effects as unimportant, so for now,
its *much* easier
to talk thru it. Do you find the previous paragraph at all convincing ?
Is there another aspect of performance that Ive overlooked ?
>>>> - /* include the few miscellaneous atts here */
>>>>
>>> This comment can stay for now?
>>>
>> Its less true than it once was - few is one, and not so miscellaneous -
>> temp-alarms belongs with temp_*, and comment implies that its something
>> of a catch-all.
>> At least that was my mis-thinking when I 1st added it.
>>
>> Or did you mean drop it *later* ?
>>
>
> No, I'm fine with that. I thought it was a left over from the first
> patch attempt, but if you remove it on purpose it's OK.
>
> (Indeed it then becomes a cleanup change without direct relation with
> what the patch does. But nevermind.)
>
Ok - I can restore it now, and pull it later - in a following
cosmetics/rewrap patch.
Id rather get your provisional acceptance of this separate-alarms patch,
2- then send my 2D for review (which applies cleanly over this one),
3- then do the rewrap,
4- then add fan_alarms (which dont exist currently BTW - only in_alarms
and temp_alarms do)
5- then do platform_driver (or maybe isa_driver, since is now in-core)
since 3,4,5 are dependent on how 2 goes, I havent touched them yet..
