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

Sathyanarayanan Kuppuswamy
2023-06-16T23:27:40Z

Hi Lukas,

On 6/16/23 2:06 AM, Lukas Wunner wrote:
> [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.

Do you know how pciehp_unconfigure_device() will be called when the device is
removed? Is it due to a DLLSC event or a PDC state change? If it is DLLSC,
pciehp_unconfigure_device() may not be called because we ignore the DLLSC event
if there is an active DPC trigger.

>
>
>>>> 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.

Ok. I will add it.

>
>
>>> 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.

Agree.

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

If we clear the DPC trigger status in the DLLSC state change handler, I
agree there could be a race. However, if we clear the DPC trigger in the
PDC state change handler, I believe it will not race because the device
has already been removed. Is my understanding correct?

> 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

--
Sathyanarayanan Kuppuswamy
Linux Kernel Developer