Re: [PATCH v7 2/5] PCI/DPC: Run recovery on device that detected the error

[Shuai Xue <xueshuai@linux.alibaba.com>]

On 2/7/26 3:48 PM, Shuai Xue wrote:
> 
> 
> On 2/3/26 5:09 AM, Lukas Wunner wrote:
>> On Mon, Feb 02, 2026 at 03:02:54PM +0100, Lukas Wunner wrote:
>>> You're assuming that the parent of the Requester is always identical
>>> to the containing Downstream Port.  But that's not necessarily the case:
>>>
>>> E.g., imagine a DPC-capable Root Port with a PCIe switch below
>>> whose Downstream Ports are not DPC-capable.  Let's say an Endpoint
>>> beneath the PCIe switch sends ERR_FATAL upstream.  AFAICS, your patch
>>> will cause pcie_do_recovery() to invoke dpc_reset_link() with the
>>> Downstream Port of the PCIe switch as argument.  That function will
>>> then happily use pdev->dpc_cap even though it's 0.
>>
>> Thinking about this some more, I realized there's another problem:
>>
>> In a scenario like the one I've outlined above, after your change,
>> pcie_do_recovery() will only broadcast error_detected (and other
>> callbacks) below the downstream port of the PCIe switch -- and not
>> to any other devices below the containing Root Port.
>>
>> However, the DPC-induced Link Down event at the Root Port results
>> in a Hot Reset being propagated down the hierarchy to any device
>> below the Root Port.  So with your change, the siblings of the
>> downstream port on the PCIe switch will no longer be informed of
>> the reset and thus are no longer given an opportunity to recover
>> after reset.
>>
>> The premise on which this patch is built is false -- that the bridge
>> upstream of the error-reporting device is always equal to the
>> containing Downstream Port.
> 
> Thanks again for the very detailed analysis and for the pointers to
> your earlier mail.
> 
> You are right, thanks for pointing it out.
> 
>>
>> It seems the only reason why you want to pass the reporting device
>> to pcie_do_recovery() is that you want to call pcie_clear_device_status()
>> and pci_aer_clear_nonfatal_status() with that device.
> 
> In the AER path, pcie_do_recovery() is indeed invoked with the Error
> Source device found by find_source_device(), and internally it treats
> that dev as the function that detected the error and derives the
> containing Downstream Port (bridge) from it.  For DPC, however, the
> error-detecting function is the DPC-capable Downstream Port itself, not
> the Endpoint identified as Error Source, so passing the Endpoint to
> pcie_do_recovery() breaks that assumption.
>>
>> However as I've said before, those calls are AER-specific and should
>> be moved out of pcie_do_recovery() so that it becomes generic and can
>> be used by EEH and s390:
>>
>> https://lore.kernel.org/all/aPYKe1UKKkR7qrt1@wunner.de/
> 
> Sure, I'd like to move it out.  I will remove the AER-specific calls
> (pcie_clear_device_status() and pci_aer_raw_clear_status()) from
> pcie_do_recovery() itself, and instead handle them in the AER and DPC
> code paths where we already know which device(s) are the actual error
> sources.  That way, pcie_do_recovery() becomes a generic recovery
> framework that can be reused by EEH and s390.
> 
>>
>> There's another problem:  When a device experiences an error while DPC
>> is ongoing (i.e. while the link is down), its ERR_FATAL or ERR_NONFATAL
>> Message may not come through.  Still the error bits are set in the
>> device's Uncorrectable Error Status register.  So I think what we need to
>> do is walk the hierarchy below the containing Downstream Port after the
>> link is back up and search for devices with any error bits set,
>> then report and clear those errors.  We may do this after first
>> examining the device in the DPC Error Source ID register.
>> Any additional errors found while walking the hierarchy can then
>> be identified as "occurred during DPC recovery".
> 
> I agree this is similar in spirit to find_source_device() -- both walk
> the bus and check AER Status registers.  For the DPC case, I'll perform
> this walk after the link is back up (i.e., after dpc_reset_link()
> succeeds).
> 
> Regarding pci_restore_state() in slot_reset(): I see now that it does
> call pci_aer_clear_status(dev) (at line 1844 in pci.c), which will
> clear the AER Status registers.  So if we walk the hierarchy after
> the slot_reset callbacks, the error bits accumulated during DPC will
> already be cleared.
> 
> To avoid losing those errors, I think the walk should happen after
> dpc_reset_link() succeeds but *before* pcie_do_recovery() invokes the
> slot_reset callbacks.  That way, we can capture the AER Status bits
> before pci_restore_state() clears them.
> 
> Does that sound like the right approach, or would you prefer a
> different placement?
> 
> Thanks a lot for your guidance.
> 
> Best Regards,
> Shuai
> 

Hi, Lukas,

Gentle ping. Any feedback?

Thanks.
Shuai