Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.34 as permitted sender) client-ip=23.128.96.34;
Date:   Fri, 8 Dec 2023 13:39:21 -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 5/6] firmware/efi: Process CXL Component Events
Message-ID: <65738d09e30e2_45e0129451@dwillia2-xfh.jf.intel.com.notmuch>
References: <20230601-cxl-cper-v1-0-d19f1ac18ab6@intel.com>
 <20230601-cxl-cper-v1-5-d19f1ac18ab6@intel.com>
 <6572740922eb6_269bd29445@dwillia2-mobl3.amr.corp.intel.com.notmuch>
 <657364d0aaa9b_1e7d27294ec@iweiny-mobl.notmuch>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <657364d0aaa9b_1e7d27294ec@iweiny-mobl.notmuch>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?eUNjp/HCcZ80j9nZZF0/y0uOWgpzGHmj0AeTsPdtiS8a3zl66P0Dym9ocQwf?=
 =?us-ascii?Q?Ercal69Rbvn7I5F+y0bpS3I6h/90RrIZeNk9EwltoX/t8WeE55lAbeHMM4gi?=
 =?us-ascii?Q?nqrmrmtsriqIT0gPqETaB5uvwO45kL9VdLJ+V9cg174aKn4c41dVOvuIbfnx?=
 =?us-ascii?Q?mZ9w04t5mvH4QNotDqBuSBf0n6IeQd4xZElIZ/gNCcGCB+llWyzGb31bMwtz?=
 =?us-ascii?Q?1Yvei1Uskk5PN2ejpXPrsq/mixWOrLVRh9Jpvyu+oiwls8UWqW1IFFC/RkNs?=
 =?us-ascii?Q?7lEGkRKOJrlkCfzt7ZFxQ2qsY/QqaF3dzmuwcOHeql80qY/+qnZvwuXSZu/S?=
 =?us-ascii?Q?4ThRdWDhhqv1dFnC3OO1cvTkfSioZBB5zdfno2ORv+lEQEvEUevtb0GJswpv?=
 =?us-ascii?Q?s1C1D24exh3yJdYol+FTEu3ValHZjYV7k+DVuIhkJUefgKSptajKvuKkZ0RK?=
 =?us-ascii?Q?BXxPulBwdUr30F6IA44l7N5995Ygqt65laN3pPmau3OPgA7CH9mrzm2NyeBR?=
 =?us-ascii?Q?rrC7MyaDXUfTTxqqZxu4vWG64w8LCVkyyr/4Ies29v1vCMTmgjfJzza0Z0l4?=
 =?us-ascii?Q?tEiaeMysVzipEA1cK8J6zKImB8u4CAG0hZCns+Wxw3R/+zoeBoDtIwiwoB6l?=
 =?us-ascii?Q?+M9iloaOjlxkWQPr0/ByQHHNn+h5oZ8zrU2e1ZAbyt9mZMPztY0zsnrk9e+Q?=
 =?us-ascii?Q?Oad3LU1HQNVdVtbpw+hs5paYmjhDxWN2uJQ/A3kjj2wCIgaS5KMSB3dqmjRY?=
 =?us-ascii?Q?0ASP19tCrjuYsfBeslip66mRL94HpCY2GrI0m3bao1joLRnl6lFuF0+a5gc2?=
 =?us-ascii?Q?Xy2vnP8iXrceO/mMlEbzvB5rv4tGwDGsjZKNaERqEhLHQYjBN9P31uelnUmM?=
 =?us-ascii?Q?Kio0zIag/ZUfXUNaC1lc/QQf00TzGaEkPt18/Vjg9++3Lx8Xmws9a5BS0ryx?=
 =?us-ascii?Q?6+CizrxI17Adv7CJXw5mr1a8Vnz6wQYuiZzScHYobFopJgsNu+zDFAzNpkeU?=
 =?us-ascii?Q?JRrxIHDmOJdVpXmqkgc3lv5Smt5rLrTBVvG9W38JJ14eGfZggzIvPgvoX7cD?=
 =?us-ascii?Q?9uIHadRP78WosTcKHCM2H4JDZK2beLA7r8z57kVBieJcG4kfgt1EmLVOTiI9?=
 =?us-ascii?Q?QpWUdhCF4ePPiGOG6X1NSgQ0Q4UmyoU3uP9FBgTC5nesqc4r5gIXllnpG9il?=
 =?us-ascii?Q?NBRcTMKax/CcJxCZwCd/unbHJ2LAt8I1Qlmvj5b9a+4yRL9vJ8YkJvNBEqUA?=
 =?us-ascii?Q?Iw3WRpl1VN5nZDH7E+J0O6C0D5h11hoX5pC4loVenWYpQDBYXW6hjzT40mii?=
 =?us-ascii?Q?kSiiXfeFmXgyQ06W6ihytiNRQERkema0ieFSv/ZWjl69qQkemPACNkQ2zrjw?=
 =?us-ascii?Q?+asQ4kiZ/f0qiLPzC40DdUbL2d47PlaCdMZE5XcsD8OuhO5AYnbDZNJWYc7r?=
 =?us-ascii?Q?Qa+2iK5eQW0SZzTyO2actmh4/5u+kU9CbhMppocuAvackd5siXaQbTNbTXYB?=
 =?us-ascii?Q?KffdR/K1f90Oo7fV4l0C91a+FuOKa4Bfzrub3u4SHYe+ooJJmBThGog02H3X?=
 =?us-ascii?Q?Bxu0aSDxRK1xqDo1+3uur7dpqC7tHe+x7yeVS2GMjctuUfsmnLIFz67Z258V?=
 =?us-ascii?Q?tw=3D=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 15c2c867-7339-49db-2ae4-08dbf836258f
