Is configfs the right solution for configuration based fs?

Is configfs the right solution for configuration based fs?

[Luis R. Rodriguez]

I was really interested in looking to start a filesystem based
approach for configuration of wireless a while back, an alternative to
nl80211 if you will, but I stopped after I was told about some major
issues with configfs. I forget the issues raised clearly so I'd like
to bring this up for debate to see what really are the issues, what
needs to be fixed so we can *properly* use a fs for configuration of
subsystems. I thought configfs was the solution. We currently use and
abuse debugfs, but it doesn't matter -- we don't expect users to
depend on those files for ABI. It, however, it would be nice to
finally export some of these values into a concise place so userspace
*can* rely on them.

I'm ultimately looking for an option to reduce the size of the kernel
you'd need for wirless configuration, or at least to make easier for
userspace. Don't get me wrong I'm not saying I want an alternative to
nl80211 -- I realize we already decided on that and are moving along
in that direction, I'm just wondering once that is done what is the
right place to look at and if we don't have anything what are the
things we need to take into consideration so we *do* use or implement
the *right thing*.

Perhaps a fs is not even the most optimized approach for size anyway
so the benefits may just be helping with the easy design of userspace
applications. Feedback on experiences are welcomed.

PS. I'm hoping those who *really* hate configs can comment

  Luis

Re: Is configfs the right solution for configuration based fs?

[Joel Becker]

On Sun, Jun 08, 2008 at 02:25:36PM -0700, Luis R. Rodriguez wrote:
> I was really interested in looking to start a filesystem based
> approach for configuration of wireless a while back, an alternative to
> nl80211 if you will, but I stopped after I was told about some major
> issues with configfs. I forget the issues raised clearly so I'd like
> to bring this up for debate to see what really are the issues, what
> needs to be fixed so we can *properly* use a fs for configuration of
> subsystems. I thought configfs was the solution. We currently use and
> abuse debugfs, but it doesn't matter -- we don't expect users to
> depend on those files for ABI. It, however, it would be nice to
> finally export some of these values into a concise place so userspace
> *can* rely on them.

	I'd love to hear about the issues as well.  I'd also love to see
what you require, so as to either be able to explain how configfs can do
it for you, or to say that configfs isn't the right fit.  configfs is
designed with a particular goal, and some things don't fit that - and
that's fine.

> Perhaps a fs is not even the most optimized approach for size anyway
> so the benefits may just be helping with the easy design of userspace
> applications. Feedback on experiences are welcomed.
> 
> PS. I'm hoping those who *really* hate configs can comment

	You probably expect me to be all "configfs solves world peace,
how can you hate it?"  Nah.  I'd really like to understand your need and
how configfs doesn't fit.  configfs may not fit.  configfs may fit and
I need to explain it better.  Or configfs may not fit but should, and we
then can see how to make it right.

Joel

-- 

"Nothing is wrong with California that a rise in the ocean level
 wouldn't cure."
        - Ross MacDonald

Joel Becker
Principal Software Developer
Oracle
E-mail: joel.becker@oracle.com
Phone: (650) 506-8127

Re: Is configfs the right solution for configuration based fs?

[Joel Becker]

On Sun, Jun 08, 2008 at 07:28:21PM -0700, Joel Becker wrote:
> On Sun, Jun 08, 2008 at 02:25:36PM -0700, Luis R. Rodriguez wrote:
> > I was really interested in looking to start a filesystem based
> > approach for configuration of wireless a while back, an alternative to
> > nl80211 if you will, but I stopped after I was told about some major
> > issues with configfs. I forget the issues raised clearly so I'd like
> 
> 	I'd love to hear about the issues as well. 

	Here's a list of "known" issues I hear about with configfs.
These are requests/complaints/etc I have gotten since it was merged.

1) configfs should be sysfs

	The argument is that sysfs should somehow support the
user-directed mkdir(2)/rmdir(2) lifecycle of configfs in addition to its
usual functions.  This, unfortunately, doesn't work.  I sent a pretty
detailed discussion of this to lkml the last time it came up, but here's
a short summary.  Number one, I tried this first.  It got ugly fast.
Number two, a goal of configfs is a simpler lifecycle than sysfs
(understanding the lifetimes of config items).  Adding an additional
mode to the already complicated lifecycle of kobjects directly opposes
this

2) There needs to be a way to pin a config item

	configfs's ->drop_item() operation returns void - if it is
called, your item must deactivate.  This is in line with configfs'
user-directed paradigm.  However, sometimes another kernel subsystem is
depending on that item - it will crash if the item goes away.
	After getting this beaten over my head a few times by good
friends, I realized they were right.  configfs now has
configfs_depend_item() to allow subsystems to pin config items when
necessary.

3) configfs should support large attributes

	A configfs attribute can be a maximum of 4k in size.  This fits
the simple show/store methods cribbed from sysfs.  However, more than
one person has given a good reason for larger attributes, often lists of
things.
	This isn't implemented yet, because I haven't come up with a
good way to do it.  seq_file works pretty well for the show side, but
there is no seq_write() to match on the store side.  I'd love to come up
with a sane semantic and make it work.  Consider it a TODO.

Finally, this thread has presented 
4) easier definition of items, perhaps with macros like DEFINE_ATTR

	I think I have a __CONFIGFS_ATTR() macro, but it's nowhere near
as nice as what sysfs has.  This definitely could use some work, making
configfs easier to use.

	That's what I have.  I don't know if any of these issues were
what you were worried about, Luis.

Joel

-- 

"In the long run...we'll all be dead."
                                        -Unknown

Joel Becker
Principal Software Developer
Oracle
E-mail: joel.becker@oracle.com
Phone: (650) 506-8127

Re: Is configfs the right solution for configuration based fs?

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 1220 bytes --]


> I was really interested in looking to start a filesystem based
> approach for configuration of wireless a while back, an alternative to
> nl80211 if you will, but I stopped after I was told about some major
> issues with configfs. I forget the issues raised clearly so I'd like
> to bring this up for debate to see what really are the issues, what
> needs to be fixed so we can *properly* use a fs for configuration of
> subsystems.

Personally, I have a few issues with this:
 1) why bother with a second configuration interface that we have to
    maintain, adjust, ...? if we need scriptable access, then make a
    good userspace tool that is scriptable.
 2) string-based stuff is often messy, especially the varying attributes
    like MAC addresses etc. Unless we just use binary files again, which
    is not very useful again. Take, for example, the monitor flags. If
    we use the same flags then nobody really knows what's up 
    (echo 0x3 > mntr_flags?) and if we use strings then we cannot easily
    ever rename the flag while keeping ABI/API compatibility.
 3) afaik configfs doesn't actually support the mkdir, ... stuff yet
    that you want for virtual interfaces.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: Is configfs the right solution for configuration based fs?

[Ben Nizette]

On Mon, 2008-06-09 at 11:03 +0200, Johannes Berg wrote:

> Personally, I have a few issues with this:
>  1) why bother with a second configuration interface that we have to
>     maintain, adjust, ...? if we need scriptable access, then make a
>     good userspace tool that is scriptable.

What's the first one, sysfs..?  ioctl (eww..)?  I do think they solve
different problems, both have their place.  IMHO sysfs is forced to do
configuration in some situations where it just doesn't fit.  Prolly 'coz
sysfs have the easy __DEVICE_ATTR kinda macros where as configfs takes
more effort to get flying.

