From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:34120) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1biQlH-0000lg-0h for qemu-devel@nongnu.org; Fri, 09 Sep 2016 14:42:52 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1biQlB-00054p-Tr for qemu-devel@nongnu.org; Fri, 09 Sep 2016 14:42:49 -0400 Received: from mx1.redhat.com ([209.132.183.28]:33858) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1biQlB-00054l-Lh for qemu-devel@nongnu.org; Fri, 09 Sep 2016 14:42:45 -0400 Date: Fri, 9 Sep 2016 12:42:43 -0600 From: Alex Williamson Message-ID: <20160909124243.0d118541@t450s.home> In-Reply-To: References: <1472097235-6332-1-git-send-email-kwankhede@nvidia.com> <1472097235-6332-2-git-send-email-kwankhede@nvidia.com> <57D11CC3.6010605@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v7 1/4] vfio: Mediated device Core driver List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kirti Wankhede Cc: Jike Song , pbonzini@redhat.com, kraxel@redhat.com, cjia@nvidia.com, qemu-devel@nongnu.org, kvm@vger.kernel.org, kevin.tian@intel.com, bjsdjshi@linux.vnet.ibm.com On Fri, 9 Sep 2016 23:18:45 +0530 Kirti Wankhede 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