On 5/25/2016 1:25 PM, Tian, Kevin wrote:
>> From: Kirti Wankhede [mailto:kwankh...@nvidia.com]
>> Sent: Wednesday, May 25, 2016 3:58 AM
>>
...
>> +
>> +config MDEV
>> + tristate "Mediated device driver framework"
>
> Sorry not a native speaker. Is it cleaner to say "Driver framework for
Mediated
> Devices" or "Mediated Device Framework"? Should we focus on driver or
device
> here?
>
Both, device and driver. This framework provides way to register
physical *devices* and also register *driver* for mediated devices.
>> + depends on VFIO
>> + default n
>> + help
>> + MDEV provides a framework to virtualize device without
SR-IOV cap
>> + See Documentation/mdev.txt for more details.
>
> Looks Documentation/mdev.txt is not included in this version.
>
Yes, will have Documentation/mdev.txt in next version of patch.
>> +static struct devices_list {
>> + struct list_head dev_list;
>> + struct mutex list_lock;
>> +} mdevices, phy_devices;
>
> phy_devices -> pdevices? and similarly we can use pdev/mdev
> pair in other places...
>
'pdevices' sometimes also refers to 'pointer to devices' that's the
reason I perfer to use phy_devices to represent 'physical devices'
>> +static struct mdev_device *find_mdev_device(uuid_le uuid, int instance)
>
> can we just call it "struct mdev* or "mdevice"? "dev_device" looks
redundant.
>
'struct mdev_device' represents 'device structure for device created by
mdev module'. Still that doesn't satisfy major folks, I'm open to change
it.
> Sorry I may have to ask same question since I didn't get an answer yet.
> what exactly does 'instance' mean here? since uuid is unique, why do
> we need match instance too?
>
'uuid' could be UUID of a VM for whom it is created. To support mutiple
mediated devices for same VM, name should be unique. Hence we need a
instance number to identify each mediated device uniquely in one VM.
>> + if (phy_dev->ops->destroy) {
>> + if (phy_dev->ops->destroy(phy_dev->dev, mdevice->uuid,
>> + mdevice->instance)) {
>> + mutex_unlock(&phy_devices.list_lock);
>
> a warning message is preferred. Also better to return -EBUSY here.
>
mdev_destroy_device() is called from 2 paths, one is sysfs mdev_destroy
and mdev_unregister_device(). For the later case, return from here will
any ways ignored. mdev_unregister_device() is called from the remove
function of physical device and that doesn't care about return error, it
just removes the device from subsystem.
>> + return;
>> + }
>> + }
>> +
>> + mdev_remove_attribute_group(&mdevice->dev,
>> + phy_dev->ops->mdev_attr_groups);
>> + mdevice->phy_dev = NULL;
>
> Am I missing something here? You didn't remove this mdev node from
> the list, and below...
>
device_unregister() calls put_device(dev) and if refcount is zero its
release function is called, which is mdev_device_release(), that is
hooked during device_register(). This node is removed from list from
mdev_device_release().
>> + mutex_unlock(&phy_devices.list_lock);
>
> you should use mutex of mdevices list
>
No, this lock is for phy_dev.
>> + phy_dev->dev = dev;
>> + phy_dev->ops = ops;
>> +
>> + mutex_lock(&phy_devices.list_lock);
>> + ret = mdev_create_sysfs_files(dev);
>> + if (ret)
>> + goto add_sysfs_error;
>> +
>> + ret = mdev_add_attribute_group(dev, ops->dev_attr_groups);
>> + if (ret)
>> + goto add_group_error;
>
> any reason to include sysfs operations inside the mutex which is
> purely about phy_devices list?
>
dev_attr_groups attribute is for physical device, hence inside
phy_devices.list_lock.
* @dev_attr_groups: Default attributes of the physical device.
>> +void mdev_unregister_device(struct device *dev)
>> +{
>> + struct phy_device *phy_dev;
>> + struct mdev_device *vdev = NULL;
>> +
>> + phy_dev = find_physical_device(dev);
>> +
>> + if (!phy_dev)
>> + return;
>> +
>> + dev_info(dev, "MDEV: Unregistering\n");
>> +
>> + while ((vdev = find_next_mdev_device(phy_dev)))
>> + mdev_destroy_device(vdev);
>
> Need check return value here since ops->destroy may fail.
>
See my comment above.
>> +static void mdev_device_release(struct device *dev)
>
> what's the difference between this release and earlier destroy version?
>
See my comment above
>> +static void __exit mdev_exit(void)
>> +{
>
> should we check any remaining mdev/pdev which are not cleaned
> up correctly here?
>
If there are any physical device registered with this module then the
usage count is not zero so rmmod would anyways fail.
All mdev_devices, which are created for any physical device, are
destroyed from mdev_unregister_device(physial_device);
Hence no need to explicitly add the code here which would never get used.
>> + mdev_bus_unregister();
>> + class_unregister(&mdev_class);
>> +}
Thanks,
Kirti
