Patchwork [net-next,1/2] net: provide a default dev->ethtool_ops

login
register
mail settings
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

Eric Dumazet - Sept. 16, 2012, 7:17 p.m.
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.

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 <edumazet@google.com>
Cc: Ben Hutchings <bhutchings@solarflare.com>
Cc: Maciej ┼╗enczykowski <maze@google.com>
---
 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
Ben Hutchings - Sept. 16, 2012, 10:35 p.m.
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(&ethcmd, 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:
> 
>
David Miller - Sept. 19, 2012, 7:41 p.m.
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(&ethcmd, 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: