Received-SPF: pass (google.com: domain of linux-kernel+bounces-91183-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Subject: Re: [PATCH pci-next] pci/edr: Ignore Surprise Down error on hot
 removal
To: Lukas Wunner <lukas@wunner.de>, Ethan Zhao <haifeng.zhao@linux.intel.com>
Cc: bhelgaas@google.com, ilpo.jarvinen@linux.intel.com,
 sathyanarayanan.kuppuswamy@linux.intel.com, linux-pci@vger.kernel.org,
 linux-kernel@vger.kernel.org, kbusch@kernel.org
References: <20240304090819.3812465-1-haifeng.zhao@linux.intel.com>
 <20240304115844.GA3541@wunner.de>
From: Smita Koralahalli <Smita.KoralahalliChannabasappa@amd.com>
Message-ID: <ff3918bc-ebd2-60e2-971f-5b48ba929335@amd.com>
Date: Mon, 4 Mar 2024 11:33:52 -0800
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101
 Thunderbird/78.4.0
In-Reply-To: <20240304115844.GA3541@wunner.de>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
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?ZE5aUWh4c0o5cEcrMGJlQnE5UGRkNkhpS3JUZVUrUW1PeFU5OFk1N0cwSVZ0?=
 =?utf-8?B?YlVaTm1tZXNsbUxraFEwRDVoWmdQWDFxdXdJMWhIL3lYVjVHVFRVVUVvTFFM?=
 =?utf-8?B?ZWMzck5mTUgwRnlsMkN5RG56UEkxbDVzWldVM0xnT3hiWE1aWHFNSGVmN0py?=
 =?utf-8?B?TUc5dEJHcHFhZEgyYTRhSDVrSW1wclA2dXVVWXVTeFI5bzlIWGhzMjYrMjNJ?=
 =?utf-8?B?V1dDdGlKM2FMR09TNjZ0U1lhQXpHUWV1cktTTnI5RjN4LzVqbWFwbTBqS2J1?=
 =?utf-8?B?dkNLZm4wQ1pCQzg1Zm04bitGeksrTjJzV1pESXY4b0dIK2xsZWhxSDdjRWVU?=
 =?utf-8?B?NCtBT1JubHYwMW1DLzBTdUQ5VGtMYnNZN2JKSWJCRWNsVHlwOE9BRjd5NmJj?=
 =?utf-8?B?Z2dJRjRPZjJQWFo4UjVoQlZhUlV5QVV1VDlWS1VkUzg0am5yV09yZTRPMW13?=
 =?utf-8?B?ZmVlSktCdkVEZDE2UDRhWmVyTnhPU1NYM1dmZWhNRGxEdGord3ZYZ1VoUHl6?=
 =?utf-8?B?YlhWWUVZN25XRWduMTRmckFNYitDSW1QeTdLMnhXVjBIQXNlc3UvQzl5NGxr?=
 =?utf-8?B?UlBmL1ZTMU5iUVV0ODlqUkVPbExmYS9lV1c5dXQyS1VXNmdINzdnWVoxSUE5?=
 =?utf-8?B?cmNlQ1BOU1ZVdzJpTzFTTk93dUw5czlicGVXUUpHSEEyQ1FRQk1DQi9BNGZl?=
 =?utf-8?B?VzQ3R1BPVmpkcEZleGlhNTNiS1Nqbm5LWWE1Rkt2RE1ab1d6Sis4WGJudkVO?=
 =?utf-8?B?VlRwajVIUVk3amJwZ3VRcFNRQlRRV1U5dWNWMTFVWFdCV1RYeXJqVlVPR1VN?=
 =?utf-8?B?aDF4MUZFeU5qTWk4WnJtQTgvc3ltQ09YWlBySkRiRy9FWURyeTJqbVQyZUxP?=
 =?utf-8?B?citPQVRLOFBXcHFQRndrcUhyc09pNkR1VFJwSzk4SnJQWXFEc2t2d2dQSmhH?=
 =?utf-8?B?Q3pDNnZkMzNsK3g0WHVnaG5rZzZRK04xNStRZitEVWtRYkptUU1JNys2TGYx?=
 =?utf-8?B?OWFBbU1ob0lQUTJ2c3F2L2dxVTkrOUNXRDNRS1VHT3pDTXFUcVBjeGhMRlFN?=
 =?utf-8?B?YmpOQzJPd3piOG4yaFpUNmVXc1hyZUk3dmRybjJ1UGp0YWhuekVJbGJGcDlV?=
 =?utf-8?B?UDZuZnJJR1NYaEtqNnFNNk5zODZFQ0V0blpGZGFMajlDUEQ2QmR6Y3JobTd5?=
 =?utf-8?B?eHBWOHVRWVR0QXVna3hYUE9SMjVsZThxcWE5WlFiblo4dHNFMkJTaEpNZVlF?=
 =?utf-8?B?dGxlNmZsem5OZDgzNFVLRUlyUmlVV1BmVlRQUGJsS0VwVEtOMGt5VnBKWkFP?=
 =?utf-8?B?YVkyRnc1NEVlRmhyWG1wWHM3bHJqUHRXSzVPU2w3S0xNM25zZGFpYytGK2Zm?=
 =?utf-8?B?eDJvUHJ6ekhRanE4MnBYUnIxRmxuZUxnVjRXeGd3MmY3ZVFvSnl1bDh6WVZu?=
 =?utf-8?B?T1FQbE9SeXYzZUZkbTJ6b3V1ci85OG5FUzNFbU9iandPZU1RMkNiZVo5dFFJ?=
 =?utf-8?B?eGhJWU9Fcy85d1ZYcDMxQ0I2MG85aHhZQVI2VnJURERIK3VZR1pwNjJUU3pT?=
 =?utf-8?B?NXYzZXVKa09TMzlMRzdXb09CRS9lUStIeGJjVi9ocEg2R2tHUjY4NW0yNHYx?=
 =?utf-8?B?S3RnQXQweVl3RmxUMmpBWTd4Tm1JdmJ0RFQvdEE2VkJhOU8zNHptU2NLOTVL?=
 =?utf-8?B?aDA2L2RkSUpRWHNNYmg3TE9YR3BVWm0zU0pwZkYrRXpuWlYxMlVWZURwb3NI?=
 =?utf-8?B?N0JBOTdMWXZVcUNxeFpkdXVOdWs0NU1CaktlM2xuYk8yQVREMDRsak1YMnho?=
 =?utf-8?B?OERiejJjaWgvR1N1alNGR3BJaGJLSk9zbjFIQWRabjZwVWNaTVlkUnVzVDdZ?=
 =?utf-8?B?L21UUUJhWGducDE0MHdYNGJiN1RTcnBWNmdKSm9WdzhEQUFYTVdEM2pPamZl?=
 =?utf-8?B?TmI4aEhjVEFscUlsTXU3NU81bUxib1BvSll5M0gwclQxOUJaNVpnTWhESTVv?=
 =?utf-8?B?RHJONThWQ0hET0lDZVhoWkRkL1drVWF3bk96MzJKUlNwRkhITzFVNENSa25z?=
 =?utf-8?B?dk1wbTN4VS9yN3NTWUptb2UxVktweS9WbDBLOEQ5SWFtZEtRU0luOUYycm9H?=
 =?utf-8?B?TXFZS1lDV2RXbTdVUE9KcjVXSHZSZVBQRDZLV1g3WUVmVVN2SkIzUk5pNGV3?=
 =?utf-8?Q?Gel13tlbbgxgZrwk7+excc+Y2Wa33oftYeUvQ6CjWQ8h?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: a4b369e0-f76b-4eb2-b121-08dc3c82043f
X-MS-Exchange-CrossTenant-AuthSource: BYAPR12MB2869.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 04 Mar 2024 19:33:49.6337
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: mq8BaoFKBz9uhMzx1mpFCFOoIr94M+DSFXHXQPKD5sbN5HdR6lqLoZAZoq/O75ptUKTb5f+4zdq6Z39FPtT3wA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH7PR12MB7209

Hi Ethan,

On 3/4/2024 3:58 AM, Lukas Wunner wrote:
> On Mon, Mar 04, 2024 at 04:08:19AM -0500, Ethan Zhao wrote:
>> Per PCI firmware spec r3.3 sec 4.6.12, for firmware first mode DPC
>> handling path, FW should clear UC errors logged by port and bring link
>> out of DPC, but because of ambiguity of wording in the spec, some BIOSes
>> doesn't clear the surprise down error and the error bits in pci status,
>> still notify OS to handle it. thus following trick is needed in EDR when
>> double reporting (hot removal interrupt && dpc notification) is hit.

Please correct me if I'm wrong.

When there is double reporting (hot removal interrupt && dpc 
notification), won't the DPC handler be called always which takes care 
of clearing the surprise down errors? Do we need it again from EDR handler?

Thanks
Smita

> 
> Please provide more detailed information about the hardware and BIOS
> affected by this.
> 
> 
>> -static void dpc_handle_surprise_removal(struct pci_dev *pdev)
>> +bool  dpc_handle_surprise_removal(struct pci_dev *pdev)
>>   {
>> +	if (!dpc_is_surprise_removal(pdev))
>> +		return false;
> 
> This change of moving dpc_is_surprise_removal() into
> dpc_handle_surprise_removal() seems unrelated to the problem at hand.
> 
> Please drop it if it's unnecessary to fix the issue.
> 
> 
>> --- a/drivers/pci/pcie/edr.c
>> +++ b/drivers/pci/pcie/edr.c
>> @@ -184,6 +184,9 @@ static void edr_handle_event(acpi_handle handle, u32 event, void *data)
>>   		goto send_ost;
>>   	}
>>   
>> +	if (dpc_handle_surprise_removal(edev))
>> +		goto send_ost;
>> +
>>   	dpc_process_error(edev);
>>   	pci_aer_raw_clear_status(edev);
> 
> This seems to be the only necessary change.  Please reduce the
> patch to contain only it and no other refactoring.
> 
> Please capitalize the "PCI/EDR: " prefix in the subject and add
> a Fixes tag.
> 
> Thanks,
> 
> Lukas
>