[PATCH v2 net 0/4] Aquantia/Marvell AQtion atlantic driver fixes 10/2019

[PATCH v2 net 0/4] Aquantia/Marvell AQtion atlantic driver fixes 10/2019

[Igor Russkikh]

Hello David!

Here is a set of various bugfixes, to be considered for stable as well.

Thanks!

V2: double space removed

Dmitry Bogdanov (2):
  net: aquantia: do not pass lro session with invalid tcp checksum
  net: aquantia: correctly handle macvlan and multicast coexistence

Igor Russkikh (2):
  net: aquantia: temperature retrieval fix
  net: aquantia: when cleaning hw cache it should be toggled

 .../net/ethernet/aquantia/atlantic/aq_main.c  |  4 +--
 .../net/ethernet/aquantia/atlantic/aq_nic.c   | 32 +++++++++----------
 .../net/ethernet/aquantia/atlantic/aq_ring.c  |  3 +-
 .../aquantia/atlantic/hw_atl/hw_atl_b0.c      | 23 ++++++++++---
 .../aquantia/atlantic/hw_atl/hw_atl_llh.c     | 17 ++++++++--
 .../aquantia/atlantic/hw_atl/hw_atl_llh.h     |  7 ++--
 .../atlantic/hw_atl/hw_atl_llh_internal.h     | 19 +++++++++++
 .../atlantic/hw_atl/hw_atl_utils_fw2x.c       |  2 +-
 8 files changed, 77 insertions(+), 30 deletions(-)

-- 
2.17.1

[PATCH v2 net 1/4] net: aquantia: temperature retrieval fix

[Igor Russkikh]

Chip temperature is a two byte word, colocated internally with cable
length data. We do all readouts from HW memory by dwords, thus
we should clear extra high bytes, otherwise temperature output
gets weird as soon as we attach a cable to the NIC.

Fixes: 8f8940118654 ("net: aquantia: add infrastructure to readout chip temperature")
Tested-by: Holger Hoffstätte <holger@applied-asynchrony.com>
Signed-off-by: Igor Russkikh <igor.russkikh@aquantia.com>
---
 .../net/ethernet/aquantia/atlantic/hw_atl/hw_atl_utils_fw2x.c   | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_utils_fw2x.c b/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_utils_fw2x.c
index da726489e3c8..7bc51f8d6f2f 100644
--- a/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_utils_fw2x.c
+++ b/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_utils_fw2x.c
@@ -337,7 +337,7 @@ static int aq_fw2x_get_phy_temp(struct aq_hw_s *self, int *temp)
 	/* Convert PHY temperature from 1/256 degree Celsius
 	 * to 1/1000 degree Celsius.
 	 */
-	*temp = temp_res  * 1000 / 256;
+	*temp = (temp_res & 0xFFFF) * 1000 / 256;
 
 	return 0;
 }
-- 
2.17.1

[PATCH v2 net 2/4] net: aquantia: when cleaning hw cache it should be toggled

[Igor Russkikh]

From HW specification to correctly reset HW caches (this is a required
workaround when stopping the device), register bit should actually
be toggled.

It was previosly always just set. Due to the way driver stops HW this
never actually caused any issues, but it still may, so cleaning this up.

