drivers/net/iseries_veth.c dubious sysfs usage

drivers/net/iseries_veth.c dubious sysfs usage

[Greg KH]

In doing a massive kobject cleanup of the kernel tree, I ran across the
iseries_veth.c driver.

It looks like the driver is creating a number of subdirectories under
the driver sysfs directory.  This is odd and probably wrong.  You want
these virtual connections to show up in the main sysfs device tree, not
under the driver directory.

I'll be glad to totally guess and try to move it around in the sysfs
tree, but odds are I'll get it all wrong as I can't really test this
out :)

Any hints on what this driver is trying to do in this sysfs directories?

thanks,

greg k-h

Re: drivers/net/iseries_veth.c dubious sysfs usage

[Michael Ellerman]

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

On Wed, 2007-12-05 at 01:30 -0800, Greg KH wrote:
> In doing a massive kobject cleanup of the kernel tree, I ran across the
> iseries_veth.c driver.
> 
> It looks like the driver is creating a number of subdirectories under
> the driver sysfs directory.  This is odd and probably wrong.  You want
> these virtual connections to show up in the main sysfs device tree, not
> under the driver directory.
> 
> I'll be glad to totally guess and try to move it around in the sysfs
> tree, but odds are I'll get it all wrong as I can't really test this
> out :)
> 
> Any hints on what this driver is trying to do in this sysfs directories?

I wrote the code, I think, but it's been a while - I'll have a look at
it tomorrow.

Why is it "odd and probably wrong" to create subdirectories under the
driver in sysfs?

cheers

-- 
Michael Ellerman
OzLabs, IBM Australia Development Lab

wwweb: http://michael.ellerman.id.au
phone: +61 2 6212 1183 (tie line 70 21183)

We do not inherit the earth from our ancestors,
we borrow it from our children. - S.M.A.R.T Person

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

Re: drivers/net/iseries_veth.c dubious sysfs usage

[Greg KH]

On Wed, Dec 05, 2007 at 10:10:31PM +1100, Michael Ellerman wrote:
> On Wed, 2007-12-05 at 01:30 -0800, Greg KH wrote:
> > In doing a massive kobject cleanup of the kernel tree, I ran across the
> > iseries_veth.c driver.
> > 
> > It looks like the driver is creating a number of subdirectories under
> > the driver sysfs directory.  This is odd and probably wrong.  You want
> > these virtual connections to show up in the main sysfs device tree, not
> > under the driver directory.
> > 
> > I'll be glad to totally guess and try to move it around in the sysfs
> > tree, but odds are I'll get it all wrong as I can't really test this
> > out :)
> > 
> > Any hints on what this driver is trying to do in this sysfs directories?
> 
> I wrote the code, I think, but it's been a while - I'll have a look at
> it tomorrow.

Yes, can you send me the sysfs tree output of the driver directory, and
what exactly the different files in there are supposed to be used for?

> Why is it "odd and probably wrong" to create subdirectories under the
> driver in sysfs?

Because a driver does not have "devices" under it in the sysfs tree.
All devices liven in the /sys/devices/ tree so we can properly manage
them that way.  A driver will then bind to a device, and the driver core
will set up the linkages in sysfs properly so that everthing looks
uniform.

By creating subdirectories associated with a driver, this breaks the
model that the entire rest of the kernel is using, which is something
that you really don't want to be doing :)

How about describing what you were trying to achieve with these
directories and files?

thanks,

greg k-h

Re: drivers/net/iseries_veth.c dubious sysfs usage

[Michael Ellerman]

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

On Wed, 2007-12-05 at 13:41 -0800, Greg KH wrote:
> On Wed, Dec 05, 2007 at 10:10:31PM +1100, Michael Ellerman wrote:
> > On Wed, 2007-12-05 at 01:30 -0800, Greg KH wrote:
> > > In doing a massive kobject cleanup of the kernel tree, I ran across the
> > > iseries_veth.c driver.
> > > 
> > > It looks like the driver is creating a number of subdirectories under
> > > the driver sysfs directory.  This is odd and probably wrong.  You want
> > > these virtual connections to show up in the main sysfs device tree, not
> > > under the driver directory.
> > > 
> > > I'll be glad to totally guess and try to move it around in the sysfs
> > > tree, but odds are I'll get it all wrong as I can't really test this
> > > out :)
> > > 
> > > Any hints on what this driver is trying to do in this sysfs directories?
> > 
> > I wrote the code, I think, but it's been a while - I'll have a look at
> > it tomorrow.
> 
> Yes, can you send me the sysfs tree output of the driver directory, and
> what exactly the different files in there are supposed to be used for?

Sure. My version of tar (1.15.1) doesn't seem to be able to tar up /sys,
so hopefully this is sufficient:

igoeast:~# cd /sys/class/net/eth1/
igoeast:/sys/class/net/eth1# ls -la
total 0
drwxr-xr-x  4 root root    0 Dec  6 10:22 .
drwxr-xr-x  6 root root    0 Dec  6 10:21 ..
-r--r--r--  1 root root 4096 Dec  6 10:30 addr_len
-r--r--r--  1 root root 4096 Dec  6 10:30 address
-r--r--r--  1 root root 4096 Dec  6 10:30 broadcast
-r--r--r--  1 root root 4096 Dec  6 10:30 carrier
lrwxrwxrwx  1 root root    0 Dec  6 10:22 device -> ../../../devices/vio/3
-r--r--r--  1 root root 4096 Dec  6 10:30 dormant
-r--r--r--  1 root root 4096 Dec  6 10:30 features
-rw-r--r--  1 root root 4096 Dec  6 10:30 flags
-r--r--r--  1 root root 4096 Dec  6 10:30 ifindex
-r--r--r--  1 root root 4096 Dec  6 10:30 iflink
-r--r--r--  1 root root 4096 Dec  6 10:30 link_mode
-rw-r--r--  1 root root 4096 Dec  6 10:30 mtu
-r--r--r--  1 root root 4096 Dec  6 10:30 operstate
drwxr-xr-x  2 root root    0 Dec  6 10:30 statistics
lrwxrwxrwx  1 root root    0 Dec  6 10:30 subsystem -> ../../../class/net
-rw-r--r--  1 root root 4096 Dec  6 10:30 tx_queue_len
-r--r--r--  1 root root 4096 Dec  6 10:30 type
-rw-r--r--  1 root root 4096 Dec  6 10:30 uevent
drwxr-xr-x  2 root root    0 Dec  6 10:30 veth_port

Each net device has a port structure associated with it, the fields
should be fairly self explanatory, they're all read only I think.

igoeast:/sys/class/net/eth1# find veth_port/
veth_port/
veth_port/mac_addr
veth_port/lpar_map
veth_port/stopped_map
veth_port/promiscuous
veth_port/num_mcast


igoeast:/sys/class/net/eth1# cd device/driver

igoeast:/sys/class/net/eth1/device/driver# ls -l
total 0
lrwxrwxrwx  1 root root    0 Dec  6 10:21 2 -> ../../../../devices/vio/2
lrwxrwxrwx  1 root root    0 Dec  6 10:21 3 -> ../../../../devices/vio/3
--w-------  1 root root 4096 Dec  6 10:21 bind
drwxr-xr-x  2 root root    0 Dec  6 10:21 cnx00
drwxr-xr-x  2 root root    0 Dec  6 10:21 cnx02
drwxr-xr-x  2 root root    0 Dec  6 10:21 cnx03
drwxr-xr-x  2 root root    0 Dec  6 10:21 cnx04
lrwxrwxrwx  1 root root    0 Dec  6 10:21 module -> ../../../../module/iseries_veth
--w-------  1 root root 4096 Dec  6 10:21 uevent
--w-------  1 root root 4096 Dec  6 10:21 unbind

The driver has a connection to all the other lpars, this is entirely
independent of the net devices.

igoeast:/sys/class/net/eth1/device/driver# find cnx00/
cnx00/
cnx00/outstanding_tx
cnx00/remote_lp
cnx00/num_events
cnx00/reset_timeout
cnx00/last_contact
cnx00/state
cnx00/src_inst
cnx00/dst_inst
cnx00/num_pending_acks
cnx00/num_ack_events
cnx00/ack_timeout


> > Why is it "odd and probably wrong" to create subdirectories under the
> > driver in sysfs?
> 
> Because a driver does not have "devices" under it in the sysfs tree.
> All devices liven in the /sys/devices/ tree so we can properly manage
> them that way.  A driver will then bind to a device, and the driver core
> will set up the linkages in sysfs properly so that everthing looks
> uniform.

OK. They're not "devices" that we create under the driver, they're just
attributes of the driver, and they happen to be in groups so I put them
in subdirectories.

cheers

-- 
Michael Ellerman
OzLabs, IBM Australia Development Lab

wwweb: http://michael.ellerman.id.au
phone: +61 2 6212 1183 (tie line 70 21183)

We do not inherit the earth from our ancestors,
we borrow it from our children. - S.M.A.R.T Person

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

[PATCH] Introduce driver_create/remove_dir

[Stephen Rothwell]

Signed-off-by: Stephen Rothwell <sfr@canb.auug.org.au>
---
 drivers/base/driver.c      |   24 ++++++++++++++++++++++++
 drivers/net/iseries_veth.c |   15 +++++++--------
 include/linux/device.h     |    3 +++
 3 files changed, 34 insertions(+), 8 deletions(-)

Greg, does this look like a reasonable solution to iseries_veth accessing
the "private" kobject in struct device_driver?  This version is against
maimline, but the stuff you have in mm would just need to update
driver_create_dir ...

Also something along the lines of device_add_dir() might be good if you
want to hide the kobject in struct device as well.

diff --git a/drivers/base/driver.c b/drivers/base/driver.c
index eb11475..6527a91 100644
--- a/drivers/base/driver.c
+++ b/drivers/base/driver.c
@@ -91,6 +91,30 @@ struct device * driver_find_device(struct device_driver *drv,
 EXPORT_SYMBOL_GPL(driver_find_device);
 
 /**
+ *	driver_create_dir - create a subdirectory for a driver.
+ *	@drv:	driver.
+ *	@kobj:	the kobject we are creating the directory for.
+ */
+int __must_check driver_create_dir(struct device_driver *drv,
+			struct kobject *kobj)
+{
+	kobj->parent = &drv->kobj;
+	return kobject_add(kobj);
+}
+EXPORT_SYMBOL_GPL(driver_create_dir);
+
+/**
+ *	driver_remove_dir - remove a subdirectory for a driver.
+ *	@drv:	driver.
+ *	@attr:	driver attribute descriptor.
+ */
+void driver_remove_dir(struct device_driver *drv, struct kobject *kobj)
+{
+	kobject_del(kobj);
+}
+EXPORT_SYMBOL_GPL(driver_remove_dir);
+
+/**
  *	driver_create_file - create sysfs file for driver.
  *	@drv:	driver.
  *	@attr:	driver attribute descriptor.
diff --git a/drivers/net/iseries_veth.c b/drivers/net/iseries_veth.c
index 97bd9dc..ab46065 100644
--- a/drivers/net/iseries_veth.c
+++ b/drivers/net/iseries_veth.c
@@ -1670,7 +1670,7 @@ static void __exit veth_module_cleanup(void)
 			continue;
 
 		/* Remove the connection from sysfs */
-		kobject_del(&cnx->kobject);
+		driver_remove_dir(&veth_driver.driver, &cnx->kobject);
 		/* Drop the driver's reference to the connection */
 		kobject_put(&cnx->kobject);
 	}
@@ -1705,15 +1705,14 @@ static int __init veth_module_init(void)
 		goto error;
 
 	for (i = 0; i < HVMAXARCHITECTEDLPS; ++i) {
-		struct kobject *kobj;
-
 		if (!veth_cnx[i])
 			continue;
-
-		kobj = &veth_cnx[i]->kobject;
-		kobj->parent = &veth_driver.driver.kobj;
-		/* If the add failes, complain but otherwise continue */
-		if (0 != kobject_add(kobj))
+		/*
+		 * If creating the directory failes, complain
+		 * but otherwise continue
+		 */
+		if (driver_create_dir(&veth_driver.driver,
+				&veth_cnx[i]->kobject))
 			veth_error("cnx %d: Failed adding to sysfs.\n", i);
 	}
 
