[cc += Smita]
On Thu, Jun 15, 2023 at 04:03:54PM -0700, Sathyanarayanan Kuppuswamy wrote:
> On 6/15/23 11:35 AM, Lukas Wunner wrote:
> > On Wed, Jun 14, 2023 at 11:25:59PM -0700, Kuppuswamy Sathyanarayanan wrote:
> > > During the EDR-based DPC recovery process, for devices with persistent
> > > issues, the firmware may choose not to handle the DPC error and leave
> > > the port in DPC triggered state. In such scenarios, if the user
> > > replaces the faulty device with a new one, the OS is expected to clear
> > > the DPC trigger status in the hotplug error handler to enable the new
> > > device enumeration.
[...]
> >
> > pciehp_unconfigure_device() seems like a more appropriate place to me.
> >
>
> I initially thought to add it there. Spec also recommends clearing it
> when removing the device. But I wasn't sure if pciehp_unconfigure_device()
> would be called only during device removal.
It is.
> > > More details about this issue can be found in PCIe
> > > firmware specification, r3.3, sec titled "DPC Event Handling"
> > > Implementation note.
> >
> > That Implementation Note contains a lot of text and a fairly complex
> > flow chart. If you could point to specific paragraphs or numbers in
> > the Implementation Note that would make life easier for a reviewer
> > to make the connection between your code and the spec.
>
> It is the text at the end of the flowchart. Copied it here for reference.
>
> For devices with persistent errors, a port may be kept in the DPC triggered
> state (disabled) to keep those devices from continuing to generate errors.
> For hot-plug slots, the errant device may be removed and replaced with a new
> device.
> If the DPC trigger state is not cleared, then the port above the newly
> inserted device will still be disabled and will be non-operational.
> Therefore, operating systems may need to modify their hot-plug interrupt
> handling code to clear DPC Trigger Status when a device is removed so that
> a subsequent insertion will succeed.
Please add that excerpt to the commit message.
> > This may run concurrently to dpc_reset_link(), so I'd expect that
> > you need some kind of serialization. What happens if pciehp clears
> > trigger status behind the DPC driver's back while it is handling an
> > error?
>
> Currently, we only call pci_dpc_reset_trigger() in PDC interrupt handler.
>
> Do you think there would be a race between error handler and PDC handler?
Yes I think so.
We need to differentiate between two cases:
(1) DPC handled by firmware, hotplug handled by OS:
In this case clearing DPC trigger status from pciehp device removal
code path seems reasonable. But it must be constrained to
!host_bridge->native_dpc.
(2) DPC handled by OS:
In this case clearing DPC trigger status from pciehp could race with
the dpc interrupt handler so must not be done. Instead, I recommend
clearing trigger status from the dpc interrupt handler. You should
see a Surprise Down error handled by the dpc interrupt handler.
Make sure DPC trigger status is *always* cleared in that case.
Note that Smita Koralahalli is currently working on something similar:
https://lore.kernel.org/linux-pci/20230418210526.36514-2-Smita.KoralahalliChannabasappa@xxxxxxx/
(@Smita sorry for the delay, I'll get to your patches ASAP.)
I recommend splitting the two cases above into two commits, one for
firmware-handled DPC and one for OS-native DPC. IIUC, you only need
the former to address Dell's finding.
Thanks,
Lukas
