[PATCH] ethtool_ops rev 4

[PATCH] ethtool_ops rev 4

[Matthew Wilcox]

At 55k, I doubt you want to see it posted to the list; patch is available from

http://ftp.linux.org.uk/pub/linux/willy/patches/ethtool4.diff

and here's the diffstat

 drivers/net/8139too.c     |  330 ++++++++--------------
 drivers/net/tg3.c         |  584 ++++++++++++++++------------------------
 include/linux/ethtool.h   |  100 ++++++
 include/linux/netdevice.h |    5 
 net/core/Makefile         |    4 
 net/core/dev.c            |   16 -
 net/core/ethtool.c        |  671 ++++++++++++++++++++++++++++++++++++++++++++++
 7 files changed, 1154 insertions(+), 556 deletions(-)

Patch has received light testing on an rtl8139c card:

Settings for eth0:
        Supported ports: [ TP MII ]
        Supported link modes:   10baseT/Half 10baseT/Full 
                                100baseT/Half 100baseT/Full 
        Supports auto-negotiation: Yes
        Advertised link modes:  10baseT/Half 10baseT/Full 
                                100baseT/Half 100baseT/Full 
        Advertised auto-negotiation: Yes
        Speed: 100Mb/s
        Duplex: Full
        Port: MII
        PHYAD: 32
        Transceiver: internal
        Auto-negotiation: on
        Supports Wake-on: pumbg
        Wake-on: d
        Current message level: 0xffffffff (-1)
        Link detected: yes

but obviously it doesn't support all the ethtool options that some
cards do.

-- 
"It's not Hollywood.  War is real, war is primarily not about defeat or
victory, it is about death.  I've seen thousands and thousands of dead bodies.
Do you think I want to have an academic debate on this subject?" -- Robert Fisk

Re: [PATCH] ethtool_ops rev 4

[Jeff Garzik]

On Fri, Aug 01, 2003 at 04:02:32PM +0100, Matthew Wilcox wrote:
> and here's the diffstat
> 
>  drivers/net/8139too.c     |  330 ++++++++--------------
>  drivers/net/tg3.c         |  584 ++++++++++++++++------------------------
>  include/linux/ethtool.h   |  100 ++++++
>  include/linux/netdevice.h |    5 
>  net/core/Makefile         |    4 
>  net/core/dev.c            |   16 -
>  net/core/ethtool.c        |  671 ++++++++++++++++++++++++++++++++++++++++++++++
>  7 files changed, 1154 insertions(+), 556 deletions(-)

Comments:

* need SET_ETHTOOL_OPS macro or HAVE_ETHTOOL_OPS test macro or similar

* I still do not see the need to change a simple storage of a constant
  (into ethtool_gdrvinfo) into _four_ separate function call hooks (reg
  dump len, eeprom dump len, nic-specific stats len, self-test len).
  Internal kernel code that needs this information is always a slow path
  anyway, so just call the ->get_drvinfo hook internally.

* I prefer not to add '#include <linux/types.h>' to ethtool.h

Other than those, looks real good.

	Jeff

Re: [PATCH] ethtool_ops rev 4

[Matthew Wilcox]

On Fri, Aug 01, 2003 at 11:40:21AM -0400, Jeff Garzik wrote:
> Comments:
> 
> * need SET_ETHTOOL_OPS macro or HAVE_ETHTOOL_OPS test macro or similar

DaveM disagreed with that...

> * I still do not see the need to change a simple storage of a constant
>   (into ethtool_gdrvinfo) into _four_ separate function call hooks (reg
>   dump len, eeprom dump len, nic-specific stats len, self-test len).
>   Internal kernel code that needs this information is always a slow path
>   anyway, so just call the ->get_drvinfo hook internally.

slow path, sure, but increased stack usage.  it's a tradeoff, and this way
feels more clean to me.

> * I prefer not to add '#include <linux/types.h>' to ethtool.h

That means that any code which includes ethtool.h has to include types.h
first (either implicitly or explicitly).  The rule so far has been that
header files should call out their dependencies explictly with an include
of the appropriate file.  So why *don't* you want it?

-- 
"It's not Hollywood.  War is real, war is primarily not about defeat or
victory, it is about death.  I've seen thousands and thousands of dead bodies.
Do you think I want to have an academic debate on this subject?" -- Robert Fisk

Re: [PATCH] ethtool_ops rev 4

[Jeff Garzik]

On Fri, Aug 01, 2003 at 04:46:56PM +0100, Matthew Wilcox wrote:
> On Fri, Aug 01, 2003 at 11:40:21AM -0400, Jeff Garzik wrote:
> > Comments:
> > 
> > * need SET_ETHTOOL_OPS macro or HAVE_ETHTOOL_OPS test macro or similar
> 
> DaveM disagreed with that...

It's standard netdevice.h practice, and, he didn't disagree w/ my
rebuttal.

It is needed.


