Move devm_request_irq() after devm_platform_ioremap_resource() so that
dev->emacp is mapped before the interrupt handler can fire.  An early
interrupt hitting emac_irq() would dereference the NULL dev->emacp and
crash.

Fixes: dcc34ef7c834 ("net: ibm: emac: manage emac_irq with devm")
Assisted-by: Opencode:Big-Pickle
Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 drivers/net/ethernet/ibm/emac/core.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/ibm/emac/core.c b/drivers/net/ethernet/ibm/emac/core.c
index b060795dace7..3e8ea0b05619 100644
--- a/drivers/net/ethernet/ibm/emac/core.c
+++ b/drivers/net/ethernet/ibm/emac/core.c
@@ -3044,6 +3044,13 @@ static int emac_probe(struct platform_device *ofdev)
 	if (err)
 		goto err_gone;

+	dev->emacp = devm_platform_ioremap_resource(ofdev, 0);
+	if (IS_ERR(dev->emacp)) {
+		dev_err(&ofdev->dev, "can't map device registers");
+		err = PTR_ERR(dev->emacp);
+		goto err_gone;
+	}
+
 	/* Setup error IRQ handler */
 	dev->emac_irq = platform_get_irq(ofdev, 0);
 	err = devm_request_irq(&ofdev->dev, dev->emac_irq, emac_irq, 0, "EMAC",
@@ -3056,13 +3063,6 @@ static int emac_probe(struct platform_device *ofdev)

 	ndev->irq = dev->emac_irq;

-	dev->emacp = devm_platform_ioremap_resource(ofdev, 0);
-	if (IS_ERR(dev->emacp)) {
-		dev_err(&ofdev->dev, "can't map device registers");
-		err = PTR_ERR(dev->emacp);
-		goto err_gone;
-	}
-
 	/* Wait for dependent devices */
 	err = emac_wait_deps(dev);
 	if (err)
--
2.54.0

On Sat, May 30, 2026 at 07:22:19PM -0700, Rosen Penev wrote:
> Move devm_request_irq() after devm_platform_ioremap_resource() so that
> dev->emacp is mapped before the interrupt handler can fire.  An early
> interrupt hitting emac_irq() would dereference the NULL dev->emacp and
> crash.

Please add an explanation how this can happen, given that iser is
written in open() not probe().

> Fixes: dcc34ef7c834 ("net: ibm: emac: manage emac_irq with devm")

Is this actually bothering people?

	Andrew

On Sun, May 31, 2026 at 7:39 AM Andrew Lunn <andrew@lunn.ch> wrote:
>
> On Sat, May 30, 2026 at 07:22:19PM -0700, Rosen Penev wrote:
> > Move devm_request_irq() after devm_platform_ioremap_resource() so that
> > dev->emacp is mapped before the interrupt handler can fire.  An early
> > interrupt hitting emac_irq() would dereference the NULL dev->emacp and
> > crash.
>
> Please add an explanation how this can happen, given that iser is
> written in open() not probe().
https://sashiko.dev/#/patchset/20260517033621.1839397-1-rosenp%40gmail.com

Also, does requesting the hardware interrupt before mapping the device I/O
registers create a NULL pointer dereference regression in emac_probe()?
The hardware interrupt is requested and enabled via devm_request_irq()
before the device I/O registers are mapped via
devm_platform_ioremap_resource().
If an interrupt is asserted early, emac_irq() will execute immediately and
attempt to read the ISR register by dereferencing dev->emacp. Since
dev->emacp is still NULL at this point, this would cause a kernel panic.
>
> > Fixes: dcc34ef7c834 ("net: ibm: emac: manage emac_irq with devm")
This is mine btw.
>
> Is this actually bothering people?
It's bothering the bot. The problem here is any other commit will also
trigger this nonsense. Having devm_platform_ioremap_resource earlier
like this is more consistent with other drivers as well.
>
>         Andrew

On Sun, May 31, 2026 at 01:52:04PM -0700, Rosen Penev wrote:
> On Sun, May 31, 2026 at 7:39 AM Andrew Lunn <andrew@lunn.ch> wrote:
> >
> > On Sat, May 30, 2026 at 07:22:19PM -0700, Rosen Penev wrote:
> > > Move devm_request_irq() after devm_platform_ioremap_resource() so that
> > > dev->emacp is mapped before the interrupt handler can fire.  An early
> > > interrupt hitting emac_irq() would dereference the NULL dev->emacp and
> > > crash.
> >
> > Please add an explanation how this can happen, given that iser is
> > written in open() not probe().
> https://sashiko.dev/#/patchset/20260517033621.1839397-1-rosenp%40gmail.com
> 
> Also, does requesting the hardware interrupt before mapping the device I/O
> registers create a NULL pointer dereference regression in emac_probe()?
> The hardware interrupt is requested and enabled via devm_request_irq()
> before the device I/O registers are mapped via
> devm_platform_ioremap_resource().
> If an interrupt is asserted early, emac_irq() will execute immediately and
> attempt to read the ISR register by dereferencing dev->emacp. Since
> dev->emacp is still NULL at this point, this would cause a kernel panic.

You have still not explained how an interrupt could happen before the
interrupts are enabled by writing to the iser in open(). I don't care
too much what the AI says, i want a human to explain how this can
happen. You have put your Signed-off-by: here, so it is down to you to
explain your patch, even is AI told you to do this, and you blindly
did it.

> Having devm_platform_ioremap_resource earlier
> like this is more consistent with other drivers as well.

So maybe this is a better thing to put in the commit message, rather
than something you cannot actually explain?

     Andrew

On Sun, May 31, 2026 at 4:06 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> On Sun, May 31, 2026 at 01:52:04PM -0700, Rosen Penev wrote:
> > On Sun, May 31, 2026 at 7:39 AM Andrew Lunn <andrew@lunn.ch> wrote:
> > >
> > > On Sat, May 30, 2026 at 07:22:19PM -0700, Rosen Penev wrote:
> > > > Move devm_request_irq() after devm_platform_ioremap_resource() so that
> > > > dev->emacp is mapped before the interrupt handler can fire.  An early
> > > > interrupt hitting emac_irq() would dereference the NULL dev->emacp and
> > > > crash.
> > >
> > > Please add an explanation how this can happen, given that iser is
> > > written in open() not probe().
> > https://sashiko.dev/#/patchset/20260517033621.1839397-1-rosenp%40gmail.com
> >
> > Also, does requesting the hardware interrupt before mapping the device I/O
> > registers create a NULL pointer dereference regression in emac_probe()?
> > The hardware interrupt is requested and enabled via devm_request_irq()
> > before the device I/O registers are mapped via
> > devm_platform_ioremap_resource().
> > If an interrupt is asserted early, emac_irq() will execute immediately and
> > attempt to read the ISR register by dereferencing dev->emacp. Since
> > dev->emacp is still NULL at this point, this would cause a kernel panic.
>
> You have still not explained how an interrupt could happen before the
> interrupts are enabled by writing to the iser in open(). I don't care
> too much what the AI says, i want a human to explain how this can
> happen. You have put your Signed-off-by: here, so it is down to you to
> explain your patch, even is AI told you to do this, and you blindly
> did it.
Beats me. When I did the devm conversions, I tested the hardware to
make sure it comes up and is usable. Various probe ordering tests were
not done.

FWIW, the AI is probably correct. How correct it is is beyond my pay grade.
>
> > Having devm_platform_ioremap_resource earlier
> > like this is more consistent with other drivers as well.
>
> So maybe this is a better thing to put in the commit message, rather
> than something you cannot actually explain?
Sure. Although this patch is targeted towards net, not net-next. Just
saying it's more consistent will get a flat out rejection.
>
>      Andrew