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
