On Fri, May 09, 2025 at 12:03:26PM +0300, Dan Carpenter wrote:
> Hello Oleksij Rempel,
>
> Commit d39f339d2603 ("net: usb: lan78xx: refactor PHY init to
> separate detection and MAC configuration") from May 5, 2025
> (linux-next), leads to the following Smatch static checker warning:
>
> drivers/net/usb/lan78xx.c:2842 lan78xx_phy_init()
> warn: missing unwind goto?
>
> drivers/net/usb/lan78xx.c
> 2805 static int lan78xx_phy_init(struct lan78xx_net *dev)
> 2806 {
> 2807 __ETHTOOL_DECLARE_LINK_MODE_MASK(fc) = { 0, };
> 2808 int ret;
> 2809 u32 mii_adv;
> 2810 struct phy_device *phydev;
> 2811
> 2812 phydev = lan78xx_get_phy(dev);
> 2813 if (IS_ERR(phydev))
> 2814 return PTR_ERR(phydev);
> 2815
> 2816 ret = lan78xx_mac_prepare_for_phy(dev);
> 2817 if (ret < 0)
> 2818 goto free_phy;
> 2819
> 2820 /* if phyirq is not set, use polling mode in phylib */
> 2821 if (dev->domain_data.phyirq > 0)
> 2822 phydev->irq = dev->domain_data.phyirq;
> 2823 else
> 2824 phydev->irq = PHY_POLL;
> 2825 netdev_dbg(dev->net, "phydev->irq = %d\n", phydev->irq);
> 2826
> 2827 /* set to AUTOMDIX */
> 2828 phydev->mdix = ETH_TP_MDI_AUTO;
> 2829
> 2830 ret = phy_connect_direct(dev->net, phydev,
> 2831 lan78xx_link_status_change,
> 2832 dev->interface);
> 2833 if (ret) {
> 2834 netdev_err(dev->net, "can't attach PHY to %s\n",
> 2835 dev->mdiobus->id);
> 2836 if (dev->chipid == ID_REV_CHIP_ID_7801_) {
> 2837 if (phy_is_pseudo_fixed_link(phydev)) {
> 2838 fixed_phy_unregister(phydev);
> 2839 phy_device_free(phydev);
> 2840 }
> 2841 }
>
> Why does this error path only cleanup for ID_REV_CHIP_ID_7801_ where the
> others do it unconditionally?
This chip-specific condition in the cleanup can be used, but it’s not
strictly necessary - none of the variants introduce regressions. The
non-conditional cleanup actually matches the logic already used in
lan78xx_disconnect(), where phy_is_pseudo_fixed_link() is checked
unconditionally.
That said, the purpose of this patch set is to prepare for migration to
phylink, where pseudo fixed links won’t be used at all. Due to the
10-patch limit, I’ve split the changes - the patch that removes pseudo
fixed-link support entirely will follow.
Nevertheless, I can send a cleanup patch that unconditionally jumps to
free_phy on phy_connect_direct() failure for consistency and clarity.
Should I?
Best Regards,
Oleksij
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
