Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:5 as permitted sender) client-ip=2620:137:e000::3:5;
Message-ID: <4aeebf95-cf12-4462-80c7-dd1dafddb611@intel.com>
Date:   Wed, 29 Nov 2023 15:04:56 -0800
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH net v2] r8169: fix rtl8125b PAUSE frames blasting when
 suspended
Content-Language: en-US
To:     ChunHao Lin <hau@realtek.com>, <hkallweit1@gmail.com>
CC:     <nic_swsd@realtek.com>, <davem@davemloft.net>,
        <edumazet@google.com>, <kuba@kernel.org>, <pabeni@redhat.com>,
        <netdev@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
        <grundler@chromium.org>, <stable@vger.kernel.org>
References: <20231129155350.5843-1-hau@realtek.com>
From:   Jacob Keller <jacob.e.keller@intel.com>
In-Reply-To: <20231129155350.5843-1-hau@realtek.com>
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?MWlxc3pNS05vaEozcTgyME9QZzJHRzhYVytLNHdiUzczblYwV0t4MEh0M0dE?=
 =?utf-8?B?UjNOdnFQK1M5WDNXWjV5K2xIdG1RWDR5eTZnMUxyT09aTGtUYUpOR2tTWjFs?=
 =?utf-8?B?S0NDdGlwdjRYWXBFK0ZlRXJwOFE0YmpIcjRjV09GS3BPMzFaNXNTMTIva1Vw?=
 =?utf-8?B?VU5qYnBMeERvNVdCQzcyZ1dwT2YzblFKU3l5T0VTZ0JMVFd3WHBSWFgwMStq?=
 =?utf-8?B?M1YzamNLaDU0MUdlZlpReHBhWVE4STB4a2FJTEJDSERKdHN5TlpQMXpSY1VN?=
 =?utf-8?B?eERSaDluM0JFb1c4Ry9BTlRKbnAvaWRNTVhZOXNKUm1IdzYxd1VoSVpQU0JM?=
 =?utf-8?B?RWJ2b3p6Zk9JcGJlQmpGSnVEVHRUNFVtVlEvd1VQTHBmQzc0SzlQc20vR2xr?=
 =?utf-8?B?V0FySzExSkkyWlBZRjFEMW9sMkZtZk9MZG15YkVBZWlNRGJPNmhOeDY5UGll?=
 =?utf-8?B?aUE2ajVML2VYZG1adDBPU0liMU5RdjAvN1RESkQ3NEdnOWNPRkMrMTFmeGRy?=
 =?utf-8?B?WTl2TDVJK0IweEo4a1FhaTIzbEFQN0NCSi9zRDZBR2NsR0tjaVlUQ2krVzJP?=
 =?utf-8?B?MGE2aEtVV2U5MGFyZkt3cUU4ZEtnUlVxYTdPbUpBZHNobFVWTHFlVlRLQ1N1?=
 =?utf-8?B?NUF0NnN5dHd2VjM1KzhzNUlhckZwTVd3QVNrSW5JdnQyNjFzY2FpQTBiR2Rx?=
 =?utf-8?B?bmZYR2Nvc0lWN0hyZGZhNEdlcjNoUG9tVmJrUkk0S00zU3NoY3lwb0RwSmR1?=
 =?utf-8?B?TWdKQ2poZlNDbVVaZENTa24zWU01STJnbk5WblVlRyt1YVZIOXNRZ0FVTStT?=
 =?utf-8?B?OTcrMGVUd1V4NjRTUUZ1bFFRV2RHeUphREMyK2NWd3l4K09XRUxmbkV1RjVJ?=
 =?utf-8?B?ZGROblhXaG55QkptNUlCOVp2WVF1SEZvcTFzZWk0N0hEcGJzRGpDY3VYWVB5?=
 =?utf-8?B?RGdrb09Mci84TUtRQ01UczdPcURmUzhSM1FFT3RUWktQNXlHd0JuVmZSc2tn?=
 =?utf-8?B?MmpjVk52ZEpWTy9YZlNQcjZkWHE0UisxS0xHeHA4ZFl3MG4ycHMweVR3WGV4?=
 =?utf-8?B?Y3RXamZVT1dwemFmMDdMRk1GMUo0eTkyQm5TU0l4TDN4akFUeTRiU1hmUHdh?=
 =?utf-8?B?YU9SUms2OEx6VVE4cysyczhGTXZ6NjYvTGI4MDVhcnVBc2IzMWVVcmh2WXlj?=
 =?utf-8?B?STZ2ZGt2TGU5VWVoOWNTeCtDTDI1aVloZlFQSHlxcVRZcFowdU95UDBUMUxH?=
 =?utf-8?B?NEVRaGJCSkRWemxnSkYwR2lnSFVvWEE2dUhyTVdHdmxoeWMybmNnVlFjYWEw?=
 =?utf-8?B?dk1XVnBFQWluQktrS2tVTFRMd1VpQUNuZm9iWDBnVmY0TkZUTy9ENStzbzlt?=
 =?utf-8?B?c0IvdHlZZG1TQU5tU2x6UHJaM3JlQ3Zqb2g0U0lzSGZBSTZmVGdpLzNRU1pD?=
 =?utf-8?B?aHFLL1cvVjZoekZLMzNTMG9uM09XdzRTa1BFbU54NUg1WHR6OTRzY1BRNUxQ?=
 =?utf-8?B?b2NWQWhzK1d3R3B5aXpCY3RJQ3VOaEdnTEIxd2E5WjZteHFyRWRrNE9DbVhC?=
 =?utf-8?B?S1JvYWtIYU92VFpKamlENTVNV2hGcFFLU3hQMUNNSUo0c1BVa2NuKzNqSDZ1?=
 =?utf-8?B?R1FhOGdDeXk5NkhWNGFlTC9oTVlHUmdGYkhLSG5jTEUrWWJmMFpzZU5qS3pJ?=
 =?utf-8?B?WVdCVGpkNTA4K1lFTThPV3dSaUt0NGlqY1FLdUpCdTFOa1JvNzVzT3Z2ekZX?=
 =?utf-8?B?cU9uRjJ3ZE9UMHliZ1Q2c0orblVDV1hEMHJaWG1ETS9FWUN4ZVRTeTRhY1pM?=
 =?utf-8?B?TzRhT2dBUHo3UWtSSzhpdHpUdE8yYkxCb3IydFFmcXlTa045MGlqT08zVVNE?=
 =?utf-8?B?SzFTK0xVc3R1Q0ZBZFhnb3ZUbkZXcnBpS1k1bFBwKys1VTRTRndnMVRkNGR0?=
 =?utf-8?B?cVpKQnkxbWVYMGErVGxnZXY4Q0xtZU9mV0FFekNDWXFFdHU3RUVIcUZaZEJr?=
 =?utf-8?B?S3hlV0dmZ2s2SWRodzMybDNnRDNoa1lDRnkzY2lDWi84dUtxSUcxeW1xbitE?=
 =?utf-8?B?VWFkRmh5OVVRbTYrVEhzMHdnTlJ4Qko4WWRjMjA1eGIwcUxJUUdvTktQMEVk?=
 =?utf-8?B?aFJQSVFmc1dvZ1NFQ0hLSlBTSE9yVEV3S1VVcVlxdXRtTWdHM3F4ZkhRQnJ4?=
 =?utf-8?B?OVE9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: ef9df4e5-afe4-48ed-e603-08dbf12f9be8
