On Thu, 1 Feb 2007 22:12:24 +0100
Tilman Schmidt [off-list ref] 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?

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);
+}
+