linux-kernel - Re: [PATCH] USB: add USB test and measurement class driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20080827031205.GA22912@kroah.com>
Date:	Tue, 26 Aug 2008 20:12:05 -0700
From:	Greg KH <greg@...ah.com>
To:	linux-usb@...r.kernel.org
Cc:	Stefan Kopp <stefan_kopp@...lent.com>,
	Marcel Janssen <korgull@...e.nl>,
	Felipe Balbi <me@...ipebalbi.com>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] USB: add USB test and measurement class driver

On Tue, Aug 26, 2008 at 05:05:01PM -0700, Greg KH wrote:
> I've now added this to my tree, and any review comments are greatly
> welcome.

Heck, I'll review my own patch, as I noticed I forgot some things:

> ---
>  drivers/usb/class/Kconfig  |   10 
>  drivers/usb/class/Makefile |    1 
>  drivers/usb/class/usbtmc.c | 1058 +++++++++++++++++++++++++++++++++++++++++++++
>  drivers/usb/class/usbtmc.h |   24 +
>  include/linux/usb/tmc.h    |   27 +
>  5 files changed, 1120 insertions(+)

needs Documentation/ABI description of new sysfs files

> +/*
> + * This structure is the capabilities for the device
> + * See section 4.2.1.8 of the USBTMC specification for details.
> + */
> +struct usbtmc_dev_capabilities {
> +	__u8 interface_capabilities;
> +	__u8 device_capabilities;
> +	__u8 usb488_interface_capabilities;
> +	__u8 usb488_device_capabilities;
> +};

These should match the usbif description names to make it easier to
understand.

> +	/* USBTMC devices have only one setting, so use that */
> +	iface_desc = data->intf->cur_altsetting;
> +
> +	/* Find bulk in endpoint */
> +	for (n = 0; n < iface_desc->desc.bNumEndpoints; n++) {
> +		endpoint = &iface_desc->endpoint[n].desc;
> +
> +		if (usb_endpoint_is_bulk_in(endpoint)) {
> +			data->bulk_in = endpoint->bEndpointAddress;
> +			dev_dbg(dev, "Found bulk in endpoint at %u\n",
> +				data->bulk_in);
> +			break;
> +		}
> +	}
> +
> +	/* Find bulk out endpoint */
> +	for (n = 0; n < iface_desc->desc.bNumEndpoints; n++) {
> +		endpoint = &iface_desc->endpoint[n].desc;
> +
> +		if (usb_endpoint_is_bulk_out(endpoint)) {
> +			data->bulk_out = endpoint->bEndpointAddress;
> +			dev_dbg(dev, "Found Bulk out endpoint at %u\n",
> +				data->bulk_out);
> +			break;
> +		}
> +	}

This can all be done at probe() time, no need to wait until open().

> +static ssize_t usbtmc_write(struct file *filp, const char __user *buf,
> +			    size_t count, loff_t *f_pos)
> +{
> +	struct usbtmc_device_data *data;
> +	u8 *buffer;
> +	int retval;
> +	int actual;
> +	unsigned long int n_bytes;
> +	int n;
> +	int remaining;
> +	int done;
> +	int this_part;
> +
> +	data = filp->private_data;
> +	buffer = data->buffer;

This buffer should be local to the function, and not the device as we
could overlap reads and writes.  It should be removed from the device
itself to make sure everything is correct.  The ioctls already have
their own local buffers, fixing a bug in the original driver which used
1! buffer for all devices and all commands...

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/