Re: [bug report] net: usb: lan78xx: refactor PHY init to separate detection and MAC configuration

[Dan Carpenter <dan.carpenter@linaro.org>]

On Fri, May 09, 2025 at 02:43:12PM +0200, Oleksij Rempel wrote:
> 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?

Don't do anything on my account.  These messages are only intended to be
helpful, so ignore them when they're not.  People with questions know to
look it up on lore and they will find this thread.

regards,
dan carpenter