Date:	Sun, 15 Jan 2012 14:36:29 +0100
From:	Oliver Neukum <oliver@...kum.org>
To:	Bjørn Mork <bjorn@...k.no>
Cc:	netdev@...r.kernel.org, linux-usb@...r.kernel.org,
	Dan Williams <dcbw@...hat.com>,
	Thomas Schäfer <tschaefer@...nline.de>
Subject: Re: [PATCH v2 2/3] net: usb: cdc_enc: New driver exposing USB CDC encapsulated protocols

Am Sonntag, 15. Januar 2012, 07:40:36 schrieb Bjørn Mork:
> USB CDC Ethernet devices may encapsulate vendor specific protocols inside
> USB_CDC_SEND_ENCAPSULATED_COMMAND +USB_CDC_GET_ENCAPSULATED_RESPONSE
> control messages. Devices based on Qualcomm MSM
> chipsets are known to use this for exporting the Qualcomm
> MSM Interface (QMI) protocol. Examples of such devices are
> the Huawei E392 and E398 LTE modems.

The problem is that this is another version of the code already
in cdc-wdm, which is tested well.
I'd really like to unify this stuff.

> +static ssize_t cdc_enc_fops_read(struct file *file, char __user *buf, size_t size, loff_t *pos)
> +{
> +	struct cdc_enc_client *client = file->private_data;
> +	int ret = -EFAULT;
> +
> +	if (!client || !client->cdc_enc)
> +		return -ENODEV;
> +
> +	while (!test_bit(CDC_ENC_CLIENT_RX, &client->flags)) {/* no data */
> +		if (file->f_flags & O_NONBLOCK)
> +			return -EAGAIN;
> +
> +		if (wait_for_completion_interruptible(&client->ready) < 0)
> +			return -EINTR;
> +
> +		/* shutdown requested? */
> +		if (test_bit(CDC_ENC_CLIENT_SHUTDOWN, &client->flags))
> +			return -ENXIO;
> +	}
> +
> +	/* someone else is using our buffer */
> +	if (test_and_set_bit(CDC_ENC_CLIENT_BUSY, &client->flags))
> +		return -ERESTARTSYS;
> +
> +	/* must read a complete packet */
> +	if (client->len > size || copy_to_user(buf, client->buf, client->len)) {

This is not nice at all.

> +		ret = -EFAULT;
> +		goto err;
> +	}
> +	ret = client->len;
> +
> +err:
> +	/* order is important! */
> +	clear_bit(CDC_ENC_CLIENT_RX, &client->flags);
> +	clear_bit(CDC_ENC_CLIENT_BUSY, &client->flags);

If order is important here, you need write barriers.
And read barriers at another place.

> +	return ret;
> +}
> +
> +static ssize_t cdc_enc_fops_write(struct file *file, const char __user *buf, size_t size, loff_t *pos)
> +{
> +	struct cdc_enc_client *client = file->private_data;
> +	int ret = -EFAULT;
> +
> +	if (!client || !client->cdc_enc)
> +		return -ENODEV;
> +
> +	/* shutdown requested? */
> +	if (test_bit(CDC_ENC_CLIENT_SHUTDOWN, &client->flags))
> +		return -ENXIO;
> +
> +	/* no meaning in attempting to send an incomplete packet */
> +	if (size > sizeof(client->tx_buf))
> +		return -EFAULT;
> +
> +	/* are someone else using our buffer? */
> +	if (test_and_set_bit(CDC_ENC_CLIENT_TX, &client->flags))
> +		return -ERESTARTSYS;
> +
> +	if (size > sizeof(client->tx_buf) || copy_from_user(client->tx_buf, buf, size))
> +		goto err;
> +
> +	/* send to the device */
> +	ret = cdc_enc_send_sync(client, client->tx_buf, size);
> +	if (ret < 0)
> +		return -EFAULT;

Error handling is shot. The device is now eternally busy.

> +static int cdc_enc_fops_open(struct inode *inode, struct file *file)
> +{
> +	struct cdc_enc_state *cdc_enc;
> +	struct cdc_enc_client *client;
> +
> +	/* associate the file with our backing CDC_ENC device */
> +	cdc_enc = container_of(inode->i_cdev, struct cdc_enc_state, cdev);
> +	if (!cdc_enc)
> +		return -ENODEV;
> +
> +	/* don't allow interface to sleep while we are using it */
> +	usb_autopm_get_interface(cdc_enc->intf);

This can fail.

> +	/* enable status URB even if networking is down */
> +	cdc_enc_status_urb(cdc_enc, GFP_KERNEL);
> +
> +	/* set up a ring buffer to receive our readable data? */
> +	client = cdc_enc_add_client(cdc_enc, cdc_enc_newdata_rcvd);
> +
> +	if (!client)
> +		return -ENOMEM;

Error handling. The device now can never again autosuspend.

> +
> +	file->private_data = client;
> +	return 0;
> +}

[..]
> +/* disable and free a CDC_ENC state device */
> +int cdc_enc_free_one(struct cdc_enc_state *cdc_enc)
> +{
> +	struct cdc_enc_client *tmp;
> +
> +	/* kill any pending recv urb */
> +	cdc_enc_kill_readurb(cdc_enc);
> +
> +	/* wait for all clients to exit first ... */
> +	list_for_each_entry(tmp, &cdc_enc->clients, list) {
> +		dev_dbg(&cdc_enc->intf->dev, "waiting for client %p to die\n", tmp);
> +		set_bit(CDC_ENC_CLIENT_SHUTDOWN, &tmp->flags);
> +		complete(&tmp->ready);
> +	}
> +	wait_event_interruptible(cdc_enc->waitq, list_empty(&cdc_enc->clients));

If you wait interruptably, you need to handle being interrupted.

> +
> +/* per CDC_ENC interface state */
> +struct cdc_enc_state {
> +	struct usb_interface *intf;
> +	struct urb *urb;		/* receive urb */
> +	struct urb *interrupt;		/* interrupt urb */
> +	unsigned char rcvbuf[CDC_ENC_BUFLEN];     /* receive buffer */

This is a violation of the DMA requirements. You must use kmalloc.

I had a not very detailed look at this driver, but I would really prefer
if you could have a look at making unified code for this, cdc-wdm and cdc-acm.

	Regards
		Oliver
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists