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

[PATCH net] 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.
The commit introducing MDIO support is -
commit ebfcb23d62ab ("enetc: Add ENETC PF level external MDIO support")
but due to subsequent refactoring this patch is applicable on
top of a later commit.

Fixes: 6517798dd343 ("enetc: Make MDIO accessors more generic and export to include/linux/fsl")

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>
---
v0: since initial "RFC" sent on net-next:
    - cleaned up w/a accessors, added comments in the code,
    lockdep checks, minor fixes
    - can be backported to 5.9 w/ trivial conflict in Kconfig

 drivers/net/ethernet/freescale/enetc/Kconfig  |   1 +
 drivers/net/ethernet/freescale/enetc/enetc.c  |  62 +++++++---
 .../net/ethernet/freescale/enetc/enetc_hw.h   | 115 +++++++++++++++++-
 .../net/ethernet/freescale/enetc/enetc_mdio.c |   8 +-
 4 files changed, 161 insertions(+), 25 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 52be6e315752..fc2075ea57fe 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;
 	}
 
+	enetc_lock_mdio();
 	count = enetc_map_tx_buffs(tx_ring, skb, priv->active_offloads);
+	enetc_unlock_mdio();
+
 	if (unlikely(!count))
 		goto drop_packet_err;
 
@@ -239,7 +242,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;
 
@@ -262,12 +265,16 @@ static irqreturn_t enetc_msix(int irq, void *data)
 	struct enetc_int_vector	*v = data;
 	int i;
 
+	enetc_lock_mdio();
+
 	/* 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);
+
+	enetc_unlock_mdio();
 
 	napi_schedule(&v->napi);
 
@@ -334,19 +341,23 @@ static int enetc_poll(struct napi_struct *napi, int budget)
 
 	v->rx_napi_work = false;
 
+	enetc_lock_mdio();
+
 	/* 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);
+
+	enetc_unlock_mdio();
 
 	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;
 }
@@ -386,7 +397,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];
+
+	enetc_lock_mdio();
 	bds_to_clean = enetc_bd_ready_count(tx_ring, i);
+	enetc_unlock_mdio();
 
 	do_tstamp = false;
 
@@ -429,16 +443,20 @@ static bool enetc_clean_tx_ring(struct enetc_bdr *tx_ring, int napi_budget)
 			tx_swbd = tx_ring->tx_swbd;
 		}
 
+		enetc_lock_mdio();
+
 		/* 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);
+
+		enetc_unlock_mdio();
 	}
 
 	tx_ring->next_to_clean = i;
@@ -515,8 +533,6 @@ static int enetc_refill_rx_ring(struct enetc_bdr *rx_ring, const int buff_cnt)
 	if (likely(j)) {
 		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);
 	}
 
 	return j;
@@ -534,8 +550,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)
@@ -684,23 +700,31 @@ static int enetc_clean_rx_ring(struct enetc_bdr *rx_ring,
 		u32 bd_status;
 		u16 size;
 
+		enetc_lock_mdio();
+
 		if (cleaned_cnt >= ENETC_RXBD_BUNDLE) {
 			int count = enetc_refill_rx_ring(rx_ring, cleaned_cnt);
 
+			/* update ENETC's consumer index */
+			enetc_wr_reg_hot(rx_ring->rcir, rx_ring->next_to_use);
 			cleaned_cnt -= count;
 		}
 
 		rxbd = enetc_rxbd(rx_ring, i);
 		bd_status = le32_to_cpu(rxbd->r.lstatus);
-		if (!bd_status)
+		if (!bd_status) {
+			enetc_unlock_mdio();
 			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) {
+			enetc_unlock_mdio();
 			break;
+		}
 
 		enetc_get_offloads(rx_ring, rxbd, skb);
 
