netdev - Re: [PATCH net-next 0/2] net: phy: set 1Gbps as default for driver features

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]

Date:   Tue, 18 Dec 2018 19:09:36 +0100
From:   Heiner Kallweit <hkallweit1@...il.com>
To:     Andrew Lunn <andrew@...n.ch>
Cc:     Florian Fainelli <f.fainelli@...il.com>,
        David Miller <davem@...emloft.net>,
        Ray Jui <rjui@...adcom.com>,
        Scott Branden <sbranden@...adcom.com>,
        bcm-kernel-feedback-list@...adcom.com,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: [PATCH net-next 0/2] net: phy: set 1Gbps as default for driver
 features

On 18.12.2018 10:39, Andrew Lunn wrote:
> On Tue, Dec 18, 2018 at 07:34:50AM +0100, Heiner Kallweit wrote:
>> Whether a PHY is 100Mbps or 1Gbps-capable can be autodetected,
>> therefore it's not needed to define this manually in the driver.
>> genphy_config_init() will remove 1Gbps from phydev->supported if
>> not supported. Having said that PHY drivers for 100Mbps not
>> calling genphy_config_init() still have to set the features field.
>> As most PHY's are 1Gbps-capable let's use this as default.
> 
> Hi Heiner
> 
> I'm not sure i like this. Today most PHYs are 1G. But multi-gige PHYs
> are starting to appear. In 5 years time, i expect most new PHYs will
> be 2.5G and 5G capable, maybe 10G. We then end up with the odd
> situation that 10M, 100M and 2.5G, 5G and 10G all need features, but 1G
> not.
> 

I see the point and thought about this too. My conclusion:
Basic idea is that we shouldn’t force driver authors to specify things
we can autodetect in phylib. And AFAICS supported speeds can be
autodetected based on the information in clause 22 and clause 45
(speed ability) registers. What remains as a challenge is to autodetect
C22 vs. C22-capable C45 vs. C45. So far it seems that C45 PHY’s can be
used only via DT configuration, MDIO bus scan (mdiobus_scan can’t deal
with C45) doesn't work with C45.

Coming back to why 1Gbps as default (at least for now):
Technically I could have used also 10Gbps as default, genphy_config_init()
masks everything above 1Gbps anyway. I think also for 2.5 Gbps and 5Gbps
PHY’s would don’t have to specify the supported speeds. What we need is
an extension to genphy_config_init() which can deal with C22 only so far.
Maybe mv3310_config_init() from the marvell10g driver can be at least
partially reused.

Eventually defining 1Gbps as default isn’t something we would need to roll
back later once PHYs above 1Gbps are the standard. We can deal with this in
phylib w/o affecting existing drivers. Therefore I think the proposed change
is future-proof.

> I would prefer to keep it consistent and always have a features.
> 
>   Andrew
> 
Heiner