On Mon, Dec 13, 2010 at 3:04 PM, Dave Young <hidave.darkstar@xxxxxxxxx> wrote: > On Sun, Dec 12, 2010 at 05:13:47PM +0100, Torsten Kaiser wrote: >> On Sun, Dec 12, 2010 at 2:15 PM, Dave Young <hidave.darkstar@xxxxxxxxx> wrote: >> > oops happen in bttv_open while locking uninitialized mutex fh->cap.vb_lock >> > add mutex_init before usage >> >> I have seen the same problem twice since I switched of the BKL in >> 2.6.37-rc2, but only had the time today to search for the cause. >> (It only happend on two boots out of probably more then 50, so I only >> investigated after it happened a second time with -rc5) >> >> The cause was making this change to bttv_open() and radio_open() to >> remove the BKL from them: >> + mutex_lock(&fh->cap.vb_lock); >> *fh = btv->init; >> + mutex_unlock(&fh->cap.vb_lock); >> >> This is wrong because of two things: >> First: The fh->cap.vb_lock has not been initialised, as you noted. >> Second: The middle line will overwrite the mutex! >> >> Just adding mutex_init() will not really help the second problem and >> seems to be wrong from the point that cap.vb_lock already has a >> mutex_init() via videobuf_queue_sg_init(). > > Yes, you are right. I wonder if the above lock code in bttv_open and radio_open can be removed or just use btv->lock. In bttv_open there is a comment directly above this lock/unlock sequence that explains why cap.vb_lock was used. As that comment sounded correct, I did not look further into changing it. Hmmh: I'm not sure, if I understand how this locking should work. It looks like each bttv_open() call will create a bttv_fh structure with its own cap.vb_lock. So if there ever is more then one bttv_fh for one btv, there is nothing that prevents concurrent access to btv->init, because there are two different cap.vb_lock. But if there always is only one bttv_fh, then why allocate it at all and not exclusively use btv->init, as that one will always be there already. Disclaimer: I only started looking into bttv-driver.c yesterday when I was trying to find out, why I got these OOPSes. I probably missed something there... >> I'm not sure what the correct fix is, but I would rather suggest this: >> >> * change &fh->cap.vb_lock in bttv_open() AND radio_open() to >> &btv->init.cap.vb_lock >> * add a mutex_init(&btv->init.cap.vb_lock) to the setup of init in bttv_probe() > > I will test this when the bttv card is available for me tommorrow. I do have a bttv card, so if you want me to test something, I could do this. Although: the current version of the code works for me. Even after the OOPS during bootup, I was later able to watch TV with that card. >> > Signed-off-by: Dave Young <hidave.darkstar@xxxxxxxxx> >> > Tested-by: Chris Clayton <chris2553@xxxxxxxxxxxxxx> >> > --- >> > drivers/media/video/bt8xx/bttv-driver.c | 2 ++ >> > 1 file changed, 2 insertions(+) >> > >> > --- linux-2.6.orig/drivers/media/video/bt8xx/bttv-driver.c 2010-11-27 11:21:30.000000000 +0800 >> > +++ linux-2.6/drivers/media/video/bt8xx/bttv-driver.c 2010-12-12 16:31:39.633333338 +0800 >> > @@ -3291,6 +3291,8 @@ static int bttv_open(struct file *file) >> > fh = kmalloc(sizeof(*fh), GFP_KERNEL); >> > if (unlikely(!fh)) >> > return -ENOMEM; >> > + >> > + mutex_init(&fh->cap.vb_lock); >> > file->private_data = fh; >> > >> > /* >> > -- >> > 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/ >> > > -- 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
