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.
