netdev - Re: [PATCH net v2] net: phy: aquantia: Introduce custom get

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] [day] [month] [year] [list]

Message-ID: <eb7e327d-996c-429d-a45e-2f3522d234a3@quicinc.com>
Date: Tue, 24 Sep 2024 17:49:51 -0700
From: "Abhishek Chauhan (ABC)" <quic_abchauha@...cinc.com>
To: Andrew Lunn <andrew@...n.ch>
CC: "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet
	<edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni
	<pabeni@...hat.com>, <netdev@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>, Andrew Halaney <ahalaney@...hat.com>,
        "Russell King (Oracle)" <linux@...linux.org.uk>,
        Heiner Kallweit
	<hkallweit1@...il.com>,
        Bartosz Golaszewski <bartosz.golaszewski@...aro.org>,
        "linux-tegra@...r.kernel.org" <linux-tegra@...r.kernel.org>,
        Brad Griffis
	<bgriffis@...dia.com>,
        Vladimir Oltean <vladimir.oltean@....com>,
        Jon Hunter
	<jonathanh@...dia.com>,
        Maxime Chevallier <maxime.chevallier@...tlin.com>,
        <kernel@...cinc.com>
Subject: Re: [PATCH net v2] net: phy: aquantia: Introduce custom get_features



On 9/24/2024 8:19 AM, Abhishek Chauhan (ABC) wrote:
> 
> 
> On 9/24/2024 5:04 AM, Andrew Lunn wrote:
>>> +static void aqr_supported_speed(struct phy_device *phydev, u32 max_speed)
>>> +{
>>> +	__ETHTOOL_DECLARE_LINK_MODE_MASK(supported) = { 0, };
>>> +
>>> +	linkmode_set_bit(ETHTOOL_LINK_MODE_Autoneg_BIT, supported);
>>> +	linkmode_set_bit(ETHTOOL_LINK_MODE_Pause_BIT, supported);
>>> +	linkmode_set_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT, supported);
>>> +	linkmode_set_bit(ETHTOOL_LINK_MODE_FIBRE_BIT, supported);
>>> +	linkmode_set_bit(ETHTOOL_LINK_MODE_TP_BIT, supported);
>>> +	linkmode_set_bit(ETHTOOL_LINK_MODE_10baseT_Half_BIT, supported);
>>> +	linkmode_set_bit(ETHTOOL_LINK_MODE_10baseT_Full_BIT, supported);
>>> +	linkmode_set_bit(ETHTOOL_LINK_MODE_100baseT_Half_BIT, supported);
>>> +	linkmode_set_bit(ETHTOOL_LINK_MODE_100baseT_Full_BIT, supported);
>>> +	linkmode_set_bit(ETHTOOL_LINK_MODE_1000baseT_Half_BIT, supported);
>>> +	linkmode_set_bit(ETHTOOL_LINK_MODE_1000baseT_Full_BIT, supported);
>>> +
>>> +	if (max_speed == SPEED_2500) {
>>> +		linkmode_set_bit(ETHTOOL_LINK_MODE_2500baseX_Full_BIT, supported);
>>> +		linkmode_set_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT, supported);
>>> +	} else if (max_speed == SPEED_5000) {
>>> +		linkmode_set_bit(ETHTOOL_LINK_MODE_2500baseX_Full_BIT, supported);
>>> +		linkmode_set_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT, supported);
>>> +		linkmode_set_bit(ETHTOOL_LINK_MODE_5000baseT_Full_BIT, supported);
>>> +	}
>>> +
>>> +	linkmode_copy(phydev->supported, supported);
>>> +}
>>
>> So you have got lots of comments....
>>
>> Please split this into two patches. One patch for the PHY you are
>> interested in, and a second patch to remove phy_set_max_speed() and
>> fix up that PHY.
>>
> Noted! 
> 
>> Also, i would prefer you do the normal feature discovery, calling
>> genphy_read_abilities() and/or genphy_c45_pma_read_abilities() and
>> then fixup the results by removing the modes which should not be
>> there.
>>
> Sounds good! 
>> Take a look at bcm84881_get_features() as an example.
>>
> Thanks Andrew! 

Andrew, (Let me know if this is okay with you)
On doing the normal feature discovery today I observed that AQR115C 
i want to document this misbehavior here for future reference

//Print added by me in the AQR driver 
[    5.583440]  AQR supported mask=00,00000000,00018000,000e102c

key points :- 

AQR115c supports 10Mbps(F/H) but feature discovery says no
AQR115C supports 1Gbps(F/H) but feature discovery says no 
AQR115C supports 2500BaseX but feature discovery says no 
AQR115C supports Autoneg but feature discovery says Hell no! 
AQR115C does not support 10GBaseT/KX4/KR but feature discovery says yes 
AQR115c does not support 5GbaseT but feature discovery says yes 

I have got 2 different FW from Marvell but none seem to help. 
I also got the confirmation from Marvell folks that Autoneg is supported but 
the feature discovery says otherwise. 

Here is the thing what i am going to do for now. (Let me know if this is okay with you)
1. Raise FIXUP for AQR115c patch 
- remove all the features which are not support 
- Add supported features which we really requires such as Autoneg/phy_gbit_features/2500BaseX/BaseT(Clearly see them supported in the NDA internal docs) 
Reference for other folks for information which is public :
https://www.marvell.com/content/dam/marvell/en/public-collateral/transceivers/marvell-phys-transceivers-aqrate-gen4-product-brief.pdf


2. Raise FIXUP patch by removing phy_set_max_speed and call 
- Generic function which sets speeds up to 2.5Gbps for AQCS109 (2.5Gbps max speed)
- Generic function which sets speeds up to 5 Gbps for AQR111/B0/114c (5Gbps max speed)

Both points 1 and 2 will be a patch series with cover letter. 



>> 	Andrew