>  2) string-based stuff is often messy, especially the varying attributes
>     like MAC addresses etc. Unless we just use binary files again, which
>     is not very useful again. Take, for example, the monitor flags. If
>     we use the same flags then nobody really knows what's up 
>     (echo 0x3 > mntr_flags?) and if we use strings then we cannot easily
>     ever rename the flag while keeping ABI/API compatibility.

Not sure I see the argument here, why would you want to change the flag
name?  If you decide the old name is stupid then can't you just alias
the old name to the new one?

String handling is always a bit iffy, though it has to be done
somewhere, either in kernel or in your "good userspace tool which is
scriptable".  I'd prefer to have it done once, well, in the kernel and
not have to ship more software than necessary.

>  3) afaik configfs doesn't actually support the mkdir, ... stuff yet
>     that you want for virtual interfaces.

It has all the  mkdir stuff I can think of, can you elaborate?  It
doesn't have the commitable object support but I just have an 'enabled'
attribute in there to switch the thing on and off.

My main problem with configfs (and I've done a few things with it) is
the complexity of the thing, particularly config_group vs config_item vs
config_attribute.  I'd quite like to see just config_group representing
"directory" type objects and attributes representing, well, attributes
in the dir.

That and a bit of wider use would probably see configfs growing helper
macros like those which make sysfs attributes a piece of cake to
implement.

And the trival problem that, ISTR, failing the make_group method always
reports -ENOMEM to userspace, no matter what the actual problem was.  I
think I had a patch around to pass the error code from make_group back
up through to userspace, I wonder what happened to that...

	--Ben.

Re: Is configfs the right solution for configuration based fs?

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 2197 bytes --]

On Tue, 2008-06-10 at 10:12 +1000, Ben Nizette wrote:
> On Mon, 2008-06-09 at 11:03 +0200, Johannes Berg wrote:
> 
> > Personally, I have a few issues with this:
> >  1) why bother with a second configuration interface that we have to
> >     maintain, adjust, ...? if we need scriptable access, then make a
> >     good userspace tool that is scriptable.
> 
> What's the first one, sysfs..?  ioctl (eww..)?  

netlink.

> >  2) string-based stuff is often messy, especially the varying attributes
> >     like MAC addresses etc. Unless we just use binary files again, which
> >     is not very useful again. Take, for example, the monitor flags. If
> >     we use the same flags then nobody really knows what's up 
> >     (echo 0x3 > mntr_flags?) and if we use strings then we cannot easily
> >     ever rename the flag while keeping ABI/API compatibility.
> 
> Not sure I see the argument here, why would you want to change the flag
> name?  If you decide the old name is stupid then can't you just alias
> the old name to the new one?

Sure can do, but it just adds a lot of complexity to the kernel. I don't
see the point, it's not like you need a lot of code to build netlink
messages. Heck, I've done it by _hand_ and used just netlink sockets.
It's not a lot of code.

> String handling is always a bit iffy, though it has to be done
> somewhere, either in kernel or in your "good userspace tool which is
> scriptable".  I'd prefer to have it done once, well, in the kernel and
> not have to ship more software than necessary.

I personally prefer to put it into userspace.

> >  3) afaik configfs doesn't actually support the mkdir, ... stuff yet
> >     that you want for virtual interfaces.
> 
> It has all the  mkdir stuff I can think of, can you elaborate?  It
> doesn't have the commitable object support but I just have an 'enabled'
> attribute in there to switch the thing on and off.

I don't remember the specifics, it's been a while, I guess I could be
thinking of the commitable object support; mostly we'd want to configure
many things in one go, even on a live object. Without disabling that
object first, obviously.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: Is configfs the right solution for configuration based fs?

[Luis R. Rodriguez]

On Tue, Jun 10, 2008 at 1:01 AM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> On Tue, 2008-06-10 at 10:12 +1000, Ben Nizette wrote:
>> On Mon, 2008-06-09 at 11:03 +0200, Johannes Berg wrote:
>>
>> > Personally, I have a few issues with this:
>> >  1) why bother with a second configuration interface that we have to
>> >     maintain, adjust, ...? if we need scriptable access, then make a
>> >     good userspace tool that is scriptable.
>>
>> What's the first one, sysfs..?  ioctl (eww..)?
>
> netlink.
>
>> >  2) string-based stuff is often messy, especially the varying attributes
>> >     like MAC addresses etc. Unless we just use binary files again, which
>> >     is not very useful again. Take, for example, the monitor flags. If
>> >     we use the same flags then nobody really knows what's up
>> >     (echo 0x3 > mntr_flags?) and if we use strings then we cannot easily
>> >     ever rename the flag while keeping ABI/API compatibility.
>>
>> Not sure I see the argument here, why would you want to change the flag
>> name?  If you decide the old name is stupid then can't you just alias
>> the old name to the new one?
>
> Sure can do, but it just adds a lot of complexity to the kernel. I don't
> see the point, it's not like you need a lot of code to build netlink
> messages. Heck, I've done it by _hand_ and used just netlink sockets.
> It's not a lot of code.

I actually agree with Johannes here, its not only complexity but also
a burden, not to mention more bloat. We get away with some flag
hackery in ath5k debug.[ch] for example to let you even use the name
of flags but to assume none of this will change over time and to leave
it to us to manage for an actual subsystem seems plainly overkill and
abuse.

>> String handling is always a bit iffy, though it has to be done
>> somewhere, either in kernel or in your "good userspace tool which is
>> scriptable".  I'd prefer to have it done once, well, in the kernel and
>> not have to ship more software than necessary.
>
> I personally prefer to put it into userspace.

This is of course subjective, I actually think your point about
leaving the complexity of string manipulation and burden on userspace
is perhaps the best argument to avoid a *complete* subsystem interface
through a configuration file system.

>> >  3) afaik configfs doesn't actually support the mkdir, ... stuff yet
>> >     that you want for virtual interfaces.
>>
>> It has all the  mkdir stuff I can think of, can you elaborate?  It
>> doesn't have the commitable object support but I just have an 'enabled'
>> attribute in there to switch the thing on and off.
>
> I don't remember the specifics, it's been a while, I guess I could be
> thinking of the commitable object support; mostly we'd want to configure
> many things in one go, even on a live object. Without disabling that
> object first, obviously.

OK we could use a fs *only* for retrieval and setting of a few
wireless configuration data then which *is tunable* without *much
complexity*. I'm thinking *standard tunable values* and *standard
stats* like those currently exported via debugfs. This could simply be
optional as well, just as debugfs is. Not even sure if configfs would
be the ideal candidate for this. Alternatively we could just not have
one too, and I'm fine with that too, but we'll just have to make great
userspace tools.

Just wanted to get an idea of what people thought and through their
experience what is best.

  Luis

Re: Is configfs the right solution for configuration based fs?

[Ben Nizette]

On Tue, 2008-06-10 at 10:01 +0200, Johannes Berg wrote:
> On Tue, 2008-06-10 at 10:12 +1000, Ben Nizette wrote:
> > On Mon, 2008-06-09 at 11:03 +0200, Johannes Berg wrote:
> > 
> > > Personally, I have a few issues with this:
> > >  1) why bother with a second configuration interface that we have to
> > >     maintain, adjust, ...? if we need scriptable access, then make a
> > >     good userspace tool that is scriptable.
> > 
> > What's the first one, sysfs..?  ioctl (eww..)?  
> 
> netlink.

Ah right.  Netlink is indeed a nice interface for this kind of network
device configuration, I'd probably use that for this task too.

Of course, it isn't a general solution to the problem.  If I were
answering the question in $(SUBJECT) I'd say configfs is a good choice.
In the specific case of network devices, you're lucky enough to have
something more specifically tailored to your needs already.

