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

[netdev-bot+sashiko@kernel.org]

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