Re: [v4l-dvb-maintainer] [GIT PATCHES] V4L/DVB updates

[Trent Piepho <xyzzy@speakeasy.org>]

On Mon, 16 Apr 2007, Markus Rechberger wrote:
> On 4/16/07, Michael Krufky <mkrufky@linuxtv.org> wrote:
> > Adrian Bunk wrote:
> > > On Sun, Apr 15, 2007 at 08:33:38PM -0400, Michael Krufky wrote:
> > >> Mauro,
> > >>
> > >> I've been out of town for the past few days... I just got home and saw
> > this:
> > >>
> > >>
> > >> Mauro Carvalho Chehab wrote:
> > >>>    - Fix 1/3 for bug 7819: fixed frontend hotplug issue
> > >>>    - Fix 2/3 for bug 7819: demux and dvr
> > >>>    - Fix 3/3 for bug 7819: fixed hotplugging for dvbnet
> > >> I don't think that this is 2.6.21 material.  These patches have not yet
> > >> received
> > >> enough testing to be sent to mainline.

I wish these patches had more comments about how they worked, because it's
not clear to me exactly what they are doing or why they do it.

Fix 3/3 for bug 7819: fixed hotplugging for dvbnet

I'm just going to talk about this one patch, since it appears to be the
simplest.

This patch removes the 'wait_queue' memory of dvb_demux.  Why does a patch
to dvbnet have anything to do with the demux device?  Should this change
have been part of the previous patch, the one what fixes demux and dvr?

The patch replace the (file_operations)->release pointer with the new
function dvb_net_close() in place of dvb_generic_release().  The first
part of dvb_net_close() is just a cut&paste from dvb_generic_release(),
so maybe it would be better to just call dvb_generic_release() instead?

There is also a bug:

+static int dvb_net_close(struct inode *inode, struct file *file)
+{
+       struct dvb_device *dvbdev = file->private_data;
+       struct dvb_net *dvbnet = dvbdev->priv;
+
+       if (!dvbdev)
+               return -ENODEV;

The check for !dvbdev is too late, dvbdev was already dereferenced on the
previous line to get dvbnet, so if dvbdev is NULL you will have already
crashed.

Is the check for !dvbdev unnecessary paranoia?  Or can dvbdev actually be
NULL is some cases?

So, w.r.t. dvbnet, my understanding is that somehow dvb_net_release() is
called while the net device is still in use.  Is that correct?
dvb_net_release() will end up deleting some stuff that needs to be around
while there is still a user of the device?

Would it be possible to keep dvb_net_release() from getting called in the
first place until there are no users of the device?

When looking at this code, keep in mind that users starts at 1 and goes
down as the device is used.

void dvb_net_release (struct dvb_net *dvbnet)
{
        int i;

        dvbnet->exit = 1;
        if (dvbnet->dvbdev->users < 1)		/* line A */
                wait_event(dvbnet->dvbdev->wait_queue,  /* line B */
                                dvbnet->dvbdev->users==1);
	/* line C */
        dvb_unregister_device(dvbnet->dvbdev);
	[...]
}

Isn't there a race condition here?  What if there are no users of the
device at the time line A runs, but the device is opened before line C
runs?  After the wait_even() on line B returns there are no users, but what
if new users opens the device after the after line B?

Is seems like you need something to prevent new users from opening the
device, i.e. make it so that dvbnet->exit=1 prevents the device from
getting opened, but if that's the intention, I don't see where it's
happening.

In dvb_net_close():

+       if(dvbdev->users == 1 && dvbnet->exit==1) {
+               fops_put(file->f_op);
+               file->f_op = NULL;
+               wake_up(&dvbdev->wait_queue);
+       }

What is the purpose of the fops_put() call and setting file->f_op to NULL?