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

[Corey Minyard <minyard@acm.org>]

Andrew Morton wrote:

>Corey Minyard <minyard@acm.org> wrote:
>  
>
>>It helps to actually attach the code.
>>    
>>
>
>Got there eventually.
>
>Patches seem reasonable, thanks.  I'm not sure how to judge the suitability
>of the socket interface but it only touches your code..
>
Let's leave the af_ipmi code out for now.  I'd like to get some opinions 
on it, though.

>
>- Several instances of IPMI_MAX_MSG_LENGTH-sized local arrays.  It's not
>  toooo large, but watch out for the stack space.
>
I will rework these.  These used to be much smaller, but newer hardware 
needed larger sizes.

>
>- You should convert the MODULE_PARMs to module_param() sometime.
>
Will do.

>
>- Be aware that the bitfields in struct seq_table will all fall into the
>  same word and the compiler doesn't access them atomically.  You must
>  provide your own locking to prevent updates to them from scribbling on
>  each other.  Or make them integers.
>
These are only accessed when the sequence number lock is held, so they 
should be ok.

>
>- You misspelt breadcrumbs!
>
Argh!  I'll get that fixed one of these days :)

>
>- This:
>
>	extern struct si_sm_handlers kcs_smi_handlers;
>
>  should be in a header file.
>
ok.

>
>- kcs_event_handler() sets `time = 0;' but never uses it again.
>
Actually, that's not true.  There's an evil goto that goes back to 
"restart:", thus the time needs to be cleared.

>
>- status2txt() should take the caller's char* rather than use a static buffer.
>
Ok, I'll fix that.

>
>- In ipmi_bt_sm.c:
>
>	volatile unsigned char status;
>
>  The volatile is a red flag.   It seems to be unneeded.
>
I'll ask the person who wrote this about it.

>
>- We have #ifdef CONFIG_HIGH_RES_TIMERS code in there?
>
Well, yes.  That's so if people add the high-res timer patch, this 
driver can take advantage of it.  Is that a problem?

>
>- drivers/char/ipmi/ipmi_si.c has lots of
>
>        struct smi_info *smi_info = (struct smi_info *) send_info;
>
>  You don't need to cast a void* and it's actually a harmful thing to do:
>  it suppresses useful warnings if someone goes and changes the type of the
>  rhs.
>
Yes, true.

>
>- ipmi_wait_for_queue() doesn't need set_current_state(TASK_RUNNING);
>  after schedule_timeout() (I removed it)
>
Ok.

>
>- There's a locking bug in ipmi_recvmsg(): it can unlock i_lock when it
>  isn't held.   I added this:
>
>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.
>
I'll look at this.

>
>
>Anyway, that's all fairly trivial.  Please retest this code in the next
>-mm, send me any updates against that as appropriate and we'll get this
>merged up, thanks.
>
Ok.

Thank you for your help.

-Corey