> 
> > >  2) string-based stuff is often messy, especially the varying attributes
> > >     like MAC addresses etc. Unless we just use binary files again, which
> > >     is not very useful again. Take, for example, the monitor flags. If
> > >     we use the same flags then nobody really knows what's up 
> > >     (echo 0x3 > mntr_flags?) and if we use strings then we cannot easily
> > >     ever rename the flag while keeping ABI/API compatibility.
> > 
> > Not sure I see the argument here, why would you want to change the flag
> > name?  If you decide the old name is stupid then can't you just alias
> > the old name to the new one?
> 
> Sure can do, but it just adds a lot of complexity to the kernel. I don't
> see the point, it's not like you need a lot of code to build netlink
> messages. Heck, I've done it by _hand_ and used just netlink sockets.
> It's not a lot of code.
> 
> > String handling is always a bit iffy, though it has to be done
> > somewhere, either in kernel or in your "good userspace tool which is
> > scriptable".  I'd prefer to have it done once, well, in the kernel and
> > not have to ship more software than necessary.
> 
> I personally prefer to put it into userspace.

Yeah, personal preference here.  I come from an embedded background and
have an aversion to tracking more userspace tools than I have to.  This
mainly comes from the fact uClibc (an embedded C library) doesn't
maintain binary compatibility across releases; each time a bugfix comes
out for that my entire userspace needs to be recompiled.  Each userspace
tool I add to my build scripts makes me die a little inside ;-)


	--Ben.

[RFC PATCHES] Re: Is configfs the right solution for configuration based fs?

[Joel Becker]

On Tue, Jun 10, 2008 at 10:12:52AM +1000, Ben Nizette wrote:
>

	Thanks, Ben, for providing constructive comments.  I know I
replied later with some of the known issues.  Here are some tentative
solutions! (Why now, you ask?  I just finished up something and had a
free moment)

> What's the first one, sysfs..?  ioctl (eww..)?  I do think they solve
> different problems, both have their place.  IMHO sysfs is forced to do
> configuration in some situations where it just doesn't fit.  Prolly 'coz
> sysfs have the easy __DEVICE_ATTR kinda macros where as configfs takes
> more effort to get flying.
<snip>
> That and a bit of wider use would probably see configfs growing helper
> macros like those which make sysfs attributes a piece of cake to
> implement.

	Ask, and ye shall receive.  I've actually been meaning to look
at this for a while.  Please check out the branch configfs-attr-macros
at my git tree.  Actually, here is the configfs.h with macros, and
configfs_example_macros.c using them.  Let me know if this fits the
bill.

[config-attr-macros:include/linux/configfs.h]
http://oss.oracle.com/git/jlbec/linux-2.6.git/?p=jlbec/linux-2.6.git;a=blob;f=include/linux/configfs.h;h=8be4507ab98f49f43c5212cbafe62178df9a517b;hb=c085c60b185d7431935397e43a868325d8f1da6c

[config-attr-macros:Documentation/filesystems/configfs/configfs_example_macros.c]
http://oss.oracle.com/git/jlbec/linux-2.6.git/?p=jlbec/linux-2.6.git;a=blob;f=Documentation/filesystems/configfs/configfs_example_macros.c;h=c2ece748abdf30a7d67dd9ea25c5e6ac93fbc005;hb=c085c60b185d7431935397e43a868325d8f1da6c

	Compare and contrast with the original configfs_example.c (now
configfs_example_explicit.c on that branch).  The diff is available
here:
http://oss.oracle.com/git/jlbec/linux-2.6.git/?p=jlbec/linux-2.6.git;a=commitdiff;h=c085c60b185d7431935397e43a868325d8f1da6c

> And the trival problem that, ISTR, failing the make_group method always
> reports -ENOMEM to userspace, no matter what the actual problem was.  I
> think I had a patch around to pass the error code from make_group back
> up through to userspace, I wonder what happened to that...

	I've also attacked this one.  The change is in the
make-item-errors branch.  It's already scheduled for linux-next.  Diff
available here:
http://oss.oracle.com/git/jlbec/linux-2.6.git/?p=jlbec/linux-2.6.git;a=commitdiff;h=34fbb494fecfd82bcbf9ed698ef305500228a84c

Joel

-- 

Life's Little Instruction Book #226

	"When someone hugs you, let them be the first to let go."

Joel Becker
Principal Software Developer
Oracle
E-mail: joel.becker@oracle.com
Phone: (650) 506-8127

Re: [RFC PATCHES] Re: Is configfs the right solution for configuration based fs?

[Ben Nizette]

On Wed, 2008-06-18 at 19:48 -0700, Joel Becker wrote:

> 	Ask, and ye shall receive.  I've actually been meaning to look
> at this for a while.  Please check out the branch configfs-attr-macros
> at my git tree.  Actually, here is the configfs.h with macros, and
> configfs_example_macros.c using them.  Let me know if this fits the
> bill.
> 
> [config-attr-macros:include/linux/configfs.h]
> http://oss.oracle.com/git/jlbec/linux-2.6.git/?p=jlbec/linux-2.6.git;a=blob;f=include/linux/configfs.h;h=8be4507ab98f49f43c5212cbafe62178df9a517b;hb=c085c60b185d7431935397e43a868325d8f1da6c
> 
> [config-attr-macros:Documentation/filesystems/configfs/configfs_example_macros.c]
> http://oss.oracle.com/git/jlbec/linux-2.6.git/?p=jlbec/linux-2.6.git;a=blob;f=Documentation/filesystems/configfs/configfs_example_macros.c;h=c2ece748abdf30a7d67dd9ea25c5e6ac93fbc005;hb=c085c60b185d7431935397e43a868325d8f1da6c
> 
> 	Compare and contrast with the original configfs_example.c (now
> configfs_example_explicit.c on that branch).  The diff is available
> here:
> http://oss.oracle.com/git/jlbec/linux-2.6.git/?p=jlbec/linux-2.6.git;a=commitdiff;h=c085c60b185d7431935397e43a868325d8f1da6c
> 

Looks great, thx :-).  I'll convert some of my stuff over to this
interface and see how it flies in the real world.

> > And the trival problem that, ISTR, failing the make_group method always
> > reports -ENOMEM to userspace, no matter what the actual problem was.  I
> > think I had a patch around to pass the error code from make_group back
> > up through to userspace, I wonder what happened to that...
> 
> 	I've also attacked this one.  The change is in the
> make-item-errors branch.  It's already scheduled for linux-next.  Diff
> available here:
> http://oss.oracle.com/git/jlbec/linux-2.6.git/?p=jlbec/linux-2.6.git;a=commitdiff;h=34fbb494fecfd82bcbf9ed698ef305500228a84c
> 

ISTR when I did this I kept the old semantics and used ERR_PTR and
friends if things went pear-shaped.  Given the small number of in-tree
users needing to be moved over, a more intrusive change for the sake of
a cleaner API like you've done is probably a good thing anyway :-)

Thanks again,
	--Ben.

Re: [RFC PATCHES] Re: Is configfs the right solution for configuration based fs?

[Joel Becker]

On Fri, Jun 20, 2008 at 04:19:10PM +1000, Ben Nizette wrote:
> Looks great, thx :-).  I'll convert some of my stuff over to this
> interface and see how it flies in the real world.

	Excellent.  What is your stuff, btw?  Got code?  Love to see it.

> ISTR when I did this I kept the old semantics and used ERR_PTR and
> friends if things went pear-shaped.  Given the small number of in-tree
> users needing to be moved over, a more intrusive change for the sake of
> a cleaner API like you've done is probably a good thing anyway :-)

	Yeah, I like this better than ERR_PTR.
	If you like the macros, I'll try to make the next merge window
as well.  So let me know.

joel

-- 

"I almost ran over an angel
 He had a nice big fat cigar.
 'In a sense,' he said, 'You're alone here
 So if you jump, you'd best jump far.'"

Joel Becker
Principal Software Developer
Oracle
E-mail: joel.becker@oracle.com
Phone: (650) 506-8127

Re: [RFC PATCHES] Re: Is configfs the right solution for configuration based fs?

[Ben Nizette]

[-- Attachment #1: Type: text/plain, Size: 1598 bytes --]


On Thu, 2008-06-19 at 23:52 -0700, Joel Becker wrote:
> On Fri, Jun 20, 2008 at 04:19:10PM +1000, Ben Nizette wrote:
> > Looks great, thx :-).  I'll convert some of my stuff over to this
> > interface and see how it flies in the real world.
> 
> 	Excellent.  What is your stuff, btw?  Got code?  Love to see it.

I've done a few things recently.  Haavard Skinnemoen has got an
out-of-tree, avr32-specific gpio interface [1] which I first converted
to the generic gpio framework then completely re-wrote.  Both use
configfs but before either version could reach completion they were
obsoleted by David Brownell's gpio sysfs interface.

A version of my gpio-dev interface is attached.  Bear in mind it was
never completed, it's full of known bugs but hey, might be useful for
you anyway :-)

At the moment I'm experimenting with configfs for pinmux control on
AVR32 SoCs.  There's really nothing to see there yet, it's all just
dicking about atm.

> 
> > ISTR when I did this I kept the old semantics and used ERR_PTR and
> > friends if things went pear-shaped.  Given the small number of in-tree
> > users needing to be moved over, a more intrusive change for the sake of
> > a cleaner API like you've done is probably a good thing anyway :-)
> 
> 	Yeah, I like this better than ERR_PTR.
> 	If you like the macros, I'll try to make the next merge window
> as well.  So let me know.

np, will do :-)

	--Ben.

> 
> joel
> 

[1]
http://git.kernel.org/?p=linux/kernel/git/hskinnemoen/avr32-2.6.git;a=blob;f=arch/avr32/mach-at32ap/gpio-dev.c;h=8cf6d1182c31db88fc069a046112a4eaac589308;hb=atmel-2.6.25


