phylib's new dynamic feature detection seems too early

phylib's new dynamic feature detection seems too early

[Russell King - ARM Linux admin]

Hi,

Back in dcdecdcfe1fc ("net: phy: switch drivers to use dynamic feature
detection"), Heiner switched a bunch of PHYs over to using his
wonderful new idea of reading the PHY capabilities from the registers.
However, this is flawed.

The features are read from the PHY shortly after the PHY driver is
bound to the device, while the PHY is in its default pin-strapped
defined mode. PHYs such as the 88E1111 set their capabilities according
to the pin-strapped host interface mode.

If the 88E1111 is pin-strapped for a 1000base-X host interface, then it
indicates that it is not capable of 100M or 10M modes - which is
entirely sensible.

However, the SFP support will switch the PHY into SGMII mode, where the
PHY will support 100M and 10M modes. Indeed, reading the PHY registers
using mii-diag after initialisation reports that the PHY supports these
speeds.

This switch happens in the Marvell PHY driver when the config_init()
method is called, via phy_init_hw() and phy_attach_direct() - which
is where the MAC driver configures the PHY for its requested interface
mode.

Therefore, the features dynamically read from the PHY are entirely
meaningless, until the PHY interface mode has been properly set.

This means that SFP modules, such as Champion One 1000SFPT and a
multitude of others which default to a 1000base-X interface end up
only advertising 1000baseT despite being switched to SGMII mode and
actually supporting 100M and 10M speeds - and that can't be changed
via ethtool as the support mask doesn't allow the other speeds.

Thoughts how to get around this?

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

Re: phylib's new dynamic feature detection seems too early

[Heiner Kallweit]

On 10.12.2019 18:15, Russell King - ARM Linux admin wrote:
> Hi,
> 
> Back in dcdecdcfe1fc ("net: phy: switch drivers to use dynamic feature
> detection"), Heiner switched a bunch of PHYs over to using his
> wonderful new idea of reading the PHY capabilities from the registers.
> However, this is flawed.
> 
> The features are read from the PHY shortly after the PHY driver is
> bound to the device, while the PHY is in its default pin-strapped
> defined mode. PHYs such as the 88E1111 set their capabilities according
> to the pin-strapped host interface mode.
> 
> If the 88E1111 is pin-strapped for a 1000base-X host interface, then it
> indicates that it is not capable of 100M or 10M modes - which is
> entirely sensible.
> 
> However, the SFP support will switch the PHY into SGMII mode, where the
> PHY will support 100M and 10M modes. Indeed, reading the PHY registers
> using mii-diag after initialisation reports that the PHY supports these
> speeds.
> 
> This switch happens in the Marvell PHY driver when the config_init()
> method is called, via phy_init_hw() and phy_attach_direct() - which
> is where the MAC driver configures the PHY for its requested interface
> mode.
> 
> Therefore, the features dynamically read from the PHY are entirely
> meaningless, until the PHY interface mode has been properly set.
> 
> This means that SFP modules, such as Champion One 1000SFPT and a
> multitude of others which default to a 1000base-X interface end up
> only advertising 1000baseT despite being switched to SGMII mode and
> actually supporting 100M and 10M speeds - and that can't be changed
> via ethtool as the support mask doesn't allow the other speeds.
> 
> Thoughts how to get around this?
> 
Before reading PHY capabilities from the registers the capabilities
were completely static, I don't think this was better.
The capabilities read in phy_probe() are correct for the default
interface mode. And for all PHY drivers not implementing
config_init() we have to read the capabilities in phy_probe().

In case the capabilities can change in config_init() a first thought
would be to clear supported/advertised and re-read the capabilities
at the end of config_init().

Heiner