diff mbox

saa7134-video.c: poll method lose race condition

Message ID 1242612794.3442.19.camel@myhost (mailing list archive)
State Accepted
Headers show

Commit Message

figo.zhang May 18, 2009, 2:13 a.m. UTC 
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(-)



--
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

Comments

hermann pitton May 18, 2009, 3:07 a.m. UTC | #1 
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




--
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
hermann pitton May 18, 2009, 3:49 a.m. UTC | #2 
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




--
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
figo.zhang May 18, 2009, 3:53 a.m. UTC | #3 
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.

--
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
figo.zhang May 18, 2009, 4:07 a.m. UTC | #4 
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.

--
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
hermann pitton May 18, 2009, 4:08 a.m. UTC | #5 
[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


--
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
Guennadi Liakhovetski May 18, 2009, 6:28 a.m. UTC | #6 
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/
--
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
figo.zhang May 18, 2009, 6:43 a.m. UTC | #7 
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

--
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
Oldřich Jedlička May 18, 2009, 7:58 p.m. UTC | #8 
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
--
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
diff mbox

Patch

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);