[-- Attachment #2: gpio-dev.c --]
[-- Type: text/x-csrc, Size: 21720 bytes --]

/*
 * GPIO /dev and configfs interface
 *
 * Copyright (C) 2008 Nias Digital P/L
 *
 * This program is free software; you can redistribute it and/or modify
 * it under the terms of the GNU General Public License version 2 as
 * published by the Free Software Foundation.
 */
#include <linux/kernel.h>
#include <linux/init.h>
#include <linux/module.h>
#include <linux/slab.h>
#include <linux/err.h>
#include <linux/cdev.h>
#include <linux/device.h>
#include <linux/fs.h>
#include <linux/spinlock.h>
#include <linux/configfs.h>
#include <linux/types.h>
#include <linux/platform_device.h>
#include <linux/poll.h>
#include <linux/interrupt.h>
#include <linux/irq.h>
#include <linux/kfifo.h>

#include <asm/uaccess.h>
#include <asm/bug.h>
#include <asm/gpio.h>

#include "gpio-dev.h"

#define MAX_NR_DEVICES	8
#define IRQ_BUFFER_SIZE	256

#define BUF_PUT_INT(k, b) \
			do { \
				char *p = (char *)&b; \
				kfifo_put(k, p, sizeof(int)); \
			} while (0)

#define BUF_GET_INT(k, b) \
			do { \
				char *p = (char *)&b; \
				kfifo_get(k, p, sizeof(int)); \
			} while (0)

static struct class *dev_class;
static dev_t base_devt;

struct gpiodev {
	spinlock_t lock;

	int id;
	char *bankname;

	struct gpiodev_pin *pins;
	int nr_pins;

	int io_enabled;
	int irq_enabled;

	struct kfifo *irq_buf;
	spinlock_t irq_lock;

	struct device *io_dev;
	struct device *irq_dev;

	/* We can unify these somewhat by doing some fop switching
	 * magic, though I can't convince myself it's worth it*/
	struct cdev io_char_dev;
	struct cdev irq_char_dev;

	wait_queue_head_t irq_wq;
	struct fasync_struct *async_queue;

	struct config_group group;
};

struct gpiodev *devices[MAX_NR_DEVICES];
spinlock_t devices_lock;

static irqreturn_t irq_interrupt(int irq, void *dev_id)
{
	int i, found = 0;
	int gpio = irq_to_gpio(irq);

	struct gpiodev *bank = dev_id;

	for (i = 0; i < bank->nr_pins; i++) {
		if (bank->pins[i].pin == gpio) {
			found = 1;
			break;
		}
	}

	if (!found)
		return IRQ_NONE;

	BUF_PUT_INT(bank->irq_buf, i);

	wake_up_interruptible(&bank->irq_wq);

	if (bank->async_queue)
		kill_fasync(&bank->async_queue, SIGIO, POLL_IN);

	return IRQ_HANDLED;
}


static int io_open(struct inode *inode, struct file *file)
{
	struct gpiodev *bank = container_of(inode->i_cdev,
					      struct gpiodev,
					      io_char_dev);

	spin_lock(&bank->lock);
	config_item_get(&bank->group.cg_item);
	file->private_data = bank;
	spin_unlock(&bank->lock);

	return 0;
}

static int io_release(struct inode *inode, struct file *file)
{
	struct gpiodev *bank = file->private_data;

	spin_lock(&bank->lock);
	config_item_put(&bank->group.cg_item);
	spin_unlock(&bank->lock);

	return 0;
}

static ssize_t io_read(struct file *file, char __user *buf,
			     size_t count, loff_t *offset)
{
	int i, ret = 0;
	char *val;

	struct gpiodev *bank = file->private_data;

	count = min(count, (size_t)(bank->nr_pins - *offset));

	val = kmalloc(sizeof(char) * count, GFP_KERNEL);
	if (!val)
		return -ENOMEM;

	spin_lock(&bank->lock);
	for (i = *offset; i < *offset + count; i++) {
		int pin = bank->pins[i].pin;
		int value;

		if (file->f_flags & O_NONBLOCK)
			value = gpio_get_value(pin);
		else
			value = gpio_get_value_cansleep(pin);

		val[i] = (!!value) + '0';
	}

	*offset += count;
	spin_unlock(&bank->lock);

	if (copy_to_user(buf, val, count))
		ret = -EFAULT;

	kfree(val);
	return ret ? ret : count;
}

static ssize_t io_write(struct file *file, const char __user *buf,
			      size_t count, loff_t *offset)
{
	int i, ret = 0;
	char *val, *p;

	struct gpiodev *bank = file->private_data;

	count = min(count, (size_t)(bank->nr_pins - *offset));

	val = kmalloc(sizeof(char) * count, GFP_KERNEL);
	if (!val)
		return -ENOMEM;

	if (copy_from_user(val, buf, count)) {
		ret = -EFAULT;
		goto out_cpy;
	}

	p = val;

	spin_lock(&bank->lock);

	for (i = *offset; i < *offset + count; i++) {
		int pin = bank->pins[i].pin;
		int value = val[i] - '0';

		if (value != 0 || value != 1)
			break;

		bank->pins[i].outval = value;

		if (file->f_flags & O_NONBLOCK)
			gpio_set_value(pin, value);
		else
			gpio_set_value_cansleep(pin, value);
	}

	*offset = i;

	spin_unlock(&bank->lock);

out_cpy:
	kfree(val);
	return ret ? ret : count;
}

static loff_t io_llseek(struct file *file, loff_t off, int whence)
{
	struct gpiodev *bank = file->private_data;
	loff_t newpos;

	spin_lock(&bank->lock);

	switch (whence) {
	case SEEK_SET:
		newpos = off;
		break;
	case SEEK_CUR:
		newpos = file->f_pos + off;
		break;
	case SEEK_END:
		newpos = bank->nr_pins + off;
		break;
	default:
		return -EINVAL;
	}

	spin_unlock(&bank->lock);

	return newpos;
}

static struct file_operations io_fops = {
	.owner		= THIS_MODULE,
	.llseek		= io_llseek,
	.open		= io_open,
	.release	= io_release,
	.read		= io_read,
	.write		= io_write,
};

static int irq_open(struct inode *inode, struct file *file)
{
	struct gpiodev *bank = container_of(inode->i_cdev,
					      struct gpiodev,
					      io_char_dev);

	nonseekable_open(inode, file);

	spin_lock(&bank->lock);
	config_item_get(&bank->group.cg_item);
	file->private_data = bank;
	spin_unlock(&bank->lock);

	return 0;
}

static int irq_release(struct inode *inode, struct file *file)
{
	struct gpiodev *bank = file->private_data;

	spin_lock(&bank->lock);
	config_item_put(&bank->group.cg_item);
	spin_unlock(&bank->lock);

	return 0;
}

static ssize_t irq_read(struct file *file, char __user *buf,
			     size_t count, loff_t *offset)
{
	int i, len, *kbuf;
	struct gpiodev *bank = file->private_data;

	while (!kfifo_len(bank->irq_buf)) {

		if (file->f_flags & O_NONBLOCK)
			return -EAGAIN;

		if (wait_event_interruptible(bank->irq_wq,
					     kfifo_len(bank->irq_buf)))
			return -ERESTARTSYS;

	}

	len = min((size_t)kfifo_len(bank->irq_buf), count);

	kbuf = kmalloc(len * sizeof(int), GFP_KERNEL);

	if (!kbuf)
		return -ENOMEM;

	for (i = 0; i < len; i++)
		BUF_GET_INT(bank->irq_buf, kbuf[i]);

	if (copy_to_user(buf, kbuf, len)) {
		kfree(kbuf);
		return -EFAULT;
	}

	kfree(kbuf);
	return len;
}

static ssize_t irq_write(struct file *file, const char __user *buf,
			      size_t count, loff_t *offset)
{
	struct gpiodev *bank = file->private_data;

	/* Flush buffer */
	kfifo_reset(bank->irq_buf);

	return count;
}

static int irq_fasync(int fd, struct file *file, int mode)
{
	struct gpiodev *bank = file->private_data;

	return fasync_helper(fd, file, mode, &bank->async_queue);
}

static struct file_operations irq_fops = {
	.owner		= THIS_MODULE,
	.llseek		= no_llseek,
	.open		= irq_open,
	.release	= irq_release,
	.read		= irq_read,
	.write		= irq_write,
	.fasync		= irq_fasync,
};

/*
 * {enable,disable}_bank_{io,irq}() must be called under that bank's
 * lock.  They should only be called through their
 * set_bank_{io,irq}_enabled() which will (amongst other things)
 * take care of this locking for you.
 */
static int enable_bank_io(struct gpiodev *bank)
{
	int i;

	/* What error should a pin request failure be?? */
	int ret = -ENODEV;

	BUG_ON(!bank);

	for (i = 0; i < bank->nr_pins; i++) {
		if (!gpio_request(bank->pins[i].pin, bank->pins[i].label))
			goto err_alloc_pins;
	}

	cdev_init(&bank->io_char_dev, &io_fops);
	bank->io_char_dev.owner = THIS_MODULE;
	ret = cdev_add(&bank->io_char_dev, MKDEV(MAJOR(base_devt),
						 bank->id * 2), 1);

	if (ret < 0)
		goto err_cdev_add;

	bank->io_dev = device_create(dev_class, NULL,
				     MKDEV(MAJOR(base_devt), bank->id * 2),
				     "gpiod-%s", bank->bankname);

	if (IS_ERR(bank->io_dev)) {
		printk(KERN_ERR "gpio-dev: failed to create gpiod%d\n",
		       bank->id);
		ret = PTR_ERR(bank->io_dev);
		goto err_dev;
	}

	printk(KERN_INFO "gpio-dev: created gpiod%d as (%d:%d)\n",
	       bank->id, MAJOR(bank->io_dev->devt),
	       MINOR(bank->io_dev->devt));

	return 0;

err_dev:
	cdev_del(&bank->io_char_dev);
err_cdev_add:
err_alloc_pins:

	while (i--)
		gpio_free(bank->pins[i].pin);

	return ret;
}

static int disable_bank_io(struct gpiodev *bank)
{
	int i;

	BUG_ON(!bank);

	device_unregister(bank->io_dev);
	cdev_del(&bank->io_char_dev);

	for (i = 0; i < bank->nr_pins; i++)
		gpio_free(bank->pins[i].pin);

	return 0;
}

static int set_bank_io_enabled(struct gpiodev *bank, int enabled)
{
	int ret = 0;

	if (!bank) {
		printk(KERN_ERR "gpio-dev: Attempt to change enable " \
				"without valid device binding\n");
		return -EINVAL;
	}

	pr_debug("gpio-dev: setting bank %s enabled from %d to %d\n",
	       bank->bankname, bank->io_enabled, enabled);

	spin_lock(&bank->lock);

	if (bank->io_enabled == enabled)
		goto out;

	if (enabled)
		ret = enable_bank_io(bank);
	else
		ret = disable_bank_io(bank);

	if (!ret)
		bank->io_enabled = enabled;

out:
	spin_unlock(&bank->lock);
	return ret;
}


static int enable_bank_irq(struct gpiodev *bank)
{
	int ret;

	BUG_ON(!bank);

	cdev_init(&bank->irq_char_dev, &irq_fops);
	bank->irq_char_dev.owner = THIS_MODULE;
	ret = cdev_add(&bank->irq_char_dev, MKDEV(MAJOR(base_devt),
						bank->id * 2 + 1), 1);

	if (ret < 0)
		goto err_cdev_add;

	bank->irq_dev = device_create(dev_class, NULL,
					     MKDEV(MAJOR(base_devt), bank->id),
					     "gpioi-%s", bank->bankname);

	if (IS_ERR(bank->irq_dev)) {
		printk(KERN_ERR "gpio-dev: failed to create gpioi%d\n",
		       bank->id);
		ret = PTR_ERR(bank->irq_dev);
		goto err_dev;
	}

	printk(KERN_INFO "gpio-dev: created gpioi%d as (%d:%d)\n",
	       bank->id, MAJOR(bank->irq_dev->devt),
	       MINOR(bank->irq_dev->devt));

	return 0;

err_dev:
	cdev_del(&bank->irq_char_dev);
err_cdev_add:

	return ret;
}

static int disable_bank_irq(struct gpiodev *bank)
{
	BUG_ON(!bank);

	device_unregister(bank->irq_dev);
	cdev_del(&bank->irq_char_dev);

	return 0;
}

static int set_bank_irq_enabled(struct gpiodev *bank, int enabled)
{
	int ret = 0;

	if (!bank) {
		printk(KERN_ERR "gpio-dev: Attempt to change irq enable " \
				"without valid device binding\n");
		return -EINVAL;
	}

	pr_debug("gpio-dev: setting bank irq %s enabled from %d to %d\n",
	       bank->bankname, bank->irq_enabled, enabled);

	spin_lock(&bank->lock);

	if (bank->irq_enabled == enabled)
		goto out;

	if (enabled)
		ret = enable_bank_irq(bank);
	else
		ret = disable_bank_irq(bank);

	if (!ret)
		bank->irq_enabled = enabled;

out:
	spin_unlock(&bank->lock);
	return ret;
}

static inline struct gpiodev *to_gpiodev(struct config_group *group)
{
	return group ? container_of(group, struct gpiodev, group) : NULL;
}

static struct configfs_attribute bank_attr_enableio = {
	.ca_owner = THIS_MODULE,
	.ca_name = "enable-io",
	.ca_mode = S_IRUGO | S_IWUGO,
};

static struct configfs_attribute bank_attr_enableirq = {
	.ca_owner = THIS_MODULE,
	.ca_name = "enable-irq",
	.ca_mode = S_IRUGO | S_IWUGO,
};

static struct configfs_attribute *cfg_bank_attrs[] = {
	&bank_attr_enableio,
	&bank_attr_enableirq,
	NULL,
};

static ssize_t cfg_bank_attr_show(struct config_item *item,
				      struct configfs_attribute *attr,
				      char *page)
{
	struct gpiodev *bank = to_gpiodev(to_config_group(item));

	if (attr == &bank_attr_enableio)
		return sprintf(page, "%d\n", bank->io_enabled);
	else if (attr == &bank_attr_enableirq)
		return sprintf(page, "%d\n", bank->irq_enabled);
	else
		WARN_ON(1);

	return 0;
}

static ssize_t cfg_bank_attr_store(struct config_item *item,
				       struct configfs_attribute *attr,
				       const char *page, size_t count)
{
	struct gpiodev *bank = to_gpiodev(to_config_group(item));

	/* The only attributes which will come here are enable-{io,irq} */

	unsigned long tmp;
	int ret = 0;
	char *p = (char *) page;

	tmp = simple_strtoul(p, &p, 10);
	if (!p || (*p && (*p != '\n')))
		return -EINVAL;

	if (tmp > INT_MAX)
		return -ERANGE;

	/* Booleanize */
	tmp = !!tmp;

	pr_debug("gpio-dev: changing enabled attr on bank %s, attribute %d\n",
	       bank->bankname, (int)attr);

	if (attr == &bank_attr_enableio)
		ret = set_bank_io_enabled(bank, tmp);
	else if (attr == &bank_attr_enableirq)
		ret = set_bank_irq_enabled(bank, tmp);
	else {
		printk(KERN_ERR "unexpected attribute at %s, %d",
			__func__, __LINE__);
		WARN_ON(1);
	}

	return ret ? ret : count;
}

static struct configfs_item_operations cfg_bank_item_ops = {
	.show_attribute		= cfg_bank_attr_show,
	.store_attribute	= cfg_bank_attr_store,
};

static struct config_item_type cfg_bank_type = {
	.ct_owner	= THIS_MODULE,
	.ct_item_ops	= &cfg_bank_item_ops,
	.ct_attrs	= cfg_bank_attrs,
};

static struct config_group *cfg_make_group(struct config_group *group,
						   const char *name)
{
	int i;

	struct gpiodev *dev = NULL;

	spin_lock(&devices_lock);
	for (i = 0; i < MAX_NR_DEVICES; i++) {
		if (!devices[i])
			continue;

		if (strcmp(name, devices[i]->bankname) == 0) {
			dev = devices[i];
			printk(KERN_INFO "gpio-dev: Bound configfs instance" \
			       " to device %s\n", devices[i]->bankname);
		}
	}
	spin_unlock(&devices_lock);

	if (!dev) {
		printk(KERN_WARNING "gpio-dev: Can't find bank \"%s\"\n", name);
		/*
		 * Once I get home and can modify the configfs_mkdir code
		 * to allow more informative return types we can modify this
		 * to, eg, ERR_PTR(-ENODEV)
		 */
		return NULL;
	}

	/*
	 * REVISIT: Do we need to get this here?  The docco says we should drop
	 * a reference in drop_group() so I assume we need to actually get one
	 * at one stage
	 */
	config_item_get(&dev->group.cg_item);

	return &dev->group;
}

static void cfg_drop_group(struct config_group *group,
				   struct config_item *item)
{
	struct gpiodev *bank = to_gpiodev(to_config_group(item));

	pr_debug("gpio-dev: bank %s getting dropped\n",
	       bank->bankname);

	set_bank_io_enabled(bank, 0);
	set_bank_irq_enabled(bank, 0);

	config_item_put(item);
}

static struct configfs_attribute attr_banks = {
	.ca_owner = THIS_MODULE,
	.ca_name = "banks",
	.ca_mode = S_IRUGO,
};

static struct configfs_attribute *cfg_attrs[] = {
	&attr_banks,
	NULL,
};

static ssize_t cfg_attr_show(struct config_item *item,
					struct configfs_attribute *attr,
					char *page)
{
	int i;
	int count = 0;

	/* The only attribute which comes here is 'banks' */

	spin_lock(&devices_lock);
	for (i = 0; i < MAX_NR_DEVICES; i++) {
		if (devices[i])
			count += sprintf(page + count, "%s\n",
					 devices[i]->bankname);
	}
	spin_unlock(&devices_lock);

	return count;
}

static struct configfs_item_operations cfg_item_ops = {
	.show_attribute	= cfg_attr_show,
};

static struct configfs_group_operations cfg_group_ops = {
	.make_group	= cfg_make_group,
	.drop_item	= cfg_drop_group,
};

static struct config_item_type cfg_type = {
	.ct_item_ops	= &cfg_item_ops,
	.ct_group_ops	= &cfg_group_ops,
	.ct_attrs	= cfg_attrs,
	.ct_owner	= THIS_MODULE,
};

static struct configfs_subsystem cfg_subsys = {
	.su_group = {
		.cg_item = {
			.ci_namebuf = "gpio-dev",
			.ci_type = &cfg_type,
		},
	},
};


static struct configfs_attribute pin_attr_index = {
	.ca_owner = THIS_MODULE,
	.ca_name = "index",
	.ca_mode = S_IRUGO,
};

static struct configfs_attribute pin_attr_direction = {
	.ca_owner = THIS_MODULE,
	.ca_name = "direction",
	.ca_mode = S_IRUGO | S_IWUGO,
};

static struct configfs_attribute pin_attr_irq = {
	.ca_owner = THIS_MODULE,
	.ca_name = "irq",
	.ca_mode = S_IRUGO | S_IWUGO,
};

static struct configfs_attribute *pin_attrs[] = {
	&pin_attr_index,
	&pin_attr_direction,
	&pin_attr_irq,
	NULL,
};

static inline struct gpiodev_pin *to_gpiodev_pin(struct config_group *group)
{
	return group ? container_of(group, struct gpiodev_pin, group) : NULL;
}

static ssize_t pin_show(struct config_item *item,
				      struct configfs_attribute *attr,
				      char *page)
{
	struct gpiodev_pin *pin = to_gpiodev_pin(to_config_group(item));

	/* All attributes for a pin come here */

	if (attr == &pin_attr_index)
		return sprintf(page, "%d\n", pin->index);
	else if (attr == &pin_attr_direction)
		return sprintf(page, "%d\n", pin->direction);
	else if (attr == &pin_attr_irq)
		return sprintf(page, "%d\n", pin->irq);
	else {
		printk(KERN_ERR "gpio-dev: Unexpected attribute in %s\n",
		       __func__);
		return 0;
	}
}

static ssize_t pin_store(struct config_item *item,
				 struct configfs_attribute *attr,
				 const char *page, size_t count)
{
	struct gpiodev_pin *pin = to_gpiodev_pin(to_config_group(item));
	char *p = (char *)page;
	int val;
	int ret = 0;

	val = simple_strtoul(p, &p, 0);
	if (!p || (*p && (*p != '\n')))
		return -EINVAL;

	if (attr == &pin_attr_direction) {
		pin->direction = val;

		if (val)
			gpio_direction_output(pin->pin, pin->outval);
		else
			gpio_direction_input(pin->pin);


	} else if (attr == &pin_attr_irq) {

		if (!pin->irq && val) {
			ret = request_irq(gpio_to_irq(pin->pin), irq_interrupt,
					  0, "gpio-dev", pin);
			if (ret)
				return ret;
		} else if (pin->irq && !val) {
			free_irq(gpio_to_irq(pin->pin), pin);
		}

		pin->irq = val;

		/*
		 * The values written by the user directly correspond to
		 * the values behind the IRQF_TRIGGER_* tokens.  Can we
		 * rely on this or should we explicitly check and set each
		 * possible trigger type?
		 */
		if (val)
			ret = set_irq_type(gpio_to_irq(pin->pin), val);

		if (ret)
			return -EINVAL;

	} else {
		printk(KERN_ERR "gpio-dev: Unexpected attribute in %s\n",
		       __func__);
	}

	return count;
}

static struct configfs_item_operations pin_ops = {
	.show_attribute = pin_show,
	.store_attribute = pin_store,
};

static struct config_item_type cfg_pin_type = {
	.ct_owner	= THIS_MODULE,
	.ct_attrs	= pin_attrs,
	.ct_item_ops	= &pin_ops,
};

static int __init gpiodev_probe(struct platform_device *pdev)
{
	int i, ret;
	struct gpiodev *bank = NULL;
	struct device *dev = &pdev->dev;
	struct gpiodev_pdata *pdata = dev->platform_data;

	if (!pdata) {
		printk(KERN_ERR "gpio-dev: No configuration data available; "\
			"aborting probe.");
		return -EINVAL;
	}

	bank = kzalloc(sizeof(struct gpiodev), GFP_KERNEL);
	if (!bank)
		return -ENOMEM;

	spin_lock_init(&bank->lock);
	spin_lock_init(&bank->irq_lock);

	bank->bankname = pdata->bankname;
	bank->nr_pins = pdata->nr_pins;

	bank->pins = kmalloc(bank->nr_pins * sizeof(struct gpiodev_pin),
			     GFP_KERNEL);

	if (!bank->pins) {
		ret = -ENOMEM;
		goto err_pin_alloc;
	}

	bank->irq_buf = kfifo_alloc(IRQ_BUFFER_SIZE, GFP_KERNEL,
				    &bank->irq_lock);

	if (IS_ERR(bank->irq_buf)) {
		ret = PTR_ERR(bank->irq_buf);
		goto err_irq_buf_alloc;
	}

	memcpy(bank->pins, pdata->pins,
	       bank->nr_pins * sizeof(struct gpiodev_pin));

	pr_debug("gpio-dev: initing device group %s\n", bank->bankname);

	config_group_init_type_name(&bank->group, bank->bankname,
				    &cfg_bank_type);

	bank->group.default_groups = kzalloc(sizeof(struct config_group)
					     * bank->nr_pins, GFP_KERNEL);

	if (!bank->group.default_groups) {
		ret = -ENOMEM;
		goto err_group_alloc;
	}

	for (i = 0; i < bank->nr_pins; i++) {
		pr_debug("gpio-dev: initing pin %s on bank %s\n",
			bank->pins[i].label, bank->bankname);
		config_group_init_type_name(&bank->pins[i].group,
			bank->pins[i].label, &cfg_pin_type);
		bank->group.default_groups[i] = &bank->pins[i].group;
	}

	bank->io_enabled = 0;
	bank->irq_enabled = 0;

	platform_set_drvdata(pdev, bank);

	bank->id = -1;

	/* Find first empty slot */
	spin_lock(&devices_lock);
	for (i = 0; i < MAX_NR_DEVICES; i++) {
		if (devices[i] == NULL) {
			devices[i] = bank;
			bank->id = i;
			break;
		}
	}
	spin_unlock(&devices_lock);

	if (bank->id == -1) {
		printk(KERN_ERR "gpio-dev: More than MAX_NR_DEVICES probed;"\
				" no free slots found!");
		ret = -ENODEV;
		goto err_no_slot;
	}

	return 0;

err_no_slot:
	kfree(bank->group.default_groups);
err_group_alloc:
	kfifo_free(bank->irq_buf);
err_irq_buf_alloc:
	kfree(bank->pins);
err_pin_alloc:
	kfree(bank);

	return ret;
}

static int __exit gpiodev_remove(struct platform_device *pdev)
{
	struct gpiodev *bank = platform_get_drvdata(pdev);

	spin_lock(&devices_lock);
	devices[bank->id] = NULL;
	spin_unlock(&devices_lock);

	kfifo_free(bank->irq_buf);

	kfree(bank->pins);
	kfree(bank);

	return 0;
}

static struct platform_driver gpiodev_driver = {
	.driver		= {
		.name	= "gpio-dev",
		.owner	= THIS_MODULE,
	},
	.remove		= __exit_p(gpiodev_remove),
	.probe		= gpiodev_probe,
};

static int __init gpiodev_init(void)
{
	int ret;

	spin_lock_init(&devices_lock);

	dev_class = class_create(THIS_MODULE, "gpio-dev");
	if (IS_ERR(dev_class)) {
		ret = PTR_ERR(dev_class);
		goto err_class_create;
	}

	ret = alloc_chrdev_region(&base_devt, 0, MAX_NR_DEVICES * 2, "gpio-dev");
	if (ret < 0)
		goto err_alloc_chrdev;

	pr_debug("gpio-dev: Registering configfs subsystem %s\n",
		cfg_subsys.su_group.cg_item.ci_namebuf);
	config_group_init(&cfg_subsys.su_group);
	mutex_init(&cfg_subsys.su_mutex);
	ret = configfs_register_subsystem(&cfg_subsys);
	if (ret) {
		printk(KERN_ERR "Error %d while registering subsystem %s\n",
		       ret,
		       cfg_subsys.su_group.cg_item.ci_namebuf);
		goto err_subsys;
	}


	ret = platform_driver_register(&gpiodev_driver);
	if (ret)
		goto err_dev_register;

	return 0;

err_dev_register:
	configfs_unregister_subsystem(&cfg_subsys);
err_subsys:
	unregister_chrdev_region(base_devt, MAX_NR_DEVICES);
err_alloc_chrdev:
	class_destroy(dev_class);
err_class_create:
	printk(KERN_ALERT "gpio-dev: Failed to init gpio-dev interface\n");
	return ret;
}

static void __exit gpiodev_exit(void)
{
	platform_driver_unregister(&gpiodev_driver);

	configfs_unregister_subsystem(&cfg_subsys);

	unregister_chrdev_region(base_devt, MAX_NR_DEVICES * 2);

	class_destroy(dev_class);

	printk(KERN_INFO "gpio-dev: unloaded gpio-dev\n");
}

module_init(gpiodev_init);
module_exit(gpiodev_exit);
MODULE_LICENSE("GPL");
MODULE_AUTHOR("Ben Nizette <bn@niasdigital.com>");
MODULE_DESCRIPTION("Userspace interface to the gpio framework");

Re: [RFC PATCHES] Re: Is configfs the right solution for configuration based fs?

[Joel Becker]

On Fri, Jun 20, 2008 at 07:22:56PM +1000, Ben Nizette wrote:
> A version of my gpio-dev interface is attached.  Bear in mind it was
> never completed, it's full of known bugs but hey, might be useful for
> you anyway :-)

	Looks good.  Fits about what I would expect a configfs interface
