Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.36 as permitted sender) client-ip=23.128.96.36;
Message-ID: <a73a2156-124b-47f7-b545-6751f6e87d54@nvidia.com>
Date:   Mon, 27 Nov 2023 20:13:50 +0530
User-Agent: Mozilla Thunderbird
Subject: Re: Race b/w pciehp and FirmwareFirst DPC->EDR flow - Reg
From:   Vidya Sagar <vidyas@nvidia.com>
To:     Bjorn Helgaas <helgaas@kernel.org>,
        Lorenzo Pieralisi <lpieralisi@kernel.org>,
        Lukas Wunner <lukas@wunner.de>,
        Sathyanarayanan Kuppuswamy 
        <sathyanarayanan.kuppuswamy@linux.intel.com>,
        "kbusch@kernel.org" <kbusch@kernel.org>
Cc:     Vikram Sethi <vsethi@nvidia.com>,
        Krishna Thota <kthota@nvidia.com>,
        "linux-pci@vger.kernel.org" <linux-pci@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        sagar.tv@gmail.com
References: <BN8PR12MB290002441CA3C24D1FA742D2B8AEA@BN8PR12MB2900.namprd12.prod.outlook.com>
 <529acc15-1932-4785-9edf-c5327db64ab1@nvidia.com>
Content-Language: en-US
In-Reply-To: <529acc15-1932-4785-9edf-c5327db64ab1@nvidia.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?dzNnQU5ibVhYSVNwWmlLcjFyY2pRdWl3TXY4Ly9jcytIaWNvM2xMbEJpSkJ6?=
 =?utf-8?B?ZDBBNkI5YXRjbExLUEJzZHUrcHJVZDFvVysrN2wxVFlKdmJyTnNiRERzbVMv?=
 =?utf-8?B?YnVsWldKQmJCNTY2WnNwdk11TUpKK2NtUDc4ZVFZTmVBL3JNNTF6WFhMdTkw?=
 =?utf-8?B?aDd5WVBNdjkwTFdpaW1RenNVZjVraXNtZjZZOENudVU5Z0orMVowQXFGVGZP?=
 =?utf-8?B?eGhMZC9hMGxxN3Z3MkMxa0l4N29yN2dXL3NqUWxlaVBJRWxLUUNkd2VIc0FM?=
 =?utf-8?B?TS9ocWZLTUJzRVJxd2pNOFV3c2lhOGMzaW54OGhiVk96RHQ4Q241bVBQWkpw?=
 =?utf-8?B?a3dZbStYMHEvOXJOaVN1U2cvTTg1T1ZxMGI4VmxuSjRwVkRYMnpLekRLZWUv?=
 =?utf-8?B?MUJHZitkMC9idFgxdG9WOXhmdDdhNW8rMGFtQjZNaUlYZW91UTNNL01yU0Vu?=
 =?utf-8?B?ajhzd3dycGQza20wT1J0MUd1M256V0NkUlVxQXZ2Rm9nN0RBbUsrTHRzcHVs?=
 =?utf-8?B?a1V6M1BxNWJONTEzQk01VlJEKzNRU3hiUUZEQ09xNk1mSHErNExTb2lMTmdm?=
 =?utf-8?B?VEI0RGY5MXJESUs0dnlqY2NCRDNycGMvZkF0RlI2aWhYME1zN1JxV2dpbmRy?=
 =?utf-8?B?RGtsUGhUWlBPTHJOR1dncVdSc0Y3di9KYXM2M1FLUm8zWktGbnRYZTRHOG8w?=
 =?utf-8?B?eGJrcGV3Yk5mb3puYjRMMWdJSndxd3AyY2RJLzdvenE1VEw1VzdNU0VHUXNH?=
 =?utf-8?B?SjZkN2d2aWxHSHZVbytTT25JcDdYYVFGc2tpenYvQ2NDWGxpNnduc0pIYXg5?=
 =?utf-8?B?QVFValdXOE1Zd0RpUC9GVlpEeHNYa2hjYnMyRG8xZFArK3dNY0c1NXQ0Wjc5?=
 =?utf-8?B?Mi9JUjRSOFBjYUl2MCtDeGNkbHZWbHI2Mk0rMytwR3dRU0k4bCtQM3dKK1hN?=
 =?utf-8?B?Mjh3YW1jRzRUMXBqTmtZT2tUY1VrK1U0NG9kdTJ4SStiQ3VId1ZUSjhyQmFq?=
 =?utf-8?B?SnRaWXRodVBRLzhZZ2F1OC9LV0tSR0FIUDZJbUpPM01mMGZuM0ZkT3B4VUJz?=
 =?utf-8?B?aHgzWFZHUGMrdy9VOEpDeUpRalhucS9tZUhJTk1SdGNrZ2FjVkVmOEpOV2k3?=
 =?utf-8?B?RGQ2aTdYdjd6TC9ZT1VvZTB2T25EK0ZucGd2N1J4VWlaR0EyS3FJSXZVNkd5?=
 =?utf-8?B?eDd5RWNPaFV1dUxkL01nQXFlS3g5dzZGOFltMXl2WmJkVXJrOTFsZ01JWjAw?=
 =?utf-8?B?SWZaY3pOWGsrS0V1dGtScXg1WW9yOVYzcjQzeEZ4N3hFWTVzOEkvZjlXMGVT?=
 =?utf-8?B?SDJ3eG0zOHJ0Ukd3S2hGRGh5ZjkzMmJMRXpoNXRRZnBKRVc1VDdmcnl3eWp6?=
 =?utf-8?B?YUt0NjI1UHhabkExVUE4N2g0endQeXNneU56UzZzdWhnQitvUTBHYnlscVVN?=
 =?utf-8?B?VjMzUlVzUVl5VTVvdVNReU5VRDdKbWZwcUFsZ3BORGJpM0lmSDIxTk52T0lU?=
 =?utf-8?B?M2hNcGdHVGRtSWFtRXpDdS9vK3A4NHZrN0Q1cTlkdXRldk5PaDlmSmhDV1V1?=
 =?utf-8?B?cGZFUEFCOWVPSUFxbEZaRDE1dHFXZVhwVVRZbTI5ZzZVUytKeUp3YnBOTEFP?=
 =?utf-8?B?WnJRbDFnM0g1UDMzMlBGcFZRMFQwc2ZMY2YxK2daOTZqNkJaaHdzK2sxcDk5?=
 =?utf-8?B?T1ZHVXVTS21pUDBleVR6alZrVkFZM3ZzVFRrTExVVTdiNWVKOU5oK2l2aVc3?=
 =?utf-8?B?dU82V3JJRUxUQVNES2FkdUJHZmluN1UzUGlLbzNzcUt0UGNIWDdhbk9kcnJC?=
 =?utf-8?B?VVlnVEpFZmFpQVRaczJxd2IraThFVFlYTHhDQlV5Z2sxVXY3MTVWeTBROHJy?=
 =?utf-8?B?dlJLN2dHNjFQb2d0eUNEd1V5ZFJETDlHa0RQKzQwOVVkcjJLdFJTYklxVlhx?=
 =?utf-8?B?aTR1a3RiME9hdVA5OVo4a3JQSklLdkRTQUV5MkJwdENyZVJ5ZjJ1bEZEMWtz?=
 =?utf-8?B?YUoyZi9EaXBNcCtWWDBNT2g0aDQ4YVJBNlBkWjNRTGUvWEk2OFJJK2tZRXlC?=
 =?utf-8?B?RXcyRlo5RFdWdVUyanJManBkdE51TklteDdEa08yOFRLVDNjbXcwMTJkQ1JP?=
 =?utf-8?Q?JRiC3vL9rIdpAAQwL0r+bRdHt?=
