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;
Message-ID: <3919acb9-04bb-0ca0-07b9-45e96c4dad10@seco.com>
Date:   Thu, 5 Jan 2023 12:43:47 -0500
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.4.0
Subject: Re: [PATCH net-next v5 4/4] phy: aquantia: Determine rate adaptation
 support from registers
Content-Language: en-US
To:     Vladimir Oltean <olteanv@gmail.com>
Cc:     Andrew Lunn <andrew@lunn.ch>,
        Heiner Kallweit <hkallweit1@gmail.com>, netdev@vger.kernel.org,
        Russell King <linux@armlinux.org.uk>,
        "David S . Miller" <davem@davemloft.net>,
        Paolo Abeni <pabeni@redhat.com>, linux-kernel@vger.kernel.org,
        Jakub Kicinski <kuba@kernel.org>,
        Eric Dumazet <edumazet@google.com>,
        Tim Harvey <tharvey@gateworks.com>
References: <20230103220511.3378316-1-sean.anderson@seco.com>
 <20230103220511.3378316-5-sean.anderson@seco.com>
 <20230105140421.bqd2aed6du5mtxn4@skbuf>
 <6ffe6719-648c-36aa-74be-467c8db40531@seco.com>
 <20230105173445.72rvdt4etvteageq@skbuf>
