Re: [PATCH net-next v5 1/8] net: phylink: Document MAC_(A)SYM_PAUSE

[Sean Anderson <sean.anderson@seco.com>]

On 9/7/22 5:38 AM, Russell King (Oracle) wrote:
> On Tue, Sep 06, 2022 at 12:18:45PM -0400, Sean Anderson wrote:
>> This documents the possible MLO_PAUSE_* settings which can result from
>> different combinations of MLO_(A)SYM_PAUSE. These are more-or-less a
>> direct consequence of IEEE 802.3 Table 28B-2.
>>
>> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
>> ---
>>
>> (no changes since v3)
>>
>> Changes in v3:
>> - New
>>
>>   include/linux/phylink.h | 16 ++++++++++++++++
>>   1 file changed, 16 insertions(+)
>>
>> diff --git a/include/linux/phylink.h b/include/linux/phylink.h
>> index 6d06896fc20d..a431a0b0d217 100644
>> --- a/include/linux/phylink.h
>> +++ b/include/linux/phylink.h
>> @@ -21,6 +21,22 @@ enum {
>>   	MLO_AN_FIXED,	/* Fixed-link mode */
>>   	MLO_AN_INBAND,	/* In-band protocol */
>>   
>> +	/* MAC_SYM_PAUSE and MAC_ASYM_PAUSE correspond to the PAUSE and
>> +	 * ASM_DIR bits used in autonegotiation, respectively. See IEEE 802.3
> 
> "used in our autonegotiation advertisement" would be more clear.

What else would it be (besides advertisement)? Regarding "our", these bits are
also set based on the link partner pause settings (e.g. by phylink_decode_c37_word).

>> +	 * Annex 28B for more information.
>> +	 *
>> +	 * The following table lists the values of MLO_PAUSE_* (aside from
>> +	 * MLO_PAUSE_AN) which might be requested depending on the results of
>> +	 * autonegotiation or user configuration:
>> +	 *
>> +	 * MAC_SYM_PAUSE MAC_ASYM_PAUSE Valid pause modes
>> +	 * ============= ============== ==============================
>> +	 *             0              0 MLO_PAUSE_NONE
>> +	 *             0              1 MLO_PAUSE_NONE, MLO_PAUSE_TX
>> +	 *             1              0 MLO_PAUSE_NONE, MLO_PAUSE_TXRX
>> +	 *             1              1 MLO_PAUSE_NONE, MLO_PAUSE_TXRX,
>> +	 *                              MLO_PAUSE_RX
> 
> Any of none, tx, txrx and rx can occur with both bits set in the last
> case, the tx-only case will be due to user configuration.

What flow did you have in mind? According to the comment on linkmode_set_pause,
if ethtool requests tx-only, we will use MAC_ASYM_PAUSE for the advertising,
which is the second row above. I don't see the path where we set both
MAC_SYM_PAUSE and MAC_ASYM_PAUSE and then we resolve to MLO_PAUSE_TX. Would this
be due to manual user configuration of both the advertising and the pause? By
that logic, couldn't we get any pause mode from any combination of MLO_PAUSE_*?

--Sean