Re: [PATCH] drivers/isdn/gigaset: new M101 driver

Re: [PATCH] drivers/isdn/gigaset: new M101 driver

[Andrew Morton]

On Thu, 1 Feb 2007 22:12:24 +0100
Tilman Schmidt <tilman@imap.cc> 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.000000000 +0100
> +++ local/drivers/isdn/gigaset/ser-gigaset.c	2007-01-29 23:41:39.000000000 +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_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_t		refcnt;
> +	struct semaphore	dead_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, cs->cmdbytes);
> +	}
> +
> +	while (cb && !cb->len) {
> +		spin_lock_irqsave(&cs->cmdlock, flags);
> +		cs->cmdbytes -= cs->curlen;
> +		tcb = cb;
> +		cs->cmdbuf = cb = cb->next;
> +		if (cb) {
> +			cb->prev = NULL;
> +			cs->curlen = cb->len;
> +		} else {
> +			cs->lastcmdbuf = NULL;
> +			cs->curlen = 0;
> +		}
> +		spin_unlock_irqrestore(&cs->cmdlock, flags);
> +
> +		if (tcb->wake_tasklet)
> +			tasklet_schedule(tcb->wake_tasklet);
> +		kfree(tcb);
> +	}
> +	return sent;
> +}
> +
>
> ...
>
> +/*
> + * queue an AT command string for transmission to the Gigaset device
> + * parameters:
> + *	cs		controller state structure
> + *	buf		buffer containing the string to send
> + *	len		number of characters to send
> + *	wake_tasklet	tasklet to run when transmission is complete, or NULL
> + * return value:
> + *	number of bytes queued, or error code < 0
> + */
> +static int gigaset_write_cmd(struct cardstate *cs, const unsigned char *buf,
> +                             int len, struct tasklet_struct *wake_tasklet)
> +{
> +	struct cmdbuf_t *cb;
> +	unsigned long flags;
> +
> +	gigaset_dbg_buffer(atomic_read(&cs->mstate) != MS_LOCKED ?
> +	                     DEBUG_TRANSCMD : DEBUG_LOCKCMD,
> +	                   "CMD Transmit", len, buf);
> +
> +	if (len <= 0)
> +		return 0;
> +
> +	if (!(cb = kmalloc(sizeof(struct cmdbuf_t) + len, GFP_ATOMIC))) {
> +		dev_err(cs->dev, "%s: out of memory!\n", __func__);
> +		return -ENOMEM;
> +	}
> +
> +	memcpy(cb->buf, buf, len);
> +	cb->len = len;
> +	cb->offset = 0;
> +	cb->next = NULL;
> +	cb->wake_tasklet = wake_tasklet;
> +
> +	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?  Or are we dealing with a
hardware-imposed layout?

> +	spin_lock_irqsave(&cs->lock, flags);
> +	if (cs->connected)
> +		tasklet_schedule(&cs->write_tasklet);
> +	spin_unlock_irqrestore(&cs->lock, flags);
> +	return len;
> +}
> +
> ...
> +
> +/*
> + * implementation of ioctl(GIGASET_BRKCHARS)
> + * parameter:
> + *	controller state structure
> + * return value:
> + *	-EINVAL (unimplemented function)
> + */
> +static int gigaset_brkchars(struct cardstate *cs, const unsigned char buf[6])
> +{
> +	/* not implemented */
> +	return -EINVAL;
> +}

ENOTTY might be more appropriate here.

> +/*
> + * 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.

> +	if (!cs->hw.ser)
> +		return;
> +	dev_set_drvdata(&cs->hw.ser->dev.dev, NULL);
> +	platform_device_unregister(&cs->hw.ser->dev);
> +	kfree(cs->hw.ser);
> +	cs->hw.ser = NULL;
> +}
> +
> +/* dummy to shut up framework warning */
> +static void gigaset_device_release(struct device *dev)
> +{
> +	//FIXME anything to do? cf. platform_device_release()
> +}

Ask Greg ;)

> +			down(&cs->hw.ser->dead_sem);

Does this actually use the semaphore's counting feature?  If not, can we
switch it to a mutex?