> > * I still do not see the need to change a simple storage of a constant
> >   (into ethtool_gdrvinfo) into _four_ separate function call hooks (reg
> >   dump len, eeprom dump len, nic-specific stats len, self-test len).
> >   Internal kernel code that needs this information is always a slow path
> >   anyway, so just call the ->get_drvinfo hook internally.
> 
> slow path, sure, but increased stack usage.  it's a tradeoff, and this way
> feels more clean to me.

Additing a function hook each time you want to retrieve a new integer
value?  That's feels overly excessive to me.


> > * I prefer not to add '#include <linux/types.h>' to ethtool.h
> 
> That means that any code which includes ethtool.h has to include types.h
> first (either implicitly or explicitly).  The rule so far has been that
> header files should call out their dependencies explictly with an include
> of the appropriate file.  So why *don't* you want it?

Because I copy it to userspace :)

	Jeff

Re: [PATCH] ethtool_ops rev 4

[David S. Miller]

On Fri, 1 Aug 2003 12:25:36 -0400
Jeff Garzik <jgarzik@pobox.com> wrote:

> On Fri, Aug 01, 2003 at 04:46:56PM +0100, Matthew Wilcox wrote:
> > On Fri, Aug 01, 2003 at 11:40:21AM -0400, Jeff Garzik wrote:
> > > Comments:
> > > 
> > > * need SET_ETHTOOL_OPS macro or HAVE_ETHTOOL_OPS test macro or similar
> > 
> > DaveM disagreed with that...
> 
> It's standard netdevice.h practice, and, he didn't disagree w/ my
> rebuttal.
> 
> It is needed.

Absolutely not, it makes no sense whatsoever to have this.

Jeff, stop and think.

The whole _POINT_ of these ops are to avoid duplicated code.
If someone is absolutely adament about supporting kernels
without ops support they should not support it at all.

The point is to avoid code duplication, but what you suggest can only
be used to keep the duplicated code around "just in case".  This makes
exactly no sense at all, it severs only to defeat the whole purpose
of the change in the first place.

I totally am against making an ifdef test available for this, it can
only result in illogical things being done by driver maintainers.

Re: [PATCH] ethtool_ops rev 4

[Jeff Garzik]

David S. Miller wrote:
> The whole _POINT_ of these ops are to avoid duplicated code.
> If someone is absolutely adament about supporting kernels
> without ops support they should not support it at all.
> 
> The point is to avoid code duplication, but what you suggest can only
> be used to keep the duplicated code around "just in case".  This makes
> exactly no sense at all, it severs only to defeat the whole purpose
> of the change in the first place.
> 
> I totally am against making an ifdef test available for this, it can
> only result in illogical things being done by driver maintainers.


Strangely enough, creating a SET_ETHTOOL_OPS() macro (or 
netif_ethtool_ops or pick your name) reduces ifdefs.

I feel that I've helped shepherd the net driver and PCI APIs to maintain 
something fairly interesting:  a driver API that [for the most part...] 
allows one to write a driver completely without compatibility ifdefs, 
and ancient-kernel junk.  When married with a compat glue lib outside 
the tree, the same ifdef-free driver works on older kernels.

