Em Fri, 29 Jan 2016 11:05:04 -0800
Jean-Baptiste Theou <jtheou@xxxxxxxxxxxxxxxxxx> escreveu:
> For tuners with no_alt_vanc=0, and VBI and analog video device
> open.
> There is two ways to close the devices:
>
> *First way (start with user=2)
>
> VBI first (user=1): URBs for the VBI are killed properly
> with cx231xx_uninit_vbi_isoc
>
> Analog second (user=0): URBs for the Analog are killed
> properly with cx231xx_uninit_isoc
>
> *Second way (start with user=2)
>
> Analog first (user=1): URBs for the Analog are NOT killed
> properly with cx231xx_uninit_isoc, because the exit path
> is not called this time.
>
> VBI first (user=0): URBs for the VBI are killed properly with
> cx231xx_uninit_vbi_isoc, but we are exiting the function
> without killing the URBs for the Analog
>
> This situation lead to various kernel panics, since
> the URBs are still processed, without the device been
> open.
>
> The patch fix the issue by calling the exit path no matter
> what, when user=0, plus remove a duplicate trace.
>
> Signed-off-by: Jean-Baptiste Theou <jtheou@xxxxxxxxxxxxxxxxxx>
>
> ---
>
> - v2: Avoid duplicate code and ensure that the queue are freed
> properly.
> ---
> drivers/media/usb/cx231xx/cx231xx-video.c | 44 +++++++++----------------------
> 1 file changed, 12 insertions(+), 32 deletions(-)
>
> diff --git a/drivers/media/usb/cx231xx/cx231xx-video.c b/drivers/media/usb/cx231xx/cx231xx-video.c
> index 9b88cd8..a832c83 100644
> --- a/drivers/media/usb/cx231xx/cx231xx-video.c
> +++ b/drivers/media/usb/cx231xx/cx231xx-video.c
> @@ -1836,10 +1836,21 @@ static int cx231xx_close(struct file *filp)
>
> cx231xx_videodbg("users=%d\n", dev->users);
>
> - cx231xx_videodbg("users=%d\n", dev->users);
> if (res_check(fh))
> res_free(fh);
>
> + videobuf_stop(&fh->vb_vidq);
> + videobuf_mmap_free(&fh->vb_vidq);
> +
> + /* the device is already disconnect,
> + * free the remaining resources
> + */
> + if (dev->state & DEV_DISCONNECTED) {
> + cx231xx_release_resources(dev);
> + fh->dev = NULL;
> + return 0;
> + }
> +
> /*
> * To workaround error number=-71 on EP0 for VideoGrabber,
> * need exclude following.
Hmm... The above doesn't sound right to stop the queue unconditionally when
users != 0, as one could do weird things like:
start video
start vbi
stop vbi
start vbi
stop video
stop vbi
Those weird workflows happen when someone is using a TV application
like TVtime for capturing images, while using a different app, like
zvbi to get VBI data.
So, I guess that the above hunked should be removed...
> @@ -1848,19 +1859,6 @@ static int cx231xx_close(struct file *filp)
> */
> if (!dev->board.no_alt_vanc)
> if (fh->type == V4L2_BUF_TYPE_VBI_CAPTURE) {
> - videobuf_stop(&fh->vb_vidq);
> - videobuf_mmap_free(&fh->vb_vidq);
> -
> - /* the device is already disconnect,
> - free the remaining resources */
> - if (dev->state & DEV_DISCONNECTED) {
> - if (atomic_read(&dev->devlist_count) > 0) {
> - cx231xx_release_resources(dev);
> - fh->dev = NULL;
> - return 0;
> - }
> - return 0;
> - }
>
> /* do this before setting alternate! */
> cx231xx_uninit_vbi_isoc(dev);
> @@ -1870,29 +1868,11 @@ static int cx231xx_close(struct file *filp)
> cx231xx_set_alt_setting(dev, INDEX_VANC, 0);
> else
> cx231xx_set_alt_setting(dev, INDEX_HANC, 0);
> -
> - v4l2_fh_del(&fh->fh);
> - v4l2_fh_exit(&fh->fh);
> - kfree(fh);
> - dev->users--;
> - wake_up_interruptible(&dev->open);
> - return 0;
> }
>
The above changes are OK...
> v4l2_fh_del(&fh->fh);
> dev->users--;
> if (!dev->users) {
> - videobuf_stop(&fh->vb_vidq);
> - videobuf_mmap_free(&fh->vb_vidq);
> -
> - /* the device is already disconnect,
> - free the remaining resources */
> - if (dev->state & DEV_DISCONNECTED) {
> - cx231xx_release_resources(dev);
> - fh->dev = NULL;
> - return 0;
> - }
> -
But the above code should be kept, as we should only stop/free
resources when neither VBI or Video is running. Other drivers do
similar things and work properly. See em28xx for example (I'm sure
em28xx video/vbi is working as expected, as I did such tests last
week).
> /* Save some power by putting tuner to sleep */
> call_all(dev, core, s_power, 0);
>
Regards,
Mauro
--
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
