Login | Register For Free | Help
Search for: (Advanced)

Mailing List Archive: Linux: Kernel

[PATCH] add new NRP power meter USB device driver

 

 

First page Previous page 1 2 Next page Last page  View All Linux kernel RSS feed   Index | Next | Previous | View Threaded


stefani at seibold

May 29, 2012, 12:14 PM

Post #1 of 28 (670 views)
Permalink
[PATCH] add new NRP power meter USB device driver

From: Stefani Seibold <stefani [at] seibold>

This driver supports all of the Rohde&Schwarz RF Power Meter NRP Sensors. These
sensors are intelligent standalone instruments that communicate via USB.

A power meter is a device for analyzing the RF power output level of an
electrical device, similar to an oscilloscope.

The Power Meter Sensors will be used in a wide range of environements, like

- Manufacturing (e.g. air planes and smart phones)
- Radio and TV broadcasting
- Mobile communications
- Engeeniering
- Science Labs
- Education

The NRP Power Meters support the following measurements:

- Dynamic range: up to 90 dB (sensor dependent)
- Level range: -67 dBm to +45 dBm (sensor dependent)
- Frequency range: DC to 67 GHz (sensor dependent)
- Measurement speed: 1500 measurements per second in the buffered mode
- Linearity uncertainty: 0.007 dB
- Precise average power measurements irrespective of modulation and bandwidth
- Flexible measurements on up to 128 time slots per power sensor
- S parameter correction of components between sensor and test object

The device will be controlled by a SCPI like interface.

The patch is against linux 3.5.0
(commit 1e2aec873ad6d16538512dbb96853caa1fa076af)

The source is checked with checkpatch.pl and has no errors. Only 11
"line over 80 characters" warnings are left. I see no reason to satisfy this
boring punch card limit for this lines, since it will make the source noisier.

make C=1 is quiet.

Please apply it to the kernel tree.
Thanks.

Signed-off-by: Stefani Seibold <stefani [at] seibold>
---
drivers/usb/misc/Kconfig | 12 +
drivers/usb/misc/Makefile | 1 +
drivers/usb/misc/nrpz.c | 1069 ++++++++++++++++++++++++++++++++++++++++
include/linux/usb/nrpzmodule.h | 47 ++
4 files changed, 1129 insertions(+), 0 deletions(-)
create mode 100644 drivers/usb/misc/nrpz.c
create mode 100644 include/linux/usb/nrpzmodule.h

diff --git a/drivers/usb/misc/Kconfig b/drivers/usb/misc/Kconfig
index 1bfcd02..2c55d5f 100644
--- a/drivers/usb/misc/Kconfig
+++ b/drivers/usb/misc/Kconfig
@@ -244,3 +244,15 @@ config USB_YUREX
To compile this driver as a module, choose M here: the
module will be called yurex.

+config USB_NRPZ
+ tristate "USB NRPZ power sensor driver support"
+ depends on USB
+ help
+ This driver supports the Rohde&Schwarz NRP RF Power Meter Sensors.
+
+ These sensors are intelligent standalone instruments that
+ communicate via USB and provide a wide range of measurements.
+
+ To compile this driver as a module, choose M here: the
+ module will be called nrpz.
+
diff --git a/drivers/usb/misc/Makefile b/drivers/usb/misc/Makefile
index 796ce7e..9a0364c 100644
--- a/drivers/usb/misc/Makefile
+++ b/drivers/usb/misc/Makefile
@@ -25,5 +25,6 @@ obj-$(CONFIG_USB_TRANCEVIBRATOR) += trancevibrator.o
obj-$(CONFIG_USB_USS720) += uss720.o
obj-$(CONFIG_USB_SEVSEG) += usbsevseg.o
obj-$(CONFIG_USB_YUREX) += yurex.o
+obj-$(CONFIG_USB_NRPZ) += nrpz.o

