Re: [PATCH] lenovo-sl-laptop : driver for review

[Henrique de Moraes Holschuh]

On Fri, 27 Feb 2009, Pavel Machek wrote:
> > +module_param(debug_ec, bool, S_IRUGO);
> > +MODULE_PARM_DESC(debug_ec,
> > +	"Present EC debugging interface in procfs. WARNING: writing to the "
> > +	"EC can hang your system and possibly damage your hardware.");
> 
> Sounds dangerous and clearly does not belong to /proc. Please drop it.

ThinkPad ACPI also has one.  OTOH, we are fairly sure one cannot damage the
standard ThinkPad hardware through the "IBM thinkpad-style" EC firmware, the
EC sanity-checks things.  I suppose you could cause the box to go away for
lunch and don't come back until a complete hardware reset, or hit one of the
crash-the-EC-firmware bugs by accident, though.

It has been invaluable for debugging. I can well guess why the thinkpad-sl
driver would want to add it as well.

What I am heavily considering is to taint the kernel when it is used (plus
it is not enabled by default, requires a module parameter to enable, etc),
plus output a big warning to syslog.  That is probably enough to keep the
users from abusing it.

> > +/*************************************************************************
> > +    bluetooth sysfs - copied nearly verbatim from thinkpad_acpi.c
> > + *************************************************************************/
> 
> That's quite a lot of code for verbatim copy; create shared helper?

Yeah, I am game.  But let's do it later on and not delay anything because of
it, please.  I will start breaking thinkpad-acpi into multiple source files,
and after that is done (should take a while, thinkpad-acpi is a big module
with a LOT of subdriver interdependencies) we can look for the best way to
reduce code duplication.

-- 
  "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 linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/