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:     Andrew Lunn <andrew@lunn.ch>
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,
        Russell King <linux@armlinux.org.uk>,
        linux-kernel@vger.kernel.org,
        Alexandru Marginean <alexandru.marginean@nxp.com>,
        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> <YtMc2qYWKRn2PxRY@lunn.ch>
From:   Sean Anderson <sean.anderson@seco.com>
Message-ID: <4172fd87-8e51-e67d-bf86-fdc6829fa9b3@seco.com>
Date:   Sat, 16 Jul 2022 18:37:22 -0400
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.12.0
In-Reply-To: <YtMc2qYWKRn2PxRY@lunn.ch>
Content-Type: text/plain; charset=utf-8; format=flowed
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?VUc1RVZrNkdZcE90eGFIeFNTV2trMlZodjhBWSt5dkg0YUdoQ0NGTm9kcTBL?=
 =?utf-8?B?OVk2Qk1sNXVrMkhWNjcrU0p5WnZsM1Y4RjlNMmR3bnhpS2tzeUJhSU4vVUpO?=
 =?utf-8?B?eDRzS3RxYWYyZlZpTmVSRTRjMFl4Mmp4TWYxRVZSa1VXU1VDbUt4eE1hSkov?=
 =?utf-8?B?YXgwRUgvbDY3cHgvZkV3ZDFPQmZHR0R4WkFoeXBQdlN3VFFydTBMSEdyb0F6?=
 =?utf-8?B?aE5RYStiVG1KV3Faem9vSTJ1RlF3aVBPbFlYaFFoM1pUekRTQlgyM05PcDlU?=
 =?utf-8?B?bHJtSG81M0xKbmFVUDIyVE1QdG1lNE5kRUR6TGRqekVXTnlUMTJqczhyNHBw?=
 =?utf-8?B?bWZ1Wm9NVW9RQ1NaRFBmN3BqOVFDcW13Q0J3MGQvYzBudVJGK3N3QVpySXNJ?=
 =?utf-8?B?SksrU1lCaVBXR3lWTDg0aUVDUjJkRURsY2tibUlHWGdFNm56ejhYQTR1bjkx?=
 =?utf-8?B?ZHNQSHJEcXpwRXd5R2VscmhUNnQrdCszYjJXUlp6Wk1jV0RnbzNGUkhHSW5Z?=
 =?utf-8?B?S0YrdlNBNWlPMVFHcEhWd3V2ajRYaEszNXhCNytUMDgyRGJqQUZkcUwwR2p2?=
 =?utf-8?B?MGtMVXVVbG04SkRUYTVpUkYrcVQ4NTc0OHE3Zlp6Zjk1UE5rdUt6SFhNSXEw?=
 =?utf-8?B?MDhYUVRuQXFhUmdsQlpyTkp6anBzVTBaZngybGR2UVVNWXByWnFqc3lkU1dQ?=
 =?utf-8?B?ZVRsUEtDUXVodnBzaWl0MEl4bVdjU0E3anJFdXczdkNuZWJnNUpoeFYzclRn?=
 =?utf-8?B?Y3o2S2FyQ2M2dHU1amluUWdLdSsySTdGUTdESndiNkE5Y2Q5bGduZGhJbm93?=
 =?utf-8?B?eDRReTJjaElqdFRQNHg1TVpaNm9kbXBBR1V6cVlnQUs2UFhIdGk2T3JRRERz?=
 =?utf-8?B?QXJzNkpqdkF2ZGwrOFhYZnJlcGVMTTc0dk9YbkFBVDJmUkNXL0NvL0JtWTI4?=
 =?utf-8?B?MUFCT0JjaXlRTWpFS3hYQ3UyZkFyb0c1MzlhQWRwVlJFakhCajNnTWw4YzNX?=
 =?utf-8?B?c1F0M1NaU3RZL0ZldWdsMEwvaDNvL3ovUHdDNjRpTWtSa2IwdnprdTVUSzBj?=
 =?utf-8?B?MTRYODJqVGhjdDVvVDZRcEgyUW9XOUlZc01xeXVQYTM1SHZOMk1XY25vUUZ2?=
 =?utf-8?B?OU5idkJHZVB3dDJ0Szc5YkxwUTN1Rnpldnc3aFpUajdENTZmbDFlekpKRm9a?=
 =?utf-8?B?TWoyNEhYZnZNU3U1OXRaV1RmMFA4ZjkyTG5LYmhQb3R6Z2xlSkw3eitqcThQ?=
 =?utf-8?B?eDhRNW5UZ0RERERQbmwzUlp3R0k5MkZZWlZvanltZDhtWWVsNE1ieGlkNGxO?=
 =?utf-8?B?UytFTldtMURtMGhFMEdqL1JZVEF1MVR3L3d4TTl6ZGZrbzU2Zm0veWNvREJ3?=
 =?utf-8?B?czN2SmhldkhFQ0UxMUxpOXhSNGlsbW5JVlFIYWFPZWhXUFBBMjNDZWZnN0o3?=
 =?utf-8?B?SGFNSmg1MHVna0tELzRieWs1Y3E3cHhpeHoxZDhJS2owdGIvU2pRck9tSTBT?=
 =?utf-8?B?MSttd0JBNTVNT0hOYk51cGM0Y2h1TGxYN3U5K0xZdDR4bmNTRUorVktreDJQ?=
 =?utf-8?B?N21wUUpjZm0ybjI2ZFJkWVF4Y2drVVdRbFE5MVhzeVZzVStIOFkzbVlySDhC?=
 =?utf-8?B?Nm9iT3VVM3JJVFBvVjMyUjdSeUxhTVA3NndzUUhmUytSU1YwMGpwYU9UektX?=
 =?utf-8?B?ai9HeGk3eTFwSjFMTDIvajJFM3FzUjFNcjhEb1BaaXNHQzRiZC9DbTc2VEJS?=
 =?utf-8?B?eTB1VzlrNndDMUlvQXNtZHZUKzU5WjZxZTllVHU2YkNCWkh0ZVdtaFR5ajBZ?=
 =?utf-8?B?c2tScndQdGtDSXF4VklHck51eExSWWl1VkJieWU3V1VnU0xFYjQ5NFJ1eCtF?=
 =?utf-8?B?Uy9oVWw5MUdzMFphekpkNzNRd2RuQnJDVFo5Sm1EU1pWMUVIRDZGSW9vbXdR?=
 =?utf-8?B?SER3L1czKzhmNXF6NlR3ckhxVnRPekw3ZEtZdTJtRnpvTGczS0tLKzhzUXRE?=
 =?utf-8?B?ODJXVnFXejVHdktEMUNjTTgzdHRudnpmWTFaMTRjMHNQZElsRmViaGdEOFRh?=
 =?utf-8?B?SUY4RVY5eW1XUDVyb2ZNQVJOZ0xqRTUrVzN0cGhmaS9Qb3dEWDRKMGxyY0Mz?=
 =?utf-8?Q?yN8ZhauY1cCq8jS1ERCGiaNt2?=