It's an explicit goal to avoid changing the driver API in such a way 
that there is a remotely sane path to supporting older kernels.  One of 
the few things that is not easily work-around-able is new additions to 
existing structures (which wouldn't exist in older kernels).  That's 
what SET_ETHTOOL_OPS would wrap, while also providing a trigger for 
generic compat glue.

This trigger is what _reduces_ code duplication.  Given such a trigger, 
a generic library can implement compat code on older kernels.  The 
drivers remain ifdef-free and compat-junk-free.  This is method used by 
the kcompat toolkit (http://sf.net/projects/gkernel/).

This (IMO) feature continually saves me real time, again and again, when 
merging a new net driver into the kernel.  It saves me time debugging a 
driver in both 2.4 and 2.6.  The time savings is in the minimization (is 
that a word?) of changes across kernel versions, and this particular 
ethtool_ops change will be a thorn in particular.  This ethtool_ops 
change _is_ trivially made backward-compatible, with a simple macro. 
Look at the future, where vendors are submitting 2.6-ready net drivers, 
because we made it easier for them to support their existing platform.

Over and above the time savings, vendors _will_ start submitting drivers 
that actually look like Linux drivers.  This has already started 
happening :)  Just today I received a Via-rhine gbit driver (GPL'd) at 
Red Hat, which I am preparing to merge into the kernel.  After removing 
the awful Hungarian notation and silly procfs apis, the driver's 
actually pretty close to a mergeable driver.  It uses the kcompat stuff, 
and as such isn't full of ifdefs and typical vendor cpp maze.

So, for the benefits of saving me real wall-clock hours, and pushing the 
vendors to create ready-for-the-kernel drivers more often, the cost is a 
simple one-line wrapper macro that in-kernel drivers would rarely use.

In the long run, I'm trying to use and abuse Intel as an example for 
other vendors to follow (using netdev@, splitting up patches, etc.), and 
push the driver maintenance load onto the vendors (where they're 
willing, etc., like Intel).  If vendors are willing to respond to 
feedback and follow standard linux-kernel email development, I'm more 
than happy for them to become a learned funnel of patches to netdev for 
review :)  This kcompat strategy -- back-compat without ifdefs -- goes a 
long way towards that, and SET_ETHTOOL_OPS is a big piece of that puzzle 
right now.

	Jeff

Re: [PATCH] ethtool_ops rev 4

[David S. Miller]

On Fri, 01 Aug 2003 18:26:37 -0400
Jeff Garzik <jgarzik@pobox.com> wrote:

> Strangely enough, creating a SET_ETHTOOL_OPS() macro (or 
> netif_ethtool_ops or pick your name) reduces ifdefs.

And then we'll have all of these functions present in
the driver, unused, and we'll get tons of warning from
gcc.

The duplication of code is still there, and this is the
main point.

> I feel that I've helped shepherd the net driver and PCI APIs to maintain 
> something fairly interesting:

It's not interesting in this case.

> It's an explicit goal to avoid changing the driver API in such a way 
> that there is a remotely sane path to supporting older kernels.

This enhancement we're talking about basically has no value unless
you accept an appearance of breakage in this particular area.

You can't get rid of the duplicated code without accepting that you
will have seperate 2.6.x and 2.4.x strains of your driver.

If you aren't willing to accept seperate strains of your driver, you
simply don't use netdev_ops.

It is the end of the conversation.

> the few things that is not easily work-around-able is new additions to 
> existing structures (which wouldn't exist in older kernels).  That's 
> what SET_ETHTOOL_OPS would wrap, while also providing a trigger for 
> generic compat glue.

What gets rid of the static functions that do the work when
SET_ETHTOOL_OPS() is a nop?

I do not accept a scheme where the functions stay there in the driver
anyways.  All you seem to be talking about is a compat library which
provides netdev_ops in library form or something silly like that.

> This (IMO) feature continually saves me real time

I don't argue that, just don't use netdev_ops in drivers you
wish to keep doing this with :-)

Look at drivers/net/acenic.c, that's similar to what your drivers will
begin to look like if you don't start accepting a disconnect in
certain areas.

Re: [PATCH] ethtool_ops rev 4

[Jeff Garzik]

David S. Miller wrote:
> On Fri, 01 Aug 2003 18:26:37 -0400
> Jeff Garzik <jgarzik@pobox.com> wrote:
> 
> 
>>Strangely enough, creating a SET_ETHTOOL_OPS() macro (or 
>>netif_ethtool_ops or pick your name) reduces ifdefs.
> 
> 
> And then we'll have all of these functions present in
> the driver, unused, and we'll get tons of warning from
> gcc.
> 
> The duplication of code is still there, and this is the
> main point.

Not correct:  there is nothing unused, there are no warnings, in either 
the in-kernel case or the older-kernel case.  Look at kcompat.  That is 
code that is working, and producing the 2.4/2.6-ready vendor drivers I 
spoke of.

I'm apparently not communicating the design that exists in kcompat, if 
you think this.  The design is:

	code for 2.6, and it magically works in 2.4

It's a back-compat system that is so good you don't even know it's 
there.  It's completely invisible to the mainline kernel -- as it should 
be -- presuming that one pays attention to subtle API change effects.

Do you see yet how there is no code duplication, no ifdefs, no warnings 
about unused functions?  That is the key point of the whole design, and 
key to the thread of discussion here.


> You can't get rid of the duplicated code without accepting that you
> will have seperate 2.6.x and 2.4.x strains of your driver.
> 
> If you aren't willing to accept seperate strains of your driver, you
> simply don't use netdev_ops.

Look at kcompat.  That is real, working code that demonstrates the approach.


>>the few things that is not easily work-around-able is new additions to 
>>existing structures (which wouldn't exist in older kernels).  That's 
>>what SET_ETHTOOL_OPS would wrap, while also providing a trigger for 
>>generic compat glue.
> 
> 
> What gets rid of the static functions that do the work when
> SET_ETHTOOL_OPS() is a nop?

SET_ETHTOOL_OPS is never a no-op.

The back-compat form of SET_ETHTOOL_OPS registers the ethtool_ops 
pointer in storage for later use.

A DO_ETHTOOL_OPS macro in the driver's ->do_ioctl -- intentionally not 
included in the kernel -- does the rest, calling kcompat's backported 
net/core/ethtool.c, which in turn calls the ethtool_ops hooks in the 
driver.  Making the kcompat'd net driver ready for 2.6 would then 
involve simply deleting one line.

That's why there is no code duplication or unused driver code.

	Jeff

Re: [PATCH] ethtool_ops rev 4

[David S. Miller]

On Fri, 01 Aug 2003 19:01:21 -0400
Jeff Garzik <jgarzik@pobox.com> wrote:

> A DO_ETHTOOL_OPS macro in the driver's ->do_ioctl -- intentionally not 
> included in the kernel -- does the rest,

I don't understand.

Where does this DO_ETHTOOL_OPS macro come from?  Is it defined
by kcompat?  If so, how will drivers in vanilla 2.4.x trees end
up with the DO_ETHTOOL_OPS define?

Re: [PATCH] ethtool_ops rev 4

[Jeff Garzik]

David S. Miller wrote:
> On Fri, 01 Aug 2003 19:01:21 -0400
> Jeff Garzik <jgarzik@pobox.com> wrote:
> 
> 
>>A DO_ETHTOOL_OPS macro in the driver's ->do_ioctl -- intentionally not 
>>included in the kernel -- does the rest,
> 
> 
> I don't understand.
> 
> Where does this DO_ETHTOOL_OPS macro come from?  Is it defined
> by kcompat?  If so, how will drivers in vanilla 2.4.x trees end
> up with the DO_ETHTOOL_OPS define?



If one wishes to implement kcompat design ("it looks like a 2.6 
driver"), then you have two needs over and above Matthew's current 
ethtool_ops patch:  (1) naked struct deref of netdev->ethtool_ops will 
break immediately on older kernels, and (2) to avoid code duplication, 
you need to insert a call to kcompat's 
do_ethtool_handling_the_old_way... i.e. basically what 
net/core/ethtool.c does now.

Problem #1 is solved with a wrapper macro that disguises the naked 
struct deref to ->ethtool_ops.
Problem #2 is solved by adding a call to DO_ETHTOOL_OPS macro in a 
driver's ->do_ioctl handler.

So, with those two minor changes, a 2.6 driver will work on an older kernel.

To answer your question above, DO_ETHTOOL_OPS can occur one of two ways: 
  (1) my preferred approach, define a no-op DO_ETHTOOL_OPS macro 
in-kernel -- but I did not think this would get accepted, so I chose (2) 
DO_ETHTOOL_OPS exists entirely in kcompat, and people submitting kcompat 
users to mainline would simply delete the one line calling 
DO_ETHTOOL_OPS.  Solution #2 chooses to create a tiny bit more 
merge-to-mainline pain, but also keeps the mainline kernel drivers more 
clean.

	Jeff

Re: [PATCH] ethtool_ops rev 4

[David S. Miller]

On Fri, 01 Aug 2003 19:17:57 -0400
Jeff Garzik <jgarzik@pobox.com> wrote:

> Solution #2 chooses to create a tiny bit more 
> merge-to-mainline pain, but also keeps the mainline kernel drivers more 
> clean.

You don't need DO_ETHTOOL_OPS and thus the merge-to-mainline pain
at all if you do something like:

1) SET_ETHDEV_OPS() also overrides the ->do_ioctl() setting to
   a kcompat_netdev_ioctl() one, but remembers the original pointer
   somewhere.

2) kcompat_netdev_ioctl() does the things DO_ETHTOOL_OPS would
   have done, failing that it calls the saved ->do_ioctl() pointer.

Re: [PATCH] ethtool_ops rev 4

[Jeff Garzik]

David S. Miller wrote:
> On Fri, 01 Aug 2003 19:17:57 -0400
> Jeff Garzik <jgarzik@pobox.com> wrote:
> 
> 
>>Solution #2 chooses to create a tiny bit more 
>>merge-to-mainline pain, but also keeps the mainline kernel drivers more 
>>clean.
> 
> 
> You don't need DO_ETHTOOL_OPS and thus the merge-to-mainline pain
> at all if you do something like:
> 
> 1) SET_ETHDEV_OPS() also overrides the ->do_ioctl() setting to
>    a kcompat_netdev_ioctl() one, but remembers the original pointer
>    somewhere.
> 
> 2) kcompat_netdev_ioctl() does the things DO_ETHTOOL_OPS would
>    have done, failing that it calls the saved ->do_ioctl() pointer.


