Thanks, Andrew, for your review. Some replies: Am 02.02.2007 02:13 schrieb Andrew Morton: > On Thu, 1 Feb 2007 22:12:24 +0100 > Tilman Schmidt <[email protected]> wrote: > >> +/* Kbuild sometimes doesn't set this */ >> +#ifndef KBUILD_MODNAME >> +#define KBUILD_MODNAME "asy_gigaset" >> +#endif > > That's a subtle way of reporting a kbuild bug ;) > > What's the story here? If an object file is linked into more than one module (like asyncdata.o which is linked into both ser_gigaset and usb_gigaset) then Kbuild compiles it only once but cannot decide which of the module names to put into KBUILD_MODNAME, so it takes the easy way out and doesn't define KBUILD_MODNAME at all. I'm not sure if that qualifies as a kbuild bug. I'd rather call it a limitation. >> +static int write_modem(struct cardstate *cs) >> +{ >> + struct tty_struct *tty = cs->hw.ser->tty; >> + struct bc_state *bcs = &cs->bcs[0]; /* only one channel */ >> + struct sk_buff *skb = bcs->tx_skb; >> + int sent; >> + >> + if (!tty || !tty->driver || !skb) >> + return -EFAULT; > > Is EFAULT appropriate? It hardly matters, as it isn't propagated anywhere. -1 would work just as well. > Can all these things happen? Theoretically no, but this is called from a tasklet and I have already traced a bug which made one of these disappear. Not having the kernel crash completely in such an event considerably helps debugging. >> + set_bit(TTY_DO_WRITE_WAKEUP, &tty->flags); > > Is a client of the tty interface supposed to be diddling tty flags like this? Documentation/tty.txt says so. (Yes, I wrote that part myself, but nobody protested. ;-) Also, the PPP line discipline does the same. >> + spin_lock_irqsave(&cs->cmdlock, flags); >> + cb = cs->cmdbuf; >> + spin_unlock_irqrestore(&cs->cmdlock, flags); > > It is doubtful if the locking here does anything useful. It assures atomicity when reading the cs->cmdbuf pointer. >> + spin_lock_irqsave(&cs->cmdlock, flags); >> + cb->prev = cs->lastcmdbuf; >> + if (cs->lastcmdbuf) >> + cs->lastcmdbuf->next = cb; >> + else { >> + cs->cmdbuf = cb; >> + cs->curlen = len; >> + } >> + cs->cmdbytes += len; >> + cs->lastcmdbuf = cb; >> + spin_unlock_irqrestore(&cs->cmdlock, flags); > > Would the use of list_heads simplify things here? I don't think so. The operations in list.h do not keep track of the total byte count, and adding that in a race-free way appears non-trivial. >> +/* >> + * Free hardware specific device data >> + * This will be called by "gigaset_freecs" in common.c >> + */ >> +static void gigaset_freecshw(struct cardstate *cs) >> +{ >> + tasklet_kill(&cs->write_tasklet); > > Does tasklet kill() wait for the tasklet to stop running on a different > CPU? I thing so, but it was written in the days before we commented code. Its description in LDD3 ch. 7 seems to imply that it does. >> + down(&cs->hw.ser->dead_sem); > > Does this actually use the semaphore's counting feature? If not, can we > switch it to a mutex? I stole that code from the PPP line discipline. It is to assure all other ldisc methods have completed before the close method proceeds. This doesn't look like a case for a mutex to me, but I'm open to suggestions if it's important to avoid a semaphore here. >> + tail = atomic_read(&inbuf->tail); >> + head = atomic_read(&inbuf->head); >> + gig_dbg(DEBUG_INTR, "buffer state: %u -> %u, receive %u bytes", >> + head, tail, count); >> + >> + if (head <= tail) { >> + n = RBUFSIZE - tail; >> + if (count >= n) { >> + /* buffer wraparound */ >> + memcpy(inbuf->data + tail, buf, n); >> + tail = 0; >> + buf += n; >> + count -= n; >> + } else { >> + memcpy(inbuf->data + tail, buf, count); >> + tail += count; >> + buf += count; >> + count = 0; >> + } >> + } > > Perhaps the (fairly revolting) circ_buf.h can be used for this stuff. It probably could, but IMHO readability would suffer rather than improve. Thanks, Tilman PS: My patch hasn't appeared on LKML so far. Any idea why? -- Tilman Schmidt E-Mail: [email protected] Bonn, Germany Diese Nachricht besteht zu 100% aus wiederverwerteten Bits. Ungeöffnet mindestens haltbar bis: (siehe Rückseite)
Attachment:
signature.asc
Description: OpenPGP digital signature
- Follow-Ups:
- Re: [PATCH] drivers/isdn/gigaset: new M101 driver
- From: Andrew Morton <[email protected]>
- Re: [PATCH] drivers/isdn/gigaset: new M101 driver
- References:
- Re: [PATCH] drivers/isdn/gigaset: new M101 driver
- From: Andrew Morton <[email protected]>
- Re: [PATCH] drivers/isdn/gigaset: new M101 driver
- Prev by Date: Re: O_NONBLOCK setting "leak" outside of a process??
- Next by Date: Re: [patch 1/1] PM: Adds remount fs ro at suspend
- Previous by thread: Re: [PATCH] drivers/isdn/gigaset: new M101 driver
- Next by thread: Re: [PATCH] drivers/isdn/gigaset: new M101 driver
- Index(es):
 |