On Fri, Feb 02, 2024 at 10:01:40AM -0800, Dan Williams wrote:
> Wang, Qingshun wrote:
> > Fetch and store the data of 3 more registers: "Link Status", "Device
> > Control 2", and "Advanced Error Capabilities and Control". This data is
> > needed for external observation to better understand ANFE.
> >
> > Signed-off-by: "Wang, Qingshun" <qingshun.w...@linux.intel.com>
> > ---
> > drivers/acpi/apei/ghes.c | 8 +++++++-
> > drivers/cxl/core/pci.c | 11 ++++++++++-
> > drivers/pci/pci.h | 4 ++++
> > drivers/pci/pcie/aer.c | 26 ++++++++++++++++++++------
> > include/linux/aer.h | 6 ++++--
> > 5 files changed, 45 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
> > index 6034039d5cff..047cc01be68c 100644
> > --- a/drivers/acpi/apei/ghes.c
> > +++ b/drivers/acpi/apei/ghes.c
> > @@ -594,7 +594,9 @@ static void ghes_handle_aer(struct
> > acpi_hest_generic_data *gdata)
> > if (pcie_err->validation_bits & CPER_PCIE_VALID_DEVICE_ID &&
> > pcie_err->validation_bits & CPER_PCIE_VALID_AER_INFO) {
> > struct pcie_capability_regs *pcie_caps;
> > + u16 device_control_2 = 0;
> > u16 device_status = 0;
> > + u16 link_status = 0;
> > unsigned int devfn;
> > int aer_severity;
> > u8 *aer_info;
> > @@ -619,7 +621,9 @@ static void ghes_handle_aer(struct
> > acpi_hest_generic_data *gdata)
> >
> > if (pcie_err->validation_bits & CPER_PCIE_VALID_CAPABILITY) {
> > pcie_caps = (struct pcie_capability_regs
> > *)pcie_err->capability;
> > + device_control_2 = pcie_caps->device_control_2;
> > device_status = pcie_caps->device_status;
> > + link_status = pcie_caps->link_status;
> > }
> >
> > aer_recover_queue(pcie_err->device_id.segment,
> > @@ -627,7 +631,9 @@ static void ghes_handle_aer(struct
> > acpi_hest_generic_data *gdata)
> > devfn, aer_severity,
> > (struct aer_capability_regs *)
> > aer_info,
> > - device_status);
> > + device_status,
> > + link_status,
> > + device_control_2);
> > }
> > #endif
> > }
> > diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
> > index 9111a4415a63..3aa57fe8db42 100644
> > --- a/drivers/cxl/core/pci.c
> > +++ b/drivers/cxl/core/pci.c
> > @@ -903,7 +903,9 @@ static void cxl_handle_rdport_errors(struct
> > cxl_dev_state *cxlds)
> > struct aer_capability_regs aer_regs;
> > struct cxl_dport *dport;
> > struct cxl_port *port;
> > + u16 device_control_2;
> > u16 device_status;
> > + u16 link_status;
> > int severity;
> >
> > port = cxl_pci_find_port(pdev, &dport);
> > @@ -918,10 +920,17 @@ static void cxl_handle_rdport_errors(struct
> > cxl_dev_state *cxlds)
> > if (!cxl_rch_get_aer_severity(&aer_regs, &severity))
> > return;
> >
> > + if (pcie_capability_read_word(pdev, PCI_EXP_DEVCTL2, &device_control_2))
> > + return;
> > +
> > if (pcie_capability_read_word(pdev, PCI_EXP_DEVSTA, &device_status))
> > return;
> >
> > - pci_print_aer(pdev, severity, &aer_regs, device_status);
> > + if (pcie_capability_read_word(pdev, PCI_EXP_LNKSTA, &link_status))
> > + return;
> > +
> > + pci_print_aer(pdev, severity, &aer_regs, device_status,
> > + link_status, device_control_2);
>
> Rather than complicate the calling convention of pci_print_aer(), update
> the internals of pci_print_aer() to get these extra registers, or
> provide a new wrapper interface that satisfies the dependencies and
> switch users over to that. Otherwise multiple touches of the same code
> path in one patch set is indicative of the need for a higher level
> helper.
Thanks for the advice, it does make sense. Will reconsider the
implementation.
--
Best regards,
Wang, Qingshun
