diff mbox

wireless: Fix ethtool stats and other ops.

Message ID 1354729192-22945-1-git-send-email-greearb@candelatech.com (mailing list archive)
State Not Applicable, archived
Headers show

Commit Message

Ben Greear Dec. 5, 2012, 5:39 p.m. UTC 
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(-)

Comments

Bjørn Mork Dec. 5, 2012, 5:57 p.m. UTC | #1 
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
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ben Greear Dec. 5, 2012, 6:02 p.m. UTC | #2 
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
>
Stanislaw Gruszka Dec. 6, 2012, 12:25 p.m. UTC | #3 
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
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Vladimir Kondratiev Dec. 6, 2012, 3:22 p.m. UTC | #4 
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);



--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ben Greear Dec. 6, 2012, 8:15 p.m. UTC | #5 
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
Stanislaw Gruszka Dec. 7, 2012, 9:50 a.m. UTC | #6 
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
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

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