Re: [PATCH] IPMI driver updates, part 1b

[Corey Minyard]

Andrew Morton wrote:

> Corey Minyard <minyard@xxxxxxx> wrote:
>  
>
> > > diff -puN net/ipmi/af_ipmi.c~af_ipmi-locking-fix net/ipmi/af_ipmi.c
> > >      
> > >--- 25/net/ipmi/af_ipmi.c~af_ipmi-locking-fix	Tue Feb 24 16:56:36 2004
> > >+++ 25-akpm/net/ipmi/af_ipmi.c	Tue Feb 24 16:57:00 2004
> > >@@ -336,6 +336,7 @@ static int ipmi_recvmsg(struct kiocb *io
> > > 		}
> > > 
> > > 		timeo = ipmi_wait_for_queue(i, timeo);
> > >+		spin_lock_irqsave(&i->lock, flags);
> > > 	}
> > > 
> > > 	rcvmsg = list_entry(i->msg_list.next, struct ipmi_recv_msg, link);
> > >
> > >
> > > which may or may not be correct.
> > >
> > Actually, I believe the code is correct, and your change will break it.  
> > This is in a "while (1)" loop, and the only way to get out of this loop 
> > is to return with the lock not held or to break out of the loop with the 
> > lock held (and later code will unlock it).  Am I correct here?
> >    
>
> With a little more context:
>
> +		spin_unlock_irqrestore(&i->lock, flags);
> +		if (!timeo) {
> +			return -EAGAIN;
> +		} else if (signal_pending (current)) {
> +			dbg("Signal pending: %d", 1);
> +			return -EINTR;
> +		}
> +
> +		timeo = ipmi_wait_for_queue(i, timeo);
> +	}
> +
> +	rcvmsg = list_entry(i->msg_list.next, struct ipmi_recv_msg, link);
> +	list_del(&rcvmsg->link);
> +	spin_unlock_irqrestore(&i->lock, flags);
>
> See, there's a direct code path from one spin_unlock() to the other.  And
> ipmi_wait_for_queue() does not retake the lock.
>  
With even more context:

   while (1) {
       spin_lock_irqsave(&i->lock, flags);
       if (!list_empty(&i->msg_list))
           break;
       spin_unlock_irqrestore(&i->lock, flags);
       if (!timeo) {
           return -EAGAIN;
       } else if (signal_pending (current)) {
           dbg("Signal pending: %d", 1);
           return -EINTR;
       }
              
       timeo = ipmi_wait_for_queue(i, timeo);
   }

   rcvmsg = list_entry(i->msg_list.next, struct ipmi_recv_msg, link);
   list_del(&rcvmsg->link);   
   spin_unlock_irqrestore(&i->lock, flags);

So it will always go back to the top of the while loop and claim the 
lock again.  It's kind of wierd looking, but I still believe it is 
correct.  It's ugly, true, I can work on rewriting this piece.

-Corey

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/