| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Wed, 30 May 2012 07:15:49 +0800
From: Greg KH <gregkh@...uxfoundation.org>
To: stefani@...bold.net
Cc: linux-kernel@...r.kernel.org,
thomas.braunstorfinger@...de-schwarz.com
Subject: Re: [PATCH] add new NRP power meter USB device driver
On Tue, May 29, 2012 at 09:14:18PM +0200, stefani@...bold.net wrote:
> From: Stefani Seibold <stefani@...bold.net>
>
> 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@...bold.net>
> ---
> 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@...bold.net>
> + *
> + * Based on USB Skeleton driver written by Greg Kroah-Hartman <greg@...ah.com>
> + *
> + * 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@...bold.net>"
> +#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@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists