Re: [PATCH v6 3/5] PCI/AER: Report fatal errors of RCiEP and EP if link recoverd

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

在 2025/10/24 14:43, Shuai Xue 写道:
> 
> 
> 在 2025/10/23 18:48, Lukas Wunner 写道:
>> On Mon, Oct 20, 2025 at 11:20:58PM +0800, Shuai Xue wrote:
>>> 2025/10/20 22:24, Lukas Wunner:
>>>> On Mon, Oct 20, 2025 at 10:17:10PM +0800, Shuai Xue wrote:
>>>>>>>      .slot_reset()
>>>>>>>        => pci_restore_state()
>>>>>>>          => pci_aer_clear_status()
>>>>>>
>>>>>> This was added in 2015 by b07461a8e45b.  The commit claims that
>>>>>> the errors are stale and can be ignored.  It turns out they cannot.
>>>>>>
>>>>>> So maybe pci_restore_state() should print information about the
>>>>>> errors before clearing them?
>>>>>
>>>>> While that could work, we would lose the error severity information at
>>>>
>>>> Wait, we've got that saved in pci_cap_saved_state, so we could restore
>>>> the severity register, report leftover errors, then clear those errors?
>>>
>>> You're right that the severity register is also sticky, so we could
>>> retrieve error severity directly from AER registers.
>>>
>>> However, I have concerns about implementing this approach:
>> [...]
>>> 3. Architectural consistency: As you noted earlier, "pci_restore_state()
>>> is only supposed to restore state, as the name implies, and not clear
>>> errors." Adding error reporting to this function would further violate
>>> this principle - we'd be making it do even more than just restore state.
>>>
>>> Would you prefer I implement this broader change, or shall we proceed
>>> with the targeted helper function approach for now? The helper function
>>> solves the immediate problem while keeping the changes focused on the
>>> AER recovery path.
>>
>> My opinion is that b07461a8e45b was wrong and that reported errors
>> should not be silently ignored. 
> 
> Thanks for your input and for discussing the history of commit
> b07461a8e45b. I understand its intention to ignore errors specifically
> during enumeration. As far as I know, AdvNonFatalErr events can occur in
> this phase and typically should be ignored to simplify handling.
> 
>> What I'd prefer is that if
>> pci_restore_state() discovers unreported errors, it asks the AER driver
>> to report them.
>>
>> We've already got a helper to do that:  aer_recover_queue()
>> It queues up an entry in AER's kfifo and asks AER to report it.
>>
>> So far the function is only used by GHES.  GHES allocates the
>> aer_regs argument from ghes_estatus_pool using gen_pool_alloc().
>> Consequently aer_recover_work_func() uses ghes_estatus_pool_region_free()
>> to free the allocation.  That prevents using aer_recover_queue()
>> for anything else than GHES.  It would first be necessary to
>> refactor aer_recover_queue() + aer_recover_work_func() such that
>> it can cope with arbitrary allocations (e.g. kmalloc()).
> 
> I agree that aer_recover_queue() and aer_recover_work_func() offer a
> generalized way to report errors.
> 
> However, I’d like to highlight some concerns regarding error discovery
> during pci_restore_state():
> 
> - Errors During Enumeration via Hotplug: Errors such as AdvNonFatalErr
>    seen during enumeration or hotplug are generally intended to be
>    ignored, as handling them adds unnecessary complexity without
>    practical benefits.
> 
> - Errors During Downstream Port Containment (DPC): When an error is
>    detected and not masked, it is expected to propagate through the usual
>    AER path, either reported directly to the OS or to the firmware.
>    Finally, these errors should be cleared and reported in a single
>    cohesive step.
> 
> For missed fatal errors during DPC, queuing additional work to report
> these errors using aer_recover_queue() could introduce significant
> overhead. Specifically: It may result in the bus being reset and the
> device reset again, which could unnecessarily disrupt system operation.
> 
> Do we really need the heavy way?
> 
> I would appreciate more feedback from the community on whether queuing
> another recovery task for errors detected during pci_restore_state()
> 

Hi, ALL,

Gentle ping.

Any feedback is welcomed.

Thanks.
Shuai