On Fri, Jul 11, 2003 at 03:32:15PM -0400, Jeff Garzik wrote:
> 1) The _ops are either too limited in scope, or too wide in scope.
Couldn't agree more. I blame acme -- he wants me to push it to be much
wider in scope. Let's push _all_ the function pointers into netdev_ops.
But this is a mere step 1. I don't have enough network-related clout
to do everything in one fell swoop.
> 2.c) If #2 is decided to be netdev_ops, and all func ptrs are moved into
> netdev_ops struct, then create the macro
> SET_NETDEV_OPS(dev, ops)
> This allows full back compat, without ugliness in mainline tree.
Yes, that was my preferred approach.
> 3) The func ptrs _count() are totally bogus. We have an unconditional
> indirect reference to a function call which does nothing but return a
> driver constant.
> I personally think that having ethtool_ops members manually calling
> the ->get_drvinfo hook is a _lot_ cleaner than 10,000 foo_count hooks.
Disagree. I'd like to completely get rid of the ->get_drvinfo hook and
have each hook return one thing. DaveM claims that these things are not
always constants, and I believe him -- it's entirely possible different
revs of a chip (with the same driver) may have more or fewer registers
to return, for example.
We might want to put these counts directly in the net_device itself and
eliminate the function calls. That would make sense.
> 4) I don't see why ethtool.h suddenly needs to include linux/types.h,
> when it hasn't needed it in all this time until now.
Otherwise you have to include <linux/types.h> before you include
<linux/ethtool.h> which sucks. No relying on other people to do your
inclusions for you ;-)
> 5) net/socket.c changes appear unrelated to this patch.
You're right, they just happen to be in that tree.
> 6) (low prio) Add documentation to
> Documentation/networking/netdevices.txt. Most importantly, this
> documents locking/context.
An excellent idea.
> 7) (low prio) All that similar code in net/core/ethtool.c can be
> template-ized with a macro, IMO. Something like
> DEF_ETHTOOL_GOP(get_coalesce, ETHTOOL_GCOALESCE, ethtool_coalesce);
> DEF_ETHTOOL_SOP(set_coalesce, ethtool_coalesce);
> (and templates for the ops that use edata)
Maybe. I'm not a fan of templated ops as it makes it harder to grep.
> 8) (security) get-eeprom op needs to check that offset+len is not
> invalid, and does not wrap.
Good idea, I'll add that check now.
> 9) phys_id op should return an error, for consistency if nothing else.
> It's simple for driver authors to unconditionally return 0 if their code
> has no failure cases, and it's a slow path so adding the return in the
> driver code is no big deal.
> 10) (low prio) since it's a slow path, what about replacing the switch
> statement in dev_ethtool() with a lookup table? All the ethtool
> commands are low numbers. If you do this, I would suggest using the gcc
> array initializer syntax:
> [ETHTOOL_GCOALESCE, ethtool_get_coalesce]
> All the ethtool ops have the same prototype, after all.
Well, they don't have quite the same prototype ... that's part of the
point -- get the type safety going as early as possible.
"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