Messages in this thread | | | From | Dan Williams <> | Date | Fri, 11 Jun 2021 17:07:32 -0700 | Subject | Re: [PATCH 2/5] cxl/pmem: Add initial infrastructure for pmem support |
| |
On Fri, Jun 11, 2021 at 4:40 AM Jonathan Cameron <Jonathan.Cameron@huawei.com> wrote: > > On Thu, 10 Jun 2021 15:26:08 -0700 > Dan Williams <dan.j.williams@intel.com> wrote: > > > Register an 'nvdimm-bridge' device to act as an anchor for a libnvdimm > > bus hierarchy. Also, flesh out the cxl_bus definition to allow a > > cxl_nvdimm_bridge_driver to attach to the bridge and trigger the > > nvdimm-bus registration. > > > > The creation of the bridge is gated on the detection of a PMEM capable > > address space registered to the root. The bridge indirection allows the > > libnvdimm module to remain unloaded on platforms without PMEM support. > > > > Given that the probing of ACPI0017 is asynchronous to CXL endpoint > > devices, and the expectation that CXL endpoint devices register other > > PMEM resources on the 'CXL' nvdimm bus, a workqueue is added. The > > workqueue is needed to run bus_rescan_devices() outside of the > > device_lock() of the nvdimm-bridge device to rendezvous nvdimm resources > > as they arrive. For now only the bus is taken online/offline in the > > workqueue. > > > > Signed-off-by: Dan Williams <dan.j.williams@intel.com> > > I'm not that familiar with nvdimm side of things, so this is mostly > superficial review of the patch itself. > > A few really minor comments inline, but otherwise looks good to me. > > Jonathan > [..] > > +static void unregister_nvb(void *_cxl_nvb) > > +{ > > + struct cxl_nvdimm_bridge *cxl_nvb = _cxl_nvb; > > + bool flush = false; > > + > > + /* > > + * If the bridge was ever activated then there might be in-flight state > > + * work to flush. Once the state has been changed to 'dead' then no new > > + * work can be queued by user-triggered bind. > > + */ > > + device_lock(&cxl_nvb->dev); > > + if (cxl_nvb->state != CXL_NVB_NEW) > > + flush = true; > > flush = clx_nvb->state != CXL_NVB_NEW; > > perhaps?
Oh, yeah, that's nicer.
[..] > > +static void cxl_nvb_update_state(struct work_struct *work) > > +{ > > + struct cxl_nvdimm_bridge *cxl_nvb = > > + container_of(work, typeof(*cxl_nvb), state_work); > > + bool release = false; > > + > > + device_lock(&cxl_nvb->dev); > > + switch (cxl_nvb->state) { > > + case CXL_NVB_ONLINE: > > + online_nvdimm_bus(cxl_nvb); > > + if (!cxl_nvb->nvdimm_bus) { > > I'd slightly prefer a simple return code from online_nvdimm_bus() > so the reviewer doesn't have to look up above to find out that > this condition corresponds to failure.
Yeah, not sure why I made that so obscure.
| |