On Wed, Mar 31, 2021 at 6:10 AM Jason Gunthorpe <[email protected]> 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 <[email protected]> Appreciate it.