From:   Sean Anderson <sean.anderson@seco.com>
In-Reply-To: <20230105173445.72rvdt4etvteageq@skbuf>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?aDJWdGc3N215SmdZQ3U2UmN5dHhqYVI3L1hham1scUxMYi9nY2FlMSs2SGx3?=
 =?utf-8?B?VlJYUndLZUJ2Yko3YXVtV090T1Z6VjBOTzFYMkdISFhWQXBGdU9rb0xiUjgv?=
 =?utf-8?B?MWtHWkprOVhBa3BNcU9aRFFTZEVMMlBqb0R4dUV5STk2WVBRM05hcTBFNEFB?=
 =?utf-8?B?d0gvSjlwQVlVQnZLemc3K01mOEhKSnNBd083RXBZTjJpTDY3M1o4enBtajFU?=
 =?utf-8?B?T3Jib0VtZ1YvMmRnVEduSnVPQTF1ZWtxempOZE55VFZBckpleWNtK1ZiTVg1?=
 =?utf-8?B?RVlkNi9OZmtiaUZXSFdUSkd0WUE5Q1FWNXJVblFTeU02ckZjWW54R1JOck1y?=
 =?utf-8?B?YlFVR2NjTWt4QU1CMlgrbkp3VWtrY3owVTBSeFA3aFVYV0JBMFFtSlhDUncx?=
 =?utf-8?B?YVVXbnBJSlBzWCtmaW1oRlU5djJCODQ5SVZhK2JOWE9tWFVWUXY5NXl3TnpQ?=
 =?utf-8?B?cW5SbTVlelFlMWtxQndGUUUzY2dtSEVydkFhL3dpRU54MGRVV3NzeVN5cmQ5?=
 =?utf-8?B?OExmNmZ2T0NxNmhJcGd6KzlJMFdvOGdFMHlvK1pIMWl5TGxJWGtVSER2TDVD?=
 =?utf-8?B?cDNsMDVtWi9iUzE1Y2xXSVg1RnN3VzVDZHNLc3Q0MENpb3RvcmYxeGovQWRq?=
 =?utf-8?B?Y3FENmFtRFFMcmJrOE1lOWtuWkhQQTZOUGVUWGZJNzZKT3JTMVJnU25XZ0s0?=
 =?utf-8?B?Qi9TM1MrTTZFV2lGRTNLWi8zUjJ2bWdDdFZJVnZoaEsycVhvbFF0WlBFVGRs?=
 =?utf-8?B?dE81amNyMERFL2ErcWtCVWR2OGhQMjZpcUxhQ2hrVE1STVNUTXdjY1ZGRFBC?=
 =?utf-8?B?d0crQTZZVGNPQnJDU2k5ZENPSjM2TjNXdldqNGN3WjlJMkxDRVZnT0J5eVpi?=
 =?utf-8?B?aVA2SDMreXRzYXcyWmdFeVNFZU1GdW1iL0hvRUVCR3p1SXk4ZU0yYXpVR1d4?=
 =?utf-8?B?ZFd4eE1KUTQ3bjQxcWpORndlTyszSytjS0d1L2ltMW9PQy9teHYxNXBnSWxU?=
 =?utf-8?B?UkFBbVpnWUYvanZmellwazB5YUFDdXEwdi9TUjNMdVpKNnJrazh3Y0ZkQkY2?=
 =?utf-8?B?Uk96L1lCcmFJeUVnMVV4TzY4Q2p2L3dmbFlBSy9WdEg4c0FobEVBNlNGYS9O?=
 =?utf-8?B?N2pJQTl1TEpnQjB5Z2dmWFJubGplTDRUQXQ4V3VENzZteUhldUVYWXRPL1Fz?=
 =?utf-8?B?U1RzOWsvd1BVbjNiSGtyQzg4QzNWTlpkckozM1dNT2llM0FSS2Z3Ym1xVXNY?=
 =?utf-8?B?ZndWT3BOeWVpZ1ZKL0NWOHllUHNxMDNXRHBQYmFCYnc1NDcvdlI1S0tkeDZO?=
 =?utf-8?B?d0lBMXVjZ0NYa2Uvdy8xSW9PQzJSUTJ6eDl5U2o0eXBZZFNIOXpwL2Nad2VB?=
 =?utf-8?B?N1Zab1JKdnNTWjk1KzBxYTRUQmxFWlN1cmZrdTNXUExYTUlJQlBIS2MyNytp?=
 =?utf-8?B?NDRhQlJSNENsVXV5OXUrb0p4eXBMSjhiQVJ3U0VDdzZ5R3J2Z29mVDhIQ2NC?=
 =?utf-8?B?c0FTN0pISU53L0FWekNZaXVIRU03SzF2eUxXN0V4V3lmSXFyeFI0d1FCc00y?=
 =?utf-8?B?YXplWmM5SVYvK2d4eEl0YVJkU2diODBLTEVYUVZvRms1bko4WEU4Y1poa3lF?=
 =?utf-8?B?R3dwS1FSdTZxZUpzWDRud2RxdC9Cc1JqcmNnQUhjL1BLNFNzak9HdkRhRmpP?=
 =?utf-8?B?RDFvUElzQXB6UWRuUlhEVGxNZElhajUzSzBFcFdXM2paZVRMYkMrRUlPYm5y?=
 =?utf-8?B?VlFnS21BcE1EWGlPQ2tXZCtFZW1Oc2tvSjhpc3ZsMkk3ZXhuZHRIdEhtSG0z?=
 =?utf-8?B?STB5OEwvV0tqMEdLL1JaN042Rm41MjFhSmQ0TUdrNTFzbW1BUFV0UTEyVWFo?=
 =?utf-8?B?b1dSRFhod3VVNHMzSXRmeVJtYTl0VDFySTBCV01neW9zZm1PV292TzdxQ0w5?=
 =?utf-8?B?KzQzcjBReVdiL0VZUktPTnNHdVhTVGJPZlFEVGppODQvWDNOa3gwYmZ4eWFG?=
 =?utf-8?B?ZkdveEFtRFIybjVJNm1qYVBZckZRSzJReHNiRHUvaXFPZFhkbVpEQWlsSlJQ?=
 =?utf-8?B?b1p5ZWdkTFUrQk4wYTlCcG1qT3huekJLQVNnT3JSK1ZlTDh0TkUyVExxdmI4?=
 =?utf-8?B?NmNtRHd6UzBkeERadnJmSjZLRi9lL0IrcU56OWpHUG1yZW1xd2RFYTlqRDMw?=
 =?utf-8?B?Q1E9PQ==?=
