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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <30fe11dd-3f21-4a61-adb0-74e39087c84c@linux.alibaba.com>
Date: Mon, 20 Oct 2025 23:20:58 +0800
From: Shuai Xue <xueshuai@...ux.alibaba.com>
To: Lukas Wunner <lukas@...ner.de>
Cc: linux-pci@...r.kernel.org, linux-kernel@...r.kernel.org,
 linuxppc-dev@...ts.ozlabs.org, bhelgaas@...gle.com, kbusch@...nel.org,
 sathyanarayanan.kuppuswamy@...ux.intel.com, mahesh@...ux.ibm.com,
 oohall@...il.com, Jonathan.Cameron@...wei.com, terry.bowman@....com,
 tianruidong@...ux.alibaba.com
Subject: Re: [PATCH v6 3/5] PCI/AER: Report fatal errors of RCiEP and EP if
 link recoverd

在 2025/10/20 22:24, Lukas Wunner 写道:
> On Mon, Oct 20, 2025 at 10:17:10PM +0800, Shuai Xue wrote:
>> 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);
>> }

Hi Lukas,
> 
> Much better.  Again, I think you don't need to rename add_error_device()
> and then the code comment even fits on the same line:

Good point. I'll keep the original function name and use the one-line
comment format.

> 
> 	pci_dev_put(dev);  /* pairs with pci_dev_get() in add_error_device() */
> 
>>>>     .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:

1. Scope creep: The current implementation is explicit - we know we're
in an AER error recovery path and intentionally report the missed fatal
errors. Your suggestion would make error reporting implicit during any
device state restoration.

2. Wider impact: pci_save_state() and pci_restore_state() are used
extensively beyond AER error handling - in power management, reset
operations, and various driver flows. Adding error reporting to
pci_restore_state() could generate unexpected error messages in
non-error scenarios.

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.

> Thanks,
> 
> Lukas

Thanks.
Shuai