Re: [PATCH 6/6] cxl/memdev: Register for and process CPER events

[Ira Weiny <ira.weiny@intel.com>]

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:

#define CXL_EVENT_HDR_FLAGS_REC_SEVERITY GENMASK(1, 0)
static void cxl_cper_event_call(struct cxl_cper_notifier_data *nd)
{       
        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);
        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?

[snip]

> > +
> > +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.

Ira