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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <d1ce9f0d-9158-47ee-a60f-640822e35610@lunn.ch>
Date: Tue, 25 Mar 2025 21:06:47 +0100
From: Andrew Lunn <andrew@...n.ch>
To: Rengarajan.S@...rochip.com
Cc: andrew+netdev@...n.ch, rmk+kernel@...linux.org.uk, davem@...emloft.net,
	Thangaraj.S@...rochip.com, Woojung.Huh@...rochip.com,
	pabeni@...hat.com, o.rempel@...gutronix.de, edumazet@...gle.com,
	kuba@...nel.org, phil@...pberrypi.org, kernel@...gutronix.de,
	horms@...nel.org, linux-kernel@...r.kernel.org,
	netdev@...r.kernel.org, UNGLinuxDriver@...rochip.com,
	maxime.chevallier@...tlin.com
Subject: Re: [PATCH net-next v5 1/6] net: usb: lan78xx: Improve error
 handling in PHY initialization

> >  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