[PATCH net v2] net: ibm: emac: fix MR0 write in emac_rx_enable()

[PATCH net v2] net: ibm: emac: fix MR0 write in emac_rx_enable()

[Rosen Penev]

When an asynchronous RX disable is still in progress, emac_rx_enable()
waits for MR0[RXI] with

	while (!(r = in_be32(&p->mr0) & EMAC_MR0_RXI) && n)

which assigns only the RXI bit to r, as & binds tighter than =. The
following write of r | EMAC_MR0_RXE then clears every other MR0 bit,
including TXE, and TX stays stopped until the watchdog resets the EMAC.

This path is taken when RX is restarted after an RX descriptor error.

Replace the open-coded loop with read_poll_timeout_atomic(), which
stores the full register value in r.

Fixes: 1d3bb996481e ("Device tree aware EMAC driver")
Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 v2: use read_poll_timeout_atomic
 drivers/net/ethernet/ibm/emac/core.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/ibm/emac/core.c b/drivers/net/ethernet/ibm/emac/core.c
index 1b95b4d5deff..0311131055f7 100644
--- a/drivers/net/ethernet/ibm/emac/core.c
+++ b/drivers/net/ethernet/ibm/emac/core.c
@@ -24,6 +24,7 @@
 #include <linux/string.h>
 #include <linux/errno.h>
 #include <linux/delay.h>
+#include <linux/iopoll.h>
 #include <linux/types.h>
 #include <linux/pci.h>
 #include <linux/etherdevice.h>
@@ -248,12 +249,10 @@ static void emac_rx_enable(struct emac_instance *dev)
 	if (!(r & EMAC_MR0_RXE)) {
 		if (unlikely(!(r & EMAC_MR0_RXI))) {
 			/* Wait if previous async disable is still in progress */
-			int n = dev->stop_timeout;
-			while (!(r = in_be32(&p->mr0) & EMAC_MR0_RXI) && n) {
-				udelay(1);
-				--n;
-			}
-			if (unlikely(!n))
+			if (read_poll_timeout_atomic(in_be32, r,
+						     r & EMAC_MR0_RXI, 1,
+						     dev->stop_timeout, false,
+						     &p->mr0))
 				emac_report_timeout_error(dev,
 							  "RX disable timeout");
 		}
-- 
2.56.0

Re: [PATCH net v2] net: ibm: emac: fix MR0 write in emac_rx_enable()

[Daniel Machon]

Hi Rosen,

> When an asynchronous RX disable is still in progress, emac_rx_enable()
> waits for MR0[RXI] with
> 
> 	while (!(r = in_be32(&p->mr0) & EMAC_MR0_RXI) && n)
> 
> which assigns only the RXI bit to r, as & binds tighter than =. The
> following write of r | EMAC_MR0_RXE then clears every other MR0 bit,
> including TXE, and TX stays stopped until the watchdog resets the EMAC.
> 
> This path is taken when RX is restarted after an RX descriptor error.
> 
> Replace the open-coded loop with read_poll_timeout_atomic(), which
> stores the full register value in r.
> 
> Fixes: 1d3bb996481e ("Device tree aware EMAC driver")
> Signed-off-by: Rosen Penev <rosenp@gmail.com>
> ---
>  v2: use read_poll_timeout_atomic
>  drivers/net/ethernet/ibm/emac/core.c | 11 +++++------
>  1 file changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/ethernet/ibm/emac/core.c b/drivers/net/ethernet/ibm/emac/core.c
> index 1b95b4d5deff..0311131055f7 100644
> --- a/drivers/net/ethernet/ibm/emac/core.c
> +++ b/drivers/net/ethernet/ibm/emac/core.c
> @@ -24,6 +24,7 @@
>  #include <linux/string.h>
>  #include <linux/errno.h>
>  #include <linux/delay.h>
> +#include <linux/iopoll.h>
>  #include <linux/types.h>
>  #include <linux/pci.h>
>  #include <linux/etherdevice.h>
> @@ -248,12 +249,10 @@ static void emac_rx_enable(struct emac_instance *dev)
>  	if (!(r & EMAC_MR0_RXE)) {
>  		if (unlikely(!(r & EMAC_MR0_RXI))) {
>  			/* Wait if previous async disable is still in progress */
> -			int n = dev->stop_timeout;
> -			while (!(r = in_be32(&p->mr0) & EMAC_MR0_RXI) && n) {
> -				udelay(1);
> -				--n;
> -			}
> -			if (unlikely(!n))
> +			if (read_poll_timeout_atomic(in_be32, r,
> +						     r & EMAC_MR0_RXI, 1,
> +						     dev->stop_timeout, false,
> +						     &p->mr0))
>  				emac_report_timeout_error(dev,
>  							  "RX disable timeout");
>  		}
> -- 
> 2.56.0
>

I think the v2 conversion to read_poll_timeout_atomic() looks good. But I wonder
why you dropped the tested line in v2? Was this version not tested? Either way,
the commit message should say so.

/Daniel