[PATCH]saa7134-video.c: poll method lose race condition

[PATCH]saa7134-video.c: poll method lose race condition

[figo.zhang]

saa7134-video.c: poll method lose race condition


Signed-off-by: Figo.zhang <figo.zhang@kolorific.com>
--- 
drivers/media/video/saa7134/saa7134-video.c |    9 ++++++---
 1 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/media/video/saa7134/saa7134-video.c b/drivers/media/video/saa7134/saa7134-video.c
index 493cad9..95733df 100644
--- a/drivers/media/video/saa7134/saa7134-video.c
+++ b/drivers/media/video/saa7134/saa7134-video.c
@@ -1423,11 +1423,13 @@ video_poll(struct file *file, struct poll_table_struct *wait)
 {
 	struct saa7134_fh *fh = file->private_data;
 	struct videobuf_buffer *buf = NULL;
+	unsigned int rc = 0;
 
 	if (V4L2_BUF_TYPE_VBI_CAPTURE == fh->type)
 		return videobuf_poll_stream(file, &fh->vbi, wait);
 
 	if (res_check(fh,RESOURCE_VIDEO)) {
+		mutex_lock(&fh->cap.vb_lock);
 		if (!list_empty(&fh->cap.stream))
 			buf = list_entry(fh->cap.stream.next, struct videobuf_buffer, stream);
 	} else {
@@ -1446,13 +1448,14 @@ video_poll(struct file *file, struct poll_table_struct *wait)
 	}
 
 	if (!buf)
-		return POLLERR;
+		rc = POLLERR;
 
 	poll_wait(file, &buf->done, wait);
 	if (buf->state == VIDEOBUF_DONE ||
 	    buf->state == VIDEOBUF_ERROR)
-		return POLLIN|POLLRDNORM;
-	return 0;
+		rc = POLLIN|POLLRDNORM;
+	mutex_unlock(&fh->cap.vb_lock);
+	return rc;
 
 err:
 	mutex_unlock(&fh->cap.vb_lock);

Re: [PATCH]saa7134-video.c: poll method lose race condition

[hermann pitton]

Am Montag, den 18.05.2009, 10:13 +0800 schrieb figo.zhang:
> saa7134-video.c: poll method lose race condition
> 
> 
> Signed-off-by: Figo.zhang <figo.zhang@kolorific.com>
> --- 
> drivers/media/video/saa7134/saa7134-video.c |    9 ++++++---
>  1 files changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/video/saa7134/saa7134-video.c b/drivers/media/video/saa7134/saa7134-video.c
> index 493cad9..95733df 100644
> --- a/drivers/media/video/saa7134/saa7134-video.c
> +++ b/drivers/media/video/saa7134/saa7134-video.c
> @@ -1423,11 +1423,13 @@ video_poll(struct file *file, struct poll_table_struct *wait)
>  {
>  	struct saa7134_fh *fh = file->private_data;
>  	struct videobuf_buffer *buf = NULL;
> +	unsigned int rc = 0;
>  
>  	if (V4L2_BUF_TYPE_VBI_CAPTURE == fh->type)
>  		return videobuf_poll_stream(file, &fh->vbi, wait);
>  
>  	if (res_check(fh,RESOURCE_VIDEO)) {
> +		mutex_lock(&fh->cap.vb_lock);
>  		if (!list_empty(&fh->cap.stream))
>  			buf = list_entry(fh->cap.stream.next, struct videobuf_buffer, stream);
>  	} else {
> @@ -1446,13 +1448,14 @@ video_poll(struct file *file, struct poll_table_struct *wait)
>  	}
>  
>  	if (!buf)
> -		return POLLERR;
> +		rc = POLLERR;
>  
>  	poll_wait(file, &buf->done, wait);
>  	if (buf->state == VIDEOBUF_DONE ||
>  	    buf->state == VIDEOBUF_ERROR)
> -		return POLLIN|POLLRDNORM;
> -	return 0;
> +		rc = POLLIN|POLLRDNORM;
> +	mutex_unlock(&fh->cap.vb_lock);
> +	return rc;
>  
>  err:
>  	mutex_unlock(&fh->cap.vb_lock);
> 
> 

Can you please give some description on what your patch might do
something?

Or are you a robot?

Then, please give us your serial number, production year, and when we
can expect you are out of duty and replaced ;)

Cheers,
Hermann

Re: [PATCH]saa7134-video.c: poll method lose race condition

[figo.zhang]

On Mon, 2009-05-18 at 05:07 +0200, hermann pitton wrote:
> Am Montag, den 18.05.2009, 10:13 +0800 schrieb figo.zhang:
> > saa7134-video.c: poll method lose race condition
> > 
> > 
> > Signed-off-by: Figo.zhang <figo.zhang@kolorific.com>
> > --- 
> > drivers/media/video/saa7134/saa7134-video.c |    9 ++++++---
> >  1 files changed, 6 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/media/video/saa7134/saa7134-video.c b/drivers/media/video/saa7134/saa7134-video.c
> > index 493cad9..95733df 100644
> > --- a/drivers/media/video/saa7134/saa7134-video.c
> > +++ b/drivers/media/video/saa7134/saa7134-video.c
> > @@ -1423,11 +1423,13 @@ video_poll(struct file *file, struct poll_table_struct *wait)
> >  {
> >  	struct saa7134_fh *fh = file->private_data;
> >  	struct videobuf_buffer *buf = NULL;
> > +	unsigned int rc = 0;
> >  
> >  	if (V4L2_BUF_TYPE_VBI_CAPTURE == fh->type)
> >  		return videobuf_poll_stream(file, &fh->vbi, wait);
> >  
> >  	if (res_check(fh,RESOURCE_VIDEO)) {
> > +		mutex_lock(&fh->cap.vb_lock);
> >  		if (!list_empty(&fh->cap.stream))
> >  			buf = list_entry(fh->cap.stream.next, struct videobuf_buffer, stream);
> >  	} else {
> > @@ -1446,13 +1448,14 @@ video_poll(struct file *file, struct poll_table_struct *wait)
> >  	}
> >  
> >  	if (!buf)
> > -		return POLLERR;
> > +		rc = POLLERR;
> >  
> >  	poll_wait(file, &buf->done, wait);
> >  	if (buf->state == VIDEOBUF_DONE ||
> >  	    buf->state == VIDEOBUF_ERROR)
> > -		return POLLIN|POLLRDNORM;
> > -	return 0;
> > +		rc = POLLIN|POLLRDNORM;
> > +	mutex_unlock(&fh->cap.vb_lock);
> > +	return rc;
> >  
> >  err:
> >  	mutex_unlock(&fh->cap.vb_lock);
> > 
> > 
> 
> Can you please give some description on what your patch might do
> something?
> 
> Or are you a robot?
> 
> Then, please give us your serial number, production year, and when we
> can expect you are out of duty and replaced ;)
> 
> Cheers,
> Hermann
> 
> 
> 

hi, I just using the saa7134 chip to do a video capture card. The
saa7134 driver in linux kernel, i found that the poll method have lose 
race condition for "RESOURCE_VIDEO". It have better to add a mutex lock.

Re: [PATCH]saa7134-video.c: poll method lose race condition

[hermann pitton]

Am Montag, den 18.05.2009, 11:53 +0800 schrieb figo.zhang:
> On Mon, 2009-05-18 at 05:07 +0200, hermann pitton wrote:
> > Am Montag, den 18.05.2009, 10:13 +0800 schrieb figo.zhang:
> > > saa7134-video.c: poll method lose race condition
> > > 
> > > 
> > > Signed-off-by: Figo.zhang <figo.zhang@kolorific.com>
> > > --- 
> > > drivers/media/video/saa7134/saa7134-video.c |    9 ++++++---
> > >  1 files changed, 6 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/media/video/saa7134/saa7134-video.c b/drivers/media/video/saa7134/saa7134-video.c
> > > index 493cad9..95733df 100644
> > > --- a/drivers/media/video/saa7134/saa7134-video.c
> > > +++ b/drivers/media/video/saa7134/saa7134-video.c
> > > @@ -1423,11 +1423,13 @@ video_poll(struct file *file, struct poll_table_struct *wait)
> > >  {
> > >  	struct saa7134_fh *fh = file->private_data;
> > >  	struct videobuf_buffer *buf = NULL;
> > > +	unsigned int rc = 0;
> > >  
> > >  	if (V4L2_BUF_TYPE_VBI_CAPTURE == fh->type)
> > >  		return videobuf_poll_stream(file, &fh->vbi, wait);
> > >  
> > >  	if (res_check(fh,RESOURCE_VIDEO)) {
> > > +		mutex_lock(&fh->cap.vb_lock);
> > >  		if (!list_empty(&fh->cap.stream))
> > >  			buf = list_entry(fh->cap.stream.next, struct videobuf_buffer, stream);
> > >  	} else {
> > > @@ -1446,13 +1448,14 @@ video_poll(struct file *file, struct poll_table_struct *wait)
> > >  	}
> > >  
> > >  	if (!buf)
> > > -		return POLLERR;
> > > +		rc = POLLERR;
> > >  
> > >  	poll_wait(file, &buf->done, wait);
> > >  	if (buf->state == VIDEOBUF_DONE ||
> > >  	    buf->state == VIDEOBUF_ERROR)
> > > -		return POLLIN|POLLRDNORM;
> > > -	return 0;
> > > +		rc = POLLIN|POLLRDNORM;
> > > +	mutex_unlock(&fh->cap.vb_lock);
> > > +	return rc;
> > >  
> > >  err:
> > >  	mutex_unlock(&fh->cap.vb_lock);
> > > 
> > > 
> > 
> > Can you please give some description on what your patch might do
> > something?
> > 
> > Or are you a robot?
> > 
> > Then, please give us your serial number, production year, and when we
> > can expect you are out of duty and replaced ;)
> > 
> > Cheers,
> > Hermann
> > 
> > 
> > 
> 
> hi, I just using the saa7134 chip to do a video capture card. The
> saa7134 driver in linux kernel, i found that the poll method have lose 
> race condition for "RESOURCE_VIDEO". It have better to add a mutex lock.
> 

OK then, but better stay away from the core files.

On what kernel this is?

Give us some copy/paste "dmesg" output for your card with "i2c_scan=1".
Don't lose tuner stuff on this.

Thanks,
Hermann

Re: [PATCH]saa7134-video.c: poll method lose race condition

[figo.zhang]

On Mon, 2009-05-18 at 05:49 +0200, hermann pitton wrote:
> Am Montag, den 18.05.2009, 11:53 +0800 schrieb figo.zhang:
> > On Mon, 2009-05-18 at 05:07 +0200, hermann pitton wrote:
> > > Am Montag, den 18.05.2009, 10:13 +0800 schrieb figo.zhang:
> > > > saa7134-video.c: poll method lose race condition
> > > > 
> > > > 
> > > > Signed-off-by: Figo.zhang <figo.zhang@kolorific.com>
> > > > --- 
> > > > drivers/media/video/saa7134/saa7134-video.c |    9 ++++++---
> > > >  1 files changed, 6 insertions(+), 3 deletions(-)
> > > > 
> > > > diff --git a/drivers/media/video/saa7134/saa7134-video.c b/drivers/media/video/saa7134/saa7134-video.c
> > > > index 493cad9..95733df 100644
> > > > --- a/drivers/media/video/saa7134/saa7134-video.c
> > > > +++ b/drivers/media/video/saa7134/saa7134-video.c
> > > > @@ -1423,11 +1423,13 @@ video_poll(struct file *file, struct poll_table_struct *wait)
> > > >  {
> > > >  	struct saa7134_fh *fh = file->private_data;
> > > >  	struct videobuf_buffer *buf = NULL;
> > > > +	unsigned int rc = 0;
> > > >  
> > > >  	if (V4L2_BUF_TYPE_VBI_CAPTURE == fh->type)
> > > >  		return videobuf_poll_stream(file, &fh->vbi, wait);
> > > >  
> > > >  	if (res_check(fh,RESOURCE_VIDEO)) {
> > > > +		mutex_lock(&fh->cap.vb_lock);
> > > >  		if (!list_empty(&fh->cap.stream))
> > > >  			buf = list_entry(fh->cap.stream.next, struct videobuf_buffer, stream);
> > > >  	} else {
> > > > @@ -1446,13 +1448,14 @@ video_poll(struct file *file, struct poll_table_struct *wait)
> > > >  	}
> > > >  
> > > >  	if (!buf)
> > > > -		return POLLERR;
> > > > +		rc = POLLERR;
> > > >  
> > > >  	poll_wait(file, &buf->done, wait);
> > > >  	if (buf->state == VIDEOBUF_DONE ||
> > > >  	    buf->state == VIDEOBUF_ERROR)
> > > > -		return POLLIN|POLLRDNORM;
> > > > -	return 0;
> > > > +		rc = POLLIN|POLLRDNORM;
> > > > +	mutex_unlock(&fh->cap.vb_lock);
> > > > +	return rc;
> > > >  
> > > >  err:
> > > >  	mutex_unlock(&fh->cap.vb_lock);
> > > > 
> > > > 
> > > 
> > > Can you please give some description on what your patch might do
> > > something?
> > > 
> > > Or are you a robot?
> > > 
> > > Then, please give us your serial number, production year, and when we
> > > can expect you are out of duty and replaced ;)
> > > 
> > > Cheers,
> > > Hermann
> > > 
> > > 
> > > 
> > 
> > hi, I just using the saa7134 chip to do a video capture card. The
> > saa7134 driver in linux kernel, i found that the poll method have lose 
> > race condition for "RESOURCE_VIDEO". It have better to add a mutex lock.
> > 
> 
> OK then, but better stay away from the core files.
> 
> On what kernel this is?
> 
> Give us some copy/paste "dmesg" output for your card with "i2c_scan=1".
> Don't lose tuner stuff on this.
> 
> Thanks,
> Hermann
> 
> 
> 
hi, hermann pitton ,i am using the git-kernel, the card have some
hardware problems now.

Re: [PATCH]saa7134-video.c: poll method lose race condition

[hermann pitton]

[big snip]
> > 
> > 
> > 
> hi, hermann pitton ,i am using the git-kernel, the card have some
> hardware problems now.

If it is a new card, 2.6.27 is for sure the last stable.

We might get it all down, but 2.6.30 looked promising again.

Seems there is some hungry black hole in between ;)

