Messages in this thread | | | From | Dan Williams <> | Date | Wed, 31 Mar 2021 09:04:32 -0700 | Subject | Re: [PATCH v3 3/4] cxl/mem: Do not rely on device_add() side effects for dev_set_name() failures |
| |
On Wed, Mar 31, 2021 at 6:10 AM Jason Gunthorpe <jgg@nvidia.com> wrote: > > On Tue, Mar 30, 2021 at 04:36:42PM -0700, Dan Williams wrote: > > +static int cxl_mem_add_memdev(struct cxl_mem *cxlm) > > +{ > > + struct cxl_memdev *cxlmd; > > + struct device *dev; > > + struct cdev *cdev; > > + int rc; > > + > > + cxlmd = cxl_memdev_alloc(cxlm); > > + if (IS_ERR(cxlmd)) > > + return PTR_ERR(cxlmd); > > + > > + dev = &cxlmd->dev; > > + rc = dev_set_name(dev, "mem%d", cxlmd->id); > > + if (rc) > > + goto err; > > > > + cdev = &cxlmd->cdev; > > cxl_memdev_activate(cxlmd, cxlm); > > rc = cdev_device_add(cdev, dev); > > if (rc) > > - goto err_add; > > + goto err; > > It might read nicer to have the error unwind here just call cxl_memdev_unregister()
Perhaps, but I don't think cdev_del() and device_del() are prepared to deal with an object that was not successfully added.
> > > - return devm_add_action_or_reset(&pdev->dev, cxl_memdev_unregister, > > + return devm_add_action_or_reset(dev->parent, cxl_memdev_unregister, > > cxlmd); > > Since that is what the error unwind does at this point.
Right, but at this point the code knows that cdev_del() and device_del() will receive an object in the appropriate state.
> > > > > -err_add: > > +err: > > /* > > * The cdev was briefly live, shutdown any ioctl operations that > > * saw that state. > > */ > > cxl_memdev_shutdown(cxlmd); > > Then this doesn't need to be a function > > But it is OK as is
Unless I'm missing something I think it's required to use only put_device() to cleanup after cdev_device_add() failure, but yes I don't like that cxl_memdev_shutdown() needs to be open coded like this.
> > Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
Appreciate it.
| |