* [PATCH] aquantia: Fix "ethtool -S" crash when adapter down.
@ 2017-05-04 16:33 Pavel Belous
2017-05-04 16:48 ` Lino Sanfilippo
2017-05-04 17:00 ` David Arcari
0 siblings, 2 replies; 15+ messages in thread
From: Pavel Belous @ 2017-05-04 16:33 UTC (permalink / raw)
To: David S . Miller; +Cc: netdev, David Arcari
From: Pavel Belous <pavel.belous@aquantia.com>
This patch fixes the crash that happens when driver tries to collect statistics
from already released "aq_vec" object.
Fixes: 97bde5c4f909 ("net: ethernet: aquantia: Support for NIC-specific code")
Signed-off-by: Pavel Belous <pavel.belous@aquantia.com>
---
drivers/net/ethernet/aquantia/atlantic/aq_nic.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_nic.c b/drivers/net/ethernet/aquantia/atlantic/aq_nic.c
index cdb0299..3a32573 100644
--- a/drivers/net/ethernet/aquantia/atlantic/aq_nic.c
+++ b/drivers/net/ethernet/aquantia/atlantic/aq_nic.c
@@ -755,7 +755,7 @@ void aq_nic_get_stats(struct aq_nic_s *self, u64 *data)
count = 0U;
for (i = 0U, aq_vec = self->aq_vec[0];
- self->aq_vecs > i; ++i, aq_vec = self->aq_vec[i]) {
+ aq_vec && self->aq_vecs > i; ++i, aq_vec = self->aq_vec[i]) {
data += count;
aq_vec_get_sw_stats(aq_vec, data, &count);
}
@@ -961,6 +961,7 @@ void aq_nic_free_hot_resources(struct aq_nic_s *self)
for (i = AQ_DIMOF(self->aq_vec); i--;) {
if (self->aq_vec[i])
aq_vec_free(self->aq_vec[i]);
+ self->aq_vec[i] = NULL;
}
err_exit:;
--
2.7.4
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH] aquantia: Fix "ethtool -S" crash when adapter down.
2017-05-04 16:33 [PATCH] aquantia: Fix "ethtool -S" crash when adapter down Pavel Belous
@ 2017-05-04 16:48 ` Lino Sanfilippo
2017-05-04 16:51 ` David Miller
2017-05-04 17:00 ` David Arcari
1 sibling, 1 reply; 15+ messages in thread
From: Lino Sanfilippo @ 2017-05-04 16:48 UTC (permalink / raw)
To: Pavel Belous, David S . Miller; +Cc: netdev, David Arcari
Hi Pavel,
On 04.05.2017 18:33, Pavel Belous wrote:
> From: Pavel Belous <pavel.belous@aquantia.com>
>
> This patch fixes the crash that happens when driver tries to collect statistics
> from already released "aq_vec" object.
>
> Fixes: 97bde5c4f909 ("net: ethernet: aquantia: Support for NIC-specific code")
> Signed-off-by: Pavel Belous <pavel.belous@aquantia.com>
> ---
> drivers/net/ethernet/aquantia/atlantic/aq_nic.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_nic.c b/drivers/net/ethernet/aquantia/atlantic/aq_nic.c
> index cdb0299..3a32573 100644
> --- a/drivers/net/ethernet/aquantia/atlantic/aq_nic.c
> +++ b/drivers/net/ethernet/aquantia/atlantic/aq_nic.c
> @@ -755,7 +755,7 @@ void aq_nic_get_stats(struct aq_nic_s *self, u64 *data)
> count = 0U;
>
> for (i = 0U, aq_vec = self->aq_vec[0];
> - self->aq_vecs > i; ++i, aq_vec = self->aq_vec[i]) {
> + aq_vec && self->aq_vecs > i; ++i, aq_vec = self->aq_vec[i]) {
> data += count;
> aq_vec_get_sw_stats(aq_vec, data, &count);
> }
> @@ -961,6 +961,7 @@ void aq_nic_free_hot_resources(struct aq_nic_s *self)
> for (i = AQ_DIMOF(self->aq_vec); i--;) {
> if (self->aq_vec[i])
> aq_vec_free(self->aq_vec[i]);
> + self->aq_vec[i] = NULL;
> }
>
> err_exit:;
>
if the driver does not support statistics when the interface is down, would not it be clearer
to check if netif_running() in get_stats() instead?
Regards,
Lino
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] aquantia: Fix "ethtool -S" crash when adapter down.
2017-05-04 16:48 ` Lino Sanfilippo
@ 2017-05-04 16:51 ` David Miller
2017-05-04 17:09 ` Pavel Belous
0 siblings, 1 reply; 15+ messages in thread
From: David Miller @ 2017-05-04 16:51 UTC (permalink / raw)
To: LinoSanfilippo; +Cc: Pavel.Belous, netdev, darcari
From: Lino Sanfilippo <LinoSanfilippo@gmx.de>
Date: Thu, 4 May 2017 18:48:12 +0200
> Hi Pavel,
>
> On 04.05.2017 18:33, Pavel Belous wrote:
>> From: Pavel Belous <pavel.belous@aquantia.com>
>>
>> This patch fixes the crash that happens when driver tries to collect statistics
>> from already released "aq_vec" object.
>>
>> Fixes: 97bde5c4f909 ("net: ethernet: aquantia: Support for NIC-specific code")
>> Signed-off-by: Pavel Belous <pavel.belous@aquantia.com>
>> ---
>> drivers/net/ethernet/aquantia/atlantic/aq_nic.c | 3 ++-
>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_nic.c b/drivers/net/ethernet/aquantia/atlantic/aq_nic.c
>> index cdb0299..3a32573 100644
>> --- a/drivers/net/ethernet/aquantia/atlantic/aq_nic.c
>> +++ b/drivers/net/ethernet/aquantia/atlantic/aq_nic.c
>> @@ -755,7 +755,7 @@ void aq_nic_get_stats(struct aq_nic_s *self, u64 *data)
>> count = 0U;
>>
>> for (i = 0U, aq_vec = self->aq_vec[0];
>> - self->aq_vecs > i; ++i, aq_vec = self->aq_vec[i]) {
>> + aq_vec && self->aq_vecs > i; ++i, aq_vec = self->aq_vec[i]) {
>> data += count;
>> aq_vec_get_sw_stats(aq_vec, data, &count);
>> }
>> @@ -961,6 +961,7 @@ void aq_nic_free_hot_resources(struct aq_nic_s *self)
>> for (i = AQ_DIMOF(self->aq_vec); i--;) {
>> if (self->aq_vec[i])
>> aq_vec_free(self->aq_vec[i]);
>> + self->aq_vec[i] = NULL;
>> }
>>
>> err_exit:;
>>
>
> if the driver does not support statistics when the interface is down, would not it be clearer
> to check if netif_running() in get_stats() instead?
Yes, much cleaner.
Much better would be to have a cached software copy so that statistics
can be reported regardless of whether the device is down or not.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] aquantia: Fix "ethtool -S" crash when adapter down.
2017-05-04 16:33 [PATCH] aquantia: Fix "ethtool -S" crash when adapter down Pavel Belous
2017-05-04 16:48 ` Lino Sanfilippo
@ 2017-05-04 17:00 ` David Arcari
2017-05-04 17:08 ` Pavel Belous
1 sibling, 1 reply; 15+ messages in thread
From: David Arcari @ 2017-05-04 17:00 UTC (permalink / raw)
To: Pavel Belous, David S . Miller; +Cc: netdev
Hi Pavel,
On 05/04/2017 12:33 PM, Pavel Belous wrote:
> From: Pavel Belous <pavel.belous@aquantia.com>
>
> This patch fixes the crash that happens when driver tries to collect statistics
> from already released "aq_vec" object.
>
> Fixes: 97bde5c4f909 ("net: ethernet: aquantia: Support for NIC-specific code")
> Signed-off-by: Pavel Belous <pavel.belous@aquantia.com>
> ---
> drivers/net/ethernet/aquantia/atlantic/aq_nic.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_nic.c b/drivers/net/ethernet/aquantia/atlantic/aq_nic.c
> index cdb0299..3a32573 100644
> --- a/drivers/net/ethernet/aquantia/atlantic/aq_nic.c
> +++ b/drivers/net/ethernet/aquantia/atlantic/aq_nic.c
> @@ -755,7 +755,7 @@ void aq_nic_get_stats(struct aq_nic_s *self, u64 *data)
> count = 0U;
>
> for (i = 0U, aq_vec = self->aq_vec[0];
> - self->aq_vecs > i; ++i, aq_vec = self->aq_vec[i]) {
> + aq_vec && self->aq_vecs > i; ++i, aq_vec = self->aq_vec[i]) {
> data += count;
> aq_vec_get_sw_stats(aq_vec, data, &count);
> }
> @@ -961,6 +961,7 @@ void aq_nic_free_hot_resources(struct aq_nic_s *self)
> for (i = AQ_DIMOF(self->aq_vec); i--;) {
> if (self->aq_vec[i])
> aq_vec_free(self->aq_vec[i]);
> + self->aq_vec[i] = NULL;
I think you intended to to add { } to the if statement. The code compiles as
is, but the indentation is not correct.
-DA
> }
>
> err_exit:;
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] aquantia: Fix "ethtool -S" crash when adapter down.
2017-05-04 17:00 ` David Arcari
@ 2017-05-04 17:08 ` Pavel Belous
2017-05-04 18:17 ` Joe Perches
0 siblings, 1 reply; 15+ messages in thread
From: Pavel Belous @ 2017-05-04 17:08 UTC (permalink / raw)
To: David Arcari, David S . Miller; +Cc: netdev
On 04.05.2017 20:00, David Arcari wrote:
> Hi Pavel,
>
> On 05/04/2017 12:33 PM, Pavel Belous wrote:
>> From: Pavel Belous <pavel.belous@aquantia.com>
>>
>> This patch fixes the crash that happens when driver tries to collect statistics
>> from already released "aq_vec" object.
>>
>> Fixes: 97bde5c4f909 ("net: ethernet: aquantia: Support for NIC-specific code")
>> Signed-off-by: Pavel Belous <pavel.belous@aquantia.com>
>> ---
>> drivers/net/ethernet/aquantia/atlantic/aq_nic.c | 3 ++-
>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_nic.c b/drivers/net/ethernet/aquantia/atlantic/aq_nic.c
>> index cdb0299..3a32573 100644
>> --- a/drivers/net/ethernet/aquantia/atlantic/aq_nic.c
>> +++ b/drivers/net/ethernet/aquantia/atlantic/aq_nic.c
>> @@ -755,7 +755,7 @@ void aq_nic_get_stats(struct aq_nic_s *self, u64 *data)
>> count = 0U;
>>
>> for (i = 0U, aq_vec = self->aq_vec[0];
>> - self->aq_vecs > i; ++i, aq_vec = self->aq_vec[i]) {
>> + aq_vec && self->aq_vecs > i; ++i, aq_vec = self->aq_vec[i]) {
>> data += count;
>> aq_vec_get_sw_stats(aq_vec, data, &count);
>> }
>> @@ -961,6 +961,7 @@ void aq_nic_free_hot_resources(struct aq_nic_s *self)
>> for (i = AQ_DIMOF(self->aq_vec); i--;) {
>> if (self->aq_vec[i])
>> aq_vec_free(self->aq_vec[i]);
>> + self->aq_vec[i] = NULL;
>
> I think you intended to to add { } to the if statement. The code compiles as
> is, but the indentation is not correct.
>
> -DA
Oh. Sorry about that. I did not see the loss of braces during merge.
I will prepare another patch with Lino and David M. comments.
Regards,
Pavel.
>
>> }
>>
>> err_exit:;
>>
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] aquantia: Fix "ethtool -S" crash when adapter down.
2017-05-04 16:51 ` David Miller
@ 2017-05-04 17:09 ` Pavel Belous
2017-05-04 18:27 ` David Arcari
0 siblings, 1 reply; 15+ messages in thread
From: Pavel Belous @ 2017-05-04 17:09 UTC (permalink / raw)
To: David Miller, LinoSanfilippo; +Cc: netdev, darcari
On 04.05.2017 19:51, David Miller wrote:
> From: Lino Sanfilippo <LinoSanfilippo@gmx.de>
> Date: Thu, 4 May 2017 18:48:12 +0200
>
>> Hi Pavel,
>>
>> On 04.05.2017 18:33, Pavel Belous wrote:
>>> From: Pavel Belous <pavel.belous@aquantia.com>
>>>
>>> This patch fixes the crash that happens when driver tries to collect statistics
>>> from already released "aq_vec" object.
>>>
>>> Fixes: 97bde5c4f909 ("net: ethernet: aquantia: Support for NIC-specific code")
>>> Signed-off-by: Pavel Belous <pavel.belous@aquantia.com>
>>> ---
>>> drivers/net/ethernet/aquantia/atlantic/aq_nic.c | 3 ++-
>>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_nic.c b/drivers/net/ethernet/aquantia/atlantic/aq_nic.c
>>> index cdb0299..3a32573 100644
>>> --- a/drivers/net/ethernet/aquantia/atlantic/aq_nic.c
>>> +++ b/drivers/net/ethernet/aquantia/atlantic/aq_nic.c
>>> @@ -755,7 +755,7 @@ void aq_nic_get_stats(struct aq_nic_s *self, u64 *data)
>>> count = 0U;
>>>
>>> for (i = 0U, aq_vec = self->aq_vec[0];
>>> - self->aq_vecs > i; ++i, aq_vec = self->aq_vec[i]) {
>>> + aq_vec && self->aq_vecs > i; ++i, aq_vec = self->aq_vec[i]) {
>>> data += count;
>>> aq_vec_get_sw_stats(aq_vec, data, &count);
>>> }
>>> @@ -961,6 +961,7 @@ void aq_nic_free_hot_resources(struct aq_nic_s *self)
>>> for (i = AQ_DIMOF(self->aq_vec); i--;) {
>>> if (self->aq_vec[i])
>>> aq_vec_free(self->aq_vec[i]);
>>> + self->aq_vec[i] = NULL;
>>> }
>>>
>>> err_exit:;
>>>
>>
>> if the driver does not support statistics when the interface is down, would not it be clearer
>> to check if netif_running() in get_stats() instead?
>
> Yes, much cleaner.
>
> Much better would be to have a cached software copy so that statistics
> can be reported regardless of whether the device is down or not.
>
Thank you.
I will think about how to do it better.
Regards,
Pavel
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] aquantia: Fix "ethtool -S" crash when adapter down.
2017-05-04 17:08 ` Pavel Belous
@ 2017-05-04 18:17 ` Joe Perches
2017-05-04 18:41 ` Pavel Belous
0 siblings, 1 reply; 15+ messages in thread
From: Joe Perches @ 2017-05-04 18:17 UTC (permalink / raw)
To: Pavel Belous, David Arcari, David S . Miller; +Cc: netdev
On Thu, 2017-05-04 at 20:08 +0300, Pavel Belous wrote:
> I will prepare another patch with Lino and David M. comments.
I'm not submitting this because it'd just cause merge conflicts,
but
something you could do one day is remove the AQ_DIMOF macro
and just use ARRAY_SIZE directly.
---
drivers/net/ethernet/aquantia/atlantic/aq_nic.c | 4 ++--
drivers/net/ethernet/aquantia/atlantic/aq_utils.h | 2 --
drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_a0.c | 2 +-
drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_b0.c | 2 +-
drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_utils.c | 2 +-
5 files changed, 5 insertions(+), 7 deletions(-)
diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_nic.c b/drivers/net/ethernet/aquantia/atlantic/aq_nic.c
index cdb02991f249..cffae53414ba 100644
--- a/drivers/net/ethernet/aquantia/atlantic/aq_nic.c
+++ b/drivers/net/ethernet/aquantia/atlantic/aq_nic.c
@@ -154,7 +154,7 @@ static void aq_nic_service_timer_cb(unsigned long param)
memset(&stats_rx, 0U, sizeof(struct aq_ring_stats_rx_s));
memset(&stats_tx, 0U, sizeof(struct aq_ring_stats_tx_s));
- for (i = AQ_DIMOF(self->aq_vec); i--;) {
+ for (i = ARRAY_SIZE(self->aq_vec); i--;) {
if (self->aq_vec[i])
aq_vec_add_stats(self->aq_vec[i], &stats_rx, &stats_tx);
}
@@ -958,7 +958,7 @@ void aq_nic_free_hot_resources(struct aq_nic_s *self)
if (!self)
goto err_exit;
- for (i = AQ_DIMOF(self->aq_vec); i--;) {
+ for (i = ARRAY_SIZE(self->aq_vec); i--;) {
if (self->aq_vec[i])
aq_vec_free(self->aq_vec[i]);
}
diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_utils.h b/drivers/net/ethernet/aquantia/atlantic/aq_utils.h
index f6012b34abe6..64a8c3c781ff 100644
--- a/drivers/net/ethernet/aquantia/atlantic/aq_utils.h
+++ b/drivers/net/ethernet/aquantia/atlantic/aq_utils.h
@@ -14,8 +14,6 @@
#include "aq_common.h"
-#define AQ_DIMOF(_ARY_) ARRAY_SIZE(_ARY_)
-
struct aq_obj_s {
spinlock_t lock; /* spinlock for nic/rings processing */
atomic_t flags;
diff --git a/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_a0.c b/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_a0.c
index 4ee15ff06a44..96c3360e7060 100644
--- a/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_a0.c
+++ b/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_a0.c
@@ -182,7 +182,7 @@ static int hw_atl_a0_hw_rss_set(struct aq_hw_s *self,
((i * 3U) & 0xFU));
}
- for (i = AQ_DIMOF(bitary); i--;) {
+ for (i = ARRAY_SIZE(bitary); i--;) {
rpf_rss_redir_tbl_wr_data_set(self, bitary[i]);
rpf_rss_redir_tbl_addr_set(self, i);
rpf_rss_redir_wr_en_set(self, 1U);
diff --git a/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_b0.c b/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_b0.c
index 42150708191d..5a19eba31786 100644
--- a/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_b0.c
+++ b/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_b0.c
@@ -182,7 +182,7 @@ static int hw_atl_b0_hw_rss_set(struct aq_hw_s *self,
((i * 3U) & 0xFU));
}
- for (i = AQ_DIMOF(bitary); i--;) {
+ for (i = ARRAY_SIZE(bitary); i--;) {
rpf_rss_redir_tbl_wr_data_set(self, bitary[i]);
rpf_rss_redir_tbl_addr_set(self, i);
rpf_rss_redir_wr_en_set(self, 1U);
diff --git a/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_utils.c b/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_utils.c
index 8d6d8f5804da..922af5f36d37 100644
--- a/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_utils.c
+++ b/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_utils.c
@@ -385,7 +385,7 @@ int hw_atl_utils_get_mac_permanent(struct aq_hw_s *self,
aq_hw_read_reg(self, 0x00000374U) +
(40U * 4U),
mac_addr,
- AQ_DIMOF(mac_addr));
+ ARRAY_SIZE(mac_addr));
if (err < 0) {
mac_addr[0] = 0U;
mac_addr[1] = 0U;
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH] aquantia: Fix "ethtool -S" crash when adapter down.
2017-05-04 17:09 ` Pavel Belous
@ 2017-05-04 18:27 ` David Arcari
2017-05-04 18:37 ` Pavel Belous
0 siblings, 1 reply; 15+ messages in thread
From: David Arcari @ 2017-05-04 18:27 UTC (permalink / raw)
To: Pavel Belous, David Miller, LinoSanfilippo; +Cc: netdev
On 05/04/2017 01:09 PM, Pavel Belous wrote:
>
>
> On 04.05.2017 19:51, David Miller wrote:
>> From: Lino Sanfilippo <LinoSanfilippo@gmx.de>
>> Date: Thu, 4 May 2017 18:48:12 +0200
>>
>>> Hi Pavel,
>>>
>>> On 04.05.2017 18:33, Pavel Belous wrote:
>>>> From: Pavel Belous <pavel.belous@aquantia.com>
>>>>
>>>> This patch fixes the crash that happens when driver tries to collect statistics
>>>> from already released "aq_vec" object.
>>>>
>>>> Fixes: 97bde5c4f909 ("net: ethernet: aquantia: Support for NIC-specific code")
>>>> Signed-off-by: Pavel Belous <pavel.belous@aquantia.com>
>>>> ---
>>>> drivers/net/ethernet/aquantia/atlantic/aq_nic.c | 3 ++-
>>>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_nic.c
>>>> b/drivers/net/ethernet/aquantia/atlantic/aq_nic.c
>>>> index cdb0299..3a32573 100644
>>>> --- a/drivers/net/ethernet/aquantia/atlantic/aq_nic.c
>>>> +++ b/drivers/net/ethernet/aquantia/atlantic/aq_nic.c
>>>> @@ -755,7 +755,7 @@ void aq_nic_get_stats(struct aq_nic_s *self, u64 *data)
>>>> count = 0U;
>>>>
>>>> for (i = 0U, aq_vec = self->aq_vec[0];
>>>> - self->aq_vecs > i; ++i, aq_vec = self->aq_vec[i]) {
>>>> + aq_vec && self->aq_vecs > i; ++i, aq_vec = self->aq_vec[i]) {
>>>> data += count;
>>>> aq_vec_get_sw_stats(aq_vec, data, &count);
>>>> }
>>>> @@ -961,6 +961,7 @@ void aq_nic_free_hot_resources(struct aq_nic_s *self)
>>>> for (i = AQ_DIMOF(self->aq_vec); i--;) {
>>>> if (self->aq_vec[i])
>>>> aq_vec_free(self->aq_vec[i]);
>>>> + self->aq_vec[i] = NULL;
>>>> }
>>>>
>>>> err_exit:;
>>>>
>>>
>>> if the driver does not support statistics when the interface is down, would
>>> not it be clearer
>>> to check if netif_running() in get_stats() instead?
>>
>> Yes, much cleaner.
>>
>> Much better would be to have a cached software copy so that statistics
>> can be reported regardless of whether the device is down or not.
>>
>
> Thank you.
> I will think about how to do it better.
It appears that the adapter is still reporting the cumulative hardware stats
even while its down. The user is just losing the per queue stats.
Although the loss of the per queue stats is not ideal, this patch still fixes a
crash.
It might be worthwhile to refactor this patch as a short term solution and then
subsequently produce a version that contains cached statistics. Assuming that
is amenable to everyone of course.
-DA
>
> Regards,
> Pavel
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] aquantia: Fix "ethtool -S" crash when adapter down.
2017-05-04 18:27 ` David Arcari
@ 2017-05-04 18:37 ` Pavel Belous
2017-05-04 19:08 ` Lino Sanfilippo
0 siblings, 1 reply; 15+ messages in thread
From: Pavel Belous @ 2017-05-04 18:37 UTC (permalink / raw)
To: David Arcari, David Miller, LinoSanfilippo; +Cc: netdev
On 04.05.2017 21:27, David Arcari wrote:
> On 05/04/2017 01:09 PM, Pavel Belous wrote:
>>
>>
>> On 04.05.2017 19:51, David Miller wrote:
>>> From: Lino Sanfilippo <LinoSanfilippo@gmx.de>
>>> Date: Thu, 4 May 2017 18:48:12 +0200
>>>
>>>> Hi Pavel,
>>>>
>>>> On 04.05.2017 18:33, Pavel Belous wrote:
>>>>> From: Pavel Belous <pavel.belous@aquantia.com>
>>>>>
>>>>> This patch fixes the crash that happens when driver tries to collect statistics
>>>>> from already released "aq_vec" object.
>>>>>
>>>>> Fixes: 97bde5c4f909 ("net: ethernet: aquantia: Support for NIC-specific code")
>>>>> Signed-off-by: Pavel Belous <pavel.belous@aquantia.com>
>>>>> ---
>>>>> drivers/net/ethernet/aquantia/atlantic/aq_nic.c | 3 ++-
>>>>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_nic.c
>>>>> b/drivers/net/ethernet/aquantia/atlantic/aq_nic.c
>>>>> index cdb0299..3a32573 100644
>>>>> --- a/drivers/net/ethernet/aquantia/atlantic/aq_nic.c
>>>>> +++ b/drivers/net/ethernet/aquantia/atlantic/aq_nic.c
>>>>> @@ -755,7 +755,7 @@ void aq_nic_get_stats(struct aq_nic_s *self, u64 *data)
>>>>> count = 0U;
>>>>>
>>>>> for (i = 0U, aq_vec = self->aq_vec[0];
>>>>> - self->aq_vecs > i; ++i, aq_vec = self->aq_vec[i]) {
>>>>> + aq_vec && self->aq_vecs > i; ++i, aq_vec = self->aq_vec[i]) {
>>>>> data += count;
>>>>> aq_vec_get_sw_stats(aq_vec, data, &count);
>>>>> }
>>>>> @@ -961,6 +961,7 @@ void aq_nic_free_hot_resources(struct aq_nic_s *self)
>>>>> for (i = AQ_DIMOF(self->aq_vec); i--;) {
>>>>> if (self->aq_vec[i])
>>>>> aq_vec_free(self->aq_vec[i]);
>>>>> + self->aq_vec[i] = NULL;
>>>>> }
>>>>>
>>>>> err_exit:;
>>>>>
>>>>
>>>> if the driver does not support statistics when the interface is down, would
>>>> not it be clearer
>>>> to check if netif_running() in get_stats() instead?
>>>
>>> Yes, much cleaner.
>>>
>>> Much better would be to have a cached software copy so that statistics
>>> can be reported regardless of whether the device is down or not.
>>>
>>
>> Thank you.
>> I will think about how to do it better.
>
> It appears that the adapter is still reporting the cumulative hardware stats
> even while its down. The user is just losing the per queue stats.
>
> Although the loss of the per queue stats is not ideal, this patch still fixes a
> crash.
>
> It might be worthwhile to refactor this patch as a short term solution and then
> subsequently produce a version that contains cached statistics. Assuming that
> is amenable to everyone of course.
>
> -DA
>
Yes, even adapter is in the down state user can still see statistics
from the HW.
For example (adapter is down):
$ ethtool -S enp2s0
NIC statistics:
InPackets: 3237727
InUCast: 3237214
InMCast: 391
InBCast: 122
InErrors: 0
OutPackets: 14157898
OutUCast: 14157089
OutMCast: 304
OutBCast: 505
InUCastOctects: 226714406
OutUCastOctects: 10463156
InMCastOctects: 58046
OutMCastOctects: 44817
InBCastOctects: 12857
OutBCastOctects: 41626
InOctects: 226785309
OutOctects: 10549599
InPacketsDma: 0
OutPacketsDma: 16
InOctetsDma: 0
OutOctetsDma: 2396
InDroppedDma: 0
Queue[0] InPackets: 0
Queue[0] OutPackets: 0
Queue[0] InJumboPackets: 0
Queue[0] InLroPackets: 0
Queue[0] InErrors: 0
Queue[1] InPackets: 0
Queue[1] OutPackets: 0
Queue[1] InJumboPackets: 0
Queue[1] InLroPackets: 0
Queue[1] InErrors: 0
Queue[2] InPackets: 0
Queue[2] OutPackets: 0
Queue[2] InJumboPackets: 0
Queue[2] InLroPackets: 0
Queue[2] InErrors: 0
Queue[3] InPackets: 0
Queue[3] OutPackets: 0
Queue[3] InJumboPackets: 0
Queue[3] InLroPackets: 0
Queue[3] InErrors: 0
Lino, David what do you think?
If you agree I can re-submit the patch (with fixed braces).
Regards,
Pavel
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] aquantia: Fix "ethtool -S" crash when adapter down.
2017-05-04 18:17 ` Joe Perches
@ 2017-05-04 18:41 ` Pavel Belous
0 siblings, 0 replies; 15+ messages in thread
From: Pavel Belous @ 2017-05-04 18:41 UTC (permalink / raw)
To: Joe Perches, David Arcari, David S . Miller; +Cc: netdev
On 04.05.2017 21:17, Joe Perches wrote:
> On Thu, 2017-05-04 at 20:08 +0300, Pavel Belous wrote:
>> I will prepare another patch with Lino and David M. comments.
>
> I'm not submitting this because it'd just cause merge conflicts,
> but
> something you could do one day is remove the AQ_DIMOF macro
> and just use ARRAY_SIZE directly.
> ---
> drivers/net/ethernet/aquantia/atlantic/aq_nic.c | 4 ++--
> drivers/net/ethernet/aquantia/atlantic/aq_utils.h | 2 --
> drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_a0.c | 2 +-
> drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_b0.c | 2 +-
> drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_utils.c | 2 +-
> 5 files changed, 5 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_nic.c b/drivers/net/ethernet/aquantia/atlantic/aq_nic.c
> index cdb02991f249..cffae53414ba 100644
> --- a/drivers/net/ethernet/aquantia/atlantic/aq_nic.c
> +++ b/drivers/net/ethernet/aquantia/atlantic/aq_nic.c
> @@ -154,7 +154,7 @@ static void aq_nic_service_timer_cb(unsigned long param)
>
> memset(&stats_rx, 0U, sizeof(struct aq_ring_stats_rx_s));
> memset(&stats_tx, 0U, sizeof(struct aq_ring_stats_tx_s));
> - for (i = AQ_DIMOF(self->aq_vec); i--;) {
> + for (i = ARRAY_SIZE(self->aq_vec); i--;) {
> if (self->aq_vec[i])
> aq_vec_add_stats(self->aq_vec[i], &stats_rx, &stats_tx);
> }
> @@ -958,7 +958,7 @@ void aq_nic_free_hot_resources(struct aq_nic_s *self)
> if (!self)
> goto err_exit;
>
> - for (i = AQ_DIMOF(self->aq_vec); i--;) {
> + for (i = ARRAY_SIZE(self->aq_vec); i--;) {
> if (self->aq_vec[i])
> aq_vec_free(self->aq_vec[i]);
> }
> diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_utils.h b/drivers/net/ethernet/aquantia/atlantic/aq_utils.h
> index f6012b34abe6..64a8c3c781ff 100644
> --- a/drivers/net/ethernet/aquantia/atlantic/aq_utils.h
> +++ b/drivers/net/ethernet/aquantia/atlantic/aq_utils.h
> @@ -14,8 +14,6 @@
>
> #include "aq_common.h"
>
> -#define AQ_DIMOF(_ARY_) ARRAY_SIZE(_ARY_)
> -
> struct aq_obj_s {
> spinlock_t lock; /* spinlock for nic/rings processing */
> atomic_t flags;
> diff --git a/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_a0.c b/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_a0.c
> index 4ee15ff06a44..96c3360e7060 100644
> --- a/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_a0.c
> +++ b/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_a0.c
> @@ -182,7 +182,7 @@ static int hw_atl_a0_hw_rss_set(struct aq_hw_s *self,
> ((i * 3U) & 0xFU));
> }
>
> - for (i = AQ_DIMOF(bitary); i--;) {
> + for (i = ARRAY_SIZE(bitary); i--;) {
> rpf_rss_redir_tbl_wr_data_set(self, bitary[i]);
> rpf_rss_redir_tbl_addr_set(self, i);
> rpf_rss_redir_wr_en_set(self, 1U);
> diff --git a/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_b0.c b/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_b0.c
> index 42150708191d..5a19eba31786 100644
> --- a/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_b0.c
> +++ b/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_b0.c
> @@ -182,7 +182,7 @@ static int hw_atl_b0_hw_rss_set(struct aq_hw_s *self,
> ((i * 3U) & 0xFU));
> }
>
> - for (i = AQ_DIMOF(bitary); i--;) {
> + for (i = ARRAY_SIZE(bitary); i--;) {
> rpf_rss_redir_tbl_wr_data_set(self, bitary[i]);
> rpf_rss_redir_tbl_addr_set(self, i);
> rpf_rss_redir_wr_en_set(self, 1U);
> diff --git a/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_utils.c b/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_utils.c
> index 8d6d8f5804da..922af5f36d37 100644
> --- a/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_utils.c
> +++ b/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_utils.c
> @@ -385,7 +385,7 @@ int hw_atl_utils_get_mac_permanent(struct aq_hw_s *self,
> aq_hw_read_reg(self, 0x00000374U) +
> (40U * 4U),
> mac_addr,
> - AQ_DIMOF(mac_addr));
> + ARRAY_SIZE(mac_addr));
> if (err < 0) {
> mac_addr[0] = 0U;
> mac_addr[1] = 0U;
>
Thank you,
I will do it, little bit later.
Regards,
Pavel
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] aquantia: Fix "ethtool -S" crash when adapter down.
2017-05-04 18:37 ` Pavel Belous
@ 2017-05-04 19:08 ` Lino Sanfilippo
0 siblings, 0 replies; 15+ messages in thread
From: Lino Sanfilippo @ 2017-05-04 19:08 UTC (permalink / raw)
To: Pavel Belous, David Arcari, David Miller; +Cc: netdev
On 04.05.2017 20:37, Pavel Belous wrote:
>
> Yes, even adapter is in the down state user can still see statistics from the HW.
> For example (adapter is down):
>
> $ ethtool -S enp2s0
> NIC statistics:
> InPackets: 3237727
> InUCast: 3237214
> InMCast: 391
> InBCast: 122
> InErrors: 0
> OutPackets: 14157898
> OutUCast: 14157089
> OutMCast: 304
> OutBCast: 505
> InUCastOctects: 226714406
> OutUCastOctects: 10463156
> InMCastOctects: 58046
> OutMCastOctects: 44817
> InBCastOctects: 12857
> OutBCastOctects: 41626
> InOctects: 226785309
> OutOctects: 10549599
> InPacketsDma: 0
> OutPacketsDma: 16
> InOctetsDma: 0
> OutOctetsDma: 2396
> InDroppedDma: 0
> Queue[0] InPackets: 0
> Queue[0] OutPackets: 0
> Queue[0] InJumboPackets: 0
> Queue[0] InLroPackets: 0
> Queue[0] InErrors: 0
> Queue[1] InPackets: 0
> Queue[1] OutPackets: 0
> Queue[1] InJumboPackets: 0
> Queue[1] InLroPackets: 0
> Queue[1] InErrors: 0
> Queue[2] InPackets: 0
> Queue[2] OutPackets: 0
> Queue[2] InJumboPackets: 0
> Queue[2] InLroPackets: 0
> Queue[2] InErrors: 0
> Queue[3] InPackets: 0
> Queue[3] OutPackets: 0
> Queue[3] InJumboPackets: 0
> Queue[3] InLroPackets: 0
> Queue[3] InErrors: 0
>
> Lino, David what do you think?
> If you agree I can re-submit the patch (with fixed braces).
>
Well my objection was related to how the bug is fixed. If in the end we have a solution
as suggested by David it would be even better, of course. I dont think that this is too
hard to realize since it is only the queues stats that are missing. But if you prefer to
solve it in two steps, sure, no objections from my side :)
Regards,
Lino
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] aquantia: Fix "ethtool -S" crash when adapter down.
2017-05-04 20:10 Pavel Belous
2017-05-04 20:30 ` Joe Perches
2017-05-04 20:39 ` David Arcari
@ 2017-05-08 16:53 ` David Miller
2 siblings, 0 replies; 15+ messages in thread
From: David Miller @ 2017-05-08 16:53 UTC (permalink / raw)
To: Pavel.Belous; +Cc: netdev, darcari, LinoSanfilippo, simon.edelhaus
From: Pavel Belous <Pavel.Belous@aquantia.com>
Date: Thu, 4 May 2017 23:10:56 +0300
> From: Pavel Belous <pavel.belous@aquantia.com>
>
> This patch fixes the crash that happens when driver tries to collect statistics
> from already released "aq_vec" object.
> If adapter is in "down" state we still allow user to see statistics from HW.
>
> V2: fixed braces around "aq_vec_free".
>
> Fixes: 97bde5c4f909 ("net: ethernet: aquantia: Support for NIC-specific code")
> Signed-off-by: Pavel Belous <pavel.belous@aquantia.com>
Applied, thanks.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] aquantia: Fix "ethtool -S" crash when adapter down.
2017-05-04 20:10 Pavel Belous
2017-05-04 20:30 ` Joe Perches
@ 2017-05-04 20:39 ` David Arcari
2017-05-08 16:53 ` David Miller
2 siblings, 0 replies; 15+ messages in thread
From: David Arcari @ 2017-05-04 20:39 UTC (permalink / raw)
To: Pavel Belous, David S . Miller; +Cc: netdev, Lino Sanfilippo, Simon Edelhaus
On 05/04/2017 04:10 PM, Pavel Belous wrote:
> From: Pavel Belous <pavel.belous@aquantia.com>
>
> This patch fixes the crash that happens when driver tries to collect statistics
> from already released "aq_vec" object.
> If adapter is in "down" state we still allow user to see statistics from HW.
>
> V2: fixed braces around "aq_vec_free".
>
> Fixes: 97bde5c4f909 ("net: ethernet: aquantia: Support for NIC-specific code")
> Signed-off-by: Pavel Belous <pavel.belous@aquantia.com>
> ---
> drivers/net/ethernet/aquantia/atlantic/aq_nic.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_nic.c b/drivers/net/ethernet/aquantia/atlantic/aq_nic.c
> index cdb0299..9ee1c50 100644
> --- a/drivers/net/ethernet/aquantia/atlantic/aq_nic.c
> +++ b/drivers/net/ethernet/aquantia/atlantic/aq_nic.c
> @@ -755,7 +755,7 @@ void aq_nic_get_stats(struct aq_nic_s *self, u64 *data)
> count = 0U;
>
> for (i = 0U, aq_vec = self->aq_vec[0];
> - self->aq_vecs > i; ++i, aq_vec = self->aq_vec[i]) {
> + aq_vec && self->aq_vecs > i; ++i, aq_vec = self->aq_vec[i]) {
> data += count;
> aq_vec_get_sw_stats(aq_vec, data, &count);
> }
> @@ -959,8 +959,10 @@ void aq_nic_free_hot_resources(struct aq_nic_s *self)
> goto err_exit;
>
> for (i = AQ_DIMOF(self->aq_vec); i--;) {
> - if (self->aq_vec[i])
> + if (self->aq_vec[i]) {
> aq_vec_free(self->aq_vec[i]);
> + self->aq_vec[i] = NULL;
> + }
> }
>
> err_exit:;
>
Resolves the ethtool crash.
Tested-by: David Arcari <darcari@redhat.com>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] aquantia: Fix "ethtool -S" crash when adapter down.
2017-05-04 20:10 Pavel Belous
@ 2017-05-04 20:30 ` Joe Perches
2017-05-04 20:39 ` David Arcari
2017-05-08 16:53 ` David Miller
2 siblings, 0 replies; 15+ messages in thread
From: Joe Perches @ 2017-05-04 20:30 UTC (permalink / raw)
To: Pavel Belous, David S . Miller
Cc: netdev, David Arcari, Lino Sanfilippo, Simon Edelhaus
On Thu, 2017-05-04 at 23:10 +0300, Pavel Belous wrote:
> diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_nic.c b/drivers/net/ethernet/aquantia/atlantic/aq_nic.c
[]
> @@ -959,8 +959,10 @@ void aq_nic_free_hot_resources(struct aq_nic_s *self)
> goto err_exit;
>
> for (i = AQ_DIMOF(self->aq_vec); i--;) {
> - if (self->aq_vec[i])
> + if (self->aq_vec[i]) {
> aq_vec_free(self->aq_vec[i]);
> + self->aq_vec[i] = NULL;
> + }
> }
>
> err_exit:;
unrelated style trivia:
err_exit:;
the error label then :; is pretty odd.
Also casting the return value to void is really odd.
A simple return instead of goto would be more common.
drivers/net/ethernet/aquantia/atlantic/aq_ring.c:311:err_exit:;
drivers/net/ethernet/aquantia/atlantic/aq_ring.c:326:err_exit:;
drivers/net/ethernet/aquantia/atlantic/aq_main.c:161:err_exit:;
drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_utils.c:277:err_exit:;
drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_utils.c:313:err_exit:;
drivers/net/ethernet/aquantia/atlantic/aq_nic.c:304:err_exit:;
drivers/net/ethernet/aquantia/atlantic/aq_nic.c:763:err_exit:;
drivers/net/ethernet/aquantia/atlantic/aq_nic.c:951:err_exit:;
drivers/net/ethernet/aquantia/atlantic/aq_nic.c:966:err_exit:;
drivers/net/ethernet/aquantia/atlantic/aq_vec.c:281:err_exit:;
drivers/net/ethernet/aquantia/atlantic/aq_vec.c:302:err_exit:;
drivers/net/ethernet/aquantia/atlantic/aq_pci_func.c:251:err_exit:;
drivers/net/ethernet/aquantia/atlantic/aq_pci_func.c:270:err_exit:;
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH] aquantia: Fix "ethtool -S" crash when adapter down.
@ 2017-05-04 20:10 Pavel Belous
2017-05-04 20:30 ` Joe Perches
` (2 more replies)
0 siblings, 3 replies; 15+ messages in thread
From: Pavel Belous @ 2017-05-04 20:10 UTC (permalink / raw)
To: David S . Miller; +Cc: netdev, David Arcari, Lino Sanfilippo, Simon Edelhaus
From: Pavel Belous <pavel.belous@aquantia.com>
This patch fixes the crash that happens when driver tries to collect statistics
from already released "aq_vec" object.
If adapter is in "down" state we still allow user to see statistics from HW.
V2: fixed braces around "aq_vec_free".
Fixes: 97bde5c4f909 ("net: ethernet: aquantia: Support for NIC-specific code")
Signed-off-by: Pavel Belous <pavel.belous@aquantia.com>
---
drivers/net/ethernet/aquantia/atlantic/aq_nic.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_nic.c b/drivers/net/ethernet/aquantia/atlantic/aq_nic.c
index cdb0299..9ee1c50 100644
--- a/drivers/net/ethernet/aquantia/atlantic/aq_nic.c
+++ b/drivers/net/ethernet/aquantia/atlantic/aq_nic.c
@@ -755,7 +755,7 @@ void aq_nic_get_stats(struct aq_nic_s *self, u64 *data)
count = 0U;
for (i = 0U, aq_vec = self->aq_vec[0];
- self->aq_vecs > i; ++i, aq_vec = self->aq_vec[i]) {
+ aq_vec && self->aq_vecs > i; ++i, aq_vec = self->aq_vec[i]) {
data += count;
aq_vec_get_sw_stats(aq_vec, data, &count);
}
@@ -959,8 +959,10 @@ void aq_nic_free_hot_resources(struct aq_nic_s *self)
goto err_exit;
for (i = AQ_DIMOF(self->aq_vec); i--;) {
- if (self->aq_vec[i])
+ if (self->aq_vec[i]) {
aq_vec_free(self->aq_vec[i]);
+ self->aq_vec[i] = NULL;
+ }
}
err_exit:;
--
2.7.4
^ permalink raw reply related [flat|nested] 15+ messages in thread
end of thread, other threads:[~2017-05-08 16:53 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-05-04 16:33 [PATCH] aquantia: Fix "ethtool -S" crash when adapter down Pavel Belous
2017-05-04 16:48 ` Lino Sanfilippo
2017-05-04 16:51 ` David Miller
2017-05-04 17:09 ` Pavel Belous
2017-05-04 18:27 ` David Arcari
2017-05-04 18:37 ` Pavel Belous
2017-05-04 19:08 ` Lino Sanfilippo
2017-05-04 17:00 ` David Arcari
2017-05-04 17:08 ` Pavel Belous
2017-05-04 18:17 ` Joe Perches
2017-05-04 18:41 ` Pavel Belous
2017-05-04 20:10 Pavel Belous
2017-05-04 20:30 ` Joe Perches
2017-05-04 20:39 ` David Arcari
2017-05-08 16:53 ` David Miller
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.