From patchwork Sun Sep 16 19:17:26 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Subject: [net-next,1/2] net: provide a default dev->ethtool_ops Date: Sun, 16 Sep 2012 09:17:26 -0000 From: Eric Dumazet X-Patchwork-Id: 184143 Message-Id: <1347823046.26523.44.camel@edumazet-glaptop> To: Ben Hutchings Cc: David Miller , netdev , Maciej =?UTF-8?Q?=C5=BBenczykowski?= From: Eric Dumazet On Sat, 2012-09-15 at 14:40 +0100, Ben Hutchings wrote: > > 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. I dont feel the need to make this variable visible outside of this function yet. But if you feel it, no problem. > [...] > > @@ -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. > > > 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. 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 Cc: Ben Hutchings Cc: Maciej Żenczykowski Reviewed-by: Ben Hutchings --- net/core/dev.c | 4 ++++ net/core/ethtool.c | 12 ------------ 2 files changed, 4 insertions(+), 12 deletions(-) -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html diff --git a/net/core/dev.c b/net/core/dev.c index dcc673d..2bcb02c 100644 --- a/net/core/dev.c +++ b/net/core/dev.c @@ -5959,6 +5959,8 @@ struct netdev_queue *dev_ingress_queue_create(struct net_device *dev) return queue; } +static const struct ethtool_ops default_ethtool_ops; + /** * alloc_netdev_mqs - allocate network device * @sizeof_priv: size of private data to allocate space for @@ -6046,6 +6048,8 @@ struct net_device *alloc_netdev_mqs(int sizeof_priv, const char *name, strcpy(dev->name, name); dev->group = INIT_NETDEV_GROUP; + if (!dev->ethtool_ops) + dev->ethtool_ops = &default_ethtool_ops; return dev; free_all: diff --git a/net/core/ethtool.c b/net/core/ethtool.c index cbf033d..4d64cc2 100644 --- a/net/core/ethtool.c +++ b/net/core/ethtool.c @@ -1426,18 +1426,6 @@ int dev_ethtool(struct net *net, struct ifreq *ifr) if (copy_from_user(ðcmd, useraddr, sizeof(ethcmd))) return -EFAULT; - if (!dev->ethtool_ops) { - /* A few commands do not require any driver support, - * are unprivileged, and do not change anything, so we - * can take a shortcut to them. */ - if (ethcmd == ETHTOOL_GDRVINFO) - return ethtool_get_drvinfo(dev, useraddr); - else if (ethcmd == ETHTOOL_GET_TS_INFO) - return ethtool_get_ts_info(dev, useraddr); - else - return -EOPNOTSUPP; - } - /* Allow some commands to be done by anyone */ switch (ethcmd) { case ETHTOOL_GSET: