From: Jonathan Cameron <Jonathan.Cameron@Huawei.com> To: Ben Widawsky <ben.widawsky@intel.com> Cc: linux-cxl@vger.kernel.org, linux-acpi@vger.kernel.org, linux-kernel@vger.kernel.org, linux-nvdimm@lists.01.org, linux-pci@vger.kernel.org, Bjorn Helgaas <helgaas@kernel.org>, "Chris Browy <cbrowy@avery-design.com>, Christoph Hellwig <hch@infradead.org>, Dan Williams <dan.j.williams@intel.com>, David Hildenbrand <david@redhat.com>, David Rientjes" <rientjes@google.com>, "Jon Masters <jcm@jonmasters.org>, Rafael Wysocki <rafael.j.wysocki@intel.com>, Randy Dunlap" <rdunlap@infradead.org>, "John Groves (jgroves)" <jgroves@micron.com>, "Kelley, Sean V" <sean.v.kelley@intel.com> Subject: Re: [PATCH v2 3/8] cxl/mem: Register CXL memX devices Date: Wed, 10 Feb 2021 18:17:25 +0000 [thread overview] Message-ID: <20210210181725.00007865@Huawei.com> (raw) In-Reply-To: <20210210000259.635748-4-ben.widawsky@intel.com> On Tue, 9 Feb 2021 16:02:54 -0800 Ben Widawsky <ben.widawsky@intel.com> wrote: > From: Dan Williams <dan.j.williams@intel.com> > > 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) > > * 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. However, going forward it will provide a mechanism to > coordinate cross-device interleave. > > Signed-off-by: Dan Williams <dan.j.williams@intel.com> > Signed-off-by: Ben Widawsky <ben.widawsky@intel.com> One stray header, and a request for a tiny bit of reordering to make it easier to chase through creation and destruction. Either way with the header move to earlier patch I'm fine with this one. Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > --- > Documentation/ABI/testing/sysfs-bus-cxl | 26 ++ > .../driver-api/cxl/memory-devices.rst | 17 + > drivers/cxl/Makefile | 3 + > drivers/cxl/bus.c | 29 ++ > drivers/cxl/cxl.h | 4 + > drivers/cxl/mem.c | 301 +++++++++++++++++- > 6 files changed, 378 insertions(+), 2 deletions(-) > create mode 100644 Documentation/ABI/testing/sysfs-bus-cxl > create mode 100644 drivers/cxl/bus.c > > diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h > index 745f5e0bfce3..b3c56fa6e126 100644 > --- a/drivers/cxl/cxl.h > +++ b/drivers/cxl/cxl.h > @@ -3,6 +3,7 @@ > > #ifndef __CXL_H__ > #define __CXL_H__ > +#include <linux/range.h> Why is this coming in now? Feels like it should have been in earlier patch that started using struct range > > #include <linux/bitfield.h> > #include <linux/bitops.h> > @@ -55,6 +56,7 @@ > (FIELD_GET(CXLMDEV_RESET_NEEDED_MASK, status) != \ > CXLMDEV_RESET_NEEDED_NOT) > > +struct cxl_memdev; > /** > * struct cxl_mem - A CXL memory device > * @pdev: The PCI device associated with this CXL device. > @@ -72,6 +74,7 @@ > struct cxl_mem { > struct pci_dev *pdev; > void __iomem *regs; > + struct cxl_memdev *cxlmd; > > void __iomem *status_regs; > void __iomem *mbox_regs; > @@ -90,4 +93,5 @@ struct cxl_mem { > } ram; > }; > > +extern struct bus_type cxl_bus_type; > #endif /* __CXL_H__ */ > diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c > index 0a868a15badc..8bbd2495e237 100644 > --- a/drivers/cxl/mem.c > +++ b/drivers/cxl/mem.c > @@ -1,11 +1,36 @@ > > + > +static void cxl_memdev_release(struct device *dev) > +{ > + struct cxl_memdev *cxlmd = to_cxl_memdev(dev); > + > + percpu_ref_exit(&cxlmd->ops_active); > + ida_free(&cxl_memdev_ida, cxlmd->id); > + kfree(cxlmd); > +} > + ... > +static int cxl_mem_add_memdev(struct cxl_mem *cxlm) > +{ > + struct pci_dev *pdev = cxlm->pdev; > + struct cxl_memdev *cxlmd; > + struct device *dev; > + struct cdev *cdev; > + int rc; > + > + cxlmd = kzalloc(sizeof(*cxlmd), GFP_KERNEL); > + if (!cxlmd) > + return -ENOMEM; > + init_completion(&cxlmd->ops_dead); > + > + /* > + * @cxlm is deallocated when the driver unbinds so operations > + * that are using it need to hold a live reference. > + */ > + cxlmd->cxlm = cxlm; > + rc = percpu_ref_init(&cxlmd->ops_active, cxlmdev_ops_active_release, 0, > + GFP_KERNEL); > + if (rc) > + goto err_ref; > + > + rc = ida_alloc_range(&cxl_memdev_ida, 0, CXL_MEM_MAX_DEVS, GFP_KERNEL); > + if (rc < 0) > + goto err_id; > + cxlmd->id = rc; > + > + dev = &cxlmd->dev; > + device_initialize(dev); > + dev->parent = &pdev->dev; > + dev->bus = &cxl_bus_type; > + dev->devt = MKDEV(cxl_mem_major, cxlmd->id); > + dev->type = &cxl_memdev_type; > + dev_set_name(dev, "mem%d", cxlmd->id); > + > + cdev = &cxlmd->cdev; > + cdev_init(cdev, &cxl_memdev_fops); > + > + rc = cdev_device_add(cdev, dev); > + if (rc) > + goto err_add; > + > + return devm_add_action_or_reset(dev->parent, cxlmdev_unregister, cxlmd); This had me scratching my head. The cxlmdev_unregister() if called normally or in the _or_reset() results in percpu_ref_kill(&cxlmd->ops_active); cdev_device_del(&cxlmd->cdev, dev); wait_for_completion(&cxlmd->ops_dead); cxlmd->cxlm = NULL; put_device(dev); /* If last ref this will result in */ percpu_ref_exit(&cxlmd->ops_active); ida_free(&cxl_memdev_ida, cxlmd->id); kfree(cxlmd); So it's doing all the correct things but not necessarily in the obvious order. For simplicity of review perhaps it's worth reordering probe a bit to get the ida immediately after the cxlmd alloc and for the cxlmdev_unregister() perhaps reorder the cdev_device_del() before the percpu_ref_kill(). Trivial obvious as the ordering has no affect but makes it easy for reviewers to tick off setup vs tear down parts. > + > +err_add: > + ida_free(&cxl_memdev_ida, cxlmd->id); > +err_id: > + /* > + * Theoretically userspace could have already entered the fops, > + * so flush ops_active. > + */ > + percpu_ref_kill(&cxlmd->ops_active); > + wait_for_completion(&cxlmd->ops_dead); > + percpu_ref_exit(&cxlmd->ops_active); > +err_ref: > + kfree(cxlmd); > + > + return rc; > +} > + _______________________________________________ Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org To unsubscribe send an email to linux-nvdimm-leave@lists.01.org
WARNING: multiple messages have this Message-ID (diff)
From: Jonathan Cameron <Jonathan.Cameron@Huawei.com> To: Ben Widawsky <ben.widawsky@intel.com> Cc: <linux-cxl@vger.kernel.org>, <linux-acpi@vger.kernel.org>, <linux-kernel@vger.kernel.org>, <linux-nvdimm@lists.01.org>, <linux-pci@vger.kernel.org>, Bjorn Helgaas <helgaas@kernel.org>, "Chris Browy" <cbrowy@avery-design.com>, Christoph Hellwig <hch@infradead.org>, "Dan Williams" <dan.j.williams@intel.com>, David Hildenbrand <david@redhat.com>, David Rientjes <rientjes@google.com>, Ira Weiny <ira.weiny@intel.com>, "Jon Masters" <jcm@jonmasters.org>, Rafael Wysocki <rafael.j.wysocki@intel.com>, Randy Dunlap <rdunlap@infradead.org>, Vishal Verma <vishal.l.verma@intel.com>, "John Groves (jgroves)" <jgroves@micron.com>, "Kelley, Sean V" <sean.v.kelley@intel.com> Subject: Re: [PATCH v2 3/8] cxl/mem: Register CXL memX devices Date: Wed, 10 Feb 2021 18:17:25 +0000 [thread overview] Message-ID: <20210210181725.00007865@Huawei.com> (raw) In-Reply-To: <20210210000259.635748-4-ben.widawsky@intel.com> On Tue, 9 Feb 2021 16:02:54 -0800 Ben Widawsky <ben.widawsky@intel.com> wrote: > From: Dan Williams <dan.j.williams@intel.com> > > 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) > > * 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. However, going forward it will provide a mechanism to > coordinate cross-device interleave. > > Signed-off-by: Dan Williams <dan.j.williams@intel.com> > Signed-off-by: Ben Widawsky <ben.widawsky@intel.com> One stray header, and a request for a tiny bit of reordering to make it easier to chase through creation and destruction. Either way with the header move to earlier patch I'm fine with this one. Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > --- > Documentation/ABI/testing/sysfs-bus-cxl | 26 ++ > .../driver-api/cxl/memory-devices.rst | 17 + > drivers/cxl/Makefile | 3 + > drivers/cxl/bus.c | 29 ++ > drivers/cxl/cxl.h | 4 + > drivers/cxl/mem.c | 301 +++++++++++++++++- > 6 files changed, 378 insertions(+), 2 deletions(-) > create mode 100644 Documentation/ABI/testing/sysfs-bus-cxl > create mode 100644 drivers/cxl/bus.c > > diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h > index 745f5e0bfce3..b3c56fa6e126 100644 > --- a/drivers/cxl/cxl.h > +++ b/drivers/cxl/cxl.h > @@ -3,6 +3,7 @@ > > #ifndef __CXL_H__ > #define __CXL_H__ > +#include <linux/range.h> Why is this coming in now? Feels like it should have been in earlier patch that started using struct range > > #include <linux/bitfield.h> > #include <linux/bitops.h> > @@ -55,6 +56,7 @@ > (FIELD_GET(CXLMDEV_RESET_NEEDED_MASK, status) != \ > CXLMDEV_RESET_NEEDED_NOT) > > +struct cxl_memdev; > /** > * struct cxl_mem - A CXL memory device > * @pdev: The PCI device associated with this CXL device. > @@ -72,6 +74,7 @@ > struct cxl_mem { > struct pci_dev *pdev; > void __iomem *regs; > + struct cxl_memdev *cxlmd; > > void __iomem *status_regs; > void __iomem *mbox_regs; > @@ -90,4 +93,5 @@ struct cxl_mem { > } ram; > }; > > +extern struct bus_type cxl_bus_type; > #endif /* __CXL_H__ */ > diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c > index 0a868a15badc..8bbd2495e237 100644 > --- a/drivers/cxl/mem.c > +++ b/drivers/cxl/mem.c > @@ -1,11 +1,36 @@ > > + > +static void cxl_memdev_release(struct device *dev) > +{ > + struct cxl_memdev *cxlmd = to_cxl_memdev(dev); > + > + percpu_ref_exit(&cxlmd->ops_active); > + ida_free(&cxl_memdev_ida, cxlmd->id); > + kfree(cxlmd); > +} > + ... > +static int cxl_mem_add_memdev(struct cxl_mem *cxlm) > +{ > + struct pci_dev *pdev = cxlm->pdev; > + struct cxl_memdev *cxlmd; > + struct device *dev; > + struct cdev *cdev; > + int rc; > + > + cxlmd = kzalloc(sizeof(*cxlmd), GFP_KERNEL); > + if (!cxlmd) > + return -ENOMEM; > + init_completion(&cxlmd->ops_dead); > + > + /* > + * @cxlm is deallocated when the driver unbinds so operations > + * that are using it need to hold a live reference. > + */ > + cxlmd->cxlm = cxlm; > + rc = percpu_ref_init(&cxlmd->ops_active, cxlmdev_ops_active_release, 0, > + GFP_KERNEL); > + if (rc) > + goto err_ref; > + > + rc = ida_alloc_range(&cxl_memdev_ida, 0, CXL_MEM_MAX_DEVS, GFP_KERNEL); > + if (rc < 0) > + goto err_id; > + cxlmd->id = rc; > + > + dev = &cxlmd->dev; > + device_initialize(dev); > + dev->parent = &pdev->dev; > + dev->bus = &cxl_bus_type; > + dev->devt = MKDEV(cxl_mem_major, cxlmd->id); > + dev->type = &cxl_memdev_type; > + dev_set_name(dev, "mem%d", cxlmd->id); > + > + cdev = &cxlmd->cdev; > + cdev_init(cdev, &cxl_memdev_fops); > + > + rc = cdev_device_add(cdev, dev); > + if (rc) > + goto err_add; > + > + return devm_add_action_or_reset(dev->parent, cxlmdev_unregister, cxlmd); This had me scratching my head. The cxlmdev_unregister() if called normally or in the _or_reset() results in percpu_ref_kill(&cxlmd->ops_active); cdev_device_del(&cxlmd->cdev, dev); wait_for_completion(&cxlmd->ops_dead); cxlmd->cxlm = NULL; put_device(dev); /* If last ref this will result in */ percpu_ref_exit(&cxlmd->ops_active); ida_free(&cxl_memdev_ida, cxlmd->id); kfree(cxlmd); So it's doing all the correct things but not necessarily in the obvious order. For simplicity of review perhaps it's worth reordering probe a bit to get the ida immediately after the cxlmd alloc and for the cxlmdev_unregister() perhaps reorder the cdev_device_del() before the percpu_ref_kill(). Trivial obvious as the ordering has no affect but makes it easy for reviewers to tick off setup vs tear down parts. > + > +err_add: > + ida_free(&cxl_memdev_ida, cxlmd->id); > +err_id: > + /* > + * Theoretically userspace could have already entered the fops, > + * so flush ops_active. > + */ > + percpu_ref_kill(&cxlmd->ops_active); > + wait_for_completion(&cxlmd->ops_dead); > + percpu_ref_exit(&cxlmd->ops_active); > +err_ref: > + kfree(cxlmd); > + > + return rc; > +} > +
next prev parent reply other threads:[~2021-02-10 18:18 UTC|newest] Thread overview: 114+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-02-10 0:02 [PATCH v2 0/8] CXL 2.0 Support Ben Widawsky 2021-02-10 0:02 ` Ben Widawsky 2021-02-10 0:02 ` [PATCH v2 1/8] cxl/mem: Introduce a driver for CXL-2.0-Type-3 endpoints Ben Widawsky 2021-02-10 0:02 ` Ben Widawsky 2021-02-10 16:17 ` Jonathan Cameron 2021-02-10 16:17 ` Jonathan Cameron 2021-02-10 17:12 ` Ben Widawsky 2021-02-10 17:12 ` Ben Widawsky 2021-02-10 17:23 ` Jonathan Cameron 2021-02-10 17:23 ` Jonathan Cameron 2021-02-10 0:02 ` [PATCH v2 2/8] cxl/mem: Find device capabilities Ben Widawsky 2021-02-10 0:02 ` Ben Widawsky 2021-02-10 13:32 ` Jonathan Cameron 2021-02-10 13:32 ` Jonathan Cameron 2021-02-10 15:07 ` Jonathan Cameron 2021-02-10 15:07 ` Jonathan Cameron 2021-02-10 16:55 ` Ben Widawsky 2021-02-10 16:55 ` Ben Widawsky 2021-02-10 17:30 ` Jonathan Cameron 2021-02-10 17:30 ` Jonathan Cameron 2021-02-10 18:16 ` Ben Widawsky 2021-02-10 18:16 ` Ben Widawsky 2021-02-11 9:55 ` Jonathan Cameron 2021-02-11 9:55 ` Jonathan Cameron 2021-02-11 15:55 ` Ben Widawsky 2021-02-11 15:55 ` Ben Widawsky 2021-02-12 13:27 ` Jonathan Cameron 2021-02-12 13:27 ` Jonathan Cameron 2021-02-12 15:54 ` Ben Widawsky 2021-02-12 15:54 ` Ben Widawsky 2021-02-11 18:27 ` Ben Widawsky 2021-02-11 18:27 ` Ben Widawsky 2021-02-12 13:23 ` Jonathan Cameron 2021-02-12 13:23 ` Jonathan Cameron 2021-02-10 19:32 ` Ben Widawsky 2021-02-10 19:32 ` Ben Widawsky 2021-02-10 17:41 ` Jonathan Cameron 2021-02-10 17:41 ` Jonathan Cameron 2021-02-10 18:53 ` Ben Widawsky 2021-02-10 18:53 ` Ben Widawsky 2021-02-10 19:54 ` Dan Williams 2021-02-10 19:54 ` Dan Williams 2021-02-11 10:01 ` Jonathan Cameron 2021-02-11 10:01 ` Jonathan Cameron 2021-02-11 16:04 ` Ben Widawsky 2021-02-11 16:04 ` Ben Widawsky 2021-02-10 0:02 ` [PATCH v2 3/8] cxl/mem: Register CXL memX devices Ben Widawsky 2021-02-10 0:02 ` Ben Widawsky 2021-02-10 18:17 ` Jonathan Cameron [this message] 2021-02-10 18:17 ` Jonathan Cameron 2021-02-11 10:17 ` Jonathan Cameron 2021-02-11 10:17 ` Jonathan Cameron 2021-02-11 20:40 ` Dan Williams 2021-02-11 20:40 ` Dan Williams 2021-02-12 13:33 ` Jonathan Cameron 2021-02-12 13:33 ` Jonathan Cameron 2021-02-10 0:02 ` [PATCH v2 4/8] cxl/mem: Add basic IOCTL interface Ben Widawsky 2021-02-10 0:02 ` Ben Widawsky 2021-02-10 18:45 ` Jonathan Cameron 2021-02-10 18:45 ` Jonathan Cameron 2021-02-10 20:22 ` Ben Widawsky 2021-02-10 20:22 ` Ben Widawsky 2021-02-11 4:40 ` Dan Williams 2021-02-11 4:40 ` Dan Williams 2021-02-11 10:06 ` Jonathan Cameron 2021-02-11 10:06 ` Jonathan Cameron 2021-02-11 16:54 ` Ben Widawsky 2021-02-11 16:54 ` Ben Widawsky 2021-02-14 16:30 ` Al Viro 2021-02-14 16:30 ` Al Viro 2021-02-14 23:14 ` Ben Widawsky 2021-02-14 23:14 ` Ben Widawsky 2021-02-14 23:50 ` Al Viro 2021-02-14 23:50 ` Al Viro 2021-02-14 23:57 ` Al Viro 2021-02-14 23:57 ` Al Viro 2021-02-10 0:02 ` [PATCH v2 5/8] cxl/mem: Add a "RAW" send command Ben Widawsky 2021-02-10 0:02 ` Ben Widawsky 2021-02-10 15:26 ` Ariel.Sibley 2021-02-10 15:26 ` Ariel.Sibley 2021-02-10 16:49 ` Ben Widawsky 2021-02-10 16:49 ` Ben Widawsky 2021-02-10 18:03 ` Ariel.Sibley 2021-02-10 18:03 ` Ariel.Sibley 2021-02-10 18:11 ` Ben Widawsky 2021-02-10 18:11 ` Ben Widawsky 2021-02-10 18:46 ` Ariel.Sibley 2021-02-10 18:46 ` Ariel.Sibley 2021-02-10 19:12 ` Ben Widawsky 2021-02-10 19:12 ` Ben Widawsky 2021-02-11 16:43 ` Dan Williams 2021-02-11 16:43 ` Dan Williams 2021-02-11 11:19 ` Jonathan Cameron 2021-02-11 11:19 ` Jonathan Cameron 2021-02-11 16:01 ` Ben Widawsky 2021-02-11 16:01 ` Ben Widawsky 2021-02-12 13:40 ` Jonathan Cameron 2021-02-12 13:40 ` Jonathan Cameron 2021-02-10 0:02 ` [PATCH v2 6/8] cxl/mem: Enable commands via CEL Ben Widawsky 2021-02-10 0:02 ` Ben Widawsky 2021-02-11 12:02 ` Jonathan Cameron 2021-02-11 12:02 ` Jonathan Cameron 2021-02-11 17:45 ` Ben Widawsky 2021-02-11 17:45 ` Ben Widawsky 2021-02-11 20:34 ` Dan Williams 2021-02-11 20:34 ` Dan Williams 2021-02-16 13:43 ` Bartosz Golaszewski 2021-02-16 13:43 ` Bartosz Golaszewski 2021-02-10 0:02 ` [PATCH v2 7/8] cxl/mem: Add set of informational commands Ben Widawsky 2021-02-10 0:02 ` Ben Widawsky 2021-02-11 12:07 ` Jonathan Cameron 2021-02-11 12:07 ` Jonathan Cameron 2021-02-10 0:02 ` [PATCH v2 8/8] MAINTAINERS: Add maintainers of the CXL driver Ben Widawsky 2021-02-10 0:02 ` Ben Widawsky
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20210210181725.00007865@Huawei.com \ --to=jonathan.cameron@huawei.com \ --cc=ben.widawsky@intel.com \ --cc=helgaas@kernel.org \ --cc=jgroves@micron.com \ --cc=linux-acpi@vger.kernel.org \ --cc=linux-cxl@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-nvdimm@lists.01.org \ --cc=linux-pci@vger.kernel.org \ --cc=rdunlap@infradead.org \ --cc=rientjes@google.com \ --cc=sean.v.kelley@intel.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.