Received-SPF: pass (google.com: domain of linux-kernel+bounces-66844-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Thu, 15 Feb 2024 11:56:46 +0000
From: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
To: Smita Koralahalli <Smita.KoralahalliChannabasappa@amd.com>
CC: <linux-efi@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
	<linux-cxl@vger.kernel.org>, 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>, Yazen Ghannam <yazen.ghannam@amd.com>
Subject: Re: [PATCH v2 1/4] acpi/ghes, cxl: Create a common CXL struct to
 handle different CXL CPER records
Message-ID: <20240215115646.000038d6@Huawei.com>
In-Reply-To: <20240109034755.100555-2-Smita.KoralahalliChannabasappa@amd.com>
References: <20240109034755.100555-1-Smita.KoralahalliChannabasappa@amd.com>
	<20240109034755.100555-2-Smita.KoralahalliChannabasappa@amd.com>
Organization: Huawei Technologies Research and Development (UK) Ltd.
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset="US-ASCII"
Content-Transfer-Encoding: 7bit

On Tue, 9 Jan 2024 03:47:52 +0000
Smita Koralahalli <Smita.KoralahalliChannabasappa@amd.com> wrote:

> Currently defined cxl_cper_callback interface between CXL subsystem and
> GHES module is just confined to handling CXL Component errors only.
> 
> Extend this callback to process CXL Protocol errors as well. Achieve
> by defining a new struct cxl_cper_event_info to include cxl_cper_event_rec
> and other fields of CXL protocol errors which will be defined in future
> patches.
> 
> Signed-off-by: Smita Koralahalli <Smita.KoralahalliChannabasappa@amd.com>
Hi Smita,

I guess this will get effected by the mess around the reporting that
Ira is fixing but in meantime some comments on the current code.
> ---
> v2:
> 	cxl_cper_rec_data -> cxl_cper_event_info
> 	data -> info
> ---
>  drivers/acpi/apei/ghes.c  | 6 +++++-
>  drivers/cxl/pci.c         | 8 ++++----
>  include/linux/cxl-event.h | 6 +++++-
>  3 files changed, 14 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
> index aed465d2fd68..60b615d361d3 100644
> --- a/drivers/acpi/apei/ghes.c
> +++ b/drivers/acpi/apei/ghes.c
> @@ -693,6 +693,10 @@ static cxl_cper_callback cper_callback;
>  static void cxl_cper_post_event(enum cxl_event_type event_type,
>  				struct cxl_cper_event_rec *rec)
>  {
> +	struct cxl_cper_event_info info;
> +
> +	info.rec = *(struct cxl_cper_event_rec *)rec;

Why cast?

> +
>  	if (rec->hdr.length <= sizeof(rec->hdr) ||
>  	    rec->hdr.length > sizeof(*rec)) {
>  		pr_err(FW_WARN "CXL CPER Invalid section length (%u)\n",
> @@ -707,7 +711,7 @@ static void cxl_cper_post_event(enum cxl_event_type event_type,
>  
>  	guard(rwsem_read)(&cxl_cper_rw_sem);
>  	if (cper_callback)
> -		cper_callback(event_type, rec);
> +		cper_callback(event_type, &info);
>  }
>  
>  int cxl_cper_register_callback(cxl_cper_callback callback)
> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> index b14237f824cf..1ad240ead4fd 100644
> --- a/drivers/cxl/pci.c
> +++ b/drivers/cxl/pci.c
> @@ -972,9 +972,9 @@ static struct pci_driver cxl_pci_driver = {
>  
>  #define CXL_EVENT_HDR_FLAGS_REC_SEVERITY GENMASK(1, 0)
>  static void cxl_cper_event_call(enum cxl_event_type ev_type,
> -				struct cxl_cper_event_rec *rec)
> +				struct cxl_cper_event_info *info)
>  {
> -	struct cper_cxl_event_devid *device_id = &rec->hdr.device_id;
> +	struct cper_cxl_event_devid *device_id = &info->rec.hdr.device_id;
>  	struct pci_dev *pdev __free(pci_dev_put) = NULL;
>  	enum cxl_event_log_type log_type;
>  	struct cxl_dev_state *cxlds;
> @@ -996,11 +996,11 @@ static void cxl_cper_event_call(enum cxl_event_type ev_type,
>  		return;
>  
>  	/* Fabricate a log type */
> -	hdr_flags = get_unaligned_le24(rec->event.generic.hdr.flags);
> +	hdr_flags = get_unaligned_le24(info->rec.event.generic.hdr.flags);
>  	log_type = FIELD_GET(CXL_EVENT_HDR_FLAGS_REC_SEVERITY, hdr_flags);
>  
>  	cxl_event_trace_record(cxlds->cxlmd, log_type, ev_type,
> -			       &uuid_null, &rec->event);
> +			       &uuid_null, &info->rec.event);
>  }
>  
>  static int __init cxl_pci_driver_init(void)
> diff --git a/include/linux/cxl-event.h b/include/linux/cxl-event.h
> index 17eadee819b6..6ce839c59749 100644
> --- a/include/linux/cxl-event.h
> +++ b/include/linux/cxl-event.h
> @@ -141,8 +141,12 @@ struct cxl_cper_event_rec {
>  	union cxl_event event;
>  } __packed;
>  
> +struct cxl_cper_event_info {
> +	struct cxl_cper_event_rec rec;

Only parts of this will be relevant to the protocol errors.
Maybe worth doing a union with the first part of rec in both
structures but not the union cxl_event in the protocol error.
Keep it all anonymous to avoid yet another structure in the
reads/and writes though.

> +};
> +
>  typedef void (*cxl_cper_callback)(enum cxl_event_type type,
> -				  struct cxl_cper_event_rec *rec);
> +				  struct cxl_cper_event_info *info);
>  
>  #ifdef CONFIG_ACPI_APEI_GHES
>  int cxl_cper_register_callback(cxl_cper_callback callback);