| Message ID | 20220927153524.49172-1-chenzhuo.1@bytedance.com |
|---|---|
| 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/27/22 8:35 AM, Zhuo Chen wrote: > Since pci_aer_clear_nonfatal_status() is used only internally, move > its declaration to the PCI internal header file. Also, no one cares > about return value of pci_aer_clear_nonfatal_status(), so make it void. > > Signed-off-by: Zhuo Chen <chenzhuo.1@bytedance.com> > --- Looks good to me. > drivers/pci/pci.h | 2 ++ > drivers/pci/pcie/aer.c | 7 ++----- > include/linux/aer.h | 5 ----- > 3 files changed, 4 insertions(+), 10 deletions(-) > > diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h > index 785f31086313..a114175d08e4 100644 > --- a/drivers/pci/pci.h > +++ b/drivers/pci/pci.h > @@ -684,6 +684,7 @@ 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 pci_aer_clear_fatal_status(struct pci_dev *dev); > +void pci_aer_clear_nonfatal_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 > @@ -691,6 +692,7 @@ 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 pci_aer_clear_fatal_status(struct pci_dev *dev) { } > +static inline void pci_aer_clear_nonfatal_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 4e637121be23..e2ebd108339d 100644 > --- a/drivers/pci/pcie/aer.c > +++ b/drivers/pci/pcie/aer.c > @@ -251,13 +251,13 @@ int pci_disable_pcie_error_reporting(struct pci_dev *dev) > } > EXPORT_SYMBOL_GPL(pci_disable_pcie_error_reporting); > > -int pci_aer_clear_nonfatal_status(struct pci_dev *dev) > +void pci_aer_clear_nonfatal_status(struct pci_dev *dev) > { > int aer = dev->aer_cap; > u32 status, sev; > > if (!pcie_aer_is_native(dev)) > - return -EIO; > + return; > > /* Clear status bits for ERR_NONFATAL errors only */ > pci_read_config_dword(dev, aer + PCI_ERR_UNCOR_STATUS, &status); > @@ -265,10 +265,7 @@ int pci_aer_clear_nonfatal_status(struct pci_dev *dev) > status &= ~sev; > if (status) > pci_write_config_dword(dev, aer + PCI_ERR_UNCOR_STATUS, status); > - > - return 0; > } > -EXPORT_SYMBOL_GPL(pci_aer_clear_nonfatal_status); > > void pci_aer_clear_fatal_status(struct pci_dev *dev) > { > diff --git a/include/linux/aer.h b/include/linux/aer.h > index 154690c278cb..f638ad955deb 100644 > --- a/include/linux/aer.h > +++ b/include/linux/aer.h > @@ -44,7 +44,6 @@ struct aer_capability_regs { > /* PCIe port driver needs this function to enable AER */ > 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); > @@ -57,10 +56,6 @@ static inline int pci_disable_pcie_error_reporting(struct pci_dev *dev) > { > return -EINVAL; > } > -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;
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 >
Hello. Here comes patch v2, which contains some fixes and optimizations of aer api usage. The original version can be found on the mailing list. Changes since v1: - Modifications to comments proposed by Bjorn. Split patch into more obvious parts. Zhuo Chen (9): PCI/AER: Add pci_aer_clear_uncorrect_error_status() to PCI core PCI/DPC: Use pci_aer_clear_uncorrect_error_status() to clear uncorrectable error status NTB: Change to use pci_aer_clear_uncorrect_error_status() scsi: lpfc: Change to use pci_aer_clear_uncorrect_error_status() PCI/AER: Unexport pci_aer_clear_nonfatal_status() PCI/AER: Move check inside pcie_clear_device_status(). PCI/AER: Use pcie_aer_is_native() to judge whether OS owns AER PCI/ERR: Clear fatal status when pci_channel_io_frozen PCI/AER: Refine status clearing process with api drivers/ntb/hw/idt/ntb_hw_idt.c | 4 +-- drivers/pci/pci.c | 7 +++-- drivers/pci/pci.h | 2 ++ drivers/pci/pcie/aer.c | 45 +++++++++++++++++++-------------- drivers/pci/pcie/dpc.c | 3 +-- drivers/pci/pcie/err.c | 15 ++++------- drivers/pci/pcie/portdrv_core.c | 3 +-- drivers/scsi/lpfc/lpfc_attr.c | 4 +-- include/linux/aer.h | 4 +-- 9 files changed, 46 insertions(+), 41 deletions(-)