On Fri, 9 Sep 2016 23:18:45 +0530
Kirti Wankhede <kwankhede@xxxxxxxxxx> 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
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
