qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [RFC PATCH v4 1/3] Mediated device Core driver


From: Tian, Kevin
Subject: Re: [Qemu-devel] [RFC PATCH v4 1/3] Mediated device Core driver
Date: Fri, 27 May 2016 09:00:17 +0000

> From: Kirti Wankhede
> Sent: Wednesday, May 25, 2016 10:47 PM
> 
> 
> >> +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'

well, I think it should be clear in this context where 'p' means 'physical'.
Just like frequently used pdev in pci files for pci_dev...

> 
> 
> >> +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.

IMO 'mdev' should be clearly enough to represent a mediated device

> 
> 
> > 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 UUID alone cannot universally identify a mediated device, what's the
point of explicitly tagging it in kernel? Either we assign a new UUID for
this mdev itself, or possibly better define it as a string? Management 
stack can pass any ID/name in string format which is sufficiently to identify 
mdev to its own purpose? Then in this framework we just do simple
string match...

> 
> 
> 
> >> +          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.

Regardless of whether caller will handle error, this function itself should
return error since it makes sense in other path e.g. sysfs to let user
know what's happening.

> 
> >> +                          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().

I'm not sure whether there'll be some race condition here, since you
put a defunc mdev on the list... 

> 
> >> +  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.

phy_devices.list_lock only protects the list, when you plan to add a
new phy_device node after it's initialized and get ready. sysfs
attribute setup is still part of initialization.


Thanks
Kevin



reply via email to

[Prev in Thread] Current Thread [Next in Thread]