Re: [PATCH net-next v12 12/12] net: airoha: add phylink support

[Christian Marangi <ansuelsmth@gmail.com>]

On Mon, Aug 10, 2026 at 10:53:13PM +0800, win847@gmail.com wrote:
> On Sun, 9 Aug 2026, Christian Marangi wrote:
> > Subject: [PATCH net-next v12 12/12] net: airoha: add phylink support
> 
> Hi Christian,
> 
> Thanks for the series. I have a question about the error path in
> `airoha_dev_open()` introduced in this patch, and a suggested cleanup.
> 
> In the new code:
> 
>     err = phylink_of_phy_connect(dev->phylink, netdev->dev.of_node, 0);
>     if (err) {
>         netdev_err(netdev, "could not attach PHY: %d\n", err);
>         return err;
>     }
> 
>     phylink_start(dev->phylink);
> 
>     netif_tx_start_all_queues(netdev);
>     err = airoha_set_vip_for_gdm_port(dev, true);
>     if (err)
>         return err;
> 
> If `airoha_set_vip_for_gdm_port(dev, true)` fails after both
> `phylink_of_phy_connect()` and `phylink_start()` have already run, we
> return `err` directly without doing `phylink_stop()` or
> `phylink_disconnect_phy()`. That leaves the phylink in a started /
> PHY-connected state. On the next `ndo_open`, `phylink_of_phy_connect()`
> will be called again and may fail because the PHY is already connected,
> preventing the interface from ever coming back up again.
> 
> I understand `airoha_set_vip_for_gdm_port()` currently always returns 0
> (based on v12), so today this is a dead path, but it is easy to make the
> error handling correct and defensive so a future change to that helper
> does not silently break reopen.
> 
> Suggested change: add an `err_phy_stop` error label and unwind phylink
> before returning:
> 
>     netif_tx_start_all_queues(netdev);
>     err = airoha_set_vip_for_gdm_port(dev, true);
>     if (err)
>         goto err_phy_stop;
> 
>     return 0;
> 
> err_phy_stop:
>     netif_tx_stop_all_queues(netdev);
>     phylink_stop(dev->phylink);
>     phylink_disconnect_phy(dev->phylink);
>     return err;
> 
> Would you be open to folding this in (or reordering so `phylink_start()`
> runs after the VIP setup, leaving fewer error exits that need phylink
> teardown)? Happy to write it up as a proper patch if useful.
> 

If you are ok, I can integrate the change in the next revision.

-- 
	Ansuel