netdev - Re: [PATCH RFC 08/18] r8168: add basic phylib support

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20171222094411.GD2431@lunn.ch>
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