[PATCH net-next 00/13] net: ethernet: aquantia: improvements and fixes

[PATCH net-next 00/13] net: ethernet: aquantia: improvements and fixes

[Pavel Belous]

From: Pavel Belous <pavel.belous@aquantia.com>

The following patchset contains improvements and fixes for aQuantia
AQtion ethernet driver from net-next tree.

Most fixes are based on the comments from Lino Sanfilippo.

Sanity testing was performed on real HW.

Please review.
Thanks.


Pavel Belous (13):
  net: ethernet: aquantia: Removed extra assignment for skb->dev.
  net: ethernet: aquantia: Removed busy_count field.
  net: ethernet: aquantia: Removed unnecessary comparsion "old_mtu ==
    new_mtu".
  net: ethernet: aquantia: Using module_pci_driver.
  net: ethernet: aquantia: Superfluous initialization of "err".
  net: ethernet: aquantia: Fixed missing rtnl_unlock.
  net: ethernet: aquantia: Using NETDEV_TX_OK instead 0.
  net: ethernet: aquantia: Null pointer check for aq_nic_ndev_alloc.
  net: ethernet: aquantia: Call netdev_register after all initialized.
  net: ethernet: aquantia: Checking for success dma_map_single.
  net: ethernet: aquantia: Refactoring buffers copying.
  net: ethernet: aquantia: Fixed incorrect buff->len calculation.
  net: ethernet: aquantia: Fixed memory allocation if
    AQ_CFG_RX_FRAME_MAX > 1 page.

 drivers/net/ethernet/aquantia/atlantic/aq_main.c   | 34 ++------------
 drivers/net/ethernet/aquantia/atlantic/aq_nic.c    | 52 +++++++++++-----------
 drivers/net/ethernet/aquantia/atlantic/aq_ring.c   | 30 +++++++------
 drivers/net/ethernet/aquantia/atlantic/aq_utils.h  |  1 -
 .../ethernet/aquantia/atlantic/hw_atl/hw_atl_a0.c  |  4 +-
 .../ethernet/aquantia/atlantic/hw_atl/hw_atl_b0.c  |  4 +-
 6 files changed, 50 insertions(+), 75 deletions(-)

-- 
2.7.4

[PATCH net-next 01/13] net: ethernet: aquantia: Removed extra assignment for skb->dev.

[Pavel Belous]

From: Pavel Belous <pavel.belous@aquantia.com>

This assignment is not needed.

Signed-off-by: Pavel Belous <pavel.belous@aquantia.com>
---
 drivers/net/ethernet/aquantia/atlantic/aq_ring.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_ring.c b/drivers/net/ethernet/aquantia/atlantic/aq_ring.c
index dea9e9b..4c40644 100644
--- a/drivers/net/ethernet/aquantia/atlantic/aq_ring.c
+++ b/drivers/net/ethernet/aquantia/atlantic/aq_ring.c
@@ -214,7 +214,6 @@ int aq_ring_rx_clean(struct aq_ring_s *self, int *work_done, int budget)
 				goto err_exit;
 			}
 
-			skb->dev = ndev;
 			skb_put(skb, buff->len);
 		} else {
 			skb = netdev_alloc_skb(ndev, ETH_HLEN);
-- 
2.7.4

Re: [PATCH net-next 01/13] net: ethernet: aquantia: Removed extra assignment for skb->dev.

[Lino Sanfilippo]

Hi,

On 15.02.2017 21:01, Pavel Belous wrote:
> From: Pavel Belous <pavel.belous@aquantia.com>
>
> This assignment is not needed.
>
> Signed-off-by: Pavel Belous <pavel.belous@aquantia.com>
> ---
>  drivers/net/ethernet/aquantia/atlantic/aq_ring.c | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_ring.c b/drivers/net/ethernet/aquantia/atlantic/aq_ring.c
> index dea9e9b..4c40644 100644
> --- a/drivers/net/ethernet/aquantia/atlantic/aq_ring.c
> +++ b/drivers/net/ethernet/aquantia/atlantic/aq_ring.c
> @@ -214,7 +214,6 @@ int aq_ring_rx_clean(struct aq_ring_s *self, int *work_done, int budget)
>  				goto err_exit;
>  			}
>
> -			skb->dev = ndev;
>  			skb_put(skb, buff->len);
>  		} else {
>  			skb = netdev_alloc_skb(ndev, ETH_HLEN);
>

Reviewed-by: Lino Sanfilippo <LinoSanfilippo@gmx.de>

[PATCH net-next 02/13] net: ethernet: aquantia: Removed busy_count field.

[Pavel Belous]

From: Pavel Belous <pavel.belous@aquantia.com>

 busy_count field and is_busy flag is not needed at all.

Signed-off-by: Pavel Belous <pavel.belous@aquantia.com>
---
 drivers/net/ethernet/aquantia/atlantic/aq_nic.c   | 11 -----------
 drivers/net/ethernet/aquantia/atlantic/aq_utils.h |  1 -
 2 files changed, 12 deletions(-)

diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_nic.c b/drivers/net/ethernet/aquantia/atlantic/aq_nic.c
index aa22a7c..a153750 100644
--- a/drivers/net/ethernet/aquantia/atlantic/aq_nic.c
+++ b/drivers/net/ethernet/aquantia/atlantic/aq_nic.c
@@ -122,14 +122,11 @@ static void aq_nic_service_timer_cb(unsigned long param)
 	struct aq_nic_s *self = (struct aq_nic_s *)param;
 	struct net_device *ndev = aq_nic_get_ndev(self);
 	int err = 0;
-	bool is_busy = false;
 	unsigned int i = 0U;
 	struct aq_hw_link_status_s link_status;
 	struct aq_ring_stats_rx_s stats_rx;
 	struct aq_ring_stats_tx_s stats_tx;
 
-	atomic_inc(&self->header.busy_count);
-	is_busy = true;
 	if (aq_utils_obj_test(&self->header.flags, AQ_NIC_FLAGS_IS_NOT_READY))
 		goto err_exit;
 
@@ -170,8 +167,6 @@ static void aq_nic_service_timer_cb(unsigned long param)
 	ndev->stats.tx_errors = stats_tx.errors;
 
 err_exit:
-	if (is_busy)
-		atomic_dec(&self->header.busy_count);
 	mod_timer(&self->service_timer,
 		  jiffies + AQ_CFG_SERVICE_TIMER_INTERVAL);
 }
@@ -574,16 +569,12 @@ __acquires(&ring->lock)
 	unsigned int trys = AQ_CFG_LOCK_TRYS;
 	int err = 0;
 	bool is_nic_in_bad_state;
-	bool is_busy = false;
 	struct aq_ring_buff_s buffers[AQ_CFG_SKB_FRAGS_MAX];
 
 	frags = skb_shinfo(skb)->nr_frags + 1;
 
 	ring = self->aq_ring_tx[AQ_NIC_TCVEC2RING(self, tc, vec)];
 
