Re: [PATCH net-next v7 02/18] net: ethernet: cortina: Keep ports bound for shared IRQ

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 · Medium: 1 · 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).

- [Medium] In gemini_ethernet_port_probe()
  (drivers/net/ethernet/cortina/gemini.c), the new `if (ret ==…

--

net: ethernet: cortina: Keep ports bound for shared IRQ

The software free queue interrupt is routed through port 1, so this patch
keeps Gemini child ports bound as IRQ-only ports when no PHY is described
or when PHY setup or netdev registration fails. It also publishes a port
to the parent only after it is initialized, under RTNL. Global interrupt
masking moves into the parent probe.

> diff --git a/drivers/net/ethernet/cortina/gemini.c b/drivers/net/ethernet/cortina/gemini.c
> index 0b05a15529832..22c3b6b8e2db5 100644
> --- a/drivers/net/ethernet/cortina/gemini.c
> +++ b/drivers/net/ethernet/cortina/gemini.c

[ ... ]

> @@ -2600,17 +2632,41 @@ 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\n");
> +		goto irq_only;
> +	}
> +
>  	ret = gmac_setup_phy(netdev);
>  	if (ret) {
> -		netdev_err(netdev,
> -			   "PHY init failed\n");
> -		goto unprepare;
> +		dev_err(dev, "PHY init failed: %d\n", ret);
> +		if (ret == -EPROBE_DEFER)
> +			goto unprepare;
> +		goto irq_only;
>  	}

[Severity: Medium]
Can gmac_setup_phy() ever return -EPROBE_DEFER here?

It calls of_phy_get_and_connect() and turns any NULL into -ENODEV:

drivers/net/ethernet/cortina/gemini.c:gmac_setup_phy() {
	phy = of_phy_get_and_connect(netdev,
				     dev->of_node,
				     gmac_adjust_link);
	if (!phy)
		return -ENODEV;
	...
}

of_phy_get_and_connect() in drivers/net/mdio/of_mdio.c returns NULL for
every failure. That includes of_phy_register_fixed_link() errors. It also
includes of_phy_connect() returning NULL when of_phy_find_device() has not
found the PHY yet. Apart from -ENODEV, gmac_setup_phy() only returns
-EINVAL, for an unsupported interface mode.

Say the MDIO bus probes after the port. Most Gemini boards reference a
virtual,mdio-gpio bus through phy-handle, and CONFIG_MDIO_GPIO=m makes a
late bus more likely. Wouldn't this path then always go to irq_only, set
port->netdev to NULL and return 0?

After that the port is bound without a netdev, and the driver core won't
retry the probe. suppress_bind_attrs = true blocks rebinding from sysfs.
fw_devlink does not order probes on phy-handle, so nothing holds the port
back until the PHY exists.

The -EPROBE_DEFER check after register_netdevice() looks unreachable in
practice too:

	if (ret) {
		phy_disconnect(netdev->phydev);
		dev_err(dev, "netdev registration failed: %d\n", ret);
		if (ret == -EPROBE_DEFER)
			goto unprepare;
		goto irq_only;
	}

The commit message says:

    Continue to propagate deferred-probe and mandatory resource
    errors.

For PHY setup, though, no deferral value seems to reach these checks.
Could gmac_setup_phy() return -EPROBE_DEFER itself? One way would be to
parse phy-handle and return -EPROBE_DEFER when of_phy_find_device()
returns NULL.

Before this patch, the same case returned -ENODEV and the interface was
also lost. So the outcome is no worse, but the new deferral branches don't
seem to do what the commit message describes.

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261110-gemini-ethernet-fixes-3-v7-0-9c32a1a46aec%40kernel.org