Re: [PATCH v2 3/4] PCI/AER: Fetch information for FTrace

[Wang, Qingshun]

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.wang@xxxxxxxxxxxxxxx>
> > ---
> >  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