Re: [PATCH 6/9] isdn4linux: Siemens Gigaset drivers - procfs interface

[Tilman Schmidt <tilman@imap.cc>]

Andrew,

thank you very much for your comments.

On 12.02.2006 11:27, Andrew Morton wrote:
> 
>>This patch adds the procfs interface to the gigaset module.
> 
> sysfs, actually.

Yes, sorry, the comment didn't get updated when we did the move from
procfs to sysfs.

> - The ringbuffer head and tail indexes are atomic_t's, but always seem to
>   be manipulated inside the lock.  Perhaps they can become integers.

We use ringbuffers in two places. I suppose you are talking about the
one implementing our event queue through the ev_head and ev_tail members
of the cardstate structure. You have a point there, although I'm not
sure if it wouldn't be better to take advantage of the atomic_t and do a
little less locking instead.

The other one, within the inbuf_t structure, used for capturing
characters received from the device, is accessed without locking from
the gigaset_handle_event() tasklet and therefore needs atomic_t indexes,
IMHO.

> - You did the ringbuffer the wrong way.  Don't constrain the head and
>   tail to be within 0..MAX_EVENTS.  Instead, just let them wrap right up to
>   0xffffffff.   Apply the masking when you actually _use_ them.
> 
>   That way, empty is (head == tail) and full is (tail - head == MAX_EVENTS).

Interesting idea. I have to admit it's rather new to me. I have always
done ringbuffers the way they are done in the Gigaset driver now. Can
you point me to some example code done the way you propose, so I can
familiarize myself with its advantages?

> - Could use kstrdup() in a few places.

Thanks for the hint. Will watch out for these.

> - A few unneeded casts of void*'s, but everyone does that.

:-)

> - There are a lot of global symbols in there.  Perhaps they don't all
>   need to be global.

Well, I *hope* there aren't any unnecessary ones, but we'll re-check.

Regards
Tilman

-- 
Tilman Schmidt                          E-Mail: tilman@imap.cc
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