to look like, with simple show/store stuff.  I can see where some macros
would have shortened some bolierplate.
	Was there any other boilerplate you found cumbersome?  I mean
outside of defining attribute structs and the show/store_attr()
trampoline functions?  Let me know, so I can incorporate it.

Joel

-- 

Life's Little Instruction Book #444

	"Never underestimate the power of a kind word or deed."

Joel Becker
Principal Software Developer
Oracle
E-mail: joel.becker@oracle.com
Phone: (650) 506-8127

Re: [RFC PATCHES] Re: Is configfs the right solution for configuration based fs?

[Ben Nizette]

On Fri, 2008-06-20 at 14:37 -0700, Joel Becker wrote:
> On Fri, Jun 20, 2008 at 07:22:56PM +1000, Ben Nizette wrote:
> > A version of my gpio-dev interface is attached.  Bear in mind it was
> > never completed, it's full of known bugs but hey, might be useful for
> > you anyway :-)
> 
> 	Looks good.  Fits about what I would expect a configfs interface
> to look like, with simple show/store stuff.  I can see where some macros
> would have shortened some bolierplate.
> 	Was there any other boilerplate you found cumbersome?  I mean
> outside of defining attribute structs and the show/store_attr()
> trampoline functions?  Let me know, so I can incorporate it.
> 

