Message-ID: <1338454041.6454.67.camel@wall-e>
Date:	Thu, 31 May 2012 10:47:21 +0200
From:	Stefani Seibold <stefani@...bold.net>
To:	Greg KH <gregkh@...uxfoundation.org>
Cc:	linux-kernel@...r.kernel.org,
	thomas.braunstorfinger@...de-schwarz.com
Subject: Re: [PATCH] add new NRP power meter USB device driver

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@...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?
> 

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@...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?
> 

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@...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