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

[Andrew Morton <akpm@osdl.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.