lkml.org 
[lkml]   [2004]   [Feb]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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/

\
 
 \ /
  Last update: 2005-03-22 14:01    [W:0.090 / U:0.104 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site