netdev_ops?

Any progress towards getting the netdev_ops into 2.4?

I have several patches that would benefit (in that no new ioctls would be needed) if this goes in.

Thanks,
Ben

Ben Greear <[email protected]>       <Ben_Greear AT excite.com>
President of Candela Technologies Inc      http://www.candelatech.com
ScryMUD:  http://scry.wanfear.com     http://scry.wanfear.com/~greear

If anything, it's going to go into 2.6.x first, and then backported to 2.4.x after it's had a few months of testing and tweaking.

Any interest in a lower-risk netdev-ops lite? I have a relatively ugly little patch that will allow me to handle certain ETHTOOL ioctls in dev.c's ioctl handling code. Ugly, but short, and very little chance of breaking anything else....

Ben Greear <[email protected]>       <Ben_Greear AT excite.com>
President of Candela Technologies Inc      http://www.candelatech.com
ScryMUD:  http://scry.wanfear.com     http://scry.wanfear.com/~greear

Not really.

Nobody is really helped with this, not even you. It makes almost no sense to make a driver only work with a 2.4.x kernel that has this netdev-ops-lite thing patched into it, because then it doesn't work on all the bazillion other 2.4.x kernels out there.

My goal is a place to add new generic net-device ioctls without having to worry about testing the ioctls on various platforms (You've said before you don't like when I try to add new ioctls because I break SPARC and who knows what else...)

My patch looks like this, and it has zero impact on drivers. It's primary benefit is to get around adding more ioctls:

### Line 2304 or so, default case of the dev_ifsioc switch... ###

default:
+ /* Handle some generic ethtool commands here */
+ if (cmd == SIOCETHTOOL) {
+ u32 cmd = 0;
+ if (copy_from_user(&cmd, ifr->ifr_data, sizeof(cmd))) {
+ return -EFAULT;
+ }
+
+ if (cmd == ETHTOOL_GNDSTATS) {
+
+ struct ethtool_ndstats* nds = (struct ethtool_ndstats*)(ifr->ifr_data);
+
+ /* Get net-device stats struct, will save it in the space

+                                         * passed in to us in ifr->ifr_data.  Would like to use
+                                         * ethtool, but it seems to require specific driver support,
+                                         * when this is a general purpose netdevice request...
+                                         */

+ struct net_device_stats *stats = dev->get_stats(dev);
+ if (stats) {
+ if (copy_to_user(nds->data, stats, sizeof(*stats))) {
+ return -EFAULT;
+ }
+ }
+ else {
+ return -EOPNOTSUPP;
+ }
+ return 0;
+ }
+ }
+
+
+
### Fall through to the rest of the ioctl (ethtool included) handling...

Ben Greear <[email protected]>       <Ben_Greear AT excite.com>
President of Candela Technologies Inc      http://www.candelatech.com
ScryMUD:  http://scry.wanfear.com     http://scry.wanfear.com/~greear

Your patch adds ethtool stuff, which works perfectly fine on all platforms, even sparc64 when executing 32-bit binaries.

Just add it to your drivers if you want them supported in 2.4.x

You're not adding new ioctls here, you adding a default implementation of an existing ioctl, and this kind of code is of no issue wrt SPARC/PPC/MIPS/etc. ioctl translation for 32-bit applications running on a 64-bit kernel.

You gain nothing from this patch, just put it into your drivers.

Your patch is even more useless than I thought it was going to be. :-)

I am not writing drivers, I'm trying to write code that works with everything that looks remotely like an ethernet device. Thus, I can make this one change and work with ALL drivers, and not have to corrupt every friggin driver under the sun.

And yes, I realize this patch is not adding a new ioctl..that is the whole point.

I really hope you mis-understood it :)

Note it allows me to get a binary representation of the net_device_stats w/out having to parse /proc/net/dev or figure out the vast complexity of libnetlink.

I have plenty of other things that are currently new ioctls that could be handled the same, and thus I could continue to avoid issues with other platforms.

Ben

Ben Greear <[email protected]>       <Ben_Greear AT excite.com>
President of Candela Technologies Inc      http://www.candelatech.com
ScryMUD:  http://scry.wanfear.com     http://scry.wanfear.com/~greear

Making ethtool interfaces available on every net device is not right, what about the ISDN folks? What if they specifically want ethtool ioctls to fail for their devices? How can one accomplish that after your changes?

Answer: You can't.

This is undesirable. Not all network drivers should implement ethtool. A certain family of network devices may not want them, and we must provide for this.

I don't like your change just as much as I did previously.

Whatever tools you write which depend upon this will not work on any existing 2.4.x kernel, therefore making their utility basically NIL.

