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

Subsystems: networking [general], the rest

STALE5069d

6 messages, 4 authors, 2012-09-19 · open the first message on its own page

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

From: Eric Dumazet <hidden>
Date: 2012-09-14 07:24:08

From: Eric Dumazet <edumazet@google.com>

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: Maciej Żenczykowski <redacted>
---
 net/core/dev.c     |    5 +++
 net/core/ethtool.c |   54 +++++++++++++++++--------------------------
 2 files changed, 27 insertions(+), 32 deletions(-)
diff --git a/net/core/dev.c b/net/core/dev.c
index b1e6d63..ff8dcfc 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -6045,6 +6045,11 @@ 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) {
+		static const struct ethtool_ops default_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..aef0162 100644
--- a/net/core/ethtool.c
+++ b/net/core/ethtool.c
@@ -175,7 +175,7 @@ static int __ethtool_get_sset_count(struct net_device *dev, int sset)
 	if (sset == ETH_SS_FEATURES)
 		return ARRAY_SIZE(netdev_features_strings);
 
-	if (ops && ops->get_sset_count && ops->get_strings)
+	if (ops->get_sset_count && ops->get_strings)
 		return ops->get_sset_count(dev, sset);
 	else
 		return -EOPNOTSUPP;
@@ -311,7 +311,7 @@ int __ethtool_get_settings(struct net_device *dev, struct ethtool_cmd *cmd)
 {
 	ASSERT_RTNL();
 
-	if (!dev->ethtool_ops || !dev->ethtool_ops->get_settings)
+	if (!dev->ethtool_ops->get_settings)
 		return -EOPNOTSUPP;
 
 	memset(cmd, 0, sizeof(struct ethtool_cmd));
@@ -355,7 +355,7 @@ static noinline_for_stack int ethtool_get_drvinfo(struct net_device *dev,
 
 	memset(&info, 0, sizeof(info));
 	info.cmd = ETHTOOL_GDRVINFO;
-	if (ops && ops->get_drvinfo) {
+	if (ops->get_drvinfo) {
 		ops->get_drvinfo(dev, &info);
 	} else if (dev->dev.parent && dev->dev.parent->driver) {
 		strlcpy(info.bus_info, dev_name(dev->dev.parent),
@@ -370,7 +370,7 @@ static noinline_for_stack int ethtool_get_drvinfo(struct net_device *dev,
 	 * this method of obtaining string set info is deprecated;
 	 * Use ETHTOOL_GSSET_INFO instead.
 	 */
-	if (ops && ops->get_sset_count) {
+	if (ops->get_sset_count) {
 		int rc;
 
 		rc = ops->get_sset_count(dev, ETH_SS_TEST);
@@ -383,9 +383,9 @@ static noinline_for_stack int ethtool_get_drvinfo(struct net_device *dev,
 		if (rc >= 0)
 			info.n_priv_flags = rc;
 	}
-	if (ops && ops->get_regs_len)
+	if (ops->get_regs_len)
 		info.regdump_len = ops->get_regs_len(dev);
-	if (ops && ops->get_eeprom_len)
+	if (ops->get_eeprom_len)
 		info.eedump_len = ops->get_eeprom_len(dev);
 
 	if (copy_to_user(useraddr, &info, sizeof(info)))
@@ -1275,7 +1275,7 @@ static int ethtool_get_dump_flag(struct net_device *dev,
 	struct ethtool_dump dump;
 	const struct ethtool_ops *ops = dev->ethtool_ops;
 
-	if (!dev->ethtool_ops->get_dump_flag)
+	if (!ops->get_dump_flag)
 		return -EOPNOTSUPP;
 
 	if (copy_from_user(&dump, useraddr, sizeof(dump)))
@@ -1299,8 +1299,7 @@ static int ethtool_get_dump_data(struct net_device *dev,
 	const struct ethtool_ops *ops = dev->ethtool_ops;
 	void *data = NULL;
 
-	if (!dev->ethtool_ops->get_dump_data ||
-		!dev->ethtool_ops->get_dump_flag)
+	if (!ops->get_dump_data || !ops->get_dump_flag)
 		return -EOPNOTSUPP;
 
 	if (copy_from_user(&dump, useraddr, sizeof(dump)))
@@ -1349,7 +1348,7 @@ static int ethtool_get_ts_info(struct net_device *dev, void __user *useraddr)
 
 		err = phydev->drv->ts_info(phydev, &info);
 
-	} else if (dev->ethtool_ops && dev->ethtool_ops->get_ts_info) {
+	} else if (ops->get_ts_info) {
 
 		err = ops->get_ts_info(dev, &info);
 
@@ -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.
+ */
 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;
 	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:
@@ -1476,8 +1466,8 @@ int dev_ethtool(struct net *net, struct ifreq *ifr)
 			return -EPERM;
 	}
 
-	if (dev->ethtool_ops->begin) {
-		rc = dev->ethtool_ops->begin(dev);
+	if (ops->begin) {
+		rc = ops->begin(dev);
 		if (rc  < 0)
 			return rc;
 	}
@@ -1504,11 +1494,11 @@ int dev_ethtool(struct net *net, struct ifreq *ifr)
 		break;
 	case ETHTOOL_GMSGLVL:
 		rc = ethtool_get_value(dev, useraddr, ethcmd,
-				       dev->ethtool_ops->get_msglevel);
+				       ops->get_msglevel);
 		break;
 	case ETHTOOL_SMSGLVL:
 		rc = ethtool_set_value_void(dev, useraddr,
-				       dev->ethtool_ops->set_msglevel);
+					    ops->set_msglevel);
 		break;
 	case ETHTOOL_GEEE:
 		rc = ethtool_get_eee(dev, useraddr);
@@ -1570,11 +1560,11 @@ int dev_ethtool(struct net *net, struct ifreq *ifr)
 		break;
 	case ETHTOOL_GPFLAGS:
 		rc = ethtool_get_value(dev, useraddr, ethcmd,
-				       dev->ethtool_ops->get_priv_flags);
+				       ops->get_priv_flags);
 		break;
 	case ETHTOOL_SPFLAGS:
 		rc = ethtool_set_value(dev, useraddr,
-				       dev->ethtool_ops->set_priv_flags);
+				       ops->set_priv_flags);
 		break;
 	case ETHTOOL_GRXFH:
 	case ETHTOOL_GRXRINGS:
@@ -1655,8 +1645,8 @@ int dev_ethtool(struct net *net, struct ifreq *ifr)
 		rc = -EOPNOTSUPP;
 	}
 
-	if (dev->ethtool_ops->complete)
-		dev->ethtool_ops->complete(dev);
+	if (ops->complete)
+		ops->complete(dev);
 
 	if (old_features != dev->features)
 		netdev_features_change(dev);

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

From: Maciej Żenczykowski <hidden>
Date: 2012-09-14 17:10:54

Any cleanups like this are always nice to see.

On Fri, Sep 14, 2012 at 12:24 AM, Eric Dumazet [off-list ref] wrote:
quoted hunk
From: Eric Dumazet <edumazet@google.com>

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: Maciej Żenczykowski <redacted>
---
 net/core/dev.c     |    5 +++
 net/core/ethtool.c |   54 +++++++++++++++++--------------------------
 2 files changed, 27 insertions(+), 32 deletions(-)
diff --git a/net/core/dev.c b/net/core/dev.c
index b1e6d63..ff8dcfc 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -6045,6 +6045,11 @@ 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) {
+               static const struct ethtool_ops default_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..aef0162 100644
--- a/net/core/ethtool.c
+++ b/net/core/ethtool.c
@@ -175,7 +175,7 @@ static int __ethtool_get_sset_count(struct net_device *dev, int sset)
        if (sset == ETH_SS_FEATURES)
                return ARRAY_SIZE(netdev_features_strings);

-       if (ops && ops->get_sset_count && ops->get_strings)
+       if (ops->get_sset_count && ops->get_strings)
                return ops->get_sset_count(dev, sset);
        else
                return -EOPNOTSUPP;
@@ -311,7 +311,7 @@ int __ethtool_get_settings(struct net_device *dev, struct ethtool_cmd *cmd)
 {
        ASSERT_RTNL();

-       if (!dev->ethtool_ops || !dev->ethtool_ops->get_settings)
+       if (!dev->ethtool_ops->get_settings)
                return -EOPNOTSUPP;

        memset(cmd, 0, sizeof(struct ethtool_cmd));
@@ -355,7 +355,7 @@ static noinline_for_stack int ethtool_get_drvinfo(struct net_device *dev,

        memset(&info, 0, sizeof(info));
        info.cmd = ETHTOOL_GDRVINFO;
-       if (ops && ops->get_drvinfo) {
+       if (ops->get_drvinfo) {
                ops->get_drvinfo(dev, &info);
        } else if (dev->dev.parent && dev->dev.parent->driver) {
                strlcpy(info.bus_info, dev_name(dev->dev.parent),
@@ -370,7 +370,7 @@ static noinline_for_stack int ethtool_get_drvinfo(struct net_device *dev,
         * this method of obtaining string set info is deprecated;
         * Use ETHTOOL_GSSET_INFO instead.
         */
-       if (ops && ops->get_sset_count) {
+       if (ops->get_sset_count) {
                int rc;

                rc = ops->get_sset_count(dev, ETH_SS_TEST);
@@ -383,9 +383,9 @@ static noinline_for_stack int ethtool_get_drvinfo(struct net_device *dev,
                if (rc >= 0)
                        info.n_priv_flags = rc;
        }
-       if (ops && ops->get_regs_len)
+       if (ops->get_regs_len)
                info.regdump_len = ops->get_regs_len(dev);
-       if (ops && ops->get_eeprom_len)
+       if (ops->get_eeprom_len)
                info.eedump_len = ops->get_eeprom_len(dev);

        if (copy_to_user(useraddr, &info, sizeof(info)))
@@ -1275,7 +1275,7 @@ static int ethtool_get_dump_flag(struct net_device *dev,
        struct ethtool_dump dump;
        const struct ethtool_ops *ops = dev->ethtool_ops;

-       if (!dev->ethtool_ops->get_dump_flag)
+       if (!ops->get_dump_flag)
                return -EOPNOTSUPP;

        if (copy_from_user(&dump, useraddr, sizeof(dump)))
@@ -1299,8 +1299,7 @@ static int ethtool_get_dump_data(struct net_device *dev,
        const struct ethtool_ops *ops = dev->ethtool_ops;
        void *data = NULL;

-       if (!dev->ethtool_ops->get_dump_data ||
-               !dev->ethtool_ops->get_dump_flag)
+       if (!ops->get_dump_data || !ops->get_dump_flag)
                return -EOPNOTSUPP;

        if (copy_from_user(&dump, useraddr, sizeof(dump)))
@@ -1349,7 +1348,7 @@ static int ethtool_get_ts_info(struct net_device *dev, void __user *useraddr)

                err = phydev->drv->ts_info(phydev, &info);

-       } else if (dev->ethtool_ops && dev->ethtool_ops->get_ts_info) {
+       } else if (ops->get_ts_info) {

                err = ops->get_ts_info(dev, &info);
@@ -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.
+ */
 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;
        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:
@@ -1476,8 +1466,8 @@ int dev_ethtool(struct net *net, struct ifreq *ifr)
                        return -EPERM;
        }

-       if (dev->ethtool_ops->begin) {
-               rc = dev->ethtool_ops->begin(dev);
+       if (ops->begin) {
+               rc = ops->begin(dev);
                if (rc  < 0)
                        return rc;
        }
@@ -1504,11 +1494,11 @@ int dev_ethtool(struct net *net, struct ifreq *ifr)
                break;
        case ETHTOOL_GMSGLVL:
                rc = ethtool_get_value(dev, useraddr, ethcmd,
-                                      dev->ethtool_ops->get_msglevel);
+                                      ops->get_msglevel);
                break;
        case ETHTOOL_SMSGLVL:
                rc = ethtool_set_value_void(dev, useraddr,
-                                      dev->ethtool_ops->set_msglevel);
+                                           ops->set_msglevel);
                break;
        case ETHTOOL_GEEE:
                rc = ethtool_get_eee(dev, useraddr);
@@ -1570,11 +1560,11 @@ int dev_ethtool(struct net *net, struct ifreq *ifr)
                break;
        case ETHTOOL_GPFLAGS:
                rc = ethtool_get_value(dev, useraddr, ethcmd,
-                                      dev->ethtool_ops->get_priv_flags);
+                                      ops->get_priv_flags);
                break;
        case ETHTOOL_SPFLAGS:
                rc = ethtool_set_value(dev, useraddr,
-                                      dev->ethtool_ops->set_priv_flags);
+                                      ops->set_priv_flags);
                break;
        case ETHTOOL_GRXFH:
        case ETHTOOL_GRXRINGS:
@@ -1655,8 +1645,8 @@ int dev_ethtool(struct net *net, struct ifreq *ifr)
                rc = -EOPNOTSUPP;
        }

-       if (dev->ethtool_ops->complete)
-               dev->ethtool_ops->complete(dev);
+       if (ops->complete)
+               ops->complete(dev);

        if (old_features != dev->features)
                netdev_features_change(dev);

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

From: Ben Hutchings <hidden>
Date: 2012-09-15 13:40:52

On Fri, 2012-09-14 at 09:24 +0200, Eric Dumazet wrote:
From: Eric Dumazet <edumazet@google.com>

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: Maciej Żenczykowski <redacted>
Yes, I should have done this ages ago.  Just some nit-picking below:

[...]
quoted hunk
diff --git a/net/core/dev.c b/net/core/dev.c
index b1e6d63..ff8dcfc 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -6045,6 +6045,11 @@ 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) {
+		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.

[...] 
quoted hunk
@@ -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.
quoted hunk
 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.

Ben.

-- 
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.

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

From: Eric Dumazet <hidden>
Date: 2012-09-16 19:17:36

From: Eric Dumazet <edumazet@google.com>

On Sat, 2012-09-15 at 14:40 +0100, Ben Hutchings wrote:
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.

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


[...] 
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.
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.

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>
---
 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:

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

From: Ben Hutchings <hidden>
Date: 2012-09-16 22:36:02

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>
quoted hunk
---
 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:
-- 
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.

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

From: David Miller <davem@davemloft.net>
Date: 2012-09-19 19:41:20

From: Eric Dumazet <redacted>
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 <redacted>
Cc: Maciej Żenczykowski <redacted>
Great work, applied, thanks Eric.
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help