Received-SPF: pass (google.com: domain of linux-kernel+bounces-16050-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Wed, 3 Jan 2024 14:08:02 -0800
From: Dan Williams <dan.j.williams@intel.com>
To: Ira Weiny <ira.weiny@intel.com>, Dan Williams <dan.j.williams@intel.com>,
	Jonathan Cameron <jonathan.cameron@huawei.com>, Smita Koralahalli
	<Smita.KoralahalliChannabasappa@amd.com>, Shiju Jose <shiju.jose@huawei.com>
CC: Yazen Ghannam <yazen.ghannam@amd.com>, Davidlohr Bueso
	<dave@stgolabs.net>, Dave Jiang <dave.jiang@intel.com>, Alison Schofield
	<alison.schofield@intel.com>, Vishal Verma <vishal.l.verma@intel.com>, "Ard
 Biesheuvel" <ardb@kernel.org>, <linux-efi@vger.kernel.org>,
	<linux-kernel@vger.kernel.org>, <linux-cxl@vger.kernel.org>, Ira Weiny
	<ira.weiny@intel.com>
Subject: RE: [PATCH v5 9/9] cxl/pci: Register for and process CPER events
Message-ID: <6595dac27b081_8dc68294e3@dwillia2-xfh.jf.intel.com.notmuch>
References: <20231220-cxl-cper-v5-0-1bb8a4ca2c7a@intel.com>
 <20231220-cxl-cper-v5-9-1bb8a4ca2c7a@intel.com>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <20231220-cxl-cper-v5-9-1bb8a4ca2c7a@intel.com>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?rhdYlanWqMhfSZsbF/IRBRrL4sSfUiUTHHKRaDeEwfWp6VtILx7WedY4iWcF?=
 =?us-ascii?Q?/5gEkDnXG+GNZK+JXfSDI6dMiNcAWCA+yIuRS3zOYjpAhr/yjbur3MyQ0vdU?=
 =?us-ascii?Q?Fz0QowOoxmj2Wt4w8LSNCHeGJukZn16iqXAnJGPTBv2xWVxvWV2898zecdyv?=
 =?us-ascii?Q?bxGwUVJlcn25H4d8Lq7NhGnwFOB9YXNxi0F30K/UCtpI+KtHSSQ5o0g4sRun?=
 =?us-ascii?Q?WzuFdcyJHwkQt/IoQq7G1tAHBi7NE8HmakDcio4i/3vVzwDQozVrqsuET+wW?=
 =?us-ascii?Q?aeAMd5RqQ9QFkW09rqfrG7eh1828zX05HmwNjaS9PxKr+XPuXcJ8eYbYhx5Y?=
 =?us-ascii?Q?ukSsXtXy4J+MRtWMSBPnwFzr/F4Iiv8ABDAwLnkCPPe+C5SUVshEs7UCra4x?=
 =?us-ascii?Q?dEZF4e+2yj/VifmtiJhALFVYuj6e6wNqKQINLZMI80sciBWx97GOUrD5Cmsd?=
 =?us-ascii?Q?bPVixOuwfri+Evj08zqG5SC6KfkZi4LeqOdQ9aPxnWiRcaVUDnAcsXw6ClYw?=
 =?us-ascii?Q?d/AFkWjMEPxTiSeHf10AZbV6OK0HiC5T+Q8SSWHH3QXoI1PbciMGjGHoMqgC?=
 =?us-ascii?Q?QV2MeFl1w3OdbElazI8WEzxt8WZr+gmfvbzdX/9LDzQaX9nqu8tjQ97kHxtH?=
 =?us-ascii?Q?2hbxXsHz9woFXzr4zwe6rHhz9JCJ4fyX90i3jd4jeAAvbhxleyp+EU5cCeTv?=
 =?us-ascii?Q?V3ROxUfa/Hk9ZKh+e2GuYNsr4IeBv9Jua294BSKXSpEecjXc2QIEiXQQyma+?=
 =?us-ascii?Q?2keeGhmqXgr3VJ/Z5ADwB5YlBzh5AmOv7AVa4V5++/P3v2LSTJfftvbAFJf0?=
 =?us-ascii?Q?1VnKoytuhfzj6C29NqDHKQfzxyNGe/etbVUL8G65pfa+BdAjactnUZI8bmut?=
 =?us-ascii?Q?o9yEMOR6sHj7NserF1RkVsdczfptZB1K+n5okz07FeXujDcUNiDOjNIY8AA4?=
 =?us-ascii?Q?4LMiqJnAtEeg96fnJk5RlQWgkCddY5i3FOhk6LxS27wOvxcRmryFG0FmSdEi?=
 =?us-ascii?Q?5W5s/HTyuHXco0U1NN4xDG0M5Nr2xWir+rpVkoJUBDWR30ubGLCGjv2N7q5k?=
 =?us-ascii?Q?6HUHvWyOa+6qpdsvF4QGJpzzEJIXJu23NgfypRDN5dNhYalDtJ5kbOwf1bY4?=
 =?us-ascii?Q?qevoamcmIUxt4875tjxUsDW3qK1H3MBLtHeE01+DJNxkHTPf2JIcp+x8tgPf?=
 =?us-ascii?Q?Wc08Arp+sQ6HNb8/eWXULjBFKCfOh/ktNOjhvZelrSD79YkaGtkUSGmMjr/l?=
 =?us-ascii?Q?hQouoAfLTJbPqgrkxHsSLhO/7dqaFp7nU0yJSRhhlGfvA9mZ/unxCoqpxHsE?=
 =?us-ascii?Q?yZtIjHMsKuJmae14iOmW1ImdoIhx7BE28RqVgB3qbwvkAqvDYZRuywNq7ihl?=
 =?us-ascii?Q?JJwDDcOCaM6+Dpf+Xq5C1S3RmLaHTBOSDnilh81yIg6afUfy7YfM9Gw55Ia3?=
 =?us-ascii?Q?rZDQpRD5Gacvzbgr4O4sA/PWc6w6nDo49a9q0ZL3jByDUkfG4Y8+DAxKHaFx?=
 =?us-ascii?Q?dGOoWcS/qedbQ8hYytm8rht1AaACs3QN4Dzr9MMs1EbVtHLncMRsjXZMEGUp?=
 =?us-ascii?Q?MusbrsYva0PRBzpR/rELaR/DUQm6Rw5MT9V7bhklMr/zZ1W/vQRGORiwIOV6?=
 =?us-ascii?Q?CA=3D=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 5655ab00-9680-4298-94f4-08dc0ca8764b
X-MS-Exchange-CrossTenant-AuthSource: PH8PR11MB8107.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 03 Jan 2024 22:08:05.9976
 (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: EsOeR3ik4ZLy7wfO1327RevMbEKu1DxO/UcBdDkllAuf69gMVSaVfJXwASpmY4YwiJnKDQsvY4dk+WYhjwU4HkmYxz3mjHt7PjTvyNY9NAQ=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: IA1PR11MB6537
X-OriginatorOrg: intel.com

Ira Weiny wrote:
> If the firmware has configured CXL event support to be firmware first
> the OS can process those events through CPER records.  The CXL layer has
> unique DPA to HPA knowledge and standard event trace parsing in place.
> 
> CPER records contain Bus, Device, Function information which can be used
> to identify the PCI device which is sending the event.
> 
> Change the PCI driver registration to include registration of a CXL
> CPER callback to process events through the trace subsystem.
> 
> Use new scoped based management to simplify the handling of the PCI
> device object.
> 
> NOTE this patch depends on Dan's addition of a device guard[1].

Now that you added guard(pci_dev) earlier in the series you can just use
that here rather than guard(device).

[..]
> +#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 cper_cxl_event_devid *device_id = &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;
> +	unsigned int devfn;
> +	u32 hdr_flags;
> +
> +	devfn = PCI_DEVFN(device_id->device_num, device_id->func_num);
> +	pdev = pci_get_domain_bus_and_slot(device_id->segment_num,
> +					   device_id->bus_num, devfn);
> +	if (!pdev)
> +		return;
> +
> +	guard(device)(&pdev->dev);

Per above:
	guard(pci_dev)(pdev);

> +	if (pdev->driver != &cxl_pci_driver)
> +		return;
> +
> +	cxlds = pci_get_drvdata(pdev);
> +	if (!cxlds)
> +		return;
> +
> +	/* Fabricate a log type */
> +	hdr_flags = get_unaligned_le24(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);
> +}
> +
> +static int __init cxl_pci_driver_init(void)
> +{
> +	int rc;
> +
> +	rc = pci_register_driver(&cxl_pci_driver);
> +	if (rc)
> +		return rc;
> +
> +	rc = cxl_cper_register_callback(cxl_cper_event_call);
> +	if (rc)
> +		pci_unregister_driver(&cxl_pci_driver);

I think this order should be flipped. That way any errors that might
arrive due to activity caused by probing have a chance to be serviced.
Any that fire while initial probing is happening will pause in
cxl_cper_event_call() and wait for probing to complete. Of course if
probing fails, all is lost, but I think there is some incremental
benefit to trying to catch those early records for things that are not
probing fatal.