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

[Dave Jiang]

On 2/10/2021 4:46 PM, Jason Gunthorpe wrote:
> On Fri, Feb 05, 2021 at 01:53:18PM -0700, Dave Jiang wrote:
> > diff --git a/drivers/dma/idxd/idxd.h b/drivers/dma/idxd/idxd.h
> > index a2438b3166db..f02c96164515 100644
> > +++ b/drivers/dma/idxd/idxd.h
> > @@ -8,6 +8,7 @@
> >   #include <linux/percpu-rwsem.h>
> >   #include <linux/wait.h>
> >   #include <linux/cdev.h>
> > +#include <linux/auxiliary_bus.h>
> >   #include "registers.h"
> >   
> >   #define IDXD_DRIVER_VERSION	"1.00"
> > @@ -221,6 +222,8 @@ struct idxd_device {
> >   	struct work_struct work;
> >   
> >   	int *int_handles;
> > +
> > +	struct auxiliary_device *mdev_auxdev;
> >   };
> If there is only one aux device there not much reason to make it a
> dedicated allocation.

Hi Jason. Thank you for the review. Very much appreciated!

Yep. I had it embedded and then changed it when I was working on the 
UACCE bits to make it uniform. Should've just kept it the way it was.

> >   /* IDXD software descriptor */
> > @@ -282,6 +285,10 @@ enum idxd_interrupt_type {
> >   	IDXD_IRQ_IMS,
> >   };
> >   
> > +struct idxd_mdev_aux_drv {
> > +	        struct auxiliary_driver auxiliary_drv;
> > +};
> Wrong indent. What is this even for?

Will remove.

