Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Subject: Re: [PATCH net-next v3 10/47] net: phylink: Adjust link settings
 based on rate adaptation
To:     "Russell King (Oracle)" <linux@armlinux.org.uk>
Cc:     "David S . Miller" <davem@davemloft.net>,
        Jakub Kicinski <kuba@kernel.org>,
        Madalin Bucur <madalin.bucur@nxp.com>, netdev@vger.kernel.org,
        Paolo Abeni <pabeni@redhat.com>,
        Eric Dumazet <edumazet@google.com>,
        linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org,
        Alexandru Marginean <alexandru.marginean@nxp.com>,
        Andrew Lunn <andrew@lunn.ch>,
        Heiner Kallweit <hkallweit1@gmail.com>,
        Vladimir Oltean <olteanv@gmail.com>
References: <20220715215954.1449214-1-sean.anderson@seco.com>
 <20220715215954.1449214-11-sean.anderson@seco.com>
 <YtWGZ4ZJ6rmLmlWk@shell.armlinux.org.uk>
From:   Sean Anderson <sean.anderson@seco.com>
Message-ID: <9a968425-5621-09b9-febe-2086d5492c96@seco.com>
Date:   Mon, 18 Jul 2022 12:45:01 -0400
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.10.0
In-Reply-To: <YtWGZ4ZJ6rmLmlWk@shell.armlinux.org.uk>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?bzArRTFVOVk1OS9FckxoOWNVbFVBWnRXc2I2K2hiclZtN2xEZ0VDZHFyeFFh?=
 =?utf-8?B?MTRYQnZzekJwVWdzRVFTT0NuaGc0cmdKWW9iK1dheTd5ZUkxQ1RGc0pjWm5C?=
 =?utf-8?B?ODA3dVpLVklQUmRxSFhPYXExL3RUTjF2cGRZZXZ3M0dTaHBDZU9TK0RRNFBI?=
 =?utf-8?B?UEFuSjNGVW94RFlQY1NGZE1pSy9xKzdlOENiaUp4dzE4N3FhdndQVmpvazlH?=
 =?utf-8?B?VnZTTkp4eFp4SkVoK3EvUmU5VnIzNzkwNUxuQVI3L2RKbnRYcnZjeVdKcnU0?=
 =?utf-8?B?ekEwRDMrUUtpQ3lYRVlHeC9oMm5YU2NTeDFYekpGbnJncDNXajVNOFB3SlVp?=
 =?utf-8?B?ZmZuU3JJYmNRNXJCUHNVTmNHdFJFRnBYcjd3QldEM1FXKzg4SXVkOHdEcHVU?=
 =?utf-8?B?T2JFd2tUdk5jUjNXM3NOaHVEOTdTQlZUS3VhdHkwVi9kU0pFejJpcG01WjZy?=
 =?utf-8?B?d2pPZWdORDRvMzc1d0FBdzB1YmUxb0VLY3JaWDBMRXIwdzBOazh5emF1YzdQ?=
 =?utf-8?B?b3grT3BQUlJJSHI4anU3cnROYmI3SzNOaGttRUpPU09IN2VpK3I3UFZEMkdi?=
 =?utf-8?B?WmxlcWpRSG9Wanh1RG1hbFBkYVJncFJhV0QyK04ySklGNEMwVm40ZW80VXJs?=
 =?utf-8?B?dXdpcm5kbGxCRVAvbXIwNno2MnNieFNDQm5xRGdGMzIzRkFmRUswdStBbTYr?=
 =?utf-8?B?ZjFqM3Z2UGtCZ09rdnoxS0lzUzlvMnM3UGF2bnJEMUFyazRBTzc2bS91ZTh3?=
 =?utf-8?B?Wlc3TVRBSkk5QTd1NmhrdVpXV3VFcmRWbHFydkdhSHdLSEcrUk9wL3AzUWlM?=
 =?utf-8?B?Si9lYXFOaytUT1k4cjNiYzJwY0VlU3dlSHVqM24yTkZVYzRsd3UyV09aamtW?=
 =?utf-8?B?a2FXY0RxV21ZSjNhZFVZKy9vOWw1bWRRQWhVb2RzbThFUFVNU3F1Uy9ITytS?=
 =?utf-8?B?VWZPYW5wVEtCRE9pN2RzUDU3ODg4RjE1OFhYcnRIWWRLeHFBaXp2aEdqeHJO?=
 =?utf-8?B?b2ZWVWM4d1ZCNVZWOWVpRGM0VEJLZitSNW1aSlJsVkVnN3dxWHlvdVZDUy9N?=
 =?utf-8?B?c3lBWmdDM3dRUVpwN29TNSt6T25hUFd5azhZMW5HemdESEMxN1VLQkNVRE5S?=
 =?utf-8?B?WVNBKzFMZE8yZ3NiQnhwd0drUHFLVjl6bGs1K0dpRlRIbHYybSt2eEhQK3o3?=
 =?utf-8?B?clpVYjZIS0tFME9mTnExaTdySk94a1c1S1lReXZ1ZXdDQUwzUVBUOUpuYnl6?=
 =?utf-8?B?dU05eitubVIzNGExNnFFNGhadW14Ykh6bHA1VWtyUnZFTW5QOVUwNkZpSnBC?=
 =?utf-8?B?SytpS3NSWEFDaE9kQlBydEZkQm9LVzlnUWJHOVVnVTFVaElwVmNnRmlWcmdE?=
 =?utf-8?B?YUs2WHJmanNWQWo1cHYyYlRHTUxnek1aMlkzMlYvZnZtRGVuR3RtaEJ0OTMv?=
 =?utf-8?B?dGlHd2NVQStHVDhOejVmbVpteVVZblVaZlN1Y3hWNzYrZVRuMkNWNnVJd1BM?=
 =?utf-8?B?MUZ0b21ocDhOWnRZUFZKeTM1TFJPS2NvaStWUVZvYlJQVTRJUmVMVVJ0bFNJ?=
 =?utf-8?B?WXBORklNc2NsNjh6U0l5ZFlsSGtZUVBPMEQyTWRrYnBOdkU0YkdwbHNNeGla?=
 =?utf-8?B?YU50bmxhSTcxWFh4TkwxSXpqRmNOQUp3UHZ2c3VxWTAySHhjckJnUWgwS25n?=
 =?utf-8?B?OXRGbDRhbWdtd0dCa1JSblNEZkIzMlJiZHZnQTNTbFQxY3JQbG1DZ0RyYWZW?=
 =?utf-8?B?Z0pKRzd2Z2QwaHlxZEZtbDl6cEpuT2Vvd1k0Q09mSWQxUG1MVElNYWRjZFc0?=
 =?utf-8?B?anpQeVFvTHRpR0hzOWlHakE5R29xNkNpRXYzUlo3MWR3WEg3dENZWm15cEhk?=
 =?utf-8?B?TmZyVGkxRDBqYmVMbUxGdTNnVWxzVUl3V2h5WUFMdS96NlBveVZPeDJ3KzNU?=
 =?utf-8?B?d3JtQ2UwYjg2Y3FHU09seGVQT1dqMVpZYzZ6MlBFWFJRN0lXZU4xL3VCdXNO?=
 =?utf-8?B?WVd5SWtCd2lYSVJ6VCs1eW0wMWZRTno2ZFk2RTRyeHZXRVc5SjVscUZTQnZF?=
 =?utf-8?B?dTZCT1BxVXV4ZWpoc2cyR1VWN0hnSUVoKzg3NHZuK3JhbVNJZ1RkMmlKZ0t4?=
 =?utf-8?B?VGFENXZpdk56QStndUFaNld1bjVSNkhYdW1jekFVUEJqVDU4ZzFYUHNadERk?=
 =?utf-8?B?d3c9PQ==?=
X-OriginatorOrg: seco.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 4da51b64-a2d0-4681-96af-08da68dcdf15
X-MS-Exchange-CrossTenant-AuthSource: DB7PR03MB4972.eurprd03.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 18 Jul 2022 16:45:07.2580
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: bebe97c3-6438-442e-ade3-ff17aa50e733
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: qrjYozCM2sYMYz88KOwXKRvTjkYd9hWgS7bNeFSjCANIlUEVWbgNVPz8dPg+G5Byz7JdKbV0l1N5s3eE1/olNA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: AM6PR03MB5814
X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,RCVD_IN_DNSWL_NONE,
        RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_PASS autolearn=ham
        autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


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@seco.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