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(-)
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
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
On Fri, 2012-12-07 at 13:16 +0100, Stanislaw Gruszka wrote:
> 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]>
[...]
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.
--
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.
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 <[email protected]> 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 <[email protected]>
> >>---
> >> 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
On Fri, Dec 07, 2012 at 12:30:12PM +0100, Bj?rn Mork wrote:
> Stanislaw Gruszka <[email protected]> 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
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(-)
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
Ben Greear <[email protected]> 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 <[email protected]>
> ---
> 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
On 12/05/2012 09:57 AM, Bjørn Mork wrote:
> Ben Greear <[email protected]> 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 <[email protected]>
>> ---
>> 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 <[email protected]>
Candela Technologies Inc http://www.candelatech.com
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 <[email protected]>
Candela Technologies Inc http://www.candelatech.com
Stanislaw Gruszka <[email protected]> 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
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);