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 <tilman@xxxxxxx> 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: tilman@xxxxxxx
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

Relevant Pages

  • Re: O2micro smartcard reader driver.
    ... soon as possible in the upstream kernel before some kernel api changes ... again which affects your current driver. ... pccard: PCMCIA card inserted into slot 1 ... > don't know how to do locking for the concurrent ioctls. ...
    (Linux-Kernel)
  • Re: [rfc] SLOB memory ordering issue
    ... I guess I wouldn't bother with your kernel. ... And if the driver isn't using locking, ... I haven't dealt much with constructors myself so I haven't really ...
    (Linux-Kernel)
  • Re: [RFC/PATCH 0/22] W1: sysfs, lifetime and other fixes
    ... >> and they must follow protocol, defined in family driver. ... > bus - it's all the same. ... it requires different locking, which may race - and I will fix it. ... Bus master driver is low-level part that lives in it's own driver, ...
    (Linux-Kernel)
  • Re: Thanks for all the work on the MPSAFE network stack project
    ... :>> Paul Saab, Hidetoshi Shimokawa, Mike Silberback, Bruce Simpson, Gleb ... Nate Lawson fixed some ACPI related locking issues that were exposed ... by some network driver that I was locking, but since I lost all email ... And I think that some chap by the name of Robert Watson did a little ...
    (freebsd-current)
  • CFR: bridge locking
    ... Beware that buried in these changes are a renaming of the bridge MIB ... their "driver lock" before passing packets up. ... driver has a totally different locking strategy where this doesn't occur. ...
    (freebsd-net)