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

[Corey Minyard <minyard@acm.org>]

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