Messages in this thread | | | Subject | Re: [PATCH 1/7] isdn4linux: Siemens Gigaset drivers - common module | From | Arjan van de Ven <> | Date | Mon, 27 Feb 2006 09:00:01 +0100 |
| |
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/
| |