On Thu, 19 Nov 2020 18:16:19 -0800 Dan Williams <dan.j.williams@xxxxxxxxx> wrote: > On Tue, Nov 17, 2020 at 7:57 AM Jonathan Cameron > <Jonathan.Cameron@xxxxxxxxxx> wrote: > > > > On Tue, 10 Nov 2020 21:43:55 -0800 > > Ben Widawsky <ben.widawsky@xxxxxxxxx> wrote: > > > > > From: Dan Williams <dan.j.williams@xxxxxxxxx> > > > > > > Create the /sys/bus/cxl hierarchy to enumerate memory devices > > > (per-endpoint control devices), memory address space devices (platform > > > address ranges with interleaving, performance, and persistence > > > attributes), and memory regions (active provisioned memory from an > > > address space device that is in use as System RAM or delegated to > > > libnvdimm as Persistent Memory regions). > > > > > > For now, only the per-endpoint control devices are registered on the > > > 'cxl' bus. > > > > Reviewing ABI without documentation is challenging even when it's simple > > so please add that for v2. > > > > This patch feels somewhat unpolished, but I guess it is mainly here to > > give an illustration of how stuff might fit together rather than > > any expectation of detailed review. > > Yeah, this is definitely an early look in the spirit of "Release early > / release often". > Definitely a good idea. ... > > > > > static int cxl_mem_probe(struct pci_dev *pdev, const struct pci_device_id *id) > > > { > > > struct cxl_mem *cxlm = ERR_PTR(-ENXIO); > > > struct device *dev = &pdev->dev; > > > + struct cxl_memdev *cxlmd; > > > int rc, regloc, i; > > > > > > rc = cxl_bus_prepared(pdev); > > > @@ -319,20 +545,31 @@ static int cxl_mem_probe(struct pci_dev *pdev, const struct pci_device_id *id) > > > if (rc) > > > return rc; > > > > > > - /* Check that hardware "looks" okay. */ > > > - rc = cxl_mem_mbox_get(cxlm); > > > + rc = cxl_mem_identify(cxlm); > > > if (rc) > > > return rc; > > > - > > > - cxl_mem_mbox_put(cxlm); > > > > It was kind of nice to see the flow earlier, but I'm also thinking it made > > a slightly harder to read patch. Hmm. Maybe just drop the version earlier > > in favour of a todo comment that you then do here? > > Not sure I follow, but I think you're saying don't bother with an > initial patch introducing just doing the raw cxl_mem_mbox_get() in > this path, jump straight to cxl_mem_identify()? Exactly. > > > > > > dev_dbg(&pdev->dev, "CXL Memory Device Interface Up\n"); > > > + > > > > Nice to tidy that up by moving to earlier patch. > > Sure. > > > > > > pci_set_drvdata(pdev, cxlm); > > > > > > + cxlmd = cxl_mem_add_memdev(cxlm); > > > + if (IS_ERR(cxlmd)) > > > + return PTR_ERR(cxlmd); > > > > Given we don't actually use cxlmd perhaps a simple return value > > of 0 or error would be better from cxl_mem_add_memdev() > > > > (I guess you may have follow up patches that do something with it > > here, though it feels wrong to ever do so given it is now registered > > and hence exposed to the system). > > It's not added if IS_ERR() is true, but it would be simpler to just > have cxl_mem_add_memdev() return an int since ->probe() doesn't use > it. Agreed. > > > > > > + > > > return 0; > > > } > > > ...