Re: [PATCH v5 04/14] vfio/mdev: idxd: Add auxialary device plumbing for idxd mdev support

[Jason Gunthorpe <jgg@nvidia.com>]

On Fri, Feb 12, 2021 at 11:56:24AM -0700, Dave Jiang wrote:

> > > @@ -434,11 +502,19 @@ static int idxd_probe(struct idxd_device *idxd)
> > >   		goto err_idr_fail;
> > >   	}
> > > +	rc = idxd_setup_mdev_auxdev(idxd);
> > > +	if (rc < 0)
> > > +		goto err_auxdev_fail;
> > > +
> > >   	idxd->major = idxd_cdev_get_major(idxd);
> > >   	dev_dbg(dev, "IDXD device %d probed successfully\n", idxd->id);
> > >   	return 0;
> > > + err_auxdev_fail:
> > > +	mutex_lock(&idxd_idr_lock);
> > > +	idr_remove(&idxd_idrs[idxd->type], idxd->id);
> > > +	mutex_unlock(&idxd_idr_lock);
> > Probably wrong to order things like this..
> 
> How should it be ordered?

The IDR is global data so some other thread could have read the IDR
and got this pointer, but now it is being torn down in some racy
way. It is best to make the store to global access be the very last
thing so you never have to try to unstore from global memory and don't
have to think about concurrency.

> > Also somehow this has a
> > 
> > 	idxd = devm_kzalloc(dev, sizeof(struct idxd_device), GFP_KERNEL);
> > 
> > but the idxd has a kref'd struct device in it:
> 
> So the conf_dev is a struct device that let the driver do configuration of
> the device and other components through sysfs. It's a child device to the
> pdev. It should have no relation to the auxdev. The confdevs for each
> component should not be released until the physical device is released. For
> the mdev case, the auxdev shouldn't be released until the removal of the
> pdev as well since it is a child of the pdev also.
> 
> pdev --- device conf_dev --- wq conf_dev
> 
>     |                   |--- engine conf_dev
> 
>     |                   |--- group conf_dev
> 
>     |--- aux_dev
> 
> > 
> > struct idxd_device {
> > 	enum idxd_type type;
> > 	struct device conf_dev;
> > 
> > So that's not right either
> > 
> > You'll need to fix the lifetime model for idxd_device before you get
> > to adding auxdevices
> 
> Can you kindly expand on how it's suppose to look like please?

Well, you can't call kfree on memory that contains a struct device,
you have to use put_device() - so the devm_kzalloc is unconditionally
wrong. Maybe you could replace it with a devm put device action, but
it would probably be alot saner to just put the required put_device's
where they need to be in the first place.

I didn't try to work out what this was all for, but once it is sorted
out you can just embed the aux device here and chain its release to
put_device on the conf_dev and all the lifetime will work out
naturally. 

Jason