-	atomic_inc(&self->header.busy_count);
-	is_busy = true;
-
 	if (frags > AQ_CFG_SKB_FRAGS_MAX) {
 		dev_kfree_skb_any(skb);
 		goto err_exit;
@@ -629,8 +620,6 @@ __acquires(&ring->lock)
 	}
 
 err_exit:
-	if (is_busy)
-		atomic_dec(&self->header.busy_count);
 	return err;
 }
 
diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_utils.h b/drivers/net/ethernet/aquantia/atlantic/aq_utils.h
index 4446bd9..f6012b3 100644
--- a/drivers/net/ethernet/aquantia/atlantic/aq_utils.h
+++ b/drivers/net/ethernet/aquantia/atlantic/aq_utils.h
@@ -19,7 +19,6 @@
 struct aq_obj_s {
 	spinlock_t lock; /* spinlock for nic/rings processing */
 	atomic_t flags;
-	atomic_t busy_count;
 };
 
 static inline void aq_utils_obj_set(atomic_t *flags, u32 mask)
-- 
2.7.4

Re: [PATCH net-next 02/13] net: ethernet: aquantia: Removed busy_count field.

[Lino Sanfilippo]

On 15.02.2017 21:01, Pavel Belous wrote:
> From: Pavel Belous <pavel.belous@aquantia.com>
>
>  busy_count field and is_busy flag is not needed at all.
>
> Signed-off-by: Pavel Belous <pavel.belous@aquantia.com>
> ---
>  drivers/net/ethernet/aquantia/atlantic/aq_nic.c   | 11 -----------
>  drivers/net/ethernet/aquantia/atlantic/aq_utils.h |  1 -
>  2 files changed, 12 deletions(-)
>
> diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_nic.c b/drivers/net/ethernet/aquantia/atlantic/aq_nic.c
> index aa22a7c..a153750 100644
> --- a/drivers/net/ethernet/aquantia/atlantic/aq_nic.c
> +++ b/drivers/net/ethernet/aquantia/atlantic/aq_nic.c
> @@ -122,14 +122,11 @@ static void aq_nic_service_timer_cb(unsigned long param)
>  	struct aq_nic_s *self = (struct aq_nic_s *)param;
>  	struct net_device *ndev = aq_nic_get_ndev(self);
>  	int err = 0;
> -	bool is_busy = false;
>  	unsigned int i = 0U;
>  	struct aq_hw_link_status_s link_status;
>  	struct aq_ring_stats_rx_s stats_rx;
>  	struct aq_ring_stats_tx_s stats_tx;
>
> -	atomic_inc(&self->header.busy_count);
> -	is_busy = true;
>  	if (aq_utils_obj_test(&self->header.flags, AQ_NIC_FLAGS_IS_NOT_READY))
>  		goto err_exit;
>
> @@ -170,8 +167,6 @@ static void aq_nic_service_timer_cb(unsigned long param)
>  	ndev->stats.tx_errors = stats_tx.errors;
>
>  err_exit:
> -	if (is_busy)
> -		atomic_dec(&self->header.busy_count);
>  	mod_timer(&self->service_timer,
>  		  jiffies + AQ_CFG_SERVICE_TIMER_INTERVAL);
>  }
> @@ -574,16 +569,12 @@ __acquires(&ring->lock)
>  	unsigned int trys = AQ_CFG_LOCK_TRYS;
>  	int err = 0;
>  	bool is_nic_in_bad_state;
> -	bool is_busy = false;
>  	struct aq_ring_buff_s buffers[AQ_CFG_SKB_FRAGS_MAX];
>
>  	frags = skb_shinfo(skb)->nr_frags + 1;
>
>  	ring = self->aq_ring_tx[AQ_NIC_TCVEC2RING(self, tc, vec)];
>
> -	atomic_inc(&self->header.busy_count);
> -	is_busy = true;
> -
>  	if (frags > AQ_CFG_SKB_FRAGS_MAX) {
>  		dev_kfree_skb_any(skb);
>  		goto err_exit;
> @@ -629,8 +620,6 @@ __acquires(&ring->lock)
>  	}
>
>  err_exit:
> -	if (is_busy)
> -		atomic_dec(&self->header.busy_count);
>  	return err;
>  }
>
> diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_utils.h b/drivers/net/ethernet/aquantia/atlantic/aq_utils.h
> index 4446bd9..f6012b3 100644
> --- a/drivers/net/ethernet/aquantia/atlantic/aq_utils.h
> +++ b/drivers/net/ethernet/aquantia/atlantic/aq_utils.h
> @@ -19,7 +19,6 @@
>  struct aq_obj_s {
>  	spinlock_t lock; /* spinlock for nic/rings processing */
>  	atomic_t flags;
> -	atomic_t busy_count;
>  };
>
>  static inline void aq_utils_obj_set(atomic_t *flags, u32 mask)
>

Reviewed-by: Lino Sanfilippo <LinoSanfilippo@gmx.de>

Regards,
Lino

[PATCH net-next 03/13] net: ethernet: aquantia: Removed unnecessary comparsion "old_mtu == new_mtu".

[Pavel Belous]

From: Pavel Belous <pavel.belous@aquantia.com>

This check is not needed. Function aq_ndev_change_mtu wont be called
if mtu has not changed.

Signed-off-by: Pavel Belous <pavel.belous@aquantia.com>
---
 drivers/net/ethernet/aquantia/atlantic/aq_main.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_main.c b/drivers/net/ethernet/aquantia/atlantic/aq_main.c
index c17c70a..e5539c8 100644
--- a/drivers/net/ethernet/aquantia/atlantic/aq_main.c
+++ b/drivers/net/ethernet/aquantia/atlantic/aq_main.c
@@ -102,17 +102,15 @@ static int aq_ndev_change_mtu(struct net_device *ndev, int new_mtu)
 	struct aq_nic_s *aq_nic = netdev_priv(ndev);
 	int err = 0;
 
-	if (new_mtu == ndev->mtu) {
-		err = 0;
-		goto err_exit;
-	}
 	if (new_mtu < 68) {
 		err = -EINVAL;
 		goto err_exit;
 	}
+
 	err = aq_nic_set_mtu(aq_nic, new_mtu + ETH_HLEN);
 	if (err < 0)
 		goto err_exit;
