| Message ID | 20220901181634.99591-3-chenzhuo.1@bytedance.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | PCI/AER: Fix and optimize usage of status clear api | expand |
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. > 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. > + if (state == pci_channel_io_frozen) > + pci_aer_clear_fatal_status(dev); > + else > pci_aer_clear_nonfatal_status(dev); > - } > + > pci_info(bridge, "device recovery successful\n"); > return status; > > -- > 2.30.1 (Apple Git-130) >
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. > >> + if (state == pci_channel_io_frozen) >> + pci_aer_clear_fatal_status(dev); >> + else >> pci_aer_clear_nonfatal_status(dev); >> - } >> + >> pci_info(bridge, "device recovery successful\n"); >> return status; >> >> -- >> 2.30.1 (Apple Git-130) >>
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
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).
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); + + if (state == pci_channel_io_frozen) + pci_aer_clear_fatal_status(dev); + else pci_aer_clear_nonfatal_status(dev); - } + pci_info(bridge, "device recovery successful\n"); return status;
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(). 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. Signed-off-by: Zhuo Chen <chenzhuo.1@bytedance.com> --- drivers/pci/pcie/err.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)