On Fri, Dec 24, 2010 at 12:59:38PM +0100, Laurent Pinchart wrote:
> Hi Greg,
>
> Thank you for the review.
>
> On Thursday 23 December 2010 04:32:53 Greg KH wrote:
> > On Mon, Dec 20, 2010 at 12:36:24PM +0100, Laurent Pinchart wrote:
> > > The media_devnode structure provides support for registering and
> > > unregistering character devices using a dynamic major number. Reference
> > > counting is handled internally, making device drivers easier to write
> > > without having to solve the open/disconnect race condition issue over
> > > and over again.
> >
> > What race condition are you referring to?
>
> In a nutshell, the race between device disconnection, which results in the
> device instance being delete (if not in use of course), and open() calls from
> userspace. The problem has been solved in V4L a couple of releases ago after
> suffering from this race for a too long time. As V4L devices (and now media
> devices) need to create both a struct device and a struct cdev instance,
> careful locking is needed.
Ok, that's not really nice, but I guess it is needed.
> > > +config MEDIA_CONTROLLER
> > > + bool "Media Controller API (EXPERIMENTAL)"
> > > + depends on EXPERIMENTAL
> > > + ---help---
> > > + Enable the media controller API used to query media devices internal
> > > + topology and configure it dynamically.
> > > +
> > > + This API is mostly used by camera interfaces in embedded platforms.
> >
> > That's nice, but why should I enable this? Or will drivers enable it
> > automatically?
>
> Drivers depending on the media controller API will enable this, yes. The
> option will probably removed later when the API won't be deemed as
> experimental anymore.
>
> > > +#define MEDIA_NUM_DEVICES 256
> >
> > Why this limit?
>
> Because I'm using a bitmap to store the used minor numbers, and I thus need a
> limit. I could get rid of it of it by using a linked list, but that will not
> be efficient (you could argue that the list will hold a few entries only most
> of the time, but in that case a limit of 256 minors wouldn't be a problem
> :-)).
As it's only needed to be looked up at open() time, why not just make it
dynamic?
> > > +#define MEDIA_NAME "media"
> >
> > Are you sure this is a good name for a camera?
>
> It's not just camera. Media devices are... well, media devices. Basically
> anything that can handle audio and/or video streams. The media controller API
> can be used by plain audio devices.
Ok.
> > > +static dev_t media_dev_t;
> >
> > Only one major number? Is it always dynamic?
>
> Yes, one major and (for now) 256 minors. Is there a problem with it being
> dynamic ?
No, just curious.
> > > +
> > > +/*
> > > + * Active devices
> > > + */
> > > +static DEFINE_MUTEX(media_devnode_lock);
> > > +static DECLARE_BITMAP(media_devnode_nums, MEDIA_NUM_DEVICES);
> > > +
> > > +/* Called when the last user of the media device exits. */
> > > +static void media_devnode_release(struct device *cd)
> > > +{
> > > + struct media_devnode *mdev = to_media_devnode(cd);
> > > +
> > > + mutex_lock(&media_devnode_lock);
> > > +
> > > + /* Delete the cdev on this minor as well */
> > > + cdev_del(&mdev->cdev);
> > > +
> > > + /* Mark device node number as free */
> > > + clear_bit(mdev->minor, media_devnode_nums);
> > > +
> > > + mutex_unlock(&media_devnode_lock);
> > > +
> > > + /* Release media_devnode and perform other cleanups as needed. */
> > > + if (mdev->release)
> > > + mdev->release(mdev);
> > > +}
> >
> > You forgot to free the device structure here as well, right?
>
> That will be done by the release callback. The media_devnode structure is
> embedded in the media_device structure, which will be embedded in driver-
> specific structures.
Ok.
> > > +static ssize_t media_read(struct file *filp, char __user *buf,
> > > + size_t sz, loff_t *off)
> > > +{
> > > + struct media_devnode *mdev = media_devnode_data(filp);
> > > +
> > > + if (!mdev->fops->read)
> > > + return -EINVAL;
> > > + if (!media_devnode_is_registered(mdev))
> > > + return -EIO;
> >
> > How could this happen?
>
> This can happen when a USB device is disconnected for instance.
But what's to keep that from happening on the next line as well? That
doesn't seem like a check you can ever be sure about, so I wouldn't do
it at all.
> > And are you sure -EIO is correct?
>
> -ENXIO is probably better (I always confuse that with -ENODEV).
>
> > > + return mdev->fops->read(filp, buf, sz, off);
> > > +}
> > > +
> > > +static ssize_t media_write(struct file *filp, const char __user *buf,
> > > + size_t sz, loff_t *off)
> > > +{
> > > + struct media_devnode *mdev = media_devnode_data(filp);
> > > +
> > > + if (!mdev->fops->write)
> > > + return -EINVAL;
> > > + if (!media_devnode_is_registered(mdev))
> > > + return -EIO;
> >
> > Same as above, and same comment in other places (poll, ioctl.)
>
> OK.
>
> > > +/* Override for the open function */
> > > +static int media_open(struct inode *inode, struct file *filp)
> > > +{
> > > + struct media_devnode *mdev;
> > > + int ret;
> > > +
> > > + /* Check if the media device is available. This needs to be done with
> > > + * the media_devnode_lock held to prevent an open/unregister race:
> > > + * without the lock, the device could be unregistered and freed between
> > > + * the media_devnode_is_registered() and get_device() calls, leading to
> > > + * a crash.
> > > + */
> > > + mutex_lock(&media_devnode_lock);
> > > + mdev = container_of(inode->i_cdev, struct media_devnode, cdev);
> >
> > By virtue of having the reference to the module held by the vfs, this
> > shouldn't ever go away, even if the lock is not held.
>
> inode->i_cdev is set to NULL by cdev_default_release() which can be called
> from media_devnode_unregister(). I could move to container_of outside the
> lock, but in that case I would have to check for mdev == NULL ||
> !mdev_devnode_is_registered(mdev) (or move the NULL check inside
> mdev_devnode_is_registered). Is that what you would like ?
As container_of _ALWAYS_ returns a valid pointer, you can't check it for
NULL. I don't know, it just doesn't seem correct here, but if you are
sure it's working properly, I'll not push the issue.
> > > + /* return ENXIO if the media device has been removed
> > > + already or if it is not registered anymore. */
> > > + if (!media_devnode_is_registered(mdev)) {
> > > + mutex_unlock(&media_devnode_lock);
> > > + return -ENXIO;
> > > + }
> >
> > So you can unregister a device at any time, even if the device is open,
> > or about to be opened?
>
> That's correct. That way drivers don't need to care about unregister/open
> races, media_devnode will handle it for them.
Ok.
> > Then that's fine, but you can put the lock after the container_of(), right?
>
> If I add a NULL check (as explained above), yes.
Again, you can't check for NULL as the result of container_of() that
does not work (hint, container_of() is just pointer math, without ever
looking at the original pointer value.)
> > > + /* and increase the device refcount */
> > > + get_device(&mdev->dev);
> >
> > How is that holding anything into memory?
>
> That will prevent the device instance from being freed until the device is
> closed, thereby holding both the device instance and the cdev instance in
> memory.
Tricky :)
> > Don't you want to keep the module that the fops pointer in the device in
> > memory, not necessarily the device itself?
>
> The cdev owner pointer is set to the fops owner. Unless I'm mistaken it will
> keep the module in memory. I need to keep the device in memory (or rather the
> media_devnode structure that embeds it) to handle file operations on a device
> that gets unregistered after it has been opened.
Ok.
> > > + mutex_unlock(&media_devnode_lock);
> > > +
> > > + filp->private_data = mdev;
> > > +
> > > + if (mdev->fops->open) {
> > > + ret = mdev->fops->open(filp);
> > > + if (ret) {
> > > + put_device(&mdev->dev);
> > > + return ret;
> > > + }
> > > + }
> > > +
> > > + return 0;
> > > +}
> >
> > No reference counting for the fops? Why not?
>
> Because cdev already increments the owner refcount on open().
Ok.
> > Anyway, it looks like what you really want is an "easier" way to handle
> > a cdev and a struct device that will export the proper information to
> > userspace, right?
> >
> > Why not do this generically, fixing up the cdev interface (which really
> > needs it) and not tie it to media devices at all, making it possible for
> > _everyone_ to use this type of infrastructure?
> >
> > That seems like the better thing to do here.
>
> Sounds like a good idea. You're a better cdev expert than me, so could you
> give me a few pointers ? Do you want me to create a new object that will hold
> a struct cdev and a struct device together, or to embed the device structure
> into the existing cdev structure ?
I don't really know, all I know is that cdev is a difficult thing to
handle at times, but not everyone who uses it needs a struct device.
But some people do (as this code shows), so I guess it needs to be a
whole new structure/interface that binds the two together like you just
did. I think that would be good for a lot more places other than just
the media subsystem, so it should go into the core kernel instead.
thanks,
greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
