| Message ID | 20220927153524.49172-2-chenzhuo.1@bytedance.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | PCI/AER: Fix and optimize usage of status clearing api | expand |
Hi, On 9/27/22 8:35 AM, Zhuo Chen wrote: > Sometimes we need to clear aer uncorrectable error status, so we add Adding n actual use case will help. > pci_aer_clear_uncorrect_error_status() to PCI core. If possible, try to avoid "we" usage in commit log. Just say "so add pci_aer_clear_uncorrect_error_status() function" > > Signed-off-by: Zhuo Chen <chenzhuo.1@bytedance.com> > --- > drivers/pci/pcie/aer.c | 16 ++++++++++++++++ > include/linux/aer.h | 5 +++++ > 2 files changed, 21 insertions(+) > > diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c > index e2d8a74f83c3..4e637121be23 100644 > --- a/drivers/pci/pcie/aer.c > +++ b/drivers/pci/pcie/aer.c > @@ -286,6 +286,22 @@ void pci_aer_clear_fatal_status(struct pci_dev *dev) > pci_write_config_dword(dev, aer + PCI_ERR_UNCOR_STATUS, status); > } > > +int pci_aer_clear_uncorrect_error_status(struct pci_dev *dev) > +{ > + int aer = dev->aer_cap; > + u32 status; > + > + if (!pcie_aer_is_native(dev)) > + return -EIO; > + > + pci_read_config_dword(dev, aer + PCI_ERR_UNCOR_STATUS, &status); > + if (status) > + pci_write_config_dword(dev, aer + PCI_ERR_UNCOR_STATUS, status); Why not just write all '1' and clear it? Why read and write? > + > + return 0; > +} > +EXPORT_SYMBOL_GPL(pci_aer_clear_uncorrect_error_status); Add details about why you want to export in commit log. > + > /** > * pci_aer_raw_clear_status - Clear AER error registers. > * @dev: the PCI device > diff --git a/include/linux/aer.h b/include/linux/aer.h > index 97f64ba1b34a..154690c278cb 100644 > --- a/include/linux/aer.h > +++ b/include/linux/aer.h > @@ -45,6 +45,7 @@ struct aer_capability_regs { > int pci_enable_pcie_error_reporting(struct pci_dev *dev); > int pci_disable_pcie_error_reporting(struct pci_dev *dev); > int pci_aer_clear_nonfatal_status(struct pci_dev *dev); > +int pci_aer_clear_uncorrect_error_status(struct pci_dev *dev); > void pci_save_aer_state(struct pci_dev *dev); > void pci_restore_aer_state(struct pci_dev *dev); > #else > @@ -60,6 +61,10 @@ static inline int pci_aer_clear_nonfatal_status(struct pci_dev *dev) > { > return -EINVAL; > } > +static inline int pci_aer_clear_uncorrect_error_status(struct pci_dev *dev) > +{ > + return -EINVAL; > +} > static inline void pci_save_aer_state(struct pci_dev *dev) {} > static inline void pci_restore_aer_state(struct pci_dev *dev) {} > #endif
On 9/28/22 3:31 AM, Sathyanarayanan Kuppuswamy wrote: > Hi, > > On 9/27/22 8:35 AM, Zhuo Chen wrote: >> Sometimes we need to clear aer uncorrectable error status, so we add > > Adding n actual use case will help. > >> pci_aer_clear_uncorrect_error_status() to PCI core. > > If possible, try to avoid "we" usage in commit log. Just say "so add > pci_aer_clear_uncorrect_error_status() function" > >> >> Signed-off-by: Zhuo Chen <chenzhuo.1@bytedance.com> >> --- >> drivers/pci/pcie/aer.c | 16 ++++++++++++++++ >> include/linux/aer.h | 5 +++++ >> 2 files changed, 21 insertions(+) >> >> diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c >> index e2d8a74f83c3..4e637121be23 100644 >> --- a/drivers/pci/pcie/aer.c >> +++ b/drivers/pci/pcie/aer.c >> @@ -286,6 +286,22 @@ void pci_aer_clear_fatal_status(struct pci_dev *dev) >> pci_write_config_dword(dev, aer + PCI_ERR_UNCOR_STATUS, status); >> } >> >> +int pci_aer_clear_uncorrect_error_status(struct pci_dev *dev) >> +{ >> + int aer = dev->aer_cap; >> + u32 status; >> + >> + if (!pcie_aer_is_native(dev)) >> + return -EIO; >> + >> + pci_read_config_dword(dev, aer + PCI_ERR_UNCOR_STATUS, &status); >> + if (status) >> + pci_write_config_dword(dev, aer + PCI_ERR_UNCOR_STATUS, status); > > Why not just write all '1' and clear it? Why read and write? Hi Sathyanarayanan, The current implementation and the previous implementation are all first read and then write to clear the status. Currently just be consistent with them: - aer_enable_rootport() - pci_aer_raw_clear_status() - pcibios_plat_dev_init() in arch/mips/pci/pci-octeon.c - commit fd3362cb73de ("PCI/AER: Squash aerdrv_core.c into aerdrv.c") pci_cleanup_aer_uncorrect_error_status > >> + >> + return 0; >> +} >> +EXPORT_SYMBOL_GPL(pci_aer_clear_uncorrect_error_status); > > Add details about why you want to export in commit log. Thanks, I will add details and improve commit log in next version. > >> + >> /** >> * pci_aer_raw_clear_status - Clear AER error registers. >> * @dev: the PCI device >> diff --git a/include/linux/aer.h b/include/linux/aer.h >> index 97f64ba1b34a..154690c278cb 100644 >> --- a/include/linux/aer.h >> +++ b/include/linux/aer.h >> @@ -45,6 +45,7 @@ struct aer_capability_regs { >> int pci_enable_pcie_error_reporting(struct pci_dev *dev); >> int pci_disable_pcie_error_reporting(struct pci_dev *dev); >> int pci_aer_clear_nonfatal_status(struct pci_dev *dev); >> +int pci_aer_clear_uncorrect_error_status(struct pci_dev *dev); >> void pci_save_aer_state(struct pci_dev *dev); >> void pci_restore_aer_state(struct pci_dev *dev); >> #else >> @@ -60,6 +61,10 @@ static inline int pci_aer_clear_nonfatal_status(struct pci_dev *dev) >> { >> return -EINVAL; >> } >> +static inline int pci_aer_clear_uncorrect_error_status(struct pci_dev *dev) >> +{ >> + return -EINVAL; >> +} >> static inline void pci_save_aer_state(struct pci_dev *dev) {} >> static inline void pci_restore_aer_state(struct pci_dev *dev) {} >> #endif >
diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c index e2d8a74f83c3..4e637121be23 100644 --- a/drivers/pci/pcie/aer.c +++ b/drivers/pci/pcie/aer.c @@ -286,6 +286,22 @@ void pci_aer_clear_fatal_status(struct pci_dev *dev) pci_write_config_dword(dev, aer + PCI_ERR_UNCOR_STATUS, status); } +int pci_aer_clear_uncorrect_error_status(struct pci_dev *dev) +{ + int aer = dev->aer_cap; + u32 status; + + if (!pcie_aer_is_native(dev)) + return -EIO; + + pci_read_config_dword(dev, aer + PCI_ERR_UNCOR_STATUS, &status); + if (status) + pci_write_config_dword(dev, aer + PCI_ERR_UNCOR_STATUS, status); + + return 0; +} +EXPORT_SYMBOL_GPL(pci_aer_clear_uncorrect_error_status); + /** * pci_aer_raw_clear_status - Clear AER error registers. * @dev: the PCI device diff --git a/include/linux/aer.h b/include/linux/aer.h index 97f64ba1b34a..154690c278cb 100644 --- a/include/linux/aer.h +++ b/include/linux/aer.h @@ -45,6 +45,7 @@ struct aer_capability_regs { int pci_enable_pcie_error_reporting(struct pci_dev *dev); int pci_disable_pcie_error_reporting(struct pci_dev *dev); int pci_aer_clear_nonfatal_status(struct pci_dev *dev); +int pci_aer_clear_uncorrect_error_status(struct pci_dev *dev); void pci_save_aer_state(struct pci_dev *dev); void pci_restore_aer_state(struct pci_dev *dev); #else @@ -60,6 +61,10 @@ static inline int pci_aer_clear_nonfatal_status(struct pci_dev *dev) { return -EINVAL; } +static inline int pci_aer_clear_uncorrect_error_status(struct pci_dev *dev) +{ + return -EINVAL; +} static inline void pci_save_aer_state(struct pci_dev *dev) {} static inline void pci_restore_aer_state(struct pci_dev *dev) {} #endif
Sometimes we need to clear aer uncorrectable error status, so we add pci_aer_clear_uncorrect_error_status() to PCI core. Signed-off-by: Zhuo Chen <chenzhuo.1@bytedance.com> --- drivers/pci/pcie/aer.c | 16 ++++++++++++++++ include/linux/aer.h | 5 +++++ 2 files changed, 21 insertions(+)