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 <[email protected]>
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
* 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 <[email protected]>
>
>Acked-by: Dan Williams <[email protected]>
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
* 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
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
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 <[email protected]>
Acked-by: Dan Williams <[email protected]>
>
> 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;
>
* 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
><[email protected]> (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
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
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 <[email protected]>
> >
> >Acked-by: Dan Williams <[email protected]>
> 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
> 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 :-)
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
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?
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
<[email protected]> (which for some reason
only went to wireless-dev, not libertas-dev), then I think it's fine.
--
dwmw2
> > 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.
* 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
From: Sebastian Siewior <[email protected]>
This will prevent the execution of lbs_ethtool_get_stats() via ethtool -S
if there is no mesh support.
Signed-off-by: Sebastian Siewior <[email protected]>
---
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
* David Woodhouse | 2008-05-19 16:32:02 [+0100]:
>Fix various problems:
> - We converted MESH_ACCESS to a direct command but missed this caller.
> - We were trying to access mesh stats even on meshless firmware.
> - We should really zero the buffer if something goes wrong.
>
>Signed-off-by: David Woodhouse <[email protected]>
Cheers
Sebastian
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
[email protected]
On Sat, 2008-05-10 at 09:24 +0200, Sebastian Siewior wrote:
> From: Sebastian Siewior <[email protected]>
>
> This will prevent the execution of lbs_ethtool_get_stats() via ethtool -S
> if there is no mesh support.
>
> Signed-off-by: Sebastian Siewior <[email protected]>
> ---
> 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;