[PATCH net v1] net: emac: move setting of netops to fix crash

[PATCH net v1] net: emac: move setting of netops to fix crash

[Christian Lamparter]

fixes the following crash on driver initialization:

|BUG: Kernel NULL pointer dereference on read at 0x00000158
|Faulting instruction address: 0xc0566b40
|Oops: Kernel access of bad area, sig: 11 [#1]
|BE PAGE_SIZE=4K  PowerPC 44x Platform
|Modules linked in:
|CPU: 0 UID: 0 PID: 1 Comm: swapper/0 Tainted: GW 7.3.0-rc3+ #1
|Tainted: [W]=WARN
|Hardware name: MyBook Live APM821XX 0x12c41c83 PowerPC 44x Platform
|NIP:  c0566b40 LR: c05648f8 CTR: c04c1e9c
|REGS: c1053a20 TRAP: 0300   Tainted: GW  (7.3.0-rc3+)
|MSR:  0002b000 <CE,EE,FP,ME>  CR: 24008808  XER: 00000000
|DEAR: 00000158 ESR: 00000000
|GPR00: c05648f8 c1053b10 c1063600 c1030000 c5ab3000 00000000 [...]
|GPR08: 00000002 00000000 00000000 c1053b40 84002808 00000000 [...]
|GPR16: cfffd210 00000002 c0beafcc cfffc960 00000000 c1030644 [...]
|GPR24: c0beafbc c1037000 00000000 0000000a 00000000 c1030000 [...]
|NIP [c0566b40] phy_link_topo_add_phy+0x2c/0x1d0
|LR [c05648f8] phy_attach_direct+0x1a4/0x368
|Call Trace:
|[c1053b10] [c0811e04] klist_put+0x54/0xb4 (unreliable)
|[c1053b40] [c05648f8] phy_attach_direct+0x1a4/0x368
|[c1053b70] [c0564ae8] phy_connect_direct+0x2c/0x60
|[c1053b90] [c056c7a8] of_phy_connect+0x50/0x74
|[c1053bc0] [c0572a88] emac_probe+0xd50/0x119c
|[c1053c90] [c04cb770] platform_probe+0x74/0xa4
|[c1053cb0] [c04c8f08] really_probe+0x120/0x2b0
|[c1053cd0] [c04c9254] __driver_probe_device+0x1bc/0x1fc
|[c1053d00] [c04c9334] driver_probe_device+0x38/0xa8
|[c1053d30] [c04c9560] __driver_attach+0xf4/0x10c
|[c1053d50] [c04c6af8] bus_for_each_dev+0x68/0xd0
|[c1053d90] [c04c7c34] bus_add_driver+0xcc/0x1ec
|[c1053dc0] [c04c9f6c] driver_register+0xcc/0x110
|[c1053de0] [c0aa3f40] emac_init+0x1c4/0x200

This bug showed up starting with v7.3-rc1. At the NIP in
phy_link_topo_add_phy() is a netdev_need_ops_lock() check.
This was added by the following
commit ded86da4bbb7 ("net: ethtool: relax ethnl_req_get_phydev() locking assertion")

The bug shows up because at the time of_phy_connect() was called, the
netdev_ops were *not yet* determined. My fix is to move the code that
sets netdev_ops+ethtool_ops further up as emac_init_config() derives
that by looking at the device-tree and sets the required dev->phy_mode
accordingly.

This patch was tested on a WD MyBook Live. the device now works again.

Fixes: ded86da4bbb7 ("net: ethtool: relax ethnl_req_get_phydev() locking assertion")
Signed-off-by: Christian Lamparter <chunkeey@gmail.com>
---
 drivers/net/ethernet/ibm/emac/core.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/ibm/emac/core.c b/drivers/net/ethernet/ibm/emac/core.c
index 1d46cf6c2c12..48ece86bcc8e 100644
--- a/drivers/net/ethernet/ibm/emac/core.c
+++ b/drivers/net/ethernet/ibm/emac/core.c
@@ -3044,6 +3044,14 @@ static int emac_probe(struct platform_device *ofdev)
 	if (err)
 		goto err_gone;
 
+	if (emac_phy_supports_gige(dev->phy_mode)) {
+		ndev->netdev_ops = &emac_gige_netdev_ops;
+		dev->commac.ops = &emac_commac_sg_ops;
+	} else {
+		ndev->netdev_ops = &emac_netdev_ops;
+	}
+	ndev->ethtool_ops = &emac_ethtool_ops;
+
 	dev->emacp = devm_platform_ioremap_resource(ofdev, 0);
 	if (IS_ERR(dev->emacp)) {
 		err = PTR_ERR(dev->emacp);
@@ -3144,12 +3152,6 @@ static int emac_probe(struct platform_device *ofdev)
 		ndev->features |= ndev->hw_features | NETIF_F_RXCSUM;
 	}
 	ndev->watchdog_timeo = 5 * HZ;
-	if (emac_phy_supports_gige(dev->phy_mode)) {
-		ndev->netdev_ops = &emac_gige_netdev_ops;
-		dev->commac.ops = &emac_commac_sg_ops;
-	} else
-		ndev->netdev_ops = &emac_netdev_ops;
-	ndev->ethtool_ops = &emac_ethtool_ops;
 
 	/* MTU range: 46 - 1500 or whatever is in OF */
 	ndev->min_mtu = EMAC_MIN_MTU;
-- 
2.55.0

Re: [PATCH net v1] net: emac: move setting of netops to fix crash

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 1 · Medium: 0 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] In `emac_probe()` (drivers/net/ethernet/ibm/emac/core.c) the
  relocated Gigabit branch now sets `dev->commac.ops =…

--

net: emac: move setting of netops to fix crash

The patch moves the netdev_ops/ethtool_ops assignment in emac_probe() to
right after emac_init_config(), so that ndev->netdev_ops is populated
before of_phy_connect() runs the netdev_need_ops_lock() check added by
commit ded86da4bbb7. The dev->commac.ops selection was moved along with
it.

> diff --git a/drivers/net/ethernet/ibm/emac/core.c b/drivers/net/ethernet/ibm/emac/core.c
> index 1d46cf6c2c127..48ece86bcc8e0 100644
> --- a/drivers/net/ethernet/ibm/emac/core.c
> +++ b/drivers/net/ethernet/ibm/emac/core.c
> @@ -3044,6 +3044,14 @@ static int emac_probe(struct platform_device *ofdev)
>  	if (err)
>  		goto err_gone;
>  
> +	if (emac_phy_supports_gige(dev->phy_mode)) {
> +		ndev->netdev_ops = &emac_gige_netdev_ops;
> +		dev->commac.ops = &emac_commac_sg_ops;
> +	} else {
> +		ndev->netdev_ops = &emac_netdev_ops;
> +	}
> +	ndev->ethtool_ops = &emac_ethtool_ops;
> +

[Severity: High]
Does moving dev->commac.ops together with netdev_ops make this store dead?
Later in emac_probe(), the "Register with MAL" block still overwrites it
unconditionally:

drivers/net/ethernet/ibm/emac/core.c:emac_probe() {
	...
	/* Register with MAL */
	dev->commac.ops = &emac_commac_ops;
	dev->commac.dev = dev;
	dev->commac.tx_chan_mask = MAL_CHAN_MASK(dev->mal_tx_chan);
	dev->commac.rx_chan_mask = MAL_CHAN_MASK(dev->mal_rx_chan);
	err = mal_register_commac(dev->mal, &dev->commac);
	...
}

Before this patch the gige branch ran after that block, so the scatter/gather
table won. Now it runs before it, so every gige-capable EMAC appears to be
registered with mal_register_commac() using emac_commac_ops while
ndev->netdev_ops is emac_gige_netdev_ops.

The two tables differ only in peek_rx:

static struct mal_commac_ops emac_commac_ops = {
	.poll_tx = &emac_poll_tx,
	.poll_rx = &emac_poll_rx,
	.peek_rx = &emac_peek_rx,
	.rxde = &emac_rxde,
};

static struct mal_commac_ops emac_commac_sg_ops = {
	.poll_tx = &emac_poll_tx,
	.poll_rx = &emac_poll_rx,
	.peek_rx = &emac_peek_rx_sg,
	.rxde = &emac_rxde,
};

emac_peek_rx() reports work as soon as the current slot is not empty, while
emac_peek_rx_sg() walks the chain and only reports work when it sees
MAL_RX_CTRL_LAST:

drivers/net/ethernet/ibm/emac/core.c:emac_peek_rx_sg() {
	...
		if (ctrl & MAL_RX_CTRL_EMPTY)
			return 0;
		else if (ctrl & MAL_RX_CTRL_LAST)
			return 1;
	...
}

Can that cause mal_poll() to treat a partially received multi-descriptor or
jumbo frame as a complete "rotting packet"?

drivers/net/ethernet/ibm/emac/mal.c:mal_poll() {
	...
		if (unlikely(mc->ops->peek_rx(mc->dev) ||
			     test_bit(MAL_COMMAC_RX_STOPPED, &mc->flags))) {
			MAL_DBG2(mal, "rotting packet" NL);
			if (!napi_schedule(napi))
				goto more_work;

			spin_lock_irqsave(&mal->lock, flags);
			mal_disable_eob_irq(mal);
	...
}

Would it be enough to hoist only ndev->netdev_ops and ndev->ethtool_ops,
which is what the netdev_need_ops_lock() check in phy_link_topo_add_phy()
needs before of_phy_connect(), and leave the commac.ops selection where it
was, or alternatively set dev->commac.ops = &emac_commac_ops only in the
non-gige case?

The commit message describes this as moving "the code that sets
netdev_ops+ethtool_ops", so the change in commac.ops selection for gige
instances does not seem to be intentional.

>  	dev->emacp = devm_platform_ioremap_resource(ofdev, 0);
>  	if (IS_ERR(dev->emacp)) {
>  		err = PTR_ERR(dev->emacp);
> @@ -3144,12 +3152,6 @@ static int emac_probe(struct platform_device *ofdev)
>  		ndev->features |= ndev->hw_features | NETIF_F_RXCSUM;
>  	}
>  	ndev->watchdog_timeo = 5 * HZ;
> -	if (emac_phy_supports_gige(dev->phy_mode)) {
> -		ndev->netdev_ops = &emac_gige_netdev_ops;
> -		dev->commac.ops = &emac_commac_sg_ops;
> -	} else
> -		ndev->netdev_ops = &emac_netdev_ops;
> -	ndev->ethtool_ops = &emac_ethtool_ops;
>  
>  	/* MTU range: 46 - 1500 or whatever is in OF */
>  	ndev->min_mtu = EMAC_MIN_MTU;

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/f11843d8416cb3d94b79a7f224bd4af0bf859c1f.1789644724.git.chunkeey%40gmail.com

Re: [PATCH net v1] net: emac: move setting of netops to fix crash

[Christian Lamparter]

On 9/21/26 2:58 PM, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 1 · Medium: 0 · Low: 0
> 
> - [High] In `emac_probe()` (drivers/net/ethernet/ibm/emac/core.c) the
>    relocated Gigabit branch now sets `dev->commac.ops =…
> 
> 
> Would it be enough to hoist only ndev->netdev_ops and ndev->ethtool_ops,
> which is what the netdev_need_ops_lock() check in phy_link_topo_add_phy()
> needs before of_phy_connect(), and leave the commac.ops selection where it
> was, or alternatively set dev->commac.ops = &emac_commac_ops only in the
> non-gige case?
> --
Oh yes. I see, thanks. I'll send a v2.

Cheers,
Christian

--
pw-bot: cr