linux-kernel - Re: [PATCH net-next 4/7] net: phy: marvell10g: Add support for 2.5GBASET and 5GBASET

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, 22 Jan 2019 11:08:51 +0100
From:   Maxime Chevallier <maxime.chevallier@...tlin.com>
To:     Andrew Lunn <andrew@...n.ch>
Cc:     davem@...emloft.net, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org,
        Florian Fainelli <f.fainelli@...il.com>,
        Heiner Kallweit <hkallweit1@...il.com>,
        Russell King <linux@...linux.org.uk>,
        linux-arm-kernel@...ts.infradead.org,
        Antoine Tenart <antoine.tenart@...tlin.com>,
        thomas.petazzoni@...tlin.com, gregory.clement@...tlin.com,
        miquel.raynal@...tlin.com, nadavh@...vell.com, stefanc@...vell.com,
        mw@...ihalf.com
Subject: Re: [PATCH net-next 4/7] net: phy: marvell10g: Add support for
 2.5GBASET and 5GBASET

Hello Andrew, Russell,

On Mon, 21 Jan 2019 21:17:15 +0100
Andrew Lunn <andrew@...n.ch> wrote:

>> @@ -264,8 +265,10 @@ static int mv3310_config_init(struct phy_device *phydev)
>>  	if (ret)
>>  		return ret;
>>  
>> -	linkmode_and(phydev->advertising, phydev->advertising,
>> -		     phydev->supported);
>> +	/* Make sure we advertise all the supported modes, and not just the
>> +	 * default one specified in the driver's .features.
>> +	 */
>> +	linkmode_copy(phydev->advertising, phydev->supported);  
>
>So by doing a copy of supported into advertising, you can stomping
>over any restrictions applied via of_set_phy_supported(),
>of_set_phy_eee_broken(phydev), and any pause control settings which
>might of happened.

Thanks for the explanations, this is indeed clearly not a good solution.

>What might make sense here is that a PHY driver can replace its
>.features member at run time, in its config_init() call. The core then
>needs to perform these evaluations. So i'm guessing we need to split
>this code out of probe() and move it into phy_init_hw()?

So the .features won't be read-only anymore ? We could also simply make
a helper that would add a mode to both the supported and advertising
modes list, that would be used in the 'genphy_c45_pma_read_abilities'
and config_init ?

I lack the big picture of the PHY init sequence, there seems to be a
lot of quirks and complex cases that we need to take into account, so
I'll let you decide :)

>Heiner, you know this code better than anybody. What do you think?
>
>	Andrew

Thanks for the feedback,

Maxime

-- 
Maxime Chevallier, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com