Re: [PATCH] aquantia: Fix "ethtool -S" crash when adapter down.

2017-05-04 Thread David Arcari 
On 05/04/2017 04:10 PM, 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.
> 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.bel...@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, );
>   }
> @@ -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 <darc...@redhat.com>

Re: [PATCH] aquantia: Fix "ethtool -S" crash when adapter down.

2017-05-04 Thread David Arcari 
On 05/04/2017 01:09 PM, Pavel Belous wrote:
> 
> 
> On 04.05.2017 19:51, David Miller wrote:
>> From: Lino Sanfilippo 
>> Date: Thu, 4 May 2017 18:48:12 +0200
>>
>>> Hi Pavel,
>>>
>>> On 04.05.2017 18:33, Pavel Belous wrote:
 From: Pavel Belous 

 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 
 ---
  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, );
  }
 @@ -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

Re: [PATCH] aquantia: Fix "ethtool -S" crash when adapter down.

2017-05-04 Thread David Arcari 
Hi Pavel,

On 05/04/2017 12:33 PM, Pavel Belous wrote:
> From: Pavel Belous 
> 
> 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 
> ---
>  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, );
>   }
> @@ -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:;
>

Re: [PATCH v3 net 2/5] net:ethernet:aquantia: Fix packet type detection (TCP/UDP) for IPv6.