+
 	ndev->mtu = new_mtu;
 
 	if (netif_running(ndev)) {
-- 
2.7.4

Re: [PATCH net-next 03/13] net: ethernet: aquantia: Removed unnecessary comparsion "old_mtu == new_mtu".

[Lino Sanfilippo]

On 15.02.2017 21:01, Pavel Belous wrote:
> From: Pavel Belous <pavel.belous@aquantia.com>
>
> This check is not needed. Function aq_ndev_change_mtu wont be called
> if mtu has not changed.
>
> Signed-off-by: Pavel Belous <pavel.belous@aquantia.com>
> ---
>  drivers/net/ethernet/aquantia/atlantic/aq_main.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_main.c b/drivers/net/ethernet/aquantia/atlantic/aq_main.c
> index c17c70a..e5539c8 100644
> --- a/drivers/net/ethernet/aquantia/atlantic/aq_main.c
> +++ b/drivers/net/ethernet/aquantia/atlantic/aq_main.c
> @@ -102,17 +102,15 @@ static int aq_ndev_change_mtu(struct net_device *ndev, int new_mtu)
>  	struct aq_nic_s *aq_nic = netdev_priv(ndev);
>  	int err = 0;
>
> -	if (new_mtu == ndev->mtu) {
> -		err = 0;
> -		goto err_exit;
> -	}
>  	if (new_mtu < 68) {
>  		err = -EINVAL;
>  		goto err_exit;
>  	}
> +
>  	err = aq_nic_set_mtu(aq_nic, new_mtu + ETH_HLEN);
>  	if (err < 0)
>  		goto err_exit;
> +
>  	ndev->mtu = new_mtu;
>
>  	if (netif_running(ndev)) {
>

Reviewed-by: Lino Sanfilippo <LinoSanfilippo@gmx.de>

Regards,
Lino

[PATCH net-next 04/13] net: ethernet: aquantia: Using module_pci_driver.

[Pavel Belous]

From: Pavel Belous <pavel.belous@aquantia.com>

Remove boilerplate code by using macro module_pci_driver.

Signed-off-by: Pavel Belous <pavel.belous@aquantia.com>
---
 drivers/net/ethernet/aquantia/atlantic/aq_main.c | 20 +-------------------
 1 file changed, 1 insertion(+), 19 deletions(-)

diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_main.c b/drivers/net/ethernet/aquantia/atlantic/aq_main.c
index e5539c8..f7513cb 100644
--- a/drivers/net/ethernet/aquantia/atlantic/aq_main.c
+++ b/drivers/net/ethernet/aquantia/atlantic/aq_main.c
@@ -250,22 +250,4 @@ static struct pci_driver aq_pci_ops = {
 	.resume = aq_pci_resume,
 };
 
-static int __init aq_module_init(void)
-{
-	int err = 0;
-
-	err = pci_register_driver(&aq_pci_ops);
-	if (err < 0)
-		goto err_exit;
-
-err_exit:
-	return err;
-}
-
-static void __exit aq_module_exit(void)
-{
-	pci_unregister_driver(&aq_pci_ops);
-}
-
-module_init(aq_module_init);
-module_exit(aq_module_exit);
+module_pci_driver(aq_pci_ops);
-- 
2.7.4

Re: [PATCH net-next 04/13] net: ethernet: aquantia: Using module_pci_driver.

[Lino Sanfilippo]

On 15.02.2017 21:01, Pavel Belous wrote:
> From: Pavel Belous <pavel.belous@aquantia.com>
>
> Remove boilerplate code by using macro module_pci_driver.
>
> Signed-off-by: Pavel Belous <pavel.belous@aquantia.com>
> ---
>  drivers/net/ethernet/aquantia/atlantic/aq_main.c | 20 +-------------------
>  1 file changed, 1 insertion(+), 19 deletions(-)
>
> diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_main.c b/drivers/net/ethernet/aquantia/atlantic/aq_main.c
> index e5539c8..f7513cb 100644
> --- a/drivers/net/ethernet/aquantia/atlantic/aq_main.c
> +++ b/drivers/net/ethernet/aquantia/atlantic/aq_main.c
> @@ -250,22 +250,4 @@ static struct pci_driver aq_pci_ops = {
>  	.resume = aq_pci_resume,
>  };
>
> -static int __init aq_module_init(void)
> -{
> -	int err = 0;
> -
> -	err = pci_register_driver(&aq_pci_ops);
> -	if (err < 0)
> -		goto err_exit;
> -
> -err_exit:
> -	return err;
> -}
> -
> -static void __exit aq_module_exit(void)
> -{
> -	pci_unregister_driver(&aq_pci_ops);
> -}
> -
> -module_init(aq_module_init);
> -module_exit(aq_module_exit);
> +module_pci_driver(aq_pci_ops);
>

Reviewed-by: Lino Sanfilippo <LinoSanfilippo@gmx.de>

Regards,
Lino

[PATCH net-next 05/13] net: ethernet: aquantia: Superfluous initialization of "err".

[Pavel Belous]

From: Pavel Belous <pavel.belous@aquantia.com>

Fixed superfluous initialization of err.

Signed-off-by: Pavel Belous <pavel.belous@aquantia.com>
---
 drivers/net/ethernet/aquantia/atlantic/aq_main.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_main.c b/drivers/net/ethernet/aquantia/atlantic/aq_main.c
index f7513cb..68c19d3 100644
--- a/drivers/net/ethernet/aquantia/atlantic/aq_main.c
+++ b/drivers/net/ethernet/aquantia/atlantic/aq_main.c
@@ -87,14 +87,8 @@ static int aq_ndev_close(struct net_device *ndev)
 static int aq_ndev_start_xmit(struct sk_buff *skb, struct net_device *ndev)
 {
 	struct aq_nic_s *aq_nic = netdev_priv(ndev);
-	int err = 0;
-
-	err = aq_nic_xmit(aq_nic, skb);
-	if (err < 0)
-		goto err_exit;
 
-err_exit:
-	return err;
+	return aq_nic_xmit(aq_nic, skb);
 }
 
 static int aq_ndev_change_mtu(struct net_device *ndev, int new_mtu)
-- 
2.7.4

Re: [PATCH net-next 05/13] net: ethernet: aquantia: Superfluous initialization of "err".

[Lino Sanfilippo]

On 15.02.2017 21:01, Pavel Belous wrote:
> From: Pavel Belous <pavel.belous@aquantia.com>
>
> Fixed superfluous initialization of err.
>
> Signed-off-by: Pavel Belous <pavel.belous@aquantia.com>
> ---
>  drivers/net/ethernet/aquantia/atlantic/aq_main.c | 8 +-------
>  1 file changed, 1 insertion(+), 7 deletions(-)
>
> diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_main.c b/drivers/net/ethernet/aquantia/atlantic/aq_main.c
> index f7513cb..68c19d3 100644
> --- a/drivers/net/ethernet/aquantia/atlantic/aq_main.c
> +++ b/drivers/net/ethernet/aquantia/atlantic/aq_main.c
> @@ -87,14 +87,8 @@ static int aq_ndev_close(struct net_device *ndev)
>  static int aq_ndev_start_xmit(struct sk_buff *skb, struct net_device *ndev)
>  {
>  	struct aq_nic_s *aq_nic = netdev_priv(ndev);
> -	int err = 0;
> -
> -	err = aq_nic_xmit(aq_nic, skb);
> -	if (err < 0)
> -		goto err_exit;
>
> -err_exit:
> -	return err;
> +	return aq_nic_xmit(aq_nic, skb);
>  }
>
>  static int aq_ndev_change_mtu(struct net_device *ndev, int new_mtu)
>

Just a nitpick: Now that ndev_start_xmit() is only an empty wrapper for
aq_nic_xmit() you can toss it completely and call aq_nic_xmit directly.

Reviewed-by: Lino Sanfilippo <LinoSanfilippo@gmx.de>

Regards,
Lino

[PATCH net-next 06/13] net: ethernet: aquantia: Fixed missing rtnl_unlock.

[Pavel Belous]

From: Pavel Belous <pavel.belous@aquantia.com>

rtnl_unlock should be called if error occurred.

Signed-off-by: Pavel Belous <pavel.belous@aquantia.com>
---
 drivers/net/ethernet/aquantia/atlantic/aq_nic.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_nic.c b/drivers/net/ethernet/aquantia/atlantic/aq_nic.c
index a153750..1bf5975 100644
--- a/drivers/net/ethernet/aquantia/atlantic/aq_nic.c
+++ b/drivers/net/ethernet/aquantia/atlantic/aq_nic.c
@@ -931,7 +931,7 @@ int aq_nic_change_pm_state(struct aq_nic_s *self, pm_message_t *pm_msg)
 
 	if (!netif_running(self->ndev)) {
 		err = 0;
-		goto err_exit;
+		goto out;
 	}
 	rtnl_lock();
 	if (pm_msg->event & PM_EVENT_SLEEP || pm_msg->event & PM_EVENT_FREEZE) {
@@ -956,8 +956,9 @@ int aq_nic_change_pm_state(struct aq_nic_s *self, pm_message_t *pm_msg)
 		netif_device_attach(self->ndev);
 		netif_tx_start_all_queues(self->ndev);
 	}
-	rtnl_unlock();
 
 err_exit:
+	rtnl_unlock();
+out:
 	return err;
 }
-- 
2.7.4

Re: [PATCH net-next 06/13] net: ethernet: aquantia: Fixed missing rtnl_unlock.

[Lino Sanfilippo]

On 15.02.2017 21:01, Pavel Belous wrote:
> From: Pavel Belous <pavel.belous@aquantia.com>
>
> rtnl_unlock should be called if error occurred.
>
> Signed-off-by: Pavel Belous <pavel.belous@aquantia.com>
> ---
>  drivers/net/ethernet/aquantia/atlantic/aq_nic.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_nic.c b/drivers/net/ethernet/aquantia/atlantic/aq_nic.c
> index a153750..1bf5975 100644
> --- a/drivers/net/ethernet/aquantia/atlantic/aq_nic.c
> +++ b/drivers/net/ethernet/aquantia/atlantic/aq_nic.c
> @@ -931,7 +931,7 @@ int aq_nic_change_pm_state(struct aq_nic_s *self, pm_message_t *pm_msg)
>
>  	if (!netif_running(self->ndev)) {
>  		err = 0;
> -		goto err_exit;
> +		goto out;
>  	}
>  	rtnl_lock();
>  	if (pm_msg->event & PM_EVENT_SLEEP || pm_msg->event & PM_EVENT_FREEZE) {
> @@ -956,8 +956,9 @@ int aq_nic_change_pm_state(struct aq_nic_s *self, pm_message_t *pm_msg)
>  		netif_device_attach(self->ndev);
>  		netif_tx_start_all_queues(self->ndev);
>  	}
> -	rtnl_unlock();
>
>  err_exit:
> +	rtnl_unlock();
> +out:
>  	return err;
>  }
>

