Messages in this thread | | | Date | Wed, 25 Feb 2004 09:59:35 -0600 | From | Corey Minyard <> | Subject | Re: [PATCH] IPMI driver updates, part 1b |
| |
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
- 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/
| |