2017-03-23 Thread David Arcari 
On 03/23/2017 07:19 AM, Pavel Belous wrote:
> From: Pavel Belous <pavel.bel...@aquantia.com>
> 
> In order for the checksum offloads to work correctly we need to set the
> packet type bit (TCP/UDP) in the TX context buffer.
> 
> 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 | 20 
>  1 file changed, 16 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_nic.c 
> b/drivers/net/ethernet/aquantia/atlantic/aq_nic.c
> index ee78444..db2b51d 100644
> --- a/drivers/net/ethernet/aquantia/atlantic/aq_nic.c
> +++ b/drivers/net/ethernet/aquantia/atlantic/aq_nic.c
> @@ -510,10 +510,22 @@ static unsigned int aq_nic_map_skb(struct aq_nic_s 
> *self,
>   if (skb->ip_summed == CHECKSUM_PARTIAL) {
>   dx_buff->is_ip_cso = (htons(ETH_P_IP) == skb->protocol) ?
>   1U : 0U;
> - dx_buff->is_tcp_cso =
> - (ip_hdr(skb)->protocol == IPPROTO_TCP) ? 1U : 0U;
> - dx_buff->is_udp_cso =
> - (ip_hdr(skb)->protocol == IPPROTO_UDP) ? 1U : 0U;
> +
> + if (ip_hdr(skb)->version == 4) {
> + dx_buff->is_tcp_cso =
> + (ip_hdr(skb)->protocol == IPPROTO_TCP) ?
> + 1U : 0U;
> + dx_buff->is_udp_cso =
> + (ip_hdr(skb)->protocol == IPPROTO_UDP) ?
> + 1U : 0U;
> + } else if (ip_hdr(skb)->version == 6) {
> + dx_buff->is_tcp_cso =
> + (ipv6_hdr(skb)->nexthdr == NEXTHDR_TCP) ?
> + 1U : 0U;
> + dx_buff->is_udp_cso =
> + (ipv6_hdr(skb)->nexthdr == NEXTHDR_UDP) ?
> + 1U : 0U;
> + }
>   }
>  
>   for (; nr_frags--; ++frag_count) {
> 

Fixes tcp/ipv6

Tested-by: David Arcari <darc...@redhat.com>

Re: [PATCH net 4/5] net:ethernet:aquantia: Fix for LSO with IPv6.

2017-03-22 Thread David Arcari 
Hi,

On 03/22/2017 01:06 PM, Pavel Belous wrote:
> From: Pavel Belous 
> 
> Fix Context Command bit: L3 type = "0" for IPv4, "1" for IPv6.
> 
> Signed-off-by: Pavel Belous 
> ---
>  drivers/net/ethernet/aquantia/atlantic/aq_nic.c   | 3 +++
>  drivers/net/ethernet/aquantia/atlantic/aq_ring.h  | 3 ++-
>  drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_a0.c | 3 +++
>  drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_b0.c | 3 +++
>  4 files changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_nic.c 
> b/drivers/net/ethernet/aquantia/atlantic/aq_nic.c
> index 95f9841..293b261 100644
> --- a/drivers/net/ethernet/aquantia/atlantic/aq_nic.c
> +++ b/drivers/net/ethernet/aquantia/atlantic/aq_nic.c
> @@ -487,6 +487,9 @@ static unsigned int aq_nic_map_skb(struct aq_nic_s *self,
>   dx_buff->mss = skb_shinfo(skb)->gso_size;
>   dx_buff->is_txc = 1U;
>  
> + dx_buff->is_ipv6 =
> + (ip_hdr(skb)->version == 6) ? 1U : 0U;
> +
>   dx = aq_ring_next_dx(ring, dx);
>   dx_buff = >buff_ring[dx];
>   ++ret;
> diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_ring.h 
> b/drivers/net/ethernet/aquantia/atlantic/aq_ring.h
> index 2572546..eecd6d1 100644
> --- a/drivers/net/ethernet/aquantia/atlantic/aq_ring.h
> +++ b/drivers/net/ethernet/aquantia/atlantic/aq_ring.h
> @@ -58,7 +58,8 @@ struct __packed aq_ring_buff_s {
>   u8 len_l2;
>   u8 len_l3;
>   u8 len_l4;
> - u8 rsvd2;
> + u8 is_ipv6:1;
> + u8 rsvd2:7;
>   u32 len_pkt;
>   };
>   };
> 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 a2b746a..d62436e 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
> @@ -433,6 +433,9 @@ static int hw_atl_a0_hw_ring_tx_xmit(struct aq_hw_s *self,
>   buff->len_l3 +
>   buff->len_l2);
>   is_gso = true;
> +
> + if (buff->is_ipv6)
> + txd->ctl |= HW_ATL_B0_TXD_CTL_CMD_IPV6;

I think this is a mistake.  I believe it should be HW_ATL_A0_TXD_CTL_CMD_IPV6.

AFAICT this file doesn't include hw_atl_b0_internal.h, so I don't believe this
will compile.

Thanks,

-DA

>   } else {
>   buff_pa_len = buff->len;
>  
> 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 cab2931..69488c9 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
> @@ -471,6 +471,9 @@ static int hw_atl_b0_hw_ring_tx_xmit(struct aq_hw_s *self,
>   buff->len_l3 +
>   buff->len_l2);
>   is_gso = true;
> +
> + if (buff->is_ipv6)
> + txd->ctl |= HW_ATL_B0_TXD_CTL_CMD_IPV6;
>   } else {
>   buff_pa_len = buff->len;
>  
>

[PATCH] net: ethernet: aquantia: set net_device mtu when mtu is changed

2017-03-13 Thread David Arcari 
When the aquantia device mtu is changed the net_device structure is not
updated.  As a result the ip command does not properly reflect the mtu change.

Commit 5513e16421cb incorrectly assumed that __dev_set_mtu() was making the
assignment ndev->mtu = new_mtu;  This is not true in the case where the driver
has a ndo_change_mtu routine.

Fixes: 5513e16421cb ("net: ethernet: aquantia: Fixes for aq_ndev_change_mtu")

Cc: Pavel Belous <pavel.bel...@aquantia.com>
Signed-off-by: David Arcari <darc...@redhat.com>
---
 drivers/net/ethernet/aquantia/atlantic/aq_main.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_main.c 
b/drivers/net/ethernet/aquantia/atlantic/aq_main.c
index dad6362..d05fbfd 100644
--- a/drivers/net/ethernet/aquantia/atlantic/aq_main.c
+++ b/drivers/net/ethernet/aquantia/atlantic/aq_main.c
@@ -98,6 +98,7 @@ static int aq_ndev_change_mtu(struct net_device *ndev, int 
new_mtu)
 
if (err < 0)
goto err_exit;
+   ndev->mtu = new_mtu;
 
if (netif_running(ndev)) {
aq_ndev_close(ndev);
-- 
1.8.3.1

Re: [PATCH] net: ethernet: aquantia: set net_device mtu when mtu is changed

2017-03-13 Thread David Arcari 
On 03/13/2017 03:56 PM, David Miller wrote:
> From: David Arcari <darc...@redhat.com>
> Date: Mon, 13 Mar 2017 11:50:50 -0400
> 
>> On 03/13/2017 02:09 AM, David Miller wrote:
>>> From: David Arcari <darc...@redhat.com>
>>> Date: Wed,  8 Mar 2017 16:33:21 -0500
>>>
>>>> When the aquantia device mtu is changed the net_device structure is not
>>>> updated.  As a result the ip command does not properly reflect the mtu 
>>>> change.
>>>>
>>>> Commit 5513e16421cb incorrectly assumed that __dev_set_mtu() was making the
>>>> assignment ndev->mtu = new_mtu;  This is not true in the case where the 
>>>> driver
>>>> has a ndo_change_mtu routine.
>>>>
>>>> Fixes: 5513e16421cb ("net: ethernet: aquantia: Fixes for 
>>>> aq_ndev_change_mtu")
>>>>
>>>> Cc: Pavel Belous <pavel.bel...@aquantia.com>
>>>> Signed-off-by: David Arcari <darc...@redhat.com>
>>>
>>> Applied, thanks.
>>>
>>
>> Hi David,
>>
>> I see that my patch:
>>
>> "net: ethernet: aquantia: call set_irq_affinity_hint before free_irq"
>>
>> has been applied to net, but I don't see that this patch has been applied.
> 
> It is marked as "changes requested" in patchwork, because you were asked to
> do the restart label removal in a separate patch.
> 


Sorry, I was not clear.  I was trying to ask if your "Applied, thanks" reply
(above) was meant for this email thread.  I believe it was meant for the
set_irq_affinity_hint patch.

Thanks,

-DA

Re: [PATCH v2] net: ethernet: aquantia: set net_device mtu when mtu is changed

2017-03-13 Thread David Arcari 
On 03/10/2017 04:19 PM, Pavel Belous wrote:
> 
> 
> On 10.03.2017 17:47, David Arcari wrote:
>> Hi,
>>
>> On 03/09/2017 05:43 PM, Lino Sanfilippo wrote:
>>> Hi,
>>>
>>> On 09.03.2017 22:03, David Arcari wrote:
>>>> When the aquantia device mtu is changed the net_device structure is not
>>>> updated.  As a result the ip command does not properly reflect the mtu 
>>>> change.
>>>>
>>>> Commit 5513e16421cb incorrectly assumed that __dev_set_mtu() was making the
>>>> assignment ndev->mtu = new_mtu;  This is not true in the case where the 
>>>> driver
>>>> has a ndo_change_mtu routine.
>>>>
>>>> Fixes: 5513e16421cb ("net: ethernet: aquantia: Fixes for 
>>>> aq_ndev_change_mtu")
>>>>
>>>> v2: no longer close/open net-device after mtu change
>>>>
>>>> Cc: Pavel Belous <pavel.bel...@aquantia.com>
>>>> Signed-off-by: David Arcari <darc...@redhat.com>
>>>> ---
>>>>  drivers/net/ethernet/aquantia/atlantic/aq_main.c | 10 ++
>>>>  1 file changed, 2 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_main.c
>>>> b/drivers/net/ethernet/aquantia/atlantic/aq_main.c
>>>> index dad6362..bba5ebd 100644
>>>> --- a/drivers/net/ethernet/aquantia/atlantic/aq_main.c
>>>> +++ b/drivers/net/ethernet/aquantia/atlantic/aq_main.c
>>>> @@ -96,15 +96,9 @@ static int aq_ndev_change_mtu(struct net_device *ndev,
>>>> int new_mtu)
>>>>  struct aq_nic_s *aq_nic = netdev_priv(ndev);
>>>>  int err = aq_nic_set_mtu(aq_nic, new_mtu + ETH_HLEN);
>>>>
>>>> -if (err < 0)
>>>> -goto err_exit;
>>>> +if (!err)
>>>> +ndev->mtu = new_mtu;
>>>>
>>>> -if (netif_running(ndev)) {
>>>> -aq_ndev_close(ndev);
>>>> -aq_ndev_open(ndev);
>>>> -}
>>>> -
>>>> -err_exit:
>>>
>>> Removing the restart has nothing to do with the bug you want to fix here, 
>>> has
>>> it?
>>> I suggest to send a separate patch for this.
>>>
>>> Regards,
>>> Lino
>>>
>>
>> I'm fine with that.  Pavel does that work for you?
>>
>> It would mean that the original version of this patch should be applied and
>> either you or I could send the follow-up patch.
>>
>> Best,
>>
>> -Dave
>>
> 
> David,
> 
> Yes, I think for this it is better to make separate patch.
> I can prepare a patch for "close/open netdev" myself.
> 
> Probably you need send the original version of this patch as "v3" (I'm no sure
> what it is possible to discard v2 and apply v1 instead.)
> 
> Thank you,
> Pavel

Please drop v2 of this patch.  We would like to have v1 applied.

Thanks,

-Dave

Re: [PATCH] net: ethernet: aquantia: set net_device mtu when mtu is changed

2017-03-13 Thread David Arcari 
On 03/13/2017 02:09 AM, David Miller wrote:
> From: David Arcari <darc...@redhat.com>
> Date: Wed,  8 Mar 2017 16:33:21 -0500
> 
>> When the aquantia device mtu is changed the net_device structure is not
>> updated.  As a result the ip command does not properly reflect the mtu 
>> change.
>>
>> Commit 5513e16421cb incorrectly assumed that __dev_set_mtu() was making the
>> assignment ndev->mtu = new_mtu;  This is not true in the case where the 
>> driver
>> has a ndo_change_mtu routine.
>>
>> Fixes: 5513e16421cb ("net: ethernet: aquantia: Fixes for aq_ndev_change_mtu")
>>
>> Cc: Pavel Belous <pavel.bel...@aquantia.com>
>> Signed-off-by: David Arcari <darc...@redhat.com>
> 
> Applied, thanks.
> 

Hi David,

I see that my patch:

"net: ethernet: aquantia: call set_irq_affinity_hint before free_irq"

has been applied to net, but I don't see that this patch has been applied.

Is there any chance that you replied to the wrong patch?  Or am I missing 
something?

Thanks,

-Dave

Re: [PATCH v2] net: ethernet: aquantia: set net_device mtu when mtu is changed

2017-03-10 Thread David Arcari 
Hi,

On 03/09/2017 05:43 PM, Lino Sanfilippo wrote:
> Hi,
> 
> On 09.03.2017 22:03, David Arcari wrote:
>> When the aquantia device mtu is changed the net_device structure is not
>> updated.  As a result the ip command does not properly reflect the mtu 
>> change.
>>
>> Commit 5513e16421cb incorrectly assumed that __dev_set_mtu() was making the
>> assignment ndev->mtu = new_mtu;  This is not true in the case where the 
>> driver
>> has a ndo_change_mtu routine.
>>
>> Fixes: 5513e16421cb ("net: ethernet: aquantia: Fixes for aq_ndev_change_mtu")
>>
>> v2: no longer close/open net-device after mtu change
>>
>> Cc: Pavel Belous <pavel.bel...@aquantia.com>
>> Signed-off-by: David Arcari <darc...@redhat.com>
>> ---
>>  drivers/net/ethernet/aquantia/atlantic/aq_main.c | 10 ++
>>  1 file changed, 2 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_main.c 
>> b/drivers/net/ethernet/aquantia/atlantic/aq_main.c
>> index dad6362..bba5ebd 100644
>> --- a/drivers/net/ethernet/aquantia/atlantic/aq_main.c
>> +++ b/drivers/net/ethernet/aquantia/atlantic/aq_main.c
>> @@ -96,15 +96,9 @@ static int aq_ndev_change_mtu(struct net_device *ndev, 
>> int new_mtu)
>>  struct aq_nic_s *aq_nic = netdev_priv(ndev);
>>  int err = aq_nic_set_mtu(aq_nic, new_mtu + ETH_HLEN);
>>  
>> -if (err < 0)
>> -goto err_exit;
>> +if (!err)
>> +ndev->mtu = new_mtu;
>>  
>> -if (netif_running(ndev)) {
>> -aq_ndev_close(ndev);
>> -aq_ndev_open(ndev);
>> -}
>> -
>> -err_exit:
> 
> Removing the restart has nothing to do with the bug you want to fix here, has 
> it?
> I suggest to send a separate patch for this.
> 
> Regards,
> Lino
> 

I'm fine with that.  Pavel does that work for you?

It would mean that the original version of this patch should be applied and
either you or I could send the follow-up patch.

Best,

-Dave

[PATCH v2] net: ethernet: aquantia: set net_device mtu when mtu is changed

2017-03-09 Thread David Arcari 
When the aquantia device mtu is changed the net_device structure is not
updated.  As a result the ip command does not properly reflect the mtu change.

Commit 5513e16421cb incorrectly assumed that __dev_set_mtu() was making the
assignment ndev->mtu = new_mtu;  This is not true in the case where the driver
has a ndo_change_mtu routine.

Fixes: 5513e16421cb ("net: ethernet: aquantia: Fixes for aq_ndev_change_mtu")

v2: no longer close/open net-device after mtu change

Cc: Pavel Belous <pavel.bel...@aquantia.com>
Signed-off-by: David Arcari <darc...@redhat.com>
---
 drivers/net/ethernet/aquantia/atlantic/aq_main.c | 10 ++
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_main.c 
b/drivers/net/ethernet/aquantia/atlantic/aq_main.c
index dad6362..bba5ebd 100644
--- a/drivers/net/ethernet/aquantia/atlantic/aq_main.c
+++ b/drivers/net/ethernet/aquantia/atlantic/aq_main.c
@@ -96,15 +96,9 @@ static int aq_ndev_change_mtu(struct net_device *ndev, int 
new_mtu)
struct aq_nic_s *aq_nic = netdev_priv(ndev);
int err = aq_nic_set_mtu(aq_nic, new_mtu + ETH_HLEN);
 
-   if (err < 0)
-   goto err_exit;
+   if (!err)
+   ndev->mtu = new_mtu;
 
-   if (netif_running(ndev)) {
-   aq_ndev_close(ndev);
-   aq_ndev_open(ndev);
-   }
-
-err_exit:
return err;
 }
 
-- 
1.8.3.1

Re: [PATCH] net: ethernet: aquantia: set net_device mtu when mtu is changed

2017-03-09 Thread David Arcari 
On 03/09/2017 03:01 PM, David Arcari wrote:
> On 03/09/2017 02:02 PM, Pavel Belous wrote:
>>
>>
>> On 09.03.2017 00:33, David Arcari wrote:
>>> When the aquantia device mtu is changed the net_device structure is not
>>> updated.  As a result the ip command does not properly reflect the mtu 
>>> change.
>>>
>>> Commit 5513e16421cb incorrectly assumed that __dev_set_mtu() was making the
>>> assignment ndev->mtu = new_mtu;  This is not true in the case where the 
>>> driver
>>> has a ndo_change_mtu routine.
>>>
>>> Fixes: 5513e16421cb ("net: ethernet: aquantia: Fixes for 
>>> aq_ndev_change_mtu")
>>>
>>> Cc: Pavel Belous <pavel.bel...@aquantia.com>
>>> Signed-off-by: David Arcari <darc...@redhat.com>
>>> ---
>>>  drivers/net/ethernet/aquantia/atlantic/aq_main.c | 1 +
>>>  1 file changed, 1 insertion(+)
>>>
>>> diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_main.c
>>> b/drivers/net/ethernet/aquantia/atlantic/aq_main.c
>>> index dad6362..d05fbfd 100644
>>> --- a/drivers/net/ethernet/aquantia/atlantic/aq_main.c
>>> +++ b/drivers/net/ethernet/aquantia/atlantic/aq_main.c
>>> @@ -98,6 +98,7 @@ static int aq_ndev_change_mtu(struct net_device *ndev, int
>>> new_mtu)
>>>
>>>  if (err < 0)
>>>  goto err_exit;
>>> +ndev->mtu = new_mtu;
>>>
>>>  if (netif_running(ndev)) {
>>>  aq_ndev_close(ndev);
>>>
>>
>> Thank you, David.
>>
>> I think we should also remove closing/opening net-device after mtu changed.
> 
> Hi Pavel,
> 
> I'll go ahead and submit v2.
> 
> Thanks,
> 
> -Dave

Hi Pavel,

Before I post v2, won't that mean that if the interface is up that the user will
have to manually toggle it for the new mtu to take effect?

Is that the desired behavior?

Thanks,

-Dave

> 
>>
>> Regards,
>> Pavel
>

Re: [PATCH] net: ethernet: aquantia: set net_device mtu when mtu is changed

2017-03-09 Thread David Arcari 
On 03/09/2017 02:02 PM, Pavel Belous wrote:
> 
> 
> On 09.03.2017 00:33, David Arcari wrote:
>> When the aquantia device mtu is changed the net_device structure is not
>> updated.  As a result the ip command does not properly reflect the mtu 
>> change.
>>
>> Commit 5513e16421cb incorrectly assumed that __dev_set_mtu() was making the
>> assignment ndev->mtu = new_mtu;  This is not true in the case where the 
>> driver
>> has a ndo_change_mtu routine.
>>
>> Fixes: 5513e16421cb ("net: ethernet: aquantia: Fixes for aq_ndev_change_mtu")
>>
>> Cc: Pavel Belous <pavel.bel...@aquantia.com>
>> Signed-off-by: David Arcari <darc...@redhat.com>
>> ---
>>  drivers/net/ethernet/aquantia/atlantic/aq_main.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_main.c
>> b/drivers/net/ethernet/aquantia/atlantic/aq_main.c
>> index dad6362..d05fbfd 100644
>> --- a/drivers/net/ethernet/aquantia/atlantic/aq_main.c
>> +++ b/drivers/net/ethernet/aquantia/atlantic/aq_main.c
>> @@ -98,6 +98,7 @@ static int aq_ndev_change_mtu(struct net_device *ndev, int
>> new_mtu)
>>
>>  if (err < 0)
>>  goto err_exit;
>> +ndev->mtu = new_mtu;
>>
>>  if (netif_running(ndev)) {
>>  aq_ndev_close(ndev);
>>
> 
> Thank you, David.
> 
> I think we should also remove closing/opening net-device after mtu changed.

Hi Pavel,

I'll go ahead and submit v2.

Thanks,

-Dave

> 
> Regards,
> Pavel

[PATCH] net: ethernet: aquantia: call set_irq_affinity_hint before free_irq

2017-03-09 Thread David Arcari 
When a network interface controlled by the aquantia ethernet driver is brought
down a warning is output in dmesg (see below).

The problem is that aq_pci_func_free_irqs() is calling free_irq() before it is
calling irq_set_affinity_hint().

WARNING: CPU: 4 PID: 10068 at kernel/irq/manage.c:1503 __free_irq+0x24d/0x2b0

Call Trace:
 dump_stack+0x63/0x87
 __warn+0xd1/0xf0
 warn_slowpath_null+0x1d/0x20
 __free_irq+0x24d/0x2b0
 free_irq+0x39/0x90
 aq_pci_func_free_irqs+0x52/0xa0 [atlantic]
 aq_nic_stop+0xca/0xd0 [atlantic]
 aq_ndev_close+0x1d/0x40 [atlantic]
 __dev_close_many+0x99/0x100
 __dev_close+0x67/0xb0


Fixes: 36a4a50f4048 ("net: ethernet: aquantia: switch to pci_alloc_irq_vectors")

Cc: Christoph Hellwig <h...@lst.de>
Cc: Pavel Belous <pavel.bel...@aquantia.com>
Signed-off-by: David Arcari <darc...@redhat.com>
---
 drivers/net/ethernet/aquantia/atlantic/aq_pci_func.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_pci_func.c 
b/drivers/net/ethernet/aquantia/atlantic/aq_pci_func.c
index 581de71..4c6c882 100644
--- a/drivers/net/ethernet/aquantia/atlantic/aq_pci_func.c
+++ b/drivers/net/ethernet/aquantia/atlantic/aq_pci_func.c
@@ -213,9 +213,9 @@ void aq_pci_func_free_irqs(struct aq_pci_func_s *self)
if (!((1U << i) & self->msix_entry_mask))
continue;
 
-   free_irq(pci_irq_vector(pdev, i), self->aq_vec[i]);
if (pdev->msix_enabled)
irq_set_affinity_hint(pci_irq_vector(pdev, i), NULL);
+   free_irq(pci_irq_vector(pdev, i), self->aq_vec[i]);
self->msix_entry_mask &= ~(1U << i);
}
 }
-- 
1.8.3.1

[PATCH] net: ethernet: aquantia: set net_device mtu when mtu is changed

2017-03-08 Thread David Arcari 
When the aquantia device mtu is changed the net_device structure is not
updated.  As a result the ip command does not properly reflect the mtu change.

Commit 5513e16421cb incorrectly assumed that __dev_set_mtu() was making the
assignment ndev->mtu = new_mtu;  This is not true in the case where the driver
has a ndo_change_mtu routine.

Fixes: 5513e16421cb ("net: ethernet: aquantia: Fixes for aq_ndev_change_mtu")

Cc: Pavel Belous <pavel.bel...@aquantia.com>
Signed-off-by: David Arcari <darc...@redhat.com>
---
 drivers/net/ethernet/aquantia/atlantic/aq_main.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_main.c 
b/drivers/net/ethernet/aquantia/atlantic/aq_main.c
index dad6362..d05fbfd 100644
--- a/drivers/net/ethernet/aquantia/atlantic/aq_main.c
+++ b/drivers/net/ethernet/aquantia/atlantic/aq_main.c
@@ -98,6 +98,7 @@ static int aq_ndev_change_mtu(struct net_device *ndev, int 
new_mtu)
 
if (err < 0)
goto err_exit;
+   ndev->mtu = new_mtu;
 
if (netif_running(ndev)) {
aq_ndev_close(ndev);
-- 
1.8.3.1

Re: [PATCH] net: ethtool: avoid allocation failure for dump_regs

2017-01-19 Thread David Arcari 
On 01/18/2017 11:45 AM, David Miller wrote:
> From: David Arcari <darc...@redhat.com>
> Date: Wed, 18 Jan 2017 08:34:05 -0500
>
>> If the user executes 'ethtool -d' for an interface and the associated
>> get_regs_len() function returns 0, the user will see a call trace from
>> the vmalloc() call in ethtool_get_regs().  This patch modifies
>> ethtool_get_regs() to avoid the call to vmalloc when the size is zero.
>>
>> Signed-off-by: David Arcari <darc...@redhat.com>
> I think when the driver indicates this, it is equivalent to saying that
> the operation isn't supported.
>
> Also, this guards us against ->get_regs() methods that don't handle
> zero length requests properly.  I see many which are going to do
> really terrible things in that situation.
>
> Therefore, if get_regs_len() returns zero, treat it the safe as if the
> ethtool operations were NULL.
>
> Thanks.

That was actually the fix that I was originally considering, but it
turns out
there is a problem with it.

I found that the vmalloc error was occurring because
ieee80211_get_regs_len() in
net/mac80211/ethtool.c was returning zero.  The ieee80211_get_regs in
the same
file returns the hw version. It turns out that this information is used
by the
at76c50x-usb driver in the user space ethtool to report which HW variant
is in
use.  Returning an error when regs_len() returns zero would break this
functionality.

-Dave

[PATCH] net: ethtool: avoid allocation failure for dump_regs

2017-01-18 Thread David Arcari 
If the user executes 'ethtool -d' for an interface and the associated
get_regs_len() function returns 0, the user will see a call trace from
the vmalloc() call in ethtool_get_regs().  This patch modifies
ethtool_get_regs() to avoid the call to vmalloc when the size is zero.

Signed-off-by: David Arcari <darc...@redhat.com>
---
 net/core/ethtool.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/core/ethtool.c b/net/core/ethtool.c
index e23766c..47acd6f 100644
--- a/net/core/ethtool.c
+++ b/net/core/ethtool.c
@@ -1405,7 +1405,7 @@ static int ethtool_get_regs(struct net_device *dev, char 
__user *useraddr)
if (regs.len > reglen)
regs.len = reglen;
 
-   regbuf = vzalloc(reglen);
+   regbuf = reglen ? vzalloc(reglen) : NULL;
if (reglen && !regbuf)
return -ENOMEM;
 
--