Received-SPF: pass (google.com: domain of linux-kernel+bounces-210726-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Tue, 11 Jun 2024 17:07:46 -0700
From: Dan Williams <dan.j.williams@intel.com>
To: Smita Koralahalli <Smita.KoralahalliChannabasappa@amd.com>,
	<linux-efi@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
	<linux-cxl@vger.kernel.org>
CC: Ard Biesheuvel <ardb@kernel.org>, Alison Schofield
	<alison.schofield@intel.com>, Vishal Verma <vishal.l.verma@intel.com>, "Ira
 Weiny" <ira.weiny@intel.com>, Dan Williams <dan.j.williams@intel.com>,
	Jonathan Cameron <Jonathan.Cameron@huawei.com>, Yazen Ghannam
	<yazen.ghannam@amd.com>, Bowman Terry <terry.bowman@amd.com>
Subject: Re: [PATCH 3/4] acpi/ghes, cxl/pci: Trace FW-First CXL Protocol
 Errors
Message-ID: <6668e6d28e9bc_310129473@dwillia2-xfh.jf.intel.com.notmuch>
References: <20240522150839.27578-1-Smita.KoralahalliChannabasappa@amd.com>
 <20240522150839.27578-4-Smita.KoralahalliChannabasappa@amd.com>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <20240522150839.27578-4-Smita.KoralahalliChannabasappa@amd.com>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?LlhkrNGIXeGSTpfen9CFOXsl0g+NKc0rZAIpWr7shNtF3QK/1fi781WlCfE+?=
 =?us-ascii?Q?0fLqXCnCHUW4CA/T1LQFoz0zsqFRI0ZgZnMgKAOQs+Jm2Sd7DCN+R8nUrPtk?=
 =?us-ascii?Q?t6rCFWWSachikt68xaggbOqK4DuFRRe1BPWpuEe2xG0eWxw8RACFKjQG036Y?=
 =?us-ascii?Q?UMawh6l/rgFSYbNcVdGSnD1TIqNn2tJuBNoRizelVBgzn1Wp8+n01fRADRw6?=
 =?us-ascii?Q?jbdw00ahuP6WEWuTshCJ+TzTjSWT4TjL0/l5sb3+Y98KJU6fQ0qIDd8RhwGb?=
 =?us-ascii?Q?0XOMn1A+v3GJSWvgh7PcGr/3Iz6I5g6GyL/i7RCOUH0eHs+fxnf/BBX0byZ8?=
 =?us-ascii?Q?ne8jCuf9yw/RAYddfS4alsN5fryXPWgrpN6922ZexY65chsN2qgYJnY99PYG?=
 =?us-ascii?Q?Qs1VIvREYDDRimzPeMEGuqBcYxUJAz1+TSAdRXwia/xDnz7Gl2iTAT9titzu?=
 =?us-ascii?Q?DSV2Y7RSxmSK3NzgjZiGfUElcRzjTp6boVgB8iYyqJAGsGQAKEzZgp01fau/?=
 =?us-ascii?Q?EwxmISd5zJj5X+D2sBzvTpaulCwrPPirm3Tq1v8RaSZLuXKfqkbW6cKpMhLM?=
 =?us-ascii?Q?c7e5OuLSSKHF2vdRBtfTXCToKLmqVgqgTxhHhZ9CfzGL1kGwyK8hvusdFIlQ?=
 =?us-ascii?Q?uLN5QfgFPjDJdeoiWsJpGCGqC+eE4VONav17pc3FoitL90PfpK76EQIdVvxd?=
 =?us-ascii?Q?Xc4Wu5GnwNfQ3wMrak7pTGC6fnklykyGkkutZX3ni6krGGQANMdfyw4aN10f?=
 =?us-ascii?Q?Ucpk7JF8JHVSc/+4eqzqMJhtLpGaZuD9XM9c9fotIS5+YfJ+hJAwpoorNfU2?=
 =?us-ascii?Q?Z4Q16jE18to5zGvxS57yCvDnN+I6eyWgnRXF4pncnzXLFn+2MnP73yBxzK8/?=
 =?us-ascii?Q?JptigNKMRQjzgFw6m2vdXTjBwqZp+zXPY7EJ5a7jsfvGlXJapqB4SkKPDWRT?=
 =?us-ascii?Q?mzxqpcwQS4Lz8TFjisvd9XkbTm13B2EBEgthfRtFnGP0LopJcZak+vfohOdj?=
 =?us-ascii?Q?KVKNr7aW5Nd8su6CxVklzfqCXYQtjgEgpJQi7gmzDx7VSXa9Ekx+nhW28Ifu?=
 =?us-ascii?Q?aRyOToNPkmldsAUT6TmugrLo0HkJpKTVkdW4AKO4NlWgQIKOalqsEZj3GqoK?=
 =?us-ascii?Q?IZ9H0DZLLomasByI4zFrvV0ScQSCmx3e1xu1AkLjJnMuTyvqGlb56TBLhDZe?=
 =?us-ascii?Q?vuVJBlA8w7UbznvYCcqk/wucayoRakFaAhn+o7/WHz3PWcljfrSoTiF+rDfq?=
 =?us-ascii?Q?xflXPCm/SZPOxhdjCPfj5sqBjyb6Ph8oeXIGyTq/I11GkEGIxfA3eNDWrcn/?=
 =?us-ascii?Q?O+e0eXk0aw4apwrOQ9wYeeIRpmVhk1y9V/vgU3NZiAvWLRu1D5s9RZaOtAue?=
 =?us-ascii?Q?B8BH7h/rMEhIbxj9MY2kFwHrcKmk5q6Q0/B8svVyR/1/KqZfkQdj14Votvtf?=
 =?us-ascii?Q?+7tXOl/slc/98ItZDeeRYpSEeP1QHoN1LY4Aky1NRYRW64NFqfBi61S2clx3?=
 =?us-ascii?Q?fhR8lY6K2PbkVO0H3Hei+IfZ899kHRTpaW2MHNp/1BOLi4SoevUWFN1COlcl?=
 =?us-ascii?Q?wFnrqzNNWUwEkt+6NzBkz6V84kYyFeCe5rERsMvfJ2GUUcKlv8dEXrPwKtGl?=
 =?us-ascii?Q?qg=3D=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: ff4fef44-c1d8-45a0-48c7-08dc8a73b1c8
X-MS-Exchange-CrossTenant-AuthSource: PH8PR11MB8107.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 12 Jun 2024 00:07:49.0149
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: QErUrp16Ugol/rFj1iZMGXXXQ/zCqCOk6kdyKfIu1gbGYzQN5uSkcTWudyf5aY1kTYIqJjv9UB3B7kFfo1W0tACeafppA27hDZWvt/o4GP4=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH7PR11MB8058
X-OriginatorOrg: intel.com

Smita Koralahalli wrote:
> When PCIe AER is in FW-First, OS should process CXL Protocol errors from
> CPER records.
> 
> Reuse the existing work queue cxl_cper_work registered with GHES to notify
> the CXL subsystem on a Protocol error.
> 
> The defined trace events cxl_aer_uncorrectable_error and
> cxl_aer_correctable_error currently trace native CXL AER errors. Reuse
> them to trace FW-First Protocol Errors.
> 
> Signed-off-by: Smita Koralahalli <Smita.KoralahalliChannabasappa@amd.com>
> ---
>  drivers/acpi/apei/ghes.c  | 14 ++++++++++++++
>  drivers/cxl/core/pci.c    | 24 ++++++++++++++++++++++++
>  drivers/cxl/cxlpci.h      |  3 +++
>  drivers/cxl/pci.c         | 34 ++++++++++++++++++++++++++++++++--
>  include/linux/cxl-event.h |  1 +
>  5 files changed, 74 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
> index 1a58032770ee..a31bd91e9475 100644
> --- a/drivers/acpi/apei/ghes.c
> +++ b/drivers/acpi/apei/ghes.c
> @@ -723,6 +723,20 @@ static void cxl_cper_handle_prot_err(struct acpi_hest_generic_data *gdata)
>  
>  	if (cxl_cper_handle_prot_err_info(gdata, &wd.p_err))
>  		return;
> +
> +	guard(spinlock_irqsave)(&cxl_cper_work_lock);
> +
> +	if (!cxl_cper_work)
> +		return;
> +
> +	wd.event_type = CXL_CPER_EVENT_PROT_ERR;
> +
> +	if (!kfifo_put(&cxl_cper_fifo, wd)) {
> +		pr_err_ratelimited("CXL CPER kfifo overflow\n");
> +		return;
> +	}
> +
> +	schedule_work(cxl_cper_work);

This seems wrong to unconditionally schedule the cxl_pci driver to look
at potentially "non-device" errors. With Terry's upcoming CXL switch
port error handling there will be a native path for those errors, but
until that arrives, I see no point in this code trying to convey
root/switch port errors to the endpoint driver.

>  }
>  
>  int cxl_cper_register_work(struct work_struct *work)
> diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
> index 0df09bd79408..ef9438cb1dd6 100644
> --- a/drivers/cxl/core/pci.c
> +++ b/drivers/cxl/core/pci.c
> @@ -686,6 +686,30 @@ void read_cdat_data(struct cxl_port *port)
>  }
>  EXPORT_SYMBOL_NS_GPL(read_cdat_data, CXL);
>  
> +void cxl_trace_prot_err(struct cxl_dev_state *cxlds,
> +			struct cxl_cper_prot_err *p_err)
> +{
> +	u32 status, fe;
> +
> +	if (p_err->severity == CXL_AER_CORRECTABLE) {
> +		status = p_err->cxl_ras.cor_status & ~p_err->cxl_ras.cor_mask;
> +
> +		trace_cxl_aer_correctable_error(cxlds->cxlmd, status);
> +	} else {
> +		status = p_err->cxl_ras.uncor_status & ~p_err->cxl_ras.uncor_mask;
> +
> +		if (hweight32(status) > 1)
> +			fe = BIT(FIELD_GET(CXL_RAS_CAP_CONTROL_FE_MASK,
> +					   p_err->cxl_ras.cap_control));
> +		else
> +			fe = status;
> +
> +		trace_cxl_aer_uncorrectable_error(cxlds->cxlmd, status, fe,
> +						  p_err->cxl_ras.header_log);
> +	}
> +}
> +EXPORT_SYMBOL_NS_GPL(cxl_trace_prot_err, CXL);
> +
>  static void __cxl_handle_cor_ras(struct cxl_dev_state *cxlds,
>  				 void __iomem *ras_base)
>  {
> diff --git a/drivers/cxl/cxlpci.h b/drivers/cxl/cxlpci.h
> index 93992a1c8eec..0ba3215786e1 100644
> --- a/drivers/cxl/cxlpci.h
> +++ b/drivers/cxl/cxlpci.h
> @@ -130,4 +130,7 @@ void read_cdat_data(struct cxl_port *port);
>  void cxl_cor_error_detected(struct pci_dev *pdev);
>  pci_ers_result_t cxl_error_detected(struct pci_dev *pdev,
>  				    pci_channel_state_t state);
> +struct cxl_cper_prot_err;
> +void cxl_trace_prot_err(struct cxl_dev_state *cxlds,
> +			struct cxl_cper_prot_err *p_err);
>  #endif /* __CXL_PCI_H__ */
> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> index 74876c9835e8..3e3c36983686 100644
> --- a/drivers/cxl/pci.c
> +++ b/drivers/cxl/pci.c
> @@ -1011,12 +1011,42 @@ static void cxl_handle_cper_event(enum cxl_event_type ev_type,
>  			       &uuid_null, &rec->event);
>  }
>  
> +static void cxl_handle_prot_err(struct cxl_cper_prot_err *p_err)

