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@xxxxxxxxx> 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) 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@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html