Re: [PATCH net-next 1/2] net: provide a default dev->ethtool_ops

On Sun, 2012-09-16 at 21:17 +0200, Eric Dumazet wrote:

From: Eric Dumazet <edumazet@google.com>

On Sat, 2012-09-15 at 14:40 +0100, Ben Hutchings wrote:

quoted

quoted

 	strcpy(dev->name, name);
 	dev->group = INIT_NETDEV_GROUP;
+	if (!dev->ethtool_ops) {
+		static const struct ethtool_ops default_ethtool_ops;
+
+		dev->ethtool_ops = &default_ethtool_ops;
+	}

This block has a blank line in it, so I think it needs a blank line
either side to make the visual grouping of code right.  Alternately you
could pull the variable out of the block.

Blank line is preferred after a variable declaration in a function.

Yes but:

	when the previous statement is right next to;
	the start of the block {
		the variable declaration is visually grouped with that;

		rather than with the statement that uses it;
	}

I dont feel the need to make this variable visible outside of this
function yet. But if you feel it, no problem.

quoted

[...] 

quoted

@@ -1410,8 +1409,9 @@ static int ethtool_get_module_eeprom(struct net_device *dev,
 				      modinfo.eeprom_len);
 }
 
-/* The main entry point in this file.  Called from net/core/dev.c */
-
+/* The main entry point in this file.  Called from net/core/dev.c
+ * with RTNL held.
+ */

Good point but an unrelated change.

Its related, because I wanted to make clear (at least for me)
why assuming dev->ethtool_ops was not NULL at this point was valid.

So we know it won't change under us?  But that is also true of
dev->netdev_ops, which is often used with a finer-grained lock.

quoted

quoted

 int dev_ethtool(struct net *net, struct ifreq *ifr)
 {
 	struct net_device *dev = __dev_get_by_name(net, ifr->ifr_name);

@@ -1419,25 +1419,15 @@ int dev_ethtool(struct net *net, struct ifreq *ifr)
 	u32 ethcmd;
 	int rc;
 	u32 old_features;
+	const struct ethtool_ops *ops;
 
 	if (!dev || !netif_device_present(dev))
 		return -ENODEV;
 
+	ops = dev->ethtool_ops;

[...]

Introducing this local variable is a useful cleanup but again should be
a separate change.

Its a patch meant for net-next, and a cleanup. I could understand your
point if we had to backport this to stable trees, buts its not the
case ?

I really dont care, so if you really prefer I dont cleanup ethtool.c, so
be it.

Some functions test dev->ethtool_ops is NULL, others lack this test.
This just makes no sense to me, maybe there is something I missed.

Most of those checks are probably redundant, and I see no problem with
your removing them.  It was just that you were also changing various
other references to dev->ethtool_ops to use local variables.

Thanks

[PATCH v2 net-next] net: provide a default dev->ethtool_ops

Instead of forcing device drivers to provide empty ethtool_ops or tweak
net/core/ethtool.c again, we could provide a generic ethtool_ops.

This occurred to me when I wanted to add GSO support to GRE tunnels.
ethtool -k support should be generic for all drivers.

Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: Ben Hutchings <redacted>
Cc: Maciej Żenczykowski <redacted>

Reviewed-by: Ben Hutchings <redacted>

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.