X-OriginatorOrg: Nvidia.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 7c643172-66da-4d73-6e4e-08dbef574a85
X-MS-Exchange-CrossTenant-AuthSource: BN8PR12MB2900.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 27 Nov 2023 14:43:59.8401
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 43083d15-7273-40c1-b7db-39efd9ccc17a
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: oF8RsT/sqbv0bwc6v+jQaL8xsNTqJf0fckeJPTRp5Bc81PraQiPixS11NrFg88zqtjc4Bv38xcDDbZCjArM01g==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: IA0PR12MB8696
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 pete.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 (pete.vger.email [0.0.0.0]); Mon, 27 Nov 2023 06:44:16 -0800 (PST)

Hi Bjorn and others,
Could you please share your thoughts on this?

Thanks,
Vidya Sagar

On 11/10/2023 10:31 PM, Vidya Sagar wrote:
> There was a typo. Corrected it.
> 
> s/If DPC->EDR flow checks DPC also/If DPC->EDR flow checks 'PD Change' also
> 
> On 11/10/2023 6:30 PM, Vidya Sagar wrote:
>> Hi folks,
>> Here are the platform details.
>> - System with a Firmware First approach for both AER and DPC
>> - CPERs are sent to the OS for the AER events
>> - DPC is notified to the OS through EDR mechanism
>> - System doesn't have support for in-band PD and supports only OOB PD 
>> where writing to a private register would set the PD state
>> - System has this design where PD state gets cleared whenever there is 
>> a link down (without even writing to the private register)
>>
>> In this system, if the root port has to experience a DPC because of 
>> any right reasons (say SW trigger of DPC for time being), I'm 
>> observing the following flow which is causing a race.
>> 1. Since DPC brings the link down, there is a DLLSC and an MSI is sent 
>> to the OS hence PCIe HP ISR is invoked.
>> 2. ISR then takes stock of the Slot_Status register to see what all 
>> events caused the MSI generation.
>> 3. It sees both DLLSC and PDC bits getting set.
>> 4. PDC is set because of the aforementioned hardware design where for 
>> every link down, PD state gets cleared and since this is considered as 
>> a change in the PD state, PDC also gets set.
>> 5. PCIe HP ISR flow transitions to the PCIe HP IST (interrupt 
>> thread/bottom half) and waits for the DPC_EDR to complete (because 
>> DLLSC is one of the events)
>> 6. Parallel to the PCIe HP ISR/IST, DPC interrupt is raised to the 
>> firmware and that would then send an EDR event to the OS. Firmware 
>> also sets the PD state to '1' before that, as the endpoint device is 
>> still available.
>> 7. Firmware programming of the private register to set the PD state 
>> raises second interrupt and PCIe HP ISR takes stock of the events and 
>> this time it is only the PDC and not DLLSC. ISR sends a wake to the 
>> IST, but since there is an IST in progress already, nothing much 
>> happens at this point.
>> 8. Once the DPC is completed and link comes back up again, DPC 
>> framework asks the endpoint drivers to handle it by calling the 
>> 'pci_error_handlers' callabacks registered by the endpoint device 
>> drivers.
>> 9. The PCIe HP IST (of the very first MSI) resumes after receiving the 
>> completion from the DPC framework saying that DPC recovery is successful.
>> 10. Since PDC (Presence Detect Change) bit is also set for the first 
>> interrupt, IST attempts to remove the devices (as part of 
>> pciehp_handle_presence_or_link_change())
>>
>> At this point, there is a race between the device driver that is 
>> trying to work with the device (through pci_error_handlers callback) 
>> and the IST that is trying to remove the device.
>> To be fair to pciehp_handle_presence_or_link_change(), after removing 
>> the devices, it checks for the link-up/PD being '1' and scans the 
>> devices again if the device is still available. But unfortunately, IST 
>> is deadlocked (with the device driver) while removing the devices 
>> itself and won't go to the next step.
>>
>> The rationale given in the form of a comment in 
>> pciehp_handle_presence_or_link_change() for removing the devices first 
>> (without checking PD/link-up) and adding them back after checking 
>> link-up/PD is that, since there is a change in PD, the new link-up 
>> need not be with the same old device rather it could be with a 
>> different device. So, it justifies in removing the existing devices 
>> and then scanning for new ones. But this is causing deadlock with the 
>> already initiated DPC recovery process.
>>
>> I see two ways to avoid the deadlock in this scenario.
>> 1. When PCIe HP IST is looking at both DLLSC and PDC, why should 
>> DPC->EDR flow look at only DLLSC and not DPC? If DPC->EDR flow checks 
>> 'PD Change' also (along with DLL) and declares that the DPC recovery 
>> can't happen (as there could be a change of device) hence returning 
>> DPC recovery failure, then, the device driver's pci_error_handlers 
>> callbacks won't be called and there won't be any deadlock.
>> 2. Check for the possibility of a common lock so that PCIe HP IST and 
>> device driver's pci_error_handlers callbacks don't race.
>>
>> Let me know your comments on this.
>>
>> Thanks,
>> Vidya Sagar