Date:	Thu, 19 Apr 2012 16:30:01 +0100
From:	Ben Hutchings <bhutchings@...arflare.com>
To:	Giuseppe CAVALLARO <peppe.cavallaro@...com>
CC:	<netdev@...r.kernel.org>, <rayagond@...avyalabs.com>,
	<davem@...emloft.net>
Subject: Re: [net-next 1/4 (V3)] net: ethtool: add the EEE support

On Thu, 2012-04-19 at 14:58 +0200, Giuseppe CAVALLARO wrote:
> Hello Ben,
> 
> On 4/16/2012 7:41 AM, Giuseppe CAVALLARO wrote:
> [snip]
> >> What I meant is that userland should be able to find out (a), and,
> >> *separately*, either (b) or (c).  That is, there must be at least 2
> >> separate flags for this.  In fact, I explicitly requested you define
> >> supported/advertising/lp_advertising bitmasks for EEE link modes just
> >> like we have for autonegotiation.  But you've made no substantive
> >> changes in response to my review, aside from dropping the added field in
> >> ethtool_cmd.
> > 
> > Sorry Ben but I believed that (c) was enough.
> > 
> >> What you're submitting just isn't good enough for a generic interface,
> >> as the ethtool API is supposed to be.  It's not even a good interface to
> >> your driver.
> > 
> > yes! I'll rework this and provide new patches asap.
> 
> sorry if I disturb you but I want to be sure to avoid to forget
> something else in the next EEE patches (avoiding to continuously disturb
> you).
> 
> I'm changing the code for getting/setting the EEE capability and trying
> to follow your suggestions.
> 
> The "get" will show the following things; this is a bit different of the
> points "a" "b" and "c" we had discussed. Maybe, this could also be a
> more complete (*) .
> The ethtool (see output below as example) could report the phy
> (supported/advertised/lp_advertised) and mac eee capabilities separately.

Sounds reasonable.

> The "set" will be useful for some eth devices (like the stmmac) that can
> stop/enable internally the eee capability (at mac level).

I don't know much about EEE, but shouldn't the driver take care of
configuring the MAC for this whenever the PHY is set to advertise EEE
capability?

> What do you think?
> 
> Regards
> Peppe
> 
> ----
> 
> # ./ethtool  eth0
> Settings for eth0:
> 
> [snip]
> 
> 	Current message level: 0x0000003f (63)
> 			       drv probe link timer ifdown ifup
> 	Link detected: yes
> 	Energy-Efficient Ethernet:           -------------------------
> 		MAC supports: yes            |-> related to MAC side  |
> 		phy supports modes: ...      |-> from MMD 3.20        |
> 		phy advertising modes: ...   |-> from MMD 7.60        |
> 		LP advertising modes: ...    |-> from MMD 7.61        |
> 					     --------------------------
>                                                     (*)
> PS. The "..." above means that we can actually dump: 100BASE-TX EEE etc
> for each advertising modes and also for phy support (reg 3.20).

Yes, that's the sort of information I would expect to see (but try to be
consistent with the wording used for AN).

The EEE advertising mask presumably should be changeable similarly to
the AN advertising mask ('ethtool -s <devname> eeeadv <mask>').  But I
don't know how the two advertising masks interact.  Is one supposed to
be a subset of the other?  Currently ethtool automatically changes the
AN advertising mask in response to a speed/duplex change; should it also
try to change the EEE advertising mask?

Ben.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

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

Powered by blists - more mailing lists