Certainly.  That's a bit nicer than the back-compat gunk I was plotting, 
even.

Still need the boring and obvious definition of SET_ETHTOOL_OPS in 
mainline, though.

	Jeff

Re: [PATCH] ethtool_ops rev 4

[David S. Miller]

On Fri, 01 Aug 2003 19:42:44 -0400
Jeff Garzik <jgarzik@pobox.com> wrote:

> Still need the boring and obvious definition of SET_ETHTOOL_OPS in 
> mainline, though.

Like I said, I've got no problem with that part.

Re: [PATCH] ethtool_ops rev 4

[Jeff Garzik]

Jeff Garzik wrote:
> It's an explicit goal to avoid changing the driver API in such a way 
> that there is a remotely sane path to supporting older kernels. 


I, of course, meant the exact opposite here :)

We want to provide a sane, ifdef-free path to kcompat, where feasible.

Re: [PATCH] ethtool_ops rev 4

[David S. Miller]

On Fri, 01 Aug 2003 18:35:31 -0400
Jeff Garzik <jgarzik@pobox.com> wrote:

> We want to provide a sane, ifdef-free path to kcompat, where feasible.

I don't believe it's possible with netdev_ops, without
undoing the entire purpose of what netdev_ops is trying
to accomplish (elimination of code duplication).

