Received-SPF: pass (google.com: domain of linux-kernel+bounces-6097-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Tue, 19 Dec 2023 15:27:05 -0800
From: Ira Weiny <ira.weiny@intel.com>
To: Jonathan Cameron <Jonathan.Cameron@huawei.com>, Ira Weiny
	<ira.weiny@intel.com>
CC: Dan Williams <dan.j.williams@intel.com>, Smita Koralahalli
	<Smita.KoralahalliChannabasappa@amd.com>, Shiju Jose <shiju.jose@huawei.com>,
	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>, Bjorn Helgaas
	<bhelgaas@google.com>
Subject: Re: [PATCH v4 7/7] cxl/memdev: Register for and process CPER events
Message-ID: <658226c9a49f3_277bd294fb@iweiny-mobl.notmuch>
References: <20231215-cxl-cper-v4-0-01b6dab44fcd@intel.com>
 <20231215-cxl-cper-v4-7-01b6dab44fcd@intel.com>
 <20231219143732.0000181e@Huawei.com>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <20231219143732.0000181e@Huawei.com>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?6kzoraZoV0Dzho+k7YsU/OXXYVUbAXaDdXh3Hc4k77kA++gg18YXVAdZuSPB?=
 =?us-ascii?Q?G8ABTBc6JDvQ7ZVOlTyYdY0F/v24CCKmTZcJ2ouxIMNY27rrMXCvgW9CYwIF?=
 =?us-ascii?Q?zUY4TcS671IA6wtuJYaRlrOQZ+hjesvn8L2dJtj/8FtithLCttR0IofVuPUJ?=
 =?us-ascii?Q?SznnwH7T+j8/f5CwnEL5BryPjL6ltAmXbe1j9ZqLJfZcWnYWxQIBVFGmvhK5?=
 =?us-ascii?Q?/FzKqiUs7vmVn1s6cCsjDgLRnf/mYKvzifLS5VzEc8BjRzwTkx3D+wFR3ugN?=
 =?us-ascii?Q?3BTryqetRIpPFINuR17U7VHf2kFxoWQIZDIhejybdSJu/hwoN+jonBluvjhI?=
 =?us-ascii?Q?zJer60MIQzArza/Swit+v7MRgAhNI4vc2zNb+bg9x59pNWL6VluENcb47SMY?=
 =?us-ascii?Q?PbBRv4SHUKEcKY/7v4rBKsUGioVVIOmNYT2u8Di7H2RIb0P5RAB75JYxc0K2?=
 =?us-ascii?Q?b5bEFPvvtcOAShi95UlkbZoKv1SBOmsGvj0NM97EZ1v5Xr6inSYsVAWfNIM+?=
 =?us-ascii?Q?Jjx4Toe9B5dDh9aFEtqRQnT3PvVlfv7r81uQiSb8PTVDLxE344CaJuCB58LY?=
 =?us-ascii?Q?X6LsuDAIktZCV5F1JxSYhk5t2XQ4YX+O6zyIflHqSA+gr7H4x6+USRG3/q+T?=
 =?us-ascii?Q?kA2V+wWA6lteLHFhuzru/k/JBLwosE2xJvlATMBXMYKSnL96a1eHeIj7HLps?=
 =?us-ascii?Q?PWv4oHnETEVMoo0DOSlA57ZXSPP+8gF/Nsi1wqd6L0OKDrQgj1UMSmfhZhzZ?=
 =?us-ascii?Q?Oh47t9HCJJr0va2hJ/u8x1lHyzexK5ktjVk+DgAVkgrIgO077IRH3pSXFDZ4?=
 =?us-ascii?Q?y9HYN2M21FkIQkAP6dekz38ypO1RlT+NfqaVwWcHwZtSdad7VUJrMbac6zjw?=
 =?us-ascii?Q?W7CfmOC08N+5HQgtbVYmOdZc9wGWUpHf1lXQD0HD0udCH3sgG262FVYjTzl/?=
 =?us-ascii?Q?DeXM4kvfsJT9b4kAr/Qy/FhujUImlofGjJh7LBZt8U1M+vyP2GNRT+Y559nj?=
 =?us-ascii?Q?bd6IgdvMjhN+JxFA2LpMRNDvGj2mrLy///DWr9+uAZIkL3vl7OycRjXmn1pX?=
 =?us-ascii?Q?SWO2/apX3kTzdj1vCK8U2lM3eZ565143sP07vGWlRI96AvH5Kx2K6USGi/In?=
 =?us-ascii?Q?ZO9BhMh1WEoApndyToPAlxg2MXu67u49Xkr9buoYKqlJBciukKljsyAMfoaN?=
 =?us-ascii?Q?u8kCECDIaRXx3Nr6k5dsfsVrkIlpivoBYqxnVmLBJhYrGabceKwfQkLlIMXj?=
 =?us-ascii?Q?lpb4lddX3N9WJRFkiid/e/kKZ+Mfpximpv8SWScg9HHWovEkxxfEHLdOllqj?=
 =?us-ascii?Q?vVUjgvEpIeCr2eC+pfH6/Q1EZCed0j/bQwXt6jDbEwc4OZkLqCS8SZKeq/WN?=
 =?us-ascii?Q?BE8MWBoJKRDjs5VLLHa3558iJJt2XVytwSmOfLXb3NhvOSlO3Lnk4A/zRnYy?=
 =?us-ascii?Q?68XbqZYjaBO0J59I3P4hYTZt+B3iHcXXPr7l+IymVjfUSCe5HKEdl6AiJYYP?=
 =?us-ascii?Q?u30z/HmOStG7ujy22g1E7gQ5fkvzf40uwIGid49I7J8Si5v3cfS7F3z+ms2+?=
 =?us-ascii?Q?noS2ChU0Hwj2VaW2q8SZ8SR1pK5SbbShOSnUTf8RDtHhK0Ogzzuv6U/+97Xy?=
 =?us-ascii?Q?UCXOdEfd3lB5Qf7UBAQuIY0NpO1KJYelyQs+HPvQwOxA?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 4f4da181-0ec5-468d-ad9b-08dc00ea06fd
X-MS-Exchange-CrossTenant-AuthSource: SA1PR11MB6733.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 19 Dec 2023 23:27:12.1719
 (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: CHVOtIXp0NFgye5Rt4cqVzJuqQAv90USZv+oY+3eggAsLFtAgiIyQPYsH6H0iglKShvfXEmRm3vae+ivzM8lwA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: IA1PR11MB6076
X-OriginatorOrg: intel.com

Jonathan Cameron wrote:
> On Fri, 15 Dec 2023 15:26:33 -0800
> Ira Weiny <ira.weiny@intel.com> 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 pci driver registration to include registration for a CXL CPER
> > notifier to process the events through the trace subsystem.
> > 
> > Define and use scoped based management to simplify the handling of the
> > pci device object.
> > 
> > Cc: Bjorn Helgaas <bhelgaas@google.com>
> > Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> 
> I'd break out the pci guard stuff as a precursor patch.  That's likely
> to be used elsewhere so it would help for backporting for other users
> if it wasn't buried in a patch doing other stuff.

That is good.  I've done that.

> 
> Not to mention that has a different set of likely reviewers to the rest
> of this patch.

Yep.

> 
> More generally maybe we should just hardcode the UUID in the tracepoint
> definitions?  I think for everything other than the generic one we
> only ever call trace_cxl_memory_module(... &mem_mod_event_uuid..)
> etc.
> 
> It's a little ugly to match on the UUID to call a function where it
> hard coded, but less so than inserting the UUID like this does.
> Better I think to make it obvious that this isn't actually a variable
> (for the ones we understand).

I thought about that a bit.  But I built the tracing code with generic
macros which contained the UUID.  That complicated my efforts.

I've reworked it again and it took a bit of time but I got it to work.  It
was not that hard but there is a caveat in the generic macros, which I
made a note of.

[snip]

> >  
> > +#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;
> > +	struct cxl_dev_state *cxlds = NULL;
> > +	enum cxl_event_log_type log_type;
> > +	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);
> > +	if (pdev->driver == &cxl_pci_driver)
> > +		cxlds = pci_get_drvdata(pdev);
> > +	if (!cxlds)
> > +		return;
> 
> This is handling two conditions. I'd find it more readable split like:
> 
> 	if (pdev->driver != &cxl_pci_driver)
> 		return;
> 
> 	cxlds = pci_get_drvdata(pdev);
> 	if (!cxlds)
> 		return;
> 
> and drop the = NULL above.

Done.

Ira