Re: [PATCH net-next v6 02/15] net: ethernet: cortina: Keep PHY-less port bound for shared IRQ

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: ethernet: cortina: Keep PHY-less port bound for shared IRQ

This patch keeps a Gemini port child bound as an IRQ-only port when its
DT node has neither phy-handle nor fixed-link. That way the threaded
software free queue handler on port 1 stays registered. Ports are now
published to the parent under RTNL only after their IRQ and optional
netdev are ready, and geth_resize_freeq() refuses to run until both ports
are published.

> The software free queue interrupt is routed through port 1 even on
> boards which only connect a PHY to port 0. Since PHY setup was moved
> into probe, the PHY-less port fails to bind and releases the threaded
> handler needed by the shared queue.

[Severity: Low]
Does this description understate what happens on the parent tree?

Before this patch, gemini_ethernet_port_probe() published the port before
PHY setup:

	if (!id)
		geth->port0 = port;
	else
		geth->port1 = port;

	gemini_ethernet_init(geth);
	...
	ret = gmac_setup_phy(netdev);

port is embedded in netdev_priv() of a devm_alloc_etherdev_mqs() netdev.
On dns-313, nas4210b, nas4220b, rut1xx, wbd111 and dir-685,
ethernet-port@1 has no phy-mode or phy-handle. gmac_setup_phy() therefore
fails, and devres frees the netdev while geth->port1 still points into it.

The next gmac_open() on port 0 then reaches geth_resize_freeq(), which
used to do:

	other_netdev = geth->port1->netdev;

	if (other_netdev && netif_running(other_netdev))
		return -EBUSY;

Isn't that a use-after-free of the port 1 private data?

The commit message also doesn't mention the new phy_disconnect() on the
register_netdevice() failure path. Before this patch, a register_netdev()
failure jumped to unprepare with the PHY still attached to a netdev that
devres was about to free.

Could the commit message describe the dangling geth->port1 pointer and
the phy_disconnect() fix?

> diff --git a/drivers/net/ethernet/cortina/gemini.c b/drivers/net/ethernet/cortina/gemini.c
> index bf7652b04f1ba..79c0463642928 100644
> --- a/drivers/net/ethernet/cortina/gemini.c
> +++ b/drivers/net/ethernet/cortina/gemini.c

[ ... ]

> @@ -2600,6 +2632,16 @@ static int gemini_ethernet_port_probe(struct platform_device *pdev)
>  	if (ret)
>  		goto unprepare;
>  
> +	if (!of_property_present(np, "phy-handle") &&
> +	    !of_phy_is_fixed_link(np)) {
> +		dev_info(dev, "no PHY, keeping port for shared IRQ\n");
> +		port->netdev = NULL;
> +		rtnl_lock();
> +		gemini_port_publish(port);
> +		rtnl_unlock();
> +		return 0;
> +	}
> +
>  	ret = gmac_setup_phy(netdev);
>  	if (ret) {
>  		netdev_err(netdev,

[ ... ]

> @@ -2607,9 +2649,15 @@ static int gemini_ethernet_port_probe(struct platform_device *pdev)
>  		goto unprepare;
>  	}
>  
> -	ret = register_netdev(netdev);
> -	if (ret)
> +	rtnl_lock();
> +	ret = register_netdevice(netdev);
> +	if (!ret)
> +		gemini_port_publish(port);
> +	rtnl_unlock();
> +	if (ret) {
> +		phy_disconnect(netdev->phydev);
>  		goto unprepare;
> +	}

[Severity: Medium]
Does this fully cover a port 1 that describes a PHY but fails to set it
up?

Only a node with neither phy-handle nor fixed-link takes the IRQ-only
publish path. Port 1 can instead have phy-handle (wbd222, ssi1328) or
fixed-link (sl93512r, sq201). If gmac_setup_phy() then fails, probe still
does goto unprepare without publishing the port. The register_netdevice()
failure path above does the same.

gmac_setup_phy() returns -ENODEV, not -EPROBE_DEFER, whenever
of_phy_get_and_connect() returns NULL:

	phy = of_phy_get_and_connect(netdev,
				     dev->of_node,
				     gmac_adjust_link);
	if (!phy)
		return -ENODEV;

That covers a PHY that doesn't answer at its MDIO address, a PHY device
that was never registered, and an unsupported phy-mode.

In that case devres releases port 1's threaded software free queue
handler, and geth->port1 stays NULL. The new check in geth_resize_freeq()
then fails permanently:

	if (!geth->initialized || !geth->port0 || !geth->port1)
		return -ENODEV;

gmac_open() only tolerates -EBUSY:

	err = geth_resize_freeq(port);
	...
	if (err && (err != -EBUSY)) {
		netdev_err(netdev, "could not resize freeq\n");
		goto err_stop_phy;
	}

Wouldn't every open of the working port 0 then fail with "could not
resize freeq"?

This looks like the same port 0 breakage that 3e813d61401a introduced.
Before that commit, a PHY failure was deferred to ifup and port 1 stayed
bound.

The gmac_setup_phy() failure path also appears to be unchanged in the
final revision of the series.