Re: [PATCH net-next v5 1/6] net: usb: lan78xx: Improve error handling in PHY initialization

[Andrew Lunn]

> >  static struct phy_device *lan7801_phy_init(struct lan78xx_net *dev)
> >  {
> > -       u32 buf;
> > -       int ret;
> >         struct fixed_phy_status fphy_status = {
> >                 .link = 1,
> >                 .speed = SPEED_1000,
> >                 .duplex = DUPLEX_FULL,
> >         };
> >         struct phy_device *phydev;
> > +       int ret;
> > 
> >         phydev = phy_find_first(dev->mdiobus);
> >         if (!phydev) {
> > @@ -2525,30 +2524,40 @@ static struct phy_device
> > *lan7801_phy_init(struct lan78xx_net *dev)
> >                 phydev = fixed_phy_register(PHY_POLL, &fphy_status,
> > NULL);
> >                 if (IS_ERR(phydev)) {
> >                         netdev_err(dev->net, "No PHY/fixed_PHY
> > found\n");
> > -                       return NULL;
> > +                       return ERR_PTR(-ENODEV);
> >                 }
> >                 netdev_dbg(dev->net, "Registered FIXED PHY\n");
> >                 dev->interface = PHY_INTERFACE_MODE_RGMII;
> >                 ret = lan78xx_write_reg(dev, MAC_RGMII_ID,
> >                                         MAC_RGMII_ID_TXC_DELAY_EN_);
> > +               if (ret < 0)
> > +                       return ERR_PTR(ret);
> > +
> 
> I noticed that fixed_phy_register is removed in later patches. However,
> in the above implementation, if a failure occurs we exit without
> unregistering it. To avoid this issue, can we include the patch that
> removes fixed_phy_register first to avoid the cleanup scenario?

phylink itself implements fixed phy. So it is being removed later
because it is not needed once the conversation to phylink is
performed. If you remove it here, before the conversion to phylink,
you break the driver when it is using fixed phy.

With this sort of refactoring, you should not break the normal
case. But there is some wiggle room for error cases, which should not
happen, so long as by the end of the patch series, it is all clean.

So i personally don't care about this leak of a fixed link, at this
stage.

	Andrew