[PATCH] libertas: Fix ethtool statistics

[PATCH] libertas: Fix ethtool statistics

[David Woodhouse]

We removed CMD_MESH_ACCESS from the big case statements and provided
lbs_mesh_access() instead.... but we missed one user. Fix it.

Signed-off-by: David Woodhouse <dwmw2@infradead.org>

diff --git a/drivers/net/wireless/libertas/ethtool.c b/drivers/net/wireless/libertas/ethtool.c
index 21e6f98..9460300 100644
--- a/drivers/net/wireless/libertas/ethtool.c
+++ b/drivers/net/wireless/libertas/ethtool.c
@@ -106,8 +106,8 @@ done:
         return ret;
 }
 
-static void lbs_ethtool_get_stats(struct net_device * dev,
-				struct ethtool_stats * stats, u64 * data)
+static void lbs_ethtool_get_stats(struct net_device *dev,
+				  struct ethtool_stats *stats, u64 *data)
 {
 	struct lbs_private *priv = dev->priv;
 	struct cmd_ds_mesh_access mesh_access;
@@ -116,9 +116,7 @@ static void lbs_ethtool_get_stats(struct net_device * dev,
 	lbs_deb_enter(LBS_DEB_ETHTOOL);
 
 	/* Get Mesh Statistics */
-	ret = lbs_prepare_and_send_command(priv,
-			CMD_MESH_ACCESS, CMD_ACT_MESH_GET_STATS,
-			CMD_OPTION_WAITFORRSP, 0, &mesh_access);
+	ret = lbs_mesh_access(priv, CMD_ACT_MESH_GET_STATS, &mesh_access);
 
 	if (ret)
 		return;

-- 
dwmw2

Re: [PATCH] libertas: Fix ethtool statistics

[Dan Williams]

On Thu, 2008-04-17 at 17:13 +0100, David Woodhouse wrote:
> We removed CMD_MESH_ACCESS from the big case statements and provided
> lbs_mesh_access() instead.... but we missed one user. Fix it.
> 
> Signed-off-by: David Woodhouse <dwmw2@infradead.org>

Acked-by: Dan Williams <dcbw@redhat.com>

> 
> diff --git a/drivers/net/wireless/libertas/ethtool.c b/drivers/net/wireless/libertas/ethtool.c
> index 21e6f98..9460300 100644
> --- a/drivers/net/wireless/libertas/ethtool.c
> +++ b/drivers/net/wireless/libertas/ethtool.c
> @@ -106,8 +106,8 @@ done:
>          return ret;
>  }
>  
> -static void lbs_ethtool_get_stats(struct net_device * dev,
> -				struct ethtool_stats * stats, u64 * data)
> +static void lbs_ethtool_get_stats(struct net_device *dev,
> +				  struct ethtool_stats *stats, u64 *data)
>  {
>  	struct lbs_private *priv = dev->priv;
>  	struct cmd_ds_mesh_access mesh_access;
> @@ -116,9 +116,7 @@ static void lbs_ethtool_get_stats(struct net_device * dev,
>  	lbs_deb_enter(LBS_DEB_ETHTOOL);
>  
>  	/* Get Mesh Statistics */
> -	ret = lbs_prepare_and_send_command(priv,
> -			CMD_MESH_ACCESS, CMD_ACT_MESH_GET_STATS,
> -			CMD_OPTION_WAITFORRSP, 0, &mesh_access);
> +	ret = lbs_mesh_access(priv, CMD_ACT_MESH_GET_STATS, &mesh_access);
>  
>  	if (ret)
>  		return;
> 

Re: [PATCH] libertas: Fix ethtool statistics

[Sebastian Siewior]

* Dan Williams | 2008-04-17 14:02:43 [-0400]:

>On Thu, 2008-04-17 at 17:13 +0100, David Woodhouse wrote:
>> We removed CMD_MESH_ACCESS from the big case statements and provided
>> lbs_mesh_access() instead.... but we missed one user. Fix it.
>> 
>> Signed-off-by: David Woodhouse <dwmw2@infradead.org>
>
>Acked-by: Dan Williams <dcbw@redhat.com>
Hmmm. This patch actually reminds me of something I was going to do. I
don't really see the big difference between this and [1]. I have to
exert myself to get on your auto-ack list...


[1]
http://lists.infradead.org/pipermail/libertas-dev/2008-March/001281.html

>> 
>> diff --git a/drivers/net/wireless/libertas/ethtool.c b/drivers/net/wireless/libertas/ethtool.c
>> index 21e6f98..9460300 100644
>> --- a/drivers/net/wireless/libertas/ethtool.c
>> +++ b/drivers/net/wireless/libertas/ethtool.c
>> @@ -106,8 +106,8 @@ done:
>>          return ret;
>>  }
>>  
>> -static void lbs_ethtool_get_stats(struct net_device * dev,
>> -				struct ethtool_stats * stats, u64 * data)
>> +static void lbs_ethtool_get_stats(struct net_device *dev,
>> +				  struct ethtool_stats *stats, u64 *data)
>>  {
>>  	struct lbs_private *priv = dev->priv;
>>  	struct cmd_ds_mesh_access mesh_access;
>> @@ -116,9 +116,7 @@ static void lbs_ethtool_get_stats(struct net_device * dev,
>>  	lbs_deb_enter(LBS_DEB_ETHTOOL);
>>  
>>  	/* Get Mesh Statistics */
>> -	ret = lbs_prepare_and_send_command(priv,
>> -			CMD_MESH_ACCESS, CMD_ACT_MESH_GET_STATS,
>> -			CMD_OPTION_WAITFORRSP, 0, &mesh_access);
>> +	ret = lbs_mesh_access(priv, CMD_ACT_MESH_GET_STATS, &mesh_access);
>>  
>>  	if (ret)
>>  		return;
>> 

Sebastian

Re: [PATCH] libertas: Fix ethtool statistics

[David Woodhouse]

On Thu, 2008-04-17 at 21:44 +0200, Sebastian Siewior wrote:
> * Dan Williams | 2008-04-17 14:02:43 [-0400]:
> 
> >On Thu, 2008-04-17 at 17:13 +0100, David Woodhouse wrote:
> >> We removed CMD_MESH_ACCESS from the big case statements and provided
> >> lbs_mesh_access() instead.... but we missed one user. Fix it.
> >> 
> >> Signed-off-by: David Woodhouse <dwmw2@infradead.org>
> >
> >Acked-by: Dan Williams <dcbw@redhat.com>
> Hmmm. This patch actually reminds me of something I was going to do. I
> don't really see the big difference between this and [1]. I have to
> exert myself to get on your auto-ack list...

Yes, your patch is better than mine because it also zeroes the stats in
the error case. Although we should avoid that error, really.

-- 
dwmw2

[PATCH] [NET] libertas: check for mesh support before trying to use it

[Sebastian Siewior]

From: Sebastian Siewior <sebastian@breakpoint.cc>

This will prevent the execution of lbs_ethtool_get_stats() via ethtool -S
if there is no mesh support.

Signed-off-by: Sebastian Siewior <sebastian@breakpoint.cc>
---
Dan, this will test prevent the usage of ethtool -S in case there is no
mesh support. If you fine with this than we could apply my or David's
other patch to enable the function again and then worry about the error
path.
David: I saw your patch that is doing this in a other way but I thing
that one looks better :)

 drivers/net/wireless/libertas/ethtool.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/drivers/net/wireless/libertas/ethtool.c b/drivers/net/wireless/libertas/ethtool.c
index dcfdb40..a79d698 100644
--- a/drivers/net/wireless/libertas/ethtool.c
+++ b/drivers/net/wireless/libertas/ethtool.c
@@ -113,8 +113,14 @@ static void lbs_ethtool_get_stats(struct net_device * dev,
 
 static int lbs_ethtool_get_sset_count(struct net_device * dev, int sset)
 {
+	struct lbs_private *priv = dev->priv;
+
 	switch (sset) {
 	case ETH_SS_STATS:
+
+		if (!priv->mesh_tlv)
+			return -EOPNOTSUPP;
+
 		return MESH_STATS_NUM;
 	default:
 		return -EOPNOTSUPP;
-- 
1.5.4.3

Re: [PATCH] [NET] libertas: check for mesh support before trying to use it

[Dan Williams]

On Sat, 2008-05-10 at 09:24 +0200, Sebastian Siewior wrote:
> From: Sebastian Siewior <sebastian@breakpoint.cc>
> 
> This will prevent the execution of lbs_ethtool_get_stats() via ethtool -S
> if there is no mesh support.
> 
> Signed-off-by: Sebastian Siewior <sebastian@breakpoint.cc>
> ---
> Dan, this will test prevent the usage of ethtool -S in case there is no
> mesh support. If you fine with this than we could apply my or David's
> other patch to enable the function again and then worry about the error
> path.
> David: I saw your patch that is doing this in a other way but I thing
> that one looks better :)
> 
>  drivers/net/wireless/libertas/ethtool.c |    6 ++++++
>  1 files changed, 6 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/net/wireless/libertas/ethtool.c b/drivers/net/wireless/libertas/ethtool.c
> index dcfdb40..a79d698 100644
> --- a/drivers/net/wireless/libertas/ethtool.c
> +++ b/drivers/net/wireless/libertas/ethtool.c
> @@ -113,8 +113,14 @@ static void lbs_ethtool_get_stats(struct net_device * dev,
>  
>  static int lbs_ethtool_get_sset_count(struct net_device * dev, int sset)
>  {
> +	struct lbs_private *priv = dev->priv;
> +
>  	switch (sset) {
>  	case ETH_SS_STATS:
> +
> +		if (!priv->mesh_tlv)
> +

We usually use

if (!priv->mesh_dev)

for checking whether mesh is open or not... Will that not work here?

Dan

> 			return -EOPNOTSUPP;
> +
>  		return MESH_STATS_NUM;
>  	default:
>  		return -EOPNOTSUPP;

Re: [PATCH] [NET] libertas: check for mesh support before trying to use it

[Sebastian Siewior]

* Dan Williams | 2008-05-12 12:34:16 [-0400]:

>> diff --git a/drivers/net/wireless/libertas/ethtool.c b/drivers/net/wireless/libertas/ethtool.c
>> index dcfdb40..a79d698 100644
>> --- a/drivers/net/wireless/libertas/ethtool.c
>> +++ b/drivers/net/wireless/libertas/ethtool.c
>> @@ -113,8 +113,14 @@ static void lbs_ethtool_get_stats(struct net_device * dev,
>>  
>>  static int lbs_ethtool_get_sset_count(struct net_device * dev, int sset)
>>  {
>> +	struct lbs_private *priv = dev->priv;
>> +
>>  	switch (sset) {
>>  	case ETH_SS_STATS:
>> +
>> +		if (!priv->mesh_tlv)
>> +
>
>We usually use
>
>if (!priv->mesh_dev)
>
>for checking whether mesh is open or not... Will that not work here?
>
Well, if this is the case than you can pick David's patch which does
this [1]. If you fine with this than I could add a patch description and
things like that :)

[1] http://article.gmane.org/gmane.linux.kernel.wireless.general/13707
>Dan
Sebastian

Re: [PATCH] [NET] libertas: check for mesh support before trying to use it

[John W. Linville]

On Mon, May 12, 2008 at 10:51:36PM +0200, Sebastian Siewior wrote:
> * Dan Williams | 2008-05-12 12:34:16 [-0400]:
> 
> >> diff --git a/drivers/net/wireless/libertas/ethtool.c b/drivers/net/wireless/libertas/ethtool.c
> >> index dcfdb40..a79d698 100644
> >> --- a/drivers/net/wireless/libertas/ethtool.c
> >> +++ b/drivers/net/wireless/libertas/ethtool.c
> >> @@ -113,8 +113,14 @@ static void lbs_ethtool_get_stats(struct net_device * dev,
> >>  
> >>  static int lbs_ethtool_get_sset_count(struct net_device * dev, int sset)
> >>  {
> >> +	struct lbs_private *priv = dev->priv;
> >> +
> >>  	switch (sset) {
> >>  	case ETH_SS_STATS:
> >> +
> >> +		if (!priv->mesh_tlv)
> >> +
> >
> >We usually use
> >
> >if (!priv->mesh_dev)
> >
> >for checking whether mesh is open or not... Will that not work here?
> >
> Well, if this is the case than you can pick David's patch which does
> this [1]. If you fine with this than I could add a patch description and
> things like that :)
> 
> [1] http://article.gmane.org/gmane.linux.kernel.wireless.general/13707

These competing patches have been under discussion for a month.
Can we get some consensus on which patch or patches we want?

John
-- 
John W. Linville
linville@tuxdriver.com

Re: [PATCH] libertas: Fix ethtool statistics

[Holger Schurig]

NACK.

When I apply this patch and do "ethtool -S eth1", I get this:

$ ethtool -S eth1
NIC statistics:
=A0 =A0 =A0drop_duplicate_bcast: 7
=A0 =A0 =A0drop_ttl_zero: 0
=A0 =A0 =A0drop_no_fwd_route: 0
=A0 =A0 =A0drop_no_buffers: 9223372036854775808
=A0 =A0 =A0fwded_unicast_cnt: 0
=A0 =A0 =A0fwded_bcast_cnt: 0
=A0 =A0 =A0drop_blind_table: 0
=A0 =A0 =A0tx_failed_cnt: 9223372036854775808

But more severely, I get this in dmesg:

libertas enter: lbs_ethtool_get_stats()
libertas: PREP_CMD: command 0x009b failed: 2
=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=
 =A0 =A0 =A0 =A0 =A0
My firmware simply doesn't support the 009b command. It seems=20
also wrong to call an MESH related command on an struct=20
net_device *dev which is not priv->mesh_dev.


That ethtool returns junk like "9223372036854775808" is another=20
error. lbs_mesh_access() returned 2, and the code does "if (ret)=20
return ret". Maybe it should be like "if (ret) return -ENOSYS=20
(or some other ENOxxx) instead?
--
To unsubscribe from this list: send the line "unsubscribe linux-wireles=
s" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] libertas: Fix ethtool statistics

[David Woodhouse]

On Fri, 2008-04-18 at 11:02 +0200, Holger Schurig wrote:
> NACK.

Thanks for testing.

> When I apply this patch and do "ethtool -S eth1", I get this:
> 
> $ ethtool -S eth1
> NIC statistics:
>      drop_duplicate_bcast: 7
>      drop_ttl_zero: 0
>      drop_no_fwd_route: 0
>      drop_no_buffers: 9223372036854775808
>      fwded_unicast_cnt: 0
>      fwded_bcast_cnt: 0
>      drop_blind_table: 0
>      tx_failed_cnt: 9223372036854775808

You were getting that anyway.

> But more severely, I get this in dmesg:
> 
> libertas enter: lbs_ethtool_get_stats()
> libertas: PREP_CMD: command 0x009b failed: 2

Now at least it's _trying_ to get the stats :)
                
> My firmware simply doesn't support the 009b command. It seems 
> also wrong to call an MESH related command on an struct 
> net_device *dev which is not priv->mesh_dev.
> 
> 
> That ethtool returns junk like "9223372036854775808" is another 
> error. lbs_mesh_access() returned 2, and the code does "if (ret) 
> return ret". Maybe it should be like "if (ret) return -ENOSYS 
> (or some other ENOxxx) instead?

The code does 'if (ret) return;', because it has no option to return an
error. I think the way round it would be something like this:

diff --git a/drivers/net/wireless/libertas/ethtool.c b/drivers/net/wireless/libertas/ethtool.c
index 21e6f98..2ca3f4f 100644
--- a/drivers/net/wireless/libertas/ethtool.c
+++ b/drivers/net/wireless/libertas/ethtool.c
@@ -146,12 +144,12 @@ static void lbs_ethtool_get_stats(struct net_device * dev,
 
 static int lbs_ethtool_get_sset_count(struct net_device * dev, int sset)
 {
-	switch (sset) {
-	case ETH_SS_STATS:
+	struct lbs_private *priv = dev->priv;
+
+	if (sset == ETH_SS_STATS && dev == priv->mesh_dev)
 		return MESH_STATS_NUM;
-	default:
+	else
 		return -EOPNOTSUPP;
-	}
 }
 
 static void lbs_ethtool_get_strings(struct net_device *dev,

-- 
dwmw2

Re: [PATCH] libertas: Fix ethtool statistics

[Holger Schurig]

> Now at least it's _trying_ to get the stats :)

Yeah, but it shouldn't. You can test the mesh capability of the 
hardware (explizitly or implicitly via priv->mesh_dev != NULL) 
and you should only issue mesh commands if the hardware can do 
it.



> You were getting that anyway.

Right :-)

> > That ethtool returns junk like "9223372036854775808" is
> > another error. lbs_mesh_access() returned 2, and the code
> > does "if (ret) return ret". Maybe it should be like "if
> > (ret) return -ENOSYS (or some other ENOxxx) instead?
>
> The code does 'if (ret) return;', because it has no option to
> return an error.

I wrote "if (ret) return ret", not "if (ret) return;".

Anyway I think that returning something positive in the error 
case here seems wrong. However, I'm only thinking this, I 
haven't checked the ethtool interface/documentation of the 
kernel.



I applied your second patch on top of the first one and now this 
happens:

$ ethtool -S eth1
ethtool -S eth1

Which seems much better. The ultimate patch would be to provide 
the result of CMD_802_GET_LOG for ethX and the result from 
CMD_ACT_MESH_GET_STATS for mshX devices.

So, if you combine both patches into one, I'd ACK from 
the "firmware without mesh capabilities" perspective :-)

Re: [PATCH] libertas: Fix ethtool statistics

[David Woodhouse]

On Fri, 2008-04-18 at 11:49 +0200, Holger Schurig wrote:
> > The code does 'if (ret) return;', because it has no option to
> > return an error.
> 
> I wrote "if (ret) return ret", not "if (ret) return;".

I know you did, but I don't know why. The lbs_ethtool_get_stats()
function returns void. Sebastian's patch which memsets it to zero makes
sense, but it can't return an error.

> I applied your second patch on top of the first one and now this 
> happens:
> 
> $ ethtool -S eth1
> ethtool -S eth1
> 
> Which seems much better. The ultimate patch would be to provide 
> the result of CMD_802_GET_LOG for ethX and the result from 
> CMD_ACT_MESH_GET_STATS for mshX devices.
> 
> So, if you combine both patches into one, I'd ACK from 
> the "firmware without mesh capabilities" perspective :-)

They're separate problems, really. With or without the patch I first
posted, you're getting crap back when you ask for statistics on a
non-mesh device. The second patch fixes that, and stands alone.

-- 
dwmw2

Re: [PATCH] libertas: Fix ethtool statistics

[Holger Schurig]

> > I wrote "if (ret) return ret", not "if (ret) return;".
>
> I know you did, but I don't know why. The
> lbs_ethtool_get_stats() function returns void. Sebastian's
> patch which memsets it to zero makes sense, but it can't
> return an error.

Ahh, I stand corrected.


> They're separate problems, really. With or without the patch I
> first posted, you're getting crap back when you ask for
> statistics on a non-mesh device. The second patch fixes that,
> and stands alone.

Yes, this is separate. That's why I wrote "The ultimate patch 
would be".

BTW, I like your two patches combined more than Sebastians memset 
approach. Because with Sebastians patch on on ethX we would get 
zeroed nonsense instead of uninitialized nonsense.

The memset should possible there anyway in the case of a firmware 
that can mshX, but not CMD_ACT_MESH_GET_STATS or which would 
return some error for another reason.

Re: [PATCH] libertas: Fix ethtool statistics

[Sebastian Siewior]

* Holger Schurig | 2008-04-18 15:10:53 [+0200]:

>> > I wrote "if (ret) return ret", not "if (ret) return;".
>>
>> I know you did, but I don't know why. The
>> lbs_ethtool_get_stats() function returns void. Sebastian's
>> patch which memsets it to zero makes sense, but it can't
>> return an error.
>
>Ahh, I stand corrected.
The function before lbs_ethtool_get_stats() (in net code that
determinate the number of elements, dunno the name right now) is able to
return an error. This one could check if the firmware has mesh support.

>> They're separate problems, really. With or without the patch I
>> first posted, you're getting crap back when you ask for
>> statistics on a non-mesh device. The second patch fixes that,
>> and stands alone.

Do I understand this correct: If the firmware has support for mesh
devices than we have ethX and mshX and only mshX should return the
statistics?

>Yes, this is separate. That's why I wrote "The ultimate patch 
>would be".
>
>BTW, I like your two patches combined more than Sebastians memset 
>approach. Because with Sebastians patch on on ethX we would get 
>zeroed nonsense instead of uninitialized nonsense.
returning uninitialized nonsense is leaking kernel memory.

>The memset should possible there anyway in the case of a firmware 
>that can mshX, but not CMD_ACT_MESH_GET_STATS or which would 
>return some error for another reason.
What about changing lbs_ethtool_get_stats() from void to int? All other
drivers are reading memory to get this data, we have to go through
usb/cs/sdio layer and all of them may fail.

I will try to form a patch around Monday that fixes Dan's comments (if
nobody else is going to).

Sebastian

Re: [PATCH] libertas: Fix ethtool statistics

[David Woodhouse]

On Sun, 2008-04-20 at 10:41 +0200, Sebastian Siewior wrote:
> The function before lbs_ethtool_get_stats() (in net code that
> determinate the number of elements, dunno the name right now) is able to
> return an error. This one could check if the firmware has mesh support.

That's what my second patch does.

> Do I understand this correct: If the firmware has support for mesh
> devices than we have ethX and mshX and only mshX should return the
> statistics?

Well, they are _mesh_ statistics :)

> returning uninitialized nonsense is leaking kernel memory.

Indeed. We need that memset there to handle errors, definitely.

> What about changing lbs_ethtool_get_stats() from void to int? All
> other drivers are reading memory to get this data, we have to go
> through usb/cs/sdio layer and all of them may fail.

Yeah, that might be worthwhile.

> I will try to form a patch around Monday that fixes Dan's comments (if
> nobody else is going to).

Which comments? If we apply your original patch, followed by my patch in
<1208509888.9212.291.camel@pmac.infradead.org> (which for some reason
only went to wireless-dev, not libertas-dev), then I think it's fine.


-- 
dwmw2

Re: [PATCH] libertas: Fix ethtool statistics

[Sebastian Siewior]

* David Woodhouse | 2008-04-20 11:56:32 [+0100]:

>On Sun, 2008-04-20 at 10:41 +0200, Sebastian Siewior wrote:
>> The function before lbs_ethtool_get_stats() (in net code that
>> determinate the number of elements, dunno the name right now) is able to
>> return an error. This one could check if the firmware has mesh support.
>
>That's what my second patch does.
got to check that patch.

>> Do I understand this correct: If the firmware has support for mesh
>> devices than we have ethX and mshX and only mshX should return the
>> statistics?
>
>Well, they are _mesh_ statistics :)
yes, but I have two interfaces or just one?

>Yeah, that might be worthwhile.
>
>> I will try to form a patch around Monday that fixes Dan's comments (if
>> nobody else is going to).
>
>Which comments? If we apply your original patch, followed by my patch in
><1208509888.9212.291.camel@pmac.infradead.org> (which for some reason
>only went to wireless-dev, not libertas-dev), then I think it's fine.
The comments were
- everything what deals with mesh should take the mash lock
- check for mesh support before calling lbs_ethtool_get_stats()

If your second patch deals with these than yes it should be fine.

>dwmw2

Sebastian

Re: [PATCH] libertas: Fix ethtool statistics

[David Woodhouse]

On Sun, 2008-04-20 at 21:10 +0200, Sebastian Siewior wrote:
> The comments were
> - everything what deals with mesh should take the mash lock
> - check for mesh support before calling lbs_ethtool_get_stats()
> 
> If your second patch deals with these than yes it should be fine.

It deals with the latter. I don't believe we need any more locking here;
we just submit our own request, wait for it to return and use the data
in the reply.

-- 
dwmw2