Re: Race b/w pciehp and FirmwareFirst DPC->EDR flow - Reg

[Vidya Sagar]

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