What is this "other platform" issue?

If you add anything new, along the lines of SIOCDEVETHTOOl, it's going to have to go through an entire full review process and in that review process any necessary 32-bit ioctl translation code would get added.

In my case, if the net_device can return net_device_stats, then I want it to work. If it can't, -ENOTSUPP is returned. I cannot fathom a reason for this in itself to harm anyone. As you noticed below, existing code would never try this ioctl, and new code can likewise ignore it if it cannot handle the consequences.

That can be said for every new feature or ioctl. Of course it won't work with older stuff...but it will work with newer stuff, and I'm smart enough to be able to fall back to the less efficient parsing of /proc/net/dev if the ioctls are not supported. Anyone else that cares can write programs just as clever.

Yes, and since I am ignorant of that stuff, and have no hardware to test with, then I want to avoid it. I can't imagine I'm the
only one. Overloading the ethtool ioctl is one way to avoid that pain..adding a new, similar ioctl would also work, but seems like duplicated effort to me.

Since it seems very unlikly that this sort of patch will be accepted in the near future, how _DO_ you want to see new features added that require configuration (and reading) from user space? IOCTLs are easy to add on x86 at least, but maybe you'd prefer some text based proc interface instead?

Ben

Ben Greear <[email protected]>       <Ben_Greear AT excite.com>
President of Candela Technologies Inc      http://www.candelatech.com
ScryMUD:  http://scry.wanfear.com     http://scry.wanfear.com/~greear

This is not for you to decide. That is the driver author's choice.

Also, succeeding for _ANY_ ethtool command is going to give a tool the impression that other basic ethtool commands should work too. Your patch makes many devices give very inconsistent behavior.

The correct "fix" on the 2.4.x side is to add the appropriate ethtool support to appropriate drivers that lack it and need this interface. It is not your hack and it is not adding a new ioctl.

You still haven't said why parsing /proc/dev is so bad, and you even admit that your tool has to fall back to this ANYWAYS.

I just showed you above how to fix this particular problem. Add ethtool support to the ethernet device in question, and submit this change to jgarzik. It isn't very hard work and things other than your particular need stand to gain from it.

My final note: You don't even have the problem you claim to have.
Use your brain and 'grep' a little bit, ok? :-)

egrep get_stats net/core/rtnetlink.c

There it is, exactly what you need and supported on every single kernel out there.

Is it their choice to participate in the /proc/net/dev output? I want the same info, only in binary format. However, this is a side issue: I am really looking for a flexible way to add new features through some ioctl interface, and these features will act primarily on struct net_device, ie at an abstract layer and independent of the underlying driver.

Only stupid tools...every use of ETHTOOL has to be checked because every driver implements different portions, or none at all. Inconsistent is when ethtool eth0 works when eth0 happens to be an 8139too driver and fails when eth0 is a tulip driver.

So, you'd accept an identical 30 line patch to *every* network device driver? And what about the ones that support no ethtool at all...would you accept the patch that only supported getting the binary stats?

I notice slowness when trying to probe 250 interfaces (vlans) very often. And no wonder, considering that to get up to date stats I need to read all of /proc/net/dev, search for the right line, and then parse it. Of course my tool will fall back: I want it to work everywhere...but that doesn't mean it shouldn't run better on newer kernels.

Yep, I looked through that..and through libnetlink, and the complexity is not worth it. Besides, I have multiple other things that are common to all ethernet and ethernet-like devices, so I need to either add IOCTLs, proc interfaces, or hack ethtool. I can continue to ship my own kernel and/or provide patches, but I would prefer to get the support into the mainline kernel. If you have ideas for how you'd like to see this done, plz tell. If you will never accept such a thing, then I'll ask again in 6 months and hope someone else answers.

Ben Greear <[email protected]>       <Ben_Greear AT excite.com>
President of Candela Technologies Inc      http://www.candelatech.com
ScryMUD:  http://scry.wanfear.com     http://scry.wanfear.com/~greear

Precisely yes, this is why they have the option of not providing the ->get_stats() method by leaving it set to NULL.

Nice cop out.

Netlink is the standard method to obtain information about network device, address, and route information. It is even defined by an RFC.

We're not going to add a hack to the kernel just because you think netlink is too complex. If it's too complex, you get to live with the text based output.

I'll tell you this, the netlink version will work on more systems, even ones that don't have /proc mounted.

Your patch duplicates existing functionality (getting network statistics in binary form), so just based upon that I cannot allow your patch.

Agreed.

FWIW Matthew and I (and several others) are at OLS and basically out of commission for a week, so pretty-please don't make any major decisions this week ;-)

Jeff