You know, I think you've about covered the boilerplate work.  Apart from
that, well it took me a millisecond to work out what the point of
config_{group,item}s was; I went in kinda expecting to see one struct
for directories and one for attributes.  In fact I still not sure I can
explain the need for config_items separate from config_groups.  Little
help?  :-)

	--Ben.

> Joel
> 

Re: [RFC PATCHES] Re: Is configfs the right solution for configuration based fs?

[Joel Becker]

On Sat, Jun 21, 2008 at 11:03:49AM +1000, Ben Nizette wrote:
> You know, I think you've about covered the boilerplate work.  Apart from
> that, well it took me a millisecond to work out what the point of
> config_{group,item}s was; I went in kinda expecting to see one struct
> for directories and one for attributes.  In fact I still not sure I can
> explain the need for config_items separate from config_groups.  Little
> help?  :-)

	Groups can create children, items cannot.  Think of kset vs
kobject, which is where it came from.
	Don't worry about directories vs files.  The view from the
client subsystem isn't about filesystem objects.  It's about a hierarchy
of items.  An item is a sigle entity.  It can have attributes.  A group
is an item that can have children.
	The fact that you access it via a filesystem is separate.  We
could have created a system call instead - the callbacks to your client
subsystem would have been the same.  Does that help?

Joel