> > +
> >   static inline int idxd_get_wq_portal_offset(enum idxd_portal_prot prot,
> >   					    enum idxd_interrupt_type irq_type)
> >   {
> > diff --git a/drivers/dma/idxd/init.c b/drivers/dma/idxd/init.c
> > index ee56b92108d8..fd57f39e4b7d 100644
> > +++ b/drivers/dma/idxd/init.c
> > @@ -382,6 +382,74 @@ static void idxd_disable_system_pasid(struct idxd_device *idxd)
> >   	idxd->sva = NULL;
> >   }
> >   
> > +static void idxd_remove_mdev_auxdev(struct idxd_device *idxd)
> > +{
> > +	if (!IS_ENABLED(CONFIG_VFIO_MDEV_IDXD))
> > +		return;
> > +
> > +	auxiliary_device_delete(idxd->mdev_auxdev);
> > +	auxiliary_device_uninit(idxd->mdev_auxdev);
> > +}
> > +
> > +static void idxd_auxdev_release(struct device *dev)
> > +{
> > +	struct auxiliary_device *auxdev = to_auxiliary_dev(dev);
> > +	struct idxd_device *idxd = dev_get_drvdata(dev);
> Nope, where did you see drvdata being used like this? You need to use
> container_of.
>
> If put the mdev_auxdev as a non pointer member then this is just:
>
>       struct idxd_device *idxd = container_of(dev, struct idxd_device, mdev_auxdev)
>       
>       put_device(&idxd->conf_dev);
>
> And fix the 'setup' to match this design

Yes. Once it's embedded, everything falls in place. The drvdata hack was 
to deal with the auxdev being a pointer.


> > +	kfree(auxdev->name);
> This is weird, the name shouldn't be allocated, it is supposed to be a
> fixed string to make it easy to find the driver name in the code base.

Will fix.


> > +static int idxd_setup_mdev_auxdev(struct idxd_device *idxd)
> > +{
> > +	struct auxiliary_device *auxdev;
> > +	struct device *dev = &idxd->pdev->dev;
> > +	int rc;
> > +
> > +	if (!IS_ENABLED(CONFIG_VFIO_MDEV_IDXD))
> > +		return 0;
> > +
> > +	auxdev = kzalloc(sizeof(*auxdev), GFP_KERNEL);
> > +	if (!auxdev)
> > +		return -ENOMEM;
> > +
> > +	auxdev->name = kasprintf(GFP_KERNEL, "mdev-%s", idxd_name[idxd->type]);
> > +	if (!auxdev->name) {
> > +		rc = -ENOMEM;
> > +		goto err_name;
> > +	}
> > +
> > +	dev_dbg(&idxd->pdev->dev, "aux dev mdev: %s\n", auxdev->name);
> > +
> > +	auxdev->dev.parent = dev;
> > +	auxdev->dev.release = idxd_auxdev_release;
> > +	auxdev->id = idxd->id;
> > +
> > +	rc = auxiliary_device_init(auxdev);
> > +	if (rc < 0) {
> > +		dev_err(dev, "Failed to init aux dev: %d\n", rc);
> > +		goto err_auxdev;
> > +	}
> Put the init earlier so it can handle the error unwinds

I think with auxdev embedded, there's really not much going on so I 
think this resolves itself.


> > +	rc = auxiliary_device_add(auxdev);
> > +	if (rc < 0) {
> > +		dev_err(dev, "Failed to add aux dev: %d\n", rc);
> > +		goto err_auxdev;
> > +	}
> > +
> > +	idxd->mdev_auxdev = auxdev;
> > +	dev_set_drvdata(&auxdev->dev, idxd);
> No to using drvdata, and this is in the wrong order anyhow.
>
> > +	return 0;
> > +
> > + err_auxdev:
> > +	kfree(auxdev->name);
> > + err_name:
> > +	kfree(auxdev);
> > +	return rc;
> > +}
> > +
> >   static int idxd_probe(struct idxd_device *idxd)
> >   {
> >   	struct pci_dev *pdev = idxd->pdev;
> > @@ -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?

> 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?


> > +static int idxd_mdev_host_init(struct idxd_device *idxd)
> > +{
> > +	/* FIXME: Fill in later */
> > +	return 0;
> > +}
> > +
> > +static int idxd_mdev_host_release(struct idxd_device *idxd)
> > +{
> > +	/* FIXME: Fill in later */
> > +	return 0;
> > +}
> Don't leave empty stubs like this, just provide the whole driver in
> the next patch

Ok will do that.


> > +static int idxd_mdev_aux_probe(struct auxiliary_device *auxdev,
> > +			       const struct auxiliary_device_id *id)
> > +{
> > +	struct idxd_device *idxd = dev_get_drvdata(&auxdev->dev);
> Continuing no to using drvdata, must use container_of
>
> > +	int rc;
> > +
> > +	rc = idxd_mdev_host_init(idxd);
> And why add this indirection? Just write what it here

ok


> > +static struct idxd_mdev_aux_drv idxd_mdev_aux_drv = {
> > +	.auxiliary_drv = {
> > +		.id_table = idxd_mdev_auxbus_id_table,
> > +		.probe = idxd_mdev_aux_probe,
> > +		.remove = idxd_mdev_aux_remove,
> > +	},
> > +};
> Why idxd_mdev_aux_drv ? Does a later patch add something here?


Yes. There is a callback function that's added later. But I can code it 
so that it gets changed later on.


> > +static int idxd_mdev_auxdev_drv_register(struct idxd_mdev_aux_drv *drv)
> > +{
> > +	return auxiliary_driver_register(&drv->auxiliary_drv);
> > +}
> > +
> > +static void idxd_mdev_auxdev_drv_unregister(struct idxd_mdev_aux_drv *drv)
> > +{
> > +	auxiliary_driver_unregister(&drv->auxiliary_drv);
> > +}
> > +
> > +module_driver(idxd_mdev_aux_drv, idxd_mdev_auxdev_drv_register, idxd_mdev_auxdev_drv_unregister);
> There is some auxillary driver macro that does this boilerplate

Ok thanks.


> Jason