[RFC] EC registers - Adding sysfs interface?

[RFC] EC registers - Adding sysfs interface?

[Carlos Corbacho]

Alexey,

I'm considering writing a sysfs interface for the EC registers, and was 
wondering if you would be ok with such a patch (before I start work on it)?

I'd like to expose the registers to userspace, as it is already possible to 
access the EC registers in userspace via /dev/ports (and quite a few of us 
would like to access them) - however, the problem with doing this is that 
ACPI throws quite a few warnings/ errors in dmesg whilst doing this; it would 
therefore be far preferable to expose the registers through sysfs, which 
would in turn be based on the ec_{read,write} calls, so we don't end up 
stepping on ACPI's toes.

So, basically, I'm proposing something like:

/sys/firmware/acpi/ec/ecX
|
|-> 0
|-> 1
|-> 2
|-> etc... (up to 255)

Where each file returns the contents of the EC register in question (via 
ec_read()), and writing to the file calls ec_write().

-Carlos
-- 
E-Mail: carlos@strangeworlds.co.uk
Web: strangeworlds.co.uk
GPG Key ID: 0x23EE722D

Re: [RFC] EC registers - Adding sysfs interface?

[Alexey Starikovskiy]

Carlos Corbacho wrote:
> Alexey,
> 
> I'm considering writing a sysfs interface for the EC registers, and was 
> wondering if you would be ok with such a patch (before I start work on it)?
What do you need that for?
> 
> I'd like to expose the registers to userspace, as it is already possible to 
> access the EC registers in userspace via /dev/ports (and quite a few of us 
Well, this is not an argument. You have access to all memory through /dev/mem, 
so what?
> would like to access them) - however, the problem with doing this is that 
> ACPI throws quite a few warnings/ errors in dmesg whilst doing this; it would 
> therefore be far preferable to expose the registers through sysfs, which 
> would in turn be based on the ec_{read,write} calls, so we don't end up 
> stepping on ACPI's toes.
I think it is better to do it in file system that allows seek(). So you could do with 
1 file instead of 256 of them. 
> 
> So, basically, I'm proposing something like:
> 
> /sys/firmware/acpi/ec/ecX
> |
> |-> 0
> |-> 1
> |-> 2
> |-> etc... (up to 255)
> 
> Where each file returns the contents of the EC register in question (via 
> ec_read()), and writing to the file calls ec_write().
Carlos, I don't like the idea of 256 files. This will consume too many resources and will not bring much value.
If you want to have something permanent, it should be slim and fast.
If we talk about some debug/research/RE, do it as you like, but don't ask for submission into kernel :) 

Regards,
Alex.

Re: [RFC] EC registers - Adding sysfs interface?

[Alexey Starikovskiy]

Alexey Starikovskiy wrote:
> Carlos Corbacho wrote:
>> Alexey,
>>
>> I'm considering writing a sysfs interface for the EC registers, and 
>> was wondering if you would be ok with such a patch (before I start 
>> work on it)?
> What do you need that for?
>>
>> I'd like to expose the registers to userspace, as it is already 
>> possible to access the EC registers in userspace via /dev/ports (and 
>> quite a few of us 
> Well, this is not an argument. You have access to all memory through 
> /dev/mem, so what?
>> would like to access them) - however, the problem with doing this is 
>> that ACPI throws quite a few warnings/ errors in dmesg whilst doing 
>> this; it would therefore be far preferable to expose the registers 
>> through sysfs, which would in turn be based on the ec_{read,write} 
>> calls, so we don't end up stepping on ACPI's toes.
> I think it is better to do it in file system that allows seek(). So you 
> could do with 1 file instead of 256 of them.
>>
>> So, basically, I'm proposing something like:
>>
>> /sys/firmware/acpi/ec/ecX
>> |
>> |-> 0
>> |-> 1
>> |-> 2
>> |-> etc... (up to 255)
>>
>> Where each file returns the contents of the EC register in question 
>> (via ec_read()), and writing to the file calls ec_write().
> Carlos, I don't like the idea of 256 files. This will consume too many 
> resources and will not bring much value.
> If you want to have something permanent, it should be slim and fast.
How about character /dev/ec0?
> If we talk about some debug/research/RE, do it as you like, but don't 
> ask for submission into kernel :)
> Regards,
> Alex.
> 

