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. > > eg the only reads are here: > > drivers/s390/cio/vfio_ccw_drv.c: if (private->mdev && > is_final) > drivers/s390/cio/vfio_ccw_drv.c: private->state = > private->mdev ? VFIO_CCW_STATE_IDLE : > > Which is from a WQ, if someone thinks setting mdev to NULL should > effect those WQs then there are problems... > > The non-atomic state is equally confusing Agreed, it's already on the list. Eric > > Jason
