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

[Rosen Penev]

On Thu, Nov 12, 2026 at 4:08 AM Daniel Machon
<daniel.machon@xxxxxxxxxxxxx> wrote:
>
> 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@xxxxxxxxx>
> > ---
> >  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.
v1 was not tested separately from other changes. v2 was not tested at
all. The whole point of iopoll.h is to prevent bugs.
>
> /Daniel
>