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

[Jason Gunthorpe]

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.

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

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

> +	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.

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

> +	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..

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:

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

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

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

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

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

Jason