Cheers,
Hermann

Re: [PATCH]saa7134-video.c: poll method lose race condition

[Guennadi Liakhovetski]

On Mon, 18 May 2009, figo.zhang wrote:

> saa7134-video.c: poll method lose race condition
> 
> 
> Signed-off-by: Figo.zhang <figo.zhang@kolorific.com>

Hi,

I am not sure why you decide to cc me on your sa7a34 patches. Those are 
not the kind of devices I'm working with, so, don't think I'll be able to 
help with your patches.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [PATCH]saa7134-video.c: poll method lose race condition

[figo.zhang]

On Mon, 2009-05-18 at 08:28 +0200, Guennadi Liakhovetski wrote:
> Guennadi

hi,Guennadi,

	I am sorry that it is my mistake.

Figo.zhang

Re: [PATCH]saa7134-video.c: poll method lose race condition

[Oldřich Jedlička]

Hi Figo Zhang,

On Monday 18 of May 2009 at 04:13:13, figo.zhang wrote:
> saa7134-video.c: poll method lose race condition
>
>
> Signed-off-by: Figo.zhang <figo.zhang@kolorific.com>
> ---
> drivers/media/video/saa7134/saa7134-video.c |    9 ++++++---
>  1 files changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/media/video/saa7134/saa7134-video.c
> b/drivers/media/video/saa7134/saa7134-video.c index 493cad9..95733df 100644
> --- a/drivers/media/video/saa7134/saa7134-video.c
> +++ b/drivers/media/video/saa7134/saa7134-video.c
> @@ -1423,11 +1423,13 @@ video_poll(struct file *file, struct
> poll_table_struct *wait) {
>  	struct saa7134_fh *fh = file->private_data;
>  	struct videobuf_buffer *buf = NULL;
> +	unsigned int rc = 0;
>
>  	if (V4L2_BUF_TYPE_VBI_CAPTURE == fh->type)
>  		return videobuf_poll_stream(file, &fh->vbi, wait);
>
>  	if (res_check(fh,RESOURCE_VIDEO)) {
> +		mutex_lock(&fh->cap.vb_lock);
>  		if (!list_empty(&fh->cap.stream))
>  			buf = list_entry(fh->cap.stream.next, struct videobuf_buffer, stream);
>  	} else {
> @@ -1446,13 +1448,14 @@ video_poll(struct file *file, struct
> poll_table_struct *wait) }
>
>  	if (!buf)
> -		return POLLERR;
> +		rc = POLLERR;

Just one note (I don't know the future and meaning of this patch). The line 
above will change the meaning of the buf==NULL check. It will not return 
immediately, but call poll_wait with buf->done instead - NULL pointer access.

Cheers,
Oldrich.

>
>  	poll_wait(file, &buf->done, wait);
>  	if (buf->state == VIDEOBUF_DONE ||
>  	    buf->state == VIDEOBUF_ERROR)
> -		return POLLIN|POLLRDNORM;
> -	return 0;
> +		rc = POLLIN|POLLRDNORM;
> +	mutex_unlock(&fh->cap.vb_lock);
> +	return rc;
>
>  err:
>  	mutex_unlock(&fh->cap.vb_lock);
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html