obj-$(CONFIG_USB_SISUSBVGA) += sisusbvga/
diff --git a/drivers/usb/misc/nrpz.c b/drivers/usb/misc/nrpz.c
new file mode 100644
index 0000000..e341703
--- /dev/null
+++ b/drivers/usb/misc/nrpz.c
@@ -0,0 +1,1069 @@
+/*
+ * Rohde & Schwarz USB NRP Zxx power meter kernel module driver
+ *
+ * Version: 4.0
+ *
+ * Copyright (c) 2012 by Rohde & Scharz GmbH & Co. KG
+ * written by Stefani Seibold <stefani [at] seibold>
+ *
+ * Based on USB Skeleton driver written by Greg Kroah-Hartman <greg [at] kroah>
+ *
+ * This driver supports the RF Power Meter R&S ® NRP Sensors. These
+ * sensors are intelligent standalone instruments that communicate via USB
+ * and support the following measurements:
+ *
+ * - Dynamic range: up to 90 dB (sensor dependent)
+ * - Level range: -67 dBm to +45 dBm (sensor dependent)
+ * - Frequency range: DC to 67 GHz (sensor dependent)
+ * - Measurement speed: 1500 measurements per second in the buffered mode
+ * - Linearity uncertainty: 0.007 dB
+ * - Precise average power measurements irrespective of modulation and bandwidth
+ * - Flexible measurements on up to 128 time slots per power sensor
+ * - S parameter correction of components between sensor and test object
+ *
+ * History:
+ *
+ * 2012_05_18 - 4.0 - revamp for kernel inclusion
+ * 2007_05_15 - 2.0 - Ported the driver to kernel 2.6, mostly rewritten
+ * 2001_05_01 - 0.1 - first version
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
+ */
+
+/*
+ * Internal format of the NRPZ USB messages:
+ *
+ * Byte 0: Signature
+ * Byte 1: Error Code
+ * Byte 2/3: Array Index
+ * or State (Byte 2)
+ * or Group Number (Byte 2) / Param Number (Byte 3)
+ * Byte 4-15: Data depening on signature type (Byte 0):
+ * floats, bit fields and integer are 32 bit
+ *
+ * Signature types:
+ * 'E': single float value
+ * 'e': single value
+ * 'A': float array
+ * 'P': auxiliary or peak float array
+ * 'a': interim float array
+ * 'p': interim auxiliary or peak float array
+ * 'F': feature bit field
+ * 'U': float parameter (32 bit)
+ * 'V': bit field parameter (32 bit)
+ * 'W': integer parameter (32 bit)
+ * 'T': string data
+ * 'R': receipt data (string or binary data)
+ * 'X': internal error
+ * 'Z': current state (Byte 2)
+ * 'z': life sign package
+ * 'L': float value (32 bit)
+ * 'M': bit field value (32 bit)
+ * 'N': long value (32 bit)
+ * 'B': binary data
+ *
+ * State values:
+ * 0: trigger idle
+ * 1: trigger reserved
+ * 2: wait for trigger
+ * 3: trigger measuring
+ *
+ * Communication in direction from host to the device are mostly like SCPI.
+ */
+
+#include <linux/kernel.h>
+#include <linux/sched.h>
+#include <linux/signal.h>
+#include <linux/errno.h>
+#include <linux/poll.h>
+#include <linux/init.h>
+#include <linux/slab.h>
+#include <linux/fcntl.h>
+#include <linux/module.h>
+#include <linux/spinlock.h>
+#include <linux/list.h>
+#include <linux/usb.h>
+#include <linux/version.h>
+#include <linux/kref.h>
+#include <linux/bitops.h>
+#include <linux/mutex.h>
+
+#include <linux/usb/nrpzmodule.h>
+
+#if 0
+#define CONFIG_NRP_DEBUG
+#endif
+#ifdef CONFIG_NRP_DEBUG
+static int debug = 1;
+#else
+static int debug;
+#endif
+
+module_param(debug, int, 0);
+MODULE_PARM_DESC(debug, "Debug enabled or not");
+
+#define nrpz_dbg(format, arg...) \
+ do { if (debug) printk(KERN_DEBUG "nrpz: " format "\n", ##arg); } while (0)
+#define nrpz_err(format, arg...) \
+ do { printk(KERN_ERR "nrpz: " format "\n", ##arg); } while (0)
+#define nrpz_info(format, arg...) \
+ do { printk(KERN_INFO "nrpz: " format "\n", ##arg); } while (0)
+
+/* Version Information */
+#define DRIVER_VERSION "v4.00"
+#define DRIVER_AUTHOR "Stefani Seibold <stefani [at] seibold>"
+#define DRIVER_DESC "Rohde&Schwarz NRP-Zxx USB Powermeter"
+
+/* Get a minor range for your devices from the usb maintainer */
+#define NRPZ_MINOR_BASE 192
+
+#define to_nrpz_dev(d) container_of(d, struct usb_nrpz, kref)
+#define list_to_urb(d) container_of(d, struct urb, urb_list)
+
+/* Define these values to match your device */
+#define USB_RS_VENDOR_ID 0x0aad
+#define USB_NRP_PRODUCT_ID 0x0002
+#define USB_NRPZ21_PRODUCT_ID 0x0003
+#define USB_NRPFU_PRODUCT_ID 0x0004
+#define USB_FSHZ1_PRODUCT_ID 0x000b
+#define USB_NRPZ11_PRODUCT_ID 0x000c
+#define USB_NRPZ22_PRODUCT_ID 0x0013
+#define USB_NRPZ23_PRODUCT_ID 0x0014
+#define USB_NRPZ24_PRODUCT_ID 0x0015
+#define USB_NRPZ51_PRODUCT_ID 0x0016
+#define USB_NRPZ52_PRODUCT_ID 0x0017
+#define USB_NRPZ55_PRODUCT_ID 0x0018
+#define USB_NRPZ56_PRODUCT_ID 0x0019
+#define USB_FSHZ18_PRODUCT_ID 0x001a
+#define USB_NRPZ91_PRODUCT_ID 0x0021
+#define USB_NRPZ81_PRODUCT_ID 0x0023
+#define USB_NRPZ31_PRODUCT_ID 0x002c
+#define USB_NRPZ37_PRODUCT_ID 0x002d
+#define USB_NRPZ96_PRODUCT_ID 0x002e
+#define USB_NRPZ27_PRODUCT_ID 0x002f
+#define USB_NRPZ28_PRODUCT_ID 0x0051
+#define USB_NRPZ98_PRODUCT_ID 0x0052
+#define USB_NRPZ92_PRODUCT_ID 0x0062
+#define USB_NRPZ57_PRODUCT_ID 0x0070
+#define USB_NRPZ85_PRODUCT_ID 0x0083
+#define USB_NRPC40_PRODUCT_ID 0x008F
+#define USB_NRPC50_PRODUCT_ID 0x0090
+#define USB_NRPZ86_PRODUCT_ID 0x0095
+#define USB_NRPZ41_PRODUCT_ID 0x0096
+#define USB_NRPZ61_PRODUCT_ID 0x0097
+#define USB_NRPZ71_PRODUCT_ID 0x0098
+#define USB_NRPZ32_PRODUCT_ID 0x009A
+#define USB_NRPZ211_PRODUCT_ID 0x00A6
+#define USB_NRPZ221_PRODUCT_ID 0x00A7
+#define USB_NRPZ58_PRODUCT_ID 0x00A8
+#define USB_NRPC33_PRODUCT_ID 0x00B6
+#define USB_NRPC18_PRODUCT_ID 0x00BF
+
+/* table of devices that work with this driver */
+static struct usb_device_id nrpz_table[] = {
+ {USB_DEVICE(USB_RS_VENDOR_ID, USB_NRP_PRODUCT_ID)},
+ {USB_DEVICE(USB_RS_VENDOR_ID, USB_NRPZ21_PRODUCT_ID)},
+ {USB_DEVICE(USB_RS_VENDOR_ID, USB_NRPFU_PRODUCT_ID)},
+ {USB_DEVICE(USB_RS_VENDOR_ID, USB_FSHZ1_PRODUCT_ID)},
+ {USB_DEVICE(USB_RS_VENDOR_ID, USB_NRPZ11_PRODUCT_ID)},
+ {USB_DEVICE(USB_RS_VENDOR_ID, USB_NRPZ22_PRODUCT_ID)},
+ {USB_DEVICE(USB_RS_VENDOR_ID, USB_NRPZ23_PRODUCT_ID)},
+ {USB_DEVICE(USB_RS_VENDOR_ID, USB_NRPZ24_PRODUCT_ID)},
+ {USB_DEVICE(USB_RS_VENDOR_ID, USB_NRPZ51_PRODUCT_ID)},
+ {USB_DEVICE(USB_RS_VENDOR_ID, USB_NRPZ52_PRODUCT_ID)},
+ {USB_DEVICE(USB_RS_VENDOR_ID, USB_NRPZ55_PRODUCT_ID)},
+ {USB_DEVICE(USB_RS_VENDOR_ID, USB_NRPZ56_PRODUCT_ID)},
+ {USB_DEVICE(USB_RS_VENDOR_ID, USB_NRPZ57_PRODUCT_ID)},
+ {USB_DEVICE(USB_RS_VENDOR_ID, USB_FSHZ18_PRODUCT_ID)},
+ {USB_DEVICE(USB_RS_VENDOR_ID, USB_NRPZ91_PRODUCT_ID)},
+ {USB_DEVICE(USB_RS_VENDOR_ID, USB_NRPZ81_PRODUCT_ID)},
+ {USB_DEVICE(USB_RS_VENDOR_ID, USB_NRPZ31_PRODUCT_ID)},
+ {USB_DEVICE(USB_RS_VENDOR_ID, USB_NRPZ37_PRODUCT_ID)},
+ {USB_DEVICE(USB_RS_VENDOR_ID, USB_NRPZ96_PRODUCT_ID)},
+ {USB_DEVICE(USB_RS_VENDOR_ID, USB_NRPZ27_PRODUCT_ID)},
+ {USB_DEVICE(USB_RS_VENDOR_ID, USB_NRPZ28_PRODUCT_ID)},
+ {USB_DEVICE(USB_RS_VENDOR_ID, USB_NRPZ98_PRODUCT_ID)},
+ {USB_DEVICE(USB_RS_VENDOR_ID, USB_NRPZ92_PRODUCT_ID)},
+ {USB_DEVICE(USB_RS_VENDOR_ID, USB_NRPZ85_PRODUCT_ID)},
+ {USB_DEVICE(USB_RS_VENDOR_ID, USB_NRPC40_PRODUCT_ID)},
+ {USB_DEVICE(USB_RS_VENDOR_ID, USB_NRPC50_PRODUCT_ID)},
+ {USB_DEVICE(USB_RS_VENDOR_ID, USB_NRPZ86_PRODUCT_ID)},
+ {USB_DEVICE(USB_RS_VENDOR_ID, USB_NRPZ41_PRODUCT_ID)},
+ {USB_DEVICE(USB_RS_VENDOR_ID, USB_NRPZ61_PRODUCT_ID)},
+ {USB_DEVICE(USB_RS_VENDOR_ID, USB_NRPZ71_PRODUCT_ID)},
+ {USB_DEVICE(USB_RS_VENDOR_ID, USB_NRPZ32_PRODUCT_ID)},
+ {USB_DEVICE(USB_RS_VENDOR_ID, USB_NRPZ211_PRODUCT_ID)},
+ {USB_DEVICE(USB_RS_VENDOR_ID, USB_NRPZ221_PRODUCT_ID)},
+ {USB_DEVICE(USB_RS_VENDOR_ID, USB_NRPZ58_PRODUCT_ID)},
+ {USB_DEVICE(USB_RS_VENDOR_ID, USB_NRPC33_PRODUCT_ID)},
+ {USB_DEVICE(USB_RS_VENDOR_ID, USB_NRPC18_PRODUCT_ID)},
+ {} /* Terminating entry */
+};
+
+MODULE_DEVICE_TABLE(usb, nrpz_table);
+
+/* Structure to hold all of our device specific stuff */
+struct usb_nrpz {
+ struct usb_device *udev;
+ struct usb_interface *intf;
+ unsigned minor; /* minor number for this device */
+ unsigned long in_use; /* in use flag */
+
+ size_t in_size; /* size of the receive buffer */
+ size_t out_size; /* size of the send buffer */
+ __u8 in_epAddr; /* address of the bulk in endpoint */
+ __u8 out_epAddr; /* address of the bulk out endpoint */
+
+ struct kref kref;
+ wait_queue_head_t wq;
+
+ struct usb_anchor out_running; /* list of in use output buffers */
+ struct list_head out_avail; /* list of available output buffers */
+ spinlock_t write_lock; /* spinlock for transmit list */
+ struct mutex write_mutex; /* exclusive write data semaphore */
+
+ struct usb_anchor in_running; /* list of in use input buffers */
+ struct list_head in_avail; /* list of available input buffers */
+ spinlock_t read_lock; /* spinlock for receive list */
+ struct mutex read_mutex; /* exclusive read data semaphore */
+
+ struct urb out_urbs[64]; /* array of urb's for output */
+ struct urb in_urbs[64]; /* array of urb's for input */
+};
+
+/* forward declaration */
+static struct usb_driver nrpz_driver;
+
+static inline void urb_list_add(spinlock_t *lock, struct urb *urb,
+ struct list_head *head)
+{
+ spin_lock_irq(lock);
+ list_add(&urb->urb_list, head);
+ spin_unlock_irq(lock);
+}
+
+static inline void urb_list_add_tail(spinlock_t *lock, struct urb *urb,
+ struct list_head *head)
+{
+ spin_lock_irq(lock);
+ list_add_tail(&urb->urb_list, head);
+ spin_unlock_irq(lock);
+}
+
+static struct urb *urb_list_get(spinlock_t *lock, struct list_head *head)
+{
+ struct list_head *p;
+
+ spin_lock_irq(lock);
+
+ if (list_empty(head)) {
+ spin_unlock_irq(lock);
+ return NULL;
+ }
+
+ p = head->next;
+ list_del(p);
+ spin_unlock_irq(lock);
+
+ return list_to_urb(p);
+}
+
+/*
+ * bulks_release
+ *
+ * release all allocated urb's and and usb buffers
+ */
+static void bulks_release(struct usb_nrpz *dev, struct urb *urb, unsigned n,
+ int buf_size)
+{
+ while (n--) {
+ if (urb->transfer_buffer)
+ usb_free_coherent(dev->udev,
+ buf_size,
+ urb->transfer_buffer,
+ urb->transfer_dma);
+ ++urb;
+ }
+}
+
+/*
+ * bulks_init
+ *
+ * preallocate urb's and and usb buffers
+ */
+static int bulks_init(struct usb_nrpz *dev,
+ struct urb *urb,
+ unsigned n,
+ unsigned int pipe,
+ int buf_size,
+ usb_complete_t complete_fn)
+{
+ void *buffer;
+
+ while (n--) {
+ usb_init_urb(urb);
+
+ buffer = usb_alloc_coherent(dev->udev,
+ buf_size,
+ GFP_KERNEL,
+ &urb->transfer_dma);
+ if (!buffer)
+ return -ENOMEM;
+
+ /* set up our read urb */
+ usb_fill_bulk_urb(urb,
+ dev->udev,
+ pipe,
+ buffer,
+ dev->in_size,
+ complete_fn,
+ dev);
+
+ urb->transfer_flags = URB_NO_TRANSFER_DMA_MAP;
+
+ ++urb;
+ }
+ return 0;
+}
+
+static void nrpz_read_callback(struct urb *urb)
+{
+ struct usb_nrpz *dev = (struct usb_nrpz *)urb->context;
+
+ if (urb->status) {
+ if (!(urb->status == -ENOENT ||
+ urb->status == -EPIPE ||
+ urb->status == -ECONNRESET ||
+ urb->status == -ESHUTDOWN))
+ nrpz_err("Nonzero read bulk status: %d", urb->status);
+ }
+
+ spin_lock(&dev->read_lock);
+ list_add_tail(&urb->urb_list, &dev->in_avail);
+ spin_unlock(&dev->read_lock);
+ wake_up_all(&dev->wq);
+}
+
+static void nrpz_write_callback(struct urb *urb)
+{
+ struct usb_nrpz *dev = (struct usb_nrpz *)urb->context;
+
+ if (urb->status) {
+ if (!(urb->status == -ENOENT ||
+ urb->status == -EPIPE ||
+ urb->status == -ECONNRESET ||
+ urb->status == -ESHUTDOWN))
+ nrpz_err("Nonzero write bulk status: %d", urb->status);
+ }
+
+ spin_lock(&dev->write_lock);
+ list_add_tail(&urb->urb_list, &dev->out_avail);
+ spin_unlock(&dev->write_lock);
+ wake_up_all(&dev->wq);
+}
+
+static void nrpz_delete(struct kref *kref)
+{
+ struct usb_nrpz *dev = to_nrpz_dev(kref);
+
+ usb_put_dev(dev->udev);
+ kfree(dev);
+}
+
+static ssize_t nrpz_read(struct file *file, char __user *buffer, size_t count,
+ loff_t *ppos)
+{
+ struct usb_nrpz *dev;
+ int ret;
+ struct urb *urb;
+ size_t n;
+
+ dev = (struct usb_nrpz *)file->private_data;
+
+ /* verify that we actually have some data to read */
+ if (!count)
+ return 0;
+
+ /* lock the read data */
+ ret = mutex_lock_interruptible(&dev->read_mutex);
+ if (ret)
+ return ret;
+
+ for (;;) {
+ urb = urb_list_get(&dev->read_lock, &dev->in_avail);
+ if (urb)
+ break;
+
+ if (file->f_flags & O_NONBLOCK) {
+ ret = -EAGAIN;
+ goto exit;
+ }
+
+ ret = wait_event_interruptible(dev->wq,
+ !list_empty(&dev->in_avail) || !dev->intf);
+ if (ret) {
+ ret = -ERESTARTSYS;
+ goto exit;
+ }
+
+ /* verify that the device wasn't unplugged */
+ if (!dev->intf) {
+ ret = -ENODEV;
+ goto exit;
+ }
+ }
+
+ if (!urb->status) {
+ n = min(count, urb->actual_length);
+
+ if (copy_to_user(buffer, urb->transfer_buffer, n)) {
+ urb_list_add(&dev->read_lock, urb, &dev->in_avail);
+ ret = -EFAULT;
+ goto exit;
+ }
+ } else
+ n = urb->status;
+
+ usb_anchor_urb(urb, &dev->in_running);
+
+ ret = usb_submit_urb(urb, GFP_KERNEL);
+ if (ret) {
+ usb_unanchor_urb(urb);
+ urb_list_add_tail(&dev->read_lock, urb, &dev->in_avail);
+ nrpz_err("Failed submitting read urb (error %d)", ret);
+ }
+
+ ret = n;
+exit:
+ mutex_unlock(&dev->read_mutex);
+ return ret;
+}
+
+static ssize_t nrpz_write(struct file *file, const char __user *buffer,
+ size_t count, loff_t *ppos)
+{
+ struct usb_nrpz *dev;
+ int ret;
+ size_t len = 0;
+ struct urb *urb;
+ size_t n;
+
+ dev = (struct usb_nrpz *)file->private_data;
+
+ /* verify that we actually have some data to write */
+ if (!count)
+ return 0;
+
+ /* lock the write data */
+ ret = mutex_lock_interruptible(&dev->write_mutex);
+ if (ret)
+ return ret;
+
+ do {
+ for (;;) {
+ /* verify that the device wasn't unplugged */
+ if (!dev->intf) {
+ ret = -ENODEV;
+ goto exit;
+ }
+
+ urb = urb_list_get(&dev->write_lock, &dev->out_avail);
+ if (urb)
+ break;
+
+ if (file->f_flags & O_NONBLOCK) {
+ ret = -EAGAIN;
+ goto exit;
+ }
+
+ ret = wait_event_interruptible(dev->wq,
+ !list_empty(&dev->out_avail) || !dev->intf);
+ if (ret) {
+ ret = -ERESTARTSYS;
+ goto exit;
+ }
+ }
+
+ n = min(count, dev->out_size);
+
+ if (copy_from_user(urb->transfer_buffer, buffer, n)) {
+ urb_list_add(&dev->write_lock, urb, &dev->out_avail);
+ ret = -EFAULT;
+ break;
+ }
+
+ urb->transfer_buffer_length = n;
+
+ usb_anchor_urb(urb, &dev->out_running);
+
+ ret = usb_submit_urb(urb, GFP_KERNEL);
+ if (ret) {
+ usb_unanchor_urb(urb);
+ urb_list_add_tail(&dev->write_lock, urb, &dev->out_avail);
+ nrpz_err("Failed submitting write urb (error %d)", ret);
+ break;
+ }
+
+ count -= n;
+ buffer += n;
+ len += n;
+ } while (count);
+exit:
+ if (len)
+ ret = len;
+
+ mutex_unlock(&dev->write_mutex);
+ return ret;
+}
+
+#define VRT_RESET_ALL 1
+#define VRT_GET_DEVICE_INFO 6
+#define VRI_DEVICE_NAME 5
+
+static long nrpz_compat_ioctl(struct file *file, unsigned int cmd,
+ unsigned long arg)
+{
+ struct usb_nrpz *dev;
+ int ret;
+
+ dev = (struct usb_nrpz *)file->private_data;
+
+ /* verify that the device wasn't unplugged */
+ if (!dev->intf)
+ return -ENODEV;
+
+ switch (cmd) {
+ case NRPZ_GETSENSORINFO:
+ {
+ struct nrpz_sensor_info __user *sensor_info =
+ (struct nrpz_sensor_info __user *)arg;
+
+ if (!access_ok(VERIFY_WRITE, sensor_info, sizeof(*sensor_info)))
+ return -EFAULT;
+
+ __put_user(dev->udev->descriptor.bcdDevice,
+ &sensor_info->bcdDevice);
+ __put_user(dev->udev->descriptor.bcdUSB,
+ &sensor_info->bcdUSB);
+ __put_user(dev->udev->descriptor.bDescriptorType,
+ &sensor_info->bDescriptorType);
+ __put_user(dev->udev->descriptor.bDeviceClass,
+ &sensor_info->bDeviceClass);
+ __put_user(dev->udev->descriptor.bDeviceSubClass,
+ &sensor_info->bDeviceSubClass);
+ __put_user(dev->udev->descriptor.bDeviceProtocol,
+ &sensor_info->bDeviceProtocol);
+ __put_user(dev->udev->descriptor.bMaxPacketSize0,
+ &sensor_info->bMaxPacketSize0);
+ __put_user(dev->udev->descriptor.bNumConfigurations,
+ &sensor_info->bNumConfigurations);
+ __put_user(dev->udev->descriptor.iManufacturer,
+ &sensor_info->iManufacturer);
+ __put_user(dev->udev->descriptor.iProduct,
+ &sensor_info->iProduct);
+ __put_user(dev->udev->descriptor.iSerialNumber,
+ &sensor_info->iSerialNumber);
+ __put_user(dev->udev->descriptor.idVendor,
+ &sensor_info->vendorId);
+ __put_user(dev->udev->descriptor.idProduct,
+ &sensor_info->productId);
+ usb_string(dev->udev, dev->udev->descriptor.iManufacturer,
+ (char __force *)sensor_info->manufacturer,
+ sizeof(sensor_info->manufacturer));
+ usb_string(dev->udev, dev->udev->descriptor.iProduct,
+ (char __force *)sensor_info->productName,
+ sizeof(sensor_info->productName));
+ usb_string(dev->udev, dev->udev->descriptor.iSerialNumber,
+ (char __force *)sensor_info->serialNumber,
+ sizeof(sensor_info->serialNumber));
+
+ return 0;
+ }
+ case NRPZ_START:
+ {
+ u8 device_state[128];
+
+ memset(device_state, 0, sizeof(device_state));
+
+ ret = usb_control_msg(dev->udev,
+ usb_rcvctrlpipe(dev->udev, 0),
+ VRT_GET_DEVICE_INFO,
+ USB_DIR_IN | USB_TYPE_VENDOR | USB_RECIP_DEVICE,
+ 0,
+ VRI_DEVICE_NAME,
+ device_state,
+ sizeof(device_state),
+ 5000);
+
+ if (ret < 0)
+ return ret;
+
+ nrpz_dbg("device state:%s", device_state);
+
+ if (strncmp(device_state, "Boot ", 5))
+ return 0;
+
+ return usb_control_msg(dev->udev,
+ usb_sndctrlpipe(dev->udev, 0),
+ VRT_RESET_ALL,
+ USB_DIR_OUT | USB_TYPE_VENDOR | USB_RECIP_DEVICE,
+ 1,
+ 1,
+ device_state,
+ sizeof(device_state),
+ 5000);
+ }
+ case NRPZ_WRITE_DONE:
+ if (arg) {
+ ret = wait_event_interruptible_timeout(
+ dev->out_running.wait,
+ list_empty(&dev->out_running.urb_list),
+ msecs_to_jiffies(arg));
+ if (!ret)
+ return -ETIMEDOUT;
+ if (ret < 0)
+ return ret;
+ return 0;
+ } else {
+ return wait_event_interruptible(
+ dev->out_running.wait,
+ list_empty(&dev->out_running.urb_list));
+ }
+ break;
+ case NRPZ_VENDOR_CONTROL_MSG_OUT:
+ {
+ struct nrpz_control_req ncr;
+ u16 size;
+
+ if (copy_from_user(&ncr, (struct nrpz_control_req __user *)arg, sizeof(ncr)))
+ return -EFAULT;
+
+ if (ncr.data) {
+ size = ncr.size;
+
+ if (!access_ok(VERIFY_WRITE, (void __user *)ncr.data, size))
+ return -EFAULT;
+ } else {
+ size = 0;
+ }
+
+ ret = usb_control_msg(dev->udev,
+ usb_sndctrlpipe(dev->udev, 0),
+ ncr.request,
+ ncr.type,
+ ncr.value,
+ ncr.index,
+ ncr.data,
+ size,
+ 0);
+
+ return ret;
+ }
+ case NRPZ_VENDOR_CONTROL_MSG_IN:
+ {
+ struct nrpz_control_req ncr;
+ u16 size;
+
+ if (copy_from_user(&ncr, (struct nrpz_control_req __user *)arg, sizeof(ncr)))
+ return -EFAULT;
+
+ if (ncr.data) {
+ size = ncr.size;
+
+ if (!access_ok(VERIFY_READ, (void __user *)ncr.data, size))
+ return -EFAULT;
+ } else {
+ size = 0;
+ }
+
+ ret = usb_control_msg(dev->udev,
+ usb_rcvctrlpipe(dev->udev, 0),
+ ncr.request,
+ ncr.type,
+ ncr.value,
+ ncr.index,
+ ncr.data,
+ size,
+ 0);
+
+ return ret;
+ }
+ default:
+ nrpz_err("Invalid ioctl call (%08x)", cmd);
+ return -EOPNOTSUPP;
+ }
+
+ return ret;
+}
+
+static long nrpz_ioctl(struct file *file, unsigned int cmd,
+ unsigned long arg)
+{
+ return nrpz_compat_ioctl(file, cmd, arg);
+}
+
+static int nrpz_open(struct inode *inode, struct file *file)
+{
+ struct usb_nrpz *dev;
+ int minor;
+ struct usb_interface *intf;
+ int ret;
+ unsigned i;
+
+ minor = iminor(inode);
+
+ intf = usb_find_interface(&nrpz_driver, minor);
+ if (!intf) {
+ nrpz_err("Can not find device for minor %d", minor);
+ return -ENODEV;
+ }
+
+ dev = usb_get_intfdata(intf);
+ if (!dev)
+ return -ENODEV;
+
+ if (test_and_set_bit(0, &dev->in_use))
+ return -EBUSY;
+
+ /* increment our usage count for the device */
+ kref_get(&dev->kref);
+
+ /* save our object in the file's private structure */
+ file->private_data = dev;
+
+ INIT_LIST_HEAD(&dev->in_avail);
+ INIT_LIST_HEAD(&dev->out_avail);
+
+ ret = bulks_init(dev,
+ dev->in_urbs,
+ ARRAY_SIZE(dev->in_urbs),
+ usb_rcvbulkpipe(dev->udev, dev->in_epAddr),
+ dev->in_size,
+ nrpz_read_callback);
+ if (ret)
+ goto error;
+
+ ret = bulks_init(dev,
+ dev->out_urbs,
+ ARRAY_SIZE(dev->out_urbs),
+ usb_sndbulkpipe(dev->udev, dev->out_epAddr),
+ dev->out_size,
+ nrpz_write_callback);
+ if (ret)
+ goto error;
+
+ for (i = 0; i != ARRAY_SIZE(dev->in_urbs); ++i) {
+ usb_anchor_urb(&dev->in_urbs[i], &dev->in_running);
+
+ ret = usb_submit_urb(&dev->in_urbs[i], GFP_KERNEL);
+ if (ret) {
+ usb_kill_anchored_urbs(&dev->in_running);
+ goto error;
+ }
+ }
+
+ for (i = 0; i != ARRAY_SIZE(dev->out_urbs); ++i)
+ list_add(&dev->out_urbs[i].urb_list, &dev->out_avail);
+
+ return 0;
+error:
+ clear_bit(0, &dev->in_use);
+
+ bulks_release(dev, dev->out_urbs, ARRAY_SIZE(dev->out_urbs),
+ dev->out_size);
+ bulks_release(dev, dev->in_urbs, ARRAY_SIZE(dev->in_urbs),
+ dev->in_size);
+
+ return 0;
+}
+
+static int nrpz_release(struct inode *inode, struct file *file)
+{
+ struct usb_nrpz *dev;
+
+ dev = (struct usb_nrpz *)file->private_data;
+ if (dev == NULL)
+ return -ENODEV;
+
+ usb_kill_anchored_urbs(&dev->in_running);
+ usb_kill_anchored_urbs(&dev->out_running);
+
+ bulks_release(dev, dev->out_urbs, ARRAY_SIZE(dev->out_urbs),
+ dev->out_size);
+ bulks_release(dev, dev->in_urbs, ARRAY_SIZE(dev->in_urbs),
+ dev->in_size);
+
+ /* decrement the count on our device */
+ kref_put(&dev->kref, nrpz_delete);
+
+ clear_bit(0, &dev->in_use);
+
+ return 0;
+}
+
+static int nrpz_flush(struct file *file, fl_owner_t id)
+{
+ struct usb_nrpz *dev;
+ int ret;
+
+ dev = (struct usb_nrpz *)file->private_data;
+ if (dev == NULL)
+ return -ENODEV;
+
+ /* lock the write data */
+ ret = mutex_lock_interruptible(&dev->write_mutex);
+ if (ret)
+ return ret;
+
+ /* verify that the device wasn't unplugged */
+ if (!dev->intf) {
+ ret = -ENODEV;
+ goto exit;
+ }
+
+ ret = wait_event_interruptible(dev->out_running.wait,
+ list_empty(&dev->out_running.urb_list));
+ if (ret) {
+ ret = -ERESTARTSYS;
+ goto exit;
+ }
+exit:
+ mutex_unlock(&dev->write_mutex);
+
+ return ret;
+}
+
+static unsigned int nrpz_poll(struct file *file, poll_table *wait)
+{
+ struct usb_nrpz *dev;
+ int ret = 0;
+
+ dev = (struct usb_nrpz *)file->private_data;
+
+ poll_wait(file, &dev->wq, wait);
+
+ if (!dev->intf)
+ ret = POLLIN | POLLOUT | POLLPRI | POLLERR | POLLHUP;
+ else {
+ if (!list_empty(&dev->in_avail))
+ ret |= POLLIN;
+
+ if (!list_empty(&dev->out_avail))
+ ret |= POLLOUT;
+ }
+
+ return ret;
+}
+
+static const struct file_operations nrpz_fops = {
+ .owner = THIS_MODULE,
+ .read = nrpz_read,
+ .write = nrpz_write,
+ .unlocked_ioctl = nrpz_ioctl,
+ .compat_ioctl = nrpz_compat_ioctl,
+ .open = nrpz_open,
+ .release = nrpz_release,
+ .flush = nrpz_flush,
+ .poll = nrpz_poll,
+ .llseek = noop_llseek,
+};
+
+static struct usb_class_driver nrpz_class = {
+ .name = "nrpz%d",
+ .fops = &nrpz_fops,
+ .minor_base = NRPZ_MINOR_BASE,
+};
+
+static int nrpz_probe(struct usb_interface *intf,
+ const struct usb_device_id *id)
+{
+ int i;
+ int ret;
+ struct usb_endpoint_descriptor *endpoint;
+ struct usb_host_interface *iface_desc;
+ struct usb_nrpz *dev;
+
+ /* allocate memory for our device state and intialize it */
+ dev = kzalloc(sizeof(*dev), GFP_KERNEL);
+ if (!dev) {
+ nrpz_err("Out of memory");
+ return -ENOMEM;
+ }
+
+ ret = -EIO;
+
+ init_waitqueue_head(&dev->wq);
+ kref_init(&dev->kref);
+
+ mutex_init(&dev->read_mutex);
+ mutex_init(&dev->write_mutex);
+
+ spin_lock_init(&dev->read_lock);
+ spin_lock_init(&dev->write_lock);
+
+ init_usb_anchor(&dev->in_running);
+ init_usb_anchor(&dev->out_running);
+
+ dev->in_use = 0;
+ dev->udev = usb_get_dev(interface_to_usbdev(intf));
+ dev->intf = intf;
+
+ /* set up the endpoint information */
+ /* check out the endpoints */
+ iface_desc = intf->cur_altsetting;
+ for (i = 0; i < iface_desc->desc.bNumEndpoints; ++i) {
+ endpoint = &iface_desc->endpoint[i].desc;
+
+ if (!dev->in_epAddr && usb_endpoint_is_bulk_in(endpoint)) {
+ /* we found a bulk in endpoint */
+ dev->in_size = le16_to_cpu(endpoint->wMaxPacketSize);
+ dev->in_epAddr = endpoint->bEndpointAddress;
+ } else
+ if (!dev->out_epAddr && usb_endpoint_is_bulk_out(endpoint)) {
+ /* we found a bulk out endpoint */
+ dev->out_size = le16_to_cpu(endpoint->wMaxPacketSize);
+ dev->out_epAddr = endpoint->bEndpointAddress;
+ }
+ }
+ if (!(dev->in_epAddr && dev->out_epAddr)) {
+ nrpz_err("Could not find both bulk in and out endpoints");
+ goto error;
+ }
+
+ usb_set_intfdata(intf, dev);
+
+ ret = usb_register_dev(intf, &nrpz_class);
+ if (ret) {
+ nrpz_err("Not able to get a minor for this device\n");
+ goto error;
+ }
+
+ dev->minor = intf->minor - NRPZ_MINOR_BASE;
+
+ /* let the user know what node this device is now attached to */
+ nrpz_info("Device now attached to USB nrpz%u", dev->minor);
+
+ return 0;
+error:
+ usb_set_intfdata(intf, NULL);
+ nrpz_delete(&dev->kref);
+ return ret;
+}
+
+static void nrpz_disconnect(struct usb_interface *intf)
+{
+ struct usb_nrpz *dev;
+ unsigned minor;
+
+ dev = usb_get_intfdata(intf);
+ usb_set_intfdata(intf, NULL);
+
+ minor = dev->minor;
+
+ /* give back our minor */
+ usb_deregister_dev(intf, &nrpz_class);
+
+ /* prevent more I/O from starting */
+ dev->intf = NULL;
+
+ usb_kill_anchored_urbs(&dev->in_running);
+ usb_kill_anchored_urbs(&dev->out_running);
+
+ wake_up_all(&dev->wq);
+
+ /* decrement our usage count */
+ kref_put(&dev->kref, nrpz_delete);
+
+ nrpz_info("nrpz%u now disconnected", minor);
+}
+
+static void nrpz_draw_down(struct usb_nrpz *dev)
+{
+ int time;
+
+ time = usb_wait_anchor_empty_timeout(&dev->out_running, 1000);
+ if (!time)
+ usb_kill_anchored_urbs(&dev->out_running);
+
+ usb_kill_anchored_urbs(&dev->in_running);
+}
+
+static int nrpz_suspend(struct usb_interface *intf, pm_message_t message)
+{
+ struct usb_nrpz *dev = usb_get_intfdata(intf);
+
+ if (dev)
+ nrpz_draw_down(dev);
+ return 0;
+}
+
+static int nrpz_resume(struct usb_interface *intf)
+{
+ return 0;
+}
+
+static int nrpz_pre_reset(struct usb_interface *intf)
+{
+ struct usb_nrpz *dev = usb_get_intfdata(intf);
+
+ if (dev)
+ nrpz_draw_down(dev);
+ return 0;
+}
+
+static int nrpz_post_reset(struct usb_interface *intf)
+{
+ return 0;
+}
+
+static struct usb_driver nrpz_driver = {
+ .name = "nrpz",
+ .probe = nrpz_probe,
+ .disconnect = nrpz_disconnect,
+ .suspend = nrpz_suspend,
+ .resume = nrpz_resume,
+ .pre_reset = nrpz_pre_reset,
+ .post_reset = nrpz_post_reset,
+ .id_table = nrpz_table,
+};
+
+static int __init usb_nrpz_init(void)
+{
+ int ret;
+
+ /* register this driver with the USB subsystem */
+ ret = usb_register(&nrpz_driver);
+ if (ret) {
+ nrpz_err("usb_register failed (%d)", ret);
+ return ret;
+ }
+
+ nrpz_info(DRIVER_DESC " " DRIVER_VERSION);
+
+ return 0;
+}
+
+static void __exit usb_nrpz_exit(void)
+{
+ /* deregister this driver with the USB subsystem */
+ usb_deregister(&nrpz_driver);
+}
+
+module_init(usb_nrpz_init);
+module_exit(usb_nrpz_exit);
+
+MODULE_AUTHOR(DRIVER_AUTHOR);
+MODULE_DESCRIPTION(DRIVER_DESC);
+MODULE_LICENSE("GPL");
diff --git a/include/linux/usb/nrpzmodule.h b/include/linux/usb/nrpzmodule.h
new file mode 100644
index 0000000..8a39aef
--- /dev/null
+++ b/include/linux/usb/nrpzmodule.h
@@ -0,0 +1,47 @@
+#ifndef __NRPZMODULE_H_
+#define __NRPZMODULE_H_
+
+#include "linux/ioctl.h"
+
+#define NRPZ_IOC_MAGIC 'N'
+
+#define NRPZ_GETSENSORINFO _IOR(NRPZ_IOC_MAGIC, 0x01, struct nrpz_sensor_info *)
+#define NRPZ_START _IO(NRPZ_IOC_MAGIC, 0x02)
+#define NRPZ_WRITE_DONE _IOW(NRPZ_IOC_MAGIC, 0x03, unsigned long)
+#define NRPZ_VENDOR_CONTROL_MSG _IOW(NRPZ_IOC_MAGIC, 0x06, struct nrpz_control_req *)
+#define NRPZ_VENDOR_CONTROL_MSG_OUT _IOW(NRPZ_IOC_MAGIC, 0x06, struct nrpz_control_req *)
+#define NRPZ_VENDOR_CONTROL_MSG_IN _IOW(NRPZ_IOC_MAGIC, 0x07, struct nrpz_control_req *)
+
+struct nrpz_sensor_info {
+ unsigned char bDescriptorType;
+ unsigned short bcdUSB;
+ unsigned char bDeviceClass;
+ unsigned char bDeviceSubClass;
+ unsigned char bDeviceProtocol;
+ unsigned char bMaxPacketSize0;
+ unsigned short vendorId;
+ unsigned short productId;
+ unsigned short bcdDevice;
+ unsigned char iManufacturer;
+ unsigned char iProduct;
+ unsigned char iSerialNumber;
+ unsigned char bNumConfigurations;
+ char protocol[128];
+ char manufacturer[128];
+ char productName[128];
+ char serialNumber[128];
+};
+
+/*
+ * struct for NRPZ_VENDOR_CONTROL
+ */
+struct nrpz_control_req {
+ unsigned char request;
+ unsigned char type;
+ unsigned short value;
+ unsigned short index;
+ unsigned char *data;
+ unsigned short size;
+};
+
+#endif
--
1.7.8.6

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


gregkh at linuxfoundation

May 29, 2012, 4:15 PM

Post #2 of 28 (669 views)
Permalink
Re: [PATCH] add new NRP power meter USB device driver [In reply to]

On Tue, May 29, 2012 at 09:14:18PM +0200, stefani [at] seibold wrote:
> From: Stefani Seibold <stefani [at] seibold>
>
> This driver supports all of the Rohde&Schwarz RF Power Meter NRP Sensors. These
> sensors are intelligent standalone instruments that communicate via USB.
>
> A power meter is a device for analyzing the RF power output level of an
> electrical device, similar to an oscilloscope.

Nice, but why is this a kernel driver? Can't you control this device
from userspace using libusb/usbfs?

> The Power Meter Sensors will be used in a wide range of environements, like
>
> - Manufacturing (e.g. air planes and smart phones)
> - Radio and TV broadcasting
> - Mobile communications
> - Engeeniering
> - Science Labs
> - Education
>
> The NRP Power Meters support the following measurements:
>
> - Dynamic range: up to 90 dB (sensor dependent)
> - Level range: -67 dBm to +45 dBm (sensor dependent)
> - Frequency range: DC to 67 GHz (sensor dependent)
> - Measurement speed: 1500 measurements per second in the buffered mode
> - Linearity uncertainty: 0.007 dB
> - Precise average power measurements irrespective of modulation and bandwidth
> - Flexible measurements on up to 128 time slots per power sensor
> - S parameter correction of components between sensor and test object

These measurements are good, but if you want to be a kernel driver, you
should tie into the IIO framework which should support a common
userspace API for these sensor readings, and not create driver-custom
ioctls for just this one device.

> The device will be controlled by a SCPI like interface.

What is scpi?

> The patch is against linux 3.5.0
> (commit 1e2aec873ad6d16538512dbb96853caa1fa076af)
>
> The source is checked with checkpatch.pl and has no errors. Only 11
> "line over 80 characters" warnings are left. I see no reason to satisfy this
> boring punch card limit for this lines, since it will make the source noisier.
>
> make C=1 is quiet.
>
> Please apply it to the kernel tree.
> Thanks.
>
> Signed-off-by: Stefani Seibold <stefani [at] seibold>
> ---
> drivers/usb/misc/Kconfig | 12 +
> drivers/usb/misc/Makefile | 1 +
> drivers/usb/misc/nrpz.c | 1069 ++++++++++++++++++++++++++++++++++++++++
> include/linux/usb/nrpzmodule.h | 47 ++
> 4 files changed, 1129 insertions(+), 0 deletions(-)
> create mode 100644 drivers/usb/misc/nrpz.c
> create mode 100644 include/linux/usb/nrpzmodule.h
>
> diff --git a/drivers/usb/misc/Kconfig b/drivers/usb/misc/Kconfig
> index 1bfcd02..2c55d5f 100644
> --- a/drivers/usb/misc/Kconfig
> +++ b/drivers/usb/misc/Kconfig
> @@ -244,3 +244,15 @@ config USB_YUREX
> To compile this driver as a module, choose M here: the
> module will be called yurex.
>
> +config USB_NRPZ
> + tristate "USB NRPZ power sensor driver support"
> + depends on USB
> + help
> + This driver supports the Rohde&Schwarz NRP RF Power Meter Sensors.
> +
> + These sensors are intelligent standalone instruments that
> + communicate via USB and provide a wide range of measurements.
> +
> + To compile this driver as a module, choose M here: the
> + module will be called nrpz.
> +
> diff --git a/drivers/usb/misc/Makefile b/drivers/usb/misc/Makefile
> index 796ce7e..9a0364c 100644
> --- a/drivers/usb/misc/Makefile
> +++ b/drivers/usb/misc/Makefile
> @@ -25,5 +25,6 @@ obj-$(CONFIG_USB_TRANCEVIBRATOR) += trancevibrator.o
> obj-$(CONFIG_USB_USS720) += uss720.o
> obj-$(CONFIG_USB_SEVSEG) += usbsevseg.o
> obj-$(CONFIG_USB_YUREX) += yurex.o
> +obj-$(CONFIG_USB_NRPZ) += nrpz.o
>
> obj-$(CONFIG_USB_SISUSBVGA) += sisusbvga/
> diff --git a/drivers/usb/misc/nrpz.c b/drivers/usb/misc/nrpz.c
> new file mode 100644
> index 0000000..e341703
> --- /dev/null
> +++ b/drivers/usb/misc/nrpz.c
> @@ -0,0 +1,1069 @@
> +/*
> + * Rohde & Schwarz USB NRP Zxx power meter kernel module driver
> + *
> + * Version: 4.0
> + *
> + * Copyright (c) 2012 by Rohde & Scharz GmbH & Co. KG
> + * written by Stefani Seibold <stefani [at] seibold>
> + *
> + * Based on USB Skeleton driver written by Greg Kroah-Hartman <greg [at] kroah>
> + *
> + * This driver supports the RF Power Meter R&S NRP Sensors. These
> + * sensors are intelligent standalone instruments that communicate via USB
> + * and support the following measurements:
> + *
> + * - Dynamic range: up to 90 dB (sensor dependent)
> + * - Level range: -67 dBm to +45 dBm (sensor dependent)
> + * - Frequency range: DC to 67 GHz (sensor dependent)
> + * - Measurement speed: 1500 measurements per second in the buffered mode
> + * - Linearity uncertainty: 0.007 dB
> + * - Precise average power measurements irrespective of modulation and bandwidth
> + * - Flexible measurements on up to 128 time slots per power sensor
> + * - S parameter correction of components between sensor and test object
> + *
> + * History:
> + *
> + * 2012_05_18 - 4.0 - revamp for kernel inclusion
> + * 2007_05_15 - 2.0 - Ported the driver to kernel 2.6, mostly rewritten
> + * 2001_05_01 - 0.1 - first version
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.

Are you sure "any later version"?

> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.

This paragraph is never needed, unless you want to track the movements
of the FSF's office for the next 40 years.

> + */
> +
> +/*
> + * Internal format of the NRPZ USB messages:
> + *
> + * Byte 0: Signature
> + * Byte 1: Error Code
> + * Byte 2/3: Array Index
> + * or State (Byte 2)
> + * or Group Number (Byte 2) / Param Number (Byte 3)
> + * Byte 4-15: Data depening on signature type (Byte 0):
> + * floats, bit fields and integer are 32 bit
> + *
> + * Signature types:
> + * 'E': single float value
> + * 'e': single value
> + * 'A': float array
> + * 'P': auxiliary or peak float array
> + * 'a': interim float array
> + * 'p': interim auxiliary or peak float array
> + * 'F': feature bit field
> + * 'U': float parameter (32 bit)
> + * 'V': bit field parameter (32 bit)
> + * 'W': integer parameter (32 bit)
> + * 'T': string data
> + * 'R': receipt data (string or binary data)
> + * 'X': internal error
> + * 'Z': current state (Byte 2)
> + * 'z': life sign package
> + * 'L': float value (32 bit)
> + * 'M': bit field value (32 bit)
> + * 'N': long value (32 bit)
> + * 'B': binary data
> + *
> + * State values:
> + * 0: trigger idle
> + * 1: trigger reserved
> + * 2: wait for trigger
> + * 3: trigger measuring
> + *
> + * Communication in direction from host to the device are mostly like SCPI.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/sched.h>
> +#include <linux/signal.h>
> +#include <linux/errno.h>
> +#include <linux/poll.h>
> +#include <linux/init.h>
> +#include <linux/slab.h>
> +#include <linux/fcntl.h>
> +#include <linux/module.h>
> +#include <linux/spinlock.h>
> +#include <linux/list.h>
> +#include <linux/usb.h>
> +#include <linux/version.h>
> +#include <linux/kref.h>
> +#include <linux/bitops.h>
> +#include <linux/mutex.h>
> +
> +#include <linux/usb/nrpzmodule.h>
> +
> +#if 0
> +#define CONFIG_NRP_DEBUG
> +#endif
> +#ifdef CONFIG_NRP_DEBUG
> +static int debug = 1;
> +#else
> +static int debug;
> +#endif
> +
> +module_param(debug, int, 0);
> +MODULE_PARM_DESC(debug, "Debug enabled or not");
> +
> +#define nrpz_dbg(format, arg...) \
> + do { if (debug) printk(KERN_DEBUG "nrpz: " format "\n", ##arg); } while (0)
> +#define nrpz_err(format, arg...) \
> + do { printk(KERN_ERR "nrpz: " format "\n", ##arg); } while (0)
> +#define nrpz_info(format, arg...) \
> + do { printk(KERN_INFO "nrpz: " format "\n", ##arg); } while (0)

Ick, no, please use dev_dbg(), dev_err(), and dev_info() instead, don't
create your own macros.

> +/* Version Information */
> +#define DRIVER_VERSION "v4.00"

What happened to the first 3 versions of this code? :)

> +#define DRIVER_AUTHOR "Stefani Seibold <stefani [at] seibold>"
> +#define DRIVER_DESC "Rohde&Schwarz NRP-Zxx USB Powermeter"
> +
> +/* Get a minor range for your devices from the usb maintainer */
> +#define NRPZ_MINOR_BASE 192

You didn't ask me for a minor base, did you?

> +#define to_nrpz_dev(d) container_of(d, struct usb_nrpz, kref)
> +#define list_to_urb(d) container_of(d, struct urb, urb_list)

Make these inline functions instead?

> +
> +/* Define these values to match your device */

This comment can be fixed, right?

> +#define USB_RS_VENDOR_ID 0x0aad
> +#define USB_NRP_PRODUCT_ID 0x0002
> +#define USB_NRPZ21_PRODUCT_ID 0x0003
> +#define USB_NRPFU_PRODUCT_ID 0x0004
> +#define USB_FSHZ1_PRODUCT_ID 0x000b
> +#define USB_NRPZ11_PRODUCT_ID 0x000c
> +#define USB_NRPZ22_PRODUCT_ID 0x0013
> +#define USB_NRPZ23_PRODUCT_ID 0x0014
> +#define USB_NRPZ24_PRODUCT_ID 0x0015
> +#define USB_NRPZ51_PRODUCT_ID 0x0016
> +#define USB_NRPZ52_PRODUCT_ID 0x0017
> +#define USB_NRPZ55_PRODUCT_ID 0x0018
> +#define USB_NRPZ56_PRODUCT_ID 0x0019
> +#define USB_FSHZ18_PRODUCT_ID 0x001a
> +#define USB_NRPZ91_PRODUCT_ID 0x0021
> +#define USB_NRPZ81_PRODUCT_ID 0x0023
> +#define USB_NRPZ31_PRODUCT_ID 0x002c
> +#define USB_NRPZ37_PRODUCT_ID 0x002d
> +#define USB_NRPZ96_PRODUCT_ID 0x002e
> +#define USB_NRPZ27_PRODUCT_ID 0x002f
> +#define USB_NRPZ28_PRODUCT_ID 0x0051
> +#define USB_NRPZ98_PRODUCT_ID 0x0052
> +#define USB_NRPZ92_PRODUCT_ID 0x0062
> +#define USB_NRPZ57_PRODUCT_ID 0x0070
> +#define USB_NRPZ85_PRODUCT_ID 0x0083
> +#define USB_NRPC40_PRODUCT_ID 0x008F
> +#define USB_NRPC50_PRODUCT_ID 0x0090
> +#define USB_NRPZ86_PRODUCT_ID 0x0095
> +#define USB_NRPZ41_PRODUCT_ID 0x0096
> +#define USB_NRPZ61_PRODUCT_ID 0x0097
> +#define USB_NRPZ71_PRODUCT_ID 0x0098
> +#define USB_NRPZ32_PRODUCT_ID 0x009A
> +#define USB_NRPZ211_PRODUCT_ID 0x00A6
> +#define USB_NRPZ221_PRODUCT_ID 0x00A7
> +#define USB_NRPZ58_PRODUCT_ID 0x00A8
> +#define USB_NRPC33_PRODUCT_ID 0x00B6
> +#define USB_NRPC18_PRODUCT_ID 0x00BF
> +
> +/* table of devices that work with this driver */
> +static struct usb_device_id nrpz_table[] = {
> + {USB_DEVICE(USB_RS_VENDOR_ID, USB_NRP_PRODUCT_ID)},
> + {USB_DEVICE(USB_RS_VENDOR_ID, USB_NRPZ21_PRODUCT_ID)},
> + {USB_DEVICE(USB_RS_VENDOR_ID, USB_NRPFU_PRODUCT_ID)},
> + {USB_DEVICE(USB_RS_VENDOR_ID, USB_FSHZ1_PRODUCT_ID)},
> + {USB_DEVICE(USB_RS_VENDOR_ID, USB_NRPZ11_PRODUCT_ID)},
> + {USB_DEVICE(USB_RS_VENDOR_ID, USB_NRPZ22_PRODUCT_ID)},
> + {USB_DEVICE(USB_RS_VENDOR_ID, USB_NRPZ23_PRODUCT_ID)},
> + {USB_DEVICE(USB_RS_VENDOR_ID, USB_NRPZ24_PRODUCT_ID)},
> + {USB_DEVICE(USB_RS_VENDOR_ID, USB_NRPZ51_PRODUCT_ID)},
> + {USB_DEVICE(USB_RS_VENDOR_ID, USB_NRPZ52_PRODUCT_ID)},
> + {USB_DEVICE(USB_RS_VENDOR_ID, USB_NRPZ55_PRODUCT_ID)},
> + {USB_DEVICE(USB_RS_VENDOR_ID, USB_NRPZ56_PRODUCT_ID)},
> + {USB_DEVICE(USB_RS_VENDOR_ID, USB_NRPZ57_PRODUCT_ID)},
> + {USB_DEVICE(USB_RS_VENDOR_ID, USB_FSHZ18_PRODUCT_ID)},
> + {USB_DEVICE(USB_RS_VENDOR_ID, USB_NRPZ91_PRODUCT_ID)},
> + {USB_DEVICE(USB_RS_VENDOR_ID, USB_NRPZ81_PRODUCT_ID)},
> + {USB_DEVICE(USB_RS_VENDOR_ID, USB_NRPZ31_PRODUCT_ID)},
> + {USB_DEVICE(USB_RS_VENDOR_ID, USB_NRPZ37_PRODUCT_ID)},
> + {USB_DEVICE(USB_RS_VENDOR_ID, USB_NRPZ96_PRODUCT_ID)},
> + {USB_DEVICE(USB_RS_VENDOR_ID, USB_NRPZ27_PRODUCT_ID)},
> + {USB_DEVICE(USB_RS_VENDOR_ID, USB_NRPZ28_PRODUCT_ID)},
> + {USB_DEVICE(USB_RS_VENDOR_ID, USB_NRPZ98_PRODUCT_ID)},
> + {USB_DEVICE(USB_RS_VENDOR_ID, USB_NRPZ92_PRODUCT_ID)},
> + {USB_DEVICE(USB_RS_VENDOR_ID, USB_NRPZ85_PRODUCT_ID)},
> + {USB_DEVICE(USB_RS_VENDOR_ID, USB_NRPC40_PRODUCT_ID)},
> + {USB_DEVICE(USB_RS_VENDOR_ID, USB_NRPC50_PRODUCT_ID)},
> + {USB_DEVICE(USB_RS_VENDOR_ID, USB_NRPZ86_PRODUCT_ID)},
> + {USB_DEVICE(USB_RS_VENDOR_ID, USB_NRPZ41_PRODUCT_ID)},
> + {USB_DEVICE(USB_RS_VENDOR_ID, USB_NRPZ61_PRODUCT_ID)},
> + {USB_DEVICE(USB_RS_VENDOR_ID, USB_NRPZ71_PRODUCT_ID)},
> + {USB_DEVICE(USB_RS_VENDOR_ID, USB_NRPZ32_PRODUCT_ID)},
> + {USB_DEVICE(USB_RS_VENDOR_ID, USB_NRPZ211_PRODUCT_ID)},
> + {USB_DEVICE(USB_RS_VENDOR_ID, USB_NRPZ221_PRODUCT_ID)},
> + {USB_DEVICE(USB_RS_VENDOR_ID, USB_NRPZ58_PRODUCT_ID)},
> + {USB_DEVICE(USB_RS_VENDOR_ID, USB_NRPC33_PRODUCT_ID)},
> + {USB_DEVICE(USB_RS_VENDOR_ID, USB_NRPC18_PRODUCT_ID)},
> + {} /* Terminating entry */
> +};
> +
> +MODULE_DEVICE_TABLE(usb, nrpz_table);
> +
> +/* Structure to hold all of our device specific stuff */
> +struct usb_nrpz {
> + struct usb_device *udev;
> + struct usb_interface *intf;

Do you really need both?

> + unsigned minor; /* minor number for this device */
> + unsigned long in_use; /* in use flag */

Why a long for this?

> +
> + size_t in_size; /* size of the receive buffer */
> + size_t out_size; /* size of the send buffer */
> + __u8 in_epAddr; /* address of the bulk in endpoint */
> + __u8 out_epAddr; /* address of the bulk out endpoint */
> +
> + struct kref kref;
> + wait_queue_head_t wq;
> +
> + struct usb_anchor out_running; /* list of in use output buffers */
> + struct list_head out_avail; /* list of available output buffers */
> + spinlock_t write_lock; /* spinlock for transmit list */
> + struct mutex write_mutex; /* exclusive write data semaphore */
> +
> + struct usb_anchor in_running; /* list of in use input buffers */
> + struct list_head in_avail; /* list of available input buffers */
> + spinlock_t read_lock; /* spinlock for receive list */
> + struct mutex read_mutex; /* exclusive read data semaphore */
> +
> + struct urb out_urbs[64]; /* array of urb's for output */
> + struct urb in_urbs[64]; /* array of urb's for input */
> +};
> +
> +/* forward declaration */
> +static struct usb_driver nrpz_driver;
> +
> +static inline void urb_list_add(spinlock_t *lock, struct urb *urb,
> + struct list_head *head)
> +{
> + spin_lock_irq(lock);
> + list_add(&urb->urb_list, head);
> + spin_unlock_irq(lock);
> +}
> +
> +static inline void urb_list_add_tail(spinlock_t *lock, struct urb *urb,
> + struct list_head *head)
> +{
> + spin_lock_irq(lock);
> + list_add_tail(&urb->urb_list, head);
> + spin_unlock_irq(lock);
> +}

Why the lists of urbs? Why not either just create them all dynamically
all the time, or use urb anchors?

> + case NRPZ_GETSENSORINFO:
> + {
> + struct nrpz_sensor_info __user *sensor_info =
> + (struct nrpz_sensor_info __user *)arg;
> +
> + if (!access_ok(VERIFY_WRITE, sensor_info, sizeof(*sensor_info)))
> + return -EFAULT;
> +
> + __put_user(dev->udev->descriptor.bcdDevice,
> + &sensor_info->bcdDevice);
> + __put_user(dev->udev->descriptor.bcdUSB,
> + &sensor_info->bcdUSB);
> + __put_user(dev->udev->descriptor.bDescriptorType,
> + &sensor_info->bDescriptorType);
> + __put_user(dev->udev->descriptor.bDeviceClass,
> + &sensor_info->bDeviceClass);
> + __put_user(dev->udev->descriptor.bDeviceSubClass,
> + &sensor_info->bDeviceSubClass);
> + __put_user(dev->udev->descriptor.bDeviceProtocol,
> + &sensor_info->bDeviceProtocol);
> + __put_user(dev->udev->descriptor.bMaxPacketSize0,
> + &sensor_info->bMaxPacketSize0);
> + __put_user(dev->udev->descriptor.bNumConfigurations,
> + &sensor_info->bNumConfigurations);
> + __put_user(dev->udev->descriptor.iManufacturer,
> + &sensor_info->iManufacturer);
> + __put_user(dev->udev->descriptor.iProduct,
> + &sensor_info->iProduct);
> + __put_user(dev->udev->descriptor.iSerialNumber,
> + &sensor_info->iSerialNumber);
> + __put_user(dev->udev->descriptor.idVendor,
> + &sensor_info->vendorId);
> + __put_user(dev->udev->descriptor.idProduct,
> + &sensor_info->productId);
> + usb_string(dev->udev, dev->udev->descriptor.iManufacturer,
> + (char __force *)sensor_info->manufacturer,
> + sizeof(sensor_info->manufacturer));
> + usb_string(dev->udev, dev->udev->descriptor.iProduct,
> + (char __force *)sensor_info->productName,
> + sizeof(sensor_info->productName));
> + usb_string(dev->udev, dev->udev->descriptor.iSerialNumber,
> + (char __force *)sensor_info->serialNumber,
> + sizeof(sensor_info->serialNumber));

Why are you sending this information through an ioctl, when it is
already exported in sysfs? That seems quite needless.

Anyway, I think the main questions are, "Why is this a kernel driver",
and "If this is a kernel driver, why not use the IIO interface"?

thanks,

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


oneukum at suse

May 30, 2012, 1:10 AM

Post #3 of 28 (661 views)
Permalink
Re: [PATCH] add new NRP power meter USB device driver [In reply to]

Am Dienstag, 29. Mai 2012, 21:14:18 schrieb stefani [at] seibold:

> The device will be controlled by a SCPI like interface.

Are there other devices using that standard whose interface you might reuse?


> +static ssize_t nrpz_read(struct file *file, char __user *buffer, size_t count,
> + loff_t *ppos)
> +{
> + struct usb_nrpz *dev;
> + int ret;
> + struct urb *urb;
> + size_t n;
> +
> + dev = (struct usb_nrpz *)file->private_data;
> +
> + /* verify that we actually have some data to read */
> + if (!count)
> + return 0;
> +
> + /* lock the read data */
> + ret = mutex_lock_interruptible(&dev->read_mutex);
> + if (ret)
> + return ret;
> +
> + for (;;) {
> + urb = urb_list_get(&dev->read_lock, &dev->in_avail);
> + if (urb)
> + break;
> +
> + if (file->f_flags & O_NONBLOCK) {
> + ret = -EAGAIN;
> + goto exit;
> + }

You should test for device disconnect before that. Otherwise
a nonblocking reader would never learn of the problem. IIRC
the skeleton has been fixed.

> +
> + ret = wait_event_interruptible(dev->wq,
> + !list_empty(&dev->in_avail) || !dev->intf);
> + if (ret) {
> + ret = -ERESTARTSYS;
> + goto exit;
> + }
> +
> + /* verify that the device wasn't unplugged */
> + if (!dev->intf) {
> + ret = -ENODEV;
> + goto exit;
> + }
> + }
> +
> + if (!urb->status) {
> + n = min(count, urb->actual_length);
> +
> + if (copy_to_user(buffer, urb->transfer_buffer, n)) {
> + urb_list_add(&dev->read_lock, urb, &dev->in_avail);
> + ret = -EFAULT;
> + goto exit;

Is there a good reason you don't resubmit in this case?

> + }
> + } else
> + n = urb->status;

You need to translate this to standard error codes
> +
> + usb_anchor_urb(urb, &dev->in_running);
> +
> + ret = usb_submit_urb(urb, GFP_KERNEL);
> + if (ret) {
> + usb_unanchor_urb(urb);
> + urb_list_add_tail(&dev->read_lock, urb, &dev->in_avail);
> + nrpz_err("Failed submitting read urb (error %d)", ret);
> + }
> +
> + ret = n;
> +exit:
> + mutex_unlock(&dev->read_mutex);
> + return ret;
> +}

> +#define VRT_RESET_ALL 1
> +#define VRT_GET_DEVICE_INFO 6
> +#define VRI_DEVICE_NAME 5
> +
> +static long nrpz_compat_ioctl(struct file *file, unsigned int cmd,
> + unsigned long arg)
> +{
> + struct usb_nrpz *dev;
> + int ret;
> +
> + dev = (struct usb_nrpz *)file->private_data;
> +
> + /* verify that the device wasn't unplugged */
> + if (!dev->intf)
> + return -ENODEV;

Locking?

> +
> + switch (cmd) {
> + case NRPZ_GETSENSORINFO:
> + {
> + struct nrpz_sensor_info __user *sensor_info =
> + (struct nrpz_sensor_info __user *)arg;
> +
> + if (!access_ok(VERIFY_WRITE, sensor_info, sizeof(*sensor_info)))
> + return -EFAULT;
> +
> + __put_user(dev->udev->descriptor.bcdDevice,
> + &sensor_info->bcdDevice);
> + __put_user(dev->udev->descriptor.bcdUSB,
> + &sensor_info->bcdUSB);
> + __put_user(dev->udev->descriptor.bDescriptorType,
> + &sensor_info->bDescriptorType);
> + __put_user(dev->udev->descriptor.bDeviceClass,
> + &sensor_info->bDeviceClass);
> + __put_user(dev->udev->descriptor.bDeviceSubClass,
> + &sensor_info->bDeviceSubClass);
> + __put_user(dev->udev->descriptor.bDeviceProtocol,
> + &sensor_info->bDeviceProtocol);
> + __put_user(dev->udev->descriptor.bMaxPacketSize0,
> + &sensor_info->bMaxPacketSize0);
> + __put_user(dev->udev->descriptor.bNumConfigurations,
> + &sensor_info->bNumConfigurations);
> + __put_user(dev->udev->descriptor.iManufacturer,
> + &sensor_info->iManufacturer);
> + __put_user(dev->udev->descriptor.iProduct,
> + &sensor_info->iProduct);
> + __put_user(dev->udev->descriptor.iSerialNumber,
> + &sensor_info->iSerialNumber);
> + __put_user(dev->udev->descriptor.idVendor,
> + &sensor_info->vendorId);
> + __put_user(dev->udev->descriptor.idProduct,
> + &sensor_info->productId);
> + usb_string(dev->udev, dev->udev->descriptor.iManufacturer,
> + (char __force *)sensor_info->manufacturer,
> + sizeof(sensor_info->manufacturer));
> + usb_string(dev->udev, dev->udev->descriptor.iProduct,
> + (char __force *)sensor_info->productName,
> + sizeof(sensor_info->productName));
> + usb_string(dev->udev, dev->udev->descriptor.iSerialNumber,
> + (char __force *)sensor_info->serialNumber,
> + sizeof(sensor_info->serialNumber));
> +
> + return 0;
> + }
> + case NRPZ_START:
> + {
> + u8 device_state[128];

DMA on stack. This may ail on some architectures. You need to use
kmalloc() or better kzalloc().

> + memset(device_state, 0, sizeof(device_state));
> +
> + ret = usb_control_msg(dev->udev,
> + usb_rcvctrlpipe(dev->udev, 0),
> + VRT_GET_DEVICE_INFO,
> + USB_DIR_IN | USB_TYPE_VENDOR | USB_RECIP_DEVICE,
> + 0,
> + VRI_DEVICE_NAME,
> + device_state,
> + sizeof(device_state),
> + 5000);
> +
> + if (ret < 0)
> + return ret;
> +
> + nrpz_dbg("device state:%s", device_state);
> +
> + if (strncmp(device_state, "Boot ", 5))
> + return 0;
> +
> + return usb_control_msg(dev->udev,
> + usb_sndctrlpipe(dev->udev, 0),
> + VRT_RESET_ALL,
> + USB_DIR_OUT | USB_TYPE_VENDOR | USB_RECIP_DEVICE,
> + 1,
> + 1,
> + device_state,
> + sizeof(device_state),
> + 5000);
> + }
> + case NRPZ_WRITE_DONE:

EEEEEEK!

> + if (arg) {
> + ret = wait_event_interruptible_timeout(
> + dev->out_running.wait,
> + list_empty(&dev->out_running.urb_list),
> + msecs_to_jiffies(arg));
> + if (!ret)
> + return -ETIMEDOUT;
> + if (ret < 0)
> + return ret;
> + return 0;
> + } else {
> + return wait_event_interruptible(
> + dev->out_running.wait,
> + list_empty(&dev->out_running.urb_list));
> + }
> + break;

This is very ugly. If you need fsync(), then implement it.

> + case NRPZ_VENDOR_CONTROL_MSG_OUT:
> + {
> + struct nrpz_control_req ncr;

DMA on stack.

> + u16 size;
> +
> + if (copy_from_user(&ncr, (struct nrpz_control_req __user *)arg, sizeof(ncr)))
> + return -EFAULT;
> +
> + if (ncr.data) {
> + size = ncr.size;
> +
> + if (!access_ok(VERIFY_WRITE, (void __user *)ncr.data, size))
> + return -EFAULT;
> + } else {
> + size = 0;
> + }
> +
> + ret = usb_control_msg(dev->udev,
> + usb_sndctrlpipe(dev->udev, 0),
> + ncr.request,
> + ncr.type,
> + ncr.value,
> + ncr.index,
> + ncr.data,
> + size,
> + 0);
> +
> + return ret;
> + }
> + case NRPZ_VENDOR_CONTROL_MSG_IN:
> + {
> + struct nrpz_control_req ncr;

DMA on stack

> + u16 size;
> +
> + if (copy_from_user(&ncr, (struct nrpz_control_req __user *)arg, sizeof(ncr)))
> + return -EFAULT;
> +
> + if (ncr.data) {
> + size = ncr.size;
> +
> + if (!access_ok(VERIFY_READ, (void __user *)ncr.data, size))
> + return -EFAULT;
> + } else {
> + size = 0;
> + }
> +
> + ret = usb_control_msg(dev->udev,
> + usb_rcvctrlpipe(dev->udev, 0),
> + ncr.request,
> + ncr.type,
> + ncr.value,
> + ncr.index,
> + ncr.data,
> + size,
> + 0);
> +
> + return ret;
> + }
> + default:
> + nrpz_err("Invalid ioctl call (%08x)", cmd);
> + return -EOPNOTSUPP;
> + }
> +
> + return ret;
> +}

> +static int nrpz_open(struct inode *inode, struct file *file)
> +{
> + struct usb_nrpz *dev;
> + int minor;
> + struct usb_interface *intf;
> + int ret;
> + unsigned i;
> +
> + minor = iminor(inode);
> +
> + intf = usb_find_interface(&nrpz_driver, minor);
> + if (!intf) {
> + nrpz_err("Can not find device for minor %d", minor);
> + return -ENODEV;
> + }
> +
> + dev = usb_get_intfdata(intf);
> + if (!dev)
> + return -ENODEV;
> +
> + if (test_and_set_bit(0, &dev->in_use))
> + return -EBUSY;
> +
> + /* increment our usage count for the device */
> + kref_get(&dev->kref);
> +
> + /* save our object in the file's private structure */
> + file->private_data = dev;
> +
> + INIT_LIST_HEAD(&dev->in_avail);
> + INIT_LIST_HEAD(&dev->out_avail);
> +
> + ret = bulks_init(dev,
> + dev->in_urbs,
> + ARRAY_SIZE(dev->in_urbs),
> + usb_rcvbulkpipe(dev->udev, dev->in_epAddr),
> + dev->in_size,
> + nrpz_read_callback);
> + if (ret)
> + goto error;
> +
> + ret = bulks_init(dev,
> + dev->out_urbs,
> + ARRAY_SIZE(dev->out_urbs),
> + usb_sndbulkpipe(dev->udev, dev->out_epAddr),
> + dev->out_size,
> + nrpz_write_callback);
> + if (ret)
> + goto error;
> +
> + for (i = 0; i != ARRAY_SIZE(dev->in_urbs); ++i) {
> + usb_anchor_urb(&dev->in_urbs[i], &dev->in_running);
> +
> + ret = usb_submit_urb(&dev->in_urbs[i], GFP_KERNEL);

You do this here and only here. So this will see a slow attrition if you don't resubmit for som reason.

> + if (ret) {
> + usb_kill_anchored_urbs(&dev->in_running);
> + goto error;
> + }
> + }
> +
> + for (i = 0; i != ARRAY_SIZE(dev->out_urbs); ++i)
> + list_add(&dev->out_urbs[i].urb_list, &dev->out_avail);
> +
> + return 0;
> +error:
> + clear_bit(0, &dev->in_use);
> +
> + bulks_release(dev, dev->out_urbs, ARRAY_SIZE(dev->out_urbs),
> + dev->out_size);
> + bulks_release(dev, dev->in_urbs, ARRAY_SIZE(dev->in_urbs),
> + dev->in_size);
> +
> + return 0;
> +}
> +
> +static int nrpz_release(struct inode *inode, struct file *file)
> +{
> + struct usb_nrpz *dev;
> +
> + dev = (struct usb_nrpz *)file->private_data;
> + if (dev == NULL)
> + return -ENODEV;
> +
> + usb_kill_anchored_urbs(&dev->in_running);
> + usb_kill_anchored_urbs(&dev->out_running);
> +
> + bulks_release(dev, dev->out_urbs, ARRAY_SIZE(dev->out_urbs),
> + dev->out_size);
> + bulks_release(dev, dev->in_urbs, ARRAY_SIZE(dev->in_urbs),
> + dev->in_size);
> +
> + /* decrement the count on our device */
> + kref_put(&dev->kref, nrpz_delete);
> +
> + clear_bit(0, &dev->in_use);
> +
> + return 0;
> +}
> +
> +static int nrpz_flush(struct file *file, fl_owner_t id)
> +{
> + struct usb_nrpz *dev;
> + int ret;
> +
> + dev = (struct usb_nrpz *)file->private_data;
> + if (dev == NULL)
> + return -ENODEV;
> +
> + /* lock the write data */
> + ret = mutex_lock_interruptible(&dev->write_mutex);
> + if (ret)
> + return ret;
> +
> + /* verify that the device wasn't unplugged */
> + if (!dev->intf) {
> + ret = -ENODEV;
> + goto exit;
> + }
> +
> + ret = wait_event_interruptible(dev->out_running.wait,
> + list_empty(&dev->out_running.urb_list));
> + if (ret) {
> + ret = -ERESTARTSYS;
> + goto exit;
> + }
> +exit:
> + mutex_unlock(&dev->write_mutex);
> +
> + return ret;
> +}
> +
> +static unsigned int nrpz_poll(struct file *file, poll_table *wait)
> +{
> + struct usb_nrpz *dev;
> + int ret = 0;
> +
> + dev = (struct usb_nrpz *)file->private_data;
> +
> + poll_wait(file, &dev->wq, wait);
> +
> + if (!dev->intf)
> + ret = POLLIN | POLLOUT | POLLPRI | POLLERR | POLLHUP;
> + else {
> + if (!list_empty(&dev->in_avail))
> + ret |= POLLIN;
> +
> + if (!list_empty(&dev->out_avail))
> + ret |= POLLOUT;
> + }
> +
> + return ret;
> +}
> +
> +static const struct file_operations nrpz_fops = {
> + .owner = THIS_MODULE,
> + .read = nrpz_read,
> + .write = nrpz_write,
> + .unlocked_ioctl = nrpz_ioctl,
> + .compat_ioctl = nrpz_compat_ioctl,
> + .open = nrpz_open,
> + .release = nrpz_release,
> + .flush = nrpz_flush,
> + .poll = nrpz_poll,
> + .llseek = noop_llseek,
> +};
> +
> +static struct usb_class_driver nrpz_class = {
> + .name = "nrpz%d",
> + .fops = &nrpz_fops,
> + .minor_base = NRPZ_MINOR_BASE,
> +};
> +
> +static int nrpz_probe(struct usb_interface *intf,
> + const struct usb_device_id *id)
> +{
> + int i;
> + int ret;
> + struct usb_endpoint_descriptor *endpoint;
> + struct usb_host_interface *iface_desc;
> + struct usb_nrpz *dev;
> +
> + /* allocate memory for our device state and intialize it */
> + dev = kzalloc(sizeof(*dev), GFP_KERNEL);
> + if (!dev) {
> + nrpz_err("Out of memory");
> + return -ENOMEM;
> + }
> +
> + ret = -EIO;
> +
> + init_waitqueue_head(&dev->wq);
> + kref_init(&dev->kref);
> +
> + mutex_init(&dev->read_mutex);
> + mutex_init(&dev->write_mutex);
> +
> + spin_lock_init(&dev->read_lock);
> + spin_lock_init(&dev->write_lock);
> +
> + init_usb_anchor(&dev->in_running);
> + init_usb_anchor(&dev->out_running);
> +
> + dev->in_use = 0;
> + dev->udev = usb_get_dev(interface_to_usbdev(intf));
> + dev->intf = intf;
> +
> + /* set up the endpoint information */
> + /* check out the endpoints */
> + iface_desc = intf->cur_altsetting;
> + for (i = 0; i < iface_desc->desc.bNumEndpoints; ++i) {
> + endpoint = &iface_desc->endpoint[i].desc;
> +
> + if (!dev->in_epAddr && usb_endpoint_is_bulk_in(endpoint)) {
> + /* we found a bulk in endpoint */
> + dev->in_size = le16_to_cpu(endpoint->wMaxPacketSize);
> + dev->in_epAddr = endpoint->bEndpointAddress;
> + } else
> + if (!dev->out_epAddr && usb_endpoint_is_bulk_out(endpoint)) {
> + /* we found a bulk out endpoint */
> + dev->out_size = le16_to_cpu(endpoint->wMaxPacketSize);
> + dev->out_epAddr = endpoint->bEndpointAddress;
> + }
> + }
> + if (!(dev->in_epAddr && dev->out_epAddr)) {
> + nrpz_err("Could not find both bulk in and out endpoints");
> + goto error;
> + }
> +
> + usb_set_intfdata(intf, dev);
> +
> + ret = usb_register_dev(intf, &nrpz_class);
> + if (ret) {
> + nrpz_err("Not able to get a minor for this device\n");
> + goto error;
> + }
> +
> + dev->minor = intf->minor - NRPZ_MINOR_BASE;
> +
> + /* let the user know what node this device is now attached to */
> + nrpz_info("Device now attached to USB nrpz%u", dev->minor);
> +
> + return 0;
> +error:
> + usb_set_intfdata(intf, NULL);
> + nrpz_delete(&dev->kref);
> + return ret;
> +}
> +
> +static void nrpz_disconnect(struct usb_interface *intf)
> +{
> + struct usb_nrpz *dev;
> + unsigned minor;
> +
> + dev = usb_get_intfdata(intf);
> + usb_set_intfdata(intf, NULL);
> +
> + minor = dev->minor;
> +
> + /* give back our minor */
> + usb_deregister_dev(intf, &nrpz_class);
> +
> + /* prevent more I/O from starting */
> + dev->intf = NULL;
> +
> + usb_kill_anchored_urbs(&dev->in_running);
> + usb_kill_anchored_urbs(&dev->out_running);
> +
> + wake_up_all(&dev->wq);
> +
> + /* decrement our usage count */
> + kref_put(&dev->kref, nrpz_delete);
> +
> + nrpz_info("nrpz%u now disconnected", minor);
> +}
> +
> +static void nrpz_draw_down(struct usb_nrpz *dev)
> +{
> + int time;
> +
> + time = usb_wait_anchor_empty_timeout(&dev->out_running, 1000);
> + if (!time)
> + usb_kill_anchored_urbs(&dev->out_running);
> +
> + usb_kill_anchored_urbs(&dev->in_running);
> +}
> +
> +static int nrpz_suspend(struct usb_interface *intf, pm_message_t message)
> +{
> + struct usb_nrpz *dev = usb_get_intfdata(intf);
> +
> + if (dev)
> + nrpz_draw_down(dev);
> + return 0;
> +}
> +
> +static int nrpz_resume(struct usb_interface *intf)
> +{
> + return 0;
> +}

And what restarts reading?

> +
> +static int nrpz_pre_reset(struct usb_interface *intf)
> +{
> + struct usb_nrpz *dev = usb_get_intfdata(intf);
> +
> + if (dev)
> + nrpz_draw_down(dev);
> + return 0;
> +}
> +
> +static int nrpz_post_reset(struct usb_interface *intf)
> +{
> + return 0;
> +}

And you don't tell user space that the device has been reset?
And what restarts reading?

Regards
Oliver
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


stefani at seibold

May 31, 2012, 12:43 AM

Post #4 of 28 (665 views)
Permalink
Re: [PATCH] add new NRP power meter USB device driver [In reply to]

Hi Oliver,

Thanks for the review.

On Wed, 2012-05-30 at 10:10 +0200, Oliver Neukum wrote:
> Am Dienstag, 29. Mai 2012, 21:14:18 schrieb stefani [at] seibold:
>
> > The device will be controlled by a SCPI like interface.
>
> Are there other devices using that standard whose interface you might reuse?
>
>

There are a lot of instruments from many different manufacturer which
use the SCPI standard. But there is now way to bring them together. The
commands are well defined, but there are a lot of vendor specific
commands. And the communication path are differing, which can be
IEEE488, USB, Socket, Serial and more.

For more information see
https://en.wikipedia.org/wiki/Standard_Commands_for_Programmable_Instruments

> > +static ssize_t nrpz_read(struct file *file, char __user *buffer, size_t count,
> > + loff_t *ppos)
> > +{
> > + struct usb_nrpz *dev;
> > + int ret;
> > + struct urb *urb;
> > + size_t n;
> > +
> > + dev = (struct usb_nrpz *)file->private_data;
> > +
> > + /* verify that we actually have some data to read */
> > + if (!count)
> > + return 0;
> > +
> > + /* lock the read data */
> > + ret = mutex_lock_interruptible(&dev->read_mutex);
> > + if (ret)
> > + return ret;
> > +
> > + for (;;) {
> > + urb = urb_list_get(&dev->read_lock, &dev->in_avail);
> > + if (urb)
> > + break;
> > +
> > + if (file->f_flags & O_NONBLOCK) {
> > + ret = -EAGAIN;
> > + goto exit;
> > + }
>
> You should test for device disconnect before that. Otherwise
> a nonblocking reader would never learn of the problem. IIRC
> the skeleton has been fixed.
>

I did this 10 lines later, i will move this check before the O_NONBLOCK
check.

> > +
> > + ret = wait_event_interruptible(dev->wq,
> > + !list_empty(&dev->in_avail) || !dev->intf);
> > + if (ret) {
> > + ret = -ERESTARTSYS;
> > + goto exit;
> > + }
> > +
> > + /* verify that the device wasn't unplugged */
> > + if (!dev->intf) {
> > + ret = -ENODEV;
> > + goto exit;
> > + }
> > + }
> > +
> > + if (!urb->status) {
> > + n = min(count, urb->actual_length);
> > +
> > + if (copy_to_user(buffer, urb->transfer_buffer, n)) {
> > + urb_list_add(&dev->read_lock, urb, &dev->in_avail);
> > + ret = -EFAULT;
> > + goto exit;
>
> Is there a good reason you don't resubmit in this case?
>

The data was not successfully transfered to the user space, so it will
stay again on the top of the input urb list.

> > + }
> > + } else
> > + n = urb->status;
>
> You need to translate this to standard error codes

I will report -EPIPE in the next version of the driver.

> > +
> > + usb_anchor_urb(urb, &dev->in_running);
> > +
> > + ret = usb_submit_urb(urb, GFP_KERNEL);
> > + if (ret) {
> > + usb_unanchor_urb(urb);
> > + urb_list_add_tail(&dev->read_lock, urb, &dev->in_avail);
> > + nrpz_err("Failed submitting read urb (error %d)", ret);
> > + }
> > +
> > + ret = n;
> > +exit:
> > + mutex_unlock(&dev->read_mutex);
> > + return ret;
> > +}
>
> > +#define VRT_RESET_ALL 1
> > +#define VRT_GET_DEVICE_INFO 6
> > +#define VRI_DEVICE_NAME 5
> > +
> > +static long nrpz_compat_ioctl(struct file *file, unsigned int cmd,
> > + unsigned long arg)
> > +{
> > + struct usb_nrpz *dev;
> > + int ret;
> > +
> > + dev = (struct usb_nrpz *)file->private_data;
> > +
> > + /* verify that the device wasn't unplugged */
> > + if (!dev->intf)
> > + return -ENODEV;
>
> Locking?
>

I see no reason why. But i will change it in the next version.

> > +
> > + switch (cmd) {
> > + case NRPZ_GETSENSORINFO:
> > + {
> > + struct nrpz_sensor_info __user *sensor_info =
> > + (struct nrpz_sensor_info __user *)arg;
> > +
> > + if (!access_ok(VERIFY_WRITE, sensor_info, sizeof(*sensor_info)))
> > + return -EFAULT;
> > +
> > + __put_user(dev->udev->descriptor.bcdDevice,
> > + &sensor_info->bcdDevice);
> > + __put_user(dev->udev->descriptor.bcdUSB,
> > + &sensor_info->bcdUSB);
> > + __put_user(dev->udev->descriptor.bDescriptorType,
> > + &sensor_info->bDescriptorType);
> > + __put_user(dev->udev->descriptor.bDeviceClass,
> > + &sensor_info->bDeviceClass);
> > + __put_user(dev->udev->descriptor.bDeviceSubClass,
> > + &sensor_info->bDeviceSubClass);
> > + __put_user(dev->udev->descriptor.bDeviceProtocol,
> > + &sensor_info->bDeviceProtocol);
> > + __put_user(dev->udev->descriptor.bMaxPacketSize0,
> > + &sensor_info->bMaxPacketSize0);
> > + __put_user(dev->udev->descriptor.bNumConfigurations,
> > + &sensor_info->bNumConfigurations);
> > + __put_user(dev->udev->descriptor.iManufacturer,
> > + &sensor_info->iManufacturer);
> > + __put_user(dev->udev->descriptor.iProduct,
> > + &sensor_info->iProduct);
> > + __put_user(dev->udev->descriptor.iSerialNumber,
> > + &sensor_info->iSerialNumber);
> > + __put_user(dev->udev->descriptor.idVendor,
> > + &sensor_info->vendorId);
> > + __put_user(dev->udev->descriptor.idProduct,
> > + &sensor_info->productId);
> > + usb_string(dev->udev, dev->udev->descriptor.iManufacturer,
> > + (char __force *)sensor_info->manufacturer,
> > + sizeof(sensor_info->manufacturer));
> > + usb_string(dev->udev, dev->udev->descriptor.iProduct,
> > + (char __force *)sensor_info->productName,
> > + sizeof(sensor_info->productName));
> > + usb_string(dev->udev, dev->udev->descriptor.iSerialNumber,
> > + (char __force *)sensor_info->serialNumber,
> > + sizeof(sensor_info->serialNumber));
> > +
> > + return 0;
> > + }
> > + case NRPZ_START:
> > + {
> > + u8 device_state[128];
>
> DMA on stack. This may ail on some architectures. You need to use
> kmalloc() or better kzalloc().
>

Never heard about this problem. The driver will work since years on X86,
PPC and ARM platforms. But it will be fixes in the next version.

> > + memset(device_state, 0, sizeof(device_state));
> > +
> > + ret = usb_control_msg(dev->udev,
> > + usb_rcvctrlpipe(dev->udev, 0),
> > + VRT_GET_DEVICE_INFO,
> > + USB_DIR_IN | USB_TYPE_VENDOR | USB_RECIP_DEVICE,
> > + 0,
> > + VRI_DEVICE_NAME,
> > + device_state,
> > + sizeof(device_state),
> > + 5000);
> > +
> > + if (ret < 0)
> > + return ret;
> > +
> > + nrpz_dbg("device state:%s", device_state);
> > +
> > + if (strncmp(device_state, "Boot ", 5))
> > + return 0;
> > +
> > + return usb_control_msg(dev->udev,
> > + usb_sndctrlpipe(dev->udev, 0),
> > + VRT_RESET_ALL,
> > + USB_DIR_OUT | USB_TYPE_VENDOR | USB_RECIP_DEVICE,
> > + 1,
> > + 1,
> > + device_state,
> > + sizeof(device_state),
> > + 5000);
> > + }
> > + case NRPZ_WRITE_DONE:
>
> EEEEEEK!
>
> > + if (arg) {
> > + ret = wait_event_interruptible_timeout(
> > + dev->out_running.wait,
> > + list_empty(&dev->out_running.urb_list),
> > + msecs_to_jiffies(arg));
> > + if (!ret)
> > + return -ETIMEDOUT;
> > + if (ret < 0)
> > + return ret;
> > + return 0;
> > + } else {
> > + return wait_event_interruptible(
> > + dev->out_running.wait,
> > + list_empty(&dev->out_running.urb_list));
> > + }
> > + break;
>
> This is very ugly. If you need fsync(), then implement it.
>

fsync() did not meat the requirements, since i need in some case a
timeout for the device. poll() will also not help, since it signals only
that there is space to write.

> > + case NRPZ_VENDOR_CONTROL_MSG_OUT:
> > + {
> > + struct nrpz_control_req ncr;
>
> DMA on stack.
>

Fixed in the next version.

> > + u16 size;
> > +
> > + if (copy_from_user(&ncr, (struct nrpz_control_req __user *)arg, sizeof(ncr)))
> > + return -EFAULT;
> > +
> > + if (ncr.data) {
> > + size = ncr.size;
> > +
> > + if (!access_ok(VERIFY_WRITE, (void __user *)ncr.data, size))
> > + return -EFAULT;
> > + } else {
> > + size = 0;
> > + }
> > +
> > + ret = usb_control_msg(dev->udev,
> > + usb_sndctrlpipe(dev->udev, 0),
> > + ncr.request,
> > + ncr.type,
> > + ncr.value,
> > + ncr.index,
> > + ncr.data,
> > + size,
> > + 0);
> > +
> > + return ret;
> > + }
> > + case NRPZ_VENDOR_CONTROL_MSG_IN:
> > + {
> > + struct nrpz_control_req ncr;
>
> DMA on stack
>

Fixed in the next version.

> > + u16 size;
> > +
> > + if (copy_from_user(&ncr, (struct nrpz_control_req __user *)arg, sizeof(ncr)))
> > + return -EFAULT;
> > +
> > + if (ncr.data) {
> > + size = ncr.size;
> > +
> > + if (!access_ok(VERIFY_READ, (void __user *)ncr.data, size))
> > + return -EFAULT;
> > + } else {
> > + size = 0;
> > + }
> > +
> > + ret = usb_control_msg(dev->udev,
> > + usb_rcvctrlpipe(dev->udev, 0),
> > + ncr.request,
> > + ncr.type,
> > + ncr.value,
> > + ncr.index,
> > + ncr.data,
> > + size,
> > + 0);
> > +
> > + return ret;
> > + }
> > + default:
> > + nrpz_err("Invalid ioctl call (%08x)", cmd);
> > + return -EOPNOTSUPP;
> > + }
> > +
> > + return ret;
> > +}
>
> > +static int nrpz_open(struct inode *inode, struct file *file)
> > +{
> > + struct usb_nrpz *dev;
> > + int minor;
> > + struct usb_interface *intf;
> > + int ret;
> > + unsigned i;
> > +
> > + minor = iminor(inode);
> > +
> > + intf = usb_find_interface(&nrpz_driver, minor);
> > + if (!intf) {
> > + nrpz_err("Can not find device for minor %d", minor);
> > + return -ENODEV;
> > + }
> > +
> > + dev = usb_get_intfdata(intf);
> > + if (!dev)
> > + return -ENODEV;
> > +
> > + if (test_and_set_bit(0, &dev->in_use))
> > + return -EBUSY;
> > +
> > + /* increment our usage count for the device */
> > + kref_get(&dev->kref);
> > +
> > + /* save our object in the file's private structure */
> > + file->private_data = dev;
> > +
> > + INIT_LIST_HEAD(&dev->in_avail);
> > + INIT_LIST_HEAD(&dev->out_avail);
> > +
> > + ret = bulks_init(dev,
> > + dev->in_urbs,
> > + ARRAY_SIZE(dev->in_urbs),
> > + usb_rcvbulkpipe(dev->udev, dev->in_epAddr),
> > + dev->in_size,
> > + nrpz_read_callback);
> > + if (ret)
> > + goto error;
> > +
> > + ret = bulks_init(dev,
> > + dev->out_urbs,
> > + ARRAY_SIZE(dev->out_urbs),
> > + usb_sndbulkpipe(dev->udev, dev->out_epAddr),
> > + dev->out_size,
> > + nrpz_write_callback);
> > + if (ret)
> > + goto error;
> > +
> > + for (i = 0; i != ARRAY_SIZE(dev->in_urbs); ++i) {
> > + usb_anchor_urb(&dev->in_urbs[i], &dev->in_running);
> > +
> > + ret = usb_submit_urb(&dev->in_urbs[i], GFP_KERNEL);
>
> You do this here and only here. So this will see a slow attrition if you don't resubmit for som reason.
>

No, i resubmit the urb in the nrpz_read() function.

> > + if (ret) {
> > + usb_kill_anchored_urbs(&dev->in_running);
> > + goto error;
> > + }
> > + }
> > +
> > + for (i = 0; i != ARRAY_SIZE(dev->out_urbs); ++i)
> > + list_add(&dev->out_urbs[i].urb_list, &dev->out_avail);
> > +
> > + return 0;
> > +error:
> > + clear_bit(0, &dev->in_use);
> > +
> > + bulks_release(dev, dev->out_urbs, ARRAY_SIZE(dev->out_urbs),
> > + dev->out_size);
> > + bulks_release(dev, dev->in_urbs, ARRAY_SIZE(dev->in_urbs),
> > + dev->in_size);
> > +
> > + return 0;
> > +}
> > +
> > +static int nrpz_release(struct inode *inode, struct file *file)
> > +{
> > + struct usb_nrpz *dev;
> > +
> > + dev = (struct usb_nrpz *)file->private_data;
> > + if (dev == NULL)
> > + return -ENODEV;
> > +
> > + usb_kill_anchored_urbs(&dev->in_running);
> > + usb_kill_anchored_urbs(&dev->out_running);
> > +
> > + bulks_release(dev, dev->out_urbs, ARRAY_SIZE(dev->out_urbs),
> > + dev->out_size);
> > + bulks_release(dev, dev->in_urbs, ARRAY_SIZE(dev->in_urbs),
> > + dev->in_size);
> > +
> > + /* decrement the count on our device */
> > + kref_put(&dev->kref, nrpz_delete);
> > +
> > + clear_bit(0, &dev->in_use);
> > +
> > + return 0;
> > +}
> > +
> > +static int nrpz_flush(struct file *file, fl_owner_t id)
> > +{
> > + struct usb_nrpz *dev;
> > + int ret;
> > +
> > + dev = (struct usb_nrpz *)file->private_data;
> > + if (dev == NULL)
> > + return -ENODEV;
> > +
> > + /* lock the write data */
> > + ret = mutex_lock_interruptible(&dev->write_mutex);
> > + if (ret)
> > + return ret;
> > +
> > + /* verify that the device wasn't unplugged */
> > + if (!dev->intf) {
> > + ret = -ENODEV;
> > + goto exit;
> > + }
> > +
> > + ret = wait_event_interruptible(dev->out_running.wait,
> > + list_empty(&dev->out_running.urb_list));
> > + if (ret) {
> > + ret = -ERESTARTSYS;
> > + goto exit;
> > + }
> > +exit:
> > + mutex_unlock(&dev->write_mutex);
> > +
> > + return ret;
> > +}
> > +
> > +static unsigned int nrpz_poll(struct file *file, poll_table *wait)
> > +{
> > + struct usb_nrpz *dev;
> > + int ret = 0;
> > +
> > + dev = (struct usb_nrpz *)file->private_data;
> > +
> > + poll_wait(file, &dev->wq, wait);
> > +
> > + if (!dev->intf)
> > + ret = POLLIN | POLLOUT | POLLPRI | POLLERR | POLLHUP;
> > + else {
> > + if (!list_empty(&dev->in_avail))
> > + ret |= POLLIN;
> > +
> > + if (!list_empty(&dev->out_avail))
> > + ret |= POLLOUT;
> > + }
> > +
> > + return ret;
> > +}
> > +
> > +static const struct file_operations nrpz_fops = {
> > + .owner = THIS_MODULE,
> > + .read = nrpz_read,
> > + .write = nrpz_write,
> > + .unlocked_ioctl = nrpz_ioctl,
> > + .compat_ioctl = nrpz_compat_ioctl,
> > + .open = nrpz_open,
> > + .release = nrpz_release,
> > + .flush = nrpz_flush,
> > + .poll = nrpz_poll,
> > + .llseek = noop_llseek,
> > +};
> > +
> > +static struct usb_class_driver nrpz_class = {
> > + .name = "nrpz%d",
> > + .fops = &nrpz_fops,
> > + .minor_base = NRPZ_MINOR_BASE,
> > +};
> > +
> > +static int nrpz_probe(struct usb_interface *intf,
> > + const struct usb_device_id *id)
> > +{
> > + int i;
> > + int ret;
> > + struct usb_endpoint_descriptor *endpoint;
> > + struct usb_host_interface *iface_desc;
> > + struct usb_nrpz *dev;
> > +
> > + /* allocate memory for our device state and intialize it */
> > + dev = kzalloc(sizeof(*dev), GFP_KERNEL);
> > + if (!dev) {
> > + nrpz_err("Out of memory");
> > + return -ENOMEM;
> > + }
> > +
> > + ret = -EIO;
> > +
> > + init_waitqueue_head(&dev->wq);
> > + kref_init(&dev->kref);
> > +
> > + mutex_init(&dev->read_mutex);
> > + mutex_init(&dev->write_mutex);
> > +
> > + spin_lock_init(&dev->read_lock);
> > + spin_lock_init(&dev->write_lock);
> > +
> > + init_usb_anchor(&dev->in_running);
> > + init_usb_anchor(&dev->out_running);
> > +
> > + dev->in_use = 0;
> > + dev->udev = usb_get_dev(interface_to_usbdev(intf));
> > + dev->intf = intf;
> > +
> > + /* set up the endpoint information */
> > + /* check out the endpoints */
> > + iface_desc = intf->cur_altsetting;
> > + for (i = 0; i < iface_desc->desc.bNumEndpoints; ++i) {
> > + endpoint = &iface_desc->endpoint[i].desc;
> > +
> > + if (!dev->in_epAddr && usb_endpoint_is_bulk_in(endpoint)) {
> > + /* we found a bulk in endpoint */
> > + dev->in_size = le16_to_cpu(endpoint->wMaxPacketSize);
> > + dev->in_epAddr = endpoint->bEndpointAddress;
> > + } else
> > + if (!dev->out_epAddr && usb_endpoint_is_bulk_out(endpoint)) {
> > + /* we found a bulk out endpoint */
> > + dev->out_size = le16_to_cpu(endpoint->wMaxPacketSize);
> > + dev->out_epAddr = endpoint->bEndpointAddress;
> > + }
> > + }
> > + if (!(dev->in_epAddr && dev->out_epAddr)) {
> > + nrpz_err("Could not find both bulk in and out endpoints");
> > + goto error;
> > + }
> > +
> > + usb_set_intfdata(intf, dev);
> > +
> > + ret = usb_register_dev(intf, &nrpz_class);
> > + if (ret) {
> > + nrpz_err("Not able to get a minor for this device\n");
> > + goto error;
> > + }
> > +
> > + dev->minor = intf->minor - NRPZ_MINOR_BASE;
> > +
> > + /* let the user know what node this device is now attached to */
> > + nrpz_info("Device now attached to USB nrpz%u", dev->minor);
> > +
> > + return 0;
> > +error:
> > + usb_set_intfdata(intf, NULL);
> > + nrpz_delete(&dev->kref);
> > + return ret;
> > +}
> > +
> > +static void nrpz_disconnect(struct usb_interface *intf)
> > +{
> > + struct usb_nrpz *dev;
> > + unsigned minor;
> > +
> > + dev = usb_get_intfdata(intf);
> > + usb_set_intfdata(intf, NULL);
> > +
> > + minor = dev->minor;
> > +
> > + /* give back our minor */
> > + usb_deregister_dev(intf, &nrpz_class);
> > +
> > + /* prevent more I/O from starting */
> > + dev->intf = NULL;
> > +
> > + usb_kill_anchored_urbs(&dev->in_running);
> > + usb_kill_anchored_urbs(&dev->out_running);
> > +
> > + wake_up_all(&dev->wq);
> > +
> > + /* decrement our usage count */
> > + kref_put(&dev->kref, nrpz_delete);
> > +
> > + nrpz_info("nrpz%u now disconnected", minor);
> > +}
> > +
> > +static void nrpz_draw_down(struct usb_nrpz *dev)
> > +{
> > + int time;
> > +
> > + time = usb_wait_anchor_empty_timeout(&dev->out_running, 1000);
> > + if (!time)
> > + usb_kill_anchored_urbs(&dev->out_running);
> > +
> > + usb_kill_anchored_urbs(&dev->in_running);
> > +}
> > +
> > +static int nrpz_suspend(struct usb_interface *intf, pm_message_t message)
> > +{
> > + struct usb_nrpz *dev = usb_get_intfdata(intf);
> > +
> > + if (dev)
> > + nrpz_draw_down(dev);
> > + return 0;
> > +}
> > +
> > +static int nrpz_resume(struct usb_interface *intf)
> > +{
> > + return 0;
> > +}
>
> And what restarts reading?
>

Fixed in the next version.

> > +
> > +static int nrpz_pre_reset(struct usb_interface *intf)
> > +{
> > + struct usb_nrpz *dev = usb_get_intfdata(intf);
> > +
> > + if (dev)
> > + nrpz_draw_down(dev);
> > + return 0;
> > +}
> > +
> > +static int nrpz_post_reset(struct usb_interface *intf)
> > +{
> > + return 0;
> > +}
>
> And you don't tell user space that the device has been reset?
> And what restarts reading?
>

I have no idea how to do this. But i will have a look in the kernel
source and figure it out.

Thanks again and best regards,
Stefani


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


oneukum at suse

May 31, 2012, 1:20 AM

Post #5 of 28 (661 views)
Permalink
Re: [PATCH] add new NRP power meter USB device driver [In reply to]

Am Donnerstag, 31. Mai 2012, 09:43:41 schrieb Stefani Seibold:
> Hi Oliver,
>
> Thanks for the review.
>
> On Wed, 2012-05-30 at 10:10 +0200, Oliver Neukum wrote:
> > Am Dienstag, 29. Mai 2012, 21:14:18 schrieb stefani [at] seibold:
> >
> > > The device will be controlled by a SCPI like interface.
> >
> > Are there other devices using that standard whose interface you might reuse?
> >
> >
>
> There are a lot of instruments from many different manufacturer which
> use the SCPI standard. But there is now way to bring them together. The
> commands are well defined, but there are a lot of vendor specific
> commands. And the communication path are differing, which can be
> IEEE488, USB, Socket, Serial and more.

This is unfortunate. It would be much better if we could have a /dev/spiX
in form of a character device.

> > > + if (copy_to_user(buffer, urb->transfer_buffer, n)) {
> > > + urb_list_add(&dev->read_lock, urb, &dev->in_avail);
> > > + ret = -EFAULT;
> > > + goto exit;
> >
> > Is there a good reason you don't resubmit in this case?
> >
>
> The data was not successfully transfered to the user space, so it will
> stay again on the top of the input urb list.

I see. Unusual approach, but perfectly valid.

> > > +static long nrpz_compat_ioctl(struct file *file, unsigned int cmd,
> > > + unsigned long arg)
> > > +{
> > > + struct usb_nrpz *dev;
> > > + int ret;
> > > +
> > > + dev = (struct usb_nrpz *)file->private_data;
> > > +
> > > + /* verify that the device wasn't unplugged */
> > > + if (!dev->intf)
> > > + return -ENODEV;
> >
> > Locking?
> >
>
> I see no reason why. But i will change it in the next version.

1. Multithreaded tasks
2. Check for disconnect

> > > + case NRPZ_START:
> > > + {
> > > + u8 device_state[128];
> >
> > DMA on stack. This may ail on some architectures. You need to use
> > kmalloc() or better kzalloc().
> >
>
> Never heard about this problem. The driver will work since years on X86,
> PPC and ARM platforms. But it will be fixes in the next version.

On PPC you were lucky. However the window for the race is small.
In a nutshell, your buffer may share a cacheline with something else
on the stack. If you read that something else on some architectures
DMA after that will not invalidate the cache line and the CPU will see
the old content of the stack before the DMA in the buffer.

And on a few architectures (ia64, ... IIRC) the stack may be incapable
of DMA at all.

> > > + case NRPZ_WRITE_DONE:
> >
> > EEEEEEK!
> >
> > > + if (arg) {
> > > + ret = wait_event_interruptible_timeout(
> > > + dev->out_running.wait,
> > > + list_empty(&dev->out_running.urb_list),
> > > + msecs_to_jiffies(arg));
> > > + if (!ret)
> > > + return -ETIMEDOUT;
> > > + if (ret < 0)
> > > + return ret;
> > > + return 0;
> > > + } else {
> > > + return wait_event_interruptible(
> > > + dev->out_running.wait,
> > > + list_empty(&dev->out_running.urb_list));
> > > + }
> > > + break;
> >
> > This is very ugly. If you need fsync(), then implement it.
> >
>
> fsync() did not meat the requirements, since i need in some case a
> timeout for the device. poll() will also not help, since it signals only
> that there is space to write.

Well, then implement fsync() with interruptible sleep and use a timer
in user space.

> > > +
> > > + ret = bulks_init(dev,
> > > + dev->out_urbs,
> > > + ARRAY_SIZE(dev->out_urbs),
> > > + usb_sndbulkpipe(dev->udev, dev->out_epAddr),
> > > + dev->out_size,
> > > + nrpz_write_callback);
> > > + if (ret)
> > > + goto error;
> > > +
> > > + for (i = 0; i != ARRAY_SIZE(dev->in_urbs); ++i) {
> > > + usb_anchor_urb(&dev->in_urbs[i], &dev->in_running);
> > > +
> > > + ret = usb_submit_urb(&dev->in_urbs[i], GFP_KERNEL);
> >
> > You do this here and only here. So this will see a slow attrition if you don't resubmit for som reason.
> >
>
> No, i resubmit the urb in the nrpz_read() function.

Yes, but this seems to be buggy:

+ ret = usb_submit_urb(urb, GFP_KERNEL);
+ if (ret) {
+ usb_unanchor_urb(urb);
+ urb_list_add_tail(&dev->read_lock, urb, &dev->in_avail);
+ nrpz_err("Failed submitting read urb (error %d)", ret);
+ }

You have already transfered the data to user space. It seems to me that you
need to zero out the URB and need to handle the case of getting an URB
without data.

> > > +static int nrpz_pre_reset(struct usb_interface *intf)
> > > +{
> > > + struct usb_nrpz *dev = usb_get_intfdata(intf);
> > > +
> > > + if (dev)
> > > + nrpz_draw_down(dev);
> > > + return 0;
> > > +}
> > > +
> > > +static int nrpz_post_reset(struct usb_interface *intf)
> > > +{
> > > + return 0;
> > > +}
> >
> > And you don't tell user space that the device has been reset?
> > And what restarts reading?
> >
>
> I have no idea how to do this. But i will have a look in the kernel
> source and figure it out.

There probably is no generic answer. But I presume a reset will
reinit the device and destroy anything you set up before, so I guess
the next read() or write() after a reset has to return an error code that
tells user space that it has to redo its setup.

Regards
Oliver
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


stefani at seibold

May 31, 2012, 1:47 AM

Post #6 of 28 (662 views)
Permalink
Re: [PATCH] add new NRP power meter USB device driver [In reply to]

Hi Greg,

thanks for review the code.

On Wed, 2012-05-30 at 07:15 +0800, Greg KH wrote:
> On Tue, May 29, 2012 at 09:14:18PM +0200, stefani [at] seibold wrote:
> > From: Stefani Seibold <stefani [at] seibold>
> >
> > This driver supports all of the Rohde&Schwarz RF Power Meter NRP Sensors. These
> > sensors are intelligent standalone instruments that communicate via USB.
> >
> > A power meter is a device for analyzing the RF power output level of an
> > electrical device, similar to an oscilloscope.
>
> Nice, but why is this a kernel driver? Can't you control this device
> from userspace using libusb/usbfs?
>

I have expected this objection. This driver has a long history over 10
years. At this time, there was no libudev and libsysfs. And libusb was
not capable to handle multi urb request nor handling multi devices.

So it is a chicken and egg problem. A lot of software is now out with
depends on this driver and a lot of embedded development environments
doesn't provide libudev, libsysfs und libusb. It will also increase the
size of the image and adds additional dependencies to the application.

Also kernel space is faster and this is a key factor.

And it possible to simple handling the device from script using printf,
echo, cat and so on.

At least it is much more easier and straight forward to do this in
kernel space, than using libudev, libsysfs and libusb.

This devices are very powerfull but also very special and can not
handled in an easy way,

The question why is the a kernel driver can be asked by many kernel
driver.

> > + * History:
> > + *
> > + * 2012_05_18 - 4.0 - revamp for kernel inclusion
> > + * 2007_05_15 - 2.0 - Ported the driver to kernel 2.6, mostly rewritten
> > + * 2001_05_01 - 0.1 - first version
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License as published by
> > + * the Free Software Foundation; either version 2 of the License, or
> > + * (at your option) any later version.
>
> Are you sure "any later version"?
>

Fixed in the next version.

> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> > + * GNU General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU General Public License
> > + * along with this program; if not, write to the Free Software
> > + * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
>
> This paragraph is never needed, unless you want to track the movements
> of the FSF's office for the next 40 years.
>

You are right, will be fixed in the version.

> > +
> > +#define nrpz_dbg(format, arg...) \
> > + do { if (debug) printk(KERN_DEBUG "nrpz: " format "\n", ##arg); } while (0)
> > +#define nrpz_err(format, arg...) \
> > + do { printk(KERN_ERR "nrpz: " format "\n", ##arg); } while (0)
> > +#define nrpz_info(format, arg...) \
> > + do { printk(KERN_INFO "nrpz: " format "\n", ##arg); } while (0)
>
> Ick, no, please use dev_dbg(), dev_err(), and dev_info() instead, don't
> create your own macros.
>

Fixed in the next version.

> > +/* Version Information */
> > +#define DRIVER_VERSION "v4.00"
>
> What happened to the first 3 versions of this code? :)
>

* 2012_05_18 - 4.0 - revamp for kernel inclusion
* 2007_12_07 - 3.0 - Rewritten, multi urbs for read and write
* 2007_05_15 - 2.0 - Ported the driver to kernel 2.6, mostly rewritten
* 2001_05_01 - 0.1 - first version

The driver have a long history, the first version was developed for
kernel 2.4. Since then it was adapted to the newest kernel version and
was optimized in performance and size.

> > +#define DRIVER_AUTHOR "Stefani Seibold <stefani [at] seibold>"
> > +#define DRIVER_DESC "Rohde&Schwarz NRP-Zxx USB Powermeter"
> > +
> > +/* Get a minor range for your devices from the usb maintainer */
> > +#define NRPZ_MINOR_BASE 192
>
> You didn't ask me for a minor base, did you?
>

Sorry, that was one checkpoint in my ToDo List which i have lost. It
would be kind if you can assign me a minor range for the NRPZ devices.

> > +#define to_nrpz_dev(d) container_of(d, struct usb_nrpz, kref)
> > +#define list_to_urb(d) container_of(d, struct urb, urb_list)
>
> Make these inline functions instead?
>

Fixed in the next version.

> > +
> > +/* Define these values to match your device */
>
> This comment can be fixed, right?
>

Copy and past :-( Fixed!

> > +MODULE_DEVICE_TABLE(usb, nrpz_table);
> > +
> > +/* Structure to hold all of our device specific stuff */
> > +struct usb_nrpz {
> > + struct usb_device *udev;
> > + struct usb_interface *intf;
>

I tried to remove the udev entry in the next version. I was successfully
in the most cases, but i find no solution for the nrpz_delete()
function, which needs the udev for the usb_put_dev() call. Since intf is
at this time NULL, there is no way to determinate the struct usb_device
pointer.

> Do you really need both?
>
> > + unsigned minor; /* minor number for this device */
> > + unsigned long in_use; /* in use flag */
>
> Why a long for this?
>

test_and_set_bit() expect a unsigned long. This kind of "in use"
handling is not uncommon in a kernel driver.


> > +
> > +static inline void urb_list_add_tail(spinlock_t *lock, struct urb *urb,
> > + struct list_head *head)
> > +{
> > + spin_lock_irq(lock);
> > + list_add_tail(&urb->urb_list, head);
> > + spin_unlock_irq(lock);
> > +}
>
> Why the lists of urbs? Why not either just create them all dynamically
> all the time, or use urb anchors?
>

Anchors will be used for the running urbs. The callback will then
collect the handled urbs in a list and read() and write() will resubmit
this urbs after processing.

Dynamically creation is an option, but i prefer this way, since it is
faster, safer and has less error handling.

> > + case NRPZ_GETSENSORINFO:
> > + {
> > + struct nrpz_sensor_info __user *sensor_info =
> > + (struct nrpz_sensor_info __user *)arg;
> > +
> > + if (!access_ok(VERIFY_WRITE, sensor_info, sizeof(*sensor_info)))
> > + return -EFAULT;
> > +
> > + __put_user(dev->udev->descriptor.bcdDevice,
> > + &sensor_info->bcdDevice);
> > + __put_user(dev->udev->descriptor.bcdUSB,
> > + &sensor_info->bcdUSB);
> > + __put_user(dev->udev->descriptor.bDescriptorType,
> > + &sensor_info->bDescriptorType);
> > + __put_user(dev->udev->descriptor.bDeviceClass,
> > + &sensor_info->bDeviceClass);
> > + __put_user(dev->udev->descriptor.bDeviceSubClass,
> > + &sensor_info->bDeviceSubClass);
> > + __put_user(dev->udev->descriptor.bDeviceProtocol,
> > + &sensor_info->bDeviceProtocol);
> > + __put_user(dev->udev->descriptor.bMaxPacketSize0,
> > + &sensor_info->bMaxPacketSize0);
> > + __put_user(dev->udev->descriptor.bNumConfigurations,
> > + &sensor_info->bNumConfigurations);
> > + __put_user(dev->udev->descriptor.iManufacturer,
> > + &sensor_info->iManufacturer);
> > + __put_user(dev->udev->descriptor.iProduct,
> > + &sensor_info->iProduct);
> > + __put_user(dev->udev->descriptor.iSerialNumber,
> > + &sensor_info->iSerialNumber);
> > + __put_user(dev->udev->descriptor.idVendor,
> > + &sensor_info->vendorId);
> > + __put_user(dev->udev->descriptor.idProduct,
> > + &sensor_info->productId);
> > + usb_string(dev->udev, dev->udev->descriptor.iManufacturer,
> > + (char __force *)sensor_info->manufacturer,
> > + sizeof(sensor_info->manufacturer));
> > + usb_string(dev->udev, dev->udev->descriptor.iProduct,
> > + (char __force *)sensor_info->productName,
> > + sizeof(sensor_info->productName));
> > + usb_string(dev->udev, dev->udev->descriptor.iSerialNumber,
> > + (char __force *)sensor_info->serialNumber,
> > + sizeof(sensor_info->serialNumber));
>
> Why are you sending this information through an ioctl, when it is
> already exported in sysfs? That seems quite needless.
>

This is very easy to do in the driver and would create a lot of code in
the user space using libsysfs and depends on additional libraries.

> Anyway, I think the main questions are, "Why is this a kernel driver",
> and "If this is a kernel driver, why not use the IIO interface"?
>

The IIO interface is an option, but not for the Rohde&Schwarz company,
since the driver is in use by our customers for 10 years.

I also not sure that the IIO interface can handle all of the features of
the power meter.

I have expected this objection "why is this a kernel driver?".

It took me years to convince the company to provide open source driver
and libraries. This is the first result of this effort.

Since the driver is very tiny and straight forward and has no side
effects with other parts to the system it would be great to add it to
the kernel drivers.

This work should not done for the trash bin.

Thanks for the review.

Regards,
Stefani


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


stefani at seibold

May 31, 2012, 2:21 AM

Post #7 of 28 (658 views)
Permalink
Re: [PATCH] add new NRP power meter USB device driver [In reply to]

On Thu, 2012-05-31 at 10:20 +0200, Oliver Neukum wrote:

> > >
> > > > + if (arg) {
> > > > + ret = wait_event_interruptible_timeout(
> > > > + dev->out_running.wait,
> > > > + list_empty(&dev->out_running.urb_list),
> > > > + msecs_to_jiffies(arg));
> > > > + if (!ret)
> > > > + return -ETIMEDOUT;
> > > > + if (ret < 0)
> > > > + return ret;
> > > > + return 0;
> > > > + } else {
> > > > + return wait_event_interruptible(
> > > > + dev->out_running.wait,
> > > > + list_empty(&dev->out_running.urb_list));
> > > > + }
> > > > + break;
> > >
> > > This is very ugly. If you need fsync(), then implement it.
> > >
> >
> > fsync() did not meat the requirements, since i need in some case a
> > timeout for the device. poll() will also not help, since it signals only
> > that there is space to write.
>
> Well, then implement fsync() with interruptible sleep and use a timer
> in user space.
>

But this will not solve the problem of older software which is still
depending on this ioctl.


> Yes, but this seems to be buggy:
>
> + ret = usb_submit_urb(urb, GFP_KERNEL);
> + if (ret) {
> + usb_unanchor_urb(urb);
> + urb_list_add_tail(&dev->read_lock, urb, &dev->in_avail);
> + nrpz_err("Failed submitting read urb (error %d)", ret);
> + }
>
> You have already transfered the data to user space. It seems to me that you
> need to zero out the URB and need to handle the case of getting an URB
> without data.
>

Okay, i understand what you mean. Zeroing out is not necessary since
usb_submit_urb will set urb->status to -EINPROGRESS. This behavior is
well documented.

I checked it again, and it will work perfectly in case of an error.

And in my opinion it is safe to reuse an urb without reinitialization,
this is a common practice in the callback handlers, so i see no reason
why this should not work in the read() function.

> > > > +static int nrpz_pre_reset(struct usb_interface *intf)
> > > > +{
> > > > + struct usb_nrpz *dev = usb_get_intfdata(intf);
> > > > +
> > > > + if (dev)
> > > > + nrpz_draw_down(dev);
> > > > + return 0;
> > > > +}
> > > > +
> > > > +static int nrpz_post_reset(struct usb_interface *intf)
> > > > +{
> > > > + return 0;
> > > > +}
> > >
> > > And you don't tell user space that the device has been reset?
> > > And what restarts reading?
> > >
> >
> > I have no idea how to do this. But i will have a look in the kernel
> > source and figure it out.
>
> There probably is no generic answer. But I presume a reset will
> reinit the device and destroy anything you set up before, so I guess
> the next read() or write() after a reset has to return an error code that
> tells user space that it has to redo its setup.
>

Is it okay to kick out the whole ..._reset() thing, since i have no idea
what it is good for.

Greetings,
Stefani


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


gregkh at linuxfoundation

May 31, 2012, 2:41 AM

Post #8 of 28 (663 views)
Permalink
Re: [PATCH] add new NRP power meter USB device driver [In reply to]

On Thu, May 31, 2012 at 10:47:21AM +0200, Stefani Seibold wrote:
> Hi Greg,
>
> thanks for review the code.
>
> On Wed, 2012-05-30 at 07:15 +0800, Greg KH wrote:
> > On Tue, May 29, 2012 at 09:14:18PM +0200, stefani [at] seibold wrote:
> > > From: Stefani Seibold <stefani [at] seibold>
> > >
> > > This driver supports all of the Rohde&Schwarz RF Power Meter NRP Sensors. These
> > > sensors are intelligent standalone instruments that communicate via USB.
> > >
> > > A power meter is a device for analyzing the RF power output level of an
> > > electrical device, similar to an oscilloscope.
> >
> > Nice, but why is this a kernel driver? Can't you control this device
> > from userspace using libusb/usbfs?
> >
>
> I have expected this objection. This driver has a long history over 10
> years. At this time, there was no libudev and libsysfs. And libusb was
> not capable to handle multi urb request nor handling multi devices.

Wait, we have had usbfs since the 2.2 kernel days, from the very
beginning of the USB subsystem, so this argument doesn't really fly.
You don't need libsysfs (which is dead, and also, over 8 years old), and
libudev doesn't seem to be needed here either.

multi-device handling for libusb has been there from the beginning (over
10 years ago) and multi-urb handling has been around for a very long
time as well.

So, sorry, I don't buy this argument :)

> So it is a chicken and egg problem. A lot of software is now out with
> depends on this driver and a lot of embedded development environments
> doesn't provide libudev, libsysfs und libusb. It will also increase the
> size of the image and adds additional dependencies to the application.

You don't need any of those libraries to do this. What's wrong with
using "raw" usbfs interactions?

> Also kernel space is faster and this is a key factor.

I don't buy it.

How much faster? What numbers do you have?

We have USB vision systems running at wire speeds using usbfs and
libusb, and have had that since the early 2.4 kernel days. Speed is not
a valid argument here, unless you have some really bad userspace code.

> And it possible to simple handling the device from script using printf,
> echo, cat and so on.

Not really a valid argument, sorry.

> At least it is much more easier and straight forward to do this in
> kernel space, than using libudev, libsysfs and libusb.
>
> This devices are very powerfull but also very special and can not
> handled in an easy way,

I don't believe it, you are going to have to prove this somehow, sorry.
Looking at your code, it is very simple, and to write a userspace
program to control these would even be less code that the kernel driver.

> The question why is the a kernel driver can be asked by many kernel
> driver.

Which ones? We have ripped out lots of USB drivers that could have been
controlled by usbfs userspace programs over the years for this same
reason. If we missed any, please let me know and I will rectify that.

> > > + * History:
> > > + *
> > > + * 2012_05_18 - 4.0 - revamp for kernel inclusion
> > > + * 2007_05_15 - 2.0 - Ported the driver to kernel 2.6, mostly rewritten
> > > + * 2001_05_01 - 0.1 - first version
> > > + *
> > > + * This program is free software; you can redistribute it and/or modify
> > > + * it under the terms of the GNU General Public License as published by
> > > + * the Free Software Foundation; either version 2 of the License, or
> > > + * (at your option) any later version.
> >
> > Are you sure "any later version"?
> >
>
> Fixed in the next version.
>
> > > + *
> > > + * This program is distributed in the hope that it will be useful,
> > > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> > > + * GNU General Public License for more details.
> > > + *
> > > + * You should have received a copy of the GNU General Public License
> > > + * along with this program; if not, write to the Free Software
> > > + * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
> >
> > This paragraph is never needed, unless you want to track the movements
> > of the FSF's office for the next 40 years.
> >
>
> You are right, will be fixed in the version.
>
> > > +
> > > +#define nrpz_dbg(format, arg...) \
> > > + do { if (debug) printk(KERN_DEBUG "nrpz: " format "\n", ##arg); } while (0)
> > > +#define nrpz_err(format, arg...) \
> > > + do { printk(KERN_ERR "nrpz: " format "\n", ##arg); } while (0)
> > > +#define nrpz_info(format, arg...) \
> > > + do { printk(KERN_INFO "nrpz: " format "\n", ##arg); } while (0)
> >
> > Ick, no, please use dev_dbg(), dev_err(), and dev_info() instead, don't
> > create your own macros.
> >
>
> Fixed in the next version.
>
> > > +/* Version Information */
> > > +#define DRIVER_VERSION "v4.00"
> >
> > What happened to the first 3 versions of this code? :)
> >
>
> * 2012_05_18 - 4.0 - revamp for kernel inclusion
> * 2007_12_07 - 3.0 - Rewritten, multi urbs for read and write
> * 2007_05_15 - 2.0 - Ported the driver to kernel 2.6, mostly rewritten
> * 2001_05_01 - 0.1 - first version
>
> The driver have a long history, the first version was developed for
> kernel 2.4. Since then it was adapted to the newest kernel version and
> was optimized in performance and size.

Ok, but what does that really matter for a kernel driver? What "value"
is it for this to be here?

> > > +#define DRIVER_AUTHOR "Stefani Seibold <stefani [at] seibold>"
> > > +#define DRIVER_DESC "Rohde&Schwarz NRP-Zxx USB Powermeter"
> > > +
> > > +/* Get a minor range for your devices from the usb maintainer */
> > > +#define NRPZ_MINOR_BASE 192
> >
> > You didn't ask me for a minor base, did you?
> >
>
> Sorry, that was one checkpoint in my ToDo List which i have lost. It
> would be kind if you can assign me a minor range for the NRPZ devices.

Not yet, sorry, you need to convince me that this really does need to be
a kernel driver.

> > > +MODULE_DEVICE_TABLE(usb, nrpz_table);
> > > +
> > > +/* Structure to hold all of our device specific stuff */
> > > +struct usb_nrpz {
> > > + struct usb_device *udev;
> > > + struct usb_interface *intf;
> >
>
> I tried to remove the udev entry in the next version. I was successfully
> in the most cases, but i find no solution for the nrpz_delete()
> function, which needs the udev for the usb_put_dev() call. Since intf is
> at this time NULL, there is no way to determinate the struct usb_device
> pointer.

Why would intf be NULL at some point in time when you really need to
find this out?

> > Do you really need both?
> >
> > > + unsigned minor; /* minor number for this device */
> > > + unsigned long in_use; /* in use flag */
> >
> > Why a long for this?
> >
>
> test_and_set_bit() expect a unsigned long. This kind of "in use"
> handling is not uncommon in a kernel driver.

Don't confuse that with a valid locking primitive. Please just use a
bool or char instead.

> > > +
> > > +static inline void urb_list_add_tail(spinlock_t *lock, struct urb *urb,
> > > + struct list_head *head)
> > > +{
> > > + spin_lock_irq(lock);
> > > + list_add_tail(&urb->urb_list, head);
> > > + spin_unlock_irq(lock);
> > > +}
> >
> > Why the lists of urbs? Why not either just create them all dynamically
> > all the time, or use urb anchors?
> >
>
> Anchors will be used for the running urbs. The callback will then
> collect the handled urbs in a list and read() and write() will resubmit
> this urbs after processing.
>
> Dynamically creation is an option, but i prefer this way, since it is
> faster, safer and has less error handling.

How do you know it is faster? Has it been measured? How? What was the
results? Why is it less error handling? Create, submit, and
automatically have the USB core destroy the urb is simpler and less code
in a driver than this logic here, right? If not, then we did something
wrong a long time ago.

> > > + case NRPZ_GETSENSORINFO:
> > > + {
> > > + struct nrpz_sensor_info __user *sensor_info =
> > > + (struct nrpz_sensor_info __user *)arg;
> > > +
> > > + if (!access_ok(VERIFY_WRITE, sensor_info, sizeof(*sensor_info)))
> > > + return -EFAULT;
> > > +
> > > + __put_user(dev->udev->descriptor.bcdDevice,
> > > + &sensor_info->bcdDevice);
> > > + __put_user(dev->udev->descriptor.bcdUSB,
> > > + &sensor_info->bcdUSB);
> > > + __put_user(dev->udev->descriptor.bDescriptorType,
> > > + &sensor_info->bDescriptorType);
> > > + __put_user(dev->udev->descriptor.bDeviceClass,
> > > + &sensor_info->bDeviceClass);
> > > + __put_user(dev->udev->descriptor.bDeviceSubClass,
> > > + &sensor_info->bDeviceSubClass);
> > > + __put_user(dev->udev->descriptor.bDeviceProtocol,
> > > + &sensor_info->bDeviceProtocol);
> > > + __put_user(dev->udev->descriptor.bMaxPacketSize0,
> > > + &sensor_info->bMaxPacketSize0);
> > > + __put_user(dev->udev->descriptor.bNumConfigurations,
> > > + &sensor_info->bNumConfigurations);
> > > + __put_user(dev->udev->descriptor.iManufacturer,
> > > + &sensor_info->iManufacturer);
> > > + __put_user(dev->udev->descriptor.iProduct,
> > > + &sensor_info->iProduct);
> > > + __put_user(dev->udev->descriptor.iSerialNumber,
> > > + &sensor_info->iSerialNumber);
> > > + __put_user(dev->udev->descriptor.idVendor,
> > > + &sensor_info->vendorId);
> > > + __put_user(dev->udev->descriptor.idProduct,
> > > + &sensor_info->productId);
> > > + usb_string(dev->udev, dev->udev->descriptor.iManufacturer,
> > > + (char __force *)sensor_info->manufacturer,
> > > + sizeof(sensor_info->manufacturer));
> > > + usb_string(dev->udev, dev->udev->descriptor.iProduct,
> > > + (char __force *)sensor_info->productName,
> > > + sizeof(sensor_info->productName));
> > > + usb_string(dev->udev, dev->udev->descriptor.iSerialNumber,
> > > + (char __force *)sensor_info->serialNumber,
> > > + sizeof(sensor_info->serialNumber));
> >
> > Why are you sending this information through an ioctl, when it is
> > already exported in sysfs? That seems quite needless.
> >
>
> This is very easy to do in the driver and would create a lot of code in
> the user space using libsysfs and depends on additional libraries.

You can't do a simple 'cat'? Sorry, this is duplication, even if I
could accept this driver, I can't accept this.

> > Anyway, I think the main questions are, "Why is this a kernel driver",
> > and "If this is a kernel driver, why not use the IIO interface"?
> >
>
> The IIO interface is an option, but not for the Rohde&Schwarz company,
> since the driver is in use by our customers for 10 years.

Ok, but I'm not that company, and neither is Linux. We strive to create
unified userspace apis that all drivers can use to ensure device
independance (hint, that's what an OS is for.) Creating one-off
user/kernel apis, like this one, is really special syscalls just for
this specific piece of hardware. Which is something we do not do
lightly at all.

> I also not sure that the IIO interface can handle all of the features of
> the power meter.

Then the IIO interface needs to be extended to do so. Seriously, that's
the only way I could see this being a kernel driver, not with this
custom ioctl interface.

> I have expected this objection "why is this a kernel driver?".
>
> It took me years to convince the company to provide open source driver
> and libraries. This is the first result of this effort.

All USB Linux drivers have to be open, this shouldn't have been any
effort at all :)

> Since the driver is very tiny and straight forward and has no side
> effects with other parts to the system it would be great to add it to
> the kernel drivers.
>
> This work should not done for the trash bin.

I understand your investment in this. But understand ours as well. You
are creating a hardware-specific set of custom system calls that no one
else can, or will, use. You also duplicate interfaces that we have
already standardized on (the USB device information in sysfs and in
usbfs). Why would that be acceptable? If you were in my situation,
would you accept this type of duplication?

So, I suggest you either use usbfs from a userspace program or library,
or if you insist on a kernel driver, use the IIO interface, extending it
where needed. As it is, in the current form, I can not accept this
driver for the above reasons.

thanks,

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


gregkh at linuxfoundation

May 31, 2012, 2:53 AM

Post #9 of 28 (660 views)
Permalink
Re: [PATCH] add new NRP power meter USB device driver [In reply to]

On Thu, May 31, 2012 at 10:47:21AM +0200, Stefani Seibold wrote:
> Hi Greg,
>
> thanks for review the code.
>
> On Wed, 2012-05-30 at 07:15 +0800, Greg KH wrote:
> > On Tue, May 29, 2012 at 09:14:18PM +0200, stefani [at] seibold wrote:
> > > From: Stefani Seibold <stefani [at] seibold>
> > >
> > > This driver supports all of the Rohde&Schwarz RF Power Meter NRP Sensors. These
> > > sensors are intelligent standalone instruments that communicate via USB.
> > >
> > > A power meter is a device for analyzing the RF power output level of an
> > > electrical device, similar to an oscilloscope.
> >
> > Nice, but why is this a kernel driver? Can't you control this device
> > from userspace using libusb/usbfs?
> >
>
> I have expected this objection. This driver has a long history over 10
> years. At this time, there was no libudev and libsysfs. And libusb was
> not capable to handle multi urb request nor handling multi devices.

Wait, we have had usbfs since the 2.2 kernel days, from the very
beginning of the USB subsystem, so this argument doesn't really fly.
You don't need libsysfs (which is dead, and also, over 8 years old), and
libudev doesn't seem to be needed here either.

multi-device handling for libusb has been there from the beginning (over
10 years ago) and multi-urb handling has been around for a very long
time as well.

So, sorry, I don't buy this argument :)

> So it is a chicken and egg problem. A lot of software is now out with
> depends on this driver and a lot of embedded development environments
> doesn't provide libudev, libsysfs und libusb. It will also increase the
> size of the image and adds additional dependencies to the application.

You don't need any of those libraries to do this. What's wrong with
using "raw" usbfs interactions?

> Also kernel space is faster and this is a key factor.

I don't buy it.

How much faster? What numbers do you have?

We have USB vision systems running at wire speeds using usbfs and
libusb, and have had that since the early 2.4 kernel days. Speed is not
a valid argument here, unless you have some really bad userspace code.

> And it possible to simple handling the device from script using printf,
> echo, cat and so on.

Not really a valid argument, sorry.

> At least it is much more easier and straight forward to do this in
> kernel space, than using libudev, libsysfs and libusb.
>
> This devices are very powerfull but also very special and can not
> handled in an easy way,

I don't believe it, you are going to have to prove this somehow, sorry.
Looking at your code, it is very simple, and to write a userspace
program to control these would even be less code that the kernel driver.

> The question why is the a kernel driver can be asked by many kernel
> driver.

Which ones? We have ripped out lots of USB drivers that could have been
controlled by usbfs userspace programs over the years for this same
reason. If we missed any, please let me know and I will rectify that.

> > > + * History:
> > > + *
> > > + * 2012_05_18 - 4.0 - revamp for kernel inclusion
> > > + * 2007_05_15 - 2.0 - Ported the driver to kernel 2.6, mostly rewritten
> > > + * 2001_05_01 - 0.1 - first version
> > > + *
> > > + * This program is free software; you can redistribute it and/or modify
> > > + * it under the terms of the GNU General Public License as published by
> > > + * the Free Software Foundation; either version 2 of the License, or
> > > + * (at your option) any later version.
> >
> > Are you sure "any later version"?
> >
>
> Fixed in the next version.
>
> > > + *
> > > + * This program is distributed in the hope that it will be useful,
> > > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> > > + * GNU General Public License for more details.
> > > + *
> > > + * You should have received a copy of the GNU General Public License
> > > + * along with this program; if not, write to the Free Software
> > > + * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
> >
> > This paragraph is never needed, unless you want to track the movements
> > of the FSF's office for the next 40 years.
> >
>
> You are right, will be fixed in the version.
>
> > > +
> > > +#define nrpz_dbg(format, arg...) \
> > > + do { if (debug) printk(KERN_DEBUG "nrpz: " format "\n", ##arg); } while (0)
> > > +#define nrpz_err(format, arg...) \
> > > + do { printk(KERN_ERR "nrpz: " format "\n", ##arg); } while (0)
> > > +#define nrpz_info(format, arg...) \
> > > + do { printk(KERN_INFO "nrpz: " format "\n", ##arg); } while (0)
> >
> > Ick, no, please use dev_dbg(), dev_err(), and dev_info() instead, don't
> > create your own macros.
> >
>
> Fixed in the next version.
>
> > > +/* Version Information */
> > > +#define DRIVER_VERSION "v4.00"
> >
> > What happened to the first 3 versions of this code? :)
> >
>
> * 2012_05_18 - 4.0 - revamp for kernel inclusion
> * 2007_12_07 - 3.0 - Rewritten, multi urbs for read and write
> * 2007_05_15 - 2.0 - Ported the driver to kernel 2.6, mostly rewritten
> * 2001_05_01 - 0.1 - first version
>
> The driver have a long history, the first version was developed for
> kernel 2.4. Since then it was adapted to the newest kernel version and
> was optimized in performance and size.

Ok, but what does that really matter for a kernel driver? What "value"
is it for this to be here?

> > > +#define DRIVER_AUTHOR "Stefani Seibold <stefani [at] seibold>"
> > > +#define DRIVER_DESC "Rohde&Schwarz NRP-Zxx USB Powermeter"
> > > +
> > > +/* Get a minor range for your devices from the usb maintainer */
> > > +#define NRPZ_MINOR_BASE 192
> >
> > You didn't ask me for a minor base, did you?
> >
>
> Sorry, that was one checkpoint in my ToDo List which i have lost. It
> would be kind if you can assign me a minor range for the NRPZ devices.

Not yet, sorry, you need to convince me that this really does need to be
a kernel driver.

> > > +MODULE_DEVICE_TABLE(usb, nrpz_table);
> > > +
> > > +/* Structure to hold all of our device specific stuff */
> > > +struct usb_nrpz {
> > > + struct usb_device *udev;
> > > + struct usb_interface *intf;
> >
>
> I tried to remove the udev entry in the next version. I was successfully
> in the most cases, but i find no solution for the nrpz_delete()
> function, which needs the udev for the usb_put_dev() call. Since intf is
> at this time NULL, there is no way to determinate the struct usb_device
> pointer.

Why would intf be NULL at some point in time when you really need to
find this out?

> > Do you really need both?
> >
> > > + unsigned minor; /* minor number for this device */
> > > + unsigned long in_use; /* in use flag */
> >
> > Why a long for this?
> >
>
> test_and_set_bit() expect a unsigned long. This kind of "in use"
> handling is not uncommon in a kernel driver.

Don't confuse that with a valid locking primitive. Please just use a
bool or char instead.

> > > +
> > > +static inline void urb_list_add_tail(spinlock_t *lock, struct urb *urb,
> > > + struct list_head *head)
> > > +{
> > > + spin_lock_irq(lock);
> > > + list_add_tail(&urb->urb_list, head);
> > > + spin_unlock_irq(lock);
> > > +}
> >
> > Why the lists of urbs? Why not either just create them all dynamically
> > all the time, or use urb anchors?
> >
>
> Anchors will be used for the running urbs. The callback will then
> collect the handled urbs in a list and read() and write() will resubmit
> this urbs after processing.
>
> Dynamically creation is an option, but i prefer this way, since it is
> faster, safer and has less error handling.

How do you know it is faster? Has it been measured? How? What was the
results? Why is it less error handling? Create, submit, and
automatically have the USB core destroy the urb is simpler and less code
in a driver than this logic here, right? If not, then we did something
wrong a long time ago.

> > > + case NRPZ_GETSENSORINFO:
> > > + {
> > > + struct nrpz_sensor_info __user *sensor_info =
> > > + (struct nrpz_sensor_info __user *)arg;
> > > +
> > > + if (!access_ok(VERIFY_WRITE, sensor_info, sizeof(*sensor_info)))
> > > + return -EFAULT;
> > > +
> > > + __put_user(dev->udev->descriptor.bcdDevice,
> > > + &sensor_info->bcdDevice);
> > > + __put_user(dev->udev->descriptor.bcdUSB,
> > > + &sensor_info->bcdUSB);
> > > + __put_user(dev->udev->descriptor.bDescriptorType,
> > > + &sensor_info->bDescriptorType);
> > > + __put_user(dev->udev->descriptor.bDeviceClass,
> > > + &sensor_info->bDeviceClass);
> > > + __put_user(dev->udev->descriptor.bDeviceSubClass,
> > > + &sensor_info->bDeviceSubClass);
> > > + __put_user(dev->udev->descriptor.bDeviceProtocol,
> > > + &sensor_info->bDeviceProtocol);
> > > + __put_user(dev->udev->descriptor.bMaxPacketSize0,
> > > + &sensor_info->bMaxPacketSize0);
> > > + __put_user(dev->udev->descriptor.bNumConfigurations,
> > > + &sensor_info->bNumConfigurations);
> > > + __put_user(dev->udev->descriptor.iManufacturer,
> > > + &sensor_info->iManufacturer);
> > > + __put_user(dev->udev->descriptor.iProduct,
> > > + &sensor_info->iProduct);
> > > + __put_user(dev->udev->descriptor.iSerialNumber,
> > > + &sensor_info->iSerialNumber);
> > > + __put_user(dev->udev->descriptor.idVendor,
> > > + &sensor_info->vendorId);
> > > + __put_user(dev->udev->descriptor.idProduct,
> > > + &sensor_info->productId);
> > > + usb_string(dev->udev, dev->udev->descriptor.iManufacturer,
> > > + (char __force *)sensor_info->manufacturer,
> > > + sizeof(sensor_info->manufacturer));
> > > + usb_string(dev->udev, dev->udev->descriptor.iProduct,
> > > + (char __force *)sensor_info->productName,
> > > + sizeof(sensor_info->productName));
> > > + usb_string(dev->udev, dev->udev->descriptor.iSerialNumber,
> > > + (char __force *)sensor_info->serialNumber,
> > > + sizeof(sensor_info->serialNumber));
> >
> > Why are you sending this information through an ioctl, when it is
> > already exported in sysfs? That seems quite needless.
> >
>
> This is very easy to do in the driver and would create a lot of code in
> the user space using libsysfs and depends on additional libraries.

You can't do a simple 'cat'? Sorry, this is duplication, even if I
could accept this driver, I can't accept this.

> > Anyway, I think the main questions are, "Why is this a kernel driver",
> > and "If this is a kernel driver, why not use the IIO interface"?
> >
>
> The IIO interface is an option, but not for the Rohde&Schwarz company,
> since the driver is in use by our customers for 10 years.

Ok, but I'm not that company, and neither is Linux. We strive to create
unified userspace apis that all drivers can use to ensure device
independance (hint, that's what an OS is for.) Creating one-off
user/kernel apis, like this one, is really special syscalls just for
this specific piece of hardware. Which is something we do not do
lightly at all.

> I also not sure that the IIO interface can handle all of the features of
> the power meter.

Then the IIO interface needs to be extended to do so. Seriously, that's
the only way I could see this being a kernel driver, not with this
custom ioctl interface.

> I have expected this objection "why is this a kernel driver?".
>
> It took me years to convince the company to provide open source driver
> and libraries. This is the first result of this effort.

All USB Linux drivers have to be open, this shouldn't have been any
effort at all :)

> Since the driver is very tiny and straight forward and has no side
> effects with other parts to the system it would be great to add it to
> the kernel drivers.
>
> This work should not done for the trash bin.

I understand your investment in this. But understand ours as well. You
are creating a hardware-specific set of custom system calls that no one
else can, or will, use. You also duplicate interfaces that we have
already standardized on (the USB device information in sysfs and in
usbfs). Why would that be acceptable? If you were in my situation,
would you accept this type of duplication?

So, I suggest you either use usbfs from a userspace program or library,
or if you insist on a kernel driver, use the IIO interface, extending it
where needed. As it is, in the current form, I can not accept this
driver for the above reasons.

thanks,

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


oneukum at suse

May 31, 2012, 3:17 AM

Post #10 of 28 (663 views)
Permalink
Re: [PATCH] add new NRP power meter USB device driver [In reply to]

Am Donnerstag, 31. Mai 2012, 11:53:04 schrieb Greg KH:
> > So it is a chicken and egg problem. A lot of software is now out with
> > depends on this driver and a lot of embedded development environments
> > doesn't provide libudev, libsysfs und libusb. It will also increase the
> > size of the image and adds additional dependencies to the application.
>
> You don't need any of those libraries to do this. What's wrong with
> using "raw" usbfs interactions?

SPI is not limited to USB. If we can get a unified subsystem then the
drivers need to be in kernel space, as some hardware does not allow
a solution in user space.

Regards
Oliver
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


gregkh at linuxfoundation

May 31, 2012, 5:04 AM

Post #11 of 28 (665 views)
Permalink
Re: [PATCH] add new NRP power meter USB device driver [In reply to]

On Thu, May 31, 2012 at 12:17:11PM +0200, Oliver Neukum wrote:
> Am Donnerstag, 31. Mai 2012, 11:53:04 schrieb Greg KH:
> > > So it is a chicken and egg problem. A lot of software is now out with
> > > depends on this driver and a lot of embedded development environments
> > > doesn't provide libudev, libsysfs und libusb. It will also increase the
> > > size of the image and adds additional dependencies to the application.
> >
> > You don't need any of those libraries to do this. What's wrong with
> > using "raw" usbfs interactions?
>
> SPI is not limited to USB. If we can get a unified subsystem then the
> drivers need to be in kernel space, as some hardware does not allow
> a solution in user space.

Ok, now that is a valid reason for a kernel driver.

But, I think it should use the IIO interface, as that should handle
these types of devices.

thanks,

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


oneukum at suse

May 31, 2012, 5:32 AM

Post #12 of 28 (661 views)
Permalink
Re: [PATCH] add new NRP power meter USB device driver [In reply to]

Am Donnerstag, 31. Mai 2012, 14:04:11 schrieben Sie:
> On Thu, May 31, 2012 at 12:17:11PM +0200, Oliver Neukum wrote:
> > Am Donnerstag, 31. Mai 2012, 11:53:04 schrieb Greg KH:
> > > > So it is a chicken and egg problem. A lot of software is now out with
> > > > depends on this driver and a lot of embedded development environments
> > > > doesn't provide libudev, libsysfs und libusb. It will also increase the
> > > > size of the image and adds additional dependencies to the application.
> > >
> > > You don't need any of those libraries to do this. What's wrong with
> > > using "raw" usbfs interactions?
> >
> > SPI is not limited to USB. If we can get a unified subsystem then the
> > drivers need to be in kernel space, as some hardware does not allow
> > a solution in user space.
>
> Ok, now that is a valid reason for a kernel driver.
>
> But, I think it should use the IIO interface, as that should handle
> these types of devices.

Bad gamble for the future. We will end up with an interface which
would not be implementable.

Regards
Oliver
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


gregkh at linuxfoundation

May 31, 2012, 5:48 AM

Post #13 of 28 (661 views)
Permalink
Re: [PATCH] add new NRP power meter USB device driver [In reply to]

On Thu, May 31, 2012 at 02:32:40PM +0200, Oliver Neukum wrote:
> Am Donnerstag, 31. Mai 2012, 14:04:11 schrieben Sie:
> > On Thu, May 31, 2012 at 12:17:11PM +0200, Oliver Neukum wrote:
> > > Am Donnerstag, 31. Mai 2012, 11:53:04 schrieb Greg KH:
> > > > > So it is a chicken and egg problem. A lot of software is now out with
> > > > > depends on this driver and a lot of embedded development environments
> > > > > doesn't provide libudev, libsysfs und libusb. It will also increase the
> > > > > size of the image and adds additional dependencies to the application.
> > > >
> > > > You don't need any of those libraries to do this. What's wrong with
> > > > using "raw" usbfs interactions?
> > >
> > > SPI is not limited to USB. If we can get a unified subsystem then the
> > > drivers need to be in kernel space, as some hardware does not allow
> > > a solution in user space.
> >
> > Ok, now that is a valid reason for a kernel driver.
> >
> > But, I think it should use the IIO interface, as that should handle
> > these types of devices.
>
> Bad gamble for the future. We will end up with an interface which
> would not be implementable.

Why do you say that? What's wrong with IIO?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


oneukum at suse

May 31, 2012, 7:22 AM

Post #14 of 28 (658 views)
Permalink
Re: [PATCH] add new NRP power meter USB device driver [In reply to]

Am Donnerstag, 31. Mai 2012, 14:48:45 schrieb Greg KH:
> On Thu, May 31, 2012 at 02:32:40PM +0200, Oliver Neukum wrote:
> > Am Donnerstag, 31. Mai 2012, 14:04:11 schrieben Sie:
> > > On Thu, May 31, 2012 at 12:17:11PM +0200, Oliver Neukum wrote:
> > > > Am Donnerstag, 31. Mai 2012, 11:53:04 schrieb Greg KH:
> > > > > > So it is a chicken and egg problem. A lot of software is now out with
> > > > > > depends on this driver and a lot of embedded development environments
> > > > > > doesn't provide libudev, libsysfs und libusb. It will also increase the
> > > > > > size of the image and adds additional dependencies to the application.
> > > > >
> > > > > You don't need any of those libraries to do this. What's wrong with
> > > > > using "raw" usbfs interactions?
> > > >
> > > > SPI is not limited to USB. If we can get a unified subsystem then the
> > > > drivers need to be in kernel space, as some hardware does not allow
> > > > a solution in user space.
> > >
> > > Ok, now that is a valid reason for a kernel driver.
> > >
> > > But, I think it should use the IIO interface, as that should handle
> > > these types of devices.
> >
> > Bad gamble for the future. We will end up with an interface which
> > would not be implementable.
>
> Why do you say that? What's wrong with IIO?

It seems to specialised for this kind of IO. SPI over USB is closer to a generic
thing rather like sg or even a bus rather than an IIO device. Kernel space drivers
that use SPI might belong into IIO. The infrastructure itself not so much.

Regards
Oliver
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


gregkh at linuxfoundation

May 31, 2012, 7:32 AM

Post #15 of 28 (658 views)
Permalink
Re: [PATCH] add new NRP power meter USB device driver [In reply to]

On Thu, May 31, 2012 at 04:22:31PM +0200, Oliver Neukum wrote:
> Am Donnerstag, 31. Mai 2012, 14:48:45 schrieb Greg KH:
> > On Thu, May 31, 2012 at 02:32:40PM +0200, Oliver Neukum wrote:
> > > Am Donnerstag, 31. Mai 2012, 14:04:11 schrieben Sie:
> > > > On Thu, May 31, 2012 at 12:17:11PM +0200, Oliver Neukum wrote:
> > > > > Am Donnerstag, 31. Mai 2012, 11:53:04 schrieb Greg KH:
> > > > > > > So it is a chicken and egg problem. A lot of software is now out with
> > > > > > > depends on this driver and a lot of embedded development environments
> > > > > > > doesn't provide libudev, libsysfs und libusb. It will also increase the
> > > > > > > size of the image and adds additional dependencies to the application.
> > > > > >
> > > > > > You don't need any of those libraries to do this. What's wrong with
> > > > > > using "raw" usbfs interactions?
> > > > >
> > > > > SPI is not limited to USB. If we can get a unified subsystem then the
> > > > > drivers need to be in kernel space, as some hardware does not allow
> > > > > a solution in user space.
> > > >
> > > > Ok, now that is a valid reason for a kernel driver.
> > > >
> > > > But, I think it should use the IIO interface, as that should handle
> > > > these types of devices.
> > >
> > > Bad gamble for the future. We will end up with an interface which
> > > would not be implementable.
> >
> > Why do you say that? What's wrong with IIO?
>
> It seems to specialised for this kind of IO. SPI over USB is closer to a generic
> thing rather like sg or even a bus rather than an IIO device. Kernel space drivers
> that use SPI might belong into IIO. The infrastructure itself not so much.

Wait, are you talking about the SPI we already support in the kernel?
Or something else? This driver really looks like it wants to send to
userspace "measurements" of something, which IIO is for.

Anyway, as-is, this isn't acceptable, I'm sure we can all agree on that
:)

thanks,

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


oneukum at suse

Jun 1, 2012, 6:38 AM

Post #16 of 28 (645 views)
Permalink
Re: [PATCH] add new NRP power meter USB device driver [In reply to]

Am Donnerstag, 31. Mai 2012, 16:32:55 schrieb Greg KH:
> On Thu, May 31, 2012 at 04:22:31PM +0200, Oliver Neukum wrote:

> > It seems to specialised for this kind of IO. SPI over USB is closer to a generic
> > thing rather like sg or even a bus rather than an IIO device. Kernel space drivers
> > that use SPI might belong into IIO. The infrastructure itself not so much.
>
> Wait, are you talking about the SPI we already support in the kernel?

Damn, one should watch one's spelling. SPI != SCPI
In the kernel we have SPI. This proposed driver is for SCPI.
The only relation they have is phonetic.

> Or something else? This driver really looks like it wants to send to
> userspace "measurements" of something, which IIO is for.

As far as I can tell, it works by messages over a bus, rather like SCSI.
>
> Anyway, as-is, this isn't acceptable, I'm sure we can all agree on that
> :)

Well, apart from the only ioctl I've seen far worse drivers.
The issue is not the driver as such, but whether and it what form it should exist.

Regards
Oliver
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


oneukum at suse

Jun 1, 2012, 7:16 AM

Post #17 of 28 (643 views)
Permalink
Re: [PATCH] add new NRP power meter USB device driver [In reply to]

Am Donnerstag, 31. Mai 2012, 11:21:40 schrieb Stefani Seibold:
> On Thu, 2012-05-31 at 10:20 +0200, Oliver Neukum wrote:
>
> > > >
> > > > > + if (arg) {
> > > > > + ret = wait_event_interruptible_timeout(
> > > > > + dev->out_running.wait,
> > > > > + list_empty(&dev->out_running.urb_list),
> > > > > + msecs_to_jiffies(arg));
> > > > > + if (!ret)
> > > > > + return -ETIMEDOUT;
> > > > > + if (ret < 0)
> > > > > + return ret;
> > > > > + return 0;
> > > > > + } else {
> > > > > + return wait_event_interruptible(
> > > > > + dev->out_running.wait,
> > > > > + list_empty(&dev->out_running.urb_list));
> > > > > + }
> > > > > + break;
> > > >
> > > > This is very ugly. If you need fsync(), then implement it.
> > > >
> > >
> > > fsync() did not meat the requirements, since i need in some case a
> > > timeout for the device. poll() will also not help, since it signals only
> > > that there is space to write.
> >
> > Well, then implement fsync() with interruptible sleep and use a timer
> > in user space.
> >
>
> But this will not solve the problem of older software which is still
> depending on this ioctl.

Yes. I guess it might be included in a depreated form. However
a sane alternative must be provided.

> > Yes, but this seems to be buggy:
> >
> > + ret = usb_submit_urb(urb, GFP_KERNEL);
> > + if (ret) {
> > + usb_unanchor_urb(urb);
> > + urb_list_add_tail(&dev->read_lock, urb, &dev->in_avail);
> > + nrpz_err("Failed submitting read urb (error %d)", ret);
> > + }
> >
> > You have already transfered the data to user space. It seems to me that you
> > need to zero out the URB and need to handle the case of getting an URB
> > without data.
> >
>
> Okay, i understand what you mean. Zeroing out is not necessary since
> usb_submit_urb will set urb->status to -EINPROGRESS. This behavior is
> well documented.
>

Look more closely at the code:

int usb_submit_urb(struct urb *urb, gfp_t mem_flags)
{
int xfertype, max;
struct usb_device *dev;
struct usb_host_endpoint *ep;
int is_out;

if (!urb || urb->hcpriv || !urb->complete)
return -EINVAL;
dev = urb->dev;
if ((!dev) || (dev->state < USB_STATE_UNAUTHENTICATED))
return -ENODEV;

/* For now, get the endpoint from the pipe. Eventually drivers
* will be required to set urb->ep directly and we will eliminate
* urb->pipe.
*/
ep = usb_pipe_endpoint(dev, urb->pipe);
if (!ep)
return -ENOENT;

urb->ep = ep;
urb->status = -EINPROGRESS;
urb->actual_length = 0;

There are a few error conditions where this is not true.


> > There probably is no generic answer. But I presume a reset will
> > reinit the device and destroy anything you set up before, so I guess
> > the next read() or write() after a reset has to return an error code that
> > tells user space that it has to redo its setup.
> >
>
> Is it okay to kick out the whole ..._reset() thing, since i have no idea
> what it is good for.

That's an option. Users will get -ENODEV.

Regards
Oliver
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


alan at lxorguk

Jun 1, 2012, 7:34 AM

Post #18 of 28 (646 views)
Permalink
Re: [PATCH] add new NRP power meter USB device driver [In reply to]

On Fri, 1 Jun 2012 16:16:36 +0200
Oliver Neukum <oneukum [at] suse> wrote:

> Am Donnerstag, 31. Mai 2012, 11:21:40 schrieb Stefani Seibold:
> > On Thu, 2012-05-31 at 10:20 +0200, Oliver Neukum wrote:
> >
> > > > >
> > > > > > + if (arg) {
> > > > > > + ret = wait_event_interruptible_timeout(
> > > > > > + dev->out_running.wait,
> > > > > > + list_empty(&dev->out_running.urb_list),
> > > > > > + msecs_to_jiffies(arg));
> > > > > > + if (!ret)
> > > > > > + return -ETIMEDOUT;
> > > > > > + if (ret < 0)
> > > > > > + return ret;
> > > > > > + return 0;
> > > > > > + } else {
> > > > > > + return wait_event_interruptible(
> > > > > > + dev->out_running.wait,
> > > > > > + list_empty(&dev->out_running.urb_list));
> > > > > > + }
> > > > > > + break;
> > > > >
> > > > > This is very ugly. If you need fsync(), then implement it.
> > > > >
> > > >
> > > > fsync() did not meat the requirements, since i need in some case a
> > > > timeout for the device. poll() will also not help, since it signals only
> > > > that there is space to write.
> > >
> > > Well, then implement fsync() with interruptible sleep and use a timer
> > > in user space.
> > >
> >
> > But this will not solve the problem of older software which is still
> > depending on this ioctl.
>
> Yes. I guess it might be included in a depreated form. However
> a sane alternative must be provided.

It's a new kernel driver it's not bound to any existing API and we
regularly enforce a sane API on drivers being merged so I think the
legacy stuff is just "tough" - the out of tree driver will work fine with
any legacy code.

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


stefani at seibold

Jun 1, 2012, 10:57 PM

Post #19 of 28 (640 views)
Permalink
Re: [PATCH] add new NRP power meter USB device driver [In reply to]

On Fri, 2012-06-01 at 16:16 +0200, Oliver Neukum wrote:
> Am Donnerstag, 31. Mai 2012, 11:21:40 schrieb Stefani Seibold:
> > On Thu, 2012-05-31 at 10:20 +0200, Oliver Neukum wrote:
> >
> > > > >
> > > > > > + if (arg) {
> > > > > > + ret = wait_event_interruptible_timeout(
> > > > > > + dev->out_running.wait,
> > > > > > + list_empty(&dev->out_running.urb_list),
> > > > > > + msecs_to_jiffies(arg));
> > > > > > + if (!ret)
> > > > > > + return -ETIMEDOUT;
> > > > > > + if (ret < 0)
> > > > > > + return ret;
> > > > > > + return 0;
> > > > > > + } else {
> > > > > > + return wait_event_interruptible(
> > > > > > + dev->out_running.wait,
> > > > > > + list_empty(&dev->out_running.urb_list));
> > > > > > + }
> > > > > > + break;
> > > > >
> > > > > This is very ugly. If you need fsync(), then implement it.
> > > > >
> > > >
> > > > fsync() did not meat the requirements, since i need in some case a
> > > > timeout for the device. poll() will also not help, since it signals only
> > > > that there is space to write.
> > >
> > > Well, then implement fsync() with interruptible sleep and use a timer
> > > in user space.
> > >
> >
> > But this will not solve the problem of older software which is still
> > depending on this ioctl.
>
> Yes. I guess it might be included in a depreated form. However
> a sane alternative must be provided.
>

I will implement the fsync() function. But your idea using an alarm
timer will break many libraries like Qt or GLIB, which depends on the
the exclusive use of the alarm timer.

I know, there is always a way to do it, but for the average coder will
create a piece of complex code, full of races and pretenses.

Also you would not believe, but most of the developer i know are not
familiar with implementing signal safe code.

> > > Yes, but this seems to be buggy:
> > >
> > > + ret = usb_submit_urb(urb, GFP_KERNEL);
> > > + if (ret) {
> > > + usb_unanchor_urb(urb);
> > > + urb_list_add_tail(&dev->read_lock, urb, &dev->in_avail);
> > > + nrpz_err("Failed submitting read urb (error %d)", ret);
> > > + }
> > >
> > > You have already transfered the data to user space. It seems to me that you
> > > need to zero out the URB and need to handle the case of getting an URB
> > > without data.
> > >
> >
> > Okay, i understand what you mean. Zeroing out is not necessary since
> > usb_submit_urb will set urb->status to -EINPROGRESS. This behavior is
> > well documented.
> >
>
> Look more closely at the code:
>
> int usb_submit_urb(struct urb *urb, gfp_t mem_flags)
> {
> int xfertype, max;
> struct usb_device *dev;
> struct usb_host_endpoint *ep;
> int is_out;
>
> if (!urb || urb->hcpriv || !urb->complete)
> return -EINVAL;
> dev = urb->dev;
> if ((!dev) || (dev->state < USB_STATE_UNAUTHENTICATED))
> return -ENODEV;
>
> /* For now, get the endpoint from the pipe. Eventually drivers
> * will be required to set urb->ep directly and we will eliminate
> * urb->pipe.
> */
> ep = usb_pipe_endpoint(dev, urb->pipe);
> if (!ep)
> return -ENOENT;
>
> urb->ep = ep;
> urb->status = -EINPROGRESS;
> urb->actual_length = 0;
>
> There are a few error conditions where this is not true.
>

Yes, but i am not sure if this conditions will be true by an already
correct initialized urb. Anyway, i will fix it.


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


stefani at seibold

Jun 2, 2012, 12:10 AM

Post #20 of 28 (649 views)
Permalink
Re: [PATCH] add new NRP power meter USB device driver [In reply to]

Hi Grek,

sorry for the late answer, but i must first do some investigations.

On Thu, 2012-05-31 at 17:41 +0800, Greg KH wrote:

> multi-device handling for libusb has been there from the beginning (over
> 10 years ago) and multi-urb handling has been around for a very long
> time as well.
>
> So, sorry, I don't buy this argument :)
>
> > So it is a chicken and egg problem. A lot of software is now out with
> > depends on this driver and a lot of embedded development environments
> > doesn't provide libudev, libsysfs und libusb. It will also increase the
> > size of the image and adds additional dependencies to the application.
>
> You don't need any of those libraries to do this. What's wrong with
> using "raw" usbfs interactions?
>

usbfs must be mounted. On some distribution this is not the case for
security reasons.

libusb has also this kind of security concerns, since the user must in
the group for accessing the usb devices.

I know there is always a way, but not for the average developer, which
is not familiar with udev-rules, ACL, capabilities, fstab, groups and so
on.

In the embedded world it is also much easier to create an device node by
hand, because you know your environment. But this is unwinnable fight of
the emdedded linux user against the fat desktop and server fraction.

> > Also kernel space is faster and this is a key factor.
>
> I don't buy it.
>
> How much faster? What numbers do you have?
>

It is not important how much, it is only important that is is!

> We have USB vision systems running at wire speeds using usbfs and
> libusb, and have had that since the early 2.4 kernel days. Speed is not
> a valid argument here, unless you have some really bad userspace code.
>
> > And it possible to simple handling the device from script using printf,
> > echo, cat and so on.
>
> Not really a valid argument, sorry.
>

Not in the embedded world, where you want check a system outside in the
field.

> Which ones? We have ripped out lots of USB drivers that could have been
> controlled by usbfs userspace programs over the years for this same
> reason. If we missed any, please let me know and I will rectify that.
>

Thats easy, e.g the rio500 driver or cyterm.

And if we include any driver which is not needed for booting a linux
kernel:

- The whole multi touch driver could be replaced by an user space
driver, using uinput.
- VFAT can be done by fuse.
- And so on...

Where is the boundary?

>
> > Sorry, that was one checkpoint in my ToDo List which i have lost. It
> > would be kind if you can assign me a minor range for the NRPZ devices.
>
> Not yet, sorry, you need to convince me that this really does need to be
> a kernel driver.
>

I will try to do this ;-)

> > > > +MODULE_DEVICE_TABLE(usb, nrpz_table);
> > > > +
> > > > +/* Structure to hold all of our device specific stuff */
> > > > +struct usb_nrpz {
> > > > + struct usb_device *udev;
> > > > + struct usb_interface *intf;
> > >
> >
> > I tried to remove the udev entry in the next version. I was successfully
> > in the most cases, but i find no solution for the nrpz_delete()
> > function, which needs the udev for the usb_put_dev() call. Since intf is
> > at this time NULL, there is no way to determinate the struct usb_device
> > pointer.
>
> Why would intf be NULL at some point in time when you really need to
> find this out?
>

That is exactly the code of the usb-skeleton driver does.

BTW: There are some issues with the usb-skeleton driver code, which the
O_NONBLOCK handling i will fix later.

And again usb_skeleton store also both usb_device and usb_interface.

If the skeleton driver is a example full of wrong code and waste memory
resources, why is it in and should be used as a template for usb
drivers? Your name is in the source code header!

> > > > + unsigned long in_use; /* in use flag */
> > >
> > > Why a long for this?
> > >
> >
> > test_and_set_bit() expect a unsigned long. This kind of "in use"
> > handling is not uncommon in a kernel driver.
>
> Don't confuse that with a valid locking primitive. Please just use a
> bool or char instead.
>

Why not? I see no benefit, but i will do.

I think it is not necessary to count each byte, in the usb_nrpz
structure. This structure will be only allocated if a device is plugged.
And some of your suggestions will create more code, which is good for 2
or 3 attached sensors.

>
> > Anchors will be used for the running urbs. The callback will then
> > collect the handled urbs in a list and read() and write() will resubmit
> > this urbs after processing.
> >
> > Dynamically creation is an option, but i prefer this way, since it is
> > faster, safer and has less error handling.
>
> How do you know it is faster? Has it been measured? How? What was the
> results? Why is it less error handling? Create, submit, and
> automatically have the USB core destroy the urb is simpler and less code
> in a driver than this logic here, right? If not, then we did something
> wrong a long time ago.
>

Thats not true. And maybe yes. The code is clean.

I preallocate the urbs which would be needed and initialize it. Than i
can use when it. Simple get the urb from the top op the list and put it
to the other list and submit it. In the callback handler move it back to
the other list. Very easy and straight forward.

> > > > + case NRPZ_GETSENSORINFO:
> > > > + {
> > > > + struct nrpz_sensor_info __user *sensor_info =
> > > > + (struct nrpz_sensor_info __user *)arg;
> > > > +
> > > > + if (!access_ok(VERIFY_WRITE, sensor_info, sizeof(*sensor_info)))
> > > > + return -EFAULT;
> > > > +
> > > > + __put_user(dev->udev->descriptor.bcdDevice,
> > > > + &sensor_info->bcdDevice);
> > > > + __put_user(dev->udev->descriptor.bcdUSB,
> > > > + &sensor_info->bcdUSB);
> > > > + __put_user(dev->udev->descriptor.bDescriptorType,
> > > > + &sensor_info->bDescriptorType);
> > > > + __put_user(dev->udev->descriptor.bDeviceClass,
> > > > + &sensor_info->bDeviceClass);
> > > > + __put_user(dev->udev->descriptor.bDeviceSubClass,
> > > > + &sensor_info->bDeviceSubClass);
> > > > + __put_user(dev->udev->descriptor.bDeviceProtocol,
> > > > + &sensor_info->bDeviceProtocol);
> > > > + __put_user(dev->udev->descriptor.bMaxPacketSize0,
> > > > + &sensor_info->bMaxPacketSize0);
> > > > + __put_user(dev->udev->descriptor.bNumConfigurations,
> > > > + &sensor_info->bNumConfigurations);
> > > > + __put_user(dev->udev->descriptor.iManufacturer,
> > > > + &sensor_info->iManufacturer);
> > > > + __put_user(dev->udev->descriptor.iProduct,
> > > > + &sensor_info->iProduct);
> > > > + __put_user(dev->udev->descriptor.iSerialNumber,
> > > > + &sensor_info->iSerialNumber);
> > > > + __put_user(dev->udev->descriptor.idVendor,
> > > > + &sensor_info->vendorId);
> > > > + __put_user(dev->udev->descriptor.idProduct,
> > > > + &sensor_info->productId);
> > > > + usb_string(dev->udev, dev->udev->descriptor.iManufacturer,
> > > > + (char __force *)sensor_info->manufacturer,
> > > > + sizeof(sensor_info->manufacturer));
> > > > + usb_string(dev->udev, dev->udev->descriptor.iProduct,
> > > > + (char __force *)sensor_info->productName,
> > > > + sizeof(sensor_info->productName));
> > > > + usb_string(dev->udev, dev->udev->descriptor.iSerialNumber,
> > > > + (char __force *)sensor_info->serialNumber,
> > > > + sizeof(sensor_info->serialNumber));
> > >
> > > Why are you sending this information through an ioctl, when it is
> > > already exported in sysfs? That seems quite needless.
> > >
> >
> > This is very easy to do in the driver and would create a lot of code in
> > the user space using libsysfs and depends on additional libraries.
>
> You can't do a simple 'cat'? Sorry, this is duplication, even if I
> could accept this driver, I can't accept this.
>

Sorry, but collect this data in user space with libudev is a much harder
task and produce a lot of code. So there is no code duplication, also
older software depends on this ioctl.

> > > Anyway, I think the main questions are, "Why is this a kernel driver",
> > > and "If this is a kernel driver, why not use the IIO interface"?
> > >
> >
> > The IIO interface is an option, but not for the Rohde&Schwarz company,
> > since the driver is in use by our customers for 10 years.
>
> Ok, but I'm not that company, and neither is Linux. We strive to create
> unified userspace apis that all drivers can use to ensure device
> independance (hint, that's what an OS is for.) Creating one-off
> user/kernel apis, like this one, is really special syscalls just for
> this specific piece of hardware. Which is something we do not do
> lightly at all.
>
> > I also not sure that the IIO interface can handle all of the features of
> > the power meter.
>
> Then the IIO interface needs to be extended to do so. Seriously, that's
> the only way I could see this being a kernel driver, not with this
> custom ioctl interface.
>

I have checked the IIO interface the last two days.

I see no reason to spend month to extend the IIO interface for the need
of high sophisticate measurement instruments since it is IMHO not
possible.

IIO is for simple sensor devices.

There is no way for an SCPI communication channel, which is the standard
in the instrument world.

Also IIO does not support float or double data values for accurate
measuring nor any text messages interface.

IIO will provide the data in separate queues, which will kick a way the
dependencies of the measurement values. So you are not able to say for
what value was a trigger for, without scanning all channels and compare
the time stamps.

The IIO userland interface is also good for wasting resources. For the
NRP Devices there where a need of a minimum of 10 open files in the
sysfs to get the data (trigger, status, measurement values, limit
changes and so on) from one device.

Your strive to create a unified userspace API for IIO will currently
have no success, the interface is to complex, waste a lot of resources
and will never meat the requirements of more complex measurement
hardware. It is only good for simple sensor devices.

If you want create a clean and useable interface than have a look at
HID. This is more what we need:

- A descriptor which shows all the capabilities of the device, and which
can easily extended.
- A report wich say what is going on.
- Only one device interface per sensor or instrument

And it would be possible to provide a nice lib which handles and hides
most of basic work from the application developer.

Greetings,
Stefani


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


gregkh at linuxfoundation

Jun 2, 2012, 5:10 AM

Post #21 of 28 (645 views)
Permalink
Re: [PATCH] add new NRP power meter USB device driver [In reply to]

On Sat, Jun 02, 2012 at 09:10:13AM +0200, Stefani Seibold wrote:
> Hi Grek,

Who is that? :)

> On Thu, 2012-05-31 at 17:41 +0800, Greg KH wrote:
>
> > multi-device handling for libusb has been there from the beginning (over
> > 10 years ago) and multi-urb handling has been around for a very long
> > time as well.
> >
> > So, sorry, I don't buy this argument :)
> >
> > > So it is a chicken and egg problem. A lot of software is now out with
> > > depends on this driver and a lot of embedded development environments
> > > doesn't provide libudev, libsysfs und libusb. It will also increase the
> > > size of the image and adds additional dependencies to the application.
> >
> > You don't need any of those libraries to do this. What's wrong with
> > using "raw" usbfs interactions?
> >
>
> usbfs must be mounted. On some distribution this is not the case for
> security reasons.

Not true at all, usbfs does not get mounted anymore, and hasn't for
years. In fact, with 3.5, it's gone, and can't be mounted. Your distro
handles this through device nodes for the usb devices dynamically.

> libusb has also this kind of security concerns, since the user must in
> the group for accessing the usb devices.

Nope, not been true for years, distros properly handle this just fine.

> I know there is always a way, but not for the average developer, which
> is not familiar with udev-rules, ACL, capabilities, fstab, groups and so
> on.

Again, this is all set up properly in good distros with no need for
anything to be changed or tweaked by anyone.

> In the embedded world it is also much easier to create an device node by
> hand, because you know your environment. But this is unwinnable fight of
> the emdedded linux user against the fat desktop and server fraction.

The embedded world uses devtmpfs, which also handles all of this
automatically, with no need for external programs at all. This is even
easier for them to do than "traditional" distros, so this is not a valid
reason, sorry.

> > > Also kernel space is faster and this is a key factor.
> >
> > I don't buy it.
> >
> > How much faster? What numbers do you have?
> >
>
> It is not important how much, it is only important that is is!

How is it faster? I need real numbers to back this up. I don't believe
it is faster as your limiting factor is the USB bus speed, not the
processor or kernel or userspace at all.

> > We have USB vision systems running at wire speeds using usbfs and
> > libusb, and have had that since the early 2.4 kernel days. Speed is not
> > a valid argument here, unless you have some really bad userspace code.
> >
> > > And it possible to simple handling the device from script using printf,
> > > echo, cat and so on.
> >
> > Not really a valid argument, sorry.
> >
>
> Not in the embedded world, where you want check a system outside in the
> field.

Those embedded systems have other tools on them.

> > Which ones? We have ripped out lots of USB drivers that could have been
> > controlled by usbfs userspace programs over the years for this same
> > reason. If we missed any, please let me know and I will rectify that.
> >
>
> Thats easy, e.g the rio500 driver or cyterm.

Ok, please send patches removing them. For some reason rio500 stuck
around the last time we purged things, but I can't remember why.

> And if we include any driver which is not needed for booting a linux
> kernel:
>
> - The whole multi touch driver could be replaced by an user space
> driver, using uinput.
> - VFAT can be done by fuse.
> - And so on...
>
> Where is the boundary?

Oh come on, that's not what I am talking about here at all and you know
it. Don't be juvenile.

> > > > > +MODULE_DEVICE_TABLE(usb, nrpz_table);
> > > > > +
> > > > > +/* Structure to hold all of our device specific stuff */
> > > > > +struct usb_nrpz {
> > > > > + struct usb_device *udev;
> > > > > + struct usb_interface *intf;
> > > >
> > >
> > > I tried to remove the udev entry in the next version. I was successfully
> > > in the most cases, but i find no solution for the nrpz_delete()
> > > function, which needs the udev for the usb_put_dev() call. Since intf is
> > > at this time NULL, there is no way to determinate the struct usb_device
> > > pointer.
> >
> > Why would intf be NULL at some point in time when you really need to
> > find this out?
> >
>
> That is exactly the code of the usb-skeleton driver does.
>
> BTW: There are some issues with the usb-skeleton driver code, which the
> O_NONBLOCK handling i will fix later.
>
> And again usb_skeleton store also both usb_device and usb_interface.
>
> If the skeleton driver is a example full of wrong code and waste memory
> resources, why is it in and should be used as a template for usb
> drivers? Your name is in the source code header!

My name is in lots of code that probably needs to be fixed, that's not a
valid reason for me to accept that it is correct :)

> > > > > + usb_string(dev->udev, dev->udev->descriptor.iSerialNumber,
> > > > > + (char __force *)sensor_info->serialNumber,
> > > > > + sizeof(sensor_info->serialNumber));
> > > >
> > > > Why are you sending this information through an ioctl, when it is
> > > > already exported in sysfs? That seems quite needless.
> > > >
> > >
> > > This is very easy to do in the driver and would create a lot of code in
> > > the user space using libsysfs and depends on additional libraries.
> >
> > You can't do a simple 'cat'? Sorry, this is duplication, even if I
> > could accept this driver, I can't accept this.
> >
>
> Sorry, but collect this data in user space with libudev is a much harder
> task and produce a lot of code. So there is no code duplication, also
> older software depends on this ioctl.

Again, we don't care, and can't care, about older software that dealt
with a kernel driver we never approved, sorry.

And you wanted to use 'cat' for other stuff, what's wrong with using it
on the sysfs files for this information.

Again, I can't accept this.

> > > > Anyway, I think the main questions are, "Why is this a kernel driver",
> > > > and "If this is a kernel driver, why not use the IIO interface"?
> > > >
> > >
> > > The IIO interface is an option, but not for the Rohde&Schwarz company,
> > > since the driver is in use by our customers for 10 years.
> >
> > Ok, but I'm not that company, and neither is Linux. We strive to create
> > unified userspace apis that all drivers can use to ensure device
> > independance (hint, that's what an OS is for.) Creating one-off
> > user/kernel apis, like this one, is really special syscalls just for
> > this specific piece of hardware. Which is something we do not do
> > lightly at all.
> >
> > > I also not sure that the IIO interface can handle all of the features of
> > > the power meter.
> >
> > Then the IIO interface needs to be extended to do so. Seriously, that's
> > the only way I could see this being a kernel driver, not with this
> > custom ioctl interface.
> >
>
> I have checked the IIO interface the last two days.
>
> I see no reason to spend month to extend the IIO interface for the need
> of high sophisticate measurement instruments since it is IMHO not
> possible.
>
> IIO is for simple sensor devices.

Have you discussed this with the IIO developers? That's not the
impression I got from the interface, but I would need to have them
verify this.

> There is no way for an SCPI communication channel, which is the standard
> in the instrument world.

Who uses such a communication channel? Why have we never seen it before
on Linux with it's different usages before? We have both IIO and comedi
intefaces for Linux, both handling instruments and data measurement
devices.

> Also IIO does not support float or double data values for accurate
> measuring nor any text messages interface.

You shouldn't be doing float in the kernel anyway, that's up to
userspace to handle, right? And IIO can pass on floating point
information the last time I checked.

What do you mean by "text message interface"?

> IIO will provide the data in separate queues, which will kick a way the
> dependencies of the measurement values. So you are not able to say for
> what value was a trigger for, without scanning all channels and compare
> the time stamps.
>
> The IIO userland interface is also good for wasting resources. For the
> NRP Devices there where a need of a minimum of 10 open files in the
> sysfs to get the data (trigger, status, measurement values, limit
> changes and so on) from one device.

Since when is 10 file handles a big deal?

> Your strive to create a unified userspace API for IIO will currently
> have no success, the interface is to complex, waste a lot of resources
> and will never meat the requirements of more complex measurement
> hardware. It is only good for simple sensor devices.
>
> If you want create a clean and useable interface than have a look at
> HID. This is more what we need:

As someone who participated in creating the HID specification almost 20
years ago, I find this sentance laughable :)

> - A descriptor which shows all the capabilities of the device, and which
> can easily extended.
> - A report wich say what is going on.
> - Only one device interface per sensor or instrument
>
> And it would be possible to provide a nice lib which handles and hides
> most of basic work from the application developer.

Can't you do that on top of IIO in userspace?

Anyway, yes, the descriptor interface of HID is nice, but I don't see
that type of interface here in this driver, so I don't understand the
issue.

If the IIO developers look at this device and driver and agree that it
does not work for their interface, then I'd be glad to revisit the need
for a custom interface for it. But, it needs to be done such that
future devices that use this type of protocol also are supported.

As I stated before, custom syscalls (which is what these ioctls are) for
a single device, are not acceptable. Especially when it can be done
from userspace with no need for a kernel driver that I can see at the
moment.

thanks,

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


stefani at seibold

Jun 2, 2012, 8:43 AM

Post #22 of 28 (637 views)
Permalink
Re: [PATCH] add new NRP power meter USB device driver [In reply to]

On Sat, 2012-06-02 at 05:10 -0700, Greg KH wrote:
> On Sat, Jun 02, 2012 at 09:10:13AM +0200, Stefani Seibold wrote:

> > > > And it possible to simple handling the device from script using printf,
> > > > echo, cat and so on.
> > >
> > > Not really a valid argument, sorry.
> > >
> >
> > Not in the embedded world, where you want check a system outside in the
> > field.
>
> Those embedded systems have other tools on them.
>

Murphy's Law: You will never have the tool nor the flash storage u need
in case of a bug.

> > > Which ones? We have ripped out lots of USB drivers that could have been
> > > controlled by usbfs userspace programs over the years for this same
> > > reason. If we missed any, please let me know and I will rectify that.
> > >
> >
> > Thats easy, e.g the rio500 driver or cyterm.
>
> Ok, please send patches removing them. For some reason rio500 stuck
> around the last time we purged things, but I can't remember why.
>

Maybe you have one ;-) But i am interested to do this.

> > And if we include any driver which is not needed for booting a linux
> > kernel:
> >
> > - The whole multi touch driver could be replaced by an user space
> > driver, using uinput.
> > - VFAT can be done by fuse.
> > - And so on...
> >
> > Where is the boundary?
>
> Oh come on, that's not what I am talking about here at all and you know
> it. Don't be juvenile.
>

The boundaries are floating and depending on an arbitrary decisions...
or zeitgeist.

> > > > > > +MODULE_DEVICE_TABLE(usb, nrpz_table);
> > > > > > +
> > > > > > +/* Structure to hold all of our device specific stuff */
> > > > > > +struct usb_nrpz {
> > > > > > + struct usb_device *udev;
> > > > > > + struct usb_interface *intf;
> > > > >
> > > >
> > > > I tried to remove the udev entry in the next version. I was successfully
> > > > in the most cases, but i find no solution for the nrpz_delete()
> > > > function, which needs the udev for the usb_put_dev() call. Since intf is
> > > > at this time NULL, there is no way to determinate the struct usb_device
> > > > pointer.
> > >
> > > Why would intf be NULL at some point in time when you really need to
> > > find this out?
> > >
> >
> > That is exactly the code of the usb-skeleton driver does.
> >
> > BTW: There are some issues with the usb-skeleton driver code, which the
> > O_NONBLOCK handling i will fix later.
> >
> > And again usb_skeleton store also both usb_device and usb_interface.
> >
> > If the skeleton driver is a example full of wrong code and waste memory
> > resources, why is it in and should be used as a template for usb
> > drivers? Your name is in the source code header!
>
> My name is in lots of code that probably needs to be fixed, that's not a
> valid reason for me to accept that it is correct :)
>

For me it is much easier to kick this intf equals zero thing away. But i
thought that the usb skeleton driver is the prefered implementation way.

I have already fixed this issue.

> > > Then the IIO interface needs to be extended to do so. Seriously, that's
> > > the only way I could see this being a kernel driver, not with this
> > > custom ioctl interface.
> > >


> Have you discussed this with the IIO developers? That's not the
> impression I got from the interface, but I would need to have them
> verify this.
>

No, it was the first time that i have looked at this interface.

> > There is no way for an SCPI communication channel, which is the standard
> > in the instrument world.
>
> Who uses such a communication channel? Why have we never seen it before
> on Linux with it's different usages before? We have both IIO and comedi
> intefaces for Linux, both handling instruments and data measurement
> devices.
>

SCPI is used by the whole measurement instrument manufacturer like Rohde
& Schwarz, Hewlett Packard (or the company that was split off),
Tektronix and much more.. It is important for inter operability.

It is based on the old IEEE 488 interface, which is a standard since the
good old 8 bit days (all the Commodore and HP machines provided this
hardware interface). These hardware interface is still in use, because
it has very good latency and you can combine all your measurement
instrument, generators and other.

Today there are a lot more SCPI communication hardware interfaces like
USB, RS232 and Ethernet.

SCPI is a command set for (remote) control this kind of instruments.
E.g. "*idn?" give you the the identification string or "freq 66GHz" set
the frequency to 66 GHz.

See the official web site of the SCPI consortium:

http://www.ivifoundation.org/scpi/default.aspx

> > Also IIO does not support float or double data values for accurate
> > measuring nor any text messages interface.
>
> You shouldn't be doing float in the kernel anyway, that's up to
> userspace to handle, right? And IIO can pass on floating point
> information the last time I checked.
>
> What do you mean by "text message interface"?
>

The devices can also reply text messages.

> > IIO will provide the data in separate queues, which will kick a way the
> > dependencies of the measurement values. So you are not able to say for
> > what value was a trigger for, without scanning all channels and compare
> > the time stamps.
> >
> > The IIO userland interface is also good for wasting resources. For the
> > NRP Devices there where a need of a minimum of 10 open files in the
> > sysfs to get the data (trigger, status, measurement values, limit
> > changes and so on) from one device.
>
> Since when is 10 file handles a big deal?
>

When you have 10 or 100 of the devices. And why so much splitting. The
IIO Interface is easy for simple devices, but unusable for complex
measurement and generator devices.

Do you have really a good feeling to control one device in that way?
Open 10 or more files and resynchronize them in userspace.

> > Your strive to create a unified userspace API for IIO will currently
> > have no success, the interface is to complex, waste a lot of resources
> > and will never meat the requirements of more complex measurement
> > hardware. It is only good for simple sensor devices.
> >
> > If you want create a clean and useable interface than have a look at
> > HID. This is more what we need:
>
> As someone who participated in creating the HID specification almost 20
> years ago, I find this sentance laughable :)
>

Why do you laugh about your one work. I like it, it is a really good
idea and is exactly what you need for the measurement framework.

> > - A descriptor which shows all the capabilities of the device, and which
> > can easily extended.
> > - A report wich say what is going on.
> > - Only one device interface per sensor or instrument
> >
> > And it would be possible to provide a nice lib which handles and hides
> > most of basic work from the application developer.
>
> Can't you do that on top of IIO in userspace?
>

I don't like IIO, it is not a design which provides the feature that are
needed for complex devices.

> Anyway, yes, the descriptor interface of HID is nice, but I don't see
> that type of interface here in this driver, so I don't understand the
> issue.
>

Because there is no way to integrate the NRPZ Sensors in the IIO
framework.

> If the IIO developers look at this device and driver and agree that it
> does not work for their interface, then I'd be glad to revisit the need
> for a custom interface for it. But, it needs to be done such that
> future devices that use this type of protocol also are supported.
>

A framework must capable to handle all kinds of devices out there, the
interface must defined to serve the hardware, because hardware which is
still out there cannot changed in a way that IIO will be happy with it.

> As I stated before, custom syscalls (which is what these ioctls are) for
> a single device, are not acceptable. Especially when it can be done
> from userspace with no need for a kernel driver that I can see at the
> moment.
>

A driver internal ioctl() is not a new syscall. The syscall is still
ioctl. And that is what this interface is for.

Anyway i will resend the driver with all the fixes applied.

Greetings,
Stefani


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


stefani at seibold

Jun 2, 2012, 9:18 AM

Post #23 of 28 (637 views)
Permalink
[PATCH] add new NRP power meter USB device driver [In reply to]

From: Stefani Seibold <stefani [at] seibold>

This driver supports all of the Rohde&Schwarz RF Power Meter NRP Sensors. These
sensors are intelligent standalone instruments that communicate via USB.

A power meter is a device for analyzing the RF power output level of an
electrical device, similar to an oscilloscope.

The Power Meter Sensors will be used in a wide range of environements, like

- Manufacturing (e.g. air planes and smart phones)
- Radio and TV broadcasting
- Mobile communications
- Engeeniering
- Science Labs
- Education

The NRP Power Meters support the following measurements:

- Dynamic range: up to 90 dB (sensor dependent)
- Level range: -67 dBm to +45 dBm (sensor dependent)
- Frequency range: DC to 67 GHz (sensor dependent)
- Measurement speed: 1500 measurements per second in the buffered mode
- Linearity uncertainty: 0.007 dB
- Precise average power measurements irrespective of modulation and bandwidth
- Flexible measurements on up to 128 time slots per power sensor
- S parameter correction of components between sensor and test object

The device will be controlled by a SCPI like interface.
(https://en.wikipedia.org/wiki/Standard_Commands_for_Programmable_Instruments)

The patch is against linux 3.5.0
(commit 829f51dbd825256197fb2a89705d42ad83f958ef)

The source is checked with checkpatch.pl and has no errors. Only 11
"line over 80 characters" warnings are left. I see no reason to satisfy this
boring punch card limit for this lines, since it will make the source noisier.

make C=1 is quiet.

ChangeLog:

2012-06-02 Fixes suggested by Alan Cox, Oliver Neukum and Greg KH
2012-05-29 First public release under GPL
2012-05-18 V4.0 - revamp for kernel inclusion
2007-12-07 V3.0 - Rewritten, multi urbs for read and write
2007-05-15 V2.0 - Ported the driver to kernel 2.6, mostly rewritten
2001-05-01 V0.1 - first version

Regards,
Stefani

Signed-off-by: Stefani Seibold <stefani [at] seibold>
---
drivers/usb/misc/Kconfig | 12 +
drivers/usb/misc/Makefile | 1 +
drivers/usb/misc/nrpz.c | 1059 ++++++++++++++++++++++++++++++++++++++++
include/linux/usb/nrpzmodule.h | 47 ++
4 files changed, 1119 insertions(+), 0 deletions(-)
create mode 100644 drivers/usb/misc/nrpz.c
create mode 100644 include/linux/usb/nrpzmodule.h

diff --git a/drivers/usb/misc/Kconfig b/drivers/usb/misc/Kconfig
index 1bfcd02..2c55d5f 100644
--- a/drivers/usb/misc/Kconfig
+++ b/drivers/usb/misc/Kconfig
@@ -244,3 +244,15 @@ config USB_YUREX
To compile this driver as a module, choose M here: the
module will be called yurex.

+config USB_NRPZ
+ tristate "USB NRPZ power sensor driver support"
+ depends on USB
+ help
+ This driver supports the Rohde&Schwarz NRP RF Power Meter Sensors.
+
+ These sensors are intelligent standalone instruments that
+ communicate via USB and provide a wide range of measurements.
+
+ To compile this driver as a module, choose M here: the
+ module will be called nrpz.
+
diff --git a/drivers/usb/misc/Makefile b/drivers/usb/misc/Makefile
index 796ce7e..9a0364c 100644
--- a/drivers/usb/misc/Makefile
+++ b/drivers/usb/misc/Makefile
@@ -25,5 +25,6 @@ obj-$(CONFIG_USB_TRANCEVIBRATOR) += trancevibrator.o
obj-$(CONFIG_USB_USS720) += uss720.o
obj-$(CONFIG_USB_SEVSEG) += usbsevseg.o
obj-$(CONFIG_USB_YUREX) += yurex.o
+obj-$(CONFIG_USB_NRPZ) += nrpz.o

obj-$(CONFIG_USB_SISUSBVGA) += sisusbvga/
diff --git a/drivers/usb/misc/nrpz.c b/drivers/usb/misc/nrpz.c
new file mode 100644
index 0000000..5ebb218
--- /dev/null
+++ b/drivers/usb/misc/nrpz.c
@@ -0,0 +1,1059 @@
+/*
+ * Rohde & Schwarz USB NRP Zxx power meter kernel module driver
+ *
+ * Version: 4.0
+ *
+ * Copyright (c) 2012 by Rohde & Scharz GmbH & Co. KG
+ * written by Stefani Seibold <stefani [at] seibold>
+ *
+ * This driver supports the RF Power Meter R&S ® NRP Sensors. These
+ * sensors are intelligent standalone instruments that communicate via USB
+ * and support the following measurements:
+ *
+ * - Dynamic range: up to 90 dB (sensor dependent)
+ * - Level range: -67 dBm to +45 dBm (sensor dependent)
+ * - Frequency range: DC to 67 GHz (sensor dependent)
+ * - Measurement speed: 1500 measurements per second in the buffered mode
+ * - Linearity uncertainty: 0.007 dB
+ * - Precise average power measurements irrespective of modulation and bandwidth
+ * - Flexible measurements on up to 128 time slots per power sensor
+ * - S parameter correction of components between sensor and test object
+ *
+ * History:
+ *
+ * 2012_05_18 - 4.0 - revamp for kernel inclusion
+ * 2007_12_07 - 3.0 - Rewritten, multi urbs for read and write
+ * 2007_05_15 - 2.0 - Ported the driver to kernel 2.6, mostly rewritten
+ * 2001_05_01 - 0.1 - first version
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License.
+ *
+ */
+
+/*
+ * Internal format of the NRPZ USB messages:
+ *
+ * Byte 0: Signature
+ * Byte 1: Error Code
+ * Byte 2/3: Array Index
+ * or State (Byte 2)
+ * or Group Number (Byte 2) / Param Number (Byte 3)
+ * Byte 4-15: Data depening on signature type (Byte 0):
+ * floats, bit fields and integer are 32 bit
+ *
+ * Signature types:
+ * 'E': single float value
+ * 'e': single value
+ * 'A': float array
+ * 'P': auxiliary or peak float array
+ * 'a': interim float array
+ * 'p': interim auxiliary or peak float array
+ * 'F': feature bit field
+ * 'U': float parameter (32 bit)
+ * 'V': bit field parameter (32 bit)
+ * 'W': integer parameter (32 bit)
+ * 'T': string data
+ * 'R': receipt data (string or binary data)
+ * 'X': internal error
+ * 'Z': current state (Byte 2)
+ * 'z': life sign package
+ * 'L': float value (32 bit)
+ * 'M': bit field value (32 bit)
+ * 'N': long value (32 bit)
+ * 'B': binary data
+ *
+ * State values:
+ * 0: trigger idle
+ * 1: trigger reserved
+ * 2: wait for trigger
+ * 3: trigger measuring
+ *
+ * Communication in direction from host to the device are mostly like SCPI.
+ */
+
+#include <linux/kernel.h>
+#include <linux/sched.h>
+#include <linux/signal.h>
+#include <linux/errno.h>
+#include <linux/poll.h>
+#include <linux/init.h>
+#include <linux/slab.h>
+#include <linux/fcntl.h>
+#include <linux/module.h>
+#include <linux/spinlock.h>
+#include <linux/list.h>
+#include <linux/usb.h>
+#include <linux/version.h>
+#include <linux/kref.h>
+#include <linux/bitops.h>
+#include <linux/mutex.h>
+
+#include <linux/usb/nrpzmodule.h>
+
+/* Version Information */
+#define DRIVER_VERSION "v4.00"
+#define DRIVER_AUTHOR "Stefani Seibold <stefani [at] seibold>"
+#define DRIVER_DESC "Rohde&Schwarz NRP-Zxx USB power meter driver"
+
+/* Get a minor range for your devices from the usb maintainer */
+#define NRPZ_MINOR_BASE 192
+
+#define USB_RS_VENDOR_ID 0x0aad
+#define USB_NRP_PRODUCT_ID 0x0002
+#define USB_NRPZ21_PRODUCT_ID 0x0003
+#define USB_NRPFU_PRODUCT_ID 0x0004
+#define USB_FSHZ1_PRODUCT_ID 0x000b
+#define USB_NRPZ11_PRODUCT_ID 0x000c
+#define USB_NRPZ22_PRODUCT_ID 0x0013
+#define USB_NRPZ23_PRODUCT_ID 0x0014
+#define USB_NRPZ24_PRODUCT_ID 0x0015
+#define USB_NRPZ51_PRODUCT_ID 0x0016
+#define USB_NRPZ52_PRODUCT_ID 0x0017
+#define USB_NRPZ55_PRODUCT_ID 0x0018
+#define USB_NRPZ56_PRODUCT_ID 0x0019
+#define USB_FSHZ18_PRODUCT_ID 0x001a
+#define USB_NRPZ91_PRODUCT_ID 0x0021
+#define USB_NRPZ81_PRODUCT_ID 0x0023
+#define USB_NRPZ31_PRODUCT_ID 0x002c
+#define USB_NRPZ37_PRODUCT_ID 0x002d
+#define USB_NRPZ96_PRODUCT_ID 0x002e
+#define USB_NRPZ27_PRODUCT_ID 0x002f
+#define USB_NRPZ28_PRODUCT_ID 0x0051
+#define USB_NRPZ98_PRODUCT_ID 0x0052
+#define USB_NRPZ92_PRODUCT_ID 0x0062
+#define USB_NRPZ57_PRODUCT_ID 0x0070
+#define USB_NRPZ85_PRODUCT_ID 0x0083
+#define USB_NRPC40_PRODUCT_ID 0x008F
+#define USB_NRPC50_PRODUCT_ID 0x0090
+#define USB_NRPZ86_PRODUCT_ID 0x0095
+#define USB_NRPZ41_PRODUCT_ID 0x0096
+#define USB_NRPZ61_PRODUCT_ID 0x0097
+#define USB_NRPZ71_PRODUCT_ID 0x0098
+#define USB_NRPZ32_PRODUCT_ID 0x009A
+#define USB_NRPZ211_PRODUCT_ID 0x00A6
+#define USB_NRPZ221_PRODUCT_ID 0x00A7
+#define USB_NRPZ58_PRODUCT_ID 0x00A8
+#define USB_NRPC33_PRODUCT_ID 0x00B6
+#define USB_NRPC18_PRODUCT_ID 0x00BF
+
+/* table of devices that work with this driver */
+static struct usb_device_id nrpz_table[] = {
+ {USB_DEVICE(USB_RS_VENDOR_ID, USB_NRP_PRODUCT_ID)},
+ {USB_DEVICE(USB_RS_VENDOR_ID, USB_NRPZ21_PRODUCT_ID)},
+ {USB_DEVICE(USB_RS_VENDOR_ID, USB_NRPFU_PRODUCT_ID)},
+ {USB_DEVICE(USB_RS_VENDOR_ID, USB_FSHZ1_PRODUCT_ID)},
+ {USB_DEVICE(USB_RS_VENDOR_ID, USB_NRPZ11_PRODUCT_ID)},
+ {USB_DEVICE(USB_RS_VENDOR_ID, USB_NRPZ22_PRODUCT_ID)},
+ {USB_DEVICE(USB_RS_VENDOR_ID, USB_NRPZ23_PRODUCT_ID)},
+ {USB_DEVICE(USB_RS_VENDOR_ID, USB_NRPZ24_PRODUCT_ID)},
+ {USB_DEVICE(USB_RS_VENDOR_ID, USB_NRPZ51_PRODUCT_ID)},
+ {USB_DEVICE(USB_RS_VENDOR_ID, USB_NRPZ52_PRODUCT_ID)},
+ {USB_DEVICE(USB_RS_VENDOR_ID, USB_NRPZ55_PRODUCT_ID)},
+ {USB_DEVICE(USB_RS_VENDOR_ID, USB_NRPZ56_PRODUCT_ID)},
+ {USB_DEVICE(USB_RS_VENDOR_ID, USB_NRPZ57_PRODUCT_ID)},
+ {USB_DEVICE(USB_RS_VENDOR_ID, USB_FSHZ18_PRODUCT_ID)},
+ {USB_DEVICE(USB_RS_VENDOR_ID, USB_NRPZ91_PRODUCT_ID)},
+ {USB_DEVICE(USB_RS_VENDOR_ID, USB_NRPZ81_PRODUCT_ID)},
+ {USB_DEVICE(USB_RS_VENDOR_ID, USB_NRPZ31_PRODUCT_ID)},
+ {USB_DEVICE(USB_RS_VENDOR_ID, USB_NRPZ37_PRODUCT_ID)},
+ {USB_DEVICE(USB_RS_VENDOR_ID, USB_NRPZ96_PRODUCT_ID)},
+ {USB_DEVICE(USB_RS_VENDOR_ID, USB_NRPZ27_PRODUCT_ID)},
+ {USB_DEVICE(USB_RS_VENDOR_ID, USB_NRPZ28_PRODUCT_ID)},
+ {USB_DEVICE(USB_RS_VENDOR_ID, USB_NRPZ98_PRODUCT_ID)},
+ {USB_DEVICE(USB_RS_VENDOR_ID, USB_NRPZ92_PRODUCT_ID)},
+ {USB_DEVICE(USB_RS_VENDOR_ID, USB_NRPZ85_PRODUCT_ID)},
+ {USB_DEVICE(USB_RS_VENDOR_ID, USB_NRPC40_PRODUCT_ID)},
+ {USB_DEVICE(USB_RS_VENDOR_ID, USB_NRPC50_PRODUCT_ID)},
+ {USB_DEVICE(USB_RS_VENDOR_ID, USB_NRPZ86_PRODUCT_ID)},
+ {USB_DEVICE(USB_RS_VENDOR_ID, USB_NRPZ41_PRODUCT_ID)},
+ {USB_DEVICE(USB_RS_VENDOR_ID, USB_NRPZ61_PRODUCT_ID)},
+ {USB_DEVICE(USB_RS_VENDOR_ID, USB_NRPZ71_PRODUCT_ID)},
+ {USB_DEVICE(USB_RS_VENDOR_ID, USB_NRPZ32_PRODUCT_ID)},
+ {USB_DEVICE(USB_RS_VENDOR_ID, USB_NRPZ211_PRODUCT_ID)},
+ {USB_DEVICE(USB_RS_VENDOR_ID, USB_NRPZ221_PRODUCT_ID)},
+ {USB_DEVICE(USB_RS_VENDOR_ID, USB_NRPZ58_PRODUCT_ID)},
+ {USB_DEVICE(USB_RS_VENDOR_ID, USB_NRPC33_PRODUCT_ID)},
+ {USB_DEVICE(USB_RS_VENDOR_ID, USB_NRPC18_PRODUCT_ID)},
+ {} /* Terminating entry */
+};
+MODULE_DEVICE_TABLE(usb, nrpz_table);
+
+struct usb_nrpz {
+ struct usb_interface *intf; /* usb interface */
+
+ size_t in_size; /* size of the receive buffer */
+ size_t out_size; /* size of the send buffer */
+ __u8 in_epAddr; /* address of the bulk in endpoint */
+ __u8 out_epAddr; /* address of the bulk out endpoint */
+ bool in_use; /* in use flag */
+ bool connected; /* true if device is still connected */
+
+ struct kref kref;
+ wait_queue_head_t wq;
+
+ struct usb_anchor out_running; /* list of in use output buffers */
+ struct list_head out_avail; /* list of available output buffers */
+ spinlock_t write_lock; /* spinlock for transmit list */
+ struct mutex write_mutex; /* exclusive write data semaphore */
+
+ struct usb_anchor in_running; /* list of in use input buffers */
+ struct list_head in_avail; /* list of available input buffers */
+ spinlock_t read_lock; /* spinlock for receive list */
+ struct mutex read_mutex; /* exclusive read data semaphore */
+
+ struct urb out_urbs[64]; /* array of urb's for output */
+ struct urb in_urbs[64]; /* array of urb's for input */
+};
+
+/* forward declaration */
+static struct usb_driver nrpz_driver;
+
+static inline void urb_list_add(spinlock_t *lock, struct urb *urb,
+ struct list_head *head)
+{
+ spin_lock_irq(lock);
+ list_add(&urb->urb_list, head);
+ spin_unlock_irq(lock);
+}
+
+static inline void urb_list_add_tail(spinlock_t *lock, struct urb *urb,
+ struct list_head *head)
+{
+ spin_lock_irq(lock);
+ list_add_tail(&urb->urb_list, head);
+ spin_unlock_irq(lock);
+}
+
+static struct urb *urb_list_get(spinlock_t *lock, struct list_head *head)
+{
+ struct list_head *p;
+
+ spin_lock_irq(lock);
+
+ if (list_empty(head)) {
+ spin_unlock_irq(lock);
+ return NULL;
+ }
+
+ p = head->next;
+ list_del(p);
+ spin_unlock_irq(lock);
+
+ return container_of(p, struct urb, urb_list);
+}
+
+/*
+ * bulks_release
+ *
+ * release all allocated urb's and and usb buffers
+ */
+static void bulks_release(struct urb *urb, unsigned n, int buf_size)
+{
+ while (n--) {
+ if (urb->transfer_buffer)
+ usb_free_coherent(urb->dev,
+ buf_size,
+ urb->transfer_buffer,
+ urb->transfer_dma);
+ ++urb;
+ }
+}
+
+/*
+ * bulks_init
+ *
+ * preallocate urb's and and usb buffers
+ */
+static int bulks_init(struct usb_nrpz *dev,
+ struct usb_device *udev,
+ struct urb *urb,
+ unsigned n,
+ unsigned int pipe,
+ int buf_size,
+ usb_complete_t complete_fn)
+{
+ void *buffer;
+
+ while (n--) {
+ usb_init_urb(urb);
+
+ buffer = usb_alloc_coherent(udev,
+ buf_size,
+ GFP_KERNEL,
+ &urb->transfer_dma);
+ if (!buffer)
+ return -ENOMEM;
+
+ /* set up our read urb */
+ usb_fill_bulk_urb(urb,
+ udev,
+ pipe,
+ buffer,
+ buf_size,
+ complete_fn,
+ dev);
+
+ urb->transfer_flags = URB_NO_TRANSFER_DMA_MAP;
+
+ ++urb;
+ }
+ return 0;
+}
+
+static int bulks_in_submit(struct usb_nrpz *dev)
+{
+ int ret;
+ unsigned i;
+
+ for (i = 0; i != ARRAY_SIZE(dev->in_urbs); ++i) {
+ usb_anchor_urb(&dev->in_urbs[i], &dev->in_running);
+
+ ret = usb_submit_urb(&dev->in_urbs[i], GFP_KERNEL);
+ if (ret) {
+ usb_kill_anchored_urbs(&dev->in_running);
+ return ret;
+ }
+ }
+ return 0;
+}
+
+static void nrpz_read_callback(struct urb *urb)
+{
+ struct usb_nrpz *dev = (struct usb_nrpz *)urb->context;
+
+ if (urb->status) {
+ if (!(urb->status == -ENOENT ||
+ urb->status == -EPIPE ||
+ urb->status == -ECONNRESET ||
+ urb->status == -ESHUTDOWN))
+ dev_err(&urb->dev->dev,
+ "Nonzero read bulk status: %d", urb->status);
+ }
+
+ spin_lock(&dev->read_lock);
+ list_add_tail(&urb->urb_list, &dev->in_avail);
+ spin_unlock(&dev->read_lock);
+ wake_up_all(&dev->wq);
+}
+
+static void nrpz_write_callback(struct urb *urb)
+{
+ struct usb_nrpz *dev = (struct usb_nrpz *)urb->context;
+
+ if (urb->status) {
+ if (!(urb->status == -ENOENT ||
+ urb->status == -EPIPE ||
+ urb->status == -ECONNRESET ||
+ urb->status == -ESHUTDOWN))
+ dev_err(&urb->dev->dev,
+ "Nonzero write bulk status: %d", urb->status);
+ }
+
+ spin_lock(&dev->write_lock);
+ list_add_tail(&urb->urb_list, &dev->out_avail);
+ spin_unlock(&dev->write_lock);
+ wake_up_all(&dev->wq);
+}
+
+static void nrpz_delete(struct kref *kref)
+{
+ struct usb_nrpz *dev = container_of(kref, struct usb_nrpz, kref);
+
+ usb_put_dev(interface_to_usbdev(dev->intf));
+ kfree(dev);
+}
+
+static ssize_t nrpz_read(struct file *file, char __user *buffer, size_t count,
+ loff_t *ppos)
+{
+ struct usb_nrpz *dev = file->private_data;
+ int ret;
+ struct urb *urb;
+ size_t n;
+
+ /* verify that we actually have some data to read */
+ if (!count)
+ return 0;
+
+ /* lock the read data */
+ if (file->f_flags & O_NONBLOCK) {
+ if (!mutex_trylock(&dev->read_mutex))
+ return -EAGAIN;
+ } else {
+ ret = mutex_lock_interruptible(&dev->read_mutex);
+ if (ret)
+ return ret;
+ }
+
+ for (;;) {
+ urb = urb_list_get(&dev->read_lock, &dev->in_avail);
+ if (urb)
+ break;
+
+ /* verify that the device wasn't unplugged */
+ if (!dev->connected) {
+ ret = -ENODEV;
+ goto exit;
+ }
+
+ if (file->f_flags & O_NONBLOCK) {
+ ret = -EAGAIN;
+ goto exit;
+ }
+
+ ret = wait_event_interruptible(dev->wq,
+ !list_empty(&dev->in_avail) || !dev->connected);
+ if (ret) {
+ ret = -ERESTARTSYS;
+ goto exit;
+ }
+ }
+
+ if (!urb->status) {
+ n = min(count, urb->actual_length);
+
+ if (copy_to_user(buffer, urb->transfer_buffer, n)) {
+ urb_list_add(&dev->read_lock, urb, &dev->in_avail);
+ ret = -EFAULT;
+ goto exit;
+ }
+ } else {
+ n = -EPIPE;
+ }
+
+ usb_anchor_urb(urb, &dev->in_running);
+
+ ret = usb_submit_urb(urb, GFP_KERNEL);
+ if (ret) {
+ usb_unanchor_urb(urb);
+ urb_list_add_tail(&dev->read_lock, urb, &dev->in_avail);
+ urb->status = ret;
+ dev_err(&urb->dev->dev,
+ "Failed submitting read urb (error %d)", ret);
+ }
+
+ ret = n;
+exit:
+ mutex_unlock(&dev->read_mutex);
+ return ret;
+}
+
+static ssize_t nrpz_write(struct file *file, const char __user *buffer,
+ size_t count, loff_t *ppos)
+{
+ struct usb_nrpz *dev = file->private_data;
+ int ret;
+ size_t len = 0;
+ struct urb *urb;
+ size_t n;
+
+ /* lock the write data */
+ if (file->f_flags & O_NONBLOCK) {
+ if (!mutex_trylock(&dev->write_mutex))
+ return -EAGAIN;
+ } else {
+ ret = mutex_lock_interruptible(&dev->write_mutex);
+ if (ret)
+ return ret;
+ }
+
+ do {
+ for (;;) {
+ /* verify that the device wasn't unplugged */
+ if (!dev->connected) {
+ ret = -ENODEV;
+ goto exit;
+ }
+
+ urb = urb_list_get(&dev->write_lock, &dev->out_avail);
+ if (urb)
+ break;
+
+ if (file->f_flags & O_NONBLOCK) {
+ ret = -EAGAIN;
+ goto exit;
+ }
+
+ ret = wait_event_interruptible(dev->wq,
+ !list_empty(&dev->out_avail) || !dev->connected);
+ if (ret) {
+ ret = -ERESTARTSYS;
+ goto exit;
+ }
+ }
+
+ n = min(count, dev->out_size);
+
+ if (copy_from_user(urb->transfer_buffer, buffer, n)) {
+ urb_list_add(&dev->write_lock, urb, &dev->out_avail);
+ ret = -EFAULT;
+ break;
+ }
+
+ urb->transfer_buffer_length = n;
+
+ usb_anchor_urb(urb, &dev->out_running);
+
+ ret = usb_submit_urb(urb, GFP_KERNEL);
+ if (ret) {
+ usb_unanchor_urb(urb);
+ urb_list_add_tail(&dev->write_lock, urb, &dev->out_avail);
+ dev_err(&urb->dev->dev,
+ "Failed submitting write urb (error %d)", ret);
+ break;
+ }
+
+ count -= n;
+ buffer += n;
+ len += n;
+ } while (count);
+exit:
+ if (len)
+ ret = len;
+
+ mutex_unlock(&dev->write_mutex);
+ return ret;
+}
+
+#define VRT_RESET_ALL 1
+#define VRT_GET_DEVICE_INFO 6
+#define VRI_DEVICE_NAME 5
+#define DEVICE_STATE_SIZE 128
+
+static long nrpz_compat_ioctl(struct file *file, unsigned int cmd,
+ unsigned long arg)
+{
+ struct usb_nrpz *dev = file->private_data;
+ struct usb_interface *intf = dev->intf;
+ struct usb_device *udev;
+ int ret;
+
+ /* verify that the device wasn't unplugged */
+ if (!dev->connected)
+ return -ENODEV;
+
+ udev = interface_to_usbdev(intf);
+
+ switch (cmd) {
+ case NRPZ_GETSENSORINFO:
+ {
+ struct nrpz_sensor_info __user *sensor_info =
+ (struct nrpz_sensor_info __user *)arg;
+
+ if (!access_ok(VERIFY_WRITE, sensor_info, sizeof(*sensor_info)))
+ return -EFAULT;
+
+ __put_user(udev->descriptor.bcdDevice,
+ &sensor_info->bcdDevice);
+ __put_user(udev->descriptor.bcdUSB,
+ &sensor_info->bcdUSB);
+ __put_user(udev->descriptor.bDescriptorType,
+ &sensor_info->bDescriptorType);
+ __put_user(udev->descriptor.bDeviceClass,
+ &sensor_info->bDeviceClass);
+ __put_user(udev->descriptor.bDeviceSubClass,
+ &sensor_info->bDeviceSubClass);
+ __put_user(udev->descriptor.bDeviceProtocol,
+ &sensor_info->bDeviceProtocol);
+ __put_user(udev->descriptor.bMaxPacketSize0,
+ &sensor_info->bMaxPacketSize0);
+ __put_user(udev->descriptor.bNumConfigurations,
+ &sensor_info->bNumConfigurations);
+ __put_user(udev->descriptor.iManufacturer,
+ &sensor_info->iManufacturer);
+ __put_user(udev->descriptor.iProduct,
+ &sensor_info->iProduct);
+ __put_user(udev->descriptor.iSerialNumber,
+ &sensor_info->iSerialNumber);
+ __put_user(udev->descriptor.idVendor,
+ &sensor_info->vendorId);
+ __put_user(udev->descriptor.idProduct,
+ &sensor_info->productId);
+ usb_string(udev, udev->descriptor.iManufacturer,
+ (char __force *)sensor_info->manufacturer,
+ sizeof(sensor_info->manufacturer));
+ usb_string(udev, udev->descriptor.iProduct,
+ (char __force *)sensor_info->productName,
+ sizeof(sensor_info->productName));
+ usb_string(udev, udev->descriptor.iSerialNumber,
+ (char __force *)sensor_info->serialNumber,
+ sizeof(sensor_info->serialNumber));
+
+ return 0;
+ }
+ case NRPZ_START:
+ {
+ u8 *device_state;
+
+ device_state = kzalloc(DEVICE_STATE_SIZE, GFP_KERNEL | GFP_DMA);
+
+ ret = usb_control_msg(udev,
+ usb_rcvctrlpipe(udev, 0),
+ VRT_GET_DEVICE_INFO,
+ USB_DIR_IN | USB_TYPE_VENDOR | USB_RECIP_DEVICE,
+ 0,
+ VRI_DEVICE_NAME,
+ device_state,
+ DEVICE_STATE_SIZE,
+ 5000);
+
+ if (ret < 0)
+ goto done;
+
+ dev_dbg(&intf->dev,
+ "device state:%s", device_state);
+
+ if (strncmp(device_state, "Boot ", 5)) {
+ ret = 0;
+ goto done;
+ }
+
+ ret = usb_control_msg(udev,
+ usb_sndctrlpipe(udev, 0),
+ VRT_RESET_ALL,
+ USB_DIR_OUT | USB_TYPE_VENDOR | USB_RECIP_DEVICE,
+ 1,
+ 1,
+ device_state,
+ sizeof(device_state),
+ 5000);
+done:
+ kfree(device_state);
+ return ret;
+ }
+ case NRPZ_WRITE_DONE:
+ if (arg) {
+ ret = wait_event_interruptible_timeout(
+ dev->out_running.wait,
+ list_empty(&dev->out_running.urb_list),
+ msecs_to_jiffies(arg));
+ if (!ret)
+ return -ETIMEDOUT;
+ if (ret < 0)
+ return ret;
+ return 0;
+ } else {
+ return wait_event_interruptible(
+ dev->out_running.wait,
+ list_empty(&dev->out_running.urb_list));
+ }
+ break;
+ case NRPZ_VENDOR_CONTROL_MSG_OUT:
+ {
+ struct nrpz_control_req ncr;
+ u8 *data;
+ u16 size;
+
+ if (copy_from_user(&ncr, (struct nrpz_control_req __user *)arg, sizeof(ncr)))
+ return -EFAULT;
+
+ if (ncr.data) {
+ size = ncr.size;
+
+ if (!access_ok(VERIFY_READ, (void __user *)ncr.data, size))
+ return -EFAULT;
+
+ data = kzalloc(size, GFP_KERNEL | GFP_DMA);
+ if (!data)
+ return -ENOMEM;
+
+ memcpy(data, ncr.data, size);
+ } else {
+ size = 0;
+ data = NULL;
+ }
+
+ ret = usb_control_msg(udev,
+ usb_sndctrlpipe(udev, 0),
+ ncr.request,
+ ncr.type,
+ ncr.value,
+ ncr.index,
+ data,
+ size,
+ 0);
+
+
+ kfree(data);
+
+ return ret;
+ }
+ case NRPZ_VENDOR_CONTROL_MSG_IN:
+ {
+ struct nrpz_control_req ncr;
+ u8 *data;
+ u16 size;
+
+ if (copy_from_user(&ncr, (struct nrpz_control_req __user *)arg, sizeof(ncr)))
+ return -EFAULT;
+
+ if (ncr.data) {
+ size = ncr.size;
+
+ if (!access_ok(VERIFY_WRITE, (void __user *)ncr.data, size))
+ return -EFAULT;
+
+ data = kzalloc(size, GFP_KERNEL | GFP_DMA);
+ if (!data)
+ return -ENOMEM;
+ } else {
+ size = 0;
+ data = NULL;
+ }
+
+ ret = usb_control_msg(udev,
+ usb_rcvctrlpipe(udev, 0),
+ ncr.request,
+ ncr.type,
+ ncr.value,
+ ncr.index,
+ data,
+ size,
+ 0);
+
+ if (data) {
+ memcpy(ncr.data, data, size);
+ kfree(data);
+ }
+
+ return ret;
+ }
+ default:
+ dev_dbg(&intf->dev,
+ "Invalid ioctl call (%08x)", cmd);
+ return -ENOTTY;
+ }
+
+ return ret;
+}
+
+static long nrpz_ioctl(struct file *file, unsigned int cmd,
+ unsigned long arg)
+{
+ return nrpz_compat_ioctl(file, cmd, arg);
+}
+
+static int nrpz_open(struct inode *inode, struct file *file)
+{
+ struct usb_nrpz *dev;
+ int minor;
+ struct usb_interface *intf;
+ struct usb_device *udev;
+ int ret;
+ unsigned i;
+
+ minor = iminor(inode);
+
+ intf = usb_find_interface(&nrpz_driver, minor);
+ if (!intf)
+ return -ENODEV;
+
+ dev = usb_get_intfdata(intf);
+ if (!dev || !dev->connected)
+ return -ENODEV;
+
+ spin_lock_irq(&dev->read_lock);
+ if (dev->in_use) {
+ spin_unlock_irq(&dev->read_lock);
+ return -EBUSY;
+ }
+ dev->in_use = true;
+ spin_unlock_irq(&dev->read_lock);
+
+ /* increment our usage count for the device */
+ kref_get(&dev->kref);
+
+ /* save our object in the file's private structure */
+ file->private_data = dev;
+
+ INIT_LIST_HEAD(&dev->in_avail);
+ INIT_LIST_HEAD(&dev->out_avail);
+
+ udev = interface_to_usbdev(intf);
+
+ ret = bulks_init(dev,
+ udev,
+ dev->in_urbs,
+ ARRAY_SIZE(dev->in_urbs),
+ usb_rcvbulkpipe(udev, dev->in_epAddr),
+ dev->in_size,
+ nrpz_read_callback);
+ if (ret)
+ goto error;
+
+ ret = bulks_init(dev,
+ udev,
+ dev->out_urbs,
+ ARRAY_SIZE(dev->out_urbs),
+ usb_sndbulkpipe(udev, dev->out_epAddr),
+ dev->out_size,
+ nrpz_write_callback);
+ if (ret)
+ goto error;
+
+ ret = bulks_in_submit(dev);
+ if (ret)
+ goto error;
+
+ for (i = 0; i != ARRAY_SIZE(dev->out_urbs); ++i)
+ list_add(&dev->out_urbs[i].urb_list, &dev->out_avail);
+
+ return 0;
+error:
+ bulks_release(dev->out_urbs, ARRAY_SIZE(dev->out_urbs), dev->out_size);
+ bulks_release(dev->in_urbs, ARRAY_SIZE(dev->in_urbs), dev->in_size);
+
+ return 0;
+}
+
+static int nrpz_release(struct inode *inode, struct file *file)
+{
+ struct usb_nrpz *dev = file->private_data;
+
+ if (dev == NULL)
+ return -ENODEV;
+
+ usb_kill_anchored_urbs(&dev->in_running);
+ usb_kill_anchored_urbs(&dev->out_running);
+
+ bulks_release(dev->out_urbs, ARRAY_SIZE(dev->out_urbs), dev->out_size);
+ bulks_release(dev->in_urbs, ARRAY_SIZE(dev->in_urbs), dev->in_size);
+
+ /* decrement the count on our device */
+ kref_put(&dev->kref, nrpz_delete);
+
+ spin_lock_irq(&dev->read_lock);
+ dev->in_use = false;
+ spin_unlock_irq(&dev->read_lock);
+
+ return 0;
+}
+
+static int nrpz_fsync(struct file *file, loff_t start, loff_t end, int datasync)
+{
+ struct usb_nrpz *dev = file->private_data;
+ int ret;
+
+ if (dev == NULL)
+ return -ENODEV;
+
+ /* lock the write data */
+ ret = mutex_lock_interruptible(&dev->write_mutex);
+ if (ret)
+ return ret;
+
+ /* verify that the device wasn't unplugged */
+ if (!dev->connected) {
+ ret = -ENODEV;
+ goto exit;
+ }
+
+ ret = wait_event_interruptible(dev->out_running.wait,
+ list_empty(&dev->out_running.urb_list));
+ if (ret) {
+ ret = -ERESTARTSYS;
+ goto exit;
+ }
+exit:
+ mutex_unlock(&dev->write_mutex);
+
+ return ret;
+}
+
+static int nrpz_flush(struct file *file, fl_owner_t id)
+{
+ return nrpz_fsync(file, 0, LLONG_MAX, 0);
+}
+
+static unsigned int nrpz_poll(struct file *file, poll_table *wait)
+{
+ struct usb_nrpz *dev = file->private_data;
+ int ret = 0;
+
+ poll_wait(file, &dev->wq, wait);
+
+ if (!dev->connected)
+ ret = POLLIN | POLLOUT | POLLPRI | POLLERR | POLLHUP;
+ else {
+ if (!list_empty(&dev->in_avail))
+ ret |= POLLIN;
+
+ if (!list_empty(&dev->out_avail))
+ ret |= POLLOUT;
+ }
+
+ return ret;
+}
+
+static const struct file_operations nrpz_fops = {
+ .owner = THIS_MODULE,
+ .read = nrpz_read,
+ .write = nrpz_write,
+ .unlocked_ioctl = nrpz_ioctl,
+ .compat_ioctl = nrpz_compat_ioctl,
+ .open = nrpz_open,
+ .release = nrpz_release,
+ .fsync = nrpz_fsync,
+ .flush = nrpz_flush,
+ .poll = nrpz_poll,
+ .llseek = noop_llseek,
+};
+
+static struct usb_class_driver nrpz_class = {
+ .name = "nrpz%d",
+ .fops = &nrpz_fops,
+ .minor_base = NRPZ_MINOR_BASE,
+};
+
+static int nrpz_probe(struct usb_interface *intf,
+ const struct usb_device_id *id)
+{
+ int i;
+ int ret;
+ struct usb_endpoint_descriptor *endpoint;
+ struct usb_host_interface *iface_desc;
+ struct usb_nrpz *dev;
+ unsigned minor;
+
+ /* allocate memory for our device state and intialize it */
+ dev = kzalloc(sizeof(*dev), GFP_KERNEL);
+ if (!dev) {
+ dev_err(&intf->dev, "Out of memory");
+ return -ENOMEM;
+ }
+
+ ret = -EIO;
+
+ init_waitqueue_head(&dev->wq);
+ kref_init(&dev->kref);
+
+ mutex_init(&dev->read_mutex);
+ mutex_init(&dev->write_mutex);
+
+ spin_lock_init(&dev->read_lock);
+ spin_lock_init(&dev->write_lock);
+
+ init_usb_anchor(&dev->in_running);
+ init_usb_anchor(&dev->out_running);
+
+ usb_get_dev(interface_to_usbdev(intf));
+
+ dev->in_use = false;
+ dev->intf = intf;
+ dev->connected = true;
+
+ /* set up the endpoint information */
+ /* check out the endpoints */
+ iface_desc = intf->cur_altsetting;
+ for (i = 0; i < iface_desc->desc.bNumEndpoints; ++i) {
+ endpoint = &iface_desc->endpoint[i].desc;
+
+ if (!dev->in_epAddr && usb_endpoint_is_bulk_in(endpoint)) {
+ /* we found a bulk in endpoint */
+ dev->in_size = le16_to_cpu(endpoint->wMaxPacketSize);
+ dev->in_epAddr = endpoint->bEndpointAddress;
+ } else
+ if (!dev->out_epAddr && usb_endpoint_is_bulk_out(endpoint)) {
+ /* we found a bulk out endpoint */
+ dev->out_size = le16_to_cpu(endpoint->wMaxPacketSize);
+ dev->out_epAddr = endpoint->bEndpointAddress;
+ }
+ }
+ if (!(dev->in_epAddr && dev->out_epAddr)) {
+ dev_err(&intf->dev,
+ "Could not find both bulk in and out endpoints");
+ goto error;
+ }
+
+ usb_set_intfdata(intf, dev);
+
+ ret = usb_register_dev(intf, &nrpz_class);
+ if (ret) {
+ dev_err(&intf->dev,
+ "Not able to get a minor for this device\n");
+ goto error;
+ }
+
+ minor = intf->minor - NRPZ_MINOR_BASE;
+
+ /* let the user know what node this device is now attached to */
+ dev_info(&intf->dev,
+ "Device now attached to nrpz%u", minor);
+
+ return 0;
+error:
+ usb_set_intfdata(intf, NULL);
+ nrpz_delete(&dev->kref);
+ return ret;
+}
+
+static void nrpz_disconnect(struct usb_interface *intf)
+{
+ struct usb_nrpz *dev;
+
+ dev_info(&intf->dev,
+ "nrpz%u disconnected", intf->minor - NRPZ_MINOR_BASE);
+
+ dev = usb_get_intfdata(intf);
+ usb_set_intfdata(intf, NULL);
+
+ /* give back our minor */
+ usb_deregister_dev(intf, &nrpz_class);
+
+ /* prevent more I/O from starting */
+ dev->connected = false;
+
+ usb_kill_anchored_urbs(&dev->in_running);
+ usb_kill_anchored_urbs(&dev->out_running);
+
+ wake_up_all(&dev->wq);
+
+ /* decrement our usage count */
+ kref_put(&dev->kref, nrpz_delete);
+}
+
+static void nrpz_draw_down(struct usb_nrpz *dev)
+{
+ int time;
+
+ time = usb_wait_anchor_empty_timeout(&dev->out_running, 1000);
+ if (!time)
+ usb_kill_anchored_urbs(&dev->out_running);
+
+ usb_kill_anchored_urbs(&dev->in_running);
+}
+
+static int nrpz_suspend(struct usb_interface *intf, pm_message_t message)
+{
+ struct usb_nrpz *dev = usb_get_intfdata(intf);
+
+ if (dev)
+ nrpz_draw_down(dev);
+ return 0;
+}
+
+static int nrpz_resume(struct usb_interface *intf)
+{
+ struct usb_nrpz *dev = usb_get_intfdata(intf);
+
+ if (dev)
+ return bulks_in_submit(dev);
+ return 0;
+}
+
+static struct usb_driver nrpz_driver = {
+ .name = "nrpz",
+ .probe = nrpz_probe,
+ .disconnect = nrpz_disconnect,
+ .suspend = nrpz_suspend,
+ .resume = nrpz_resume,
+ .id_table = nrpz_table,
+};
+
+module_usb_driver(nrpz_driver);
+
+MODULE_AUTHOR(DRIVER_AUTHOR);
+MODULE_DESCRIPTION(DRIVER_DESC);
+MODULE_LICENSE("GPL");
diff --git a/include/linux/usb/nrpzmodule.h b/include/linux/usb/nrpzmodule.h
new file mode 100644
index 0000000..8a39aef
--- /dev/null
+++ b/include/linux/usb/nrpzmodule.h
@@ -0,0 +1,47 @@
+#ifndef __NRPZMODULE_H_
+#define __NRPZMODULE_H_
+
+#include "linux/ioctl.h"
+
+#define NRPZ_IOC_MAGIC 'N'
+
+#define NRPZ_GETSENSORINFO _IOR(NRPZ_IOC_MAGIC, 0x01, struct nrpz_sensor_info *)
+#define NRPZ_START _IO(NRPZ_IOC_MAGIC, 0x02)
+#define NRPZ_WRITE_DONE _IOW(NRPZ_IOC_MAGIC, 0x03, unsigned long)
+#define NRPZ_VENDOR_CONTROL_MSG _IOW(NRPZ_IOC_MAGIC, 0x06, struct nrpz_control_req *)
+#define NRPZ_VENDOR_CONTROL_MSG_OUT _IOW(NRPZ_IOC_MAGIC, 0x06, struct nrpz_control_req *)
+#define NRPZ_VENDOR_CONTROL_MSG_IN _IOW(NRPZ_IOC_MAGIC, 0x07, struct nrpz_control_req *)
+
+struct nrpz_sensor_info {
+ unsigned char bDescriptorType;
+ unsigned short bcdUSB;
+ unsigned char bDeviceClass;
+ unsigned char bDeviceSubClass;
+ unsigned char bDeviceProtocol;
+ unsigned char bMaxPacketSize0;
+ unsigned short vendorId;
+ unsigned short productId;
+ unsigned short bcdDevice;
+ unsigned char iManufacturer;
+ unsigned char iProduct;
+ unsigned char iSerialNumber;
+ unsigned char bNumConfigurations;
+ char protocol[128];
+ char manufacturer[128];
+ char productName[128];
+ char serialNumber[128];
+};
+
+/*
+ * struct for NRPZ_VENDOR_CONTROL
+ */
+struct nrpz_control_req {
+ unsigned char request;
+ unsigned char type;
+ unsigned short value;
+ unsigned short index;
+ unsigned char *data;
+ unsigned short size;
+};
+
+#endif
--
1.7.8.6

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


oliver at neukum

Jun 2, 2012, 1:24 PM

Post #24 of 28 (632 views)
Permalink
Re: [PATCH] add new NRP power meter USB device driver [In reply to]

Am Samstag, 2. Juni 2012, 18:18:59 schrieb stefani [at] seibold:
> From: Stefani Seibold <stefani [at] seibold>
>
> This driver supports all of the Rohde&Schwarz RF Power Meter NRP Sensors. These
> sensors are intelligent standalone instruments that communicate via USB.

Hi,

I am commenting here only on the code as such, not on whether it should be
included.


> +static int bulks_init(struct usb_nrpz *dev,
> + struct usb_device *udev,
> + struct urb *urb,
> + unsigned n,
> + unsigned int pipe,
> + int buf_size,
> + usb_complete_t complete_fn)
> +{
> + void *buffer;
> +
> + while (n--) {
> + usb_init_urb(urb);
> +
> + buffer = usb_alloc_coherent(udev,
> + buf_size,
> + GFP_KERNEL,
> + &urb->transfer_dma);
> + if (!buffer)
> + return -ENOMEM;
> +
> + /* set up our read urb */
> + usb_fill_bulk_urb(urb,
> + udev,
> + pipe,
> + buffer,
> + buf_size,
> + complete_fn,
> + dev);
> +
> + urb->transfer_flags = URB_NO_TRANSFER_DMA_MAP;

I'd prefer |= in case the fill macros set a flag.

> +
> + ++urb;
> + }
> + return 0;
> +}
> +
> +static int bulks_in_submit(struct usb_nrpz *dev)
> +{
> + int ret;
> + unsigned i;
> +
> + for (i = 0; i != ARRAY_SIZE(dev->in_urbs); ++i) {
> + usb_anchor_urb(&dev->in_urbs[i], &dev->in_running);
> +
> + ret = usb_submit_urb(&dev->in_urbs[i], GFP_KERNEL);
> + if (ret) {
> + usb_kill_anchored_urbs(&dev->in_running);
> + return ret;
> + }
> + }
> + return 0;
> +}

This is used in the resume code path. During resume you
cannot swap, as the swap device may still be asleep.
Therefore GFP_NOIO should be used. It's considered
best practice to pass the gfp parameter to such methods.

> +static ssize_t nrpz_read(struct file *file, char __user *buffer, size_t count,
> + loff_t *ppos)
> +{
> + struct usb_nrpz *dev = file->private_data;
> + int ret;
> + struct urb *urb;
> + size_t n;
> +
> + /* verify that we actually have some data to read */
> + if (!count)
> + return 0;
> +
> + /* lock the read data */
> + if (file->f_flags & O_NONBLOCK) {
> + if (!mutex_trylock(&dev->read_mutex))
> + return -EAGAIN;

Congratulations. I overlooked this. Does skeleton do it right?

> + } else {
> + ret = mutex_lock_interruptible(&dev->read_mutex);
> + if (ret)
> + return ret;
> + }
> +
> + for (;;) {
> + urb = urb_list_get(&dev->read_lock, &dev->in_avail);
> + if (urb)
> + break;
> +
> + /* verify that the device wasn't unplugged */
> + if (!dev->connected) {
> + ret = -ENODEV;
> + goto exit;
> + }
> +
> + if (file->f_flags & O_NONBLOCK) {
> + ret = -EAGAIN;
> + goto exit;
> + }
> +
> + ret = wait_event_interruptible(dev->wq,
> + !list_empty(&dev->in_avail) || !dev->connected);
> + if (ret) {
> + ret = -ERESTARTSYS;
> + goto exit;
> + }
> + }
> +
> + if (!urb->status) {
> + n = min(count, urb->actual_length);
> +
> + if (copy_to_user(buffer, urb->transfer_buffer, n)) {
> + urb_list_add(&dev->read_lock, urb, &dev->in_avail);
> + ret = -EFAULT;
> + goto exit;
> + }
> + } else {
> + n = -EPIPE;
> + }
> +
> + usb_anchor_urb(urb, &dev->in_running);
> +
> + ret = usb_submit_urb(urb, GFP_KERNEL);
> + if (ret) {
> + usb_unanchor_urb(urb);
> + urb_list_add_tail(&dev->read_lock, urb, &dev->in_avail);
> + urb->status = ret;

That is a bug. You will report that error the next time the URB comes up.
That's wrong. I think you need a special error code that will cause you
to just resubmit the URB and go for the next URB.

> + dev_err(&urb->dev->dev,
> + "Failed submitting read urb (error %d)", ret);
> + }
> +
> + ret = n;
> +exit:
> + mutex_unlock(&dev->read_mutex);
> + return ret;
> +}

> +static long nrpz_compat_ioctl(struct file *file, unsigned int cmd,
> + unsigned long arg)
> +{
> + struct usb_nrpz *dev = file->private_data;
> + struct usb_interface *intf = dev->intf;
> + struct usb_device *udev;
> + int ret;
> +
> + /* verify that the device wasn't unplugged */
> + if (!dev->connected)
> + return -ENODEV;
> +
> + udev = interface_to_usbdev(intf);
> +
> + switch (cmd) {
> + case NRPZ_GETSENSORINFO:
> + {
> + struct nrpz_sensor_info __user *sensor_info =
> + (struct nrpz_sensor_info __user *)arg;
> +
> + if (!access_ok(VERIFY_WRITE, sensor_info, sizeof(*sensor_info)))
> + return -EFAULT;
> +
> + __put_user(udev->descriptor.bcdDevice,
> + &sensor_info->bcdDevice);
> + __put_user(udev->descriptor.bcdUSB,
> + &sensor_info->bcdUSB);
> + __put_user(udev->descriptor.bDescriptorType,
> + &sensor_info->bDescriptorType);
> + __put_user(udev->descriptor.bDeviceClass,
> + &sensor_info->bDeviceClass);
> + __put_user(udev->descriptor.bDeviceSubClass,
> + &sensor_info->bDeviceSubClass);
> + __put_user(udev->descriptor.bDeviceProtocol,
> + &sensor_info->bDeviceProtocol);
> + __put_user(udev->descriptor.bMaxPacketSize0,
> + &sensor_info->bMaxPacketSize0);
> + __put_user(udev->descriptor.bNumConfigurations,
> + &sensor_info->bNumConfigurations);
> + __put_user(udev->descriptor.iManufacturer,
> + &sensor_info->iManufacturer);
> + __put_user(udev->descriptor.iProduct,
> + &sensor_info->iProduct);
> + __put_user(udev->descriptor.iSerialNumber,
> + &sensor_info->iSerialNumber);
> + __put_user(udev->descriptor.idVendor,
> + &sensor_info->vendorId);
> + __put_user(udev->descriptor.idProduct,
> + &sensor_info->productId);
> + usb_string(udev, udev->descriptor.iManufacturer,
> + (char __force *)sensor_info->manufacturer,
> + sizeof(sensor_info->manufacturer));
> + usb_string(udev, udev->descriptor.iProduct,
> + (char __force *)sensor_info->productName,
> + sizeof(sensor_info->productName));
> + usb_string(udev, udev->descriptor.iSerialNumber,
> + (char __force *)sensor_info->serialNumber,
> + sizeof(sensor_info->serialNumber));
> +
> + return 0;
> + }
> + case NRPZ_START:
> + {
> + u8 *device_state;
> +
> + device_state = kzalloc(DEVICE_STATE_SIZE, GFP_KERNEL | GFP_DMA);

I apologize for misleading naming in the MM code. GFP_DMA goes back
to the time the ISA bus was alive and kicking. GFP_DMA means memory
that you can do DMA with with the old PC_AT ISA DMA controller. For USB
just GFP_KERNEL will do.

> + ret = usb_control_msg(udev,
> + usb_rcvctrlpipe(udev, 0),
> + VRT_GET_DEVICE_INFO,
> + USB_DIR_IN | USB_TYPE_VENDOR | USB_RECIP_DEVICE,
> + 0,
> + VRI_DEVICE_NAME,
> + device_state,
> + DEVICE_STATE_SIZE,
> + 5000);
> +
> + if (ret < 0)
> + goto done;
> +
> + dev_dbg(&intf->dev,
> + "device state:%s", device_state);
> +
> + if (strncmp(device_state, "Boot ", 5)) {
> + ret = 0;
> + goto done;
> + }
> +
> + ret = usb_control_msg(udev,
> + usb_sndctrlpipe(udev, 0),
> + VRT_RESET_ALL,
> + USB_DIR_OUT | USB_TYPE_VENDOR | USB_RECIP_DEVICE,
> + 1,
> + 1,
> + device_state,
> + sizeof(device_state),
> + 5000);
> +done:
> + kfree(device_state);
> + return ret;
> + }
> + case NRPZ_WRITE_DONE:
> + if (arg) {
> + ret = wait_event_interruptible_timeout(
> + dev->out_running.wait,
> + list_empty(&dev->out_running.urb_list),
> + msecs_to_jiffies(arg));
> + if (!ret)
> + return -ETIMEDOUT;
> + if (ret < 0)
> + return ret;
> + return 0;
> + } else {
> + return wait_event_interruptible(
> + dev->out_running.wait,
> + list_empty(&dev->out_running.urb_list));
> + }
> + break;
> + case NRPZ_VENDOR_CONTROL_MSG_OUT:
> + {
> + struct nrpz_control_req ncr;
> + u8 *data;
> + u16 size;
> +
> + if (copy_from_user(&ncr, (struct nrpz_control_req __user *)arg, sizeof(ncr)))
> + return -EFAULT;
> +
> + if (ncr.data) {
> + size = ncr.size;
> +
> + if (!access_ok(VERIFY_READ, (void __user *)ncr.data, size))
> + return -EFAULT;
> +
> + data = kzalloc(size, GFP_KERNEL | GFP_DMA);
> + if (!data)
> + return -ENOMEM;
> +
> + memcpy(data, ncr.data, size);
> + } else {
> + size = 0;
> + data = NULL;
> + }
> +
> + ret = usb_control_msg(udev,
> + usb_sndctrlpipe(udev, 0),
> + ncr.request,
> + ncr.type,
> + ncr.value,
> + ncr.index,
> + data,
> + size,
> + 0);
> +
> +
> + kfree(data);
> +
> + return ret;
> + }
> + case NRPZ_VENDOR_CONTROL_MSG_IN:
> + {
> + struct nrpz_control_req ncr;
> + u8 *data;
> + u16 size;
> +
> + if (copy_from_user(&ncr, (struct nrpz_control_req __user *)arg, sizeof(ncr)))
> + return -EFAULT;
> +
> + if (ncr.data) {
> + size = ncr.size;
> +
> + if (!access_ok(VERIFY_WRITE, (void __user *)ncr.data, size))
> + return -EFAULT;
> +
> + data = kzalloc(size, GFP_KERNEL | GFP_DMA);
> + if (!data)
> + return -ENOMEM;
> + } else {
> + size = 0;
> + data = NULL;
> + }
> +
> + ret = usb_control_msg(udev,
> + usb_rcvctrlpipe(udev, 0),
> + ncr.request,
> + ncr.type,
> + ncr.value,
> + ncr.index,
> + data,
> + size,
> + 0);
> +
> + if (data) {
> + memcpy(ncr.data, data, size);
> + kfree(data);
> + }
> +
> + return ret;
> + }
> + default:
> + dev_dbg(&intf->dev,
> + "Invalid ioctl call (%08x)", cmd);
> + return -ENOTTY;
> + }
> +
> + return ret;
> +}

> +static int nrpz_release(struct inode *inode, struct file *file)
> +{
> + struct usb_nrpz *dev = file->private_data;
> +
> + if (dev == NULL)
> + return -ENODEV;
> +
> + usb_kill_anchored_urbs(&dev->in_running);
> + usb_kill_anchored_urbs(&dev->out_running);

Isn't this a noop as you've implemented flush() ?

> +
> + bulks_release(dev->out_urbs, ARRAY_SIZE(dev->out_urbs), dev->out_size);
> + bulks_release(dev->in_urbs, ARRAY_SIZE(dev->in_urbs), dev->in_size);
> +
> + /* decrement the count on our device */
> + kref_put(&dev->kref, nrpz_delete);
> +
> + spin_lock_irq(&dev->read_lock);
> + dev->in_use = false;
> + spin_unlock_irq(&dev->read_lock);

Looks like a use after free. You should drop the reference as the very
last thing.

> +
> + return 0;
> +}
> +

HTH
Oliver
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


stefani at seibold

Jun 2, 2012, 10:15 PM

Post #25 of 28 (639 views)
Permalink
Re: [PATCH] add new NRP power meter USB device driver [In reply to]

Am Samstag, den 02.06.2012, 22:24 +0200 schrieb Oliver Neukum:
> Am Samstag, 2. Juni 2012, 18:18:59 schrieb stefani [at] seibold:
> > +static int bulks_in_submit(struct usb_nrpz *dev)
> > +{
> > + int ret;
> > + unsigned i;
> > +
> > + for (i = 0; i != ARRAY_SIZE(dev->in_urbs); ++i) {
> > + usb_anchor_urb(&dev->in_urbs[i], &dev->in_running);
> > +
> > + ret = usb_submit_urb(&dev->in_urbs[i], GFP_KERNEL);
> > + if (ret) {
> > + usb_kill_anchored_urbs(&dev->in_running);
> > + return ret;
> > + }
> > + }
> > + return 0;
> > +}
>
> This is used in the resume code path. During resume you
> cannot swap, as the swap device may still be asleep.
> Therefore GFP_NOIO should be used. It's considered
> best practice to pass the gfp parameter to such methods.
>

Good point. Fixed.

> > +static ssize_t nrpz_read(struct file *file, char __user *buffer, size_t count,
> > + loff_t *ppos)
> > +{
> > + struct usb_nrpz *dev = file->private_data;
> > + int ret;
> > + struct urb *urb;
> > + size_t n;
> > +
> > + /* verify that we actually have some data to read */
> > + if (!count)
> > + return 0;
> > +
> > + /* lock the read data */
> > + if (file->f_flags & O_NONBLOCK) {
> > + if (!mutex_trylock(&dev->read_mutex))
> > + return -EAGAIN;
>
> Congratulations. I overlooked this. Does skeleton do it right?
>

This issue was reported by Alan Cox. Skeleton must be also fixed.

> > + } else {
> > + ret = mutex_lock_interruptible(&dev->read_mutex);
> > + if (ret)
> > + return ret;
> > + }
> > +
> > + for (;;) {
> > + urb = urb_list_get(&dev->read_lock, &dev->in_avail);
> > + if (urb)
> > + break;
> > +
> > + /* verify that the device wasn't unplugged */
> > + if (!dev->connected) {
> > + ret = -ENODEV;
> > + goto exit;
> > + }
> > +
> > + if (file->f_flags & O_NONBLOCK) {
> > + ret = -EAGAIN;
> > + goto exit;
> > + }
> > +
> > + ret = wait_event_interruptible(dev->wq,
> > + !list_empty(&dev->in_avail) || !dev->connected);
> > + if (ret) {
> > + ret = -ERESTARTSYS;
> > + goto exit;
> > + }
> > + }
> > +
> > + if (!urb->status) {
> > + n = min(count, urb->actual_length);
> > +
> > + if (copy_to_user(buffer, urb->transfer_buffer, n)) {
> > + urb_list_add(&dev->read_lock, urb, &dev->in_avail);
> > + ret = -EFAULT;
> > + goto exit;
> > + }
> > + } else {
> > + n = -EPIPE;
> > + }
> > +
> > + usb_anchor_urb(urb, &dev->in_running);
> > +
> > + ret = usb_submit_urb(urb, GFP_KERNEL);
> > + if (ret) {
> > + usb_unanchor_urb(urb);
> > + urb_list_add_tail(&dev->read_lock, urb, &dev->in_avail);
> > + urb->status = ret;
>
> That is a bug. You will report that error the next time the URB comes up.
> That's wrong. I think you need a special error code that will cause you
> to just resubmit the URB and go for the next URB.
>

I think it is okay to push it back to the tail of the list and the next
time i will get this URB i will report this error to the userspace. This
keeps the order of the errors. But i will do more investigation on this.

> > + case NRPZ_GETSENSORINFO:
> > + {

Kick away in the next release :-(

> > +static int nrpz_release(struct inode *inode, struct file *file)
> > +{
> > + struct usb_nrpz *dev = file->private_data;
> > +
> > + if (dev == NULL)
> > + return -ENODEV;
> > +
> > + usb_kill_anchored_urbs(&dev->in_running);
> > + usb_kill_anchored_urbs(&dev->out_running);
>
> Isn't this a noop as you've implemented flush() ?
>

No, it is not a noop. flush() can be interrupted and will only handle
the out_running urbs.

> > +
> > + bulks_release(dev->out_urbs, ARRAY_SIZE(dev->out_urbs), dev->out_size);
> > + bulks_release(dev->in_urbs, ARRAY_SIZE(dev->in_urbs), dev->in_size);
> > +
> > + /* decrement the count on our device */
> > + kref_put(&dev->kref, nrpz_delete);
> > +
> > + spin_lock_irq(&dev->read_lock);
> > + dev->in_use = false;
> > + spin_unlock_irq(&dev->read_lock);
>
> Looks like a use after free. You should drop the reference as the very
> last thing.
>

Thanks. One of this nasty little tiny race conditions...

Greetings,
Stefani


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

First page Previous page 1 2 Next page Last page  View All Linux kernel RSS feed   Index | Next | Previous | View Threaded
 
 


Interested in having your list archived? Contact Gossamer Threads
 
  Web Applications & Managed Hosting Powered by Gossamer Threads Inc.