* [PATCH] wireless: Fix ethtool stats and other ops. @ 2012-12-05 17:39 greearb 2012-12-05 17:57 ` Bjørn Mork 0 siblings, 1 reply; 11+ messages in thread From: greearb @ 2012-12-05 17:39 UTC (permalink / raw) To: linux-wireless; +Cc: Ben Greear 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 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH] wireless: Fix ethtool stats and other ops. 2012-12-05 17:39 [PATCH] wireless: Fix ethtool stats and other ops greearb @ 2012-12-05 17:57 ` Bjørn Mork 2012-12-05 18:02 ` Ben Greear 0 siblings, 1 reply; 11+ messages in thread From: Bjørn Mork @ 2012-12-05 17:57 UTC (permalink / raw) To: Ben Greear; +Cc: linux-wireless 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] wireless: Fix ethtool stats and other ops. 2012-12-05 17:57 ` Bjørn Mork @ 2012-12-05 18:02 ` Ben Greear 2012-12-06 12:25 ` Stanislaw Gruszka 0 siblings, 1 reply; 11+ messages in thread From: Ben Greear @ 2012-12-05 18:02 UTC (permalink / raw) To: Bjørn Mork; +Cc: linux-wireless 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] wireless: Fix ethtool stats and other ops. 2012-12-05 18:02 ` Ben Greear @ 2012-12-06 12:25 ` Stanislaw Gruszka 2012-12-06 15:22 ` Vladimir Kondratiev 2012-12-06 20:15 ` Ben Greear 0 siblings, 2 replies; 11+ messages in thread From: Stanislaw Gruszka @ 2012-12-06 12:25 UTC (permalink / raw) To: Ben Greear; +Cc: Bjørn Mork, linux-wireless 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] wireless: Fix ethtool stats and other ops. 2012-12-06 12:25 ` Stanislaw Gruszka @ 2012-12-06 15:22 ` Vladimir Kondratiev 2012-12-06 20:15 ` Ben Greear 1 sibling, 0 replies; 11+ messages in thread From: Vladimir Kondratiev @ 2012-12-06 15:22 UTC (permalink / raw) To: linux-wireless 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); ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] wireless: Fix ethtool stats and other ops. 2012-12-06 12:25 ` Stanislaw Gruszka 2012-12-06 15:22 ` Vladimir Kondratiev @ 2012-12-06 20:15 ` Ben Greear 2012-12-07 9:50 ` Stanislaw Gruszka 1 sibling, 1 reply; 11+ messages in thread From: Ben Greear @ 2012-12-06 20:15 UTC (permalink / raw) To: Stanislaw Gruszka; +Cc: Bjørn Mork, linux-wireless 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] wireless: Fix ethtool stats and other ops. 2012-12-06 20:15 ` Ben Greear @ 2012-12-07 9:50 ` Stanislaw Gruszka 2012-12-07 11:30 ` Bjørn Mork 0 siblings, 1 reply; 11+ messages in thread From: Stanislaw Gruszka @ 2012-12-07 9:50 UTC (permalink / raw) To: Ben Greear; +Cc: Bjørn Mork, linux-wireless 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] wireless: Fix ethtool stats and other ops. 2012-12-07 9:50 ` Stanislaw Gruszka @ 2012-12-07 11:30 ` Bjørn Mork 2012-12-07 11:40 ` Stanislaw Gruszka 2012-12-07 12:16 ` [RFC] wireless: check against default_ethtool_ops Stanislaw Gruszka 0 siblings, 2 replies; 11+ messages in thread From: Bjørn Mork @ 2012-12-07 11:30 UTC (permalink / raw) To: Stanislaw Gruszka; +Cc: Ben Greear, linux-wireless [-- 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 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH] wireless: Fix ethtool stats and other ops. 2012-12-07 11:30 ` Bjørn Mork @ 2012-12-07 11:40 ` Stanislaw Gruszka 2012-12-07 12:16 ` [RFC] wireless: check against default_ethtool_ops Stanislaw Gruszka 1 sibling, 0 replies; 11+ messages in thread From: Stanislaw Gruszka @ 2012-12-07 11:40 UTC (permalink / raw) To: Bjørn Mork; +Cc: Ben Greear, linux-wireless 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* [RFC] wireless: check against default_ethtool_ops 2012-12-07 11:30 ` Bjørn Mork 2012-12-07 11:40 ` Stanislaw Gruszka @ 2012-12-07 12:16 ` Stanislaw Gruszka 2012-12-07 16:27 ` Ben Hutchings 1 sibling, 1 reply; 11+ messages in thread From: Stanislaw Gruszka @ 2012-12-07 12:16 UTC (permalink / raw) To: Eric Dumazet, netdev; +Cc: Ben Greear, linux-wireless, Bjørn Mork 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 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [RFC] wireless: check against default_ethtool_ops 2012-12-07 12:16 ` [RFC] wireless: check against default_ethtool_ops Stanislaw Gruszka @ 2012-12-07 16:27 ` Ben Hutchings 0 siblings, 0 replies; 11+ messages in thread From: Ben Hutchings @ 2012-12-07 16:27 UTC (permalink / raw) To: Stanislaw Gruszka Cc: Eric Dumazet, netdev, Ben Greear, linux-wireless, Bjørn Mork 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. ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2012-12-10 6:39 UTC | newest] Thread overview: 11+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2012-12-05 17:39 [PATCH] wireless: Fix ethtool stats and other ops greearb 2012-12-05 17:57 ` Bjørn Mork 2012-12-05 18:02 ` Ben Greear 2012-12-06 12:25 ` Stanislaw Gruszka 2012-12-06 15:22 ` Vladimir Kondratiev 2012-12-06 20:15 ` Ben Greear 2012-12-07 9:50 ` Stanislaw Gruszka 2012-12-07 11:30 ` Bjørn Mork 2012-12-07 11:40 ` Stanislaw Gruszka 2012-12-07 12:16 ` [RFC] wireless: check against default_ethtool_ops Stanislaw Gruszka 2012-12-07 16:27 ` Ben Hutchings
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).