X-MS-Exchange-CrossTenant-AuthSource: CO1PR11MB5089.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 29 Nov 2023 23:04:58.5515
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: Z1wFKWK8Qc2buph5rRJZGiQH8Qh/mpJLW6dZmC24KVH9xVHBwBQRlfDI0uk7d3nqCvVV2mJlG720vKmYA2QHrXf0XnAllNXHLap+lmRCAaQ=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH8PR11MB7965
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-0.9 required=5.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED,
	DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,
	MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,T_SCC_BODY_TEXT_LINE
	autolearn=unavailable autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on groat.vger.email
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (groat.vger.email [0.0.0.0]); Wed, 29 Nov 2023 15:06:39 -0800 (PST)


On 11/29/2023 7:53 AM, ChunHao Lin wrote:
> When FIFO reaches near full state, device will issue pause frame.
> If pause slot is enabled(set to 1), in this time, device will issue
> pause frame only once. But if pause slot is disabled(set to 0), device
> will keep sending pause frames until FIFO reaches near empty state.
> 
> When pause slot is disabled, if there is no one to handle receive
> packets, device FIFO will reach near full state and keep sending
> pause frames. That will impact entire local area network.
> 
> This issue can be reproduced in Chromebox (not Chromebook) in
> developer mode running a test image (and v5.10 kernel):
> 1) ping -f $CHROMEBOX (from workstation on same local network)
> 2) run "powerd_dbus_suspend" from command line on the $CHROMEBOX
> 3) ping $ROUTER (wait until ping fails from workstation)
> 
> Takes about ~20-30 seconds after step 2 for the local network to
> stop working.
> 
> Fix this issue by enabling pause slot to only send pause frame once
> when FIFO reaches near full state.
> 

