Received-SPF: pass (google.com: domain of linux-kernel+bounces-192128-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Message-ID: <43e74464-029d-431f-93e2-809d64371c4c@intel.com>
Date: Tue, 28 May 2024 12:43:05 +0200
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] ice: irdma hardware init failed after suspend/resume
To: Ricky Wu <en-wei.wu@canonical.com>, <jesse.brandeburg@intel.com>
CC: <anthony.l.nguyen@intel.com>, <davem@davemloft.net>,
	<edumazet@google.com>, <kuba@kernel.org>, <pabeni@redhat.com>,
	<intel-wired-lan@lists.osuosl.org>, <netdev@vger.kernel.org>,
	<linux-kernel@vger.kernel.org>, <rickywu0421@gmail.com>, Michal Swiatkowski
	<michal.swiatkowski@linux.intel.com>
References: <20240528100315.24290-1-en-wei.wu@canonical.com>
Content-Language: en-US
From: Wojciech Drewek <wojciech.drewek@intel.com>
In-Reply-To: <20240528100315.24290-1-en-wei.wu@canonical.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?cHFDZkhVZmh6QzFyZThHai9QUWlRZFZqSXl3V3JzT1lEOHh6T1RkdWlBUnJv?=
 =?utf-8?B?a0lGUnJFUnpaMnJ2VjF4U0l2akVYWDRKRFVJUk9wYjZlazFhZjV4M1l5R1ZH?=
 =?utf-8?B?b2xrUjFrQ1Z3STYxVzd5UldsbnFoQ3QxeGF3dUpRMHJ2cllDbVQwVXd3NTJw?=
 =?utf-8?B?UjRuaHdaZWJ6cnNXbUFlMlVXMkxidFkzcXBWald3RXlwQk82RjZXTUVIWVUx?=
 =?utf-8?B?ZGkvMHlNSFFBeWEwWUNiL21OM0VNdzJ6YmNMWS9DSlU3VGJic3A4VFBoNy9h?=
 =?utf-8?B?RmNtMSt6eWkySVE4WWtJR1Azb2VJWFhscWlrNjZYcFpWb2U0c2tWVStRKzBJ?=
 =?utf-8?B?aUlrcGtKL3ZWOXFQWFExKzlvWFJoSnR0VEJ3bDRaNkxhNVZjZUh0dVZzME9M?=
 =?utf-8?B?dzVSQlVjLzUyNXk0ellGUmc4WlpReDFsVWlpeWFtZzFwWXhKdU9yUVFzOTFI?=
 =?utf-8?B?TDJMSXZkRnIzdC8xU1ZrNXJ1bWdlOGNkM0gvU3pWaUR3OENNRlFCckxnWFFk?=
 =?utf-8?B?SkFHWVBFRk9GY2UwdFF6SG5DM1pWQmJkSUdGekdIc3pYcndLR01mVDRXVWNj?=
 =?utf-8?B?MnhlRXRBd25qcHRtbm5nV3RzMmhrVmdNSHZLQkZKbjVmbTdxWm1kVjV2YjFO?=
 =?utf-8?B?NWh5ZEZhWmZ3cnZhOVl1amUxaWZxVExMNWZJZ2djRmhtODNoVzF2TERVcHFR?=
 =?utf-8?B?S2NMSlhiMnVaYVppMGZjdkVjZVNNa3lNS2JzdkpIUzJlWTUzd051MHpDaUNv?=
 =?utf-8?B?aU94cFR2UWZzbGNhaDBRbUk4TG53VWN2OC9xZDZXWmQ5UUR2WjFIZFk3Tncx?=
 =?utf-8?B?TEN6Qkt4dWY3QXluUzlVMkpmanVBRklkMEdGTXpmOUZyYlRtQ2ZuMERhc3Fh?=
 =?utf-8?B?ZWhCTlpzMWc4TFZKUDd1dmVxTVJpNXNZbzFkeVdORXVtckMrVFNYa01uTmkr?=
 =?utf-8?B?WklDTE9PbFErOUtqSWM4andqZ1JuTzg4cXYzV3ROTzVCMFBmTEtRQUpZUGdL?=
 =?utf-8?B?MElUdS9uMW1mYWt3bUxSWW9qTW1wcmV3dFgyQURuT25xS3dFQkxDcDUwdVZG?=
 =?utf-8?B?bk1WSDB2ZmNPNjhZdHZqMHlNZG11Qm5Geno5R2V6RGJ0VEJXY0N4TFhlQXgw?=
 =?utf-8?B?ZURFU2g5RGFFdCtiR2g4OWQ1cmFpSG9Cdlk4elRRbU1pa0wvNHVaQXJtcHZO?=
 =?utf-8?B?SUpjTE1XeHJqSHFTVWZCR0VHS21hTnhtMUwrVGNoeFhjZ0U1eWpqcGJMY2dG?=
 =?utf-8?B?WTZSeFQ5YnNaSjh6UTR5MnBNV3NWNlZsYVdLenVKOHlKUWFiek8wUWIxZXB0?=
 =?utf-8?B?S0o1TlQ2ODl5UHlzbFlUSnR2SmZna2cycERvQStldEpNc0MzMHlaSmtxUzBj?=
 =?utf-8?B?bVprL0ZSNVR6RXNIR3p0cWk5aDFlbnFmbjhkMmZmU3Z6aGpKblppQUxTL29x?=
 =?utf-8?B?eEQ4NWZRYlZ6eGdJM0dlV0ZLQmEwc2ZKejZNTlh6cTdLZ0RjOHJnVXYzbUJX?=
 =?utf-8?B?bWt5czBSeUY0bkQzandwK3dXN0UrVkYwZ3JRbkV0aDBvVFhXYXM3S1NsSi9Y?=
 =?utf-8?B?eHpPdjNSOWtjOTQ0emY5cUMvZllPNWVrM0tHR2FpOUNzd1pJWGhwSUI2dEtH?=
 =?utf-8?B?bXRsWTBaK09rNWprU1lEZVBtY0V1QnhGWk9WbGh5dXNIRHBta2RnVjRnZTN4?=
 =?utf-8?B?c1E0a1ptZkpIL1lSV3o4SHNFeUZyUzIrL1pNVnc0RVZPcEJFanZSMVMxNEFP?=
 =?utf-8?B?UG9XTEtwWVZHWkhHQ2RabFZyYStXT0tvVTdWd1RKRThCRmwzcVFZdi8yMFpB?=
 =?utf-8?B?VUpFQmlydmZlbDBHckNsTjNrZkRnajJpUStaQUlZWjYzTG5IQ3hhMU9zWkMv?=
 =?utf-8?B?ejFIQmtiVFVuSENzRjN5ZGtubWxCYW5nUTZycFNkTjNyTndVVG1zTjMyRXNV?=
 =?utf-8?B?ckFVK2dlWlJOZ2hJL3R3M0hkcFBDdzFiVWtiN2NLNUVndElhY3pjazFRZ1o0?=
 =?utf-8?B?ZXBhR0gyVXZiR0w2VFlNSG13Y0NyMXVCU0R3K0pIcFlYQzNQN3BUTGJySlZW?=
 =?utf-8?B?cGp0cThaanVMUUFTTTBCM0VBVnd6ajBjTEw3eGVEa2ZCdFpXOCs2WFFtdHNP?=
 =?utf-8?B?U3p0RHhGbmUyb3hPMUloQ0lvSStibzZLQXAvN3dIWElNeURvVzkvbWpwM0pn?=
 =?utf-8?B?Q3c9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: ab54a552-9c38-4a7f-22c9-08dc7f02f848
