netdev - Re: [net-next.git 4/4 (v8)] phy: add the EEE support and the way to access to the MMD registers.

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

Date:	Thu, 28 Jun 2012 08:54:44 +0200
From:	Giuseppe CAVALLARO <peppe.cavallaro@...com>
To:	Ben Hutchings <bhutchings@...arflare.com>
Cc:	netdev@...r.kernel.org, eric.dumazet@...il.com,
	rayagond@...avyalabs.com, davem@...emloft.net,
	yuvalmin@...adcom.com
Subject: Re: [net-next.git 4/4 (v8)] phy: add the EEE support and the way
 to access to the MMD registers.

On 6/27/2012 6:15 PM, Ben Hutchings wrote:
> On Thu, 2012-06-21 at 08:03 +0200, Giuseppe CAVALLARO wrote:
> [...]
>> v8: fixed a problem in the phy_init_eee return value erroneously added
>>     when included the phy_read_status call.
> 
> Almost there. :-/

no problem :-)

> [...]
>> +int phy_init_eee(struct phy_device *phydev, bool clk_stop_enable)
>> +{
>> +	int ret = -EPROTONOSUPPORT;
>> +
>> +	/* According to 802.3az,the EEE is supported only in full duplex-mode.
>> +	 * Also EEE feature is active when core is operating with MII, GMII
>> +	 * or RGMII.
>> +	 */
>> +	if ((phydev->duplex == DUPLEX_FULL) &&
>> +	    ((phydev->interface == PHY_INTERFACE_MODE_MII) ||
>> +	    (phydev->interface == PHY_INTERFACE_MODE_GMII) ||
>> +	    (phydev->interface == PHY_INTERFACE_MODE_RGMII))) {
>> +		u16 eee_lp, eee_cap, eee_adv;
>> +		u32 lp, cap, adv;
>> +		int idx, status;

[snip]

>> +
>> +		eee_adv = phy_read_mmd_indirect(phydev->bus, MDIO_AN_EEE_ADV,
>> +						MDIO_MMD_AN, phydev->addr);
>> +		if (eee_adv < 0)
>> +			return eee_adv;
> 
> You check for eee_{cap,lp,adv} < 0 but that's impossible since the
> variables are declared unsigned (u16).  (I wonder what compiler you are
> using, as I would expect this to result in a warning.)  I think they
> need to be declared int.

IIRC I have compiled w/ no warnings (on arm  and sh4 -- gcc 4.6.3) but
you are right and I'll fix that, no problem at all.

[snip]

>> +			u32 val = phy_read_mmd_indirect(phydev->bus, MDIO_CTRL1,
>> +							MDIO_MMD_PCS,
>> +							phydev->addr);
>> +			if (val < 0)
>> +				return val;
> 
> Same problem here.

yes

> 
> [...]
>> --- a/include/linux/mdio.h
>> +++ b/include/linux/mdio.h
> [...]
>> @@ -237,9 +241,18 @@
>>  #define MDIO_AN_10GBT_STAT_MS		0x4000	/* Master/slave config */
>>  #define MDIO_AN_10GBT_STAT_MSFLT	0x8000	/* Master/slave config fault */
>>  
>> -/* AN EEE Advertisement register. */
>> -#define MDIO_AN_EEE_ADV_100TX		0x0002	/* Advertise 100TX EEE cap */
>> -#define MDIO_AN_EEE_ADV_1000T		0x0004	/* Advertise 1000T EEE cap */
> [...]
> 
> This header is exported to userland so I don't think these definitions
> can be removed.  But you could comment that they're redundant with the
> following MDIO_EEE_* definitions.

Indeed I agree with you and I've already fixed this w/o removing them.
I'm sending the new patches now.

Thanks again for your feedback and help.

Peppe

> 
> Ben.
> 


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html