Re: [RFC] EC registers - Adding sysfs interface?

[Carlos Corbacho]

Alexey,

On Tuesday 13 November 2007 18:40:48 Alexey Starikovskiy wrote:
> I think it is better to do it in file system that allows seek(). So you
> could do with 1 file instead of 256 of them.

Fair enough.

> Carlos, I don't like the idea of 256 files. This will consume too many
> resources and will not bring much value. If you want to have something
> permanent, it should be slim and fast. If we talk about some
> debug/research/RE, do it as you like, but don't ask for submission into
> kernel :)

Two reasons:

1) Yes, the obvious reverse engineering (being well behaved when reading EC 
registers, without doing strange things to ACPI when it also wants to call 
the EC - I've had at least one report where going through /dev/ports on an 
Acer laptop managed to cause the keyboard to lockup, until the system was 
reset).

2) I'm toying with the idea of re-implementing acer_acpi as a userspace 
application (which also requires EC access for certain functions - I don't 
mind poking /dev/ports for the odd bit of reverse engineering, but for any 
other kind of normal usage access to the EC, I really wouldn't want to be 
poking in /dev/ports and tripping over the EC kernel driver).

-Carlos
-- 
E-Mail: carlos@strangeworlds.co.uk
Web: strangeworlds.co.uk
GPG Key ID: 0x23EE722D

Re: [RFC] EC registers - Adding sysfs interface?

[Alexey Starikovskiy]

Carlos Corbacho wrote:
> Alexey,
> 
> 2) I'm toying with the idea of re-implementing acer_acpi as a userspace 
> application (which also requires EC access for certain functions - I don't 
> mind poking /dev/ports for the odd bit of reverse engineering, but for any 
> other kind of normal usage access to the EC, I really wouldn't want to be 
> poking in /dev/ports and tripping over the EC kernel driver).
What is the benefit of having acer_acpi in userspace, rather than one more 
*-laptop in /devices/misc?

Re: [RFC] EC registers - Adding sysfs interface?

[Carlos Corbacho]

Alexey,

> How about character /dev/ec0?

Yes, that would be fine.

On Tuesday 13 November 2007 18:54:51 Alexey Starikovskiy wrote:
> What is the benefit of having acer_acpi in userspace, rather than one more
> *-laptop in /devices/misc?

A debate of "would it be easier for me to maintain an in kernel driver (if/ 
when I can get it upstream) or a userspace application". At the moment, the 
idea of a full blown userspace application is just something I'm toying with 
in my head whilst working on WMI userspace.

-Carlos
-- 
E-Mail: carlos@strangeworlds.co.uk
Web: strangeworlds.co.uk
GPG Key ID: 0x23EE722D

Re: [RFC] EC registers - Adding sysfs interface?

[Henrique de Moraes Holschuh]

On Tue, 13 Nov 2007, Carlos Corbacho wrote:
> I'm considering writing a sysfs interface for the EC registers, and was 
> wondering if you would be ok with such a patch (before I start work on it)?

I have exactly such a beast in thinkpad-acpi (using /proc, though),
inherited from ibm-acpi who has had it for a LONG while.  So, I feel
entitled to share some of my experiences with it...

To make it short:

1. It is dangerous.  There is no other way to look at it.  Even reads can
   have side-effects on some boxes.   Writing can do *very* dangerous
   things.  So, it is a "use it only when told to by someone who actually
   knows what it does" tool for the vast majority out there.

2. Users will enable it even if they don't know what they're doing, and they
   will use it as if it were just yet another toy, no matter how many
   warnings you give them.  They will place scripts using it on Wikis, and
   you will get to hear about it for a long, long time.

3. Unless you stick *heavy* warnings, and require explicit enabling through
   module parameters/kernel command line, distros will enable it and *you*
   will get the blame and complains when it blows up something.

4. A binary attribute that can handle mmap() is a much better interface
   IMHO (or just use a plain UNIX char device).

So, while I am *for* adding such a thing (it has proved to be useful on
thinkpads), I'd say you should at least require a lot of steps to enable it,
and I do NOT mean a sysfs "write 1 here to enable" attribute.

I am against the idea of providing this as a regular (non-debug) tool for
usespace to poke at, for the simple reason that once you have "userspace
drivers" messing with it, all hope is lost to track the hardware state on
write-only registers.

