[PATCH 1/2] em28xx: Get rid of some unused modprobe parameters at vbi code

[PATCH 1/2] em28xx: Get rid of some unused modprobe parameters at vbi code

[Mauro Carvalho Chehab]

There are two modprobe parameters for VBI that aren't used
anywhere (one for debug, the other one related to the buffer
size). Get rid of them!

Signed-off-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com>

diff --git a/drivers/media/usb/em28xx/em28xx-vbi.c b/drivers/media/usb/em28xx/em28xx-vbi.c
index 6d7f657f6f55..34ee1e03a732 100644
--- a/drivers/media/usb/em28xx/em28xx-vbi.c
+++ b/drivers/media/usb/em28xx/em28xx-vbi.c
@@ -29,17 +29,6 @@
 #include "em28xx.h"
 #include "em28xx-v4l.h"
 
-static unsigned int vbibufs = 5;
-module_param(vbibufs, int, 0644);
-MODULE_PARM_DESC(vbibufs, "number of vbi buffers, range 2-32");
-
-static unsigned int vbi_debug;
-module_param(vbi_debug, int, 0644);
-MODULE_PARM_DESC(vbi_debug, "enable debug messages [vbi]");
-
-#define dprintk(level, fmt, arg...)	if (vbi_debug >= level) \
-	printk(KERN_DEBUG "%s: " fmt, dev->core->name , ## arg)
-
 /* ------------------------------------------------------------------ */
 
 static int vbi_queue_setup(struct vb2_queue *vq, const struct v4l2_format *fmt,
-- 
1.9.3

[PATCH 2/2] em28xx: fix VBI handling logic

[Mauro Carvalho Chehab]

When both VBI and video are streaming, and video stream is stopped,
a subsequent trial to restart it will fail, because S_FMT will
return -EBUSY.

That prevents applications like zvbi to work properly.

Please notice that, while this fix it fully for zvbi, the
best is to get rid of streaming_users and res_get logic as a hole.

However, this single-line patch is better to be merged at -stable.

Signed-off-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com>

diff --git a/drivers/media/usb/em28xx/em28xx-video.c b/drivers/media/usb/em28xx/em28xx-video.c
index 08569cbccd95..d75e7f82dfb9 100644
--- a/drivers/media/usb/em28xx/em28xx-video.c
+++ b/drivers/media/usb/em28xx/em28xx-video.c
@@ -1351,7 +1351,7 @@ static int vidioc_s_fmt_vid_cap(struct file *file, void *priv,
 	struct em28xx *dev = video_drvdata(file);
 	struct em28xx_v4l2 *v4l2 = dev->v4l2;
 
-	if (v4l2->streaming_users > 0)
+	if (vb2_is_busy(&v4l2->vb_vidq))
 		return -EBUSY;
 
 	vidioc_try_fmt_vid_cap(file, priv, f);
-- 
1.9.3

Re: [PATCH 2/2] em28xx: fix VBI handling logic

[Frank Schäfer]

Am 19.09.2014 um 18:02 schrieb Mauro Carvalho Chehab:
> When both VBI and video are streaming, and video stream is stopped,
> a subsequent trial to restart it will fail, because S_FMT will
> return -EBUSY.
>
> That prevents applications like zvbi to work properly.
>
> Please notice that, while this fix it fully for zvbi, the
> best is to get rid of streaming_users and res_get logic as a hole.
>
> However, this single-line patch is better to be merged at -stable.
>
> Signed-off-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com>
>
> diff --git a/drivers/media/usb/em28xx/em28xx-video.c b/drivers/media/usb/em28xx/em28xx-video.c
> index 08569cbccd95..d75e7f82dfb9 100644
> --- a/drivers/media/usb/em28xx/em28xx-video.c
> +++ b/drivers/media/usb/em28xx/em28xx-video.c
> @@ -1351,7 +1351,7 @@ static int vidioc_s_fmt_vid_cap(struct file *file, void *priv,
>  	struct em28xx *dev = video_drvdata(file);
>  	struct em28xx_v4l2 *v4l2 = dev->v4l2;
>  
> -	if (v4l2->streaming_users > 0)
> +	if (vb2_is_busy(&v4l2->vb_vidq))
Looks dangerous.
Are you 100% sure that VIDIOC_S_FMT can have no effect on VBI capturing ?
It seems to trigger writes to multiple registers...

Regards,
Frank

>  		return -EBUSY;
>  
>  	vidioc_try_fmt_vid_cap(file, priv, f);

Re: [PATCH 2/2] em28xx: fix VBI handling logic

[Mauro Carvalho Chehab]

Em Tue, 23 Sep 2014 21:32:02 +0200
Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:

> 
> Am 19.09.2014 um 18:02 schrieb Mauro Carvalho Chehab:
> > When both VBI and video are streaming, and video stream is stopped,
> > a subsequent trial to restart it will fail, because S_FMT will
> > return -EBUSY.
> >
> > That prevents applications like zvbi to work properly.
> >
> > Please notice that, while this fix it fully for zvbi, the
> > best is to get rid of streaming_users and res_get logic as a hole.
> >
> > However, this single-line patch is better to be merged at -stable.
> >
> > Signed-off-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com>
> >
> > diff --git a/drivers/media/usb/em28xx/em28xx-video.c b/drivers/media/usb/em28xx/em28xx-video.c
> > index 08569cbccd95..d75e7f82dfb9 100644
> > --- a/drivers/media/usb/em28xx/em28xx-video.c
> > +++ b/drivers/media/usb/em28xx/em28xx-video.c
> > @@ -1351,7 +1351,7 @@ static int vidioc_s_fmt_vid_cap(struct file *file, void *priv,
> >  	struct em28xx *dev = video_drvdata(file);
> >  	struct em28xx_v4l2 *v4l2 = dev->v4l2;
> >  
> > -	if (v4l2->streaming_users > 0)
> > +	if (vb2_is_busy(&v4l2->vb_vidq))
> Looks dangerous.

Why Dangerous? 

Did you identify any problem? With what application?

Regards,
Mauro

Re: [PATCH 2/2] em28xx: fix VBI handling logic

[Frank Schäfer]

Am 24.09.2014 um 01:18 schrieb Mauro Carvalho Chehab:
> Em Tue, 23 Sep 2014 21:32:02 +0200
> Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:
>
>> Am 19.09.2014 um 18:02 schrieb Mauro Carvalho Chehab:
>>> When both VBI and video are streaming, and video stream is stopped,
>>> a subsequent trial to restart it will fail, because S_FMT will
>>> return -EBUSY.
>>>
>>> That prevents applications like zvbi to work properly.
>>>
>>> Please notice that, while this fix it fully for zvbi, the
>>> best is to get rid of streaming_users and res_get logic as a hole.
>>>
>>> However, this single-line patch is better to be merged at -stable.
>>>
>>> Signed-off-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com>
>>>
>>> diff --git a/drivers/media/usb/em28xx/em28xx-video.c b/drivers/media/usb/em28xx/em28xx-video.c
>>> index 08569cbccd95..d75e7f82dfb9 100644
>>> --- a/drivers/media/usb/em28xx/em28xx-video.c
>>> +++ b/drivers/media/usb/em28xx/em28xx-video.c
>>> @@ -1351,7 +1351,7 @@ static int vidioc_s_fmt_vid_cap(struct file *file, void *priv,
>>>  	struct em28xx *dev = video_drvdata(file);
>>>  	struct em28xx_v4l2 *v4l2 = dev->v4l2;
>>>  
>>> -	if (v4l2->streaming_users > 0)
>>> +	if (vb2_is_busy(&v4l2->vb_vidq))
>> Looks dangerous.
> Why Dangerous? 
You are an experienced kernel developer. If you still fail to see that
after so many years, sorry, I can't help you.

> Did you identify any problem?
Yes I've identified a potential problem.
Read it again, it's in the line you skipped in this reply.

> With what application?
I would be willing to spend more time on this and test this critical
patch (like I already did in the past).
But I don't have access to analog TV here at the moment, sorry. It would
have to wait ~2 weeks.

My fault. I shouldn't have wasted any time on reviewing your patch.

Regards,
Frank

Re: [PATCH 2/2] em28xx: fix VBI handling logic

[Mauro Carvalho Chehab]

Em Thu, 25 Sep 2014 15:59:12 +0200
Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:

> 
> Am 24.09.2014 um 01:18 schrieb Mauro Carvalho Chehab:
> > Em Tue, 23 Sep 2014 21:32:02 +0200
> > Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:
> >
> >> Am 19.09.2014 um 18:02 schrieb Mauro Carvalho Chehab:
> >>> When both VBI and video are streaming, and video stream is stopped,
> >>> a subsequent trial to restart it will fail, because S_FMT will
> >>> return -EBUSY.
> >>>
> >>> That prevents applications like zvbi to work properly.
> >>>
> >>> Please notice that, while this fix it fully for zvbi, the
> >>> best is to get rid of streaming_users and res_get logic as a hole.
> >>>
> >>> However, this single-line patch is better to be merged at -stable.
> >>>
> >>> Signed-off-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com>
> >>>
> >>> diff --git a/drivers/media/usb/em28xx/em28xx-video.c b/drivers/media/usb/em28xx/em28xx-video.c
> >>> index 08569cbccd95..d75e7f82dfb9 100644
> >>> --- a/drivers/media/usb/em28xx/em28xx-video.c
> >>> +++ b/drivers/media/usb/em28xx/em28xx-video.c
> >>> @@ -1351,7 +1351,7 @@ static int vidioc_s_fmt_vid_cap(struct file *file, void *priv,
> >>>  	struct em28xx *dev = video_drvdata(file);
> >>>  	struct em28xx_v4l2 *v4l2 = dev->v4l2;
> >>>  
> >>> -	if (v4l2->streaming_users > 0)
> >>> +	if (vb2_is_busy(&v4l2->vb_vidq))
> >> Looks dangerous.
> > Why Dangerous? 
> You are an experienced kernel developer. If you still fail to see that
> after so many years, sorry, I can't help you.
> 
> > Did you identify any problem?
> Yes I've identified a potential problem.
> Read it again, it's in the line you skipped in this reply.

Did you read the video_ioctl_ops struct?

See:

	.vidioc_g_fmt_vid_cap       = vidioc_g_fmt_vid_cap,
	.vidioc_try_fmt_vid_cap     = vidioc_try_fmt_vid_cap,
	.vidioc_s_fmt_vid_cap       = vidioc_s_fmt_vid_cap,
	.vidioc_g_fmt_vbi_cap       = vidioc_g_fmt_vbi_cap,
	.vidioc_try_fmt_vbi_cap     = vidioc_g_fmt_vbi_cap,
	.vidioc_s_fmt_vbi_cap       = vidioc_g_fmt_vbi_cap,
	.vidioc_enum_framesizes     = vidioc_enum_framesizes,
	.vidioc_g_audio             = vidioc_g_audio,
	.vidioc_s_audio             = vidioc_s_audio,

Only the video node calls s_fmt_vid_cap. There's nothing to be
set for VBI. In other words, a call to VIDIOC_S_FMT will
actually call vidioc_g_fmt_vbi_cap() if called from VBI.

So, I was unable to see any potencial issues.

> > With what application?
> I would be willing to spend more time on this and test this critical
> patch (like I already did in the past).
> But I don't have access to analog TV here at the moment, sorry. It would
> have to wait ~2 weeks.

On my tests with the 3 existing VBI apps, they all worked fine
after the patch, no matter if the VBI or the video application
is started first.

That's why I asked.

Regards,
Mauro

Re: [PATCH 2/2] em28xx: fix VBI handling logic

[Frank Schäfer]

(Resending because my crappy modem crashed while sending)

Am 25.09.2014 um 16:07 schrieb Mauro Carvalho Chehab:
> Em Thu, 25 Sep 2014 15:59:12 +0200
> Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:
>
>> Am 24.09.2014 um 01:18 schrieb Mauro Carvalho Chehab:
>>> Em Tue, 23 Sep 2014 21:32:02 +0200
>>> Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:
>>>
>>>> Am 19.09.2014 um 18:02 schrieb Mauro Carvalho Chehab:
>>>>> When both VBI and video are streaming, and video stream is stopped,
>>>>> a subsequent trial to restart it will fail, because S_FMT will
>>>>> return -EBUSY.
>>>>>
>>>>> That prevents applications like zvbi to work properly.
>>>>>
>>>>> Please notice that, while this fix it fully for zvbi, the
>>>>> best is to get rid of streaming_users and res_get logic as a hole.
>>>>>
>>>>> However, this single-line patch is better to be merged at -stable.
>>>>>
>>>>> Signed-off-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com>
>>>>>
>>>>> diff --git a/drivers/media/usb/em28xx/em28xx-video.c b/drivers/media/usb/em28xx/em28xx-video.c
>>>>> index 08569cbccd95..d75e7f82dfb9 100644
>>>>> --- a/drivers/media/usb/em28xx/em28xx-video.c
>>>>> +++ b/drivers/media/usb/em28xx/em28xx-video.c
>>>>> @@ -1351,7 +1351,7 @@ static int vidioc_s_fmt_vid_cap(struct file *file, void *priv,
>>>>>  	struct em28xx *dev = video_drvdata(file);
>>>>>  	struct em28xx_v4l2 *v4l2 = dev->v4l2;
>>>>>  
>>>>> -	if (v4l2->streaming_users > 0)
>>>>> +	if (vb2_is_busy(&v4l2->vb_vidq))
>>>> Looks dangerous.
>>> Why Dangerous? 
>> You are an experienced kernel developer. If you still fail to see that
>> after so many years, sorry, I can't help you.
>>
>>> Did you identify any problem?
>> Yes I've identified a potential problem.
>> Read it again, it's in the line you skipped in this reply.
> Did you read the video_ioctl_ops struct?
>
> See:
>
> 	.vidioc_g_fmt_vid_cap       = vidioc_g_fmt_vid_cap,
> 	.vidioc_try_fmt_vid_cap     = vidioc_try_fmt_vid_cap,
> 	.vidioc_s_fmt_vid_cap       = vidioc_s_fmt_vid_cap,
> 	.vidioc_g_fmt_vbi_cap       = vidioc_g_fmt_vbi_cap,
> 	.vidioc_try_fmt_vbi_cap     = vidioc_g_fmt_vbi_cap,
> 	.vidioc_s_fmt_vbi_cap       = vidioc_g_fmt_vbi_cap,
> 	.vidioc_enum_framesizes     = vidioc_enum_framesizes,
> 	.vidioc_g_audio             = vidioc_g_audio,
> 	.vidioc_s_audio             = vidioc_s_audio,
>
> Only the video node calls s_fmt_vid_cap. There's nothing to be
> set for VBI. In other words, a call to VIDIOC_S_FMT will
> actually call vidioc_g_fmt_vbi_cap() if called from VBI.
>
> So, I was unable to see any potencial issues.
Yes, I checked that, too. No problems here.

I was concerned about the||consequences of the
em28xx_set_video_format||() call.
It calls em28xx_resolution_set() which in turn calls

em28xx_set_outfmt()
em28xx_accumulator_set()
em28xx_capture_area_set()||
em28xx_scaler_set()

So lot's of device registers are reconfigured.
That might cause problems even if the register values don't change.

If that has been verified and it causes no problems, than of course
everything is fine.

Regards,
Frank

>
>>> With what application?
>> I would be willing to spend more time on this and test this critical
>> patch (like I already did in the past).
>> But I don't have access to analog TV here at the moment, sorry. It would
>> have to wait ~2 weeks.
> On my tests with the 3 existing VBI apps, they all worked fine
> after the patch, no matter if the VBI or the video application
> is started first.
>
> That's why I asked.
>
> Regards,
> Mauro