X-MS-Exchange-CrossTenant-AuthSource: MW4PR11MB5776.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 28 May 2024 10:43:11.4358
 (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: n2Ok18jz/82tZobyMhR4yBjdAwecNBxJlfavw0CSygeu94fiI26Uqm/XjC+z15GT+Vx0wHTVlwxk6L8lS2B6OlHeHSXq3VeS2wTLtcOGgUE=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ0PR11MB5183
X-OriginatorOrg: intel.com


On 28.05.2024 12:03, Ricky Wu wrote:
> A bug in https://bugzilla.kernel.org/show_bug.cgi?id=218906 describes
> that irdma would break and report hardware initialization failed after
> suspend/resume with Intel E810 NIC (tested on 6.9.0-rc5).
> 
> The problem is caused due to the collision between the irq numbers
> requested in irdma and the irq numbers requested in other drivers
> after suspend/resume.
> 
> The irq numbers used by irdma are derived from ice's ice_pf->msix_entries
> which stores mappings between MSI-X index and Linux interrupt number.
> It's supposed to be cleaned up when suspend and rebuilt in resume but
> it's not, causing irdma using the old irq numbers stored in the old
> ice_pf->msix_entries to request_irq() when resume. And eventually
> collide with other drivers.
> 
> This patch fixes this problem. On suspend, we call ice_deinit_rdma() to
> clean up the ice_pf->msix_entries (and free the MSI-X vectors used by
> irdma if we've dynamically allocated them). On Resume, we call
> ice_init_rdma() to rebuild the ice_pf->msix_entries (and allocate the
> MSI-X vectors if we would like to dynamically allocate them).
> 
> Signed-off-by: Ricky Wu <en-wei.wu@canonical.com>
> ---

Thanks for the patch!

>  drivers/net/ethernet/intel/ice/ice_main.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
> index f60c022f7960..ec3cbadaa162 100644
> --- a/drivers/net/ethernet/intel/ice/ice_main.c
> +++ b/drivers/net/ethernet/intel/ice/ice_main.c
> @@ -5544,7 +5544,7 @@ static int ice_suspend(struct device *dev)
>  	 */
>  	disabled = ice_service_task_stop(pf);
>  
> -	ice_unplug_aux_dev(pf);
> +	ice_deinit_rdma(pf);

I think this should be called later in the reset path IMO.
You should call ice_deinit_rdma in ice_prepare_for_reset (replace ice_unplug_aux_dev),

>  
>  	/* Already suspended?, then there is nothing to do */
>  	if (test_and_set_bit(ICE_SUSPENDED, pf->state)) {
> @@ -5624,6 +5624,10 @@ static int ice_resume(struct device *dev)
>  	if (ret)
>  		dev_err(dev, "Cannot restore interrupt scheme: %d\n", ret);
>  
> +	ret = ice_init_rdma(pf);
> +	if (ret)
> +		dev_err(dev, "Reinitialize RDMA during resume failed: %d\n", ret);

And ice_init_rdma should be moved to ice_rebuild (replace ice_plug_aux_dev)

> +
>  	clear_bit(ICE_DOWN, pf->state);
>  	/* Now perform PF reset and rebuild */
>  	reset_type = ICE_RESET_PFR;