Reviewed-by: Lino Sanfilippo <LinoSanfilippo@gmx.de>

Regards,
Lino

[PATCH net-next 07/13] net: ethernet: aquantia: Using NETDEV_TX_OK instead 0.

[Pavel Belous]

From: Pavel Belous <pavel.belous@aquantia.com>

Use NETDEV_TX_OK as the return value for successful transmission.

Signed-off-by: Pavel Belous <pavel.belous@aquantia.com>
---
 drivers/net/ethernet/aquantia/atlantic/aq_nic.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_nic.c b/drivers/net/ethernet/aquantia/atlantic/aq_nic.c
index 1bf5975..4cf633c 100644
--- a/drivers/net/ethernet/aquantia/atlantic/aq_nic.c
+++ b/drivers/net/ethernet/aquantia/atlantic/aq_nic.c
@@ -567,7 +567,7 @@ __acquires(&ring->lock)
 	unsigned int vec = skb->queue_mapping % self->aq_nic_cfg.vecs;
 	unsigned int tc = 0U;
 	unsigned int trys = AQ_CFG_LOCK_TRYS;
-	int err = 0;
+	int err = NETDEV_TX_OK;
 	bool is_nic_in_bad_state;
 	struct aq_ring_buff_s buffers[AQ_CFG_SKB_FRAGS_MAX];
 
-- 
2.7.4

Re: [PATCH net-next 07/13] net: ethernet: aquantia: Using NETDEV_TX_OK instead 0.

[Lino Sanfilippo]

On 15.02.2017 21:01, Pavel Belous wrote:
> From: Pavel Belous <pavel.belous@aquantia.com>
>
> Use NETDEV_TX_OK as the return value for successful transmission.
>
> Signed-off-by: Pavel Belous <pavel.belous@aquantia.com>
> ---
>  drivers/net/ethernet/aquantia/atlantic/aq_nic.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_nic.c b/drivers/net/ethernet/aquantia/atlantic/aq_nic.c
> index 1bf5975..4cf633c 100644
> --- a/drivers/net/ethernet/aquantia/atlantic/aq_nic.c
> +++ b/drivers/net/ethernet/aquantia/atlantic/aq_nic.c
> @@ -567,7 +567,7 @@ __acquires(&ring->lock)
>  	unsigned int vec = skb->queue_mapping % self->aq_nic_cfg.vecs;
>  	unsigned int tc = 0U;
>  	unsigned int trys = AQ_CFG_LOCK_TRYS;
> -	int err = 0;
> +	int err = NETDEV_TX_OK;
>  	bool is_nic_in_bad_state;
>  	struct aq_ring_buff_s buffers[AQ_CFG_SKB_FRAGS_MAX];
>
>

Reviewed-by: Lino Sanfilippo <LinoSanfilippo@gmx.de>

Regards,
Lino

[PATCH net-next 08/13] net: ethernet: aquantia: Null pointer check for aq_nic_ndev_alloc.

[Pavel Belous]

From: Pavel Belous <pavel.belous@aquantia.com>

We should check for a null pointer for aq_nic_ndev_alloc
instead netdev_priv.

Signed-off-by: Pavel Belous <pavel.belous@aquantia.com>
---
 drivers/net/ethernet/aquantia/atlantic/aq_nic.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_nic.c b/drivers/net/ethernet/aquantia/atlantic/aq_nic.c
