On Wed, Apr 28, 2021 at 08:55:51AM -0400, Eric Farman wrote: > On Tue, 2021-04-27 at 19:10 -0300, Jason Gunthorpe wrote: > > On Tue, Apr 27, 2021 at 04:06:04PM -0400, Eric Farman wrote: > > > > @@ -132,19 +137,28 @@ static int vfio_ccw_mdev_create(struct > > > > mdev_device *mdev) > > > > private->sch->schid.ssid, > > > > private->sch->schid.sch_no); > > > > > > > > + ret = vfio_register_group_dev(&private->vdev); > > > > + if (ret) > > > > + goto err_atomic; > > > > + dev_set_drvdata(&mdev->dev, private); > > > > return 0; > > > > + > > > > +err_atomic: > > > > + atomic_inc(&private->avail); > > > > > > Since we're unwinding, should also do > > > > > > private->mdev = NULL > > > private->state = VFIO_CCW_STATE_STANDBY > > > > I can change this, but it looks quite weird to do stuff like this > > with > > no locking. > > I agree, but mdev_create didn't fail before, so backing out part of its > work seems weird too. Before if vfio_register_group_dev() failed the device would be left half created but without a driver attached. It wasn't good. The way it should work is up until vfio_register_group_dev() returns success there should be no concurrancy and no touches to 'private' - those WQs should all be shutdown. Ideally the private would be allocated here as well so these rules are clear and obvious Jason
