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