index 4cf633c..e50fba2 100644
--- a/drivers/net/ethernet/aquantia/atlantic/aq_nic.c
+++ b/drivers/net/ethernet/aquantia/atlantic/aq_nic.c
@@ -202,12 +202,13 @@ struct aq_nic_s *aq_nic_alloc_cold(const struct net_device_ops *ndev_ops,
 	int err = 0;
 
 	ndev = aq_nic_ndev_alloc();
-	self = netdev_priv(ndev);
-	if (!self) {
-		err = -EINVAL;
+	if (!ndev) {
+		err = -ENOMEM;
 		goto err_exit;
 	}
 
+	self = netdev_priv(ndev);
+
 	ndev->netdev_ops = ndev_ops;
 	ndev->ethtool_ops = et_ops;
 
-- 
2.7.4

Re: [PATCH net-next 08/13] net: ethernet: aquantia: Null pointer check for aq_nic_ndev_alloc.

[Lino Sanfilippo]

On 15.02.2017 21:01, Pavel Belous wrote:
> From: Pavel Belous <pavel.belous@aquantia.com>
>
> We should check for a null pointer for aq_nic_ndev_alloc
> instead netdev_priv.
>
> Signed-off-by: Pavel Belous <pavel.belous@aquantia.com>
> ---
>  drivers/net/ethernet/aquantia/atlantic/aq_nic.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_nic.c b/drivers/net/ethernet/aquantia/atlantic/aq_nic.c
> index 4cf633c..e50fba2 100644
> --- a/drivers/net/ethernet/aquantia/atlantic/aq_nic.c
> +++ b/drivers/net/ethernet/aquantia/atlantic/aq_nic.c
> @@ -202,12 +202,13 @@ struct aq_nic_s *aq_nic_alloc_cold(const struct net_device_ops *ndev_ops,
>  	int err = 0;
>
>  	ndev = aq_nic_ndev_alloc();
> -	self = netdev_priv(ndev);
> -	if (!self) {
> -		err = -EINVAL;
> +	if (!ndev) {
> +		err = -ENOMEM;
>  		goto err_exit;
>  	}
>
> +	self = netdev_priv(ndev);
> +
>  	ndev->netdev_ops = ndev_ops;
>  	ndev->ethtool_ops = et_ops;
>
>

Reviewed-by: Lino Sanfilippo <LinoSanfilippo@gmx.de>

Regards,
Lino

[PATCH net-next 09/13] net: ethernet: aquantia: Call netdev_register after all initialized.

[Pavel Belous]

From: Pavel Belous <pavel.belous@aquantia.com>

netdev_register should be called when everything is initialized.

Signed-off-by: Pavel Belous <pavel.belous@aquantia.com>
---
 drivers/net/ethernet/aquantia/atlantic/aq_nic.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_nic.c b/drivers/net/ethernet/aquantia/atlantic/aq_nic.c
index e50fba2..daed4c1 100644
--- a/drivers/net/ethernet/aquantia/atlantic/aq_nic.c
+++ b/drivers/net/ethernet/aquantia/atlantic/aq_nic.c
@@ -260,16 +260,18 @@ int aq_nic_ndev_register(struct aq_nic_s *self)
 		ether_addr_copy(self->ndev->dev_addr, mac_addr_permanent);
 	}
 #endif
-	err = register_netdev(self->ndev);
-	if (err < 0)
-		goto err_exit;
 
-	self->is_ndev_registered = true;
 	netif_carrier_off(self->ndev);
 
 	for (i = AQ_CFG_VECS_MAX; i--;)
 		aq_nic_ndev_queue_stop(self, i);
 
+	err = register_netdev(self->ndev);
+	if (err < 0)
+		goto err_exit;
+
+	self->is_ndev_registered = true;
+
 err_exit:
 	return err;
 }
-- 
2.7.4

Re: [PATCH net-next 09/13] net: ethernet: aquantia: Call netdev_register after all initialized.

[Lino Sanfilippo]

On 15.02.2017 21:01, Pavel Belous wrote:
> From: Pavel Belous <pavel.belous@aquantia.com>
>
> netdev_register should be called when everything is initialized.
>
> Signed-off-by: Pavel Belous <pavel.belous@aquantia.com>
> ---
>  drivers/net/ethernet/aquantia/atlantic/aq_nic.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_nic.c b/drivers/net/ethernet/aquantia/atlantic/aq_nic.c
> index e50fba2..daed4c1 100644
> --- a/drivers/net/ethernet/aquantia/atlantic/aq_nic.c
> +++ b/drivers/net/ethernet/aquantia/atlantic/aq_nic.c
> @@ -260,16 +260,18 @@ int aq_nic_ndev_register(struct aq_nic_s *self)
>  		ether_addr_copy(self->ndev->dev_addr, mac_addr_permanent);
>  	}
>  #endif
> -	err = register_netdev(self->ndev);
> -	if (err < 0)
> -		goto err_exit;
>
> -	self->is_ndev_registered = true;
>  	netif_carrier_off(self->ndev);
>
>  	for (i = AQ_CFG_VECS_MAX; i--;)
>  		aq_nic_ndev_queue_stop(self, i);
>
> +	err = register_netdev(self->ndev);
> +	if (err < 0)
> +		goto err_exit;
> +
> +	self->is_ndev_registered = true;
> +
>  err_exit:
>  	return err;
>  }
>

Reviewed-by: Lino Sanfilippo <LinoSanfilippo@gmx.de>

Regards,
Lino

[PATCH net-next 10/13] net: ethernet: aquantia: Checking for success dma_map_single.

[Pavel Belous]

From: Pavel Belous <pavel.belous@aquantia.com>

Dma mapping can fail. We should check the result.

Signed-off-by: Pavel Belous <pavel.belous@aquantia.com>
---
 drivers/net/ethernet/aquantia/atlantic/aq_nic.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_nic.c b/drivers/net/ethernet/aquantia/atlantic/aq_nic.c
