Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:6 as permitted sender) client-ip=2620:137:e000::3:6;
Date:   Fri, 8 Dec 2023 13:46:07 -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 6/6] cxl/memdev: Register for and process CPER events
Message-ID: <65738e9fc239a_45e0129476@dwillia2-xfh.jf.intel.com.notmuch>
References: <20230601-cxl-cper-v1-0-d19f1ac18ab6@intel.com>
 <20230601-cxl-cper-v1-6-d19f1ac18ab6@intel.com>
 <657279a68c270_b991294e@dwillia2-mobl3.amr.corp.intel.com.notmuch>
 <657371eec6ac5_1e7d272948d@iweiny-mobl.notmuch>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <657371eec6ac5_1e7d272948d@iweiny-mobl.notmuch>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?Cy8AARwO3TS95PjU1GpqLbTEcJs711yK+DsAbYrkGnaVCjb85zFKnaj7QnF0?=
 =?us-ascii?Q?6/bOnkeMX+2tlUcXqyRDyRPhiPOvW4nJjSEDnAWjFpX5QkryQ18SWDNmFK1N?=
 =?us-ascii?Q?oMHbbZQItx5H/Pwt1AobqNk4kyummoDsuISh3jEdk+VzgO5QenmTAMtxOzH6?=
 =?us-ascii?Q?waEP0SLuEX6uVphRD3zCjCdC3qb/cjXftjKjTo4lDBlQihP0zPmWrwwhP6mO?=
 =?us-ascii?Q?8mRN1azId+MrY6fP6gGwVPTDfBu3UWwxFbCqJuWTVyTy/F3ZbgAB72okPiPP?=
 =?us-ascii?Q?8jQq/QJHk7RjDr76GjkQ2p9jNof7lpy31uCNVq8PsxRWgRa1cmGzfuNIJXlW?=
 =?us-ascii?Q?s5HFLRV+ElaoGwuRFJ1BFMTwTI9EvICu6ujDBESTE1OWUxkabjgTLLXpWBMy?=
 =?us-ascii?Q?byviteXafVtbUSwY73ASjAbfoq0fMapW78/nXH8e5XbDJJNfj+m5wjQRDWOc?=
 =?us-ascii?Q?l+BXpShaTKyRqORqrtgyKT1t8Doto+u0ybFa8+/pTwghau1276dqVKxpMWa3?=
 =?us-ascii?Q?ztuJvHNIljfASbScjHhAnQCfdqL0Lsh0L+t3l610FVh/jm3armZ/G7NMc1xe?=
 =?us-ascii?Q?osJ6E9Nl98UseSOxQrLZX97/p55cpt9MQABirGkZ29KqvE7/VPV1Z9e+rh9/?=
 =?us-ascii?Q?htwjDdRyDrYZsyGrWjiwXVVJWrYuX8orm7iFZJZaoQc5tGVqtNQfrlkCi6Lt?=
 =?us-ascii?Q?Ujb5rdqHyyNVCzK/om5MokkO/LArv3SVrMgHzLneBinrzQ/mPHgnu7nCfEGM?=
 =?us-ascii?Q?sUa+oxrWfbitxVcu3OB/v8+nMKiddFPvdo6XuSAF0E7LIICiYq+hXezza4gm?=
 =?us-ascii?Q?/gEYzIqqqK5VE1KWnk2uUQ+OxgWFMBLbN+AC2m4ZbYrdl7LZm8Ivj5HWviXc?=
 =?us-ascii?Q?61H0DEoQJ2NGa04qmQ5fIQT1cuhNCyNuc+ddwivIDbNPblSaf0SNYLj9AKRU?=
 =?us-ascii?Q?d4InuNEpBctolCqyIvgsO6ymLh1Nmy65W8Q1sJs06fnSV7gqffKEnkgCRquH?=
 =?us-ascii?Q?Bkux+9uoKLgQhOgm44nXlWPBIMyNokZARzxApiImWirzy90acwaK8M33h6GB?=
 =?us-ascii?Q?RBoHvsoYUaNYlLxjHtw9nF8HbvOqHMLUrxu7lFUcH2Z0EzWs7oLc21pVkri4?=
 =?us-ascii?Q?ttwrt7ACkVFNZwKMMhkjkceLUIJ0HpQk6P/xSjytv9CleORNyoew1wNlJI3V?=
 =?us-ascii?Q?56Iqut55GXMb4rO+K4zIo+PyNTmZBY7YmtO5wlvlruvXjlcN9a/BpU6FQpNM?=
 =?us-ascii?Q?Cy0pq9azxzuQr4TGI0BT9yYQzCkPH4NHeVFf+dglazSJv0Y3eplJaztC1SIa?=
 =?us-ascii?Q?uDBfG2WefZ2db0F7cbqcxTU23tt74TBUeeyIeCVp/50lnhgoFF/j/Ef1MaEk?=
 =?us-ascii?Q?MshgRxuboQqGyyGBIi/aVq/Y0UUypmszAq4KXBGkzQ4BlvlaHl7D4G7VrUy6?=
 =?us-ascii?Q?x0wC9+hTp7XWlySaWcnQP55H+ROF8t8AzYW/wHCCiSDDDUf2nbFoNBGccvJl?=
 =?us-ascii?Q?O8iF/gn3CRvQ8N3HYCYxvTh9s+kiAngQFY8ykhlpMMWyLJJTZaezeHXxj3/s?=
 =?us-ascii?Q?U8Ui6PHFw4AmYdXpRocV/7Yb5HCV99CyVeW93lIHS1hg/XeBD4X7Zsfedv5C?=
 =?us-ascii?Q?vw=3D=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: d18c3356-81e9-4110-5143-08dbf83717b4
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:46:11.3352
 (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: 6Z6UR6UKz19iqxgIy01hbf5ljBmee8iPwjq8xcmAcuV6Oo/wAlkHV/RN1alIzn1Na2dHte8u2iQ2EC3ZR9hJCORetU9agDjyORfJOAAOoLQ=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CO1PR11MB5137
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 pete.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 (pete.vger.email [0.0.0.0]); Fri, 08 Dec 2023 13:47:04 -0800 (PST)

Ira Weiny wrote:
> Dan Williams wrote:
> > Ira Weiny wrote:
> 
> [snip]
> 
> > >  
> > > +#define CXL_EVENT_HDR_FLAGS_REC_SEVERITY GENMASK(1, 0)
> > > +static int cxl_cper_event_call(struct notifier_block *nb, unsigned long action,
> > > +			       void *data)
> > > +{
> > > +	struct cxl_cper_notifier_data *nd = data;
> > > +	struct cper_cxl_event_devid *device_id = &nd->rec->hdr.device_id;
> > > +	enum cxl_event_log_type log_type;
> > > +	struct cxl_memdev_state *mds;
> > > +	struct cxl_dev_state *cxlds;
> > > +	struct pci_dev *pdev;
> > > +	unsigned int devfn;
> > > +	u32 hdr_flags;
> > > +
> > > +	mds = container_of(nb, struct cxl_memdev_state, cxl_cper_nb);
> > > +
> > > +	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);
> > > +	cxlds = pci_get_drvdata(pdev);
> > > +	if (cxlds != &mds->cxlds) {
> > 
> > Checks of drvdata are only valid under the device lock, or with the
> > assumption that this callback will never be called while pci_get_drvdata
> > would return NULL.
> 
> For the device we have registered pci_get_drvdata() will be always be valid.
> Each driver is registering it's own call with valid driver state in the chain.
> 
> However, I see I have a bug here.  Using devm_add_action_or_reset() breaks
> this assumption.
> 
> > 
> > With that, the check of cxlds looks like another artifact of using a
> > blocking notifier chain for this callback.
> 
> It is a desired artifact.  This check is determining if this event is for this
> device.  It is not checking if cxlds is valid.
> 
> > With an explicit single
> > callback it simply becomes safe to assume that it is being called back
> > before unregister_cxl_cper() has run. I.e. it is impossible to even
> > write this check in that case.
> 
> Exploring the use of a single register call...  you must check if the cxlds is
> valid on that pdev.  Because the driver may not be attached.
> 
> Something like this in cxl_core vs cxl_pci:

I replied with sample implementation on the other thread, but some
comments here:


> #define CXL_EVENT_HDR_FLAGS_REC_SEVERITY GENMASK(1, 0)
> static void cxl_cper_event_call(struct cxl_cper_notifier_data *nd)

Is struct cxl_cper_notifier_data needed anymore, just pass the record
reference?


> {       
>         struct cper_cxl_event_devid *device_id = &nd->rec->hdr.device_id;
>         enum cxl_event_log_type log_type;
>         struct cxl_dev_state *cxlds;
>         struct pci_dev *pdev;
>         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);
>         device_lock(&pdev->dev);
>         cxlds = pci_get_drvdata(pdev);

You would need to first validate that it is indeed a pci device that
cxl_pci is driving before making this assumption. That's what the
cxl_pci_driver check is doing in the other reply.

>         if (!cxlds)
>                 goto out;
>         
>         /* Fabricate a log type */
>         hdr_flags = get_unaligned_le24(nd->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, nd->event_type,
>                                &nd->rec->event);
> out:    
>         device_unlock(&pdev->dev);
>         pci_dev_put(pdev);
> }
> 
> This does simplify registering.
> 
> Is this what you were thinking?

Just not in the core since the core has no idea how to do the "is this
one of *my* CXL pci_dev instances" check.

> > > +
> > > +static void register_cper_events(struct cxl_memdev_state *mds)
> > > +{
> > > +	mds->cxl_cper_nb.notifier_call = cxl_cper_event_call;
> > > +
> > > +	if (register_cxl_cper_notifier(&mds->cxl_cper_nb)) {
> > > +		dev_err(mds->cxlds.dev, "CPER registration failed\n");
> > > +		return;
> > > +	}
> > > +
> > > +	devm_add_action_or_reset(mds->cxlds.dev, cxl_unregister_cper_events, mds);
> > 
> > Longer term I am not sure cxl_pci should be doing this registration
> > directly to the CPER code vs some indirection in the core that the
> > generic type-3 and the type-2 cases can register for processing. That
> > can definitely wait until a Type-2 CXL.mem device driver arrives and
> > wants to get notified of CXL CPER events.
> > 
> 
> Yes these calls will need to be moved to the core for drivers to share
> later.  Same for mailbox event handling.

That would come with some pdev to memdev lookup facility to replace the
pci_dev->driver check.