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:   Wed, 9 May 2018 08:50:58 -0500
From:   Dan Murphy <dmurphy@...com>
To:     Andrew Lunn <andrew@...n.ch>
CC:     <f.fainelli@...il.com>, <netdev@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] net: phy: DP83TC811: Introduce support for the DP83TC811
 phy

Andrew

Thanks for the review

On 05/09/2018 08:43 AM, Andrew Lunn wrote:
>> +static int dp83811_config_aneg(struct phy_device *phydev)
>> +{
>> +	int err;
>> +	int value;
>> +
>> +	value = phy_read(phydev, MII_DP83811_SGMII_CTRL);
>> +	if (phydev->autoneg == AUTONEG_ENABLE) {
>> +		err = phy_write(phydev, MII_DP83811_SGMII_CTRL,
>> +				(DP83811_SGMII_AUTO_NEG_EN | value));
>> +		if (err < 0)
>> +			return err;
>> +	} else {
>> +		err = phy_write(phydev, MII_DP83811_SGMII_CTRL,
>> +				(~DP83811_SGMII_AUTO_NEG_EN & value));
>> +		if (err < 0)
>> +			return err;
>> +	}
>> +
> 
> Hi Dan
> 
> You say SGMII is unreliable on one of these devices. Should you check
> phydev->interface before enabling SGMII autoneg?


If SGMII enable bit(12) is not set in the device then setting auto neg has no affect on the device.
Only the SGMII has auto negotiation capability so if we set this with other MII interfaces then it is ignored by the device.

If we want to protect this with the SGMII check I can add it but it may be over kill.

Let me know what is preferred.

> 
>> +	return genphy_config_aneg(phydev);
>> +}
>> +
>> +static int dp83811_config_init(struct phy_device *phydev)
>> +{
>> +	int err;
>> +	int value;
>> +
>> +	err = genphy_config_init(phydev);
>> +	if (err < 0)
>> +		return err;
>> +
>> +	if (phydev->interface == PHY_INTERFACE_MODE_SGMII) {
>> +		value = phy_read(phydev, MII_DP83811_SGMII_CTRL);
>> +		if (!(value & DP83811_SGMII_EN)) {
>> +			err = phy_write(phydev, MII_DP83811_SGMII_CTRL,
>> +					(DP83811_SGMII_EN | value));
>> +			if (err < 0)
>> +				return err;
>> +		} else {
>> +			err = phy_write(phydev, MII_DP83811_SGMII_CTRL,
>> +				(~DP83811_SGMII_EN & value));
>> +			if (err < 0)
>> +				return err;
>> +		}
> 
> This looks to be a duplicate of dp83811_config_aneg()?

It is almost the same but this function sets bit 12 and aneg function sets bit 13.
We can have SGMII with or without auto neg.

> 
>> +	}
>> +
>> +	value = DP83811_WOL_MAGIC_EN | DP83811_WOL_SECURE_ON | DP83811_WOL_EN;
>> +
>> +	return phy_write_mmd(phydev, DP83811_DEVADDR, MII_DP83811_WOL_CFG,
>> +	      value);
>> +}
>> +
>> +static int dp83811_phy_reset(struct phy_device *phydev)
>> +{
>> +	int err;
>> +
>> +	err = phy_write(phydev, MII_DP83811_RESET_CTRL, DP83811_HW_RESET);
>> +	if (err < 0)
>> +		return err;
>> +
>> +	dp83811_config_init(phydev);
> 
> I don't think you need to initialize it here. phylib should call that
> soon after the reset.

OK I will remove it.  

> 
>> +
>> +	return 0;
>> +}
>> +
> 
>> +static struct phy_driver dp83811_driver[] = {
>> +	{
>> +		.phy_id = DP83TC811_PHY_ID,
>> +		.phy_id_mask = 0xfffffff0,
>> +		.name = "TI DP83TC811",
>> +		.features = PHY_BASIC_FEATURES,
>> +		.flags = PHY_HAS_INTERRUPT,
>> +		.config_init = genphy_config_init,
> 
> ????
> 

I missed that I must have had the config init being called in my testing through the reset function.
I will update.

> 	Andrew
> 


-- 
------------------
Dan Murphy

Powered by blists - more mailing lists