| Message ID | 20200717195619.766662-1-helgaas@kernel.org (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | PCI/ERR: Rename pci_aer_clear_device_status() to pcie_clear_device_status() | expand |
Hi Bjorn, On 7/17/20 12:56 PM, Bjorn Helgaas wrote: > From: Bjorn Helgaas <bhelgaas@google.com> > > pci_aer_clear_device_status() clears the error bits in the PCIe Device > Status Register (PCI_EXP_DEVSTA). Every PCIe device has this register, > regardless of whether it supports AER. Since its not related to AER, can we move it out of AER driver ? May be to pci.c ? > > Rename pci_aer_clear_device_status() to pcie_clear_device_status() to make > clear that it is PCIe-specific but not AER-specific. No functional change > intended. > > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> > --- > drivers/pci/pci.h | 4 ++-- > drivers/pci/pcie/aer.c | 4 ++-- > drivers/pci/pcie/err.c | 2 +- > 3 files changed, 5 insertions(+), 5 deletions(-) > > diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h > index c6c0c455f59f..c5f271e6e276 100644 > --- a/drivers/pci/pci.h > +++ b/drivers/pci/pci.h > @@ -657,16 +657,16 @@ void pci_no_aer(void); > void pci_aer_init(struct pci_dev *dev); > void pci_aer_exit(struct pci_dev *dev); > extern const struct attribute_group aer_stats_attr_group; > +void pcie_clear_device_status(struct pci_dev *dev); > void pci_aer_clear_fatal_status(struct pci_dev *dev); > -void pci_aer_clear_device_status(struct pci_dev *dev); > int pci_aer_clear_status(struct pci_dev *dev); > int pci_aer_raw_clear_status(struct pci_dev *dev); > #else > static inline void pci_no_aer(void) { } > static inline void pci_aer_init(struct pci_dev *d) { } > static inline void pci_aer_exit(struct pci_dev *d) { } > +static inline void pcie_clear_device_status(struct pci_dev *dev) { } > static inline void pci_aer_clear_fatal_status(struct pci_dev *dev) { } > -static inline void pci_aer_clear_device_status(struct pci_dev *dev) { } > static inline int pci_aer_clear_status(struct pci_dev *dev) { return -EINVAL; } > static inline int pci_aer_raw_clear_status(struct pci_dev *dev) { return -EINVAL; } > #endif > diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c > index ca886bf91fd9..d3ea667c8520 100644 > --- a/drivers/pci/pcie/aer.c > +++ b/drivers/pci/pcie/aer.c > @@ -241,7 +241,7 @@ int pci_disable_pcie_error_reporting(struct pci_dev *dev) > } > EXPORT_SYMBOL_GPL(pci_disable_pcie_error_reporting); > > -void pci_aer_clear_device_status(struct pci_dev *dev) > +void pcie_clear_device_status(struct pci_dev *dev) > { > u16 sta; > > @@ -947,7 +947,7 @@ static void handle_error_source(struct pci_dev *dev, struct aer_err_info *info) > if (aer) > pci_write_config_dword(dev, aer + PCI_ERR_COR_STATUS, > info->status); > - pci_aer_clear_device_status(dev); > + pcie_clear_device_status(dev); > } else if (info->severity == AER_NONFATAL) > pcie_do_recovery(dev, pci_channel_io_normal, aer_root_reset); > else if (info->severity == AER_FATAL) > diff --git a/drivers/pci/pcie/err.c b/drivers/pci/pcie/err.c > index 467686ee2d8b..55755bc493f1 100644 > --- a/drivers/pci/pcie/err.c > +++ b/drivers/pci/pcie/err.c > @@ -197,7 +197,7 @@ pci_ers_result_t pcie_do_recovery(struct pci_dev *dev, > pci_dbg(dev, "broadcast resume message\n"); > pci_walk_bus(bus, report_resume, &status); > > - pci_aer_clear_device_status(dev); > + pcie_clear_device_status(dev); > pci_aer_clear_nonfatal_status(dev); > pci_info(dev, "device recovery successful\n"); > return status; >
On Fri, Jul 17, 2020 at 01:20:22PM -0700, Kuppuswamy, Sathyanarayanan wrote: > Hi Bjorn, > > On 7/17/20 12:56 PM, Bjorn Helgaas wrote: > > From: Bjorn Helgaas <bhelgaas@google.com> > > > > pci_aer_clear_device_status() clears the error bits in the PCIe Device > > Status Register (PCI_EXP_DEVSTA). Every PCIe device has this register, > > regardless of whether it supports AER. > > Since its not related to AER, can we move it out of AER driver ? May be > to pci.c ? OK. pci.c is really a grab bag of random stuff, but it *is* true that this doesn't really seem to belong in aer.c. So I don't mind moving it to pci.c (does just before pcie_clear_root_pme_status() seem like a reasonable spot?) But looking at this again makes me wonder whether putting the pcie_aer_is_native() test inside pcie_clear_device_status() is the right thing. It seems like that test fits better with the AER code, i.e., in the *callers* of pcie_clear_device_status(). It would mean repeating the test, since we call it twice, but it seems like it might match up with the spec better. And I have a slight aversion to functions that can silently return without doing what it looks like they're supposed to do. I can fix this all up if that seems right. Or let me know if you have alternate ideas. Bjorn
On 7/21/20 2:53 PM, Bjorn Helgaas wrote: > On Fri, Jul 17, 2020 at 01:20:22PM -0700, Kuppuswamy, Sathyanarayanan wrote: >> Hi Bjorn, >> >> On 7/17/20 12:56 PM, Bjorn Helgaas wrote: >>> From: Bjorn Helgaas <bhelgaas@google.com> >>> >>> pci_aer_clear_device_status() clears the error bits in the PCIe Device >>> Status Register (PCI_EXP_DEVSTA). Every PCIe device has this register, >>> regardless of whether it supports AER. >> >> Since its not related to AER, can we move it out of AER driver ? May be >> to pci.c ? > > OK. pci.c is really a grab bag of random stuff, but it *is* true that > this doesn't really seem to belong in aer.c. > > So I don't mind moving it to pci.c (does just before > pcie_clear_root_pme_status() seem like a reasonable spot?) > > But looking at this again makes me wonder whether putting the > pcie_aer_is_native() test inside pcie_clear_device_status() is the > right thing. It seems like that test fits better with the AER code, > i.e., in the *callers* of pcie_clear_device_status(). Yes. pcie_aer_is_native() should be left in AER driver. > > It would mean repeating the test, since we call it twice, but it seems > like it might match up with the spec better. And I have a slight > aversion to functions that can silently return without doing what it > looks like they're supposed to do. > > I can fix this all up if that seems right. Or let me know if you have > alternate ideas. Agree with your approach. Its ok to add separate check for pcie_aer_is_native(). > > Bjorn >
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h index c6c0c455f59f..c5f271e6e276 100644 --- a/drivers/pci/pci.h +++ b/drivers/pci/pci.h @@ -657,16 +657,16 @@ void pci_no_aer(void); void pci_aer_init(struct pci_dev *dev); void pci_aer_exit(struct pci_dev *dev); extern const struct attribute_group aer_stats_attr_group; +void pcie_clear_device_status(struct pci_dev *dev); void pci_aer_clear_fatal_status(struct pci_dev *dev); -void pci_aer_clear_device_status(struct pci_dev *dev); int pci_aer_clear_status(struct pci_dev *dev); int pci_aer_raw_clear_status(struct pci_dev *dev); #else static inline void pci_no_aer(void) { } static inline void pci_aer_init(struct pci_dev *d) { } static inline void pci_aer_exit(struct pci_dev *d) { } +static inline void pcie_clear_device_status(struct pci_dev *dev) { } static inline void pci_aer_clear_fatal_status(struct pci_dev *dev) { } -static inline void pci_aer_clear_device_status(struct pci_dev *dev) { } static inline int pci_aer_clear_status(struct pci_dev *dev) { return -EINVAL; } static inline int pci_aer_raw_clear_status(struct pci_dev *dev) { return -EINVAL; } #endif diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c index ca886bf91fd9..d3ea667c8520 100644 --- a/drivers/pci/pcie/aer.c +++ b/drivers/pci/pcie/aer.c @@ -241,7 +241,7 @@ int pci_disable_pcie_error_reporting(struct pci_dev *dev) } EXPORT_SYMBOL_GPL(pci_disable_pcie_error_reporting); -void pci_aer_clear_device_status(struct pci_dev *dev) +void pcie_clear_device_status(struct pci_dev *dev) { u16 sta; @@ -947,7 +947,7 @@ static void handle_error_source(struct pci_dev *dev, struct aer_err_info *info) if (aer) pci_write_config_dword(dev, aer + PCI_ERR_COR_STATUS, info->status); - pci_aer_clear_device_status(dev); + pcie_clear_device_status(dev); } else if (info->severity == AER_NONFATAL) pcie_do_recovery(dev, pci_channel_io_normal, aer_root_reset); else if (info->severity == AER_FATAL) diff --git a/drivers/pci/pcie/err.c b/drivers/pci/pcie/err.c index 467686ee2d8b..55755bc493f1 100644 --- a/drivers/pci/pcie/err.c +++ b/drivers/pci/pcie/err.c @@ -197,7 +197,7 @@ pci_ers_result_t pcie_do_recovery(struct pci_dev *dev, pci_dbg(dev, "broadcast resume message\n"); pci_walk_bus(bus, report_resume, &status); - pci_aer_clear_device_status(dev); + pcie_clear_device_status(dev); pci_aer_clear_nonfatal_status(dev); pci_info(dev, "device recovery successful\n"); return status;