Re: [PATCH v1] PCI: pciehp: Make sure DPC trigger status is reset in PDC handler

[Lukas Wunner <lukas@wunner.de>]

[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@amd.com/

    (@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