On 05/04/2017 01:09 PM, Pavel Belous wrote:
>
>
> On 04.05.2017 19:51, David Miller wrote:
>> From: Lino Sanfilippo <linosanfili...@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.bel...@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.bel...@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
