Re: [PATCH]saa7134-video.c: poll method lose race condition
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 > --- > 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 majord...@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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH]saa7134-video.c: poll method lose race condition
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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH]saa7134-video.c: poll method lose race condition
On Mon, 18 May 2009, figo.zhang wrote: > saa7134-video.c: poll method lose race condition > > > Signed-off-by: Figo.zhang 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH]saa7134-video.c: poll method lose race condition
[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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH]saa7134-video.c: poll method lose race condition
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 > > > > --- > > > > 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH]saa7134-video.c: poll method lose race condition
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 > > > --- > > > 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH]saa7134-video.c: poll method lose race condition
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 > > --- > > 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH]saa7134-video.c: poll method lose race condition
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 > --- > 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH]saa7134-video.c: poll method lose race condition
saa7134-video.c: poll method lose race condition Signed-off-by: Figo.zhang --- 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); -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html