Re: [B.A.T.M.A.N.] [PATCH 13/26] staging:batman-adv: convert multiple /proc files to use sysfs

[Sven Eckelmann <sven.eckelmann@gmx.de>]

[-- Attachment #1: Type: Text/Plain, Size: 4862 bytes --]

Thank you for the fast answer. 

Greg KH wrote:
> On Thu, May 13, 2010 at 01:19:38AM +0800, Marek Lindner wrote:
> > Hi Greg,
> > 
> > > Please document all sysfs files with a Documentation/ABI/ entry.  You
> > > can put it in the drivers/staging/batman-adv/ directory for now.
> > > 
> > > Also note, binary sysfs files are for things that are "pass-through",
> > > not for things that you try to intrepret like I think you are doing
> > > here.  What's wrong with configfs for something like this?
> > 
> > I drafted the ABI documentation which you will find attached to this
> > mail. Since this is my very first attempt I'd appreciate feedback from
> > your side before we submit it. I looked at various existing ABI docs in
> > the linux kernel tree to get an idea how it should look like. Bear with
> > me if it is not perfect yet.  ;-)
> > 
> > Regarding the question which kernel configuration interface to use, we
> > would be more than happy to receive some guidance. At this point we are
> > rather unsure which of the many (procfs/sysfs/configfs/etc) filesystems
> > are we supposed to go forward with. To better judge our case I recommend
> > reading this page http://www.open-mesh.org/wiki/tweaking-batman-adv
> > as it explains the purpose of the various sysfs files.
> > 
> > Right now, I would say that configfs does not belong to my favorite
> > solutions, simply because it seems to be a rather new/uncommon choice.
> 
> configfs is very old, and has been around almost as long as sysfs has
> been.  But yes, it's not used as much, that is true.
> 
> > Batman's main deployment field are embedded devices (mostly low cost
> > routers) that have quite some contraints regarding cpu power and
> > available disk space. Typically, those systems try to deactivate all
> > "unnecessary" functionality in the kernel to have a few more bytes
> > available for other stuff. For example OpenWRT: They deactivated
> > configfs on all platforms (except one). I fear we create more problems
> > than we solve if we go down this path.
> 
> Well, it is simple to enable configfs if you need/want batman, right?
> It's not that much extra kernel code.

Yes, it should be possible to enable it. It was just meant to describe the 
current situation and the target systems were batman-adv is currently used. 
There is for example the requirement by one developer that it runs on his 
hardware which currently only support 2.6.24 as kernel due to different out of 
kernel drivers. There were also some request to port it back to 2.6.15 to get 
the Atheros drivers working... which is out of scope for us due to different 
changes in the kernel since 2.6.15. It isn't always easy to support all those 
requirements and get it in the right shape for a merge in the mainline kernel 
(and my/our gap in education about all virtualfs doesn't help either).

This makes invalid entries in sysfs not better, but maybe helps you to get in 
the correct mood to point us in the right direction.

> > What:           /sys/class/net/<mesh_iface>/mesh/originators
[...]
> 
> Sorry, but sysfs is for "one value per file" attributes, which this one
> violates.
> 
> Is this something that everyone always needs to know?  Or could it be a
> debugfs file?

Everyone/always - not really. They are needed by someone who wants to test and 
debug the status of batman-adv - which sounds like a perfect target for 
debugfs, right?

So batctl has to check if /sys/kernel/debug/ is mounted (or the batman-
adv/mesh_iface/originator) file can be found and give the user a helpful error 
message that either batman-adv is not loaded, debugfs is not mounted at 
/sys/kernel/debug/ or the mesh_iface just doesn't exists.

> > What:           /sys/class/net/<mesh_iface>/mesh/transtable_global
[...]
> 
> Again, no "tables" in sysfs please.
> 
> > What:           /sys/class/net/<mesh_iface>/mesh/transtable_local
[...]
> 
> Same here.
> 
> > What:           /sys/class/net/<mesh_iface>/mesh/vis_mode
[...]
> > 
> > What:           /sys/class/net/<mesh_iface>/mesh/vis_data
[...]
> 
> Again, this is not a single value, so please move it somewhere else.

As far as I can see only these files with (more or less) debug information are 
the problem right now and should go to the debugfs?

The rest can be in sysfs?:
 * /sys/class/net/<iface>/batman-adv/mesh_iface
 * /sys/class/net/<iface>/batman-adv/iface_status
 * /sys/class/net/<mesh_iface>/mesh/aggregate_ogm
 * /sys/class/net/<mesh_iface>/mesh/orig_interval
 * /sys/class/net/<mesh_iface>/mesh/vis_mode (not sure about that one because
   it is in the no-list, but without comment. You can only write "client"/
   "server" into that file and read the same value. So probably in the sysfs
   list)


Best regards,
	Sven Eckelmann

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