Re: [PATCH v7 1/4] vfio: Mediated device Core driver

[Alex Williamson <alex.williamson@redhat.com>]

On Fri, 9 Sep 2016 23:18:45 +0530
Kirti Wankhede <kwankhede@nvidia.com> wrote:

> On 9/8/2016 1:39 PM, Jike Song wrote:
> > On 08/25/2016 11:53 AM, Kirti Wankhede wrote:  
> 
> >>  +---------------+
> >>  |               |
> >>  | +-----------+ |  mdev_register_driver() +--------------+
> >>  | |           | +<------------------------+ __init()     |
> >>  | |  mdev     | |                         |              |
> >>  | |  bus      | +------------------------>+              |<-> VFIO user
> >>  | |  driver   | |     probe()/remove()    | vfio_mdev.ko |    APIs
> >>  | |           | |                         |              |
> >>  | +-----------+ |                         +--------------+
> >>  |               |  
> > 
> > This aimed to have only one single vfio bus driver for all mediated devices,
> > right?
> >  
> 
> Yes. That's correct.
> 
> 
> >> +
> >> +static int mdev_add_attribute_group(struct device *dev,
> >> +				    const struct attribute_group **groups)
> >> +{
> >> +	return sysfs_create_groups(&dev->kobj, groups);
> >> +}
> >> +
> >> +static void mdev_remove_attribute_group(struct device *dev,
> >> +					const struct attribute_group **groups)
> >> +{
> >> +	sysfs_remove_groups(&dev->kobj, groups);
> >> +}  
> > 
> > These functions are not necessary. You can always specify the attribute groups
> > to dev->groups before registering a new device.
> >   
> 
> At the time of mdev device create, I specifically didn't used
> dev->groups because we callback in vendor driver before that, see below
> code snippet, and those attributes should only be added if create()
> callback returns success.
> 
>         ret = parent->ops->create(mdev, mdev_params);
>         if (ret)
>                 return ret;
> 
>         ret = mdev_add_attribute_group(&mdev->dev,
>                                         parent->ops->mdev_attr_groups);
>         if (ret)
>                 parent->ops->destroy(mdev);
> 
> 
> 
> >> +
> >> +static struct parent_device *mdev_get_parent_from_dev(struct device *dev)
> >> +{
> >> +	struct parent_device *parent;
> >> +
> >> +	mutex_lock(&parent_list_lock);
> >> +	parent = mdev_get_parent(__find_parent_device(dev));
> >> +	mutex_unlock(&parent_list_lock);
> >> +
> >> +	return parent;
> >> +}  
> > 
> > As we have demonstrated, all these refs and locks and release workqueue are not necessary,
> > as long as you have an independent device associated with the mdev host device
> > ("parent" device here).
> >  
> 
> I don't think every lock will go away with that. This also changes how
> mdev devices entries are created in sysfs. It adds an extra directory.

Exposing the parent-child relationship through sysfs is a desirable
feature, so I'm not sure how this is a negative.  This part of Jike's
conversion was a big improvement, I thought.  Thanks,

Alex