netdev - Re: [PATCH net-next v2] net: phy: xilinx: add Xilinx PHY driver

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: <7684776f-2bec-e9e2-1a79-dbc3e9844f7e@sedsystems.ca>
Date:   Tue, 4 Jun 2019 10:39:03 -0600
From:   Robert Hancock <hancock@...systems.ca>
To:     Heiner Kallweit <hkallweit1@...il.com>, netdev@...r.kernel.org,
        Florian Fainelli <f.fainelli@...il.com>,
        Andrew Lunn <andrew@...n.ch>
Subject: Re: [PATCH net-next v2] net: phy: xilinx: add Xilinx PHY driver

On 2019-06-03 11:37 p.m., Heiner Kallweit wrote:
>> +	/* Xilinx PHY wrongly indicates BMSR_ESTATEN = 0 even though
>> +	 * extended status registers are supported. So we force the PHY
>> +	 * features to PHY_GBIT_FEATURES in order to allow gigabit support
>> +	 * to be detected.
>> +	 */
>> +	.features	= PHY_GBIT_FEATURES,
> 
> BMSR_ESTATEN is used by genphy_config_advert() too. Means you would
> need to implement your own config_aneg callback and basically
> copy 99% of genphy_config_advert().
> 
> I think the better alternative is to implement a quirk flag in phylib
> similar to PHY_RST_AFTER_CLK_EN. Let me come up with a proposal.
> 
> Last but not least: Not setting BMSR_ESTATEN for a GBit PHY violates
> the standard. Any intention from Xilinx to fix this?

My apologies, it seems like my initial diagnosis of this was incorrect.
ESTATEN is indeed set to 1. The problem is that in the configuration of
the PHY that we are using, which is set up for 1000Base-X mode, ESTATUS
returns 0x8000 indicating support for 1000Base-X full duplex and not
ESTATUS_1000_TFULL or ESTATUS_1000_THALF, which are the only bits that
genphy_config_init checks for. Therefore, genphy_config_init comes back
with no modes supported for the PHY, and phydev therefore rejects
attaching the PHY to the network device.

Adding PHY_GBIT_FEATURES in causes 1000Base-T half and full duplex to be
added into the mix, which fakes things out enough for it to work, it
appears.

So it seems like what is missing is the ability of genphy_config_init to
detect the bits in the extended status register for 1000Base-X and add
the corresponding mode flags. It appears bit 15 for 1000Base-X full
duplex is standardized in 802.3 Clause 22, so I would expect Linux
should be able to detect that and add it as a supported mode for the
PHY. genphy_config_init is dealing with the "legacy" 32-bit mode masks
that have no bit for 1000BaseX though.. how is that intended to work?

-- 
Robert Hancock
Senior Software Developer
SED Systems, a division of Calian Ltd.
Email: hancock@...systems.ca