On Wed, Mar 31, 2021 at 6:07 AM Jason Gunthorpe <[email protected]> wrote: > > On Tue, Mar 30, 2021 at 04:36:37PM -0700, Dan Williams wrote: > > -static void cxlmdev_unregister(void *_cxlmd) > > +static void cxl_memdev_activate(struct cxl_memdev *cxlmd, struct cxl_mem > > *cxlm) > > { > > - struct cxl_memdev *cxlmd = _cxlmd; > > - struct device *dev = &cxlmd->dev; > > + cxlmd->cxlm = cxlm; > > + down_write(&cxl_memdev_rwsem); > > + up_write(&cxl_memdev_rwsem); > > +} > > No reason not to put the assignment inside the lock. Though using the > lock at all is overkill as the pointer hasn't left the local stack > frame at this point.
Right, I was considering just leaving it as a bare pointer assignment, in fact that must be sufficient as publishing the cdev needs to depend on all cdev init having completed. So if this write somehow leaks into cdev_device_add() there are much larger problems afoot. > > > err_add: > > - ida_free(&cxl_memdev_ida, cxlmd->id); > > -err_id: > > /* > > - * Theoretically userspace could have already entered the fops, > > - * so flush ops_active. > > + * The cdev was briefly live, shutdown any ioctl operations that > > + * saw that state. > > */ > > Wow it is really subtle that cdev_device_add has this tiny hole where > it can fail but have already allowed open() :< Yes, this was something I wanted to address in the cdev api proposal integrating the debugfs fops proxy / reference counting aproach. I want a cdev api that does not allow open until after the associated device has registered and fired the KOBJ_ADD event. > > Other than the lock it looks OK > > Reviewed-by: Jason Gunthorpe <[email protected]> Thanks, Jason.
