(I am still digesting your feedback and thinking about some of your questions; these are preliminary thoughts.) On Thu, Apr 14, 2011 at 10:16:57AM +0100, Alan Cox wrote: > 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 ? Does a change to the README like so answer your question? To use this device, you will need: 1) This driver. 2) A userspace to use the interface(s) presented by this driver. ModemManager (http://cgit.freedesktop.org/ModemManager/ModemManager/) can do this to some extent; Qualcomm has a closed-source SDK, available under restrictive license terms, that supports all features of the device. 3) A firmware loader. This device comes up 'dumb' (that is, with no firmware on it). Qualcomm ships a firmware loader as part of their SDK, and Matthew Garrett has written an open-source one: http://www.codon.org.uk/~mjg59/gobi_loader/ 4) A firmware image. These are copyright Qualcomm and not redistributable. Existing OEMs of Gobi modems might make these available on their websites. > 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 ?) Oops. > > + status = qc_register(dev); > > + if (status) { > > + qc_deregister(dev); > > If this falls through is fiddling with addr and dev-> stuff safe ? Probably not, no. > > + } 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 ? No. Is it important that it be? (I've only tested this driver on Arm and x86). If it is important, is there an idiomatic way to do this? > > + 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 ????? I feel embarassed about having missed these. This driver was (I suspect) ported over from Windows and the original Qualcomm author stuck these in. Removing them doesn't seem to hurt anything. > > +#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. These ioctl numbers have to stay the same in order to remain compatible with a closed userspace SDK that we can't change. > > + 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 Doh. > > + if (!device_valid(dev)) { > > + DBG("Invalid device!\n"); > > + return -ENXIO; > > + } > > So what locking here ensures dev stays valid ? Hrm. Not much, as it turns out.This ought to be reparable by taking a lock on the device for the entire function body. > > + result = down_interruptible(&sem); > > Really wants to be a mutex so it'll work with the realtime stuff I don't understand this comment. Is there a document I should have read? > > + 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 Again, not much. It seems I will need a lock on the whole device. > > + 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 ? We set the handle to NULL inside IOCTL_QMI_CLOSE. Userspace could invoke that ioctl during the body of this function, though, and we'd be really sad. Also fixable with a per-device lock, I think. > > + > > + 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 ? Oh, excellent catch! Does a per-device lock on the entire device (and an accompanying flattening of 'struct qmidev' into 'struct qcusbnet', and replacement of qmidev's clients_lock with a device lock) seem like a good solution to the concurrency problems here? -- Elly -- 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
