wireless: Fix ethtool stats and other ops.

PATCH In review [email protected] 5 participants 11 messages Dec 5, 2012 – Dec 10, 2012

Subsystems (3)

Networking Network drivers 802.11 (including cfg80211/NL80211)

lore

Share & tools

lore

[email protected]

author

2012-12-05 17:40 UTC #0 lore raw

PATCH 1/1 wireless: Fix ethtool stats and other ops.

From: Ben Greear <[email protected]>

net/core/dev.c now assigns a default ethtool ops, so the net/wireless/core.c check for existing ops is always true so the wireless ops would never be assigned.

Simply remove the check for existing ops and always assign the wireless ops.

Signed-off-by Ben Greear <[email protected]>

---
 net/wireless/core.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

net/wireless/core.c +1 -2

index 4e6fe62..6309699 100644
@@ -863,8 +863,7 @@ static int cfg80211_netdev_notifier_call(struct notifier_block *nb,
 		/* allow mac80211 to determine the timeout */
 		wdev->ps_timeout = -1;
 
-		if (!dev->ethtool_ops)
-			dev->ethtool_ops = &cfg80211_ethtool_ops;
+		dev->ethtool_ops = &cfg80211_ethtool_ops;
 
 		if ((wdev->iftype == NL80211_IFTYPE_STATION ||
 		     wdev->iftype == NL80211_IFTYPE_P2P_CLIENT ||

Signature · 1 line

1.7.3.4

Ben Greear

author

2012-12-05 18:02 UTC #2 lore raw

Quoted 26 lines · Bjørn Mork

Quoted 24 lines · Ben Greear
net/core/dev.c now assigns a default ethtool ops, so the net/wireless/core.c check for existing ops is always true so the wireless ops would never be assigned.
Simply remove the check for existing ops and always assign the wireless ops.
Signed-off-by Ben Greear <[email protected]>
---
  net/wireless/core.c |    3 +--
  1 files changed, 1 insertions(+), 2 deletions(-)
net/wireless/core.c +1 -2
index 4e6fe62..6309699 100644
@@ -863,8 +863,7 @@ static int cfg80211_netdev_notifier_call(struct notifier_block *nb,
  		/* allow mac80211 to determine the timeout */
  		wdev->ps_timeout = -1;
 
-		if (!dev->ethtool_ops)
-			dev->ethtool_ops = &cfg80211_ethtool_ops;
+		dev->ethtool_ops = &cfg80211_ethtool_ops;
 

Quoted 6 lines

net/wireless/core.c static int cfg80211_netdev_notifier_call(struct notifier_block *nb, · line 869
Quoted 2 lines
net/wireless/core.c +0 -0
  		if ((wdev->iftype == NL80211_IFTYPE_STATION ||
  		     wdev->iftype == NL80211_IFTYPE_P2P_CLIENT ||
Won't this break drivers which for some reason have their own ethtool_ops?

I guess it will. What a mess.

Maybe we could assign individual method pointers in the ethtool_ops struct if it already exists (and if those pointers are NULL)?

Thanks,
Ben

Quoted 13 lines

bjorn@nemi:/usr/local/src/git/linux$ git grep -- "->ethtool_ops" drivers/net/wireless/
drivers/net/wireless/brcm80211/brcmfmac/dhd_linux.c:    ndev->ethtool_ops = &brcmf_ethtool_ops;
drivers/net/wireless/ipw2x00/ipw2100.c: dev->ethtool_ops = &ipw2100_ethtool_ops;
drivers/net/wireless/ipw2x00/ipw2200.c: net_dev->ethtool_ops = &ipw_ethtool_ops;
drivers/net/wireless/libertas/main.c:   dev->ethtool_ops = &lbs_ethtool_ops;
drivers/net/wireless/libertas/mesh.c:   mesh_dev->ethtool_ops = &lbs_ethtool_ops;
drivers/net/wireless/prism54/islpci_dev.c:      ndev->ethtool_ops = &islpci_ethtool_ops;

Bjørn

Signature · 2 lines

Ben Greear <[email protected]>
Candela Technologies Inc  http://www.candelatech.com

Stanislaw Gruszka

2012-12-06 12:25 UTC #3 lore raw

Quoted 34 lines · Ben Greear

Quoted 26 lines · Bjørn Mork
Quoted 24 lines · Ben Greear
net/core/dev.c now assigns a default ethtool ops, so the net/wireless/core.c check for existing ops is always true so the wireless ops would never be assigned.
Simply remove the check for existing ops and always assign the wireless ops.
Signed-off-by Ben Greear <[email protected]>
---
 net/wireless/core.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)
net/wireless/core.c +1 -2
index 4e6fe62..6309699 100644
@@ -863,8 +863,7 @@ static int cfg80211_netdev_notifier_call(struct notifier_block *nb,
 		/* allow mac80211 to determine the timeout */
 		wdev->ps_timeout = -1;
 
-		if (!dev->ethtool_ops)
-			dev->ethtool_ops = &cfg80211_ethtool_ops;
+		dev->ethtool_ops = &cfg80211_ethtool_ops;
 
Quoted 6 lines
net/wireless/core.c static int cfg80211_netdev_notifier_call(struct notifier_block *nb, · line 869
Quoted 2 lines
net/wireless/core.c +0 -0
 		if ((wdev->iftype == NL80211_IFTYPE_STATION ||
 		     wdev->iftype == NL80211_IFTYPE_P2P_CLIENT ||
Won't this break drivers which for some reason have their own ethtool_ops?

Quoted 4 lines

I guess it will. What a mess.
Maybe we could assign individual method pointers in the ethtool_ops struct if it already exists (and if those pointers are NULL)?

We should probably assing ethtool_ops before wireless core will call alloc_netdev.

Stanislaw

Ben Greear

author

2012-12-06 20:15 UTC #4 lore raw

Quoted 5 lines · Stanislaw Gruszka

Quoted 5 lines
Quoted 2 lines
Won't this break drivers which for some reason have their own ethtool_ops?
I guess it will. What a mess.

Quoted 8 lines

Quoted 2 lines
Maybe we could assign individual method pointers in the ethtool_ops struct if it already exists (and if those pointers are NULL)?
We should probably assing ethtool_ops before wireless core will call alloc_netdev.
Stanislaw

I don't have time to work on this today...maybe not tomorrow either.

So, if someone else feels like fixing it, please feel free.

Thanks,
Ben

Signature · 2 lines

Ben Greear <[email protected]>
Candela Technologies Inc  http://www.candelatech.com

Stanislaw Gruszka

2012-12-07 09:50 UTC #5 lore raw

Quoted 3 lines · Ben Greear

I don't have time to work on this today...maybe not tomorrow either.
So, if someone else feels like fixing it, please feel free.

Ok, I'll try to fix that. However this could require modification of all cfg80211 drivers, so could not be fast.

Stanislaw

Bjørn Mork

2012-12-07 11:30 UTC #6 lore raw

Quoted 7 lines · Stanislaw Gruszka

Quoted 3 lines · Ben Greear
I don't have time to work on this today...maybe not tomorrow either.
So, if someone else feels like fixing it, please feel free.
Ok, I'll try to fix that. However this could require modification of all cfg80211 drivers, so could not be fast.

I wonder if that strategy isn't just adding unnecessary complexity? How about using something like the (completely untested) patch below instead?

Bjørn

Quoted 1 line

From 6b6ce488f64d5530e1886072f64b034d5a65e5f7 Mon Sep 17 00:00:00 2001

From: =?UTF-8?q?Bj=C3=B8rn=20Mork?= <[email protected]>
Date: Fri, 7 Dec 2012 12:14:02 +0100
Subject: [RFC] net: fix ethtool_ops for wireless
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

commit 2c60db03 ("net: provide a default dev->ethtool_ops") made tests for unset ethtool_ops always fail. This caused a regression for wireless drivers relying on the wireless specific ethtool_ops default.

Adding a new test for code which need to know whether ethtool_ops was changed after netdev allocation, and update the wireless core to use it.

Signed-off-by Bjørn Mork <[email protected]>

---
 include/linux/netdevice.h |   14 ++++++++++++++
 net/core/dev.c            |    1 +
 net/wireless/core.c       |    2 +-
 3 files changed, 16 insertions(+), 1 deletion(-)

include/linux/netdevice.h +14 -0

index 18c5dc9..95741f1 100644
@@ -2707,6 +2707,20 @@ static inline bool netif_supports_nofcs(struct net_device *dev)
 
 extern struct pernet_operations __net_initdata loopback_net_ops;
 
+extern struct ethtool_ops default_ethtool_ops;
+
+/**
+ *	netdev_has_default_ethtool_ops - test for default ethtool_ops
+ *	@dev: device
+ *
+ *	This function returns true if dev is using the generic default
+ *	ethtool_ops
+ */
+static inline bool netdev_has_default_ethtool_ops(struct net_device *dev)
+{
+	return dev->ethtool_ops && (dev->ethtool_ops == &default_ethtool_ops);
+}
+
 /* Logging, debugging and troubleshooting/diagnostic helpers. */
 
 /* netdev_printk helpers, similar to dev_printk */

net/core/dev.c +1 -0

index 0aea3fe..0b20be5 100644
@@ -6107,6 +6107,7 @@ struct netdev_queue *dev_ingress_queue_create(struct net_device *dev)
 }
 
 static const struct ethtool_ops default_ethtool_ops;
+EXPORT_SYMBOL_GPL(default_ethtool_ops);
 
 /**
  *	alloc_netdev_mqs - allocate network device

net/wireless/core.c +1 -1

index 14d9904..95467d0 100644
@@ -866,7 +866,7 @@ static int cfg80211_netdev_notifier_call(struct notifier_block *nb,
 		/* allow mac80211 to determine the timeout */
 		wdev->ps_timeout = -1;
 
-		if (!dev->ethtool_ops)
+		if (netdev_has_default_ethtool_ops(dev)
 			dev->ethtool_ops = &cfg80211_ethtool_ops;
 
 		if ((wdev->iftype == NL80211_IFTYPE_STATION ||

Signature · 1 line

1.7.10.4

Stanislaw Gruszka

2012-12-07 11:40 UTC #7 lore raw

Quoted 11 lines · Bjørn Mork

Quoted 7 lines · Stanislaw Gruszka
Quoted 3 lines · Ben Greear
I don't have time to work on this today...maybe not tomorrow either.
So, if someone else feels like fixing it, please feel free.
Ok, I'll try to fix that. However this could require modification of all cfg80211 drivers, so could not be fast.
I wonder if that strategy isn't just adding unnecessary complexity? How about using something like the (completely untested) patch below instead?

Yeah, I wrote very similar patch and I'm testing it locally. Plan to post RFC to net folks ...

Stanislaw

Stanislaw Gruszka

2012-12-07 12:16 UTC #8 lore raw

RFC 1/1 wireless: check against default_ethtool_ops

In reply to Bjørn Mork

Since:

commit 2c60db037034d27f8c636403355d52872da92f81

Author: Eric Dumazet <[email protected]>
Date:   Sun Sep 16 09:17:26 2012 +0000

    net: provide a default dev->ethtool_ops

wireless core does not correctly assign ethtool_ops. In order to fix the problem, and avoid assigning ethtool_ops on each individual cfg80211 driver, we check against default_ethool_ops pointer instead of NULL in wireless core.

Signed-off-by Stanislaw Gruszka <[email protected]>

---
 include/linux/netdevice.h |    2 ++
 net/core/dev.c            |    3 ++-
 net/wireless/core.c       |    2 +-
 3 files changed, 5 insertions(+), 2 deletions(-)

include/linux/netdevice.h +2 -0

index f8eda02..c98e1c3 100644
@@ -60,6 +60,8 @@ struct wireless_dev;
 #define SET_ETHTOOL_OPS(netdev,ops) \
 	( (netdev)->ethtool_ops = (ops) )
 
+extern const struct ethtool_ops default_ethtool_ops;
+
 /* hardware address assignment types */
 #define NET_ADDR_PERM		0	/* address is permanent (default) */
 #define NET_ADDR_RANDOM		1	/* address is generated randomly */

net/core/dev.c +2 -1

index c0946cb..4cd2168 100644
@@ -6008,7 +6008,8 @@ struct netdev_queue *dev_ingress_queue_create(struct net_device *dev)
 	return queue;
 }
 
-static const struct ethtool_ops default_ethtool_ops;
+const struct ethtool_ops default_ethtool_ops;
+EXPORT_SYMBOL_GPL(default_ethtool_ops);
 
 /**
  *	alloc_netdev_mqs - allocate network device

net/wireless/core.c +1 -1

index 14d9904..90915d4 100644
@@ -866,7 +866,7 @@ static int cfg80211_netdev_notifier_call(struct notifier_block *nb,
 		/* allow mac80211 to determine the timeout */
 		wdev->ps_timeout = -1;
 
-		if (!dev->ethtool_ops)
+		if (dev->ethtool_ops == &default_ethtool_ops)
 			dev->ethtool_ops = &cfg80211_ethtool_ops;
 
 		if ((wdev->iftype == NL80211_IFTYPE_STATION ||

Signature · 1 line

1.7.1

Ben Hutchings

Acked-by on v1 1/1

2012-12-07 16:27 UTC #9 lore raw

Re: [RFC] wireless: check against default_ethtool_ops

Quoted 6 lines · Stanislaw Gruszka

Since:
commit 2c60db037034d27f8c636403355d52872da92f81
Author: Eric Dumazet <[email protected]>
Date:   Sun Sep 16 09:17:26 2012 +0000

Quoted 8 lines

    net: provide a default dev->ethtool_ops
wireless core does not correctly assign ethtool_ops. In order to fix the problem, and avoid assigning ethtool_ops on each individual cfg80211 driver, we check against default_ethool_ops pointer instead of NULL in wireless core.
Signed-off-by Stanislaw Gruszka <[email protected]>

[...]

Acked-by Ben Hutchings <[email protected]>

Ideally you could do this assignment unconditionally in the setup function for the device, but it doesn't seem like there's a common allocation path that you could do that in.

Ben.

Signature · 3 lines

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.

Vladimir Kondratiev

2012-12-10 06:39 UTC #10 lore raw

In reply to Stanislaw Gruszka

Quoted 2 lines · Stanislaw Gruszka

We should probably assing ethtool_ops before wireless core will call alloc_netdev.

Another option (I verified - it works) - set in the driver

ndev->ethtool_ops = NULL;

before register_netdev(ndev);