Re: [PATCH 09/10] ieee802154: add serial dongle driver
flat view
From: Alan Cox <hidden>
Date: 2009-06-01 15:26:38
Also in:
lkml, netdev
quoted hunk ↗ jump to hunk
+ zbdev->pending_data = kzalloc(zbdev->pending_size, GFP_KERNEL); + if (!zbdev->pending_data) { + printk(KERN_ERR "%s(): unable to allocate memory\n", __func__); + zbdev->pending_id = 0; + zbdev->pending_size = 0; + return -ENOMEM; + } + memcpy(zbdev->pending_data, buf, len); + + return _send_pending_data(zbdev);
Where do you check that the tty has enough space ?
quoted hunk ↗ jump to hunk
+ case STATE_WAIT_COMMAND: + if (is_command(c)) { + zbdev->id = c; + zbdev->state = STATE_WAIT_PARAM1; + } else { + cleanup(zbdev); + printk(KERN_ERR "%s, unexpected command id: %x\n", __func__, c);
In all these ERR cases what stops a remote device from having fun spewing garbage at you and filling the log ?
quoted hunk ↗ jump to hunk
+ * channels 0-10 are not valid for us */ + BUG_ON(channel < 11 || channel > 26); + /* ... but our crappy firmware numbers channels from 1 to 16 + * which is a mystery. We suould enforce that using PIB API + * but additional checking here won't kill, and gcc will + * optimize this stuff anyway. */ + BUG_ON((channel - 10) < 1 && (channel - 10) > 16);
Shouldn't be driver specific hacks in the ldisc surely - or is the ldisc only applicable to a specific single bit of hardware ?
quoted hunk ↗ jump to hunk
+ minor = tty->index + tty->driver->minor_start; + zbdev->dev->parent = class_find_device(tty_class, NULL, &minor, dev_minor_match); +
That sort of thing shouldn't be buried in the depths of a driver. I'm not entirely averse to that sort of thing but it belongs in a helper in the tty layer.
+ zbdev->tty = tty;
Refcounting ?
quoted hunk ↗ jump to hunk
+ cleanup(zbdev); + + tty->disc_data = zbdev; + tty->receive_room = MAX_DATA_SIZE; + tty->low_latency = 1;
You can't go around mashing driver internal values - many drivers don't support low_latency mode and this will make them crash.
quoted hunk ↗ jump to hunk
+ + /* FIXME: why is this needed. Note don't use ldisc_ref here as the + open path is before the ldisc is referencable */ +
[Because otherwise after a SET_LDISC there may be bits from the old stuff left over - this one will go away as I finish the ldisc switching rewrite that is in the ttydev tree]
quoted hunk ↗ jump to hunk
+/* + * Called when the tty is put into another line discipline or it hangs up. We + * have to wait for any cpu currently executing in any of the other zb_tty_* + * routines to finish before we can call zb_tty_close and free the + * zb_serial_dev struct. This routine must be called from process context, not + * interrupt or softirq context. + */ +static void +ieee802154_tty_close(struct tty_struct *tty) +{ + struct zb_device *zbdev; + + zbdev = tty->disc_data; + if (NULL == zbdev) { + printk(KERN_WARNING "%s: match is not found\n", __func__); + return; + } + + tty->disc_data = NULL; + zbdev->tty = NULL;
Again you want a refcount on these I suspect. You may actually be safe anyway but it would be cleaner to take/drop refs.
quoted hunk ↗ jump to hunk
+static int +ieee802154_tty_ioctl(struct tty_struct *tty, struct file *file, unsigned int cmd, unsigned long arg) +{ + struct zb_device *zbdev; + struct ifreq ifr; + int err; + void __user *argp = (void __user *) arg; + + pr_debug("cmd = 0x%x\n", cmd); + memset(&ifr, 0, sizeof(ifr)); + + zbdev = tty->disc_data; + if (NULL == zbdev) { + pr_debug("match is not found\n"); + return -EINVAL; + }
If that is NULL it's a serious bug so WARN on it I think
quoted hunk ↗ jump to hunk
+ default: + pr_debug("Unknown ioctl cmd: %u\n", cmd); + return -EINVAL;
This will break default ioctl processing. You probably want to call into some of the generic handlers at this point depending upon your hardware. Either way -EINVAL is wrong.
+ } + return 0;
Unreachable
quoted hunk ↗ jump to hunk
+static void +ieee802154_tty_receive(struct tty_struct *tty, const unsigned char *buf, char *cflags, int count) +{ + struct zb_device *zbdev; + int i; + + /* Debug info */ + printk(KERN_INFO "%lu %s, received %d bytes:", jiffies, __func__, count); + for (i = 0; i < count; ++i) + printk(KERN_CONT " 0x%02X", buf[i]); + printk(KERN_CONT "\n");
I don't think the above is meant to be there....