Makes sense. Avoiding the spam is good. The naming is a bit confusing
but I guess that comes from realtek datasheet?

> Fixes: f1bce4ad2f1c ("r8169: add support for RTL8125")
> Reported-by: Grant Grundler <grundler@chromium.org>
> Tested-by: Grant Grundler <grundler@chromium.org>
> Cc: stable@vger.kernel.org
> Signed-off-by: ChunHao Lin <hau@realtek.com>
> ---
> v2:
> - update comment and title.
> ---
>  drivers/net/ethernet/realtek/r8169_main.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
> index 62cabeeb842a..bb787a52bc75 100644
> --- a/drivers/net/ethernet/realtek/r8169_main.c
> +++ b/drivers/net/ethernet/realtek/r8169_main.c
> @@ -196,6 +196,7 @@ enum rtl_registers {
>  					/* No threshold before first PCI xfer */
>  #define	RX_FIFO_THRESH			(7 << RXCFG_FIFO_SHIFT)
>  #define	RX_EARLY_OFF			(1 << 11)
> +#define	RX_PAUSE_SLOT_ON		(1 << 11)	/* 8125b and later */

This confuses me though: RX_EARLY_OFF is (1 << 11) as well.. Is that
from a different set of devices? We're writing to the same register
RxConfig here I think in both cases?

Can you clarify if these are supposed to be the same bit?

>  #define	RXCFG_DMA_SHIFT			8
>  					/* Unlimited maximum PCI burst. */
>  #define	RX_DMA_BURST			(7 << RXCFG_DMA_SHIFT)
> @@ -2306,9 +2307,13 @@ static void rtl_init_rxcfg(struct rtl8169_private *tp)
>  	case RTL_GIGA_MAC_VER_40 ... RTL_GIGA_MAC_VER_53:
>  		RTL_W32(tp, RxConfig, RX128_INT_EN | RX_MULTI_EN | RX_DMA_BURST | RX_EARLY_OFF);
>  		break;
> -	case RTL_GIGA_MAC_VER_61 ... RTL_GIGA_MAC_VER_63:
> +	case RTL_GIGA_MAC_VER_61:
>  		RTL_W32(tp, RxConfig, RX_FETCH_DFLT_8125 | RX_DMA_BURST);
>  		break;

I assume there isn't a VER_62 between these?

> +	case RTL_GIGA_MAC_VER_63:
> +		RTL_W32(tp, RxConfig, RX_FETCH_DFLT_8125 | RX_DMA_BURST |
> +			RX_PAUSE_SLOT_ON);

We add RX_PAUSE_SLOT_ON now for RTL_GIGA_MAC_VER_63 in addition. Makes
sense.
> +		break;
>  	default:
>  		RTL_W32(tp, RxConfig, RX128_INT_EN | RX_DMA_BURST);
>  		break;