* [PATCH] [media] cx231xx: fix close sequence for VBI + analog
@ 2016-01-29 18:36 Jean-Baptiste Theou
2016-01-29 19:05 ` [PATCH v2] " Jean-Baptiste Theou
0 siblings, 1 reply; 5+ messages in thread
From: Jean-Baptiste Theou @ 2016-01-29 18:36 UTC (permalink / raw)
To: linux-media; +Cc: Jean-Baptiste Theou
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@adeneo-embedded.us>
---
drivers/media/usb/cx231xx/cx231xx-video.c | 8 --------
1 file changed, 8 deletions(-)
diff --git a/drivers/media/usb/cx231xx/cx231xx-video.c b/drivers/media/usb/cx231xx/cx231xx-video.c
index 9b88cd8..559ca77 100644
--- a/drivers/media/usb/cx231xx/cx231xx-video.c
+++ b/drivers/media/usb/cx231xx/cx231xx-video.c
@@ -1836,7 +1836,6 @@ 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);
@@ -1870,13 +1869,6 @@ 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;
}
v4l2_fh_del(&fh->fh);
--
2.7.0
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH v2] [media] cx231xx: fix close sequence for VBI + analog
2016-01-29 18:36 [PATCH] [media] cx231xx: fix close sequence for VBI + analog Jean-Baptiste Theou
@ 2016-01-29 19:05 ` Jean-Baptiste Theou
2016-02-01 15:33 ` Mauro Carvalho Chehab
0 siblings, 1 reply; 5+ messages in thread
From: Jean-Baptiste Theou @ 2016-01-29 19:05 UTC (permalink / raw)
To: linux-media; +Cc: Jean-Baptiste Theou
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@adeneo-embedded.us>
---
- 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.
@@ -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;
}
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;
- }
-
/* Save some power by putting tuner to sleep */
call_all(dev, core, s_power, 0);
--
2.7.0
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v2] [media] cx231xx: fix close sequence for VBI + analog
2016-01-29 19:05 ` [PATCH v2] " Jean-Baptiste Theou
@ 2016-02-01 15:33 ` Mauro Carvalho Chehab
2016-02-01 17:47 ` Jean-Baptiste Theou
0 siblings, 1 reply; 5+ messages in thread
From: Mauro Carvalho Chehab @ 2016-02-01 15:33 UTC (permalink / raw)
To: Jean-Baptiste Theou; +Cc: linux-media
Em Fri, 29 Jan 2016 11:05:04 -0800
Jean-Baptiste Theou <jtheou@adeneo-embedded.us> 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@adeneo-embedded.us>
>
> ---
>
> - 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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] [media] cx231xx: fix close sequence for VBI + analog
2016-02-01 15:33 ` Mauro Carvalho Chehab
@ 2016-02-01 17:47 ` Jean-Baptiste Theou
2016-03-24 4:51 ` Luke Suchocki
0 siblings, 1 reply; 5+ messages in thread
From: Jean-Baptiste Theou @ 2016-02-01 17:47 UTC (permalink / raw)
To: Mauro Carvalho Chehab; +Cc: linux-media
Hi Mauro,
Thanks for your feedback.
On Mon, Feb 1, 2016 at 7:33 AM, Mauro Carvalho Chehab
<mchehab@osg.samsung.com> wrote:
> Em Fri, 29 Jan 2016 11:05:04 -0800
> Jean-Baptiste Theou <jtheou@adeneo-embedded.us> 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@adeneo-embedded.us>
>>
>> ---
>>
>> - 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).
My understanding of this code is that the VBI and the VIDEO device have
they own vb_vidq,
so if videobuf_stop and video_mmap_free are called only when user=0,
one device
(the first one to be close) will not be freed properly.
This is why I am stopping the use of the buffers and release the memory
first for every call
of close().
Am I missing the way this code works?
>
>
>> /* Save some power by putting tuner to sleep */
>> call_all(dev, core, s_power, 0);
>>
>
> Regards,
> Mauro
Best regards,
Jean-Baptiste
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] [media] cx231xx: fix close sequence for VBI + analog
2016-02-01 17:47 ` Jean-Baptiste Theou
@ 2016-03-24 4:51 ` Luke Suchocki
0 siblings, 0 replies; 5+ messages in thread
From: Luke Suchocki @ 2016-03-24 4:51 UTC (permalink / raw)
To: linux-media
Jean-Baptiste Theou <jtheou <at> adeneo-embedded.us> writes:
>
> Hi Mauro,
>
> Thanks for your feedback.
>
> On Mon, Feb 1, 2016 at 7:33 AM, Mauro Carvalho Chehab
> <mchehab <at> osg.samsung.com> wrote:
> > Em Fri, 29 Jan 2016 11:05:04 -0800
> > Jean-Baptiste Theou <jtheou <at> adeneo-embedded.us> 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 <at> adeneo-embedded.us>
> >>
> >> ---
> >>
> >> - v2: Avoid duplicate code and ensure that the queue are freed
> >> properly.
> >> ---
....
> >
> > 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).
>
> My understanding of this code is that the VBI and the VIDEO device have
> they own vb_vidq,
> so if videobuf_stop and video_mmap_free are called only when user=0,
> one device
> (the first one to be close) will not be freed properly.
>
> This is why I am stopping the use of the buffers and release the memory
> first for every call
> of close().
>
> Am I missing the way this code works?
>
> >
> > Regards,
> > Mauro
>
> Best regards,
>
> Jean-Baptiste
Not to hijack, but I've just worked through this same issue.
For my implementation, i could see that the video endpoint was not properly
shutdown by enabling 'isoc_debug' since get_next_buf() would print the "No
active queue" messages after the video file handle was closed and not torn
down correctly, and while VBI was still open.. I also saw the panics when
trying to rmmod.
Since the VBI and video are different endpoints and different buffers when
(!dev->board.no_alt_vanc) is true, they must be cx231xx_uninit_*
independently.
When the video fh is closed first and VBI still open, dev->users is > 0 and
video is never uninit_. dev->users decrements, VBI fh gets closed, users ==
0 and VBI is torn down, but the video endpoint is still running.
My solution was to track fh opens of type V4L2_BUF_TYPE_VBI_CAPTURE
independently with a new dev->vbi_users and run the
cx231xx_uninit_vbi_isoc() block on close when that is 0, and run the other
cx231xx_uninit_isoc() block when dev->users == 0.
Even if dev->users == 0 while VBI is open, video can still shutdown
beforehand.
Also, I think the V2 proposed patch might be ok, as long as there's never 2
VBI users (or 1 user and it's just VBI) and thus when dev->users == 0 it's
always ok to run the video cx231xx_uninit_* code / set_alt_setting(dev,
INDEX_VIDEO,0).
I would be happy to share my patch w/ RFC if you're interested, but now I
think there's more to this than what all of us have fully considered.
--Luke Suchocki
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2016-03-24 5:00 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-01-29 18:36 [PATCH] [media] cx231xx: fix close sequence for VBI + analog Jean-Baptiste Theou
2016-01-29 19:05 ` [PATCH v2] " Jean-Baptiste Theou
2016-02-01 15:33 ` Mauro Carvalho Chehab
2016-02-01 17:47 ` Jean-Baptiste Theou
2016-03-24 4:51 ` Luke Suchocki
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).