Re: [PATCH] Add ethtool to mii advertisment conversion helpers

[Ben Hutchings <bhutchings@solarflare.com>]

On Wed, 2011-11-16 at 18:06 -0800, Matt Carlson wrote:
> On Wed, Nov 16, 2011 at 05:29:42PM -0800, Ben Hutchings wrote:
> > On Wed, 2011-11-16 at 17:16 -0800, Matt Carlson wrote:
> > > On Wed, Nov 16, 2011 at 04:34:37PM -0800, Ben Hutchings wrote:
> > [...]
> > > > > +#define mii_lpa_to_ethtool_100bt(lpa)	mii_adv_to_ethtool_100bt(lpa)
> > > > 
> > > > Shouldn't this additionally translate LPA_LPACK into ADVERTISED_Autoneg?
> > > 
> > > You mean, like this?
> > > 
> > > static inline u32 mii_lpa_to_ethtool_100bt(u32 lpa)
> > > {
> > > 	u32 result = 0;
> > > 
> > > 	if (lpa & LPA_LPACK)
> > > 		result |= ADVERTISED_Autoneg;
> > > 
> > > 	return result | mii_adv_to_ethtool_100bt(lpa);
> > > }
> > > 
> > > Yes, that looks like a better implementation.
> > 
> > Think so.
> > 
> > And I think the mii_adv_to_ethtool_* functions should add
> > ADVERTISED_Autoneg unconditionally.  But I'm not entirely sure that's
> > right.
> 
> The primary purpose of these functions is to translate the information
> between two representations.  It seems wise to be careful not to add
> from it or take anything away from it.  It is certainly possible to
> have a valid AN advertisement register configuration, but not have
> autoneg enabled.  Keeping the ADVERTISED_Autoneg out could prevent
> misuse.  Do you agree?

I'm not sure what you mean about misuse.  If autoneg is not enabled then
the register contents are not used and I don't think the
mii_adv_to_ethtool functions should be called at all.  But I'm not that
bothered either way.

> > > > Shouldn't there be mii_lpa_to_ethtool_1000X (or
> > > > mii_lpa_to_ethtool_lpa_x)?
> > > 
> > > Yes.  You're right.  Should it just be a preprocessor definition that
> > > points to mii_adv_to_ethtool_1000X()?
> > 
> > I think that would need to handle LPA_LPACK as well.
> 
> I was wondering if that was present in 1000Base-X mode.  I didn't see it
> in my passing glance at a spec.

IEEE 802.3-2005 §37.2.1.6 seems to define it similarly to the TP case.

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.