Re: [PATCH] drivers/isdn/gigaset: new M101 driver
Am 12.02.2007 19:47 schrieb Greg KH: > +static void gigaset_device_release(struct device *dev) > +{ > + //FIXME anything to do? cf. platform_device_release() > +} > The memory of the platform device itself needs to be freed here, > otherwise, to do it earlier would cause race conditions and oopses. I don't do it earlier. I do it later. My platform_device structure is part of my driver's device state structure which is freed explicitly later after the call to platform_device_unregister(). Is that bad? > Look at how the other platform drivers do things. They do things differently from each other as well as from mine. block/floppy.c, for example, just has a call to complete() there. Anyway, in the latest version of my driver, its platform_device release function finally does something, too: it frees dev->platform_data and pdev->resource just in case something might have materialized there. I hope that's ok. Regards, Tilman -- Tilman Schmidt E-Mail: [EMAIL PROTECTED] Bonn, Germany Diese Nachricht besteht zu 100% aus wiederverwerteten Bits. Ungeöffnet mindestens haltbar bis: (siehe Rückseite) signature.asc Description: OpenPGP digital signature
Re: [PATCH] drivers/isdn/gigaset: new M101 driver
On Sun, Feb 04, 2007 at 01:26:48AM +0100, Tilman Schmidt wrote: > Am 03.02.2007 17:09 schrieb Greg KH: > > On Thu, Feb 01, 2007 at 05:13:45PM -0800, Andrew Morton wrote: > >>> +/* dummy to shut up framework warning */ > >>> +static void gigaset_device_release(struct device *dev) > >>> +{ > >>> + //FIXME anything to do? cf. platform_device_release() > >>> +} > >> Ask Greg ;) > > > > Oh come on people. Don't provide an empty function because the kernel > > is complaining that you don't provide a release function. Yes it will > > shut the kernel up but did you ever think _why_ the kernel was > > complaining? > > Actually, I did. Just guess how that FIXME came to be. > Hint: the kernel shuts up just as well without it. Sure, but only because I can't do a test for a function pointer that points to a function that does nothing :( > > You need to free your memory here, don't just provide an empty function, > > that shows the driver is broken. > > Call me silly and incapable of thinking, but to me it's far from > clear _what_ memory I am supposed to free there. Certainly neither > memory I will still be needing after platform_device_unregister() > nor memory that's being taken care of elsewhere. Between the two, > in my case there's nothing left, so the release function is empty. > If that shows my driver is broken, please explain in what way. The memory of the platform device itself needs to be freed here, otherwise, to do it earlier would cause race conditions and oopses. Look at how the other platform drivers do things. thanks, greg k-h - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] drivers/isdn/gigaset: new M101 driver
On Sun, Feb 04, 2007 at 01:26:48AM +0100, Tilman Schmidt wrote: Am 03.02.2007 17:09 schrieb Greg KH: On Thu, Feb 01, 2007 at 05:13:45PM -0800, Andrew Morton wrote: +/* dummy to shut up framework warning */ +static void gigaset_device_release(struct device *dev) +{ + //FIXME anything to do? cf. platform_device_release() +} Ask Greg ;) Oh come on people. Don't provide an empty function because the kernel is complaining that you don't provide a release function. Yes it will shut the kernel up but did you ever think _why_ the kernel was complaining? Actually, I did. Just guess how that FIXME came to be. Hint: the kernel shuts up just as well without it. Sure, but only because I can't do a test for a function pointer that points to a function that does nothing :( You need to free your memory here, don't just provide an empty function, that shows the driver is broken. Call me silly and incapable of thinking, but to me it's far from clear _what_ memory I am supposed to free there. Certainly neither memory I will still be needing after platform_device_unregister() nor memory that's being taken care of elsewhere. Between the two, in my case there's nothing left, so the release function is empty. If that shows my driver is broken, please explain in what way. The memory of the platform device itself needs to be freed here, otherwise, to do it earlier would cause race conditions and oopses. Look at how the other platform drivers do things. thanks, greg k-h - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] drivers/isdn/gigaset: new M101 driver
Am 12.02.2007 19:47 schrieb Greg KH: +static void gigaset_device_release(struct device *dev) +{ + //FIXME anything to do? cf. platform_device_release() +} The memory of the platform device itself needs to be freed here, otherwise, to do it earlier would cause race conditions and oopses. I don't do it earlier. I do it later. My platform_device structure is part of my driver's device state structure which is freed explicitly later after the call to platform_device_unregister(). Is that bad? Look at how the other platform drivers do things. They do things differently from each other as well as from mine. block/floppy.c, for example, just has a call to complete() there. Anyway, in the latest version of my driver, its platform_device release function finally does something, too: it frees dev-platform_data and pdev-resource just in case something might have materialized there. I hope that's ok. Regards, Tilman -- Tilman Schmidt E-Mail: [EMAIL PROTECTED] Bonn, Germany Diese Nachricht besteht zu 100% aus wiederverwerteten Bits. Ungeöffnet mindestens haltbar bis: (siehe Rückseite) signature.asc Description: OpenPGP digital signature
Re: [PATCH] drivers/isdn/gigaset: new M101 driver
Andrew Morton schrieb: > On Mon, 05 Feb 2007 02:42:09 +0100 Tilman Schmidt <[EMAIL PROTECTED]> wrote: > >> It's a pointer. Are reads and writes of pointer sized objects >> guaranteed to be atomic on every platform? > > Yup - we make the same assumption about longs in various places. > > It's a bit strange to read a pointer which can be changing at the > same time. Because the local copy will no longer represent the > thing which it was just copied from. It's not that bad. The only possible concurrent change is from NULL to non-NULL. Fearing an inconsistent read in that event was too paranoid apparently. Thanks for all your suggestions. I'll prepare a new patch based on them. -- Tilman SchmidtE-Mail: [EMAIL PROTECTED] Bonn, Germany Diese Nachricht besteht zu 100% aus wiederverwerteten Bits. Ungeöffnet mindestens haltbar bis: (siehe Rückseite) signature.asc Description: OpenPGP digital signature
Re: [PATCH] drivers/isdn/gigaset: new M101 driver
Andrew Morton schrieb: On Mon, 05 Feb 2007 02:42:09 +0100 Tilman Schmidt [EMAIL PROTECTED] wrote: It's a pointer. Are reads and writes of pointer sized objects guaranteed to be atomic on every platform? Yup - we make the same assumption about longs in various places. It's a bit strange to read a pointer which can be changing at the same time. Because the local copy will no longer represent the thing which it was just copied from. It's not that bad. The only possible concurrent change is from NULL to non-NULL. Fearing an inconsistent read in that event was too paranoid apparently. Thanks for all your suggestions. I'll prepare a new patch based on them. -- Tilman SchmidtE-Mail: [EMAIL PROTECTED] Bonn, Germany Diese Nachricht besteht zu 100% aus wiederverwerteten Bits. Ungeöffnet mindestens haltbar bis: (siehe Rückseite) signature.asc Description: OpenPGP digital signature
Re: [PATCH] drivers/isdn/gigaset: new M101 driver
On Mon, 05 Feb 2007 02:42:09 +0100 Tilman Schmidt <[EMAIL PROTECTED]> wrote: > Am 04.02.2007 02:56 schrieb Andrew Morton: > > On Sun, 04 Feb 2007 02:32:41 +0100 Tilman Schmidt <[EMAIL PROTECTED]> wrote: > > > +spin_lock_irqsave(>cmdlock, flags); > +cb = cs->cmdbuf; > +spin_unlock_irqrestore(>cmdlock, flags); > >>> It is doubtful if the locking here does anything useful. > >> It assures atomicity when reading the cs->cmdbuf pointer. > > > > I think it's bogus. If the quantity being copied here is more than 32-bits > > then yes, a lock is appropriate. But if it's a single word then it's > > unlikely that the locking does anything useful. Or there might be a bug > > here. > > It's a pointer. Are reads and writes of pointer sized objects > guaranteed to be atomic on every platform? Yup - we make the same assumption about longs in various places. It's a bit strange to read a pointer which can be changing at the same time. Because the local copy will no longer represent the thing which it was just copied from. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] drivers/isdn/gigaset: new M101 driver
Am 04.02.2007 02:56 schrieb Andrew Morton: > On Sun, 04 Feb 2007 02:32:41 +0100 Tilman Schmidt <[EMAIL PROTECTED]> wrote: > + spin_lock_irqsave(>cmdlock, flags); + cb = cs->cmdbuf; + spin_unlock_irqrestore(>cmdlock, flags); >>> It is doubtful if the locking here does anything useful. >> It assures atomicity when reading the cs->cmdbuf pointer. > > I think it's bogus. If the quantity being copied here is more than 32-bits > then yes, a lock is appropriate. But if it's a single word then it's > unlikely that the locking does anything useful. Or there might be a bug > here. It's a pointer. Are reads and writes of pointer sized objects guaranteed to be atomic on every platform? If so, I'll happily omit the locking. + spin_lock_irqsave(>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(>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. > > Maintaining a byte count isn't related to maintaining a list. Sure. But your question was whether the list.h operations would simplify this code. AFAICS it wouldn't, because the necessity of maintaining the byte count would complicate a list.h based solution beyond the current one. Also, this is part of the interface with the components of the Gigaset driver which are already part of the kernel. Changing this to a list_head now would require significant changes in those other parts, too. + tail = atomic_read(>tail); + head = atomic_read(>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. > > How about kernel/kfifo.c? That would indeed fit the bill. But again, this code matches parts of drivers/isdn/gigaset which are already in the kernel, and changing it here would require significant corresponding changes in those other parts. I'll gladly consider your last two propositions (list_head for cs->lastcmdbuf and kfifo for cs->inbuf) for a future revision of the entire set of drivers in drivers/isdn/gigaset, but it goes way beyond the scope of the present patch, which merely aims at adding the missing M101 hardware driver. Thanks, Tilman -- Tilman Schmidt E-Mail: [EMAIL PROTECTED] Bonn, Germany Diese Nachricht besteht zu 100% aus wiederverwerteten Bits. Ungeöffnet mindestens haltbar bis: (siehe Rückseite) signature.asc Description: OpenPGP digital signature
Re: [PATCH] drivers/isdn/gigaset: new M101 driver
Am 04.02.2007 02:56 schrieb Andrew Morton: On Sun, 04 Feb 2007 02:32:41 +0100 Tilman Schmidt [EMAIL PROTECTED] wrote: + 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. I think it's bogus. If the quantity being copied here is more than 32-bits then yes, a lock is appropriate. But if it's a single word then it's unlikely that the locking does anything useful. Or there might be a bug here. It's a pointer. Are reads and writes of pointer sized objects guaranteed to be atomic on every platform? If so, I'll happily omit the locking. + 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. Maintaining a byte count isn't related to maintaining a list. Sure. But your question was whether the list.h operations would simplify this code. AFAICS it wouldn't, because the necessity of maintaining the byte count would complicate a list.h based solution beyond the current one. Also, this is part of the interface with the components of the Gigaset driver which are already part of the kernel. Changing this to a list_head now would require significant changes in those other parts, too. + 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. How about kernel/kfifo.c? That would indeed fit the bill. But again, this code matches parts of drivers/isdn/gigaset which are already in the kernel, and changing it here would require significant corresponding changes in those other parts. I'll gladly consider your last two propositions (list_head for cs-lastcmdbuf and kfifo for cs-inbuf) for a future revision of the entire set of drivers in drivers/isdn/gigaset, but it goes way beyond the scope of the present patch, which merely aims at adding the missing M101 hardware driver. Thanks, Tilman -- Tilman Schmidt E-Mail: [EMAIL PROTECTED] Bonn, Germany Diese Nachricht besteht zu 100% aus wiederverwerteten Bits. Ungeöffnet mindestens haltbar bis: (siehe Rückseite) signature.asc Description: OpenPGP digital signature
Re: [PATCH] drivers/isdn/gigaset: new M101 driver
On Mon, 05 Feb 2007 02:42:09 +0100 Tilman Schmidt [EMAIL PROTECTED] wrote: Am 04.02.2007 02:56 schrieb Andrew Morton: On Sun, 04 Feb 2007 02:32:41 +0100 Tilman Schmidt [EMAIL PROTECTED] wrote: +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. I think it's bogus. If the quantity being copied here is more than 32-bits then yes, a lock is appropriate. But if it's a single word then it's unlikely that the locking does anything useful. Or there might be a bug here. It's a pointer. Are reads and writes of pointer sized objects guaranteed to be atomic on every platform? Yup - we make the same assumption about longs in various places. It's a bit strange to read a pointer which can be changing at the same time. Because the local copy will no longer represent the thing which it was just copied from. - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] drivers/isdn/gigaset: new M101 driver
On Sun, 04 Feb 2007 02:32:41 +0100 Tilman Schmidt <[EMAIL PROTECTED]> wrote: > >> + spin_lock_irqsave(>cmdlock, flags); > >> + cb = cs->cmdbuf; > >> + spin_unlock_irqrestore(>cmdlock, flags); > > > > It is doubtful if the locking here does anything useful. > > It assures atomicity when reading the cs->cmdbuf pointer. I think it's bogus. If the quantity being copied here is more than 32-bits then yes, a lock is appropriate. But if it's a single word then it's unlikely that the locking does anything useful. Or there might be a bug here. > >> + spin_lock_irqsave(>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(>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. Maintaining a byte count isn't related to maintaining a list. > >> + down(>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. If a sleeping lock is being used as a mutex, please use a mutex. We prefer that semaphores only be used in those situations where their counting feature is being used. Reasons: a) mutexes have better runtime debugging support and b) Ingo had some plans to reimplement semaphores in an arch-neutral way and for some reason reducing the number of callers would help that. I forget what the reason was, actually. > >> + tail = atomic_read(>tail); > >> + head = atomic_read(>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. > How about kernel/kfifo.c? - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] drivers/isdn/gigaset: new M101 driver
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 = >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, >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(>cmdlock, flags); >> +cb = cs->cmdbuf; >> +spin_unlock_irqrestore(>cmdlock, flags); > > It is doubtful if the locking here does anything useful. It assures atomicity when reading the cs->cmdbuf pointer. >> +spin_lock_irqsave(>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(>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(>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(>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(>tail); >> +head = atomic_read(>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) signature.asc Description: OpenPGP digital signature
Re: [PATCH] drivers/isdn/gigaset: new M101 driver
Am 03.02.2007 17:09 schrieb Greg KH: > On Thu, Feb 01, 2007 at 05:13:45PM -0800, Andrew Morton wrote: >>> +/* dummy to shut up framework warning */ >>> +static void gigaset_device_release(struct device *dev) >>> +{ >>> + //FIXME anything to do? cf. platform_device_release() >>> +} >> Ask Greg ;) > > Oh come on people. Don't provide an empty function because the kernel > is complaining that you don't provide a release function. Yes it will > shut the kernel up but did you ever think _why_ the kernel was > complaining? Actually, I did. Just guess how that FIXME came to be. Hint: the kernel shuts up just as well without it. > Did you think it did it just for fun? No, that was not among the explanations I considered. Although, come to think of it ... nah, that would really be too weird. > Think people, think... Ahem. > You need to free your memory here, don't just provide an empty function, > that shows the driver is broken. Call me silly and incapable of thinking, but to me it's far from clear _what_ memory I am supposed to free there. Certainly neither memory I will still be needing after platform_device_unregister() nor memory that's being taken care of elsewhere. Between the two, in my case there's nothing left, so the release function is empty. If that shows my driver is broken, please explain in what way. Thanks, Tilman -- Tilman Schmidt E-Mail: [EMAIL PROTECTED] Bonn, Germany Diese Nachricht besteht zu 100% aus wiederverwerteten Bits. Ungeöffnet mindestens haltbar bis: (siehe Rückseite) signature.asc Description: OpenPGP digital signature
Re: [PATCH] drivers/isdn/gigaset: new M101 driver
On Thu, Feb 01, 2007 at 05:13:45PM -0800, Andrew Morton wrote: > > +/* dummy to shut up framework warning */ > > +static void gigaset_device_release(struct device *dev) > > +{ > > + //FIXME anything to do? cf. platform_device_release() > > +} > > Ask Greg ;) Oh come on people. Don't provide an empty function because the kernel is complaining that you don't provide a release function. Yes it will shut the kernel up but did you ever think _why_ the kernel was complaining? Did you think it did it just for fun? Think people, think... You need to free your memory here, don't just provide an empty function, that shows the driver is broken. thanks, greg k-h - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] drivers/isdn/gigaset: new M101 driver
On Thu, Feb 01, 2007 at 05:13:45PM -0800, Andrew Morton wrote: +/* dummy to shut up framework warning */ +static void gigaset_device_release(struct device *dev) +{ + //FIXME anything to do? cf. platform_device_release() +} Ask Greg ;) Oh come on people. Don't provide an empty function because the kernel is complaining that you don't provide a release function. Yes it will shut the kernel up but did you ever think _why_ the kernel was complaining? Did you think it did it just for fun? Think people, think... You need to free your memory here, don't just provide an empty function, that shows the driver is broken. thanks, greg k-h - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] drivers/isdn/gigaset: new M101 driver
Am 03.02.2007 17:09 schrieb Greg KH: On Thu, Feb 01, 2007 at 05:13:45PM -0800, Andrew Morton wrote: +/* dummy to shut up framework warning */ +static void gigaset_device_release(struct device *dev) +{ + //FIXME anything to do? cf. platform_device_release() +} Ask Greg ;) Oh come on people. Don't provide an empty function because the kernel is complaining that you don't provide a release function. Yes it will shut the kernel up but did you ever think _why_ the kernel was complaining? Actually, I did. Just guess how that FIXME came to be. Hint: the kernel shuts up just as well without it. Did you think it did it just for fun? No, that was not among the explanations I considered. Although, come to think of it ... nah, that would really be too weird. Think people, think... Ahem. You need to free your memory here, don't just provide an empty function, that shows the driver is broken. Call me silly and incapable of thinking, but to me it's far from clear _what_ memory I am supposed to free there. Certainly neither memory I will still be needing after platform_device_unregister() nor memory that's being taken care of elsewhere. Between the two, in my case there's nothing left, so the release function is empty. If that shows my driver is broken, please explain in what way. Thanks, Tilman -- Tilman Schmidt E-Mail: [EMAIL PROTECTED] Bonn, Germany Diese Nachricht besteht zu 100% aus wiederverwerteten Bits. Ungeöffnet mindestens haltbar bis: (siehe Rückseite) signature.asc Description: OpenPGP digital signature
Re: [PATCH] drivers/isdn/gigaset: new M101 driver
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) signature.asc Description: OpenPGP digital signature
Re: [PATCH] drivers/isdn/gigaset: new M101 driver
On Sun, 04 Feb 2007 02:32:41 +0100 Tilman Schmidt [EMAIL PROTECTED] wrote: + 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. I think it's bogus. If the quantity being copied here is more than 32-bits then yes, a lock is appropriate. But if it's a single word then it's unlikely that the locking does anything useful. Or there might be a bug here. + 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. Maintaining a byte count isn't related to maintaining a list. + 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. If a sleeping lock is being used as a mutex, please use a mutex. We prefer that semaphores only be used in those situations where their counting feature is being used. Reasons: a) mutexes have better runtime debugging support and b) Ingo had some plans to reimplement semaphores in an arch-neutral way and for some reason reducing the number of callers would help that. I forget what the reason was, actually. + 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. How about kernel/kfifo.c? - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] drivers/isdn/gigaset: new M101 driver
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? > --- linux-2.6.20-rc6-mm3-orig/drivers/isdn/gigaset/ser-gigaset.c > 1970-01-01 01:00:00.0 +0100 > +++ local/drivers/isdn/gigaset/ser-gigaset.c 2007-01-29 23:41:39.0 > +0100 > @@ -0,0 +1,853 @@ > +/* This is the serial hardware link layer (HLL) for the Gigaset 307x isdn > + * DECT base (aka Sinus 45 isdn) using the RS232 DECT data module M101, > + * written as a line discipline. > + * > + * = > + * This program is free software; you can redistribute it and/or > + * modify it under the terms of the GNU General Public License as > + * published by the Free Software Foundation; either version 2 of > + * the License, or (at your option) any later version. > + * = > + */ > + > +#include "gigaset.h" > + > +#include > +#include > +#include > +#include > +#include > + > +/* Version Information */ > +#define DRIVER_AUTHOR "Tilman Schmidt" > +#define DRIVER_DESC "Serial Driver for Gigaset 307x using Siemens M101" > + > +#define GIGASET_MINORS 1 > +#define GIGASET_MINOR 0 > +#define GIGASET_MODULENAME "ser_gigaset" > +#define GIGASET_DEVNAME"ttyGS" > + > +/* length limit according to Siemens 3070usb-protokoll.doc ch. 2.1 */ > +#define IF_WRITEBUF 264 > + > +MODULE_AUTHOR(DRIVER_AUTHOR); > +MODULE_DESCRIPTION(DRIVER_DESC); > +MODULE_LICENSE("GPL"); > +MODULE_ALIAS_LDISC(N_GIGASET_M101); > + > +static int startmode = SM_ISDN; > +module_param(startmode, int, S_IRUGO); > +MODULE_PARM_DESC(startmode, "initial operation mode"); > +static int cidmode = 1; > +module_param(cidmode, int, S_IRUGO); > +MODULE_PARM_DESC(cidmode, "stay in CID mode when idle"); > + > +static struct gigaset_driver *driver = NULL; Unneeded initialisation. > +struct ser_cardstate { > + struct platform_device dev; > + struct tty_struct *tty; > + atomic_trefcnt; > + struct semaphoredead_sem; > +}; > + > +static struct platform_driver device_driver = { > + .driver = { > + .name = GIGASET_MODULENAME, > + }, > +}; > + > +struct ser_bc_state {}; Does this need kernel-wide scope? > +static void flush_send_queue(struct cardstate *); > + > +/* transmit data from current open skb > + * result: number of bytes sent or error code < 0 > + */ > +static int write_modem(struct cardstate *cs) > +{ > + struct tty_struct *tty = cs->hw.ser->tty; > + struct bc_state *bcs = >bcs[0]; /* only one channel */ > + struct sk_buff *skb = bcs->tx_skb; > + int sent; > + > + if (!tty || !tty->driver || !skb) > + return -EFAULT; Is EFAULT appropriate? Can all these things happen? > + if (!skb->len) { > + dev_kfree_skb_any(skb); > + bcs->tx_skb = NULL; > + return -EINVAL; > + } > + > + set_bit(TTY_DO_WRITE_WAKEUP, >flags); Is a client of the tty interface supposed to be diddling tty flags like this? > + sent = tty->driver->write(tty, skb->data, skb->len); > + gig_dbg(DEBUG_OUTPUT, "write_modem: sent %d", sent); > + if (sent < 0) { > + /* error */ > + flush_send_queue(cs); > + return sent; > + } > + skb_pull(skb, sent); > + if (!skb->len) { > + /* skb sent completely */ > + gigaset_skb_sent(bcs, skb); > + > + gig_dbg(DEBUG_INTR, "kfree skb (Adr: %lx)!", > + (unsigned long) skb); > + dev_kfree_skb_any(skb); > + bcs->tx_skb = NULL; > + } > + return sent; > +} > + > +/* > + * transmit first queued command buffer > + * result: number of bytes sent or error code < 0 > + */ > +static int send_cb(struct cardstate *cs) > +{ > + struct tty_struct *tty = cs->hw.ser->tty; > + struct cmdbuf_t *cb, *tcb; > + unsigned long flags; > + int sent = 0; > + > + if (!tty || !tty->driver) > + return -EFAULT; > + > + spin_lock_irqsave(>cmdlock, flags); > + cb = cs->cmdbuf; > + spin_unlock_irqrestore(>cmdlock, flags); It is doubtful if the locking here does anything useful. > + if (!cb) > + return 0; /* nothing to do */ > + > + if (cb->len) { > + set_bit(TTY_DO_WRITE_WAKEUP, >flags); > + sent = tty->driver->write(tty, cb->buf + cb->offset, cb->len); > + if (sent < 0) { > + /* error */ > + gig_dbg(DEBUG_OUTPUT, "send_cb: write error %d", sent); > + flush_send_queue(cs); > + return sent; > + } > + cb->offset += sent; > + cb->len -= sent; > +
Re: [PATCH] drivers/isdn/gigaset: new M101 driver
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? --- linux-2.6.20-rc6-mm3-orig/drivers/isdn/gigaset/ser-gigaset.c 1970-01-01 01:00:00.0 +0100 +++ local/drivers/isdn/gigaset/ser-gigaset.c 2007-01-29 23:41:39.0 +0100 @@ -0,0 +1,853 @@ +/* This is the serial hardware link layer (HLL) for the Gigaset 307x isdn + * DECT base (aka Sinus 45 isdn) using the RS232 DECT data module M101, + * written as a line discipline. + * + * = + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License as + * published by the Free Software Foundation; either version 2 of + * the License, or (at your option) any later version. + * = + */ + +#include gigaset.h + +#include linux/module.h +#include linux/moduleparam.h +#include linux/platform_device.h +#include linux/tty.h +#include linux/poll.h + +/* Version Information */ +#define DRIVER_AUTHOR Tilman Schmidt +#define DRIVER_DESC Serial Driver for Gigaset 307x using Siemens M101 + +#define GIGASET_MINORS 1 +#define GIGASET_MINOR 0 +#define GIGASET_MODULENAME ser_gigaset +#define GIGASET_DEVNAMEttyGS + +/* length limit according to Siemens 3070usb-protokoll.doc ch. 2.1 */ +#define IF_WRITEBUF 264 + +MODULE_AUTHOR(DRIVER_AUTHOR); +MODULE_DESCRIPTION(DRIVER_DESC); +MODULE_LICENSE(GPL); +MODULE_ALIAS_LDISC(N_GIGASET_M101); + +static int startmode = SM_ISDN; +module_param(startmode, int, S_IRUGO); +MODULE_PARM_DESC(startmode, initial operation mode); +static int cidmode = 1; +module_param(cidmode, int, S_IRUGO); +MODULE_PARM_DESC(cidmode, stay in CID mode when idle); + +static struct gigaset_driver *driver = NULL; Unneeded initialisation. +struct ser_cardstate { + struct platform_device dev; + struct tty_struct *tty; + atomic_trefcnt; + struct semaphoredead_sem; +}; + +static struct platform_driver device_driver = { + .driver = { + .name = GIGASET_MODULENAME, + }, +}; + +struct ser_bc_state {}; Does this need kernel-wide scope? +static void flush_send_queue(struct cardstate *); + +/* transmit data from current open skb + * result: number of bytes sent or error code 0 + */ +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? Can all these things happen? + if (!skb-len) { + dev_kfree_skb_any(skb); + bcs-tx_skb = NULL; + return -EINVAL; + } + + set_bit(TTY_DO_WRITE_WAKEUP, tty-flags); Is a client of the tty interface supposed to be diddling tty flags like this? + sent = tty-driver-write(tty, skb-data, skb-len); + gig_dbg(DEBUG_OUTPUT, write_modem: sent %d, sent); + if (sent 0) { + /* error */ + flush_send_queue(cs); + return sent; + } + skb_pull(skb, sent); + if (!skb-len) { + /* skb sent completely */ + gigaset_skb_sent(bcs, skb); + + gig_dbg(DEBUG_INTR, kfree skb (Adr: %lx)!, + (unsigned long) skb); + dev_kfree_skb_any(skb); + bcs-tx_skb = NULL; + } + return sent; +} + +/* + * transmit first queued command buffer + * result: number of bytes sent or error code 0 + */ +static int send_cb(struct cardstate *cs) +{ + struct tty_struct *tty = cs-hw.ser-tty; + struct cmdbuf_t *cb, *tcb; + unsigned long flags; + int sent = 0; + + if (!tty || !tty-driver) + return -EFAULT; + + 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. + if (!cb) + return 0; /* nothing to do */ + + if (cb-len) { + set_bit(TTY_DO_WRITE_WAKEUP, tty-flags); + sent = tty-driver-write(tty, cb-buf + cb-offset, cb-len); + if (sent 0) { + /* error */ + gig_dbg(DEBUG_OUTPUT, send_cb: write error %d, sent); + flush_send_queue(cs); + return sent; + } + cb-offset += sent; + cb-len -= sent; + gig_dbg(DEBUG_OUTPUT, send_cb: sent %d, left %u, queued %u, + sent, cb-len,