by Dave Jiang

[permalink] [raw]

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

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

2021-02-12 19:16:28

by Jason Gunthorpe

[permalink] [raw]

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

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