Messages in this thread | | | Date | Tue, 27 Sep 2022 21:41:09 +0800 | Subject | Re: [External] Re: [PATCH 2/3] PCI/ERR: Clear fatal status in pcie_do_recovery() | From | Zhuo Chen <> |
| |
On 9/27/22 2:09 AM, Bjorn Helgaas wrote: > On Mon, Sep 26, 2022 at 10:01:55PM +0800, Zhuo Chen wrote: >> On 9/23/22 5:08 AM, Bjorn Helgaas wrote: >>> On Fri, Sep 02, 2022 at 02:16:33AM +0800, Zhuo Chen wrote: >>>> When state is pci_channel_io_frozen in pcie_do_recovery(), >>>> the severity is fatal and fatal status should be cleared. >>>> So we add pci_aer_clear_fatal_status(). >>> >>> Seems sensible to me. Did you find this by code inspection or by >>> debugging a problem? If the latter, it would be nice to mention the >>> symptoms of the problem in the commit log. >> >> I found this by code inspection so I may not enumerate what kind of problems >> this code will cause. >>> >>>> Since pcie_aer_is_native() in pci_aer_clear_fatal_status() >>>> and pci_aer_clear_nonfatal_status() contains the function of >>>> 'if (host->native_aer || pcie_ports_native)', so we move them >>>> out of it. >>> >>> Wrap commit log to fill 75 columns. >>> >>>> Signed-off-by: Zhuo Chen <chenzhuo.1@bytedance.com> >>>> --- >>>> drivers/pci/pcie/err.c | 8 ++++++-- >>>> 1 file changed, 6 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/drivers/pci/pcie/err.c b/drivers/pci/pcie/err.c >>>> index 0c5a143025af..e0a8ade4c3fe 100644 >>>> --- a/drivers/pci/pcie/err.c >>>> +++ b/drivers/pci/pcie/err.c >>>> @@ -243,10 +243,14 @@ pci_ers_result_t pcie_do_recovery(struct pci_dev *dev, >>>> * it is responsible for clearing this status. In that case, the >>>> * signaling device may not even be visible to the OS. >>>> */ >>>> - if (host->native_aer || pcie_ports_native) { >>>> + if (host->native_aer || pcie_ports_native) >>>> pcie_clear_device_status(dev); >>> >>> pcie_clear_device_status() doesn't check for pcie_aer_is_native() >>> internally, but after 068c29a248b6 ("PCI/ERR: Clear PCIe Device Status >>> errors only if OS owns AER") and aa344bc8b727 ("PCI/ERR: Clear AER >>> status only when we control AER"), both callers check before calling >>> it. >>> >>> I think we should move the check inside pcie_clear_device_status(). >>> That could be a separate preliminary patch. >>> >>> There are a couple other places (aer_root_reset() and >>> get_port_device_capability()) that do the same check and could be >>> changed to use pcie_aer_is_native() instead. That could be another >>> preliminary patch. >>> >> Good suggestion. But I have only one doubt. In aer_root_reset(), if we use >> "if (pcie_aer_is_native(dev) && aer)", when dev->aer_cap >> is NULL and root->aer_cap is not NULL, pcie_aer_is_native() will return >> false. It's different from just using "(host->native_aer || >> pcie_ports_native)". >> Or if we can use "if (pcie_aer_is_native(root))", at this time a NULL >> pointer check should be added in pcie_aer_is_native() because root may be >> NULL. > > Good point. In aer_root_reset(), we're updating Root Port registers, > so I think they should look like: > > if (pcie_aer_is_native(root) && aer) { > ... > } > > Does that seem safe and equivalent to you? > > Bjorn
I think ‘if (aer && pcie_aer_is_native(root))’ might be safer, because when root is NULL, 'aer' will be NUll as well, and the predicate will return false without entering pcie_aer_is_native(root).
-- Thanks, Zhuo Chen
| |