netdev - Re: [PATCH net-next 9/9] phy: fixed_phy: Set phy capabilities even when link is down

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]

Message-ID: <20150823210214.GB20710@lunn.ch>
Date:	Sun, 23 Aug 2015 23:02:14 +0200
From:	Andrew Lunn <andrew@...n.ch>
To:	Florian Fainelli <f.fainelli@...il.com>
Cc:	David Miller <davem@...emloft.net>, netdev <netdev@...r.kernel.org>
Subject: Re: [PATCH net-next 9/9] phy: fixed_phy: Set phy capabilities even
 when link is down

On Sun, Aug 23, 2015 at 11:40:07AM -0700, Florian Fainelli wrote:
> Le 08/23/15 02:47, Andrew Lunn a écrit :
> > What features a phy supports is masked in genphy_config_init() by
> > looking at the PHYs BMSR register.
> > 
> > If the link is down, fixed_phy_update_regs() will only set the auto-
> > negotiation capable bit in BMSR. Thus genphy_config_init() comes to
> > the conclusion the PHY can only perform 10/Half, and masks out the
> > higher speed features. If however the link it up, BMSR is set to
> > indicate the speed the PHY is capable of auto-negotiating, and
> > genphy_config_init() does not mask out the high speed features.
> > 
> > To fix this, when the link is down, have fixed_phy_update_regs() leave
> > the link status and auto-negotiation complete bit unset, but set all
> > the other bits depending on the fixed phy speed.
> 
> This kinds of revert what Staas did in commit
> 868a4215be9a6d80548ccb74763b883dc99d32a2 ("net: phy: fixed_phy: handle
> link-down case"). When the link is down, it does not seem to me like we
> can rely on the previous speed and duplex parameters to be considered valid.

I must admit to not spending the time needed to understand what all
these bits mean. Are they what we are advertising we are capable off,
or what we have negotiated.

We definitely should not be configuring the phy layer on what we have
negotiated in the past. But what we are advertising should be a good
indication of what we are capable of. Maybe this function needs to set
the advertised features separate from negotiated features? If the link
is down, leave the negotiated at 10/Half, but set the advertised
features based on the fixed-link settings?

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