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
