| Message ID | 162336397948.2462439.5230237265829121099.stgit@dwillia2-desk3.amr.corp.intel.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | cxl/pmem: Add core infrastructure for PMEM support | expand |
On Thu, 10 Jun 2021 15:26:19 -0700 Dan Williams <dan.j.williams@intel.com> wrote: > LIBNVDIMM device objects register sysfs power attributes despite nothing > requiring that support. Clean up sysfs remove the power/ attribute > group. This requires a device_create() and a device_register() usage to > be converted to the device_initialize() + device_add() pattern. > > Signed-off-by: Dan Williams <dan.j.williams@intel.com> Trivial comment below. Looks good. > --- > drivers/nvdimm/bus.c | 45 +++++++++++++++++++++++++++++++++++++-------- > 1 file changed, 37 insertions(+), 8 deletions(-) > > diff --git a/drivers/nvdimm/bus.c b/drivers/nvdimm/bus.c > index a11821df83b5..e6aa87043a95 100644 > --- a/drivers/nvdimm/bus.c > +++ b/drivers/nvdimm/bus.c > @@ -363,8 +363,13 @@ struct nvdimm_bus *nvdimm_bus_register(struct device *parent, > nvdimm_bus->dev.groups = nd_desc->attr_groups; > nvdimm_bus->dev.bus = &nvdimm_bus_type; > nvdimm_bus->dev.of_node = nd_desc->of_node; > - dev_set_name(&nvdimm_bus->dev, "ndbus%d", nvdimm_bus->id); > - rc = device_register(&nvdimm_bus->dev); > + device_initialize(&nvdimm_bus->dev); > + device_set_pm_not_required(&nvdimm_bus->dev); > + rc = dev_set_name(&nvdimm_bus->dev, "ndbus%d", nvdimm_bus->id); > + if (rc) > + goto err; Maybe mention in patch description that you also now handle errors in dev_set_name()? > + > + rc = device_add(&nvdimm_bus->dev); > if (rc) { > dev_dbg(&nvdimm_bus->dev, "registration failed: %d\n", rc); > goto err; > @@ -525,6 +530,7 @@ void __nd_device_register(struct device *dev) > set_dev_node(dev, to_nd_region(dev)->numa_node); > > dev->bus = &nvdimm_bus_type; > + device_set_pm_not_required(dev); > if (dev->parent) { > get_device(dev->parent); > if (dev_to_node(dev) == NUMA_NO_NODE) > @@ -717,18 +723,41 @@ const struct attribute_group nd_numa_attribute_group = { > .is_visible = nd_numa_attr_visible, > }; > > +static void ndctl_release(struct device *dev) > +{ > + kfree(dev); > +} > + > int nvdimm_bus_create_ndctl(struct nvdimm_bus *nvdimm_bus) > { > dev_t devt = MKDEV(nvdimm_bus_major, nvdimm_bus->id); > struct device *dev; > + int rc; > > - dev = device_create(nd_class, &nvdimm_bus->dev, devt, nvdimm_bus, > - "ndctl%d", nvdimm_bus->id); > + dev = kzalloc(sizeof(*dev), GFP_KERNEL); > + if (!dev) > + return -ENOMEM; > + device_initialize(dev); > + device_set_pm_not_required(dev); > + dev->class = nd_class; > + dev->parent = &nvdimm_bus->dev; > + dev->devt = devt; > + dev->release = ndctl_release; > + rc = dev_set_name(dev, "ndctl%d", nvdimm_bus->id); > + if (rc) > + goto err; > > - if (IS_ERR(dev)) > - dev_dbg(&nvdimm_bus->dev, "failed to register ndctl%d: %ld\n", > - nvdimm_bus->id, PTR_ERR(dev)); > - return PTR_ERR_OR_ZERO(dev); > + rc = device_add(dev); > + if (rc) { > + dev_dbg(&nvdimm_bus->dev, "failed to register ndctl%d: %d\n", > + nvdimm_bus->id, rc); > + goto err; > + } > + return 0; > + > +err: > + put_device(dev); > + return rc; > } > > void nvdimm_bus_destroy_ndctl(struct nvdimm_bus *nvdimm_bus) >
On Fri, Jun 11, 2021 at 4:47 AM Jonathan Cameron <Jonathan.Cameron@huawei.com> wrote: > > On Thu, 10 Jun 2021 15:26:19 -0700 > Dan Williams <dan.j.williams@intel.com> wrote: > > > LIBNVDIMM device objects register sysfs power attributes despite nothing > > requiring that support. Clean up sysfs remove the power/ attribute > > group. This requires a device_create() and a device_register() usage to > > be converted to the device_initialize() + device_add() pattern. > > > > Signed-off-by: Dan Williams <dan.j.williams@intel.com> > Trivial comment below. Looks good. > > > --- > > drivers/nvdimm/bus.c | 45 +++++++++++++++++++++++++++++++++++++-------- > > 1 file changed, 37 insertions(+), 8 deletions(-) > > > > diff --git a/drivers/nvdimm/bus.c b/drivers/nvdimm/bus.c > > index a11821df83b5..e6aa87043a95 100644 > > --- a/drivers/nvdimm/bus.c > > +++ b/drivers/nvdimm/bus.c > > @@ -363,8 +363,13 @@ struct nvdimm_bus *nvdimm_bus_register(struct device *parent, > > nvdimm_bus->dev.groups = nd_desc->attr_groups; > > nvdimm_bus->dev.bus = &nvdimm_bus_type; > > nvdimm_bus->dev.of_node = nd_desc->of_node; > > - dev_set_name(&nvdimm_bus->dev, "ndbus%d", nvdimm_bus->id); > > - rc = device_register(&nvdimm_bus->dev); > > + device_initialize(&nvdimm_bus->dev); > > + device_set_pm_not_required(&nvdimm_bus->dev); > > + rc = dev_set_name(&nvdimm_bus->dev, "ndbus%d", nvdimm_bus->id); > > + if (rc) > > + goto err; > > Maybe mention in patch description that you also now handle errors in > dev_set_name()? Yeah, that's a philosophy change from when this code was first written.
diff --git a/drivers/nvdimm/bus.c b/drivers/nvdimm/bus.c index a11821df83b5..e6aa87043a95 100644 --- a/drivers/nvdimm/bus.c +++ b/drivers/nvdimm/bus.c @@ -363,8 +363,13 @@ struct nvdimm_bus *nvdimm_bus_register(struct device *parent, nvdimm_bus->dev.groups = nd_desc->attr_groups; nvdimm_bus->dev.bus = &nvdimm_bus_type; nvdimm_bus->dev.of_node = nd_desc->of_node; - dev_set_name(&nvdimm_bus->dev, "ndbus%d", nvdimm_bus->id); - rc = device_register(&nvdimm_bus->dev); + device_initialize(&nvdimm_bus->dev); + device_set_pm_not_required(&nvdimm_bus->dev); + rc = dev_set_name(&nvdimm_bus->dev, "ndbus%d", nvdimm_bus->id); + if (rc) + goto err; + + rc = device_add(&nvdimm_bus->dev); if (rc) { dev_dbg(&nvdimm_bus->dev, "registration failed: %d\n", rc); goto err; @@ -525,6 +530,7 @@ void __nd_device_register(struct device *dev) set_dev_node(dev, to_nd_region(dev)->numa_node); dev->bus = &nvdimm_bus_type; + device_set_pm_not_required(dev); if (dev->parent) { get_device(dev->parent); if (dev_to_node(dev) == NUMA_NO_NODE) @@ -717,18 +723,41 @@ const struct attribute_group nd_numa_attribute_group = { .is_visible = nd_numa_attr_visible, }; +static void ndctl_release(struct device *dev) +{ + kfree(dev); +} + int nvdimm_bus_create_ndctl(struct nvdimm_bus *nvdimm_bus) { dev_t devt = MKDEV(nvdimm_bus_major, nvdimm_bus->id); struct device *dev; + int rc; - dev = device_create(nd_class, &nvdimm_bus->dev, devt, nvdimm_bus, - "ndctl%d", nvdimm_bus->id); + dev = kzalloc(sizeof(*dev), GFP_KERNEL); + if (!dev) + return -ENOMEM; + device_initialize(dev); + device_set_pm_not_required(dev); + dev->class = nd_class; + dev->parent = &nvdimm_bus->dev; + dev->devt = devt; + dev->release = ndctl_release; + rc = dev_set_name(dev, "ndctl%d", nvdimm_bus->id); + if (rc) + goto err; - if (IS_ERR(dev)) - dev_dbg(&nvdimm_bus->dev, "failed to register ndctl%d: %ld\n", - nvdimm_bus->id, PTR_ERR(dev)); - return PTR_ERR_OR_ZERO(dev); + rc = device_add(dev); + if (rc) { + dev_dbg(&nvdimm_bus->dev, "failed to register ndctl%d: %d\n", + nvdimm_bus->id, rc); + goto err; + } + return 0; + +err: + put_device(dev); + return rc; } void nvdimm_bus_destroy_ndctl(struct nvdimm_bus *nvdimm_bus)
LIBNVDIMM device objects register sysfs power attributes despite nothing requiring that support. Clean up sysfs remove the power/ attribute group. This requires a device_create() and a device_register() usage to be converted to the device_initialize() + device_add() pattern. Signed-off-by: Dan Williams <dan.j.williams@intel.com> --- drivers/nvdimm/bus.c | 45 +++++++++++++++++++++++++++++++++++++-------- 1 file changed, 37 insertions(+), 8 deletions(-)