Received-SPF: pass (google.com: domain of linux-kernel+bounces-154476-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Date: Mon, 22 Apr 2024 20:47:12 -0700
From: Ira Weiny <ira.weiny@intel.com>
To: Dan Williams <dan.j.williams@intel.com>, Ira Weiny <ira.weiny@intel.com>,
	Dave Jiang <dave.jiang@intel.com>, Jonathan Cameron
	<jonathan.cameron@huawei.com>, Smita Koralahalli
	<Smita.KoralahalliChannabasappa@amd.com>, Shiju Jose <shiju.jose@huawei.com>
CC: Dan Carpenter <dan.carpenter@linaro.org>, Yazen Ghannam
	<yazen.ghannam@amd.com>, Davidlohr Bueso <dave@stgolabs.net>, "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>, "Rafael J.
 Wysocki" <rafael@kernel.org>, Tony Luck <tony.luck@intel.com>, "Borislav
 Petkov" <bp@alien8.de>
Subject: RE: [PATCH v2 1/3] acpi/ghes: Process CXL Component Events
Message-ID: <66272f40420e6_bbee62945d@iweiny-mobl.notmuch>
References: <20240422-cxl-cper3-v2-0-5cdd378fcd0b@intel.com>
 <20240422-cxl-cper3-v2-1-5cdd378fcd0b@intel.com>
 <6626f8c9404f2_690a294d8@dwillia2-xfh.jf.intel.com.notmuch>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <6626f8c9404f2_690a294d8@dwillia2-xfh.jf.intel.com.notmuch>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?w7ps7i9NEvUceqKpsyjBSp2tN9qxZq61F0EmQkKmvGMC2kJ290+pTNMib4Sm?=
 =?us-ascii?Q?8NigsCOQgknK484lH61Hv+goyGPnTea7qvcE2yXdqL9KGfBEMoNF2QtaSNdh?=
 =?us-ascii?Q?skC0p8vSSMDxepEru98VmYMHc6fiXpawxOFdaPb/kLSy+R+9N0di3MI5Y5Yp?=
 =?us-ascii?Q?q5vRu6+hr09DEaipg4Pk9Om2p5oD6hbgXR5lxdlQvhZxOVqRx86W2EoF8KlC?=
 =?us-ascii?Q?V2AnYMDvHd/mew3njdgoAIchtT6PdbS/ki0o15FnNScv/JaI5R64lSIbgQmh?=
 =?us-ascii?Q?MvOictNmYeoStG3ysL5nC1eXak6e0zOO7PRW0H+K2sPZwMRgFZGBZYfWbF0m?=
 =?us-ascii?Q?oftEppKcfaYWaCB15tES4GPTFJurkW/UOVSMs3KfC5UIavE4hsZImL2a/ncc?=
 =?us-ascii?Q?Tsdoh71XTEPAPRYnzSBfvcxhVEuc4FsGq1ol9v0xejEigBUM/6zGfGT1v1Pg?=
 =?us-ascii?Q?jlGdl3zBPCKB2AhVzdF5UoQpCJmKHy5aGLft5ytLUHU4uldvVp49n3h4kwna?=
 =?us-ascii?Q?pEjmLiD2q0Y4fbq0/O7jkrjGOucPetl2HKjO3ZjvRbc6vWC0FTSCbWoM+y6o?=
 =?us-ascii?Q?CvR/TqI9hBHuvELWAAUHiq953NkWvCBjGawTnIAkr4hODnDL2e6LCC2L1w2u?=
 =?us-ascii?Q?M8jgt/Yn8VXebDTSPSJN0EuAb+CwTDwwjxDA0bRc3h/8ZpJb+I0H1cpGMq2m?=
 =?us-ascii?Q?pv5v4BoQFg1VMaKRzM41GroNXhym9I6wZJ/z/c+j0o5a06OdCPLy+b+1A5kI?=
 =?us-ascii?Q?d6xlf12yOm6XRm6cOu7S0kGXDk/FGCJ/TbyOpoWKhPIedXLeMfpLUsG/5VzD?=
 =?us-ascii?Q?55y1gTGzxvzQdhMBYU2lkR9b6EOD80gyYSiAcEykfHepn8spNSHAeaPDoYIf?=
 =?us-ascii?Q?FSgoCHrxcmhrSZEIr3cahs/7eVaCfUS5Khc95+GVPYkVWZ4CPhDe5M1U8q4H?=
 =?us-ascii?Q?MgwRs469TaErBcDEusJSWzLfbYRSk2wbMxhkBc8wbJEBlJosS+gngqaHSObp?=
 =?us-ascii?Q?lJOaOwm/BVfyT3vpj2MC72Xvx/cPJSJ4epsBxUoZNumq6t7WjhB1/4MZdyaT?=
 =?us-ascii?Q?2rrzxzt5bp30lWThcUiRobtnqdGPqAuSHqrUVYK8nIrxk0tdrSiK9+Dq6MXn?=
 =?us-ascii?Q?zqGNkE4L0kc0Er8QmwlFZcK2qBzVGhD18HVB3p1pAtHeDTReikMmSIMOqUKk?=
 =?us-ascii?Q?gtW+lqvXXspUKyURXva2ear1vVpkq46LyqoGs3QF7LckIqkUho48DSHaDCpI?=
 =?us-ascii?Q?8RKTVI+xwAdcT/AmswdQIPv5vTEA9JGsfQUWnn5GSGYCHjCN0MkmLOPzG7xl?=
 =?us-ascii?Q?Og5fsgPXroiaZYUQ84KxSpc/n/xUMhWbiW/BQVsZqcRQISKxzt7ZEAhDOYPW?=
 =?us-ascii?Q?MPs0ZStbT7mWIhupIvnPjQ1+Wbrsc+bqktvjdgCJ7VqVikBEK4yA67khk8Ry?=
 =?us-ascii?Q?w8ZYgaePjksIU5g/acOJ+U+y160HaViCs0NO8Sem/oxyio5sjwVb2iJIODCS?=
 =?us-ascii?Q?iWdHKx/1yKIosF0YJRoB7tj5wOG2w9m7JcUYMjo+lneR22gd6U7YYz1iMo6C?=
 =?us-ascii?Q?I2SaopKQgul2SGJbGEg1R0Ic4iWor1AuqO1hWUmt?=
X-MS-Exchange-CrossTenant-Network-Message-Id: c77371bb-b86b-4c7c-ab5f-08dc63481162
X-MS-Exchange-CrossTenant-AuthSource: SA1PR11MB6733.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 23 Apr 2024 03:47:16.1940
 (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: l2GMv9iTZ3cjYoiCTZ46LBHyQtiHmTCdnZuMBo4iNKqSK78mLbqAZy8qLMIyH9gELPBzyWfhyaSiee7hZt4DvA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SA2PR11MB4889
X-OriginatorOrg: intel.com

Dan Williams wrote:
> Ira Weiny wrote:

[snip]

> > 
> > [0]
> > Link: https://lore.kernel.org/all/cover.1711598777.git.alison.schofield@intel.com/
> > [1]
> > Link: https://lore.kernel.org/all/65d111eb87115_6c745294ac@dwillia2-xfh.jf.intel.com.notmuch/
> > [2]
> > Link: https://lore.kernel.org/all/b963c490-2c13-4b79-bbe7-34c6568423c7@moroto.mountain/
> 
> Minor, but this can be reformatted a bit cleaner:
> 
> Link: http://lore.kernel.org/r/cover.1711598777.git.alison.schofield@intel.com [0]
> Link: http://lore.kernel.org/r/65d111eb87115_6c745294ac@dwillia2-xfh.jf.intel.com.notmuch [1]
> Link: http://lore.kernel.org/r/b963c490-2c13-4b79-bbe7-34c6568423c7@moroto.mountain [2]
> 

Sure.


[snip]

> > +/*
> > + * Memory Module Event Record
> > + * CXL rev 3.0 section 8.2.9.2.1.3; Table 8-45
> > + */
> > +#define CPER_SEC_CXL_MEM_MODULE_GUID					\
> > +	GUID_INIT(0xfe927475, 0xdd59, 0x4339,				\
> > +		  0xa5, 0x86, 0x79, 0xba, 0xb1, 0x13, 0xb7, 0x74)
> > +
> > +struct cxl_cper_work_data {
> > +	enum cxl_event_type event_type;
> > +	struct cxl_cper_event_rec rec;
> > +};
> > +
> > +DEFINE_KFIFO(cxl_cper_fifo, struct cxl_cper_work_data, 32);
> 
> Any comment on where that "32" comes from?

If anyone has any better queue depth I'm open.  It is just a guess.

> 
> > +static DEFINE_SPINLOCK(cxl_cper_work_lock);
> 
> Needs a comment on what it is specifically protecting.

Ok.

> 
> > +static cxl_cper_callback cper_callback;
> > +static void cxl_cper_cb_fn(struct work_struct *work)
> > +{
> > +	struct cxl_cper_work_data wd;
> > +
> > +	while (kfifo_get(&cxl_cper_fifo, &wd))
> > +		cper_callback(wd.event_type, &wd.rec);
> > +}
> > +static DECLARE_WORK(cxl_cb_work, cxl_cper_cb_fn);
> > +struct work_struct *cxl_cper_work = NULL;
> 
> Initializing global data to NULL is redundant, however this feels like
> one too many dynamic things registered.
> 
> cxl_cper_work and cper_callback are dynamic, but from the GHES
> perspective all it cares about is checking if work is registered and if
> so put the data in the kfifo and trigger that work func.
> 
> It need not care about what happens after the work is queued. So, lets
> just have the CXL driver register its own cxl_cper_work instance and
> skip defining one locally here. Export cxl_cper_fifo for the driver to
> optionally reference.

Ok I thought we had decided not to do that.  If I recall exporting the
fifo had some difficulties but it may have been my unfamiliarity with it.

Ira

[snip]