Show me, in code not words, how you are able to accomplish
this with SET_NETDEV_OPS() or whatever.  I will not read
english text describing the scheme, I will read only code :)

Re: [PATCH] ethtool_ops rev 4

[Jeff Garzik]

David S. Miller wrote:
> On Fri, 01 Aug 2003 18:35:31 -0400
> Jeff Garzik <jgarzik@pobox.com> wrote:
> 
> 
>>We want to provide a sane, ifdef-free path to kcompat, where feasible.
> 
> 
> I don't believe it's possible with netdev_ops, without
> undoing the entire purpose of what netdev_ops is trying
> to accomplish (elimination of code duplication).
> 
> Show me, in code not words, how you are able to accomplish
> this with SET_NETDEV_OPS() or whatever.  I will not read
> english text describing the scheme, I will read only code :)

Read kcompat.  Then:

#define SET_ETHTOOL_OPS kcompat_set_ethtool_ops

#define DO_ETHTOOL_OPS /* duplicate net/core/ethtool.c, basically */

I would define both of these in Matthew's patch, but one only _needs_ to 
define SET_ETHTOOL_OPS, so I pushed for the latter course.

So why is SET_ETHTOOL_OPS needed?  It covered up the one place

It intentionally follows the same design as SET_MODULE_OWNER, and for 
the same purpose:  hiding what would otherwise be a naked struct deref 
to a struct member that does not exist on an older kernel.

Hiding naked struct derefs is also the reason I created 
pci_{get,drv}_drvdata, pci_resource_*, etc.  Back compat is really a big 
syntactic sugar game, and naked struct derefs are really the only big 
thorn in the side.  Everything else can be beaten down with syntactic 
sugar behind the scenes, that never ever gets merged into the upstream 
kernel.

	Jeff

Re: [PATCH] ethtool_ops rev 4

[David S. Miller]

On Fri, 01 Aug 2003 19:09:35 -0400
Jeff Garzik <jgarzik@pobox.com> wrote:

> #define SET_ETHTOOL_OPS kcompat_set_ethtool_ops
> 
> #define DO_ETHTOOL_OPS /* duplicate net/core/ethtool.c, basically */

Where does kcompat_set_ethtool_ops store the pointer if
it does not exist in struct netdevice?

Re: [PATCH] ethtool_ops rev 4

[Jeff Garzik]

David S. Miller wrote:
> On Fri, 01 Aug 2003 19:09:35 -0400
> Jeff Garzik <jgarzik@pobox.com> wrote:
> 
> 
>>#define SET_ETHTOOL_OPS kcompat_set_ethtool_ops
>>
>>#define DO_ETHTOOL_OPS /* duplicate net/core/ethtool.c, basically */
> 
> 
> Where does kcompat_set_ethtool_ops store the pointer if
> it does not exist in struct netdevice?


Inside an area allocated by the kcompat lib.  SET_ETHTOOL_OPS takes 
'struct net_device *' and 'struct ethtool_ops *' arguments, so it simply 
needs to create a lookup list/table somewhere.

You keep asking for code, read kcompat :)  kcompat_set_ethtool_ops has 
exactly the same task as the 2.2.x-era backcompat implementation of 
pci_{get,set}_drvdata.

The perfect back-porting/back-compat system would magically make all 
Linus-tree drivers work without any change on older kernels.  I really 
think the kcompat design is as close as you can come to that.

Here is a linux-kernel-friendly version of the kcompat design:

	"naked struct derefs hurt. otherwise, happy hacking!"

And further, experience shows that the number of naked struct derefs 
that matter is fairly small.

(Another less-common area that hurts besides naked-struct-deref is 
function return type, which is why Linus created irqreturn_t)

	Jeff

Re: [PATCH] ethtool_ops rev 4

[David S. Miller]

On Fri, 01 Aug 2003 19:35:20 -0400
Jeff Garzik <jgarzik@pobox.com> wrote:

> Inside an area allocated by the kcompat lib.  SET_ETHTOOL_OPS takes 
> 'struct net_device *' and 'struct ethtool_ops *' arguments, so it simply 
> needs to create a lookup list/table somewhere.

Ok ok ok, we're converging :-)

Please just comment on my other email suggesting a way
to do away with DO_ETHTOOL_OPS.

I'm OK with a SET_ETHTOOL_OPS() macro.

Re: [PATCH] ethtool_ops rev 4

[Matthew Wilcox]

On Fri, Aug 01, 2003 at 12:25:36PM -0400, Jeff Garzik wrote:
> On Fri, Aug 01, 2003 at 04:46:56PM +0100, Matthew Wilcox wrote:
> > On Fri, Aug 01, 2003 at 11:40:21AM -0400, Jeff Garzik wrote:
> > > * need SET_ETHTOOL_OPS macro or HAVE_ETHTOOL_OPS test macro or similar
> 
> It's standard netdevice.h practice, and, he didn't disagree w/ my
> rebuttal.