Fixes: 7a1bb49461b1 ("net: aquantia: fix potential IOMMU fault after driver unbind")
Signed-off-by: Igor Russkikh <igor.russkikh@aquantia.com>
---
 .../aquantia/atlantic/hw_atl/hw_atl_b0.c      | 16 ++++++++++++++--
 .../aquantia/atlantic/hw_atl/hw_atl_llh.c     | 17 +++++++++++++++--
 .../aquantia/atlantic/hw_atl/hw_atl_llh.h     |  7 +++++--
 .../atlantic/hw_atl/hw_atl_llh_internal.h     | 19 +++++++++++++++++++
 4 files changed, 53 insertions(+), 6 deletions(-)

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 30f7fc4c97ff..3459fadb7ddd 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
@@ -968,14 +968,26 @@ static int hw_atl_b0_hw_interrupt_moderation_set(struct aq_hw_s *self)
 
 static int hw_atl_b0_hw_stop(struct aq_hw_s *self)
 {
+	int err;
+	u32 val;
+
 	hw_atl_b0_hw_irq_disable(self, HW_ATL_B0_INT_MASK);
 
 	/* Invalidate Descriptor Cache to prevent writing to the cached
 	 * descriptors and to the data pointer of those descriptors
 	 */
-	hw_atl_rdm_rx_dma_desc_cache_init_set(self, 1);
+	hw_atl_rdm_rx_dma_desc_cache_init_tgl(self);
 
-	return aq_hw_err_from_flags(self);
+	err = aq_hw_err_from_flags(self);
+
+	if (err)
+		goto err_exit;
+
+	readx_poll_timeout_atomic(hw_atl_rdm_rx_dma_desc_cache_init_done_get,
+				  self, val, val == 1, 1000U, 10000U);
+
+err_exit:
+	return err;
 }
 
 static int hw_atl_b0_hw_ring_tx_stop(struct aq_hw_s *self,
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 1149812ae463..6f340695e6bd 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
@@ -606,12 +606,25 @@ void hw_atl_rpb_rx_flow_ctl_mode_set(struct aq_hw_s *aq_hw, u32 rx_flow_ctl_mode
 			    HW_ATL_RPB_RX_FC_MODE_SHIFT, rx_flow_ctl_mode);
 }
 
-void hw_atl_rdm_rx_dma_desc_cache_init_set(struct aq_hw_s *aq_hw, u32 init)
+void hw_atl_rdm_rx_dma_desc_cache_init_tgl(struct aq_hw_s *aq_hw)
 {
+	u32 val;
+
+	val = aq_hw_read_reg_bit(aq_hw, HW_ATL_RDM_RX_DMA_DESC_CACHE_INIT_ADR,
+				 HW_ATL_RDM_RX_DMA_DESC_CACHE_INIT_MSK,
+				 HW_ATL_RDM_RX_DMA_DESC_CACHE_INIT_SHIFT);
+
 	aq_hw_write_reg_bit(aq_hw, HW_ATL_RDM_RX_DMA_DESC_CACHE_INIT_ADR,
 			    HW_ATL_RDM_RX_DMA_DESC_CACHE_INIT_MSK,
 			    HW_ATL_RDM_RX_DMA_DESC_CACHE_INIT_SHIFT,
-			    init);
+			    val ^ 1);
+}
+
+u32 hw_atl_rdm_rx_dma_desc_cache_init_done_get(struct aq_hw_s *aq_hw)
+{
+	return aq_hw_read_reg_bit(aq_hw, RDM_RX_DMA_DESC_CACHE_INIT_DONE_ADR,
+				  RDM_RX_DMA_DESC_CACHE_INIT_DONE_MSK,
+				  RDM_RX_DMA_DESC_CACHE_INIT_DONE_SHIFT);
 }
 
 void hw_atl_rpb_rx_pkt_buff_size_per_tc_set(struct aq_hw_s *aq_hw,
diff --git a/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_llh.h b/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_llh.h
index 0c37abbabca5..c3ee278c3747 100644
--- a/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_llh.h
+++ b/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_llh.h
@@ -313,8 +313,11 @@ void hw_atl_rpb_rx_pkt_buff_size_per_tc_set(struct aq_hw_s *aq_hw,
 					    u32 rx_pkt_buff_size_per_tc,
 					    u32 buffer);
 
-/* set rdm rx dma descriptor cache init */
-void hw_atl_rdm_rx_dma_desc_cache_init_set(struct aq_hw_s *aq_hw, u32 init);
+/* toggle rdm rx dma descriptor cache init */
+void hw_atl_rdm_rx_dma_desc_cache_init_tgl(struct aq_hw_s *aq_hw);
+
+/* get rdm rx dma descriptor cache init done */
+u32 hw_atl_rdm_rx_dma_desc_cache_init_done_get(struct aq_hw_s *aq_hw);
 
 /* set rx xoff enable (per tc) */
 void hw_atl_rpb_rx_xoff_en_per_tc_set(struct aq_hw_s *aq_hw, u32 rx_xoff_en_per_tc,
diff --git a/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_llh_internal.h b/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_llh_internal.h
index c3febcdfa92e..35887ad89025 100644
--- a/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_llh_internal.h
+++ b/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_llh_internal.h
@@ -318,6 +318,25 @@
 /* default value of bitfield rdm_desc_init_i */
 #define HW_ATL_RDM_RX_DMA_DESC_CACHE_INIT_DEFAULT 0x0
 
+/* rdm_desc_init_done_i bitfield definitions
+ * preprocessor definitions for the bitfield rdm_desc_init_done_i.
+ * port="pif_rdm_desc_init_done_i"
+ */
+
+/* register address for bitfield rdm_desc_init_done_i */
+#define RDM_RX_DMA_DESC_CACHE_INIT_DONE_ADR 0x00005a10
+/* bitmask for bitfield rdm_desc_init_done_i */
+#define RDM_RX_DMA_DESC_CACHE_INIT_DONE_MSK 0x00000001U
+/* inverted bitmask for bitfield rdm_desc_init_done_i */
+#define RDM_RX_DMA_DESC_CACHE_INIT_DONE_MSKN 0xfffffffe
+/* lower bit position of bitfield  rdm_desc_init_done_i */
+#define RDM_RX_DMA_DESC_CACHE_INIT_DONE_SHIFT 0U
+/* width of bitfield rdm_desc_init_done_i */
+#define RDM_RX_DMA_DESC_CACHE_INIT_DONE_WIDTH 1
+/* default value of bitfield rdm_desc_init_done_i */
+#define RDM_RX_DMA_DESC_CACHE_INIT_DONE_DEFAULT 0x0
+
+
 /* rx int_desc_wrb_en bitfield definitions
  * preprocessor definitions for the bitfield "int_desc_wrb_en".
  * port="pif_rdm_int_desc_wrb_en_i"
-- 
2.17.1

[PATCH v2 net 3/4] net: aquantia: do not pass lro session with invalid tcp checksum

[Igor Russkikh]

From: Dmitry Bogdanov <dmitry.bogdanov@aquantia.com>

Individual descriptors on LRO TCP session should be checked
for CRC errors. It was discovered that HW recalculates
L4 checksums on LRO session and does not break it up on bad L4
csum.

Thus, driver should aggregate HW LRO L4 statuses from all individual
buffers of LRO session and drop packet if one of the buffers has bad
L4 checksum.

Fixes: f38f1ee8aeb2 ("net: aquantia: check rx csum for all packets in LRO session")
Signed-off-by: Dmitry Bogdanov <dmitry.bogdanov@aquantia.com>
Signed-off-by: Igor Russkikh <igor.russkikh@aquantia.com>
---
 drivers/net/ethernet/aquantia/atlantic/aq_ring.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_ring.c b/drivers/net/ethernet/aquantia/atlantic/aq_ring.c
index 3901d7994ca1..76bdbe1596d6 100644
--- a/drivers/net/ethernet/aquantia/atlantic/aq_ring.c
+++ b/drivers/net/ethernet/aquantia/atlantic/aq_ring.c
@@ -313,6 +313,7 @@ int aq_ring_rx_clean(struct aq_ring_s *self,
 					break;
 
 				buff->is_error |= buff_->is_error;
+				buff->is_cso_err |= buff_->is_cso_err;
 
 			} while (!buff_->is_eop);
 
@@ -320,7 +321,7 @@ int aq_ring_rx_clean(struct aq_ring_s *self,
 				err = 0;
 				goto err_exit;
 			}
-			if (buff->is_error) {
+			if (buff->is_error || buff->is_cso_err) {
 				buff_ = buff;
 				do {
 					next_ = buff_->next,
-- 
2.17.1

[PATCH v2 net 4/4] net: aquantia: correctly handle macvlan and multicast coexistence

[Igor Russkikh]

From: Dmitry Bogdanov <dmitry.bogdanov@aquantia.com>

macvlan and multicast handling is now mixed up.
The explicit issue is that macvlan interface gets broken (no traffic)
after clearing MULTICAST flag on the real interface.

We now do separate logic and consider both ALLMULTI and MULTICAST
flags on the device.

Fixes: 11ba961c9161 ("net: aquantia: Fix IFF_ALLMULTI flag functionality")
Signed-off-by: Dmitry Bogdanov <dmitry.bogdanov@aquantia.com>
Signed-off-by: Igor Russkikh <igor.russkikh@aquantia.com>
---
 .../net/ethernet/aquantia/atlantic/aq_main.c  |  4 +--
 .../net/ethernet/aquantia/atlantic/aq_nic.c   | 32 +++++++++----------
 .../aquantia/atlantic/hw_atl/hw_atl_b0.c      |  7 ++--
 3 files changed, 21 insertions(+), 22 deletions(-)

diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_main.c b/drivers/net/ethernet/aquantia/atlantic/aq_main.c
index b4a0fb281e69..bb65dd39f847 100644
--- a/drivers/net/ethernet/aquantia/atlantic/aq_main.c
+++ b/drivers/net/ethernet/aquantia/atlantic/aq_main.c
@@ -194,9 +194,7 @@ static void aq_ndev_set_multicast_settings(struct net_device *ndev)
 {
 	struct aq_nic_s *aq_nic = netdev_priv(ndev);
 
-	aq_nic_set_packet_filter(aq_nic, ndev->flags);
-
-	aq_nic_set_multicast_list(aq_nic, ndev);
+	(void)aq_nic_set_multicast_list(aq_nic, ndev);
 }
 
 static int aq_ndo_vlan_rx_add_vid(struct net_device *ndev, __be16 proto,
diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_nic.c b/drivers/net/ethernet/aquantia/atlantic/aq_nic.c
index 8f66e7817811..2a18439b36fb 100644
--- a/drivers/net/ethernet/aquantia/atlantic/aq_nic.c
+++ b/drivers/net/ethernet/aquantia/atlantic/aq_nic.c
@@ -631,9 +631,12 @@ int aq_nic_set_packet_filter(struct aq_nic_s *self, unsigned int flags)
 
 int aq_nic_set_multicast_list(struct aq_nic_s *self, struct net_device *ndev)
 {
-	unsigned int packet_filter = self->packet_filter;
+	const struct aq_hw_ops *hw_ops = self->aq_hw_ops;
+	struct aq_nic_cfg_s *cfg = &self->aq_nic_cfg;
+	unsigned int packet_filter = ndev->flags;
 	struct netdev_hw_addr *ha = NULL;
 	unsigned int i = 0U;
+	int err = 0;
 
 	self->mc_list.count = 0;
 	if (netdev_uc_count(ndev) > AQ_HW_MULTICAST_ADDRESS_MAX) {
@@ -641,29 +644,26 @@ int aq_nic_set_multicast_list(struct aq_nic_s *self, struct net_device *ndev)
 	} else {
 		netdev_for_each_uc_addr(ha, ndev) {
 			ether_addr_copy(self->mc_list.ar[i++], ha->addr);
-
-			if (i >= AQ_HW_MULTICAST_ADDRESS_MAX)
-				break;
 		}
 	}
 
-	if (i + netdev_mc_count(ndev) > AQ_HW_MULTICAST_ADDRESS_MAX) {
-		packet_filter |= IFF_ALLMULTI;
-	} else {
-		netdev_for_each_mc_addr(ha, ndev) {
-			ether_addr_copy(self->mc_list.ar[i++], ha->addr);
-
-			if (i >= AQ_HW_MULTICAST_ADDRESS_MAX)
-				break;
+	cfg->is_mc_list_enabled = !!(packet_filter & IFF_MULTICAST);
+	if (cfg->is_mc_list_enabled) {
+		if (i + netdev_mc_count(ndev) > AQ_HW_MULTICAST_ADDRESS_MAX) {
+			packet_filter |= IFF_ALLMULTI;
+		} else {
+			netdev_for_each_mc_addr(ha, ndev) {
+				ether_addr_copy(self->mc_list.ar[i++],
+						ha->addr);
+			}
 		}
 	}
 
 	if (i > 0 && i <= AQ_HW_MULTICAST_ADDRESS_MAX) {
-		packet_filter |= IFF_MULTICAST;
 		self->mc_list.count = i;
-		self->aq_hw_ops->hw_multicast_list_set(self->aq_hw,
-						       self->mc_list.ar,
-						       self->mc_list.count);
+		err = hw_ops->hw_multicast_list_set(self->aq_hw,
+						    self->mc_list.ar,
+						    self->mc_list.count);
 	}
 	return aq_nic_set_packet_filter(self, packet_filter);
 }
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 3459fadb7ddd..2ad3fa6316ce 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
@@ -818,14 +818,15 @@ static int hw_atl_b0_hw_packet_filter_set(struct aq_hw_s *self,
 				     cfg->is_vlan_force_promisc);
 
 	hw_atl_rpfl2multicast_flr_en_set(self,
-					 IS_FILTER_ENABLED(IFF_ALLMULTI), 0);
+					 IS_FILTER_ENABLED(IFF_ALLMULTI) &&
+					 IS_FILTER_ENABLED(IFF_MULTICAST), 0);
 
 	hw_atl_rpfl2_accept_all_mc_packets_set(self,
-					       IS_FILTER_ENABLED(IFF_ALLMULTI));
+					      IS_FILTER_ENABLED(IFF_ALLMULTI) &&
+					      IS_FILTER_ENABLED(IFF_MULTICAST));
 
 	hw_atl_rpfl2broadcast_en_set(self, IS_FILTER_ENABLED(IFF_BROADCAST));
 
-	cfg->is_mc_list_enabled = IS_FILTER_ENABLED(IFF_MULTICAST);
 
 	for (i = HW_ATL_B0_MAC_MIN; i < HW_ATL_B0_MAC_MAX; ++i)
 		hw_atl_rpfl2_uc_flr_en_set(self,
-- 
2.17.1

Re: [PATCH v2 net 2/4] net: aquantia: when cleaning hw cache it should be toggled

[Jakub Kicinski]

On Fri, 11 Oct 2019 13:45:20 +0000, Igor Russkikh wrote:
> From HW specification to correctly reset HW caches (this is a required
> workaround when stopping the device), register bit should actually
> be toggled.

Does the bit get set by the driver or HW?

If it gets set by HW there is still a tiny race from reading to
writing.. Perhaps doing two writes -> to 0 and to 1 would be a better
option?  

Just wondering, obviously I don't know your HW :)

> It was previosly always just set. Due to the way driver stops HW this
> never actually caused any issues, but it still may, so cleaning this up.

Hm. So is it a cleanup of fix? Does the way code is written guarantee
it will never cause issues?

> Fixes: 7a1bb49461b1 ("net: aquantia: fix potential IOMMU fault after driver unbind")
> Signed-off-by: Igor Russkikh <igor.russkikh@aquantia.com>
> ---
>  .../aquantia/atlantic/hw_atl/hw_atl_b0.c      | 16 ++++++++++++++--
>  .../aquantia/atlantic/hw_atl/hw_atl_llh.c     | 17 +++++++++++++++--
>  .../aquantia/atlantic/hw_atl/hw_atl_llh.h     |  7 +++++--
>  .../atlantic/hw_atl/hw_atl_llh_internal.h     | 19 +++++++++++++++++++
>  4 files changed, 53 insertions(+), 6 deletions(-)
> 
> 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 30f7fc4c97ff..3459fadb7ddd 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
> @@ -968,14 +968,26 @@ static int hw_atl_b0_hw_interrupt_moderation_set(struct aq_hw_s *self)
>  
>  static int hw_atl_b0_hw_stop(struct aq_hw_s *self)
>  {
> +	int err;
> +	u32 val;
> +
>  	hw_atl_b0_hw_irq_disable(self, HW_ATL_B0_INT_MASK);
>  
>  	/* Invalidate Descriptor Cache to prevent writing to the cached
>  	 * descriptors and to the data pointer of those descriptors
>  	 */
> -	hw_atl_rdm_rx_dma_desc_cache_init_set(self, 1);
> +	hw_atl_rdm_rx_dma_desc_cache_init_tgl(self);
>  
> -	return aq_hw_err_from_flags(self);
> +	err = aq_hw_err_from_flags(self);
> +
> +	if (err)
> +		goto err_exit;
> +
> +	readx_poll_timeout_atomic(hw_atl_rdm_rx_dma_desc_cache_init_done_get,
> +				  self, val, val == 1, 1000U, 10000U);

It's a little strange to toggle, yet wait for it to be of a specific
value..

> +
> +err_exit:
> +	return err;

Just return err instead of doing this pointless goto. It make the code
harder to follow.

>  }
>  
>  static int hw_atl_b0_hw_ring_tx_stop(struct aq_hw_s *self,
> 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 1149812ae463..6f340695e6bd 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
> @@ -606,12 +606,25 @@ void hw_atl_rpb_rx_flow_ctl_mode_set(struct aq_hw_s *aq_hw, u32 rx_flow_ctl_mode
>  			    HW_ATL_RPB_RX_FC_MODE_SHIFT, rx_flow_ctl_mode);
>  }
>  
> -void hw_atl_rdm_rx_dma_desc_cache_init_set(struct aq_hw_s *aq_hw, u32 init)
> +void hw_atl_rdm_rx_dma_desc_cache_init_tgl(struct aq_hw_s *aq_hw)
>  {
> +	u32 val;
> +
> +	val = aq_hw_read_reg_bit(aq_hw, HW_ATL_RDM_RX_DMA_DESC_CACHE_INIT_ADR,
> +				 HW_ATL_RDM_RX_DMA_DESC_CACHE_INIT_MSK,
> +				 HW_ATL_RDM_RX_DMA_DESC_CACHE_INIT_SHIFT);

hw_atl_rdm_rx_dma_desc_cache_init_done_get() ?

>  	aq_hw_write_reg_bit(aq_hw, HW_ATL_RDM_RX_DMA_DESC_CACHE_INIT_ADR,
>  			    HW_ATL_RDM_RX_DMA_DESC_CACHE_INIT_MSK,
>  			    HW_ATL_RDM_RX_DMA_DESC_CACHE_INIT_SHIFT,
> -			    init);
> +			    val ^ 1);
> +}
> +
> +u32 hw_atl_rdm_rx_dma_desc_cache_init_done_get(struct aq_hw_s *aq_hw)
> +{
> +	return aq_hw_read_reg_bit(aq_hw, RDM_RX_DMA_DESC_CACHE_INIT_DONE_ADR,
> +				  RDM_RX_DMA_DESC_CACHE_INIT_DONE_MSK,
> +				  RDM_RX_DMA_DESC_CACHE_INIT_DONE_SHIFT);
>  }
>  
>  void hw_atl_rpb_rx_pkt_buff_size_per_tc_set(struct aq_hw_s *aq_hw,
> diff --git a/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_llh.h b/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_llh.h
> index 0c37abbabca5..c3ee278c3747 100644
> --- a/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_llh.h
> +++ b/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_llh.h
> @@ -313,8 +313,11 @@ void hw_atl_rpb_rx_pkt_buff_size_per_tc_set(struct aq_hw_s *aq_hw,
>  					    u32 rx_pkt_buff_size_per_tc,
>  					    u32 buffer);
>  
> -/* set rdm rx dma descriptor cache init */
> -void hw_atl_rdm_rx_dma_desc_cache_init_set(struct aq_hw_s *aq_hw, u32 init);
> +/* toggle rdm rx dma descriptor cache init */
> +void hw_atl_rdm_rx_dma_desc_cache_init_tgl(struct aq_hw_s *aq_hw);
> +
> +/* get rdm rx dma descriptor cache init done */
> +u32 hw_atl_rdm_rx_dma_desc_cache_init_done_get(struct aq_hw_s *aq_hw);
>  
>  /* set rx xoff enable (per tc) */
>  void hw_atl_rpb_rx_xoff_en_per_tc_set(struct aq_hw_s *aq_hw, u32 rx_xoff_en_per_tc,
> diff --git a/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_llh_internal.h b/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_llh_internal.h
> index c3febcdfa92e..35887ad89025 100644
> --- a/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_llh_internal.h
> +++ b/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_llh_internal.h
> @@ -318,6 +318,25 @@
>  /* default value of bitfield rdm_desc_init_i */
>  #define HW_ATL_RDM_RX_DMA_DESC_CACHE_INIT_DEFAULT 0x0
>  
> +/* rdm_desc_init_done_i bitfield definitions
> + * preprocessor definitions for the bitfield rdm_desc_init_done_i.
> + * port="pif_rdm_desc_init_done_i"
> + */
> +
> +/* register address for bitfield rdm_desc_init_done_i */
> +#define RDM_RX_DMA_DESC_CACHE_INIT_DONE_ADR 0x00005a10
> +/* bitmask for bitfield rdm_desc_init_done_i */
> +#define RDM_RX_DMA_DESC_CACHE_INIT_DONE_MSK 0x00000001U
> +/* inverted bitmask for bitfield rdm_desc_init_done_i */
> +#define RDM_RX_DMA_DESC_CACHE_INIT_DONE_MSKN 0xfffffffe
> +/* lower bit position of bitfield  rdm_desc_init_done_i */
> +#define RDM_RX_DMA_DESC_CACHE_INIT_DONE_SHIFT 0U
> +/* width of bitfield rdm_desc_init_done_i */
> +#define RDM_RX_DMA_DESC_CACHE_INIT_DONE_WIDTH 1
> +/* default value of bitfield rdm_desc_init_done_i */
> +#define RDM_RX_DMA_DESC_CACHE_INIT_DONE_DEFAULT 0x0
> +
> +

two empty lines here?

>  /* rx int_desc_wrb_en bitfield definitions
>   * preprocessor definitions for the bitfield "int_desc_wrb_en".
>   * port="pif_rdm_int_desc_wrb_en_i"

Re: [PATCH v2 net 2/4] net: aquantia: when cleaning hw cache it should be toggled

[Igor Russkikh]

Hello Jakub,

>> workaround when stopping the device), register bit should actually
>> be toggled.
> 
> Does the bit get set by the driver or HW?
> 
> If it gets set by HW there is still a tiny race from reading to
> writing.. Perhaps doing two writes -> to 0 and to 1 would be a better
> option?  

No, set is done by the driver, not HW. HW just tracks for the toggle.

>> It was previosly always just set. Due to the way driver stops HW this
>> never actually caused any issues, but it still may, so cleaning this up.
> 
> Hm. So is it a cleanup of fix? Does the way code is written guarantee
> it will never cause issues?

Yes, thats a cleanup. We just had other products where this cache reset had to
be done multiple times. Obviously doing that second time was just no-op for
hardware ;)
On linux this always gets called on deinit only once - thus it was safe.
We just aligning here the linux driver with actual HW specification.

>> +	if (err)
>> +		goto err_exit;
>> +
>> +	readx_poll_timeout_atomic(hw_atl_rdm_rx_dma_desc_cache_init_done_get,
>> +				  self, val, val == 1, 1000U, 10000U);
> 
> It's a little strange to toggle, yet wait for it to be of a specific
> value..

Notice thats a different value - 'cache_init_done' bit.
This is used by HW to indicate completion of cache reset operation.

>> +err_exit:
>> +	return err;
> 
> Just return err instead of doing this pointless goto. It make the code
> harder to follow.

Sure

>> +#define RDM_RX_DMA_DESC_CACHE_INIT_DONE_DEFAULT 0x0
>> +
>> +
> 
> two empty lines here?

Will fix.

Regards,
  Igor

Re: [PATCH v2 net 2/4] net: aquantia: when cleaning hw cache it should be toggled

[Jakub Kicinski]

On Wed, 16 Oct 2019 13:19:30 +0000, Igor Russkikh wrote:
> >> It was previosly always just set. Due to the way driver stops HW this
> >> never actually caused any issues, but it still may, so cleaning this up.  
> > 
> > Hm. So is it a cleanup of fix? Does the way code is written guarantee
> > it will never cause issues?  
> 
> Yes, thats a cleanup. We just had other products where this cache reset had to
> be done multiple times. Obviously doing that second time was just no-op for
> hardware ;)
> On linux this always gets called on deinit only once - thus it was safe.
> We just aligning here the linux driver with actual HW specification.

I see, in light of that explanation I think it'd be appropriate to take
the Fixes tag away and move this patch to the net-next series.

> >> +	if (err)
> >> +		goto err_exit;
> >> +
> >> +	readx_poll_timeout_atomic(hw_atl_rdm_rx_dma_desc_cache_init_done_get,
> >> +				  self, val, val == 1, 1000U, 10000U);  
> > 
> > It's a little strange to toggle, yet wait for it to be of a specific
> > value..  
> 
> Notice thats a different value - 'cache_init_done' bit.
> This is used by HW to indicate completion of cache reset operation.

Ah, ack, it's an "eyeful".. :)