-- 
  "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

Re: [RFC] EC registers - Adding sysfs interface?

[Thomas Renninger]

On Tue, 2007-11-13 at 19:03 +0000, Carlos Corbacho wrote:
> Alexey,
> 
> > How about character /dev/ec0?
> 
> Yes, that would be fine.
> 
> On Tuesday 13 November 2007 18:54:51 Alexey Starikovskiy wrote:
> > What is the benefit of having acer_acpi in userspace, rather than one more
> > *-laptop in /devices/misc?
> 
> A debate of "would it be easier for me to maintain an in kernel driver (if/ 
> when I can get it upstream) or a userspace application". At the moment, the 
> idea of a full blown userspace application is just something I'm toying with 
> in my head whilst working on WMI userspace.

I very much like the idea of a general EC debug/devel interface to
userspace.
It should be a separate CONFIG, marked "DEBUG", "EXPERIMENTAL" or
whatever and be per default off.
It is stupid that everybody who wants to debug a bit on EC registers
needs to duplicate the IBM EC implementation, this should IMO be moved
where it belongs to:
drivers/acpi/ec.c

The question is whether Len will accept/like it, Len?

   Thomas

Re: [RFC] EC registers - Adding sysfs interface?

[Alexey Starikovskiy]

Thomas Renninger wrote:
> On Tue, 2007-11-13 at 19:03 +0000, Carlos Corbacho wrote:
>   
>> Alexey,
>>
>>     
>>> How about character /dev/ec0?
>>>       
>> Yes, that would be fine.
>>
>> On Tuesday 13 November 2007 18:54:51 Alexey Starikovskiy wrote:
>>     
>>> What is the benefit of having acer_acpi in userspace, rather than one more
>>> *-laptop in /devices/misc?
>>>       
>> A debate of "would it be easier for me to maintain an in kernel driver (if/ 
>> when I can get it upstream) or a userspace application". At the moment, the 
>> idea of a full blown userspace application is just something I'm toying with 
>> in my head whilst working on WMI userspace.
>>     
>
> I very much like the idea of a general EC debug/devel interface to
> userspace.
> It should be a separate CONFIG, marked "DEBUG", "EXPERIMENTAL" or
> whatever and be per default off.
> It is stupid that everybody who wants to debug a bit on EC registers
> needs to duplicate the IBM EC implementation, this should IMO be moved
> where it belongs to:
> drivers/acpi/ec.c
>
> The question is whether Len will accept/like it, Len?
>
>   
Having it as /dev/ec0 should not introduce too many problems...
Also, it has much smaller impact than /dev/mem or /dev/ioport...

Regards,
Alex.

Re: [RFC] EC registers - Adding sysfs interface?

[Henrique de Moraes Holschuh]

On Wed, 28 Nov 2007, Alexey Starikovskiy wrote:
> Having it as /dev/ec0 should not introduce too many problems...

As long as it is with a debugging intent, and not as a place to plug
"userspace" drivers at.  That would be much too dangerous IMO.

> Also, it has much smaller impact than /dev/mem or /dev/ioport...

IMHO, it is just as dangerous.

-- 
  "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

Re: [RFC] EC registers - Adding sysfs interface?

[Len Brown]

On Wednesday 28 November 2007 11:28, Thomas Renninger wrote:
> On Tue, 2007-11-13 at 19:03 +0000, Carlos Corbacho wrote:
> > Alexey,
> > 
> > > How about character /dev/ec0?
> > 
> > Yes, that would be fine.
> > 
> > On Tuesday 13 November 2007 18:54:51 Alexey Starikovskiy wrote:
> > > What is the benefit of having acer_acpi in userspace, rather than one more
> > > *-laptop in /devices/misc?
> > 
> > A debate of "would it be easier for me to maintain an in kernel driver (if/ 
> > when I can get it upstream) or a userspace application". At the moment, the 
> > idea of a full blown userspace application is just something I'm toying with 
> > in my head whilst working on WMI userspace.
> 
> I very much like the idea of a general EC debug/devel interface to
> userspace.
> It should be a separate CONFIG, marked "DEBUG", "EXPERIMENTAL" or
> whatever and be per default off.
> It is stupid that everybody who wants to debug a bit on EC registers
> needs to duplicate the IBM EC implementation, this should IMO be moved
> where it belongs to:
> drivers/acpi/ec.c
> 
> The question is whether Len will accept/like it, Len?


