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

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

Hi, Lukas,

在 2025/10/20 21:54, Lukas Wunner 写道:
> On Mon, Oct 20, 2025 at 08:58:55PM +0800, Shuai Xue wrote:
>> ??? 2025/10/20 18:10, Lukas Wunner ??????:
>>> On Wed, Oct 15, 2025 at 10:41:57AM +0800, Shuai Xue wrote:
>>>> +++ b/drivers/pci/pcie/err.c
>>>> @@ -253,6 +254,16 @@ pci_ers_result_t pcie_do_recovery(struct pci_dev *dev,
>>>>    			pci_warn(bridge, "subordinate device reset failed\n");
>>>>    			goto failed;
>>>>    		}
>>>> +
>>>> +		/* Link recovered, report fatal errors of RCiEP or EP */
>>>> +		if (state == pci_channel_io_frozen &&
>>>> +		    (type == PCI_EXP_TYPE_ENDPOINT || type == PCI_EXP_TYPE_RC_END)) {
>>>> +			aer_add_error_device(&info, dev);
>>>> +			info.severity = AER_FATAL;
>>>> +			if (aer_get_device_error_info(&info, 0, true))
>>>> +				aer_print_error(&info, 0);
>>>> +			pci_dev_put(dev);
>>>> +		}
>>>
>>> Where is the the pci_dev_get() to balance the pci_dev_put() here?
>>
>> The corresponding pci_dev_get() is called in add_error_device(). Please
>> refer to commit 60271ab044a5 ("PCI/AER: Take reference on error
>> devices") which introduced this reference counting mechanism.
> 
> That is non-obvious and needs a code comment.

Agreed. I'll add a comment to clarify the reference counting relationship.

> 
>>> It feels awkward to leak AER-specific details into pcie_do_recovery().
>>> That function is supposed to implement the flow described in
>>> Documentation/PCI/pci-error-recovery.rst in a platform-agnostic way
>>> so that powerpc (EEH) and s390 could conceivably take advantage of it.
>>>
>>> Can you find a way to avoid this, e.g. report errors after
>>> pcie_do_recovery() has concluded?
>>
>> I understand your concern about keeping pcie_do_recovery()
>> platform-agnostic.
> 
> The code you're adding above, with the exception of the check for
> pci_channel_io_frozen, should live in a helper in aer.c.
> Then you also don't need to rename add_error_device().

Good point.

That's a much cleaner approach. I'll create a helper function in aer.c,
something like:

void aer_report_frozen_error(struct pci_dev *dev)
{
     struct aer_err_info info;

     if (dev->pci_type != PCI_EXP_TYPE_ENDPOINT &&
         dev->pci_type != PCI_EXP_TYPE_RC_END)
         return;

     aer_info_init(&info);
     aer_add_error_device(&info, dev);
     info.severity = AER_FATAL;
     if (aer_get_device_error_info(&info, 0, true))
         aer_print_error(&info, 0);

     /* pci_dev_put() pairs with pci_dev_get() in aer_add_error_device() */
     pci_dev_put(dev);
}

>> I explored the possibility of reporting errors after
>> recovery concludes, but unfortunately, this approach isn't feasible due
>> to the recovery sequence. The issue is that most drivers'
>> pci_error_handlers implement .slot_reset() which internally calls
>> pci_restore_state() to restore the device's configuration space and
>> state. This function also clears the device's AER status registers:
>>
>>    .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
that point, which could lead to duplicate or less informative error
messages compared to what the AER driver provides. The helper function
approach preserves all error details for proper reporting.

> 
> Actually pci_restore_state() is only supposed to restore state,
> as the name implies, and not clear errors.  It seems questionable
> that the commit amended it to do that.
> 
>>> I'm also worried that errors are reported *during* recovery.
>>> I imagine this looks confusing to a user.  The logged messages
>>> should make it clear that these are errors that occurred *earlier*
>>> and are reported belatedly.
>>
>> You raise an excellent point about potential user confusion. The current
>> aer_print_error() interface doesn't indicate that these are historical
>> errors being reported belatedly. Would it be acceptable to add a
>> clarifying message before calling aer_print_error()? For example:
>>
>>    pci_err(dev, "Reporting error that occurred before recovery:\n");
> 
> Yes, something like that.  "Errors reported prior to reset"?  Dunno.

I'll use "Errors reported prior to reset" - it's clear and concise.

> 
> Thanks,
> 
> Lukas

Thanks.
Shuai