RE: [PATCH] ethtool_ops

RE: [PATCH] ethtool_ops

[Feldman, Scott]

> Right now, each network driver which supports the ethtool 
> ioctl has its own implementation of everything from decoding 
> which ethtool ioctl it is, copying data to and from 
> userspace, marshalling and unmarshalling data from ethtool 
> packets, etc.  The current setup makes it impossible to use 
> alternative interfaces to get at the same data (eg sysfs) and 
> it's not exactly typesafe.

This is really cool!  Thanks for doing this Matthew.

Some questions:

* On get_gregs, for example, would it make sense to ->get_drvinfo
  so you'll know regdump_len and therefore can kmalloc an ethtool_regs
  with enough space to pass to ->get_regs?  Keep the kmalloc and
  kfree together.  Same for self_test, get_strings, and get_stats.
  For get_strings, size = max{n_stats, testinfo_len)*sizeof(u64).

* If the above is done, can we have one function type for the
ethtool_ops
  functions?  int f(struct netdev *, struct ethtool_cmd *).  The 
  drawback is the driver needs to cast to the specific ethtool_* struct.

* Can we get an HAVE_ETHTOOL_OPS defined in netdevice.h to support
  backward compat?

-scott

Re: [PATCH] ethtool_ops

[Jeff Garzik]

On Fri, Jun 06, 2003 at 01:17:46PM -0700, Feldman, Scott wrote:
> > Right now, each network driver which supports the ethtool 
> > ioctl has its own implementation of everything from decoding 
> > which ethtool ioctl it is, copying data to and from 
> > userspace, marshalling and unmarshalling data from ethtool 
> > packets, etc.  The current setup makes it impossible to use 
> > alternative interfaces to get at the same data (eg sysfs) and 
> > it's not exactly typesafe.
> 
> This is really cool!  Thanks for doing this Matthew.

Indeed.  :)


> Some questions:
> 
> * On get_gregs, for example, would it make sense to ->get_drvinfo
>   so you'll know regdump_len and therefore can kmalloc an ethtool_regs
>   with enough space to pass to ->get_regs?  Keep the kmalloc and
>   kfree together.  Same for self_test, get_strings, and get_stats.
>   For get_strings, size = max{n_stats, testinfo_len)*sizeof(u64).

Yes, absolutely.

There is a bug in some of the arch ioctl32 translation layers
that just assumes the ethtool output can fit inside a single page.
Prior to Matthew's work, the ioctl32 layer needed to directly issue
ETHTOOL_GDRVINFO ioctl to obtain this information.  Now, the ioctl32
layer can directly call ->get_drvinfo.  This is what the drvinfo
information is designed to be used for.

Hooks being able to call ->get_drvinfo (and perhaps a couple others)
is an important and useful attribute.


> * If the above is done, can we have one function type for the
> ethtool_ops
>   functions?  int f(struct netdev *, struct ethtool_cmd *).  The 
>   drawback is the driver needs to cast to the specific ethtool_* struct.

I disagree:  prefer the increased type checking and lack of type
casting.  We already have net/core/ethtool.c having a separate call-it
function for each hook... and each of those functions needs to know
the specific subcommand struct type _anyway_ to copy in the struct
from userspace, so let's go ahead and propagate the type information.

Typically in Linux we want to preserve as much type information as
possible.  Less work for the compiler, less potential aliasing problems,
and discourages type mistakes in the low-level driver.


> * Can we get an HAVE_ETHTOOL_OPS defined in netdevice.h to support
>   backward compat?

heh, I suggested this independently, too.

	Jeff

Re: [PATCH] ethtool_ops

[Matthew Wilcox]

(sorry for the delayed reply; PCI domain work got a lot more important
for a while)

On Fri, Jun 06, 2003 at 01:17:46PM -0700, Feldman, Scott wrote:
> * On get_gregs, for example, would it make sense to ->get_drvinfo
>   so you'll know regdump_len and therefore can kmalloc an ethtool_regs
>   with enough space to pass to ->get_regs?  Keep the kmalloc and
>   kfree together.  Same for self_test, get_strings, and get_stats.
>   For get_strings, size = max{n_stats, testinfo_len)*sizeof(u64).

That would be one possibility, but get_drvinfo is quite heavyweight.
I think I'd prefer to not do that unless there's a strong feeling about
thing.

> * If the above is done, can we have one function type for the
> ethtool_ops
>   functions?  int f(struct netdev *, struct ethtool_cmd *).  The 
>   drawback is the driver needs to cast to the specific ethtool_* struct.

Definitely not -- the point is adding type safety.

> * Can we get an HAVE_ETHTOOL_OPS defined in netdevice.h to support
>   backward compat?

I'm hoping to avoid that by getting compatibility code merged into 2.4.22.

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

[Feldman, Scott]

> On Fri, Jun 06, 2003 at 01:17:46PM -0700, Feldman, Scott wrote:
> > * On get_gregs, for example, would it make sense to ->get_drvinfo
> >   so you'll know regdump_len and therefore can kmalloc an 
> ethtool_regs
> >   with enough space to pass to ->get_regs?  Keep the kmalloc and
> >   kfree together.  Same for self_test, get_strings, and get_stats.
> >   For get_strings, size = max{n_stats, testinfo_len)*sizeof(u64).
> 
> That would be one possibility, but get_drvinfo is quite 
> heavyweight. I think I'd prefer to not do that unless there's 
> a strong feeling about thing.

I'm pretty sure you want to do this.  The less work done in the drivers,
the better.  See Jeff's response on this as well.
 
> > * Can we get an HAVE_ETHTOOL_OPS defined in netdevice.h to support
> >   backward compat?
> 
> I'm hoping to avoid that by getting compatibility code merged 
> into 2.4.22.

I'm not sure what compatibility code you're referring to.  We need to
target older kernels with the same code base, so a simple
HAVE_ETHTOOL_OPS would make this easy.  I'd really like to move over to
ethtool_ops for e100/e1000/ixgb, but it's problematic if we need to
manage multiple code bases.

-scott

[PATCH] ethtool_ops

[Matthew Wilcox]

The patch is 40k, so I'm not going to post it inline.
http://ftp.linux.org.uk/pub/linux/willy/patches/ethtool.diff

Right now, each network driver which supports the ethtool ioctl has its
own implementation of everything from decoding which ethtool ioctl it is,
copying data to and from userspace, marshalling and unmarshalling data
from ethtool packets, etc.  The current setup makes it impossible to
use alternative interfaces to get at the same data (eg sysfs) and it's
not exactly typesafe.

This patch introduces ethtool_ops and converts tg3 to use it.
Drivers don't access userspace on their own under this scheme; they
just do the requested operation and return the appropriate value(s).
Compile-tested only; design approved by jgarzik.  Comments welcomed.

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

[David S. Miller]

   From: Matthew Wilcox <willy@debian.org>
   Date: Fri, 6 Jun 2003 15:29:51 +0100
   
   This patch introduces ethtool_ops and converts tg3 to use it.

I really like this.