On Tue, May 19, 2015, at 12:26, Bjørn Mork wrote:
> Henrique de Moraes Holschuh <hmh@xxxxxxxxxx> writes:
> >> -static ssize_t hotkey_wakeup_reason_show(struct device *dev,
> >> +static ssize_t wakeup_reason_show(struct device *dev,
> >
> > Before, all "hotkey subdriver" functions had the hotkey_ prefix. Now,
> > we have to intrude into whatever "namespace" an attribute name happens
> > to clash (in this case, the "wakeup" namespace).
> >
> > If this is the least horrible way to fix the issue, I will ack it even
> > if I don't like it. However, if there's a way to do it that would keep
> > the "hotkey_" prefix in the C side, and only change the rest of the C
> > function name (if required), I'd rather the fix did that.
>
> We could use the DEVICE_ATTR(_name, _mode, _show, _store) macro instead
> for the affected attributes if you prefer that.
Yes, please. I'd rather have a bit more boilerplate than sacrifice the
"subdriver prefix" in the C functions handling the sysfs nodes.
> I didn't think the names of these static functions mattered that much,
> but I do see that yout have a point for a 10kloc source file like
> thinkpad_acpi.c. You haven't ever considered splitting it? :-)
Heh, yeah, I did. I was going to at least kick the hwmon and the
alsa-related stuff to separate drivers. But there are all sorts of ties
between the separate functionalities due to a common firmware event
interface, so the result looked worse than the monolithic driver. And
we often get more of those when new firmware/hardware functionality
arrives...
A split-and-refactor might be in thinkpad-acpi's future, yes. But the
result would not be a bunch of independent modules due to the way the
firmware interface works, so there is little reason to not have it as a
monolitic module. This is not something you'd have in an embedded
kernel, after all...
Those 10k loc are mostly organized into highly modular blocks
("subdrivers"), thus my dislike for the change on the C function
prefixes for the sysfs attributes. Those prefixes make it easier to
maintain the driver ;-)
When I last tried it several years ago, splitting the large C file into
several smaller C files did not make that large a difference in
maintainability, for a noticeable cost in lost compile-time
optimizations. So, I never published it.
> > Also, the fix needs to go to stable (4.0+).
>
> I don't think so. The bug was introduced in v4.1-rc1 AFAICS. Am I
> missing something?
Hmm, I screwed up when I was trying to locate which trees shipped the
broken commit. You're right, this fix does _not_ need to go to stable.
Sorry about that!
--
"One disk to rule them all, One disk to find them. One disk to bring
them all and in the darkness grind them. In the Land of Redmond
where the shadows lie." -- The Silicon Valley Tarot
Henrique Holschuh
--
To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
