Message-ID: <65738e9fc239a_45e0129476@dwillia2-xfh.jf.intel.com.notmuch>
Date:   Fri, 8 Dec 2023 13:46:07 -0800
From:   Dan Williams <dan.j.williams@...el.com>
To:     Ira Weiny <ira.weiny@...el.com>,
        Dan Williams <dan.j.williams@...el.com>,
        Jonathan Cameron <jonathan.cameron@...wei.com>,
        Smita Koralahalli <Smita.KoralahalliChannabasappa@....com>,
        Shiju Jose <shiju.jose@...wei.com>
CC:     Yazen Ghannam <yazen.ghannam@....com>,
        Davidlohr Bueso <dave@...olabs.net>,
        Dave Jiang <dave.jiang@...el.com>,
        Alison Schofield <alison.schofield@...el.com>,
        Vishal Verma <vishal.l.verma@...el.com>,
        "Ard Biesheuvel" <ardb@...nel.org>, <linux-efi@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>, <linux-cxl@...r.kernel.org>,
        Ira Weiny <ira.weiny@...el.com>
Subject: Re: [PATCH 6/6] cxl/memdev: Register for and process CPER events

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.

Powered by blists - more mailing lists