diff --git a/include/linux/device.h b/include/linux/device.h
index 2e15822..88f2251 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -157,6 +157,9 @@ struct driver_attribute {
 #define DRIVER_ATTR(_name,_mode,_show,_store)	\
 struct driver_attribute driver_attr_##_name = __ATTR(_name,_mode,_show,_store)
 
+extern int __must_check driver_create_dir(struct device_driver *,
+					struct kobject *);
+extern void driver_remove_dir(struct device_driver *, struct kobject *);
 extern int __must_check driver_create_file(struct device_driver *,
 					struct driver_attribute *);
 extern void driver_remove_file(struct device_driver *, struct driver_attribute *);
-- 
1.5.3.7

-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/

Re: [PATCH] Introduce driver_create/remove_dir

[Randy Dunlap]

On Wed, 12 Dec 2007 10:56:33 +1100 Stephen Rothwell wrote:

> 
> Signed-off-by: Stephen Rothwell <sfr@canb.auug.org.au>
> ---
>  drivers/base/driver.c      |   24 ++++++++++++++++++++++++
>  drivers/net/iseries_veth.c |   15 +++++++--------
>  include/linux/device.h     |    3 +++
>  3 files changed, 34 insertions(+), 8 deletions(-)
> 
> 
> diff --git a/drivers/base/driver.c b/drivers/base/driver.c
> index eb11475..6527a91 100644
> --- a/drivers/base/driver.c
> +++ b/drivers/base/driver.c
> @@ -91,6 +91,30 @@ struct device * driver_find_device(struct device_driver *drv,
>  EXPORT_SYMBOL_GPL(driver_find_device);
>  
>  /**
> + *	driver_create_dir - create a subdirectory for a driver.
> + *	@drv:	driver.
> + *	@kobj:	the kobject we are creating the directory for.
> + */
> +int __must_check driver_create_dir(struct device_driver *drv,
> +			struct kobject *kobj)
> +{
> +	kobj->parent = &drv->kobj;
> +	return kobject_add(kobj);
> +}
> +EXPORT_SYMBOL_GPL(driver_create_dir);
> +
> +/**
> + *	driver_remove_dir - remove a subdirectory for a driver.
> + *	@drv:	driver.
> + *	@attr:	driver attribute descriptor.

Second arg below is @kobj.


> + */
> +void driver_remove_dir(struct device_driver *drv, struct kobject *kobj)
> +{
> +	kobject_del(kobj);
> +}
> +EXPORT_SYMBOL_GPL(driver_remove_dir);
> +
> +/**
>   *	driver_create_file - create sysfs file for driver.
>   *	@drv:	driver.
>   *	@attr:	driver attribute descriptor.


---
~Randy

Re: [PATCH] Introduce driver_create/remove_dir

[Stephen Rothwell]

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

On Tue, 11 Dec 2007 16:40:39 -0800 Randy Dunlap <randy.dunlap@ORACLE.COM> wrote:
>
> On Wed, 12 Dec 2007 10:56:33 +1100 Stephen Rothwell wrote:
> 
> > +/**
> > + *	driver_remove_dir - remove a subdirectory for a driver.
> > + *	@drv:	driver.
> > + *	@attr:	driver attribute descriptor.
> 
> Second arg below is @kobj.

Thanks, will fix (cut and paste error).

-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: drivers/net/iseries_veth.c dubious sysfs usage

[Greg KH]

On Thu, Dec 06, 2007 at 02:48:18PM +1100, Michael Ellerman wrote:
> igoeast:~# cd /sys/class/net/eth1/
> igoeast:/sys/class/net/eth1# ls -la
> total 0
> drwxr-xr-x  4 root root    0 Dec  6 10:22 .
> drwxr-xr-x  6 root root    0 Dec  6 10:21 ..
> -r--r--r--  1 root root 4096 Dec  6 10:30 addr_len
> -r--r--r--  1 root root 4096 Dec  6 10:30 address
> -r--r--r--  1 root root 4096 Dec  6 10:30 broadcast
> -r--r--r--  1 root root 4096 Dec  6 10:30 carrier
> lrwxrwxrwx  1 root root    0 Dec  6 10:22 device -> ../../../devices/vio/3
> -r--r--r--  1 root root 4096 Dec  6 10:30 dormant
> -r--r--r--  1 root root 4096 Dec  6 10:30 features
> -rw-r--r--  1 root root 4096 Dec  6 10:30 flags
> -r--r--r--  1 root root 4096 Dec  6 10:30 ifindex
> -r--r--r--  1 root root 4096 Dec  6 10:30 iflink
> -r--r--r--  1 root root 4096 Dec  6 10:30 link_mode
> -rw-r--r--  1 root root 4096 Dec  6 10:30 mtu
> -r--r--r--  1 root root 4096 Dec  6 10:30 operstate
> drwxr-xr-x  2 root root    0 Dec  6 10:30 statistics
> lrwxrwxrwx  1 root root    0 Dec  6 10:30 subsystem -> ../../../class/net
> -rw-r--r--  1 root root 4096 Dec  6 10:30 tx_queue_len
> -r--r--r--  1 root root 4096 Dec  6 10:30 type
> -rw-r--r--  1 root root 4096 Dec  6 10:30 uevent
> drwxr-xr-x  2 root root    0 Dec  6 10:30 veth_port
> 
> Each net device has a port structure associated with it, the fields
> should be fairly self explanatory, they're all read only I think.
> 
> igoeast:/sys/class/net/eth1# find veth_port/
> veth_port/
> veth_port/mac_addr
> veth_port/lpar_map
> veth_port/stopped_map
> veth_port/promiscuous
> veth_port/num_mcast

That's fine, I'll let you fight with the network people over that :)

> igoeast:/sys/class/net/eth1# cd device/driver
> 
> igoeast:/sys/class/net/eth1/device/driver# ls -l
> total 0
> lrwxrwxrwx  1 root root    0 Dec  6 10:21 2 -> ../../../../devices/vio/2
> lrwxrwxrwx  1 root root    0 Dec  6 10:21 3 -> ../../../../devices/vio/3
> --w-------  1 root root 4096 Dec  6 10:21 bind
> drwxr-xr-x  2 root root    0 Dec  6 10:21 cnx00
> drwxr-xr-x  2 root root    0 Dec  6 10:21 cnx02
> drwxr-xr-x  2 root root    0 Dec  6 10:21 cnx03
> drwxr-xr-x  2 root root    0 Dec  6 10:21 cnx04
> lrwxrwxrwx  1 root root    0 Dec  6 10:21 module -> ../../../../module/iseries_veth
> --w-------  1 root root 4096 Dec  6 10:21 uevent
> --w-------  1 root root 4096 Dec  6 10:21 unbind
> 
> The driver has a connection to all the other lpars, this is entirely
> independent of the net devices.
> 
> igoeast:/sys/class/net/eth1/device/driver# find cnx00/
> cnx00/
> cnx00/outstanding_tx
> cnx00/remote_lp
> cnx00/num_events
> cnx00/reset_timeout
> cnx00/last_contact
> cnx00/state
> cnx00/src_inst
> cnx00/dst_inst
> cnx00/num_pending_acks
> cnx00/num_ack_events
> cnx00/ack_timeout

Hm, ok, it's odd as you are the only driver in the whole tree doing
something like this, but it seems semi-resonable, so I can't complain :)

I'll fix the core up to allow you to do this, thanks for the
explanation.

greg k-h

Re: [PATCH] Introduce driver_create/remove_dir

[Greg KH]

On Wed, Dec 12, 2007 at 10:56:33AM +1100, Stephen Rothwell wrote:
> 
> Signed-off-by: Stephen Rothwell <sfr@canb.auug.org.au>
> ---
>  drivers/base/driver.c      |   24 ++++++++++++++++++++++++
>  drivers/net/iseries_veth.c |   15 +++++++--------
>  include/linux/device.h     |    3 +++
>  3 files changed, 34 insertions(+), 8 deletions(-)
> 
> Greg, does this look like a reasonable solution to iseries_veth accessing
> the "private" kobject in struct device_driver?  This version is against
> maimline, but the stuff you have in mm would just need to update
> driver_create_dir ...

Hm, we just want to be able to get to the kobject somehow here.  Not
create a new api that doesn't match up.  I'll think about it and figure
something that matches the other portions of the api.

> Also something along the lines of device_add_dir() might be good if you
> want to hide the kobject in struct device as well.

As devices _should_ always be dynamic, hopefully I'll not have to do
that.  But knowing some of the platform devices, I'm afraid I'll have to
do that split :)

thanks,

greg k-h

Re: drivers/net/iseries_veth.c dubious sysfs usage

[Stephen Rothwell]

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

Hi Greg,

On Wed, 12 Dec 2007 23:08:29 -0800 Greg KH <greg@kroah.com> wrote:
>
> Hm, ok, it's odd as you are the only driver in the whole tree doing
> something like this, but it seems semi-resonable, so I can't complain :)
> 
> I'll fix the core up to allow you to do this, thanks for the
> explanation.

So if this "seems semi-reasonable", why was the result
gregkh-driver-driver-add-driver_add_kobj-for-looney-iseries_veth-driver
containing "Hopefully no one uses this function in the future and the
iseries_veth driver authors come to their senses so I can remove this
hack..." as part of its comment.  If you expect respect, you need to
treat others the same way ...

If what the driver writers are doing is "looney" (in your opinion), then
please describe a better way of doing what they are trying to do.
Sometimes, if people have to abuse the infrastructure, it is possible that
the infrastructure is lacking?

-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: drivers/net/iseries_veth.c dubious sysfs usage

[Greg KH]

On Mon, Dec 24, 2007 at 01:52:08PM +1100, Stephen Rothwell wrote:
> Hi Greg,
> 
> On Wed, 12 Dec 2007 23:08:29 -0800 Greg KH <greg@kroah.com> wrote:
> >
> > Hm, ok, it's odd as you are the only driver in the whole tree doing
> > something like this, but it seems semi-resonable, so I can't complain :)
> > 
> > I'll fix the core up to allow you to do this, thanks for the
> > explanation.
> 
> So if this "seems semi-reasonable", why was the result
> gregkh-driver-driver-add-driver_add_kobj-for-looney-iseries_veth-driver
> containing "Hopefully no one uses this function in the future and the
> iseries_veth driver authors come to their senses so I can remove this
> hack..." as part of its comment.  If you expect respect, you need to
> treat others the same way ...

Well, sarcasm doesn't come accross very easily in changelog comments it
seems :)

> If what the driver writers are doing is "looney" (in your opinion), then
> please describe a better way of doing what they are trying to do.
> Sometimes, if people have to abuse the infrastructure, it is possible that
> the infrastructure is lacking?

In thinking about this some more, no, I think you all are abusing the
infrastructure here.  This is the ONLY driver in the entire kernel tree
that thinks it is acceptable to hang kobjects off of the driver
structure.  For some reason, no one else does this either because they
never would think of doing such a thing, or that they are not as unique
as this driver.

So I take back the "semi-resonable" statement above.  Please prove to me
that:
	- it is ok to hang kobjects off of a driver when:
		- no userspace tool will ever be notified that they have
		  been created
		- no known userspace library knows how to find such
		  attributes (libsysfs can't do that last I looked, and
		  it's no longer maintained.)
		- there is no documentation in the Documentation/ABI/
		  explaining this usage.
	- this can not be just a easily expressed in debugfs, or some
	  other representation (netlink for configuration, configfs,
	  some other location in sysfs, a driver-specific filesystem,
	  etc.)
	- that there is a tool out there using this current interface.

I don't like this usage of sysfs as it is very abnormal, and we are
trying very hard to fix up the rough edges here, to:
	- make it easier to program to and not get things incorrect
	  within the kernel
	- present a unified, semi-sane interface that is documented well
	  to userspace so users don't get even madder then they
	  currently are.


thanks,

greg k-h