[PATCH] wireless: Fix ethtool stats and other ops.

[PATCH] wireless: Fix ethtool stats and other ops.

[greearb]

From: Ben Greear <greearb@candelatech.com>

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 <greearb@candelatech.com>
---
 net/wireless/core.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/net/wireless/core.c b/net/wireless/core.c
index 4e6fe62..6309699 100644
--- a/net/wireless/core.c
+++ b/net/wireless/core.c
@@ -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 ||
-- 
1.7.3.4

Re: [PATCH] wireless: Fix ethtool stats and other ops.

[Bjørn Mork]

Ben Greear <greearb@candelatech.com> writes:

> 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 <greearb@candelatech.com>
> ---
>  net/wireless/core.c |    3 +--
>  1 files changed, 1 insertions(+), 2 deletions(-)
>
> diff --git a/net/wireless/core.c b/net/wireless/core.c
> index 4e6fe62..6309699 100644
> --- a/net/wireless/core.c
> +++ b/net/wireless/core.c
> @@ -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 ||


Won't this break drivers which for some reason have their own
ethtool_ops?

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

Re: [PATCH] wireless: Fix ethtool stats and other ops.

[Ben Greear]

On 12/05/2012 09:57 AM, Bjørn Mork wrote:
> Ben Greear <greearb@candelatech.com> writes:
>
>> 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 <greearb@candelatech.com>
>> ---
>>   net/wireless/core.c |    3 +--
>>   1 files changed, 1 insertions(+), 2 deletions(-)
>>
>> diff --git a/net/wireless/core.c b/net/wireless/core.c
>> index 4e6fe62..6309699 100644
>> --- a/net/wireless/core.c
>> +++ b/net/wireless/core.c
>> @@ -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 ||
>
>
> 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

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


-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: [PATCH] wireless: Fix ethtool stats and other ops.

[Stanislaw Gruszka]

On Wed, Dec 05, 2012 at 10:02:00AM -0800, Ben Greear wrote:
> On 12/05/2012 09:57 AM, Bjørn Mork wrote:
> >Ben Greear <greearb@candelatech.com> writes:
> >
> >>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 <greearb@candelatech.com>
> >>---
> >>  net/wireless/core.c |    3 +--
> >>  1 files changed, 1 insertions(+), 2 deletions(-)
> >>
> >>diff --git a/net/wireless/core.c b/net/wireless/core.c
> >>index 4e6fe62..6309699 100644
> >>--- a/net/wireless/core.c
> >>+++ b/net/wireless/core.c
> >>@@ -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 ||
> >
> >
> >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)?

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

Stanislaw

Re: [PATCH] wireless: Fix ethtool stats and other ops.

[Vladimir Kondratiev]

Stanislaw Gruszka <sgruszka@...> writes:

> 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);

Re: [PATCH] wireless: Fix ethtool stats and other ops.

[Ben Greear]

On 12/06/2012 04:25 AM, Stanislaw Gruszka wrote:

>>> 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)?
>
> 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

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: [PATCH] wireless: Fix ethtool stats and other ops.

[Stanislaw Gruszka]

On Thu, Dec 06, 2012 at 12:15:28PM -0800, Ben Greear wrote:
> 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

Re: [PATCH] wireless: Fix ethtool stats and other ops.

[Bjørn Mork]

[-- Attachment #1: Type: text/plain, Size: 541 bytes --]

Stanislaw Gruszka <sgruszka@redhat.com> writes:
> On Thu, Dec 06, 2012 at 12:15:28PM -0800, Ben Greear wrote:
>> 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


[-- Attachment #2: 0001-net-fix-ethtool_ops-for-wireless.patch --]
[-- Type: text/x-diff, Size: 2620 bytes --]

>From 6b6ce488f64d5530e1886072f64b034d5a65e5f7 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Bj=C3=B8rn=20Mork?= <bjorn@mork.no>
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 <bjorn@mork.no>
---
 include/linux/netdevice.h |   14 ++++++++++++++
 net/core/dev.c            |    1 +
 net/wireless/core.c       |    2 +-
 3 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 18c5dc9..95741f1 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -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 */
diff --git a/net/core/dev.c b/net/core/dev.c
index 0aea3fe..0b20be5 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -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
diff --git a/net/wireless/core.c b/net/wireless/core.c
index 14d9904..95467d0 100644
--- a/net/wireless/core.c
+++ b/net/wireless/core.c
@@ -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 ||
-- 
1.7.10.4

Re: [PATCH] wireless: Fix ethtool stats and other ops.

[Stanislaw Gruszka]

On Fri, Dec 07, 2012 at 12:30:12PM +0100, Bjørn Mork wrote:
> Stanislaw Gruszka <sgruszka@redhat.com> writes:
> > On Thu, Dec 06, 2012 at 12:15:28PM -0800, Ben Greear wrote:
> >> 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

[RFC] wireless: check against default_ethtool_ops

[Stanislaw Gruszka]

Since:

commit 2c60db037034d27f8c636403355d52872da92f81
Author: Eric Dumazet <edumazet@google.com>
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 <sgruszka@redhat.com>
---
 include/linux/netdevice.h |    2 ++
 net/core/dev.c            |    3 ++-
 net/wireless/core.c       |    2 +-
 3 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index f8eda02..c98e1c3 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -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 */
diff --git a/net/core/dev.c b/net/core/dev.c
index c0946cb..4cd2168 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -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
diff --git a/net/wireless/core.c b/net/wireless/core.c
index 14d9904..90915d4 100644
--- a/net/wireless/core.c
+++ b/net/wireless/core.c
@@ -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 ||
-- 
1.7.1

Re: [RFC] wireless: check against default_ethtool_ops

[Ben Hutchings]

On Fri, 2012-12-07 at 13:16 +0100, Stanislaw Gruszka wrote:
> Since:
> 
> commit 2c60db037034d27f8c636403355d52872da92f81
> Author: Eric Dumazet <edumazet@google.com>
> 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 <sgruszka@redhat.com>
[...]
Acked-by: Ben Hutchings <bhutchings@solarflare.com>

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.

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