From 587f0d5d6d44e3529028bf197d424f866fb2411d Mon Sep 17 00:00:00 2001 From: Mauro Carvalho Chehab Date: Wed, 15 Dec 2010 18:45:42 -0300 Subject: [media] bttv: fix mutex use before init (BZ#24602) Fix a regression where bttv driver causes oopses when loading, since it were using some non-initialized mutexes. While it would be possible to fix the issue, there are some other lock troubles, like to the presence of lock code at free_btres_lock(). It is possible to fix, but the better is to just use the core-assisted locking schema. This way, V4L2 core will serialize access to all ioctl's/open/close/mmap/read/poll operations, avoiding to have two processes accessing the hardware at the same time. Also, as there's just one lock, instead of 3, there's no risk of dead locks. The net result is a cleaner code, with just one lock. Reported-by: Dan Carpenter Reported-by: Brandon Philips Reported-by: Chris Clayton Reported-by: Torsten Kaiser Tested-by: Chris Clayton Tested-by: Torsten Kaiser Signed-off-by: Mauro Carvalho Chehab --- drivers/media/video/bt8xx/bttv-driver.c | 97 +-------------------------------- 1 file changed, 3 insertions(+), 94 deletions(-) (limited to 'drivers/media/video/bt8xx/bttv-driver.c') diff --git a/drivers/media/video/bt8xx/bttv-driver.c b/drivers/media/video/bt8xx/bttv-driver.c index 3da6e80e1041..c14b819d6b78 100644 --- a/drivers/media/video/bt8xx/bttv-driver.c +++ b/drivers/media/video/bt8xx/bttv-driver.c @@ -855,7 +855,6 @@ int check_alloc_btres_lock(struct bttv *btv, struct bttv_fh *fh, int bit) xbits |= RESOURCE_VIDEO_READ | RESOURCE_VIDEO_STREAM; /* is it free? */ - mutex_lock(&btv->lock); if (btv->resources & xbits) { /* no, someone else uses it */ goto fail; @@ -885,11 +884,9 @@ int check_alloc_btres_lock(struct bttv *btv, struct bttv_fh *fh, int bit) /* it's free, grab it */ fh->resources |= bit; btv->resources |= bit; - mutex_unlock(&btv->lock); return 1; fail: - mutex_unlock(&btv->lock); return 0; } @@ -941,7 +938,6 @@ void free_btres_lock(struct bttv *btv, struct bttv_fh *fh, int bits) /* trying to free ressources not allocated by us ... */ printk("bttv: BUG! (btres)\n"); } - mutex_lock(&btv->lock); fh->resources &= ~bits; btv->resources &= ~bits; @@ -952,8 +948,6 @@ void free_btres_lock(struct bttv *btv, struct bttv_fh *fh, int bits) if (0 == (bits & VBI_RESOURCES)) disclaim_vbi_lines(btv); - - mutex_unlock(&btv->lock); } /* ----------------------------------------------------------------------- */ @@ -1714,28 +1708,20 @@ static int bttv_prepare_buffer(struct videobuf_queue *q,struct bttv *btv, /* Make sure tvnorm and vbi_end remain consistent until we're done. */ - mutex_lock(&btv->lock); norm = btv->tvnorm; /* In this mode capturing always starts at defrect.top (default VDELAY), ignoring cropping parameters. */ if (btv->vbi_end > bttv_tvnorms[norm].cropcap.defrect.top) { - mutex_unlock(&btv->lock); return -EINVAL; } - mutex_unlock(&btv->lock); - c.rect = bttv_tvnorms[norm].cropcap.defrect; } else { - mutex_lock(&btv->lock); - norm = btv->tvnorm; c = btv->crop[!!fh->do_crop]; - mutex_unlock(&btv->lock); - if (width < c.min_scaled_width || width > c.max_scaled_width || height < c.min_scaled_height) @@ -1859,7 +1845,6 @@ static int bttv_s_std(struct file *file, void *priv, v4l2_std_id *id) unsigned int i; int err; - mutex_lock(&btv->lock); err = v4l2_prio_check(&btv->prio, fh->prio); if (err) goto err; @@ -1875,7 +1860,6 @@ static int bttv_s_std(struct file *file, void *priv, v4l2_std_id *id) set_tvnorm(btv, i); err: - mutex_unlock(&btv->lock); return err; } @@ -1899,7 +1883,6 @@ static int bttv_enum_input(struct file *file, void *priv, struct bttv *btv = fh->btv; int rc = 0; - mutex_lock(&btv->lock); if (i->index >= bttv_tvcards[btv->c.type].video_inputs) { rc = -EINVAL; goto err; @@ -1929,7 +1912,6 @@ static int bttv_enum_input(struct file *file, void *priv, i->std = BTTV_NORMS; err: - mutex_unlock(&btv->lock); return rc; } @@ -1939,9 +1921,7 @@ static int bttv_g_input(struct file *file, void *priv, unsigned int *i) struct bttv_fh *fh = priv; struct bttv *btv = fh->btv; - mutex_lock(&btv->lock); *i = btv->input; - mutex_unlock(&btv->lock); return 0; } @@ -1953,7 +1933,6 @@ static int bttv_s_input(struct file *file, void *priv, unsigned int i) int err; - mutex_lock(&btv->lock); err = v4l2_prio_check(&btv->prio, fh->prio); if (unlikely(err)) goto err; @@ -1966,7 +1945,6 @@ static int bttv_s_input(struct file *file, void *priv, unsigned int i) set_input(btv, i, btv->tvnorm); err: - mutex_unlock(&btv->lock); return 0; } @@ -1980,7 +1958,6 @@ static int bttv_s_tuner(struct file *file, void *priv, if (unlikely(0 != t->index)) return -EINVAL; - mutex_lock(&btv->lock); if (unlikely(btv->tuner_type == TUNER_ABSENT)) { err = -EINVAL; goto err; @@ -1996,7 +1973,6 @@ static int bttv_s_tuner(struct file *file, void *priv, btv->audio_mode_gpio(btv, t, 1); err: - mutex_unlock(&btv->lock); return 0; } @@ -2007,10 +1983,8 @@ static int bttv_g_frequency(struct file *file, void *priv, struct bttv_fh *fh = priv; struct bttv *btv = fh->btv; - mutex_lock(&btv->lock); f->type = btv->radio_user ? V4L2_TUNER_RADIO : V4L2_TUNER_ANALOG_TV; f->frequency = btv->freq; - mutex_unlock(&btv->lock); return 0; } @@ -2025,7 +1999,6 @@ static int bttv_s_frequency(struct file *file, void *priv, if (unlikely(f->tuner != 0)) return -EINVAL; - mutex_lock(&btv->lock); err = v4l2_prio_check(&btv->prio, fh->prio); if (unlikely(err)) goto err; @@ -2040,7 +2013,6 @@ static int bttv_s_frequency(struct file *file, void *priv, if (btv->has_matchbox && btv->radio_user) tea5757_set_freq(btv, btv->freq); err: - mutex_unlock(&btv->lock); return 0; } @@ -2173,7 +2145,6 @@ limit_scaled_size_lock (struct bttv_fh * fh, /* Make sure tvnorm, vbi_end and the current cropping parameters remain consistent until we're done. */ - mutex_lock(&btv->lock); b = &bttv_tvnorms[btv->tvnorm].cropcap.bounds; @@ -2251,7 +2222,6 @@ limit_scaled_size_lock (struct bttv_fh * fh, rc = 0; /* success */ fail: - mutex_unlock(&btv->lock); return rc; } @@ -2283,9 +2253,7 @@ verify_window_lock (struct bttv_fh * fh, if (V4L2_FIELD_ANY == field) { __s32 height2; - mutex_lock(&fh->btv->lock); height2 = fh->btv->crop[!!fh->do_crop].rect.height >> 1; - mutex_unlock(&fh->btv->lock); field = (win->w.height > height2) ? V4L2_FIELD_INTERLACED : V4L2_FIELD_TOP; @@ -2361,7 +2329,6 @@ static int setup_window_lock(struct bttv_fh *fh, struct bttv *btv, } } - mutex_lock(&fh->cap.vb_lock); /* clip against screen */ if (NULL != btv->fbuf.base) n = btcx_screen_clips(btv->fbuf.fmt.width, btv->fbuf.fmt.height, @@ -2413,7 +2380,6 @@ static int setup_window_lock(struct bttv_fh *fh, struct bttv *btv, bttv_overlay_risc(btv, &fh->ov, fh->ovfmt, new); retval = bttv_switch_overlay(btv,fh,new); } - mutex_unlock(&fh->cap.vb_lock); return retval; } @@ -2527,9 +2493,7 @@ static int bttv_try_fmt_vid_cap(struct file *file, void *priv, if (V4L2_FIELD_ANY == field) { __s32 height2; - mutex_lock(&btv->lock); height2 = btv->crop[!!fh->do_crop].rect.height >> 1; - mutex_unlock(&btv->lock); field = (f->fmt.pix.height > height2) ? V4L2_FIELD_INTERLACED : V4L2_FIELD_BOTTOM; @@ -2615,7 +2579,6 @@ static int bttv_s_fmt_vid_cap(struct file *file, void *priv, fmt = format_by_fourcc(f->fmt.pix.pixelformat); /* update our state informations */ - mutex_lock(&fh->cap.vb_lock); fh->fmt = fmt; fh->cap.field = f->fmt.pix.field; fh->cap.last = V4L2_FIELD_NONE; @@ -2624,7 +2587,6 @@ static int bttv_s_fmt_vid_cap(struct file *file, void *priv, btv->init.fmt = fmt; btv->init.width = f->fmt.pix.width; btv->init.height = f->fmt.pix.height; - mutex_unlock(&fh->cap.vb_lock); return 0; } @@ -2650,11 +2612,9 @@ static int vidiocgmbuf(struct file *file, void *priv, struct video_mbuf *mbuf) unsigned int i; struct bttv_fh *fh = priv; - mutex_lock(&fh->cap.vb_lock); retval = __videobuf_mmap_setup(&fh->cap, gbuffers, gbufsize, V4L2_MEMORY_MMAP); if (retval < 0) { - mutex_unlock(&fh->cap.vb_lock); return retval; } @@ -2666,7 +2626,6 @@ static int vidiocgmbuf(struct file *file, void *priv, struct video_mbuf *mbuf) for (i = 0; i < gbuffers; i++) mbuf->offsets[i] = i * gbufsize; - mutex_unlock(&fh->cap.vb_lock); return 0; } #endif @@ -2776,10 +2735,8 @@ static int bttv_overlay(struct file *file, void *f, unsigned int on) int retval = 0; if (on) { - mutex_lock(&fh->cap.vb_lock); /* verify args */ if (unlikely(!btv->fbuf.base)) { - mutex_unlock(&fh->cap.vb_lock); return -EINVAL; } if (unlikely(!fh->ov.setup_ok)) { @@ -2788,13 +2745,11 @@ static int bttv_overlay(struct file *file, void *f, unsigned int on) } if (retval) return retval; - mutex_unlock(&fh->cap.vb_lock); } if (!check_alloc_btres_lock(btv, fh, RESOURCE_OVERLAY)) return -EBUSY; - mutex_lock(&fh->cap.vb_lock); if (on) { fh->ov.tvnorm = btv->tvnorm; new = videobuf_sg_alloc(sizeof(*new)); @@ -2806,7 +2761,6 @@ static int bttv_overlay(struct file *file, void *f, unsigned int on) /* switch over */ retval = bttv_switch_overlay(btv, fh, new); - mutex_unlock(&fh->cap.vb_lock); return retval; } @@ -2845,7 +2799,6 @@ static int bttv_s_fbuf(struct file *file, void *f, } /* ok, accept it */ - mutex_lock(&fh->cap.vb_lock); btv->fbuf.base = fb->base; btv->fbuf.fmt.width = fb->fmt.width; btv->fbuf.fmt.height = fb->fmt.height; @@ -2877,7 +2830,6 @@ static int bttv_s_fbuf(struct file *file, void *f, retval = bttv_switch_overlay(btv, fh, new); } } - mutex_unlock(&fh->cap.vb_lock); return retval; } @@ -2956,7 +2908,6 @@ static int bttv_queryctrl(struct file *file, void *priv, c->id >= V4L2_CID_PRIVATE_LASTP1)) return -EINVAL; - mutex_lock(&btv->lock); if (!btv->volume_gpio && (c->id == V4L2_CID_AUDIO_VOLUME)) *c = no_ctl; else { @@ -2964,7 +2915,6 @@ static int bttv_queryctrl(struct file *file, void *priv, *c = (NULL != ctrl) ? *ctrl : no_ctl; } - mutex_unlock(&btv->lock); return 0; } @@ -2975,10 +2925,8 @@ static int bttv_g_parm(struct file *file, void *f, struct bttv_fh *fh = f; struct bttv *btv = fh->btv; - mutex_lock(&btv->lock); v4l2_video_std_frame_period(bttv_tvnorms[btv->tvnorm].v4l2_id, &parm->parm.capture.timeperframe); - mutex_unlock(&btv->lock); return 0; } @@ -2994,7 +2942,6 @@ static int bttv_g_tuner(struct file *file, void *priv, if (0 != t->index) return -EINVAL; - mutex_lock(&btv->lock); t->rxsubchans = V4L2_TUNER_SUB_MONO; bttv_call_all(btv, tuner, g_tuner, t); strcpy(t->name, "Television"); @@ -3006,7 +2953,6 @@ static int bttv_g_tuner(struct file *file, void *priv, if (btv->audio_mode_gpio) btv->audio_mode_gpio(btv, t, 0); - mutex_unlock(&btv->lock); return 0; } @@ -3015,9 +2961,7 @@ static int bttv_g_priority(struct file *file, void *f, enum v4l2_priority *p) struct bttv_fh *fh = f; struct bttv *btv = fh->btv; - mutex_lock(&btv->lock); *p = v4l2_prio_max(&btv->prio); - mutex_unlock(&btv->lock); return 0; } @@ -3029,9 +2973,7 @@ static int bttv_s_priority(struct file *file, void *f, struct bttv *btv = fh->btv; int rc; - mutex_lock(&btv->lock); rc = v4l2_prio_change(&btv->prio, &fh->prio, prio); - mutex_unlock(&btv->lock); return rc; } @@ -3046,9 +2988,7 @@ static int bttv_cropcap(struct file *file, void *priv, cap->type != V4L2_BUF_TYPE_VIDEO_OVERLAY) return -EINVAL; - mutex_lock(&btv->lock); *cap = bttv_tvnorms[btv->tvnorm].cropcap; - mutex_unlock(&btv->lock); return 0; } @@ -3066,9 +3006,7 @@ static int bttv_g_crop(struct file *file, void *f, struct v4l2_crop *crop) inconsistent with fh->width or fh->height and apps do not expect a change here. */ - mutex_lock(&btv->lock); crop->c = btv->crop[!!fh->do_crop].rect; - mutex_unlock(&btv->lock); return 0; } @@ -3092,17 +3030,14 @@ static int bttv_s_crop(struct file *file, void *f, struct v4l2_crop *crop) /* Make sure tvnorm, vbi_end and the current cropping parameters remain consistent until we're done. Note read() may change vbi_end in check_alloc_btres_lock(). */ - mutex_lock(&btv->lock); retval = v4l2_prio_check(&btv->prio, fh->prio); if (0 != retval) { - mutex_unlock(&btv->lock); return retval; } retval = -EBUSY; if (locked_btres(fh->btv, VIDEO_RESOURCES)) { - mutex_unlock(&btv->lock); return retval; } @@ -3114,7 +3049,6 @@ static int bttv_s_crop(struct file *file, void *f, struct v4l2_crop *crop) b_top = max(b->top, btv->vbi_end); if (b_top + 32 >= b_bottom) { - mutex_unlock(&btv->lock); return retval; } @@ -3137,12 +3071,8 @@ static int bttv_s_crop(struct file *file, void *f, struct v4l2_crop *crop) btv->crop[1] = c; - mutex_unlock(&btv->lock); - fh->do_crop = 1; - mutex_lock(&fh->cap.vb_lock); - if (fh->width < c.min_scaled_width) { fh->width = c.min_scaled_width; btv->init.width = c.min_scaled_width; @@ -3159,8 +3089,6 @@ static int bttv_s_crop(struct file *file, void *f, struct v4l2_crop *crop) btv->init.height = c.max_scaled_height; } - mutex_unlock(&fh->cap.vb_lock); - return 0; } @@ -3228,7 +3156,6 @@ static unsigned int bttv_poll(struct file *file, poll_table *wait) return videobuf_poll_stream(file, &fh->vbi, wait); } - mutex_lock(&fh->cap.vb_lock); if (check_btres(fh,RESOURCE_VIDEO_STREAM)) { /* streaming capture */ if (list_empty(&fh->cap.stream)) @@ -3263,7 +3190,6 @@ static unsigned int bttv_poll(struct file *file, poll_table *wait) else rc = 0; err: - mutex_unlock(&fh->cap.vb_lock); return rc; } @@ -3303,14 +3229,11 @@ static int bttv_open(struct file *file) * Let's first copy btv->init at fh, holding cap.vb_lock, and then work * with the rest of init, holding btv->lock. */ - mutex_lock(&fh->cap.vb_lock); *fh = btv->init; - mutex_unlock(&fh->cap.vb_lock); fh->type = type; fh->ov.setup_ok = 0; - mutex_lock(&btv->lock); v4l2_prio_open(&btv->prio, &fh->prio); videobuf_queue_sg_init(&fh->cap, &bttv_video_qops, @@ -3318,13 +3241,13 @@ static int bttv_open(struct file *file) V4L2_BUF_TYPE_VIDEO_CAPTURE, V4L2_FIELD_INTERLACED, sizeof(struct bttv_buffer), - fh, NULL); + fh, &btv->lock); videobuf_queue_sg_init(&fh->vbi, &bttv_vbi_qops, &btv->c.pci->dev, &btv->s_lock, V4L2_BUF_TYPE_VBI_CAPTURE, V4L2_FIELD_SEQ_TB, sizeof(struct bttv_buffer), - fh, NULL); + fh, &btv->lock); set_tvnorm(btv,btv->tvnorm); set_input(btv, btv->input, btv->tvnorm); @@ -3347,7 +3270,6 @@ static int bttv_open(struct file *file) bttv_vbi_fmt_reset(&fh->vbi_fmt, btv->tvnorm); bttv_field_count(btv); - mutex_unlock(&btv->lock); return 0; } @@ -3356,7 +3278,6 @@ static int bttv_release(struct file *file) struct bttv_fh *fh = file->private_data; struct bttv *btv = fh->btv; - mutex_lock(&btv->lock); /* turn off overlay */ if (check_btres(fh, RESOURCE_OVERLAY)) bttv_switch_overlay(btv,fh,NULL); @@ -3386,10 +3307,8 @@ static int bttv_release(struct file *file) * videobuf uses cap.vb_lock - we should avoid holding btv->lock, * otherwise we may have dead lock conditions */ - mutex_unlock(&btv->lock); videobuf_mmap_free(&fh->cap); videobuf_mmap_free(&fh->vbi); - mutex_lock(&btv->lock); v4l2_prio_close(&btv->prio, fh->prio); file->private_data = NULL; kfree(fh); @@ -3399,7 +3318,6 @@ static int bttv_release(struct file *file) if (!btv->users) audio_mute(btv, 1); - mutex_unlock(&btv->lock); return 0; } @@ -3503,11 +3421,8 @@ static int radio_open(struct file *file) if (unlikely(!fh)) return -ENOMEM; file->private_data = fh; - mutex_lock(&fh->cap.vb_lock); *fh = btv->init; - mutex_unlock(&fh->cap.vb_lock); - mutex_lock(&btv->lock); v4l2_prio_open(&btv->prio, &fh->prio); btv->radio_user++; @@ -3515,7 +3430,6 @@ static int radio_open(struct file *file) bttv_call_all(btv, tuner, s_radio); audio_input(btv,TVAUDIO_INPUT_RADIO); - mutex_unlock(&btv->lock); return 0; } @@ -3525,7 +3439,6 @@ static int radio_release(struct file *file) struct bttv *btv = fh->btv; struct rds_command cmd; - mutex_lock(&btv->lock); v4l2_prio_close(&btv->prio, fh->prio); file->private_data = NULL; kfree(fh); @@ -3533,7 +3446,6 @@ static int radio_release(struct file *file) btv->radio_user--; bttv_call_all(btv, core, ioctl, RDS_CMD_CLOSE, &cmd); - mutex_unlock(&btv->lock); return 0; } @@ -3562,7 +3474,6 @@ static int radio_g_tuner(struct file *file, void *priv, struct v4l2_tuner *t) return -EINVAL; if (0 != t->index) return -EINVAL; - mutex_lock(&btv->lock); strcpy(t->name, "Radio"); t->type = V4L2_TUNER_RADIO; @@ -3571,8 +3482,6 @@ static int radio_g_tuner(struct file *file, void *priv, struct v4l2_tuner *t) if (btv->audio_mode_gpio) btv->audio_mode_gpio(btv, t, 0); - mutex_unlock(&btv->lock); - return 0; } @@ -3693,7 +3602,7 @@ static const struct v4l2_file_operations radio_fops = .open = radio_open, .read = radio_read, .release = radio_release, - .ioctl = video_ioctl2, + .unlocked_ioctl = video_ioctl2, .poll = radio_poll, }; -- cgit v1.2.3 From 692e42df12e8427219958468301f3d03ca5f0f0d Mon Sep 17 00:00:00 2001 From: Brandon Philips Date: Fri, 17 Dec 2010 12:58:22 -0300 Subject: [media] bttv: remove unneeded locking comments After Mauro's "bttv: Fix locking issues due to BKL removal code" there are a number of comments that are no longer needed about lock ordering. Remove them. Signed-off-by: Brandon Philips Signed-off-by: Mauro Carvalho Chehab --- drivers/media/video/bt8xx/bttv-driver.c | 20 -------------------- 1 file changed, 20 deletions(-) (limited to 'drivers/media/video/bt8xx/bttv-driver.c') diff --git a/drivers/media/video/bt8xx/bttv-driver.c b/drivers/media/video/bt8xx/bttv-driver.c index c14b819d6b78..e071b62f36aa 100644 --- a/drivers/media/video/bt8xx/bttv-driver.c +++ b/drivers/media/video/bt8xx/bttv-driver.c @@ -2359,13 +2359,6 @@ static int setup_window_lock(struct bttv_fh *fh, struct bttv *btv, fh->ov.field = win->field; fh->ov.setup_ok = 1; - /* - * FIXME: btv is protected by btv->lock mutex, while btv->init - * is protected by fh->cap.vb_lock. This seems to open the - * possibility for some race situations. Maybe the better would - * be to unify those locks or to use another way to store the - * init values that will be consumed by videobuf callbacks - */ btv->init.ov.w.width = win->w.width; btv->init.ov.w.height = win->w.height; btv->init.ov.field = win->field; @@ -3220,15 +3213,6 @@ static int bttv_open(struct file *file) return -ENOMEM; file->private_data = fh; - /* - * btv is protected by btv->lock mutex, while btv->init and other - * streaming vars are protected by fh->cap.vb_lock. We need to take - * care of both locks to avoid troubles. However, vb_lock is used also - * inside videobuf, without calling buf->lock. So, it is a very bad - * idea to hold both locks at the same time. - * Let's first copy btv->init at fh, holding cap.vb_lock, and then work - * with the rest of init, holding btv->lock. - */ *fh = btv->init; fh->type = type; @@ -3303,10 +3287,6 @@ static int bttv_release(struct file *file) /* free stuff */ - /* - * videobuf uses cap.vb_lock - we should avoid holding btv->lock, - * otherwise we may have dead lock conditions - */ videobuf_mmap_free(&fh->cap); videobuf_mmap_free(&fh->vbi); v4l2_prio_close(&btv->prio, fh->prio); -- cgit v1.2.3