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