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.
OK, ditto.
> 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
|