X-MS-Exchange-CrossTenant-AuthSource: PH8PR11MB8107.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 08 Dec 2023 21:39:24.6985
 (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: 2O6Z+bRHTdaC9I2IM8WOvYtlWm9woloARtUkvD7jwELHw8wsGOo3cibOPKlVQLKTiK0MFRp4ocSKkS+nxUGjAgyBxJDMFSkqawY00tSTSm8=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MW3PR11MB4586
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-0.9 required=5.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED,
	DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,
	MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,T_SCC_BODY_TEXT_LINE
	autolearn=unavailable autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on howler.vger.email
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (howler.vger.email [0.0.0.0]); Fri, 08 Dec 2023 13:46:40 -0800 (PST)

Ira Weiny wrote:
> Dan Williams wrote:
> > Ira Weiny wrote:
> 
> [snip]
> 
> > > +
> > > +int register_cxl_cper_notifier(struct notifier_block *nb)
> > > +{
> > > +	return blocking_notifier_chain_register(&cxl_cper_chain_head, nb);
> > > +}
> > > +EXPORT_SYMBOL_NS_GPL(register_cxl_cper_notifier, CXL);
> > > +
> > > +void unregister_cxl_cper_notifier(struct notifier_block *nb)
> > > +{
> > > +	blocking_notifier_chain_unregister(&cxl_cper_chain_head, nb);
> > > +}
> > > +EXPORT_SYMBOL_NS_GPL(unregister_cxl_cper_notifier, CXL);
> > 
> > So I am struggling with why is this a notifier chain vs something
> > simpler and more explicit, something like:
> > 
> > typedef (int)(*cxl_cper_event_fn)(struct cper_cxl_event_rec *rec)
> > 
> > int register_cxl_cper(cxl_cper_event_fn func)
> > {
> > 	guard(rwsem_write)(cxl_cper_rwsem);
> > 	if (cxl_cper_event)
> > 		return -EBUSY;
> > 	cxl_cper_event = func;
> > 	return 0;
> > }
> 
> This is easier in the register code but then the CXL code must create a
> loop over the available memdevs to match the incoming CPER record.
> 
> By allowing each memdev to register their own callback they each get
> called and match the CPER record to themselves.
> 
> > 
> > ...do the reverse on unregister and hold the rwsem for read while
> > invoking to hold off unregistration while event processing is in flight.
> > 
> > There are a couple properties of a blocking notifier chain that are
> > unwanted: chaining, only the CXL subsystem cares about seeing these
> > records,
> 
> True but there are multiple memdev driver instances which care.  It is not
> just 1 entity which cares about these.
> 
> > and loss of type-safety, no need to redirect through a (void *)
> > payload compared to a direct call. Overall makes the implementation more
> > explicit.
> 
> Let me see how it works out with your comments on the final patch.  But
> the additional chain state of the notifier made this much easier in my
> head.  IOW chain up any memdev which wants these notifiers.

AFAICS, the loop over memdevs is obviated by the call to pci_get_domain_bus_and_slot().

The cxl_pci driver switches from using module_pci_driver() to having a
custom module_init(). Inside that module_init() it does the registration
once with the CPER code. In the registered callback it does:

    guard(mutex)(&pdev->dev->mutex);
    if (pdev->driver == &cxl_pci_driver)
        cxlds = pci_dev_get_drvdata(pdev);
    if (!cxlds)
    	return -ENXIO;
    cxl_event_trace_record(cxlds->cxlmd, log_type, event_type, &rec->event);

...no need for per-instance registration.

> > > diff --git a/drivers/firmware/efi/cper_cxl.h b/drivers/firmware/efi/cper_cxl.h
> > > index 86bfcf7909ec..aa3d36493586 100644
> > > --- a/drivers/firmware/efi/cper_cxl.h
> > > +++ b/drivers/firmware/efi/cper_cxl.h
> > > @@ -10,11 +10,38 @@
> > >  #ifndef LINUX_CPER_CXL_H
> > >  #define LINUX_CPER_CXL_H
> > >  
> > > +#include <linux/cxl-event.h>
> > > +
> > >  /* CXL Protocol Error Section */
> > >  #define CPER_SEC_CXL_PROT_ERR						\
> 
> FYI this is not added code.

Yes, but I reacted to seeing enums and GUIDs with similar names and
more explicit details in the naming.

> 
> > >  	GUID_INIT(0x80B9EFB4, 0x52B5, 0x4DE3, 0xA7, 0x77, 0x68, 0x78,	\
> > >  		  0x4B, 0x77, 0x10, 0x48)
> > 
> > I like these defines, I notice that mbox.c uses "static const"
> > defintions for something similar. Perhaps unify on the #define method?
> 
> The static const's are defined such that they can be passed to the trace
> code as a reference without the creation of a temp variable.  These only
> need to be used as static data.

ok.

> 
> > I
> > think this define also wants a _GUID suffix to reduce potential
> > confusion between the _UUID variant and the cxl_event_log_type
> > definitions?
> 
> The UUID's are never defined as a macro.  I also followed the current
> convention here of prefixing 'CPER_SEC_' as per CPER_SEC_CXL_PROT_ERR.

Once those definitions start being reused in code outside of CPER where
it is not clear what a CPER_SEC is, I think the _GUID helps.

The UUIDs as macros would cleanup the open coded UUID_INIT in cxl_test.

> 
> > 
> > >  
> > > +/* CXL Event record UUIDs are formated at GUIDs and reported in section type */
> > > +/*
> > > + * General Media Event Record
> > > + * CXL rev 3.0 Section 8.2.9.2.1.1; Table 8-43
> > > + */
> > > +#define CPER_SEC_CXL_GEN_MEDIA						\
> > > +	GUID_INIT(0xfbcd0a77, 0xc260, 0x417f,				\
> > > +		  0x85, 0xa9, 0x08, 0x8b, 0x16, 0x21, 0xeb, 0xa6)
> > > +
> 
> 'CPER_SEC_' is in my mind different from an actual '*CPER_EVENT*'.
> 
> But I can see how the macro/enums are similar enough to have confused
> you.
> 
> I can append _GUID if you really want.

Yes, please.