-- 

Life's Little Instruction Book #511

	"Call your mother."

Joel Becker
Principal Software Developer
Oracle
E-mail: joel.becker@oracle.com
Phone: (650) 506-8127

Re: [RFC PATCHES] Re: Is configfs the right solution for configuration based fs?

[Ben Nizette]

On Fri, 2008-06-20 at 19:02 -0700, Joel Becker wrote:
> On Sat, Jun 21, 2008 at 11:03:49AM +1000, Ben Nizette wrote:
> > You know, I think you've about covered the boilerplate work.  Apart from
> > that, well it took me a millisecond to work out what the point of
> > config_{group,item}s was; I went in kinda expecting to see one struct
> > for directories and one for attributes.  In fact I still not sure I can
> > explain the need for config_items separate from config_groups.  Little
> > help?  :-)
> 
> 	Groups can create children, items cannot.  Think of kset vs
> kobject, which is where it came from.
> 	Don't worry about directories vs files.  The view from the
> client subsystem isn't about filesystem objects.  It's about a hierarchy
> of items.  An item is a sigle entity.  It can have attributes.  A group
> is an item that can have children.

I guess I don't quite see why all items don't have the ability to have
children.  I mean, a config_item has a config_item_type which contains
the group_ops anyway; I don't quite see why the children, default_groups
and subsys members from config_group don't live inside config_item and
be done with it.

In the kobject/kset case the separation makes sense as kobjects are
embedded in things all over the kernel controlling things like ref
counting, device model glue, hotplug info as well as the sysfs
representation.  The config_item doesn't have any responsibilities
outside of the configfs representation (does it?) so the analogy isn't
100%.  Though of course the item/group split makes sense if there are
grander plans for the config_item in the future.

> 	The fact that you access it via a filesystem is separate.  We
> could have created a system call instead - the callbacks to your client
> subsystem would have been the same.

Right

>   Does that help?

Indeed, thx.

	--Ben.

> 
> Joel
> 

Re: [RFC PATCHES] Re: Is configfs the right solution for configuration based fs?

[Joel Becker]

On Sat, Jun 21, 2008 at 06:03:42PM +1000, Ben Nizette wrote:
> In the kobject/kset case the separation makes sense as kobjects are
> embedded in things all over the kernel controlling things like ref
> counting, device model glue, hotplug info as well as the sysfs
> representation.  The config_item doesn't have any responsibilities
> outside of the configfs representation (does it?) so the analogy isn't
> 100%.  Though of course the item/group split makes sense if there are
> grander plans for the config_item in the future.

	The config_item is indeed embedded in whatever struct it maps
too.  eg, the ocfs2 nodemanager can have a couple hundred nodes, and
that's one config_item per.  The fs/dlm stuff can have even more nodes.

Joel

-- 

Life's Little Instruction Book #222

	"Think twice before burdening a friend with a secret."

Joel Becker
Principal Software Developer
Oracle
E-mail: joel.becker@oracle.com
Phone: (650) 506-8127

Re: [RFC PATCHES] Re: Is configfs the right solution for configuration based fs?

[Ben Nizette]

On Sat, 2008-06-21 at 01:44 -0700, Joel Becker wrote:
> On Sat, Jun 21, 2008 at 06:03:42PM +1000, Ben Nizette wrote:
> > In the kobject/kset case the separation makes sense as kobjects are
> > embedded in things all over the kernel controlling things like ref
> > counting, device model glue, hotplug info as well as the sysfs
> > representation.  The config_item doesn't have any responsibilities
> > outside of the configfs representation (does it?) so the analogy isn't
> > 100%.  Though of course the item/group split makes sense if there are
> > grander plans for the config_item in the future.
> 
> 	The config_item is indeed embedded in whatever struct it maps
> too.  eg, the ocfs2 nodemanager can have a couple hundred nodes, and
> that's one config_item per.  The fs/dlm stuff can have even more nodes.

Righteo, I guess the few 10s of bytes per struct saved by not tracking
children if we know that none can ever exist is the aim here?  Coming
from an embedded background that makes sense to me.

>From a conceptual point of view I still think they would work better
amalgamated in the configfs case but really this whole discussion is
pretty moot. I'm certainly not advocating a re-write of anything in
there!

Thanks for the clarifications (and patience ^_^), I'll do some work
using the new macros and get back to you asap.

Cheers,
	--Ben.

> 
> Joel
>