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

From: Markus Rechberger
Date: Fri Apr 27 2007 - 18:44:14 EST


On 4/27/07, Trent Piepho <xyzzy@xxxxxxxxxxxxx> wrote:
On Mon, 16 Apr 2007, Markus Rechberger wrote:
> On 4/16/07, Michael Krufky <mkrufky@xxxxxxxxxxx> 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 waitqueue got added in a previous patch just ignore it..

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?


no because it needs the dvb_net structure which cannot be used in the
dvb_generic_release function.

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.


I think this check isn't needed at all actually, why should someone
release dvbdev before already?

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?


yes this is a valid argument, practically it will never occure though.
Since I also wrote that I didn't have a dvb signal at the time of
writing this I don't even know if the dvb-net implementation is ok at
all.
I don't even know if that part of the framework works and if someone
really uses it.

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?


fops_put lowers the usecount on the inode, after wake_up the f_op
structure is invalid because it's just allocated memory which gets
freed by the dvb release function.

thanks for reviewing,
Markus
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/