linux-kernel - Re: [PATCH net-next v3 10/47] net: phylink: Adjust link settings based on rate adaptation

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:   Mon, 18 Jul 2022 12:45:01 -0400
From:   Sean Anderson <sean.anderson@...o.com>
To:     "Russell King (Oracle)" <linux@...linux.org.uk>
Cc:     "David S . Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Madalin Bucur <madalin.bucur@....com>, netdev@...r.kernel.org,
        Paolo Abeni <pabeni@...hat.com>,
        Eric Dumazet <edumazet@...gle.com>,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
        Alexandru Marginean <alexandru.marginean@....com>,
        Andrew Lunn <andrew@...n.ch>,
        Heiner Kallweit <hkallweit1@...il.com>,
        Vladimir Oltean <olteanv@...il.com>
Subject: Re: [PATCH net-next v3 10/47] net: phylink: Adjust link settings
 based on rate adaptation



On 7/18/22 12:12 PM, Russell King (Oracle) wrote:
> On Fri, Jul 15, 2022 at 05:59:17PM -0400, Sean Anderson wrote:
>> If the phy is configured to use pause-based rate adaptation, ensure that
>> the link is full duplex with pause frame reception enabled. Note that these
>> settings may be overridden by ethtool.
>> 
>> Signed-off-by: Sean Anderson <sean.anderson@...o.com>
>> ---
>> 
>> Changes in v3:
>> - New
>> 
>>  drivers/net/phy/phylink.c | 4 ++++
>>  1 file changed, 4 insertions(+)
>> 
>> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
>> index 7fa21941878e..7f65413aa778 100644
>> --- a/drivers/net/phy/phylink.c
>> +++ b/drivers/net/phy/phylink.c
>> @@ -1445,6 +1445,10 @@ static void phylink_phy_change(struct phy_device *phydev, bool up)
>>  	pl->phy_state.speed = phy_interface_speed(phydev->interface,
>>  						  phydev->speed);
>>  	pl->phy_state.duplex = phydev->duplex;
>> +	if (phydev->rate_adaptation == RATE_ADAPT_PAUSE) {
>> +		pl->phy_state.duplex = DUPLEX_FULL;

Just form context, as discussed with Andrew, this should never change
anything. That is, it could be replaced with

WARN_ON_ONCE(pl->phy_state.duplex != DUPLEX_FULL);

Since the phy should never report that it is using rate_adaptation
unless it is using full duplex.

>> +		rx_pause = true;
>> +	}
> 
> I really don't like this - as I've pointed out in my previous email, the
> reporting in the kernel message log for "Link is Up" will be incorrect
> if you force the phy_state here like this. If the media side link has
> been negotiated to be half duplex, we should state that in the "Link is
> Up" message.

So I guess the question here is whether there are phys which do duplex
adaptation. There definitely are phys which support a half-duplex
interface mode and a full duplex link mode (such as discussed in patch 08/47).
If it's important to get this right, I can do the same treatment for duplex
as I did for speed.

> It's only the PCS and MAC that care about this, so this should be dealt
> with when calling into the PCS and MAC's link_up() method.
> 
> The problem we have are the legacy drivers (of which mv88e6xxx and
> mtk_eth_soc are the only two I'm aware of) that make use of the
> state->speed and state->duplex when configuring stuff. We could've been
> down to just mv88e6xxx had the DSA and mv88e6xxx patches been sorted
> out, but sadly that's now going to be some time off due to reviewer
> failure.
> 

--Sean