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