linux-kernel - Re: [PATCH v3 7/7] acpi/ghes, cxl/pci: Process CXL CPER Protocol Errors

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <674e01034ca7a_3cb8e0294b0@iweiny-mobl.notmuch>
Date: Mon, 2 Dec 2024 12:48:35 -0600
From: Ira Weiny <ira.weiny@...el.com>
To: Smita Koralahalli <Smita.KoralahalliChannabasappa@....com>, "Jonathan
 Cameron" <Jonathan.Cameron@...wei.com>
CC: <linux-efi@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
	<linux-cxl@...r.kernel.org>, Ard Biesheuvel <ardb@...nel.org>, "Alison
 Schofield" <alison.schofield@...el.com>, Vishal Verma
	<vishal.l.verma@...el.com>, Ira Weiny <ira.weiny@...el.com>, Dan Williams
	<dan.j.williams@...el.com>, Yazen Ghannam <yazen.ghannam@....com>, "Terry
 Bowman" <terry.bowman@....com>
Subject: Re: [PATCH v3 7/7] acpi/ghes, cxl/pci: Process CXL CPER Protocol
 Errors

Smita Koralahalli wrote:
> On 11/26/2024 8:05 AM, Jonathan Cameron wrote:
> > On Tue, 19 Nov 2024 00:39:15 +0000
> > Smita Koralahalli <Smita.KoralahalliChannabasappa@....com> wrote:
> > 
> >> When PCIe AER is in FW-First, OS should process CXL Protocol errors from
> >> CPER records. Introduce support for handling and logging CXL Protocol
> >> errors.
> >>
> >> The defined trace events cxl_aer_uncorrectable_error and
> >> cxl_aer_correctable_error trace native CXL AER endpoint errors, while
> >> cxl_cper_trace_corr_prot_err and cxl_cper_trace_uncorr_prot_err
> >> trace native CXL AER port errors. Reuse both sets to trace FW-First
> >> protocol errors.
> >>
> >> Since the CXL code is required to be called from process context and
> >> GHES is in interrupt context, use workqueues for processing.
> >>
> >> Similar to CXL CPER event handling, use kfifo to handle errors as it
> >> simplifies queue processing by providing lock free fifo operations.
> >>
> >> Add the ability for the CXL sub-system to register a workqueue to
> >> process CXL CPER protocol errors.
> >>
> >> Signed-off-by: Smita Koralahalli <Smita.KoralahalliChannabasappa@....com>
> > 
> > A few minor comments inline.
> > 
> > Thanks
> > 
> > Jonathan
> > 
> >> diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
> >> index 4ede038a7148..c992b34c290b 100644
> >> --- a/drivers/cxl/core/pci.c
> >> +++ b/drivers/cxl/core/pci.c
> >> @@ -650,6 +650,56 @@ void read_cdat_data(struct cxl_port *port)
> >>   }
> >>   EXPORT_SYMBOL_NS_GPL(read_cdat_data, CXL);
> >>   
> >> +void cxl_cper_trace_corr_prot_err(struct pci_dev *pdev, bool flag,
> >> +				  struct cxl_ras_capability_regs ras_cap)
> >> +{
> >> +	struct cxl_dev_state *cxlds;
> >> +	u32 status;
> >> +
> >> +	status = ras_cap.cor_status & ~ras_cap.cor_mask;
> >> +
> >> +	if (!flag) {
> > 
> > As below. Name of flag is not very helpful when reading the code.
> > Perhaps we can rename?
> 
> Okay. May be flag -> is_device_error ?

I had the same question about 'flag'.

> > 
> >> +		trace_cxl_port_aer_correctable_error(&pdev->dev, status);
> >> +		return;
> >> +	}
> >> +
> >> +	cxlds = pci_get_drvdata(pdev);
> >> +	if (!cxlds)
> >> +		return;
> >> +
> >> +	trace_cxl_aer_correctable_error(cxlds->cxlmd, status);
> >> +}
> >> +EXPORT_SYMBOL_NS_GPL(cxl_cper_trace_corr_prot_err, CXL);
> >> +
> >> +void cxl_cper_trace_uncorr_prot_err(struct pci_dev *pdev, bool flag,
> >> +				    struct cxl_ras_capability_regs ras_cap)
> >> +{
> >> +	struct cxl_dev_state *cxlds;
> >> +	u32 status, fe;
> >> +
> >> +	status = ras_cap.uncor_status & ~ras_cap.uncor_mask;
> >> +
> >> +	if (hweight32(status) > 1)
> >> +		fe = BIT(FIELD_GET(CXL_RAS_CAP_CONTROL_FE_MASK,
> >> +				   ras_cap.cap_control));
> >> +	else
> >> +		fe = status;
> >> +
> >> +	if (!flag) {
> > 
> > Why does  a bool named flag indicate it's a port error?
> 
> I will rename it.
> 
> Or may be use an enum to explicitly define the error type 
> (CXL_ERROR_TYPE_DEVICE and CXL_ERROR_TYPE_PORT).
> 
> Or may be split the function into two distinct ones, one for port errors 
> and one for device errors.

I would vote for 2 functions.
Ira