Re: [PATCH] IPMI driver updates, part 1b
From: Andrew Morton (akpm_at_osdl.org)
Date: 02/25/04
- Previous message: Andrew Morton: "Re: 2.6.3-mm3 hangs on boot x440 (scsi?)"
- In reply to: Corey Minyard: "Re: [PATCH] IPMI driver updates, part 1b"
- Next in thread: Corey Minyard: "Re: [PATCH] IPMI driver updates, part 1b"
- Reply: Corey Minyard: "Re: [PATCH] IPMI driver updates, part 1b"
- Reply: Corey Minyard: "Re: [PATCH] IPMI driver updates, part 1b"
- Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]
Date: Tue, 24 Feb 2004 17:00:24 -0800 To: Corey Minyard <minyard@acm.org>
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..
- Several instances of IPMI_MAX_MSG_LENGTH-sized local arrays. It's not
toooo large, but watch out for the stack space.
- You should convert the MODULE_PARMs to module_param() sometime.
- 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.
- You misspelt breadcrumbs!
- This:
extern struct si_sm_handlers kcs_smi_handlers;
should be in a header file.
- kcs_event_handler() sets `time = 0;' but never uses it again.
- status2txt() should take the caller's char* rather than use a static buffer.
- In ipmi_bt_sm.c:
volatile unsigned char status;
The volatile is a red flag. It seems to be unneeded.
- We have #ifdef CONFIG_HIGH_RES_TIMERS code in there?
- 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.
- ipmi_wait_for_queue() doesn't need set_current_state(TASK_RUNNING);
after schedule_timeout() (I removed it)
- 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.
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.
-
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/
- Previous message: Andrew Morton: "Re: 2.6.3-mm3 hangs on boot x440 (scsi?)"
- In reply to: Corey Minyard: "Re: [PATCH] IPMI driver updates, part 1b"
- Next in thread: Corey Minyard: "Re: [PATCH] IPMI driver updates, part 1b"
- Reply: Corey Minyard: "Re: [PATCH] IPMI driver updates, part 1b"
- Reply: Corey Minyard: "Re: [PATCH] IPMI driver updates, part 1b"
- Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]
Relevant Pages
|
|
