Re: [PATCH 09/10] ieee802154: add serial dongle driver

Alan Cox
2009-06-01T15:26:38Z

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

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

>
> + * 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 ?

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

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

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

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

> +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

> + 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

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