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

[Corey Minyard]

Andrew Morton wrote:

> Corey Minyard <minyard@xxxxxxx> 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

-
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/