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

[PATCH net] 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.

Tested on a Cisco Meraki MX60.

Fixes: 1d3bb996481e ("Device tree aware EMAC driver")
Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 drivers/net/ethernet/ibm/emac/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/ibm/emac/core.c b/drivers/net/ethernet/ibm/emac/core.c
index 640618c0a09e..b676b286e339 100644
--- a/drivers/net/ethernet/ibm/emac/core.c
+++ b/drivers/net/ethernet/ibm/emac/core.c
@@ -249,7 +249,7 @@ static void emac_rx_enable(struct emac_instance *dev)
 		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) {
+			while (!((r = in_be32(&p->mr0)) & EMAC_MR0_RXI) && n) {
 				udelay(1);
 				--n;
 			}
-- 
2.56.0

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

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - How the issue was discovered, e.g. hit in production, hit during
   development, syzbot report, manual code inspection, LLM or static
   analysis tool scan.

 - Whether the issue was actually triggered, or is only theoretical
   (e.g. found by code inspection). If it was triggered please include
   the symptoms, like the stack trace or error messages.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.

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

[Andrew Lunn]

On Thu, Nov 05, 2026 at 03:14:17PM -0800, Rosen Penev wrote:
> 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.
> 
> Tested on a Cisco Meraki MX60.
> 
> Fixes: 1d3bb996481e ("Device tree aware EMAC driver")
> Signed-off-by: Rosen Penev <rosenp@gmail.com>
> ---
>  drivers/net/ethernet/ibm/emac/core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/ibm/emac/core.c b/drivers/net/ethernet/ibm/emac/core.c
> index 640618c0a09e..b676b286e339 100644
> --- a/drivers/net/ethernet/ibm/emac/core.c
> +++ b/drivers/net/ethernet/ibm/emac/core.c
> @@ -249,7 +249,7 @@ static void emac_rx_enable(struct emac_instance *dev)
>  		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) {
> +			while (!((r = in_be32(&p->mr0)) & EMAC_MR0_RXI) && n) {
>  				udelay(1);
>  				--n;

This sort of code should really make use of iopoll.h

	Andrew

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

[Rosen Penev]

On Sat, Nov 7, 2026 at 9:35 AM Andrew Lunn <andrew@lunn.ch> wrote:
>
> On Thu, Nov 05, 2026 at 03:14:17PM -0800, Rosen Penev wrote:
> > 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.
> >
> > Tested on a Cisco Meraki MX60.
> >
> > Fixes: 1d3bb996481e ("Device tree aware EMAC driver")
> > Signed-off-by: Rosen Penev <rosenp@gmail.com>
> > ---
> >  drivers/net/ethernet/ibm/emac/core.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/ethernet/ibm/emac/core.c b/drivers/net/ethernet/ibm/emac/core.c
> > index 640618c0a09e..b676b286e339 100644
> > --- a/drivers/net/ethernet/ibm/emac/core.c
> > +++ b/drivers/net/ethernet/ibm/emac/core.c
> > @@ -249,7 +249,7 @@ static void emac_rx_enable(struct emac_instance *dev)
> >               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) {
> > +                     while (!((r = in_be32(&p->mr0)) & EMAC_MR0_RXI) && n) {
> >                               udelay(1);
> >                               --n;
>
> This sort of code should really make use of iopoll.h
I agree. Is it appropriate for net?
>
>         Andrew

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

[Andrew Lunn]

On Sat, Nov 07, 2026 at 04:07:04PM -0800, Rosen Penev wrote:
> On Sat, Nov 7, 2026 at 9:35 AM Andrew Lunn <andrew@lunn.ch> wrote:
> >
> > On Thu, Nov 05, 2026 at 03:14:17PM -0800, Rosen Penev wrote:
> > > 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.
> > >
> > > Tested on a Cisco Meraki MX60.
> > >
> > > Fixes: 1d3bb996481e ("Device tree aware EMAC driver")
> > > Signed-off-by: Rosen Penev <rosenp@gmail.com>
> > > ---
> > >  drivers/net/ethernet/ibm/emac/core.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/net/ethernet/ibm/emac/core.c b/drivers/net/ethernet/ibm/emac/core.c
> > > index 640618c0a09e..b676b286e339 100644
> > > --- a/drivers/net/ethernet/ibm/emac/core.c
> > > +++ b/drivers/net/ethernet/ibm/emac/core.c
> > > @@ -249,7 +249,7 @@ static void emac_rx_enable(struct emac_instance *dev)
> > >               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) {
> > > +                     while (!((r = in_be32(&p->mr0)) & EMAC_MR0_RXI) && n) {
> > >                               udelay(1);
> > >                               --n;
> >
> > This sort of code should really make use of iopoll.h
> I agree. Is it appropriate for net?

Yes, that is fine.

     Andrew