@@ -712,6 +736,7 @@ static int enetc_clean_rx_ring(struct enetc_bdr *rx_ring,
 
 		if (unlikely(bd_status &
 			     ENETC_RXBD_LSTATUS(ENETC_RXBD_ERR_MASK))) {
+			enetc_unlock_mdio();
 			dev_kfree_skb(skb);
 			while (!(bd_status & ENETC_RXBD_LSTATUS_F)) {
 				dma_rmb();
@@ -751,6 +776,8 @@ static int enetc_clean_rx_ring(struct enetc_bdr *rx_ring,
 
 		enetc_process_skb(rx_ring, skb);
 
+		enetc_unlock_mdio();
+
 		napi_gro_receive(napi, skb);
 
 		rx_frm_cnt++;
@@ -1225,6 +1252,7 @@ static void enetc_setup_rxbdr(struct enetc_hw *hw, struct enetc_bdr *rx_ring)
 	rx_ring->idr = hw->reg + ENETC_SIRXIDR;
 
 	enetc_refill_rx_ring(rx_ring, enetc_bd_unused(rx_ring));
+	enetc_wr(hw, ENETC_SIRXIDR, rx_ring->next_to_use);
 
 	/* enable ring */
 	enetc_rxbdr_wr(hw, idx, ENETC_RBMR, rbmr);
diff --git a/drivers/net/ethernet/freescale/enetc/enetc_hw.h b/drivers/net/ethernet/freescale/enetc/enetc_hw.h
index 17cf7c94fdb5..eb6bbf1113c7 100644
--- a/drivers/net/ethernet/freescale/enetc/enetc_hw.h
+++ b/drivers/net/ethernet/freescale/enetc/enetc_hw.h
@@ -324,14 +324,100 @@ struct enetc_hw {
 	void __iomem *global;
 };
 
-/* general register accessors */
-#define enetc_rd_reg(reg)	ioread32((reg))
-#define enetc_wr_reg(reg, val)	iowrite32((val), (reg))
+/* ENETC register accessors */
+
+/* MDIO issue workaround (on LS1028A) -
+ * 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.
+ * To protect the MDIO accesses a readers-writers locking
+ * scheme is used, where the MDIO register accesses are
+ * protected by write locks to insure exclusivity, while
+ * the remaining ENETC registers are accessed under read
+ * locks since they only compete with MDIO accesses.
+ */
+extern rwlock_t enetc_mdio_lock;
+
+/* use this locking primitive only on the fast datapath to
+ * group together multiple non-MDIO register accesses to
+ * minimize the overhead of the lock
+ */
+static inline void enetc_lock_mdio(void)
+{
+	read_lock(&enetc_mdio_lock);
+}
+
+static inline void enetc_unlock_mdio(void)
+{
+	read_unlock(&enetc_mdio_lock);
+}
+
+/* use these accessors only on the fast datapath under
+ * the enetc_lock_mdio() locking primitive to minimize
+ * the overhead of the lock
+ */
+static inline u32 enetc_rd_reg_hot(void __iomem *reg)
+{
+	lockdep_assert_held(&enetc_mdio_lock);
+
+	return ioread32(reg);
+}
+
+static inline void enetc_wr_reg_hot(void __iomem *reg, u32 val)
+{
+	lockdep_assert_held(&enetc_mdio_lock);
+
+	iowrite32(val, reg);
+}
+
+/* internal helpers for the MDIO w/a */
+static inline u32 _enetc_rd_reg_wa(void __iomem *reg)
+{
+	u32 val;
+
+	enetc_lock_mdio();
+	val = ioread32(reg);
+	enetc_unlock_mdio();
+
+	return val;
+}
+
+static inline void _enetc_wr_reg_wa(void __iomem *reg, u32 val)
+{
+	enetc_lock_mdio();
+	iowrite32(val, reg);
+	enetc_unlock_mdio();
+}
+
+static inline u32 _enetc_rd_mdio_reg_wa(void __iomem *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_mdio_reg_wa(void __iomem *reg, u32 val)
+{
+	unsigned long flags;
+
+	write_lock_irqsave(&enetc_mdio_lock, flags);
+	iowrite32(val, reg);
+	write_unlock_irqrestore(&enetc_mdio_lock, flags);
+}
+
 #ifdef ioread64
-#define enetc_rd_reg64(reg)	ioread64((reg))
+static inline u64 _enetc_rd_reg64(void __iomem *reg)
+{
+	return ioread64(reg);
+}
 #else
 /* using this to read out stats on 32b systems */
-static inline u64 enetc_rd_reg64(void __iomem *reg)
+static inline u64 _enetc_rd_reg64(void __iomem *reg)
 {
 	u32 low, high, tmp;
 
@@ -345,12 +431,29 @@ static inline u64 enetc_rd_reg64(void __iomem *reg)
 }
 #endif
 
+static inline u64 _enetc_rd_reg64_wa(void __iomem *reg)
+{
+	u64 val;
+
+	enetc_lock_mdio();
+	val = _enetc_rd_reg64(reg);
+	enetc_unlock_mdio();
+
+	return val;
+}
+
+/* general register accessors */
+#define enetc_rd_reg(reg)		_enetc_rd_reg_wa((reg))
+#define enetc_wr_reg(reg, val)		_enetc_wr_reg_wa((reg), (val))
 #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))
+#define enetc_rd64(hw, off)		_enetc_rd_reg64_wa((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_mdio(hw, off)	_enetc_rd_mdio_reg_wa((hw)->port + (off))
+#define enetc_port_wr_mdio(hw, off, val)	_enetc_wr_mdio_reg_wa(\
+							(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..ee0116ed4738 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_mdio(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_mdio(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] enetc: Workaround for MDIO register access issue

[Andrew Lunn]

> +static inline void enetc_lock_mdio(void)
> +{
> +	read_lock(&enetc_mdio_lock);
> +}
> +

> +static inline u32 _enetc_rd_mdio_reg_wa(void __iomem *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;
> +}

Can you mix read_lock() with write_lock_irqsave()?  Normal locks you
should not mix, so i assume read/writes also cannot be mixed?

       Andrew

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

[Claudiu Manoil]

>-----Original Message-----
>From: Andrew Lunn <andrew@lunn.ch>
>Sent: Tuesday, November 17, 2020 4:45 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] enetc: Workaround for MDIO register access issue
>
>> +static inline void enetc_lock_mdio(void)
>> +{
>> +	read_lock(&enetc_mdio_lock);
>> +}
>> +
>
>> +static inline u32 _enetc_rd_mdio_reg_wa(void __iomem *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;
>> +}
>
>Can you mix read_lock() with write_lock_irqsave()?  Normal locks you
>should not mix, so i assume read/writes also cannot be mixed?
>

Not sure I understand your concerns, but this is the readers-writers locking
scheme. The readers (read_lock) are "lightweight", they get the most calls,
can be taken from any context including interrupt context, and compete only
with the writers (write_lock). The writers can take the lock only when there are
no readers holding it, and the writer must insure that it doesn't get preempted
(by interrupts etc.) when holding the lock (irqsave). The good part is that mdio
operations are not frequent. Also, we had this code out of the tree for quite some
time, it's well exercised.

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

[Jakub Kicinski]

On Thu, 12 Nov 2020 20:26:08 +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.
> The commit introducing MDIO support is -
> commit ebfcb23d62ab ("enetc: Add ENETC PF level external MDIO support")
> but due to subsequent refactoring this patch is applicable on
> top of a later commit.
> 
> Fixes: 6517798dd343 ("enetc: Make MDIO accessors more generic and export to include/linux/fsl")
> 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>

Applied.

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

[Andrew Lunn]

On Tue, Nov 17, 2020 at 10:22:20AM +0000, Claudiu Manoil wrote:
> >-----Original Message-----
> >From: Andrew Lunn <andrew@lunn.ch>
> >Sent: Tuesday, November 17, 2020 4:45 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] enetc: Workaround for MDIO register access issue
> >
> >> +static inline void enetc_lock_mdio(void)
> >> +{
> >> +	read_lock(&enetc_mdio_lock);
> >> +}
> >> +
> >
> >> +static inline u32 _enetc_rd_mdio_reg_wa(void __iomem *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;
> >> +}
> >
> >Can you mix read_lock() with write_lock_irqsave()?  Normal locks you
> >should not mix, so i assume read/writes also cannot be mixed?
> >
> 
> Not sure I understand your concerns, but this is the readers-writers locking
> scheme. The readers (read_lock) are "lightweight", they get the most calls,
> can be taken from any context including interrupt context, and compete only
> with the writers (write_lock). The writers can take the lock only when there are
> no readers holding it, and the writer must insure that it doesn't get preempted
> (by interrupts etc.) when holding the lock (irqsave). The good part is that mdio
> operations are not frequent. Also, we had this code out of the tree for quite some
> time, it's well exercised.

Hi CLaidiu

Thanks for the explanation. I don't think i've every reviewed a driver
using read/write locks like this. But thinking it through, it does
seem O.K.

     Andrew

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

[Vladimir Oltean]

On Wed, Nov 18, 2020 at 02:38:56PM +0100, Andrew Lunn wrote:
> On Tue, Nov 17, 2020 at 10:22:20AM +0000, Claudiu Manoil wrote:
> > >-----Original Message-----
> > >From: Andrew Lunn <andrew@lunn.ch>
> > >Sent: Tuesday, November 17, 2020 4:45 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] enetc: Workaround for MDIO register access issue
> > >
> > >> +static inline void enetc_lock_mdio(void)
> > >> +{
> > >> +	read_lock(&enetc_mdio_lock);
> > >> +}
> > >> +
> > >
> > >> +static inline u32 _enetc_rd_mdio_reg_wa(void __iomem *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;
> > >> +}
> > >
> > >Can you mix read_lock() with write_lock_irqsave()?  Normal locks you
> > >should not mix, so i assume read/writes also cannot be mixed?
> > >
> > 
> > Not sure I understand your concerns, but this is the readers-writers locking
> > scheme. The readers (read_lock) are "lightweight", they get the most calls,
> > can be taken from any context including interrupt context, and compete only
> > with the writers (write_lock). The writers can take the lock only when there are
> > no readers holding it, and the writer must insure that it doesn't get preempted
> > (by interrupts etc.) when holding the lock (irqsave). The good part is that mdio
> > operations are not frequent. Also, we had this code out of the tree for quite some
> > time, it's well exercised.
> 
> Hi CLaidiu
> 
> Thanks for the explanation. I don't think i've every reviewed a driver
> using read/write locks like this. But thinking it through, it does
> seem O.K.

Thanks for reviewing and getting this merged. It sure is helpful to not
have the link flap while running iperf3 or other intensive network
activity.

Even if this use of rwlocks may seem unconventional, I think it is the
right tool for working around the hardware bug.

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

[Jakub Kicinski]

On Wed, 18 Nov 2020 19:00:44 +0200 Vladimir Oltean wrote:
> On Wed, Nov 18, 2020 at 02:38:56PM +0100, Andrew Lunn wrote:
> > Thanks for the explanation. I don't think i've every reviewed a driver
> > using read/write locks like this. But thinking it through, it does
> > seem O.K.  
> 
> Thanks for reviewing and getting this merged. It sure is helpful to not
> have the link flap while running iperf3 or other intensive network
> activity.
> 
> Even if this use of rwlocks may seem unconventional, I think it is the
> right tool for working around the hardware bug.

Out of curiosity - did you measure the performance hit?

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

[Vladimir Oltean]

On Wed, Nov 18, 2020 at 09:03:43AM -0800, Jakub Kicinski wrote:
> On Wed, 18 Nov 2020 19:00:44 +0200 Vladimir Oltean wrote:
> > On Wed, Nov 18, 2020 at 02:38:56PM +0100, Andrew Lunn wrote:
> > > Thanks for the explanation. I don't think i've every reviewed a driver
> > > using read/write locks like this. But thinking it through, it does
> > > seem O.K.
> >
> > Thanks for reviewing and getting this merged. It sure is helpful to not
> > have the link flap while running iperf3 or other intensive network
> > activity.
> >
> > Even if this use of rwlocks may seem unconventional, I think it is the
> > right tool for working around the hardware bug.
>
> Out of curiosity - did you measure the performance hit?

It's not something that is noticeable, at least on 1Gbps where I'm
testing now. I'm not even sure what a valid metric would be. The CPU
utilization is about the same, the throughput across a 100 second iperf3
TCP test is the same (942 Mbits/sec at sender), and when I look at the
perf events for CPU cycles I get the feeling that any variation there is
mostly noise. There doesn't seem to be any outlier.

I might come back to this when I submit some hardware bug workarounds of
my own, for the 2.5Gbps ENETC that acts as a DSA master for the Ocelot
switch. There, I have a chance of testing at 2.5Gbps. But I don't have
that set up right now.