Can we call this variable @rec instead of @p_err? The data being passed
is CPER data which is a "record" structure.

> +{
> +	struct pci_dev *pdev __free(pci_dev_put) = NULL;
> +	struct cxl_dev_state *cxlds;
> +	unsigned int devfn;
> +
> +	devfn = PCI_DEVFN(p_err->device, p_err->function);
> +	pdev = pci_get_domain_bus_and_slot(p_err->segment,
> +					   p_err->bus, devfn);
> +	if (!pdev)
> +		return;
> +
> +	guard(device)(&pdev->dev);
> +	if (pdev->driver != &cxl_pci_driver)
> +		return;
> +
> +	cxlds = pci_get_drvdata(pdev);
> +	if (!cxlds)
> +		return;
> +
> +	if (((u64)p_err->upper_dw << 32 | p_err->lower_dw) != cxlds->serial)
> +		pr_warn("CPER-reported device serial number does not match expected value\n");

Not much the end user can do about this warning, I would say skip this
message, or make it a pci_dbg() because matching by BDF should be
sufficient.

> +
> +	cxl_trace_prot_err(cxlds, p_err);
> +}
> +
>  static void cxl_cper_work_fn(struct work_struct *work)
>  {
>  	struct cxl_cper_work_data wd;
>  
> -	while (cxl_cper_kfifo_get(&wd))
> -		cxl_handle_cper_event(wd.event_type, &wd.rec);
> +	while (cxl_cper_kfifo_get(&wd)) {
> +		if (wd.event_type == CXL_CPER_EVENT_PROT_ERR)
> +			cxl_handle_prot_err(&wd.p_err);
> +		else
> +			cxl_handle_cper_event(wd.event_type, &wd.rec);
> +	}
>  }
>  static DECLARE_WORK(cxl_cper_work, cxl_cper_work_fn);
>  
> diff --git a/include/linux/cxl-event.h b/include/linux/cxl-event.h
> index 9c7b69e076a0..5562844df850 100644
> --- a/include/linux/cxl-event.h
> +++ b/include/linux/cxl-event.h
> @@ -122,6 +122,7 @@ struct cxl_event_record_raw {
>  } __packed;
>  
>  enum cxl_event_type {
> +	CXL_CPER_EVENT_PROT_ERR,
>  	CXL_CPER_EVENT_GENERIC,
>  	CXL_CPER_EVENT_GEN_MEDIA,
>  	CXL_CPER_EVENT_DRAM,
> -- 
> 2.17.1
>