| Message ID | 52F4A82C.7010104@xs4all.nl (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 02/07/2014 10:32 AM, Hans Verkuil wrote: > Hi Arnd! > > On 01/17/2014 03:28 PM, Arnd Bergmann wrote: >> On Friday 17 January 2014, Hans Verkuil wrote: >>>> @@ -323,25 +324,32 @@ static ssize_t cadet_read(struct file *file, char __user *data, size_t count, lo >>>> struct cadet *dev = video_drvdata(file); >>>> unsigned char readbuf[RDS_BUFFER]; >>>> int i = 0; >>>> + DEFINE_WAIT(wait); >>>> >>>> mutex_lock(&dev->lock); >>>> if (dev->rdsstat == 0) >>>> cadet_start_rds(dev); >>>> - if (dev->rdsin == dev->rdsout) { >>>> + while (1) { >>>> + prepare_to_wait(&dev->read_queue, &wait, TASK_INTERRUPTIBLE); >>>> + if (dev->rdsin != dev->rdsout) >>>> + break; >>>> + >>>> if (file->f_flags & O_NONBLOCK) { >>>> i = -EWOULDBLOCK; >>>> goto unlock; >>>> } >>>> mutex_unlock(&dev->lock); >>>> - interruptible_sleep_on(&dev->read_queue); >>>> + schedule(); >>>> mutex_lock(&dev->lock); >>>> } >>>> + >>> >>> This seems overly complicated. Isn't it enough to replace interruptible_sleep_on >>> by 'wait_event_interruptible(&dev->read_queue, dev->rdsin != dev->rdsout);'? >>> >>> Or am I missing something subtle? >> >> The existing code sleeps with &dev->lock released because the cadet_handler() >> function needs to grab (and release) the same lock before it can wake up >> the reader thread. >> >> Doing the simple wait_event_interruptible() would result in a deadlock here. > > I don't see it. I propose this patch: > > Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com> > > diff --git a/drivers/media/radio/radio-cadet.c b/drivers/media/radio/radio-cadet.c > index 545c04c..2f658c6 100644 > --- a/drivers/media/radio/radio-cadet.c > +++ b/drivers/media/radio/radio-cadet.c > @@ -327,13 +327,15 @@ static ssize_t cadet_read(struct file *file, char __user *data, size_t count, lo > mutex_lock(&dev->lock); > if (dev->rdsstat == 0) > cadet_start_rds(dev); > - if (dev->rdsin == dev->rdsout) { > + while (dev->rdsin == dev->rdsout) { > if (file->f_flags & O_NONBLOCK) { > i = -EWOULDBLOCK; > goto unlock; > } > mutex_unlock(&dev->lock); > - interruptible_sleep_on(&dev->read_queue); > + if (wait_event_interruptible(&dev->read_queue, Oops, that's without an '&' before dev->read_queue. I forgot to update my patch before posting, sorry about that. Hans > + dev->rdsin != dev->rdsout)) > + return -EINTR; > mutex_lock(&dev->lock); > } > while (i < count && dev->rdsin != dev->rdsout) > > Tested with my radio-cadet card. > > This looks good to me. If I am still missing something, let me know! > > Regards, > > Hans > -- > 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
On Friday 07 February 2014 10:32:28 Hans Verkuil wrote: > mutex_lock(&dev->lock); > if (dev->rdsstat == 0) > cadet_start_rds(dev); > - if (dev->rdsin == dev->rdsout) { > + while (dev->rdsin == dev->rdsout) { > if (file->f_flags & O_NONBLOCK) { > i = -EWOULDBLOCK; > goto unlock; > } > mutex_unlock(&dev->lock); > - interruptible_sleep_on(&dev->read_queue); > + if (wait_event_interruptible(&dev->read_queue, > + dev->rdsin != dev->rdsout)) > + return -EINTR; > mutex_lock(&dev->lock); > } > while (i < count && dev->rdsin != dev->rdsout) > This will normally work, but now the mutex is no longer protecting the shared access to the dev->rdsin and dev->rdsout variables, which was evidently the intention of the author of the original code. AFAICT, the possible result is a similar race as before: if once CPU changes dev->rdsin after the process in cadet_read dropped the lock, the wakeup may get lost. It's quite possible this race never happens in practice, but the code is probably still wrong. If you think we don't actually need the lock to check "dev->rdsin != dev->rdsout", the code can be simplified further, to if ((dev->rdsin == dev->rdsout) && (file->f_flags & O_NONBLOCK)) { return -EWOULDBLOCK; i = wait_event_interruptible(&dev->read_queue, dev->rdsin != dev->rdsout); if (i) return i; Arnd -- 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 --git a/drivers/media/radio/radio-cadet.c b/drivers/media/radio/radio-cadet.c index 545c04c..2f658c6 100644 --- a/drivers/media/radio/radio-cadet.c +++ b/drivers/media/radio/radio-cadet.c @@ -327,13 +327,15 @@ static ssize_t cadet_read(struct file *file, char __user *data, size_t count, lo mutex_lock(&dev->lock); if (dev->rdsstat == 0) cadet_start_rds(dev); - if (dev->rdsin == dev->rdsout) { + while (dev->rdsin == dev->rdsout) { if (file->f_flags & O_NONBLOCK) { i = -EWOULDBLOCK; goto unlock; } mutex_unlock(&dev->lock); - interruptible_sleep_on(&dev->read_queue); + if (wait_event_interruptible(&dev->read_queue, + dev->rdsin != dev->rdsout)) + return -EINTR; mutex_lock(&dev->lock); } while (i < count && dev->rdsin != dev->rdsout)