It is crazy to poke ioports from user-space when ec.c thinks it owns them.
So if you're going to do so, it would certainly be an improvement
to go through the driver rather than around it.

What other users of this interface would there be other than
a user-space version of acer_acpi?  If a user-space driver
were using the I/F, then we couldn't make it a DEBUG-only feature,
it would have to be enabled all the time.

-Len

Re: [RFC] EC registers - Adding sysfs interface?

[Thomas Renninger]

On Thu, 2007-12-06 at 21:52 -0500, Len Brown wrote:
> On Wednesday 28 November 2007 11:28, Thomas Renninger wrote:
> > On Tue, 2007-11-13 at 19:03 +0000, Carlos Corbacho wrote:
> > > Alexey,
> > > 
> > > > How about character /dev/ec0?
> > > 
> > > Yes, that would be fine.
> > > 
> > > On Tuesday 13 November 2007 18:54:51 Alexey Starikovskiy wrote:
> > > > What is the benefit of having acer_acpi in userspace, rather than one more
> > > > *-laptop in /devices/misc?
> > > 
> > > A debate of "would it be easier for me to maintain an in kernel driver (if/ 
> > > when I can get it upstream) or a userspace application". At the moment, the 
> > > idea of a full blown userspace application is just something I'm toying with 
> > > in my head whilst working on WMI userspace.
> > 
> > I very much like the idea of a general EC debug/devel interface to
> > userspace.
> > It should be a separate CONFIG, marked "DEBUG", "EXPERIMENTAL" or
> > whatever and be per default off.
> > It is stupid that everybody who wants to debug a bit on EC registers
> > needs to duplicate the IBM EC implementation, this should IMO be moved
> > where it belongs to:
> > drivers/acpi/ec.c
> > 
> > The question is whether Len will accept/like it, Len?
> 
> 
> It is crazy to poke ioports from user-space when ec.c thinks it owns them.
> So if you're going to do so, it would certainly be an improvement
> to go through the driver rather than around it.
> 
> What other users of this interface would there be other than
> a user-space version of acer_acpi?
> If a user-space driver
> were using the I/F, then we couldn't make it a DEBUG-only feature,
> it would have to be enabled all the time.

I see the "danger" of applications misusing this as an interface and
implementing workarounds for unsupported ACPI parts and (this is the
real problem) stop working on the real problems or a proper
implementation.

Especially because this option should be compiled in e.g. OpenSUSE
versions, for better remote debugging (comparing EC register values with
DSDT Embedded Controller variable implementations on a related problem
could help a lot, without the need of digging in the dark for weeks...).

>From your comments above, I read out that you tend to accept such an
interface as suggested by Alexey?
Making it read-only by default, would mainly help debugging purposes and
should avoid any workaround implementations.

   Thomas

Re: [RFC] EC registers - Adding sysfs interface?

[Henrique de Moraes Holschuh]

On Fri, 07 Dec 2007, Thomas Renninger wrote:
> I see the "danger" of applications misusing this as an interface and
> implementing workarounds for unsupported ACPI parts and (this is the
> real problem) stop working on the real problems or a proper
> implementation.

Exactly.  Or causing shadow issues that are hell to debug... unless this
this EC register interface taints the kernel the first time it is used for
reads OR writes...

> Especially because this option should be compiled in e.g. OpenSUSE
> versions, for better remote debugging (comparing EC register values with
> DSDT Embedded Controller variable implementations on a related problem
> could help a lot, without the need of digging in the dark for weeks...).

If it is something you can tell users to load explictly (load the "ec_debug"
module, with the parameter "yes_dammit_do_load_it=1") or
somesuch, maybe.

> >From your comments above, I read out that you tend to accept such an
> interface as suggested by Alexey?
> Making it read-only by default, would mainly help debugging purposes and
> should avoid any workaround implementations.

Read-only is *much* better than read-write, but it is NOT completely safe.
IMO, it would still need to be something that requires the user to
explicitly enable.  It can't just sit there active by default.  It'd have to
be something through which a stray open() or stat() wouldn't cause EC I/O.

-- 
  "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