lkml.org 
[lkml]   [2004]   [Feb]   [24]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: [PATCH] IPMI driver updates, part 1b
    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/

    \
     
     \ /
      Last update: 2005-03-22 14:01    [W:0.023 / U:31.500 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site