netdev_ops?

netdev_ops?

[Ben Greear]

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 <greearb@candelatech.com>       <Ben_Greear AT excite.com>
President of Candela Technologies Inc      http://www.candelatech.com
ScryMUD:  http://scry.wanfear.com     http://scry.wanfear.com/~greear

Re: netdev_ops?

[David S. Miller]

On Tue, 22 Jul 2003 22:06:04 -0700
Ben Greear <greearb@candelatech.com> wrote:

> 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.

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.

Re: netdev_ops?

[Ben Greear]

David S. Miller wrote:
> On Tue, 22 Jul 2003 22:06:04 -0700
> Ben Greear <greearb@candelatech.com> wrote:
> 
> 
>>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.
> 
> 
> 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 <greearb@candelatech.com>       <Ben_Greear AT excite.com>
President of Candela Technologies Inc      http://www.candelatech.com
ScryMUD:  http://scry.wanfear.com     http://scry.wanfear.com/~greear

Re: netdev_ops?

[David S. Miller]

On Tue, 22 Jul 2003 22:30:10 -0700
Ben Greear <greearb@candelatech.com> wrote:

> Any interest in a lower-risk netdev-ops lite?

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.

Re: netdev_ops?

[Ben Greear]

David S. Miller wrote:
> On Tue, 22 Jul 2003 22:30:10 -0700
> Ben Greear <greearb@candelatech.com> wrote:
> 
> 
>>Any interest in a lower-risk netdev-ops lite?
> 
> 
> 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 <greearb@candelatech.com>       <Ben_Greear AT excite.com>
President of Candela Technologies Inc      http://www.candelatech.com
ScryMUD:  http://scry.wanfear.com     http://scry.wanfear.com/~greear

Re: netdev_ops?

[David S. Miller]

On Tue, 22 Jul 2003 23:24:00 -0700
Ben Greear <greearb@candelatech.com> wrote:

> My goal is a place to add new generic net-device ioctls without having
> to worry about testing the ioctls on various platforms

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've said
> before you don't like when I try to add new ioctls because I break SPARC and
> who knows what else...)

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.

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

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. :-)

Re: netdev_ops?

[Ben Greear]

David S. Miller wrote:

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

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.

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

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 <greearb@candelatech.com>       <Ben_Greear AT excite.com>
President of Candela Technologies Inc      http://www.candelatech.com
ScryMUD:  http://scry.wanfear.com     http://scry.wanfear.com/~greear

Re: netdev_ops?

[David S. Miller]

On Tue, 22 Jul 2003 23:36:25 -0700
Ben Greear <greearb@candelatech.com> wrote:

> I am not writing drivers, I'm trying to write code that works with
> everything that looks remotely like an ethernet device.

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.

> I can make this one change and work with ALL drivers, and not have
> to corrupt every friggin driver under the sun.

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.

> 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.

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

> 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.

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.

Re: netdev_ops?

[Ben Greear]

David S. Miller wrote:
> On Tue, 22 Jul 2003 23:36:25 -0700
> Ben Greear <greearb@candelatech.com> wrote:
> 
> 
>>I am not writing drivers, I'm trying to write code that works with
>>everything that looks remotely like an ethernet device.
> 
> 
> 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.

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.

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

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.


> 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.

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 <greearb@candelatech.com>       <Ben_Greear AT excite.com>
President of Candela Technologies Inc      http://www.candelatech.com
ScryMUD:  http://scry.wanfear.com     http://scry.wanfear.com/~greear

Re: netdev_ops?

[David S. Miller]

On Wed, 23 Jul 2003 00:28:27 -0700
Ben Greear <greearb@candelatech.com> wrote:

> In my case, if the net_device can return net_device_stats, then I want it to work.

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.

> 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.

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.

> 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?

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.

Re: netdev_ops?

[Ben Greear]

David S. Miller wrote:
> On Wed, 23 Jul 2003 00:28:27 -0700
> Ben Greear <greearb@candelatech.com> wrote:
> 
> 
>>In my case, if the net_device can return net_device_stats, then I want it to work.
> 
> 
> This is not for you to decide.  That is the driver author's
> choice.

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.

> 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.

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.

> 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.

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?

> 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 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.

> 
> 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.

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 <greearb@candelatech.com>       <Ben_Greear AT excite.com>
President of Candela Technologies Inc      http://www.candelatech.com
ScryMUD:  http://scry.wanfear.com     http://scry.wanfear.com/~greear

Re: netdev_ops?

[David S. Miller]

On Wed, 23 Jul 2003 01:08:48 -0700
Ben Greear <greearb@candelatech.com> wrote:

> Is it their choice to participate in the /proc/net/dev output?

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

> > 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.
> 
> Yep, I looked through that..and through libnetlink, and the complexity
> is not worth it.

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.

Re: netdev_ops?

[Jeff Garzik]

On Tue, Jul 22, 2003 at 10:07:45PM -0700, David S. Miller wrote:
> On Tue, 22 Jul 2003 22:06:04 -0700
> Ben Greear <greearb@candelatech.com> wrote:
> 
> > 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.
> 
> 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.

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