X-OriginatorOrg: seco.com
X-MS-Exchange-CrossTenant-Network-Message-Id: e951a8b4-ea05-49ca-7f6d-08da677bc27b
X-MS-Exchange-CrossTenant-AuthSource: DB7PR03MB4972.eurprd03.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 16 Jul 2022 22:37:26.8951
 (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: Czo8unPwV2/yiZXnHlp2rtIoB2XJ1F2D1VocIQZMNkCsQlCRPsuOdyxIcoqXQvFBcwOulIP/McgdzQ8sW0SLfg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PAXPR03MB7901
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/16/22 4:17 PM, Andrew Lunn 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;
>> +		rx_pause = true;
>> +	}
> 
> I would not do this. If the requirements for rate adaptation are not
> fulfilled, you should turn off rate adaptation.
> 
> A MAC which knows rate adaptation is going on can help out, by not
> advertising 10Half, 100Half etc. Autoneg will then fail for modes
> where rate adaptation does not work.

OK, so maybe it is better to phylink_warn here. Something along the
lines of "phy using pause-based rate adaptation, but duplex is %s".

> The MAC should also be declaring what sort of pause it supports, so
> disable rate adaptation if it does not have async pause.

That's what we do in the previous patch.

The problem is that rx_pause and tx_pause are resolved based on our
advertisement and the link partner's advertisement. However, the link
partner may not support pause frames at all. In that case, we will get
rx_pause and tx_pause as false. However, we still want to enable rx_pause,
because we know that the phy will be emitting pause frames. And of course
the user can always force disable pause frames anyway through ethtool.

--Sean