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
| ||
|
Date: Fri, 22 Dec 2017 10:44:11 +0100 From: Andrew Lunn <andrew@...n.ch> To: Heiner Kallweit <hkallweit1@...il.com> Cc: Realtek linux nic maintainers <nic_swsd@...ltek.com>, Chun-Hao Lin <hau@...ltek.com>, David Miller <davem@...emloft.net>, "netdev@...r.kernel.org" <netdev@...r.kernel.org> Subject: Re: [PATCH RFC 08/18] r8168: add basic phylib support On Thu, Dec 21, 2017 at 09:50:25PM +0100, Heiner Kallweit wrote: Hi Heiner This code looks good. Just a few minor comments. > +static int r8168_phy_connect(struct rtl8168_private *tp) > +{ > + struct phy_device *phydev; > + phy_interface_t phy_mode; > + int ret; > + > + phy_mode = tp->mii.supports_gmii ? PHY_INTERFACE_MODE_GMII : > + PHY_INTERFACE_MODE_MII; > + > + phydev = phy_find_first(tp->mii_bus); > + if (!phydev) > + return -ENODEV; > + > + ret = phy_connect_direct(tp->dev, phydev, r8168_phylink_handler, > + phy_mode); > + if (ret) > + return ret; > + > + phy_attached_info(phydev); > + > + if (!tp->mii.supports_gmii && (phydev->supported & > + (SUPPORTED_1000baseT_Half | SUPPORTED_1000baseT_Full))) { > + netif_info(tp, probe, tp->dev, "Restrict PHY to 100Mbit because MAC doesn't support 1GBit"); > + phy_set_max_speed(phydev, SPEED_100); > + ret = genphy_restart_aneg(phydev); > + } You might be able to put this higher up, after phy_first_first, and skip the genphy_restart_aneg(). When phy_start() is called i think it will perform an aneg, so there is no need to do it here. > + > + return ret; > +} > + > static void rtl_hw_init_8168g(struct rtl8168_private *tp) > { > void __iomem *ioaddr = tp->mmio_addr; > @@ -8212,10 +8284,18 @@ static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent) > if (!tp->counters) > return -ENOMEM; > > - rc = register_netdev(dev); > + rc = r8168_mdio_register(tp); > if (rc < 0) > return rc; > > + rc = register_netdev(dev); > + if (rc < 0) > + goto err_mdio_unregister; > + > + rc = r8168_phy_connect(tp); > + if (rc < 0) > + goto err_netdev_unregister; > + > pci_set_drvdata(pdev, dev); This is the wrong order. Everything should be setup before you call register_netdev(). As soon as that is called, things can start happening with the device, so it is dangerous not to have the phy connected, nor drvdata set. This is an old bug, but now would be a good time to fix it. Andrew
Powered by blists - more mailing lists