Message-ID: <5819C08A.7080703@intel.com>
Date: Wed, 02 Nov 2016 18:31:38 +0800
From: Jike Song <jike.song@intel.com>
User-Agent: Mozilla/5.0 (X11; Linux i686 on x86_64; rv:17.0) Gecko/20130801 Thunderbird/17.0.8
MIME-Version: 1.0
To: Kirti Wankhede <kwankhede@nvidia.com>
CC: kevin.tian@intel.com, cjia@nvidia.com, kvm@vger.kernel.org,
        qemu-devel@nongnu.org, linux-kernel@vger.kernel.org,
        alex.williamson@redhat.com, kraxel@redhat.com, pbonzini@redhat.com,
        bjsdjshi@linux.vnet.ibm.com
Subject: Re: [Qemu-devel] [PATCH v10 01/19] vfio: Mediated device Core driver
References: <1477517366-27871-1-git-send-email-kwankhede@nvidia.com> <1477517366-27871-2-git-send-email-kwankhede@nvidia.com> <581425F9.5070902@intel.com> <c339d6f5-8dba-f69b-b0bc-baf45f2d5ab5@nvidia.com> <5814E63F.9070609@intel.com> <9d417e40-f4d3-ab3c-47a1-ac730e6e4ae6@nvidia.com>
In-Reply-To: <9d417e40-f4d3-ab3c-47a1-ac730e6e4ae6@nvidia.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2625
Lines: 99

On 11/02/2016 03:59 PM, Kirti Wankhede wrote:
> On 10/29/2016 11:41 PM, Jike Song wrote:
>> On 10/29/2016 06:06 PM, Kirti Wankhede wrote:
>>>
>>>
>>> On 10/29/2016 10:00 AM, Jike Song wrote:
>>>> On 10/27/2016 05:29 AM, Kirti Wankhede wrote:
>>>>> +int mdev_register_device(struct device *dev, const struct parent_ops *ops)
>>>>> +{
>>>>> +	int ret;
>>>>> +	struct parent_device *parent;
>>>>> +
>>>>> +	/* check for mandatory ops */
>>>>> +	if (!ops || !ops->create || !ops->remove || !ops->supported_type_groups)
>>>>> +		return -EINVAL;
>>>>> +
>>>>> +	dev = get_device(dev);
>>>>> +	if (!dev)
>>>>> +		return -EINVAL;
>>>>> +
>>>>> +	mutex_lock(&parent_list_lock);
>>>>> +
>>>>> +	/* Check for duplicate */
>>>>> +	parent = __find_parent_device(dev);
>>>>> +	if (parent) {
>>>>> +		ret = -EEXIST;
>>>>> +		goto add_dev_err;
>>>>> +	}
>>>>> +
>>>>> +	parent = kzalloc(sizeof(*parent), GFP_KERNEL);
>>>>> +	if (!parent) {
>>>>> +		ret = -ENOMEM;
>>>>> +		goto add_dev_err;
>>>>> +	}
>>>>> +
>>>>> +	kref_init(&parent->ref);
>>>>> +	mutex_init(&parent->lock);
>>>>> +
>>>>> +	parent->dev = dev;
>>>>> +	parent->ops = ops;
>>>>> +
>>>>> +	ret = parent_create_sysfs_files(parent);
>>>>> +	if (ret) {
>>>>> +		mutex_unlock(&parent_list_lock);
>>>>> +		mdev_put_parent(parent);
>>>>> +		return ret;
>>>>> +	}
>>>>> +
>>>>> +	ret = class_compat_create_link(mdev_bus_compat_class, dev, NULL);
>>>>> +	if (ret)
>>>>> +		dev_warn(dev, "Failed to create compatibility class link\n");
>>>>> +
>>>>
>>>> Hi Kirti,
>>>>
>>>> Like I replied to previous version:
>>>>
>>>> 	http://www.spinics.net/lists/kvm/msg139331.html
>>>>
>>>
>>> Hi Jike,
>>>
>>> I saw your reply but by that time v10 version of patch series was out
>>> for review.
>>>
>>
>> Ah..yes, I forgot that :)
>>
>>>> You can always check if mdev_bus_compat_class already registered
>>>> here, and register it if not yet. Same logic should be adopted to
>>>> mdev_init.
>>>>
>>>> Current implementation will simply panic if configured as builtin,
>>>> which is rare but far from impossible.
>>>>
>>>
>>> Can you verify attached patch with v10 patch-set whether this works for you?
>>> I'll incorporate this change in my next version.
>>>
>>
>> Seems cool. But would you please also keep the register in mdev_init(),
>> just check the 'in case it was already registered' case? Thanks!
>>
> 
> The class is used only to keep symbolic to the devices which are
> registered to mdev framework. So I think its ok to register this class
> when first device is registered.
> 

That's also cool :-)

So if you like it:

Reviewed-by: Jike Song <jike.song@intel.com>


--
Thanks,
Jike