Messages in this thread | | | Date | Wed, 25 Feb 2004 15:12:14 -0600 | From | Corey Minyard <> | Subject | Re: [PATCH] IPMI driver updates, part 1b |
| |
Andrew Morton wrote:
>Corey Minyard <minyard@acm.org> 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |