Re: schedule inside spin_lock_irqsave?

[Jiri Slaby <jirislaby@gmail.com>]

On 05/30/2010 04:52 PM, Richard Zidlicky wrote:
> Hi,
> 
> came across following snippet of code (2.6.34:drivers/media/dvb/siano/smscoreapi.c) and 
> since prepare_to_wait is new for me I am wondering if this is can work?
> 
> struct smscore_buffer_t *smscore_getbuffer(struct smscore_device_t *coredev)
> {
> 	struct smscore_buffer_t *cb = NULL;
> 	unsigned long flags;
> 
> 	DEFINE_WAIT(wait);
> 
> 	spin_lock_irqsave(&coredev->bufferslock, flags);
> 
> 	/* This function must return a valid buffer, since the buffer list is
> 	 * finite, we check that there is an available buffer, if not, we wait
> 	 * until such buffer become available.
> 	 */
> 
> 	prepare_to_wait(&coredev->buffer_mng_waitq, &wait, TASK_INTERRUPTIBLE);
> 
> 	if (list_empty(&coredev->buffers))
> 		schedule();
> 
> 	finish_wait(&coredev->buffer_mng_waitq, &wait);
> 
> 	cb = (struct smscore_buffer_t *) coredev->buffers.next;
> 	list_del(&cb->entry);
> 
> 	spin_unlock_irqrestore(&coredev->bufferslock, flags);


Yep, that's a double bug.
1) If the waiting is interrupted, it will die because the list is still
empty.
2) If there is no entry in the list, it will deadlock at least on UP.
This should be
wait_event(&coredev->buffer_mng_waitq, cb = get_entry());
with get_entry like:
struct smscore_buffer_t *get_entry(void)
{
  struct smscore_buffer_t *cb = NULL;
  spin_lock_irqsave(&coredev->bufferslock, flags);
  if (!list_empty(&coredev->buffers)) {
    cb = (struct smscore_buffer_t *) coredev->buffers.next;
    list_del(&cb->entry);
  }
  spin_unlock_irqrestore(&coredev->bufferslock, flags);
  return cb;
}

regards,
-- 
js