X-OriginatorOrg: seco.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 6cf660a2-d7aa-4c47-d133-08daef446860
X-MS-Exchange-CrossTenant-AuthSource: DB9PR03MB8847.eurprd03.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 05 Jan 2023 17:43:51.5910
 (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: 5zeGUHLIundkpSPLKq0wdFqM4Dih1OogeBV2Cfni+4+eR1kBE9pWlkRZ9gD0wOK3+w8qODuuYz/UGuFL1vjdyQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PAXPR03MB8273
X-Spam-Status: No, score=-5.0 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 1/5/23 12:34, Vladimir Oltean wrote:
> On Thu, Jan 05, 2023 at 11:21:14AM -0500, Sean Anderson wrote:
>> > Your code walks through the speed_table[] of media speeds (from 10M up
>> > until the max speed of the SERDES) and sees whether the PHY was
>> > provisioned, for that speed, to use PAUSE rate adaptation.
>> 
>> This is because we assume that if a phy supports rate matching for a phy
>> interface mode, then it supports rate matching to all slower speeds that
>> it otherwise supports. This seemed like a pretty reasonable assumption
>> when I wrote the code, but it turns out that some firmwares don't abide
>> by this. This is firstly a problem with the firmware (as it should be
>> configured so that Linux can use the phy's features), but we have to be
>> careful not to end up with an unsupported combination.
> 
> When you say "problem with the firmware", you're referring specifically
> to my example (10GBASE-R for >1G speeds, SGMII for <=1G speeds)?

Actually, I am mostly referring to cases where rate adaptation is set up
to use a phy interface mode which isn't supported. In particular, Tim
has a board where the phy is set up to rate adapt using 5GBASE-R (-X?),
even though the host only supports 10GBASE-R.

> Why do you consider this a firmware misconfiguration? Let's say the host
> supports both 10GBASE-R and SGMII, but the system designer preferred not
> to use PAUSE-based rate adaptation for the speeds where native rate
> adaptation was available.

This is supported, you just can't get 5G or 2.5G.

>> > If the PHY firmware uses a combination like this: 10GBASE-R/XFI for
>> > media speeds of 10G, 5G, 2.5G (rate adapted), and SGMII for 1G, 100M
>> > and 10M, a call to your implementation of
>> > aqr107_get_rate_matching(PHY_INTERFACE_MODE_10GBASER) would return
>> > RATE_MATCH_NONE, right?
>> 
>> Correct.
>> 
>> > So only ETHTOOL_LINK_MODE_10000baseT_Full_BIT
>> > would be advertised on the media side?
>> 
>> If the host only supports 10GBASE-R and nothing else. If the host
>> supports SGMII as well, we will advertise 10G, 1G, 100M, and 10M. But
>> really, this is a problem with the firmware, since if the host supports
>> only 10GBASE-R, then the firmware should be set up to rate adapt to all
>> speeds.
> 
> So we lose the advertisement of 5G and 2.5G, even if the firmware is
> provisioned for them via 10GBASE-R rate adaptation, right? Because when
> asked "What kind of rate matching is supported for 10GBASE-R?", the
> Aquantia driver will respond "None".

Correct.

>> > Shouldn't you take into consideration in your aqr107_rate_adapt_ok()
>> > function only the media side link speeds for which the PHY was actually
>> > *configured* to use the SERDES protocol @iface?
>> 
>> No, because we don't know what phy interface modes are actually going to
>> be used. The phy doesn't know whether e.g. the host supports both
>> 10GBASE-R and SGMII or whether it only supports 10GBASE-R. With the
>> current API we cannot say "I support 5G" without also saying "I support
>> 1G". If you don't like this, please send a patch for an API returning
>> supported speeds for a phy interface mode.

Again, this is to comply with the existing API assumptions. The current
code is buggy. Of course, another way around this is to modify the API.
I have chosen this route because I don't have a situation like you
described. But if support for that is important to you, I encourage you
to refactor things.

--Sean