On 5/25/2016 1:25 PM, Tian, Kevin wrote: >> From: Kirti Wankhede [mailto:kwankhede@xxxxxxxxxx] >> 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 -- 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
