Re: [PATCH] acpi ec_sys: Export fields of all regions from the EC to debugfs readable

[Thomas Renninger <trenn@suse.de>]

On Saturday 31 July 2010 01:30:53 am Andrew Morton wrote:
> On Fri, 30 Jul 2010 17:47:04 +0200
>
> Thomas Renninger <trenn@suse.de> wrote:
> > -static int acpi_ec_open_io(struct inode *i, struct file *f)
> > +static int acpi_ec_open(struct inode *i, struct file *f)
> >  {
> >  	f->private_data = i->i_private;
> > +	if (mutex_trylock(&ec_fs_lock))
> > +		return 0;
> > +	else
> > +		return -EBUSY;
> > +}
>
> Well that sucks - a userspace interface which is _designed_ to randomly
> and rarely fail?

> An application tries to open the thing and gets -EBUSY, what's it
> supposed to do?  Sleep and try again?  Crash and dump core?
This is  a debug interface nobody should rely on or use in real applications.
There should only exist one app which dumps the EC or write modify single
bytes/fields and this one shouldn't be called twice in parallel.
My idea was: if someone accesses this file simultaneously, something is
wrong in userspace and it's even safer...

On the other hand side, the ec.c code should already parallelize register 
accesses  and the whole lock should not be needed at all.
Someone might want to check whether the locks
in ec.c really do what they should and could use this as a stress test if the 
lock is simply removed.

> Also the code's uncommented,
/* Only allow one user to access any EC io/field file at a time */
static DEFINE_MUTEX(ec_fs_lock);
> so the reader has no clue why a trylock 
> was used.  There are very few circumstances where any trylock can
> acceptably be left uncommented.
>
> afacit we can use mutex_lock() here and the implementation would be
> heaps better.
Not sure it really matters. But removing it altogether might be even better.
Waiting for some more comments...

       Thomas