index daed4c1..7000b9f 100644
--- a/drivers/net/ethernet/aquantia/atlantic/aq_nic.c
+++ b/drivers/net/ethernet/aquantia/atlantic/aq_nic.c
@@ -476,11 +476,15 @@ static unsigned int aq_nic_map_skb_frag(struct aq_nic_s *self,
 	unsigned int ret = 0U;
 	unsigned int nr_frags = skb_shinfo(skb)->nr_frags;
 	unsigned int frag_count = 0U;
+	struct device *dev = aq_nic_get_dev(self);
 
 	dx->flags = 0U;
 	dx->len = skb_headlen(skb);
-	dx->pa = dma_map_single(aq_nic_get_dev(self), skb->data, dx->len,
-				DMA_TO_DEVICE);
+	dx->pa = dma_map_single(dev, skb->data, dx->len, DMA_TO_DEVICE);
+
+	if (unlikely(dma_mapping_error(dev, dx->pa)))
+		goto err_exit;
+
 	dx->len_pkt = skb->len;
 	dx->is_sop = 1U;
 	dx->is_mapped = 1U;
@@ -502,8 +506,10 @@ static unsigned int aq_nic_map_skb_frag(struct aq_nic_s *self,
 
 		frag_len = skb_frag_size(frag);
 
-		frag_pa = skb_frag_dma_map(aq_nic_get_dev(self), frag, 0,
+		frag_pa = skb_frag_dma_map(dev, frag, 0,
 					   frag_len, DMA_TO_DEVICE);
+		if (unlikely(dma_mapping_error(dev, frag_pa)))
+			goto err_exit;
 
 		while (frag_len > AQ_CFG_TX_FRAME_MAX) {
 			++dx;
@@ -529,6 +535,7 @@ static unsigned int aq_nic_map_skb_frag(struct aq_nic_s *self,
 	dx->is_eop = 1U;
 	dx->skb = skb;
 
+err_exit:
 	return ret;
 }
 
-- 
2.7.4

Re: [PATCH net-next 10/13] net: ethernet: aquantia: Checking for success dma_map_single.

[Lino Sanfilippo]

On 15.02.2017 21:01, Pavel Belous wrote:
> From: Pavel Belous <pavel.belous@aquantia.com>
>
> Dma mapping can fail. We should check the result.
>
> Signed-off-by: Pavel Belous <pavel.belous@aquantia.com>
> ---
>  drivers/net/ethernet/aquantia/atlantic/aq_nic.c | 13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_nic.c b/drivers/net/ethernet/aquantia/atlantic/aq_nic.c
> index daed4c1..7000b9f 100644
> --- a/drivers/net/ethernet/aquantia/atlantic/aq_nic.c
> +++ b/drivers/net/ethernet/aquantia/atlantic/aq_nic.c
> @@ -476,11 +476,15 @@ static unsigned int aq_nic_map_skb_frag(struct aq_nic_s *self,
>  	unsigned int ret = 0U;
>  	unsigned int nr_frags = skb_shinfo(skb)->nr_frags;
>  	unsigned int frag_count = 0U;
> +	struct device *dev = aq_nic_get_dev(self);
>
>  	dx->flags = 0U;
>  	dx->len = skb_headlen(skb);
> -	dx->pa = dma_map_single(aq_nic_get_dev(self), skb->data, dx->len,
> -				DMA_TO_DEVICE);
> +	dx->pa = dma_map_single(dev, skb->data, dx->len, DMA_TO_DEVICE);
> +
> +	if (unlikely(dma_mapping_error(dev, dx->pa)))
> +		goto err_exit;
> +
>  	dx->len_pkt = skb->len;
>  	dx->is_sop = 1U;
>  	dx->is_mapped = 1U;
> @@ -502,8 +506,10 @@ static unsigned int aq_nic_map_skb_frag(struct aq_nic_s *self,
>
>  		frag_len = skb_frag_size(frag);
>
> -		frag_pa = skb_frag_dma_map(aq_nic_get_dev(self), frag, 0,
> +		frag_pa = skb_frag_dma_map(dev, frag, 0,
>  					   frag_len, DMA_TO_DEVICE);
> +		if (unlikely(dma_mapping_error(dev, frag_pa)))
> +			goto err_exit;


In case of this error you have to undo all mappings that you have
done so far (i.e the complete frag list and the head buffer).


>  		while (frag_len > AQ_CFG_TX_FRAME_MAX) {
>  			++dx;
> @@ -529,6 +535,7 @@ static unsigned int aq_nic_map_skb_frag(struct aq_nic_s *self,
>  	dx->is_eop = 1U;
>  	dx->skb = skb;
>
> +err_exit:
>  	return ret;
>  }
>
>

Regards,
Lino

Re: [PATCH net-next 10/13] net: ethernet: aquantia: Checking for success dma_map_single.

[Lino Sanfilippo]

On 15.02.2017 22:23, Lino Sanfilippo wrote:

>
>
> In case of this error you have to undo all mappings that you have
> done so far (i.e the complete frag list and the head buffer).
>

And, since mapping failed, set ret = 0 and handle this case in the
caller, too.

Regards,
Lino

Re: [PATCH net-next 10/13] net: ethernet: aquantia: Checking for success dma_map_single.

[Pavel Belous]

Ok, I will fix it in the patch v2.

Thank you.

Regards,
Pavel

On 16.02.2017 00:40, Lino Sanfilippo wrote:
> On 15.02.2017 22:23, Lino Sanfilippo wrote:
>
>>
>>
>> In case of this error you have to undo all mappings that you have
>> done so far (i.e the complete frag list and the head buffer).
>>
>
> And, since mapping failed, set ret = 0 and handle this case in the
> caller, too.
>
> Regards,
> Lino
>

[PATCH net-next 11/13] net: ethernet: aquantia: Refactoring buffers copying.

[Pavel Belous]

From: Pavel Belous <pavel.belous@aquantia.com>

This fix simplified copying data to the ring buffer.
Also, there was an error in the code when the second memcpy
is called with zero length. It didn't break the driver, but it's bad.

Signed-off-by: Pavel Belous <pavel.belous@aquantia.com>
---
 drivers/net/ethernet/aquantia/atlantic/aq_ring.c | 25 ++++++++++++------------
 1 file changed, 13 insertions(+), 12 deletions(-)

diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_ring.c b/drivers/net/ethernet/aquantia/atlantic/aq_ring.c
index 4c40644..8ebed0d 100644
--- a/drivers/net/ethernet/aquantia/atlantic/aq_ring.c
+++ b/drivers/net/ethernet/aquantia/atlantic/aq_ring.c
@@ -108,18 +108,19 @@ void aq_ring_tx_append_buffs(struct aq_ring_s *self,
 			     struct aq_ring_buff_s *buffer,
 			     unsigned int buffers)
 {
-	if (likely(self->sw_tail + buffers < self->size)) {
-		memcpy(&self->buff_ring[self->sw_tail], buffer,
-		       sizeof(buffer[0]) * buffers);
-	} else {
-		unsigned int first_part = self->size - self->sw_tail;
-		unsigned int second_part = buffers - first_part;
-
-		memcpy(&self->buff_ring[self->sw_tail], buffer,
-		       sizeof(buffer[0]) * first_part);
-
-		memcpy(&self->buff_ring[0], &buffer[first_part],
-		       sizeof(buffer[0]) * second_part);
+	int buff_len = min(self->size - self->sw_tail, buffers);
+
+	memcpy(&self->buff_ring[self->sw_tail],
+	       buffer,
+	       sizeof(struct aq_ring_buff_s) * buff_len);
+
+	/* We are in the end of the ring.
+	 *  Copy remains data to beginning of the ring
+	 */
+	if (buffers > buff_len) {
+		memcpy(self->buff_ring,
+		       &buffer[buff_len],
+		       sizeof(struct aq_ring_buff_s) * (buffers - buff_len));
 	}
 }
 
-- 
2.7.4

Re: [PATCH net-next 11/13] net: ethernet: aquantia: Refactoring buffers copying.

[Lino Sanfilippo]

On 15.02.2017 21:01, Pavel Belous wrote:
> From: Pavel Belous <pavel.belous@aquantia.com>
>
> This fix simplified copying data to the ring buffer.
> Also, there was an error in the code when the second memcpy
> is called with zero length. It didn't break the driver, but it's bad.
>
> Signed-off-by: Pavel Belous <pavel.belous@aquantia.com>
> ---
>  drivers/net/ethernet/aquantia/atlantic/aq_ring.c | 25 ++++++++++++------------
>  1 file changed, 13 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_ring.c b/drivers/net/ethernet/aquantia/atlantic/aq_ring.c
> index 4c40644..8ebed0d 100644
> --- a/drivers/net/ethernet/aquantia/atlantic/aq_ring.c
> +++ b/drivers/net/ethernet/aquantia/atlantic/aq_ring.c
> @@ -108,18 +108,19 @@ void aq_ring_tx_append_buffs(struct aq_ring_s *self,
>  			     struct aq_ring_buff_s *buffer,
>  			     unsigned int buffers)
>  {
> -	if (likely(self->sw_tail + buffers < self->size)) {
> -		memcpy(&self->buff_ring[self->sw_tail], buffer,
> -		       sizeof(buffer[0]) * buffers);
> -	} else {
> -		unsigned int first_part = self->size - self->sw_tail;
> -		unsigned int second_part = buffers - first_part;
> -
> -		memcpy(&self->buff_ring[self->sw_tail], buffer,
> -		       sizeof(buffer[0]) * first_part);
> -
> -		memcpy(&self->buff_ring[0], &buffer[first_part],
> -		       sizeof(buffer[0]) * second_part);
> +	int buff_len = min(self->size - self->sw_tail, buffers);
> +
> +	memcpy(&self->buff_ring[self->sw_tail],
> +	       buffer,
> +	       sizeof(struct aq_ring_buff_s) * buff_len);
> +
> +	/* We are in the end of the ring.
> +	 *  Copy remains data to beginning of the ring
> +	 */
> +	if (buffers > buff_len) {
> +		memcpy(self->buff_ring,
> +		       &buffer[buff_len],
> +		       sizeof(struct aq_ring_buff_s) * (buffers - buff_len));
>  	}
>  }

Well, you should really try to avoid copying the tx buffers _at all_.
E.g. by passing self->buff_ring to aq_ring_tx_append_buffs() instead of
the temporary array.

Regards,
Lino
buffer array.

Re: [PATCH net-next 11/13] net: ethernet: aquantia: Refactoring buffers copying.

[Pavel Belous]

Thank you.

I will think about how to avoid copying..

Regards,
Pavel


On 16.02.2017 00:31, Lino Sanfilippo wrote:
> On 15.02.2017 21:01, Pavel Belous wrote:
>> From: Pavel Belous <pavel.belous@aquantia.com>
>>
>> This fix simplified copying data to the ring buffer.
>> Also, there was an error in the code when the second memcpy
>> is called with zero length. It didn't break the driver, but it's bad.
>>
>> Signed-off-by: Pavel Belous <pavel.belous@aquantia.com>
>> ---
>>  drivers/net/ethernet/aquantia/atlantic/aq_ring.c | 25
>> ++++++++++++------------
>>  1 file changed, 13 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_ring.c
>> b/drivers/net/ethernet/aquantia/atlantic/aq_ring.c
>> index 4c40644..8ebed0d 100644
>> --- a/drivers/net/ethernet/aquantia/atlantic/aq_ring.c
>> +++ b/drivers/net/ethernet/aquantia/atlantic/aq_ring.c
>> @@ -108,18 +108,19 @@ void aq_ring_tx_append_buffs(struct aq_ring_s
>> *self,
>>                   struct aq_ring_buff_s *buffer,
>>                   unsigned int buffers)
>>  {
>> -    if (likely(self->sw_tail + buffers < self->size)) {
>> -        memcpy(&self->buff_ring[self->sw_tail], buffer,
>> -               sizeof(buffer[0]) * buffers);
>> -    } else {
>> -        unsigned int first_part = self->size - self->sw_tail;
>> -        unsigned int second_part = buffers - first_part;
>> -
>> -        memcpy(&self->buff_ring[self->sw_tail], buffer,
>> -               sizeof(buffer[0]) * first_part);
>> -
>> -        memcpy(&self->buff_ring[0], &buffer[first_part],
>> -               sizeof(buffer[0]) * second_part);
>> +    int buff_len = min(self->size - self->sw_tail, buffers);
>> +
>> +    memcpy(&self->buff_ring[self->sw_tail],
>> +           buffer,
>> +           sizeof(struct aq_ring_buff_s) * buff_len);
>> +
>> +    /* We are in the end of the ring.
>> +     *  Copy remains data to beginning of the ring
>> +     */
>> +    if (buffers > buff_len) {
>> +        memcpy(self->buff_ring,
>> +               &buffer[buff_len],
>> +               sizeof(struct aq_ring_buff_s) * (buffers - buff_len));
>>      }
>>  }
>
> Well, you should really try to avoid copying the tx buffers _at all_.
> E.g. by passing self->buff_ring to aq_ring_tx_append_buffs() instead of
> the temporary array.
>
> Regards,
> Lino
> buffer array.
>

RE: [PATCH net-next 11/13] net: ethernet: aquantia: Refactoring buffers copying.

[David Laight]

From: Lino Sanfilippo
> Sent: 15 February 2017 21:31
...
> Well, you should really try to avoid copying the tx buffers _at all_.
> E.g. by passing self->buff_ring to aq_ring_tx_append_buffs() instead of
> the temporary array.

Copying can help for horridly fragmented frames or when iommu (etc)
setup is expensive.
At least some ethernet hardware has minimum fragment lengths.

However, if the frame doesn't fit at the end - just copy to the front.

Something should ensure the copies are aligned (you may want gaps
between frames.)

I presume there is a 'space in buffer' check elsewhere.

	David

Re: RE: [PATCH net-next 11/13] net: ethernet: aquantia: Refactoring buffers copying.

[Lino Sanfilippo]

Hi,

> ...
> > Well, you should really try to avoid copying the tx buffers _at all_.
> > E.g. by passing self->buff_ring to aq_ring_tx_append_buffs() instead of
> > the temporary array.
> 
> Copying can help for horridly fragmented frames or when iommu (etc)
> setup is expensive.
> At least some ethernet hardware has minimum fragment lengths.
> 

I was referring to the copy of tx descriptors, not the frames/fragments itself. 
I wrote "tx buffers" because in this driver a descriptor is represented as
 a struct "aq_ring_buff_s". I cannot see a reason why this descriptor copy
 should be necessary.
However I agree that there can be circumstances that would justify even the
copy of frame/fragment data.

Regards,
Lino

RE: RE: [PATCH net-next 11/13] net: ethernet: aquantia: Refactoring buffers copying.

[David Laight]

From: Lino Sanfilippo
> Sent: 16 February 2017 16:02
...
> I was referring to the copy of tx descriptors, not the frames/fragments itself.
> I wrote "tx buffers" because in this driver a descriptor is represented as
>  a struct "aq_ring_buff_s". I cannot see a reason why this descriptor copy
>  should be necessary.

Not unless the descriptor memory is on the card.

Actually it might be worse than that.
The transmit descriptor probably has an 'owner' bit.
The write of the word containing that bit must be last and
preceded by the appropriate barrier (probably dma_wmb()).
Another barrier is needed before the 'doorbell' io write that kicks
the hardware.

Using memcpy() won't have that effect at all.

If the ethernet frame is fragmented (and so uses multiple
descriptors) the owner bit of the first descriptor must be
in the last write.

	David

Re: [PATCH net-next 11/13] net: ethernet: aquantia: Refactoring buffers copying.

[Lino Sanfilippo]

Hi,

On 16.02.2017 17:37, David Laight wrote:
> From: Lino Sanfilippo
>> Sent: 16 February 2017 16:02
> ...
>> I was referring to the copy of tx descriptors, not the frames/fragments itself.
>> I wrote "tx buffers" because in this driver a descriptor is represented as
>>  a struct "aq_ring_buff_s". I cannot see a reason why this descriptor copy
>>  should be necessary.
> 
> Not unless the descriptor memory is on the card.
> 
> Actually it might be worse than that.
> The transmit descriptor probably has an 'owner' bit.
> The write of the word containing that bit must be last and
> preceded by the appropriate barrier (probably dma_wmb()).
> Another barrier is needed before the 'doorbell' io write that kicks
> the hardware.
> 

After taking a look at the concerning code parts, I can say that there is
at least no barrier to ensure dma flush before the doorbell.
What about the patch below to fix this?

Regards,
Lino

Subject: [PATCH] net: aquantia: add memory barrier before tx descriptors are
 passed to the hardware

Add a memory barrier to ensure that tx descriptors are written completely
before the descriptors are passed to the hardware.

Signed-off-by: Lino Sanfilippo <LinoSanfilippo@gmx.de>
Suggested-by: David Laight <David.Laight@ACULAB.COM>
---
 drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_llh.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_llh.c b/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_llh.c
index 3de651a..c3d73e7 100644
--- a/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_llh.c
+++ b/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_llh.c
@@ -526,6 +526,8 @@ void reg_tx_dma_desc_base_addressmswset(struct aq_hw_s *aq_hw,
 void reg_tx_dma_desc_tail_ptr_set(struct aq_hw_s *aq_hw,
 				  u32 tx_dma_desc_tail_ptr, u32 descriptor)
 {
+	/* make sure writes to dma descriptors are completed */
+	wmb();
 	aq_hw_write_reg(aq_hw, tx_dma_desc_tail_ptr_adr(descriptor),
 			tx_dma_desc_tail_ptr);
 }
-- 
2.7.4

[PATCH net-next 12/13] net: ethernet: aquantia: Fixed incorrect buff->len calculation.

[Pavel Belous]

From: Pavel Belous <pavel.belous@aquantia.com>

rxd_wb->pkt_len is the total length of the packet.
If we received a large packet (with length > AQ_CFG_RX_FRAME_MAX) then we
will get multiple buffers. We need to fix the length of the last buffer.

Signed-off-by: Pavel Belous <pavel.belous@aquantia.com>
---
 drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_a0.c | 4 ++--
 drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_b0.c | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

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 1f38805..a2b746a 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
@@ -659,8 +659,8 @@ static int hw_atl_a0_hw_ring_rx_receive(struct aq_hw_s *self,
 			}
 
 			if (HW_ATL_A0_RXD_WB_STAT2_EOP & rxd_wb->status) {
-				buff->len = (rxd_wb->pkt_len &
-						(AQ_CFG_RX_FRAME_MAX - 1U));
+				buff->len = rxd_wb->pkt_len %
+					AQ_CFG_RX_FRAME_MAX;
 				buff->len = buff->len ?
 					buff->len : AQ_CFG_RX_FRAME_MAX;
 				buff->next = 0U;
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 e7e694f..cab2931 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
@@ -673,8 +673,8 @@ static int hw_atl_b0_hw_ring_rx_receive(struct aq_hw_s *self,
 			}
 
 			if (HW_ATL_B0_RXD_WB_STAT2_EOP & rxd_wb->status) {
-				buff->len = (rxd_wb->pkt_len &
-						(AQ_CFG_RX_FRAME_MAX - 1U));
+				buff->len = rxd_wb->pkt_len %
+					AQ_CFG_RX_FRAME_MAX;
 				buff->len = buff->len ?
 					buff->len : AQ_CFG_RX_FRAME_MAX;
 				buff->next = 0U;
-- 
2.7.4

[PATCH net-next 13/13] net: ethernet: aquantia: Fixed memory allocation if AQ_CFG_RX_FRAME_MAX > 1 page.

[Pavel Belous]

From: Pavel Belous <pavel.belous@aquantia.com>

We should allocate the number of pages based on the config parameter
AQ_CFG_RX_FRAME_MAX.

Signed-off-by: Pavel Belous <pavel.belous@aquantia.com>
---
 drivers/net/ethernet/aquantia/atlantic/aq_nic.c  | 4 ++--
 drivers/net/ethernet/aquantia/atlantic/aq_ring.c | 4 +++-
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_nic.c b/drivers/net/ethernet/aquantia/atlantic/aq_nic.c
index 7000b9f..5e10732 100644
--- a/drivers/net/ethernet/aquantia/atlantic/aq_nic.c
+++ b/drivers/net/ethernet/aquantia/atlantic/aq_nic.c
@@ -603,9 +603,9 @@ __acquires(&ring->lock)
 
 	do {
 		if (spin_trylock(&ring->header.lock)) {
-			frags = aq_nic_map_skb(self, skb, &buffers[0]);
+			frags = aq_nic_map_skb(self, skb, buffers);
 
-			aq_ring_tx_append_buffs(ring, &buffers[0], frags);
+			aq_ring_tx_append_buffs(ring, buffers, frags);
 
 			err = self->aq_hw_ops.hw_ring_tx_xmit(self->aq_hw,
 							      ring, frags);
diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_ring.c b/drivers/net/ethernet/aquantia/atlantic/aq_ring.c
index 8ebed0d..b747d10 100644
--- a/drivers/net/ethernet/aquantia/atlantic/aq_ring.c
+++ b/drivers/net/ethernet/aquantia/atlantic/aq_ring.c
@@ -279,6 +279,8 @@ int aq_ring_rx_fill(struct aq_ring_s *self)
 	struct aq_ring_buff_s *buff = NULL;
 	int err = 0;
 	int i = 0;
+	unsigned int pages_order = fls(AQ_CFG_RX_FRAME_MAX / PAGE_SIZE +
+		(AQ_CFG_RX_FRAME_MAX % PAGE_SIZE ? 1 : 0)) - 1;
 
 	for (i = aq_ring_avail_dx(self); i--;
 		self->sw_tail = aq_ring_next_dx(self, self->sw_tail)) {
@@ -288,7 +290,7 @@ int aq_ring_rx_fill(struct aq_ring_s *self)
 		buff->len = AQ_CFG_RX_FRAME_MAX;
 
 		buff->page = alloc_pages(GFP_ATOMIC | __GFP_COLD |
-					 __GFP_COMP, 0);
+					 __GFP_COMP, pages_order);
 		if (!buff->page) {
 			err = -ENOMEM;
 			goto err_exit;
-- 
2.7.4