OK, now that the two of you thrashed out a design, here's my implementation:

diff -u drivers/net/8139too.c drivers/net/8139too.c
--- drivers/net/8139too.c       31 Jul 2003 17:09:52 -0000
+++ drivers/net/8139too.c       2 Aug 2003 18:38:25 -0000
@@ -973,7 +973,7 @@
        dev->do_ioctl = netdev_ioctl;
        dev->tx_timeout = rtl8139_tx_timeout;
        dev->watchdog_timeo = TX_TIMEOUT;
-       dev->ethtool_ops = &rtl8139_ethtool_ops;
+       set_ethtool_ops(dev, &rtl8139_ethtool_ops);
 
        /* note: the hardware is not capable of sg/csum/highdma, however
         * through the use of skb_copy_and_csum_dev we enable these
diff -u drivers/net/tg3.c drivers/net/tg3.c
--- drivers/net/tg3.c   31 Jul 2003 11:12:10 -0000
+++ drivers/net/tg3.c   2 Aug 2003 18:37:54 -0000
@@ -6724,11 +6724,11 @@
        dev->do_ioctl = tg3_ioctl;
        dev->tx_timeout = tg3_tx_timeout;
        dev->poll = tg3_poll;
-       dev->ethtool_ops = &tg3_ethtool_ops;
        dev->weight = 64;
        dev->watchdog_timeo = TG3_TX_TIMEOUT;
        dev->change_mtu = tg3_change_mtu;
        dev->irq = pdev->irq;
+       set_ethtool_ops(dev, &tg3_ethtool_ops);
 
        err = tg3_get_invariants(tp);
        if (err) {
diff -u include/linux/netdevice.h include/linux/netdevice.h
--- include/linux/netdevice.h   31 Jul 2003 13:06:23 -0000
+++ include/linux/netdevice.h   2 Aug 2003 18:37:16 -0000
@@ -477,6 +477,10 @@
  */
 #define SET_NETDEV_DEV(net, pdev)      ((net)->class_dev.dev = (pdev))
 
+static inline void set_ethtool_ops(struct net_device *dev, struct ethtool_ops *
ops)
+{
+       dev->ethtool_ops = ops;
+}
 
 struct packet_type 
 {

Happy with that?

> > > * I still do not see the need to change a simple storage of a constant
> > >   (into ethtool_gdrvinfo) into _four_ separate function call hooks (reg
> > >   dump len, eeprom dump len, nic-specific stats len, self-test len).
> > >   Internal kernel code that needs this information is always a slow path
> > >   anyway, so just call the ->get_drvinfo hook internally.
> > 
> > slow path, sure, but increased stack usage.  it's a tradeoff, and this way
> > feels more clean to me.
> 
> Additing a function hook each time you want to retrieve a new integer
> value?  That's feels overly excessive to me.

Actually, it's a useful thing to do because it specifies what kind of
answer we want.  For example, up here, you called them all foo_len.
That's not true.  Some of them are a byte-count (== len), but some of
them are a count of N-byte quantities.  That's an unfortunate bit of
design, but at least we can make it obvious to the driver-writer what
we're expecting of them.

> > > * I prefer not to add '#include <linux/types.h>' to ethtool.h
> > 
> > That means that any code which includes ethtool.h has to include types.h
> > first (either implicitly or explicitly).  The rule so far has been that
> > header files should call out their dependencies explictly with an include
> > of the appropriate file.  So why *don't* you want it?
> 
> Because I copy it to userspace :)

