| Submitter | Eric Dumazet |
|---|---|
| Date | Sept. 16, 2012, 7:17 p.m. |
| Message ID | <1347823046.26523.44.camel@edumazet-glaptop> |
| Download | mbox | patch |
| Permalink | /patch/184143/ |
| State | Accepted |
| Delegated to: | David Miller |
| Headers | show |
Comments
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: > > > > 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. > > > [...] > > > @@ -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. > > > 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 <bhutchings@solarflare.com> > Cc: Maciej Żenczykowski <maze@google.com> Reviewed-by: Ben Hutchings <bhutchings@solarflare.com> > --- > net/core/dev.c | 4 ++++ > net/core/ethtool.c | 12 ------------ > 2 files changed, 4 insertions(+), 12 deletions(-) > > 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: > >
From: Eric Dumazet <eric.dumazet@gmail.com> Date: Sun, 16 Sep 2012 21:17:26 +0200 > From: Eric Dumazet <edumazet@google.com> ... > [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 <bhutchings@solarflare.com> > Cc: Maciej Żenczykowski <maze@google.com> Great work, applied, thanks Eric. -- 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
Patch
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: