[PATCH net-next] enetc: Workaround for MDIO register access issue

[PATCH net-next] enetc: Workaround for MDIO register access issue

[Claudiu Manoil]

From: Alex Marginean <alexandru.marginean@nxp.com>

Due to a hardware issue, an access to MDIO registers
that is concurrent with other ENETC register accesses
may lead to the MDIO access being dropped or corrupted.
The workaround introduces locking for all register accesses
to the ENETC register space.  To reduce performance impact,
a readers-writers locking scheme has been implemented.
The writer in this case is the MDIO access code (irrelevant
whether that MDIO access is a register read or write), and
the reader is any access code to non-MDIO ENETC registers.
Also, the datapath functions acquire the read lock fewer times
and use _hot accessors.  All the rest of the code uses the _wa
accessors which lock every register access.

Signed-off-by: Alex Marginean <alexandru.marginean@nxp.com>
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Signed-off-by: Claudiu Manoil <claudiu.manoil@nxp.com>
---
 drivers/net/ethernet/freescale/enetc/Kconfig  |  1 +
 drivers/net/ethernet/freescale/enetc/enetc.c  | 60 ++++++++++++++-----
 .../net/ethernet/freescale/enetc/enetc_hw.h   | 60 +++++++++++++++++--
 .../net/ethernet/freescale/enetc/enetc_mdio.c |  8 ++-
 4 files changed, 107 insertions(+), 22 deletions(-)

diff --git a/drivers/net/ethernet/freescale/enetc/Kconfig b/drivers/net/ethernet/freescale/enetc/Kconfig
index 0fa18b00c49b..d99ea0f4e4a6 100644
--- a/drivers/net/ethernet/freescale/enetc/Kconfig
+++ b/drivers/net/ethernet/freescale/enetc/Kconfig
@@ -16,6 +16,7 @@ config FSL_ENETC
 config FSL_ENETC_VF
 	tristate "ENETC VF driver"
 	depends on PCI && PCI_MSI
+	select FSL_ENETC_MDIO
 	select PHYLINK
 	select DIMLIB
 	help
diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c b/drivers/net/ethernet/freescale/enetc/enetc.c
index 01089c30b462..83ab8e5d691b 100644
--- a/drivers/net/ethernet/freescale/enetc/enetc.c
+++ b/drivers/net/ethernet/freescale/enetc/enetc.c
@@ -33,7 +33,10 @@ netdev_tx_t enetc_xmit(struct sk_buff *skb, struct net_device *ndev)
 		return NETDEV_TX_BUSY;
 	}
 
+	read_lock(&enetc_mdio_lock);
 	count = enetc_map_tx_buffs(tx_ring, skb, priv->active_offloads);
+	read_unlock(&enetc_mdio_lock);
+
 	if (unlikely(!count))
 		goto drop_packet_err;
 
@@ -199,7 +202,7 @@ static int enetc_map_tx_buffs(struct enetc_bdr *tx_ring, struct sk_buff *skb,
 	skb_tx_timestamp(skb);
 
 	/* let H/W know BD ring has been updated */
-	enetc_wr_reg(tx_ring->tpir, i); /* includes wmb() */
+	enetc_wr_reg_hot(tx_ring->tpir, i); /* includes wmb() */
 
 	return count;
 
@@ -222,12 +225,16 @@ static irqreturn_t enetc_msix(int irq, void *data)
 	struct enetc_int_vector	*v = data;
 	int i;
 
+	read_lock(&enetc_mdio_lock);
+
 	/* disable interrupts */
-	enetc_wr_reg(v->rbier, 0);
-	enetc_wr_reg(v->ricr1, v->rx_ictt);
+	enetc_wr_reg_hot(v->rbier, 0);
+	enetc_wr_reg_hot(v->ricr1, v->rx_ictt);
 
 	for_each_set_bit(i, &v->tx_rings_map, ENETC_MAX_NUM_TXQS)
-		enetc_wr_reg(v->tbier_base + ENETC_BDR_OFF(i), 0);
+		enetc_wr_reg_hot(v->tbier_base + ENETC_BDR_OFF(i), 0);
+
+	read_unlock(&enetc_mdio_lock);
 
 	napi_schedule(&v->napi);
 
@@ -294,19 +301,23 @@ static int enetc_poll(struct napi_struct *napi, int budget)
 
 	v->rx_napi_work = false;
 
+	read_lock(&enetc_mdio_lock);
+
 	/* enable interrupts */
-	enetc_wr_reg(v->rbier, ENETC_RBIER_RXTIE);
+	enetc_wr_reg_hot(v->rbier, ENETC_RBIER_RXTIE);
 
 	for_each_set_bit(i, &v->tx_rings_map, ENETC_MAX_NUM_TXQS)
-		enetc_wr_reg(v->tbier_base + ENETC_BDR_OFF(i),
-			     ENETC_TBIER_TXTIE);
+		enetc_wr_reg_hot(v->tbier_base + ENETC_BDR_OFF(i),
+				 ENETC_TBIER_TXTIE);
+
+	read_unlock(&enetc_mdio_lock);
 
 	return work_done;
 }
 
 static int enetc_bd_ready_count(struct enetc_bdr *tx_ring, int ci)
 {
-	int pi = enetc_rd_reg(tx_ring->tcir) & ENETC_TBCIR_IDX_MASK;
+	int pi = enetc_rd_reg_hot(tx_ring->tcir) & ENETC_TBCIR_IDX_MASK;
 
 	return pi >= ci ? pi - ci : tx_ring->bd_count - ci + pi;
 }
@@ -346,7 +357,10 @@ static bool enetc_clean_tx_ring(struct enetc_bdr *tx_ring, int napi_budget)
 
 	i = tx_ring->next_to_clean;
 	tx_swbd = &tx_ring->tx_swbd[i];
+
+	read_lock(&enetc_mdio_lock);
 	bds_to_clean = enetc_bd_ready_count(tx_ring, i);
+	read_unlock(&enetc_mdio_lock);
 
 	do_tstamp = false;
 
@@ -389,16 +403,20 @@ static bool enetc_clean_tx_ring(struct enetc_bdr *tx_ring, int napi_budget)
 			tx_swbd = tx_ring->tx_swbd;
 		}
 
+		read_lock(&enetc_mdio_lock);
+
 		/* BD iteration loop end */
 		if (is_eof) {
 			tx_frm_cnt++;
 			/* re-arm interrupt source */
-			enetc_wr_reg(tx_ring->idr, BIT(tx_ring->index) |
-				     BIT(16 + tx_ring->index));
+			enetc_wr_reg_hot(tx_ring->idr, BIT(tx_ring->index) |
+					 BIT(16 + tx_ring->index));
 		}
 
 		if (unlikely(!bds_to_clean))
 			bds_to_clean = enetc_bd_ready_count(tx_ring, i);
+
+		read_unlock(&enetc_mdio_lock);
 	}
 
 	tx_ring->next_to_clean = i;
@@ -476,13 +494,14 @@ static int enetc_refill_rx_ring(struct enetc_bdr *rx_ring, const int buff_cnt)
 		rx_ring->next_to_alloc = i; /* keep track from page reuse */
 		rx_ring->next_to_use = i;
 		/* update ENETC's consumer index */
-		enetc_wr_reg(rx_ring->rcir, i);
+		enetc_wr_reg_hot(rx_ring->rcir, i);
 	}
 
 	return j;
 }
 
 #ifdef CONFIG_FSL_ENETC_PTP_CLOCK
+/* Must be called with the read-side enetc_mdio_lock held */
 static void enetc_get_rx_tstamp(struct net_device *ndev,
 				union enetc_rx_bd *rxbd,
 				struct sk_buff *skb)
@@ -494,8 +513,8 @@ static void enetc_get_rx_tstamp(struct net_device *ndev,
 	u64 tstamp;
 
 	if (le16_to_cpu(rxbd->r.flags) & ENETC_RXBD_FLAG_TSTMP) {
-		lo = enetc_rd(hw, ENETC_SICTR0);
-		hi = enetc_rd(hw, ENETC_SICTR1);
+		lo = enetc_rd_reg_hot(hw->reg + ENETC_SICTR0);
+		hi = enetc_rd_reg_hot(hw->reg + ENETC_SICTR1);
 		rxbd = enetc_rxbd_ext(rxbd);
 		tstamp_lo = le32_to_cpu(rxbd->ext.tstamp);
 		if (lo <= tstamp_lo)
@@ -644,6 +663,8 @@ static int enetc_clean_rx_ring(struct enetc_bdr *rx_ring,
 		u32 bd_status;
 		u16 size;
 
+		read_lock(&enetc_mdio_lock);
+
 		if (cleaned_cnt >= ENETC_RXBD_BUNDLE) {
 			int count = enetc_refill_rx_ring(rx_ring, cleaned_cnt);
 
@@ -652,15 +673,19 @@ static int enetc_clean_rx_ring(struct enetc_bdr *rx_ring,
 
 		rxbd = enetc_rxbd(rx_ring, i);
 		bd_status = le32_to_cpu(rxbd->r.lstatus);
-		if (!bd_status)
+		if (!bd_status) {
+			read_unlock(&enetc_mdio_lock);
 			break;
+		}
 
-		enetc_wr_reg(rx_ring->idr, BIT(rx_ring->index));
+		enetc_wr_reg_hot(rx_ring->idr, BIT(rx_ring->index));
 		dma_rmb(); /* for reading other rxbd fields */
 		size = le16_to_cpu(rxbd->r.buf_len);
 		skb = enetc_map_rx_buff_to_skb(rx_ring, i, size);
-		if (!skb)
+		if (!skb) {
+			read_unlock(&enetc_mdio_lock);
 			break;
+		}
 
 		enetc_get_offloads(rx_ring, rxbd, skb);
 
@@ -672,6 +697,7 @@ static int enetc_clean_rx_ring(struct enetc_bdr *rx_ring,
 
 		if (unlikely(bd_status &
 			     ENETC_RXBD_LSTATUS(ENETC_RXBD_ERR_MASK))) {
+			read_unlock(&enetc_mdio_lock);
 			dev_kfree_skb(skb);
 			while (!(bd_status & ENETC_RXBD_LSTATUS_F)) {
 				dma_rmb();
@@ -711,6 +737,8 @@ static int enetc_clean_rx_ring(struct enetc_bdr *rx_ring,
 
 		enetc_process_skb(rx_ring, skb);
 
+		read_unlock(&enetc_mdio_lock);
+
 		napi_gro_receive(napi, skb);
 
 		rx_frm_cnt++;
diff --git a/drivers/net/ethernet/freescale/enetc/enetc_hw.h b/drivers/net/ethernet/freescale/enetc/enetc_hw.h
index 68ef4f959982..95cb4fc30fbf 100644
--- a/drivers/net/ethernet/freescale/enetc/enetc_hw.h
+++ b/drivers/net/ethernet/freescale/enetc/enetc_hw.h
@@ -325,8 +325,15 @@ struct enetc_hw {
 };
 
 /* general register accessors */
-#define enetc_rd_reg(reg)	ioread32((reg))
-#define enetc_wr_reg(reg, val)	iowrite32((val), (reg))
+#define enetc_rd_reg(reg)	enetc_rd_reg_wa((reg))
+#define enetc_wr_reg(reg, val)	enetc_wr_reg_wa((reg), (val))
+
+/* accessors for data-path, due to MDIO issue on LS1028 these should be called
+ * only under the rwlock_t enetc_mdio_lock
+ */
+#define enetc_rd_reg_hot(reg)	ioread32((reg))
+#define enetc_wr_reg_hot(reg, val)	iowrite32((val), (reg))
+
 #ifdef ioread64
 #define enetc_rd_reg64(reg)	ioread64((reg))
 #else
@@ -345,12 +352,57 @@ static inline u64 enetc_rd_reg64(void __iomem *reg)
 }
 #endif
 
+extern rwlock_t enetc_mdio_lock;
+
+static inline u32 enetc_rd_reg_wa(void *reg)
+{
+	u32 val;
+
+	read_lock(&enetc_mdio_lock);
+	val = ioread32(reg);
+	read_unlock(&enetc_mdio_lock);
+
+	return val;
+}
+
+static inline void enetc_wr_reg_wa(void *reg, u32 val)
+{
+	read_lock(&enetc_mdio_lock);
+	iowrite32(val, reg);
+	read_unlock(&enetc_mdio_lock);
+}
+
+static inline u32 enetc_rd_reg_wa_single(void *reg)
+{
+	unsigned long flags;
+	u32 val;
+
+	write_lock_irqsave(&enetc_mdio_lock, flags);
+	val = ioread32(reg);
+	write_unlock_irqrestore(&enetc_mdio_lock, flags);
+
+	return val;
+}
+
+static inline void enetc_wr_reg_wa_single(void *reg, u32 val)
+{
+	unsigned long flags;
+
+	write_lock_irqsave(&enetc_mdio_lock, flags);
+	iowrite32(val, reg);
+	write_unlock_irqrestore(&enetc_mdio_lock, flags);
+}
+
 #define enetc_rd(hw, off)		enetc_rd_reg((hw)->reg + (off))
 #define enetc_wr(hw, off, val)		enetc_wr_reg((hw)->reg + (off), val)
 #define enetc_rd64(hw, off)		enetc_rd_reg64((hw)->reg + (off))
 /* port register accessors - PF only */
-#define enetc_port_rd(hw, off)		enetc_rd_reg((hw)->port + (off))
-#define enetc_port_wr(hw, off, val)	enetc_wr_reg((hw)->port + (off), val)
+#define enetc_port_rd(hw, off)		enetc_rd_reg_wa((hw)->port + (off))
+#define enetc_port_wr(hw, off, val)	enetc_wr_reg_wa((hw)->port + (off), val)
+#define enetc_port_rd_single(hw, off)		enetc_rd_reg_wa_single(\
+							(hw)->port + (off))
+#define enetc_port_wr_single(hw, off, val)	enetc_wr_reg_wa_single(\
+							(hw)->port + (off), val)
 /* global register accessors - PF only */
 #define enetc_global_rd(hw, off)	enetc_rd_reg((hw)->global + (off))
 #define enetc_global_wr(hw, off, val)	enetc_wr_reg((hw)->global + (off), val)
diff --git a/drivers/net/ethernet/freescale/enetc/enetc_mdio.c b/drivers/net/ethernet/freescale/enetc/enetc_mdio.c
index 48c32a171afa..a4a6f373eb41 100644
--- a/drivers/net/ethernet/freescale/enetc/enetc_mdio.c
+++ b/drivers/net/ethernet/freescale/enetc/enetc_mdio.c
@@ -16,13 +16,13 @@
 
 static inline u32 _enetc_mdio_rd(struct enetc_mdio_priv *mdio_priv, int off)
 {
-	return enetc_port_rd(mdio_priv->hw, mdio_priv->mdio_base + off);
+	return enetc_port_rd_single(mdio_priv->hw, mdio_priv->mdio_base + off);
 }
 
 static inline void _enetc_mdio_wr(struct enetc_mdio_priv *mdio_priv, int off,
 				  u32 val)
 {
-	enetc_port_wr(mdio_priv->hw, mdio_priv->mdio_base + off, val);
+	enetc_port_wr_single(mdio_priv->hw, mdio_priv->mdio_base + off, val);
 }
 
 #define enetc_mdio_rd(mdio_priv, off) \
@@ -174,3 +174,7 @@ struct enetc_hw *enetc_hw_alloc(struct device *dev, void __iomem *port_regs)
 	return hw;
 }
 EXPORT_SYMBOL_GPL(enetc_hw_alloc);
+
+/* Lock for MDIO access errata on LS1028A */
+DEFINE_RWLOCK(enetc_mdio_lock);
+EXPORT_SYMBOL_GPL(enetc_mdio_lock);
-- 
2.17.1

Re: [PATCH net-next] enetc: Workaround for MDIO register access issue

[Andrew Lunn]

On Tue, Nov 10, 2020 at 05:43:04PM +0200, Claudiu Manoil wrote:
> From: Alex Marginean <alexandru.marginean@nxp.com>
> 
> Due to a hardware issue, an access to MDIO registers
> that is concurrent with other ENETC register accesses
> may lead to the MDIO access being dropped or corrupted.
> The workaround introduces locking for all register accesses
> to the ENETC register space.  To reduce performance impact,
> a readers-writers locking scheme has been implemented.
> The writer in this case is the MDIO access code (irrelevant
> whether that MDIO access is a register read or write), and
> the reader is any access code to non-MDIO ENETC registers.
> Also, the datapath functions acquire the read lock fewer times
> and use _hot accessors.  All the rest of the code uses the _wa
> accessors which lock every register access.

Hi Claudiu

The code you are adding makes no comment about the odd using of
read/writer locks. This is going to confused people.

Please could you add helpers, probably as inline functions in a
header, which take/release the read_lock and the write_lock, which
don't use the name read_ or write_. Maybe something like
enetc_lock_mdio()/enetc_unlock_mdio(), enetc_lock_reg(),
enetc_unlock_reg(). Put comments by the helpers explaining what is
going on. That should help avoid future confusion and questions.

Thanks
	Andrew

Re: [PATCH net-next] enetc: Workaround for MDIO register access issue

[Florian Fainelli]

On 11/10/20 7:43 AM, Claudiu Manoil wrote:
> From: Alex Marginean <alexandru.marginean@nxp.com>
> 
> Due to a hardware issue, an access to MDIO registers
> that is concurrent with other ENETC register accesses
> may lead to the MDIO access being dropped or corrupted.
> The workaround introduces locking for all register accesses
> to the ENETC register space.  To reduce performance impact,
> a readers-writers locking scheme has been implemented.
> The writer in this case is the MDIO access code (irrelevant
> whether that MDIO access is a register read or write), and
> the reader is any access code to non-MDIO ENETC registers.
> Also, the datapath functions acquire the read lock fewer times
> and use _hot accessors.  All the rest of the code uses the _wa
> accessors which lock every register access.

I suppose this is the best you can do given the nature of the bug, my
only concern is that it is error prone and may not really scale over
time as you start adding more features/bug fixes that require making
register accesses and use incorrectly _hot in a context where the
reader's lock is not acquired.
-- 
Florian

Re: [PATCH net-next] enetc: Workaround for MDIO register access issue

[Jakub Kicinski]

On Tue, 10 Nov 2020 17:43:04 +0200 Claudiu Manoil wrote:
> From: Alex Marginean <alexandru.marginean@nxp.com>
> 
> Due to a hardware issue, an access to MDIO registers
> that is concurrent with other ENETC register accesses
> may lead to the MDIO access being dropped or corrupted.
> The workaround introduces locking for all register accesses
> to the ENETC register space.  To reduce performance impact,
> a readers-writers locking scheme has been implemented.
> The writer in this case is the MDIO access code (irrelevant
> whether that MDIO access is a register read or write), and
> the reader is any access code to non-MDIO ENETC registers.
> Also, the datapath functions acquire the read lock fewer times
> and use _hot accessors.  All the rest of the code uses the _wa
> accessors which lock every register access.
> 
> Signed-off-by: Alex Marginean <alexandru.marginean@nxp.com>
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> Signed-off-by: Claudiu Manoil <claudiu.manoil@nxp.com>

Please check for new sparse warnings.

RE: [PATCH net-next] enetc: Workaround for MDIO register access issue

[Claudiu Manoil]

>-----Original Message-----
>From: Andrew Lunn <andrew@lunn.ch>
>Sent: Wednesday, November 11, 2020 1:05 AM
>To: Claudiu Manoil <claudiu.manoil@nxp.com>
>Cc: netdev@vger.kernel.org; Jakub Kicinski <kuba@kernel.org>; David S .
>Miller <davem@davemloft.net>; Alexandru Marginean
><alexandru.marginean@nxp.com>; Vladimir Oltean
><vladimir.oltean@nxp.com>
>Subject: Re: [PATCH net-next] enetc: Workaround for MDIO register access
>issue
>
>On Tue, Nov 10, 2020 at 05:43:04PM +0200, Claudiu Manoil wrote:
>> From: Alex Marginean <alexandru.marginean@nxp.com>
>>
>> Due to a hardware issue, an access to MDIO registers
>> that is concurrent with other ENETC register accesses
>> may lead to the MDIO access being dropped or corrupted.
>> The workaround introduces locking for all register accesses
>> to the ENETC register space.  To reduce performance impact,
>> a readers-writers locking scheme has been implemented.
>> The writer in this case is the MDIO access code (irrelevant
>> whether that MDIO access is a register read or write), and
>> the reader is any access code to non-MDIO ENETC registers.
>> Also, the datapath functions acquire the read lock fewer times
>> and use _hot accessors.  All the rest of the code uses the _wa
>> accessors which lock every register access.
>
>Hi Claudiu
>
>The code you are adding makes no comment about the odd using of
>read/writer locks. This is going to confused people.
>
>Please could you add helpers, probably as inline functions in a
>header, which take/release the read_lock and the write_lock, which
>don't use the name read_ or write_. Maybe something like
>enetc_lock_mdio()/enetc_unlock_mdio(), enetc_lock_reg(),
>enetc_unlock_reg(). Put comments by the helpers explaining what is
>going on. That should help avoid future confusion and questions.
>

Good point Andrew, will look into using better names and adding comments.
This patch was actually intended as rfc, the final patch should target the "net" 
tree as a fix.

Thanks.

RE: [PATCH net-next] enetc: Workaround for MDIO register access issue

[Claudiu Manoil]

>-----Original Message-----
>From: Florian Fainelli <f.fainelli@gmail.com>
>Sent: Wednesday, November 11, 2020 1:25 AM
>To: Claudiu Manoil <claudiu.manoil@nxp.com>; netdev@vger.kernel.org
>Cc: Jakub Kicinski <kuba@kernel.org>; David S . Miller
><davem@davemloft.net>; Alexandru Marginean
><alexandru.marginean@nxp.com>; Vladimir Oltean
><vladimir.oltean@nxp.com>
>Subject: Re: [PATCH net-next] enetc: Workaround for MDIO register access
>issue
>
>On 11/10/20 7:43 AM, Claudiu Manoil wrote:
>> From: Alex Marginean <alexandru.marginean@nxp.com>
>>
>> Due to a hardware issue, an access to MDIO registers
>> that is concurrent with other ENETC register accesses
>> may lead to the MDIO access being dropped or corrupted.
>> The workaround introduces locking for all register accesses
>> to the ENETC register space.  To reduce performance impact,
>> a readers-writers locking scheme has been implemented.
>> The writer in this case is the MDIO access code (irrelevant
>> whether that MDIO access is a register read or write), and
>> the reader is any access code to non-MDIO ENETC registers.
>> Also, the datapath functions acquire the read lock fewer times
>> and use _hot accessors.  All the rest of the code uses the _wa
>> accessors which lock every register access.
>
>I suppose this is the best you can do given the nature of the bug, my
>only concern is that it is error prone and may not really scale over
>time as you start adding more features/bug fixes that require making
>register accesses and use incorrectly _hot in a context where the
>reader's lock is not acquired.

Absolutely, maybe I can use lockdep to insure correctness. I started
looking into this. Thanks.

RE: [PATCH net-next] enetc: Workaround for MDIO register access issue

[Claudiu Manoil]

>-----Original Message-----
>From: Jakub Kicinski <kuba@kernel.org>
[...]
>
>Please check for new sparse warnings.

I see... __iomem missing. Thanks.