Comments inline for the most part - there doesn't seem to be a clear explained locking model, there are lots of tests in random places with no apparent locking to make the tests meaningful and there are some other obvious prblems (missing NULL checks etc). > +the device speaks a more complex set of protocols embodied in the closed-source > +Gobi SDK which are necessary to use advanced features of the device. > +Furthermore, firmware (also proprietary) is needed to make the device useful. An > +open-source firmware loader exists: http://www.codon.org.uk/~mjg59/gobi_loader/. So what bits of it actually work without the non-free code ? and what bits of the driver are only relevant to the non-free code. Ie what bits should be submitted ? On the the whole looks like a good basis but the locking really needs to be much more clearly defined for all these validity/dying type checks. > + dev = kmalloc(sizeof(struct qcusbnet), GFP_KERNEL); > + if (!dev) { > + DBG("failed to allocate device buffers\n"); > + return -ENOMEM; Seems to leak dev if this works and the next if case fails ? (Also why not just put them all in one struct given they have the same object lifetime ?) > + status = qc_register(dev); > + if (status) { > + qc_deregister(dev); If this falls through is fiddling with addr and dev-> stuff safe ? > + } else { > + mutex_lock(&qcusbnet_lock); > + /* Give our initial ref to the list */ > + list_add(&dev->node, &qcusbnet_list); > + mutex_unlock(&qcusbnet_lock); > + } > + /* After calling qc_register, MEID is valid */ > + addr = &usbnet->net->dev_addr[0]; > + for (i = 0; i < 6; i++) > + addr[i] = (nibble(dev->meid[i*2+2]) << 4)+ > + nibble(dev->meid[i*2+3]); > + addr[0] &= 0xfe; /* clear multicast bit */ > + addr[0] |= 0x02; /* set local assignment bit (IEEE802) */ > + > + for (pos = 4; pos + 3 < msgsize; pos += msize + 3) { > + msize = *(u16 *)(msg + pos + 1); Is this always guaranteed to be aligned ? > + if (tlv_get(msg, size, 2, &tlv[0], 4) == 4) { > + if (*(u16 *)&tlv[0] != 0) Ditto the others of this form > +#define true 1 > +#define false 0 > + > +#define ENOMEM 12 > +#define EFAULT 14 > +#define EINVAL 22 > +#define ENOMSG 42 > +#define ENODATA 61 WTF ????? > +#define IOCTL_QMI_GET_SERVICE_FILE (0x8BE0 + 1) > +#define IOCTL_QMI_GET_DEVICE_VIDPID (0x8BE0 + 2) > +#define IOCTL_QMI_GET_DEVICE_MEID (0x8BE0 + 3) > +#define IOCTL_QMI_CLOSE (0x8BE0 + 4) > +#define CDC_GET_ENCAPSULATED_RESPONSE 0x01A1ll > +#define CDC_CONNECTION_SPEED_CHANGE 0x08000000002AA1ll We have IOR/IOW/etc macros for building ioctl numbers and it should be using those. To start with that helps enormously for trace tools as it shows the directions and sizes in the ioctl code > + if (client->cid == cid || (client->cid | 0xff00) == cid) { > + copy = kmalloc(size, GFP_ATOMIC); > + memcpy(copy, data, size); copy == NULL, memcpy over NULL whoops bang splat > + if (!device_valid(dev)) { > + DBG("Invalid device!\n"); > + return -ENXIO; > + } So what locking here ensures dev stays valid ? > + result = down_interruptible(&sem); Really wants to be a mutex so it'll work with the realtime stuff > + if (handle->dev->dying) { > + DBG("Dying device"); > + return -ENXIO; > + } > + > + if (!device_valid(handle->dev)) { > + DBG("Invalid device!\n"); > + return -ENXIO; > + } So like validity - what lock stops it dying mid ioctl > + struct qmihandle *handle = (struct qmihandle *)file->private_data; Lots of casts that are not needed > +static ssize_t devqmi_write(struct file *file, const char __user * buf, > + size_t size, loff_t *pos) > +{ > + int status; > + void *wbuf; > + struct qmihandle *handle = (struct qmihandle *)file->private_data; > + > + if (!handle) { > + DBG("Bad file data\n"); > + return -EBADF; > + } Exactly how can this check occur - why is it present, and if it is relevant what locks prevent whatever it checks from occuring during the execution ? > + > + if (!device_valid(handle->dev)) { > + DBG("Invalid device! Updating f_ops\n"); > + file->f_op = file->f_dentry->d_inode->i_fop; > + return -ENXIO; > + } > + > + if (handle->cid == (u16)-1) { > + DBG("Client ID must be set before writing 0x%04X\n", > + handle->cid); > + return -EBADR; > + } > + > + wbuf = kmalloc(size + qmux_size, GFP_KERNEL); Maths overflow ? -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
