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