lkml.org 
[lkml]   [2006]   [Feb]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH 1/7] isdn4linux: Siemens Gigaset drivers - common module
    From
    Date
    On Mon, 2006-02-27 at 07:23 +0100, Hansjoerg Lipp wrote:

    > + struct semaphore sem; /* locks this structure:
    > + * connected is not changed,
    > + * hardware_up is not changed,
    > + * MState is not changed to or from
    > + * MS_LOCKED */
    > +

    please turn this into a mutex



    > +/* handling routines for sk_buff */
    > +/* ============================= */
    > +
    > +/* private version of __skb_put()
    > + * append 'len' bytes to the content of 'skb', already knowing that the
    > + * existing buffer can accomodate them
    > + * returns a pointer to the location where the new bytes should be copied to
    > + * This function does not take any locks so it must be called with the
    > + * appropriate locks held only.
    > + */
    > +static inline unsigned char *gigaset_skb_put_quick(struct sk_buff *skb,
    > + unsigned int len)
    > +{
    > + unsigned char *tmp = skb->tail;
    > + /*SKB_LINEAR_ASSERT(skb);*/ /* not needed here */
    > + skb->tail += len;
    > + skb->len += len;
    > + return tmp;
    > +}


    this looks truely scary and wrong

    > +/* append received bytes to inbuf */
    > +static inline int gigaset_fill_inbuf(struct inbuf_t *inbuf,
    > + const unsigned char *src,
    > + unsigned numbytes)
    > +{
    > + unsigned n, head, tail, bytesleft;
    > +
    > + gig_dbg(DEBUG_INTR, "received %u bytes", numbytes);
    > +
    > + if (!numbytes)
    [snip 30 lines]

    isn't this function rather big to be inlined ?
    > +
    > +/*======================================================================
    > + Prototypes of internal functions
    > + */
    > +
    > +static struct cardstate *alloc_cs(struct gigaset_driver *drv);
    > +static void free_cs(struct cardstate *cs);
    > +static void make_valid(struct cardstate *cs, unsigned mask);
    > +static void make_invalid(struct cardstate *cs, unsigned mask);

    most of the time these can just go away by ordering the functions better

    > +
    > +void gigaset_dbg_buffer(enum debuglevel level, const unsigned char *msg,
    > + size_t len, const unsigned char *buf, int from_user)


    such "from_user" parameter is highly evil, and also breaks sparse and
    friends.. (btw please run sparse on the code and fix all warnings)




    -
    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: 2006-02-27 09:02    [W:0.029 / U:1.148 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site