> +/*
> + * Ioctl on the tty.
> + * Called in process context only.
> + * May be re-entered by multiple ioctl calling threads.
> + */
> +static int
> +gigaset_tty_ioctl(struct tty_struct *tty, struct file *file,
> +		  unsigned int cmd, unsigned long arg)
> +{
> +	struct cardstate *cs = cs_get(tty);
> +	int rc, val;
> +	int __user *p = (int __user *)arg;
> +
> +	if (!cs)
> +		return -ENXIO;
> +
> +	switch (cmd) {
> +	case TCGETS:
> +	case TCGETA:
> +		/* pass through to underlying serial device */
> +		rc = n_tty_ioctl(tty, file, cmd, arg);
> +		break;
> +
> +	case TCFLSH:
> +		/* flush our buffers and the serial port's buffer */
> +		switch (arg) {
> +		case TCIFLUSH:
> +			/* no own input buffer to flush */
> +			break;
> +		case TCIOFLUSH:
> +		case TCOFLUSH:
> +			flush_send_queue(cs);
> +			break;
> +		}
> +		/* flush the serial port's buffer */
> +		rc = n_tty_ioctl(tty, file, cmd, arg);
> +		break;
> +
> +	case FIONREAD:
> +		/* unused, always return zero */
> +		val = 0;
> +		rc = put_user(val, p) ? -EFAULT : 0;

I think
		rc = put_user(val, p);
will do the same thing here.

> + * Will not be re-entered while running but other ldisc functions
> + * may be called in parallel.
> + * Can be called from hard interrupt level as well as soft interrupt
> + * level or mainline.
> + * Parameters:
> + *	tty	tty structure
> + *	buf	buffer containing received characters
> + *	cflags	buffer containing error flags for received characters (ignored)
> + *	count	number of received characters
> + */
> +static void
> +gigaset_tty_receive(struct tty_struct *tty, const unsigned char *buf,
> +		    char *cflags, int count)
> +{
> +	struct cardstate *cs = cs_get(tty);
> +	unsigned tail, head, n;
> +	struct inbuf_t *inbuf;
> +
> +	if (!cs)
> +		return;
> +	if (!(inbuf = cs->inbuf)) {
> +		dev_err(cs->dev, "%s: no inbuf\n", __func__);
> +		cs_put(cs);
> +		return;
> +	}
> +
> +	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.

> +	if (count > 0) {
> +		/* tail < head and some data left */
> +		n = head - tail - 1;
> +		if (count > n) {
> +			dev_err(cs->dev,
> +				"inbuf overflow, discarding %d bytes\n",
> +				count - n);
> +			count = n;
> +		}
> +		memcpy(inbuf->data + tail, buf, count);
> +		tail += count;
> +	}
> +
> +	gig_dbg(DEBUG_INTR, "setting tail to %u", tail);
> +	atomic_set(&inbuf->tail, tail);
> +
> +	/* Everything was received .. Push data into handler */
> +	gig_dbg(DEBUG_INTR, "%s-->BH", __func__);
> +	gigaset_schedule_event(cs);
> +	cs_put(cs);
> +}
> +

Re: [PATCH] drivers/isdn/gigaset: new M101 driver

[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?  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

Re: [PATCH] drivers/isdn/gigaset: new M101 driver

[Tilman Schmidt]

[-- Attachment #1: Type: text/plain, Size: 1597 bytes --]

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: tilman@imap.cc
Bonn, Germany
Diese Nachricht besteht zu 100% aus wiederverwerteten Bits.
Ungeöffnet mindestens haltbar bis: (siehe Rückseite)


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 253 bytes --]

Re: [PATCH] drivers/isdn/gigaset: new M101 driver

[Greg KH]

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

Re: [PATCH] drivers/isdn/gigaset: new M101 driver

[Tilman Schmidt]

[-- Attachment #1: Type: text/plain, Size: 1187 bytes --]

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: tilman@imap.cc
Bonn, Germany
Diese Nachricht besteht zu 100% aus wiederverwerteten Bits.
Ungeöffnet mindestens haltbar bis: (siehe Rückseite)


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 253 bytes --]

Re: [PATCH] drivers/isdn/gigaset: new M101 driver

[Tilman Schmidt]

[-- Attachment #1: Type: text/plain, Size: 4314 bytes --]

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 <tilman@imap.cc> 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: tilman@imap.cc
Bonn, Germany
Diese Nachricht besteht zu 100% aus wiederverwerteten Bits.
Ungeöffnet mindestens haltbar bis: (siehe Rückseite)


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 253 bytes --]

Re: [PATCH] drivers/isdn/gigaset: new M101 driver

[Andrew Morton]

On Sun, 04 Feb 2007 02:32:41 +0100 Tilman Schmidt <tilman@imap.cc> 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?

Re: [PATCH] drivers/isdn/gigaset: new M101 driver

[Tilman Schmidt]

[-- Attachment #1: Type: text/plain, Size: 3333 bytes --]

Am 04.02.2007 02:56 schrieb Andrew Morton:
> On Sun, 04 Feb 2007 02:32:41 +0100 Tilman Schmidt <tilman@imap.cc> 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@imap.cc
Bonn, Germany
Diese Nachricht besteht zu 100% aus wiederverwerteten Bits.
Ungeöffnet mindestens haltbar bis: (siehe Rückseite)


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 253 bytes --]

Re: [PATCH] drivers/isdn/gigaset: new M101 driver

[Andrew Morton]

On Mon, 05 Feb 2007 02:42:09 +0100 Tilman Schmidt <tilman@imap.cc> wrote:

> Am 04.02.2007 02:56 schrieb Andrew Morton:
> > On Sun, 04 Feb 2007 02:32:41 +0100 Tilman Schmidt <tilman@imap.cc> 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.

Re: [PATCH] drivers/isdn/gigaset: new M101 driver

[Tilman Schmidt]

[-- Attachment #1: Type: text/plain, Size: 902 bytes --]

Andrew Morton schrieb:
> On Mon, 05 Feb 2007 02:42:09 +0100 Tilman Schmidt <tilman@imap.cc> 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 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 #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 250 bytes --]