linux/types.h exists in userspace ;-)  You even _expect_ userspce to have
already included it -- or where else are the `u32' quantities defined?

-- 
"It's not Hollywood.  War is real, war is primarily not about defeat or
victory, it is about death.  I've seen thousands and thousands of dead bodies.
Do you think I want to have an academic debate on this subject?" -- Robert Fisk

Re: [PATCH] ethtool_ops rev 4

[Jeff Garzik]

Matthew Wilcox wrote:
> On Fri, Aug 01, 2003 at 12:25:36PM -0400, Jeff Garzik wrote:
> 
>>On Fri, Aug 01, 2003 at 04:46:56PM +0100, Matthew Wilcox wrote:
>>
>>>On Fri, Aug 01, 2003 at 11:40:21AM -0400, Jeff Garzik wrote:
>>>
>>>>* need SET_ETHTOOL_OPS macro or HAVE_ETHTOOL_OPS test macro or similar
>>
>>It's standard netdevice.h practice, and, he didn't disagree w/ my
>>rebuttal.
> 
> 
> OK, now that the two of you thrashed out a design, here's my implementation:
> 
> diff -u drivers/net/8139too.c drivers/net/8139too.c
> --- drivers/net/8139too.c       31 Jul 2003 17:09:52 -0000
> +++ drivers/net/8139too.c       2 Aug 2003 18:38:25 -0000
> @@ -973,7 +973,7 @@
>         dev->do_ioctl = netdev_ioctl;
>         dev->tx_timeout = rtl8139_tx_timeout;
>         dev->watchdog_timeo = TX_TIMEOUT;
> -       dev->ethtool_ops = &rtl8139_ethtool_ops;
> +       set_ethtool_ops(dev, &rtl8139_ethtool_ops);
>  
>         /* note: the hardware is not capable of sg/csum/highdma, however
>          * through the use of skb_copy_and_csum_dev we enable these
> diff -u drivers/net/tg3.c drivers/net/tg3.c
> --- drivers/net/tg3.c   31 Jul 2003 11:12:10 -0000
> +++ drivers/net/tg3.c   2 Aug 2003 18:37:54 -0000
> @@ -6724,11 +6724,11 @@
>         dev->do_ioctl = tg3_ioctl;
>         dev->tx_timeout = tg3_tx_timeout;
>         dev->poll = tg3_poll;
> -       dev->ethtool_ops = &tg3_ethtool_ops;
>         dev->weight = 64;
>         dev->watchdog_timeo = TG3_TX_TIMEOUT;
>         dev->change_mtu = tg3_change_mtu;
>         dev->irq = pdev->irq;
> +       set_ethtool_ops(dev, &tg3_ethtool_ops);
>  
>         err = tg3_get_invariants(tp);
>         if (err) {
> diff -u include/linux/netdevice.h include/linux/netdevice.h
> --- include/linux/netdevice.h   31 Jul 2003 13:06:23 -0000
> +++ include/linux/netdevice.h   2 Aug 2003 18:37:16 -0000
> @@ -477,6 +477,10 @@
>   */
>  #define SET_NETDEV_DEV(net, pdev)      ((net)->class_dev.dev = (pdev))
>  
> +static inline void set_ethtool_ops(struct net_device *dev, struct ethtool_ops *
> ops)
> +{
> +       dev->ethtool_ops = ops;
> +}


It needs to be a macro for maximum flexibility.

Also, no need to convert in-kernel drivers over to using it...  Let 
driver authors use it or not as they choose.

	Jeff

Re: [PATCH] ethtool_ops rev 4

[Matthew Wilcox]

On Sat, Aug 02, 2003 at 06:34:46PM -0400, Jeff Garzik wrote:
> >diff -u include/linux/netdevice.h include/linux/netdevice.h
> >--- include/linux/netdevice.h   31 Jul 2003 13:06:23 -0000
> >+++ include/linux/netdevice.h   2 Aug 2003 18:37:16 -0000
> >@@ -477,6 +477,10 @@
> >  */
> > #define SET_NETDEV_DEV(net, pdev)      ((net)->class_dev.dev = (pdev))
> > 
> >+static inline void set_ethtool_ops(struct net_device *dev, struct 
> >ethtool_ops *
> >ops)
> >+{
> >+       dev->ethtool_ops = ops;
> >+}
> 
> 
> It needs to be a macro for maximum flexibility.

Nothing stops it being implemented as a macro in kcompat.  Having it as
an inline function gives it argument typechecking which always gives me
the warm fuzzies.

> Also, no need to convert in-kernel drivers over to using it...  Let 
> driver authors use it or not as they choose.

I took "Like pci_set_drvdata" as the most important part of your
argument...  having everyone use it is no bad thing.

-- 
"It's not Hollywood.  War is real, war is primarily not about defeat or
victory, it is about death.  I've seen thousands and thousands of dead bodies.
Do you think I want to have an academic debate on this subject?" -- Robert Fisk

Re: [PATCH] ethtool_ops rev 4

[Jeff Garzik]

Matthew Wilcox wrote:
> On Sat, Aug 02, 2003 at 06:34:46PM -0400, Jeff Garzik wrote:
> 
>>>diff -u include/linux/netdevice.h include/linux/netdevice.h
>>>--- include/linux/netdevice.h   31 Jul 2003 13:06:23 -0000
>>>+++ include/linux/netdevice.h   2 Aug 2003 18:37:16 -0000
>>>@@ -477,6 +477,10 @@
>>> */
>>>#define SET_NETDEV_DEV(net, pdev)      ((net)->class_dev.dev = (pdev))
>>>
>>>+static inline void set_ethtool_ops(struct net_device *dev, struct 
>>>ethtool_ops *
>>>ops)
>>>+{
>>>+       dev->ethtool_ops = ops;
>>>+}
>>
>>
>>It needs to be a macro for maximum flexibility.
> 
> 
> Nothing stops it being implemented as a macro in kcompat.  Having it as
> an inline function gives it argument typechecking which always gives me
> the warm fuzzies.

No, it _needs_ to be a macro for maximum flexibility.

Most importantly, kcompat code may use '#ifndef SET_ETHTOOL_OPS' as a 
trigger, to signal that compat code is needed.  No need for drivers to 
create tons of kernel-version-code ifdefs, just to test for when 
ethtool_ops appeared in 2.6, for when it starts appearing in 2.4 vendor 
backports, and (possibly) 2.4 itself.  Also, doing it at the cpp level 
allows compat code to #undef it, if it _really_ knows what its doing, 
and the situation calls for it.


>>Also, no need to convert in-kernel drivers over to using it...  Let 
>>driver authors use it or not as they choose.
> 
> 
> I took "Like pci_set_drvdata" as the most important part of your
> argument...  having everyone use it is no bad thing.

Certainly.  I have no real preferences either way, just noting that 
in-kernel drivers don't _need_ to use this macro.

	Jeff

Re: [PATCH] ethtool_ops rev 4

[Matthew Wilcox]

On Sat, Aug 02, 2003 at 11:14:26PM -0400, Jeff Garzik wrote:
> Matthew Wilcox wrote:
> >Nothing stops it being implemented as a macro in kcompat.  Having it as
> >an inline function gives it argument typechecking which always gives me
> >the warm fuzzies.
> 
> No, it _needs_ to be a macro for maximum flexibility.
> 
> Most importantly, kcompat code may use '#ifndef SET_ETHTOOL_OPS' as a 
> trigger, to signal that compat code is needed.  No need for drivers to 
> create tons of kernel-version-code ifdefs, just to test for when 
> ethtool_ops appeared in 2.6, for when it starts appearing in 2.4 vendor 
> backports, and (possibly) 2.4 itself.  Also, doing it at the cpp level 
> allows compat code to #undef it, if it _really_ knows what its doing, 
> and the situation calls for it.

OK.  At this point, I really feel like I'm getting in the way and
hindering more than I'm helping.  Can I pass the torch to you and let
you finish the job?

-- 
"It's not Hollywood.  War is real, war is primarily not about defeat or
victory, it is about death.  I've seen thousands and thousands of dead bodies.
Do you think I want to have an academic debate on this subject?" -- Robert Fisk

Re: [PATCH] ethtool_ops rev 4

[Jeff Garzik]

Matthew Wilcox wrote:
> On Sat, Aug 02, 2003 at 11:14:26PM -0400, Jeff Garzik wrote:
> 
>>Matthew Wilcox wrote:
>>
>>>Nothing stops it being implemented as a macro in kcompat.  Having it as
>>>an inline function gives it argument typechecking which always gives me
>>>the warm fuzzies.
>>
>>No, it _needs_ to be a macro for maximum flexibility.
>>
>>Most importantly, kcompat code may use '#ifndef SET_ETHTOOL_OPS' as a 
>>trigger, to signal that compat code is needed.  No need for drivers to 
>>create tons of kernel-version-code ifdefs, just to test for when 
>>ethtool_ops appeared in 2.6, for when it starts appearing in 2.4 vendor 
>>backports, and (possibly) 2.4 itself.  Also, doing it at the cpp level 
>>allows compat code to #undef it, if it _really_ knows what its doing, 
>>and the situation calls for it.
> 
> 
> OK.  At this point, I really feel like I'm getting in the way and
> hindering more than I'm helping.  Can I pass the torch to you and let
> you finish the job?


Sorry to give that impression :(  I think we're pretty much "there". 
But if you wanna hand it off to me for the last little bits, and 
merging, that's fine too.  I'll leave it up to you.

	Jeff

Re: [PATCH] ethtool_ops rev 4

[Matthew Wilcox]

On Sun, Aug 03, 2003 at 01:09:11PM -0400, Jeff Garzik wrote:
> Matthew Wilcox wrote:
> >OK.  At this point, I really feel like I'm getting in the way and
> >hindering more than I'm helping.  Can I pass the torch to you and let
> >you finish the job?
> 
> Sorry to give that impression :(  I think we're pretty much "there". 
> But if you wanna hand it off to me for the last little bits, and 
> merging, that's fine too.  I'll leave it up to you.

Oh, I completely agree, I think we're down to quibbling over the last tiny
details.  And I think that's exactly why I should bow out at this point;
you know this area much better than I do.  I'm not leaving in a huff or
anything -- this was a weekend hack rather than a major project to me.

-- 
"It's not Hollywood.  War is real, war is primarily not about defeat or
victory, it is about death.  I've seen thousands and thousands of dead bodies.
Do you think I want to have an academic debate on this subject?" -- Robert Fisk

Re: [PATCH] ethtool_ops rev 4

[David S. Miller]

On Sat, 02 Aug 2003 18:34:46 -0400
Jeff Garzik <jgarzik@pobox.com> wrote:

> Matthew Wilcox wrote:
> > +static inline void set_ethtool_ops(struct net_device *dev, struct ethtool_ops *
> > ops)
> > +{
> > +       dev->ethtool_ops = ops;
> > +}
> 
> 
> It needs to be a macro for maximum flexibility.

Yes, and please name it with capitol letters, ie.
SET_ETHTOOL_OPS(), I have no idea why you used
lower-case letters when Jeff and I referred to it
consistently with caps. :-)