lkml.org 
[lkml]   [2021]   [Feb]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v5 04/14] vfio/mdev: idxd: Add auxialary device plumbing for idxd mdev support
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

\
 
 \ /
  Last update: 2021-02-11 00:49    [W:0.141 / U:0.896 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site