V4L/DVB (7188): radio-si470x version 1.0.6

This patch combines all the finished discussions and its resulting patches from
the mailing list.

The version 1.0.6 is mainly influenced by Oliver Neukum. He found a lot of
small issues, that are fixed with this patch now. For me the most interesting
thing is, that it's now safer to use it on other architectures.

The history for version 1.0.6 is:
- fixed coverity checker warnings in *_usb_driver_disconnect
- probe()/open() race by correct ordering in probe()
- DMA coherency rules by separate allocation of all buffers
- use of endianness macros
- abuse of spinlock, replaced by mutex
- racy handling of timer in disconnect, replaced by delayed_work
- racy interruptible_sleep_on(), replaced with wait_event_interruptible()
- handle signals in read()

The driver is tested with all Debian/testing radio programs and rdsd. The patch
is tested against checkpatch.pl v1.12.
Signed-off-by: NTobias Lorenz <tobias.lorenz@gmx.net>
Signed-off-by: NMauro Carvalho Chehab <mchehab@infradead.org>

drivers/media/radio/radio-si470x.c

@@ -66,8 +66,21 @@
  *		Version 1.0.5
  *		- number of seek_retries changed to tune_timeout
  *		- fixed problem with incomplete tune operations by own buffers
- *		- optimization of variables
+ *		- optimization of variables and printf types
  *		- improved error logging
+ * 2008-01-31	Tobias Lorenz <tobias.lorenz@gmx.net>
+ *		Oliver Neukum <oliver@neukum.org>
+ *		Version 1.0.6
+ *		- fixed coverity checker warnings in *_usb_driver_disconnect
+ *		- probe()/open() race by correct ordering in probe()
+ *		- DMA coherency rules by separate allocation of all buffers
+ *		- use of endianness macros
+ *		- abuse of spinlock, replaced by mutex
+ *		- racy handling of timer in disconnect,
+ *		  replaced by delayed_work
+ *		- racy interruptible_sleep_on(),
+ *		  replaced with wait_event_interruptible()
+ *		- handle signals in read()
  *
  * ToDo:
  * - add seeking support
@@ -80,10 +93,10 @@
 /* driver definitions */
 #define DRIVER_AUTHOR "Tobias Lorenz <tobias.lorenz@gmx.net>"
 #define DRIVER_NAME "radio-si470x"
-#define DRIVER_KERNEL_VERSION KERNEL_VERSION(1, 0, 5)
+#define DRIVER_KERNEL_VERSION KERNEL_VERSION(1, 0, 6)
 #define DRIVER_CARD "Silicon Labs Si470x FM Radio Receiver"
 #define DRIVER_DESC "USB radio driver for Si470x FM Radio Receivers"
-#define DRIVER_VERSION "1.0.5"
+#define DRIVER_VERSION "1.0.6"
 
 
 /* kernel includes */
@@ -96,8 +109,10 @@
 #include <linux/hid.h>
 #include <linux/version.h>
 #include <linux/videodev2.h>
+#include <linux/mutex.h>
 #include <media/v4l2-common.h>
 #include <media/rds.h>
+#include <asm/unaligned.h>
 
 
 /* USB Device ID List */
@@ -409,10 +424,9 @@ struct si470x_device {
 	unsigned short registers[RADIO_REGISTER_NUM];
 
 	/* RDS receive buffer */
-	struct work_struct work;
+	struct delayed_work work;
 	wait_queue_head_t read_queue;
-	struct timer_list timer;
-	spinlock_t lock;		/* buffer locking */
+	struct mutex lock;		/* buffer locking */
 	unsigned char *buffer;		/* size is always multiple of three */
 	unsigned int buf_size;
 	unsigned int rd_index;
@@ -494,7 +508,8 @@ static int si470x_get_register(struct si470x_device *radio, int regnr)
 	retval = si470x_get_report(radio, (void *) &buf, sizeof(buf));
 
 	if (retval >= 0)
-		radio->registers[regnr] = (buf[1] << 8) | buf[2];
+		radio->registers[regnr] = be16_to_cpu(get_unaligned(
+			(unsigned short *) &buf[1]));
 
 	return (retval < 0) ? -EINVAL : 0;
 }
@@ -509,8 +524,8 @@ static int si470x_set_register(struct si470x_device *radio, int regnr)
 	int retval;
 
 	buf[0] = REGISTER_REPORT(regnr);
-	buf[1] = (radio->registers[regnr] & 0xff00) >> 8;
-	buf[2] = (radio->registers[regnr] & 0x00ff);
+	put_unaligned(cpu_to_be16(radio->registers[regnr]),
+		(unsigned short *) &buf[1]);
 
 	retval = si470x_set_report(radio, (void *) &buf, sizeof(buf));
 
@@ -533,9 +548,9 @@ static int si470x_get_all_registers(struct si470x_device *radio)
 
 	if (retval >= 0)
 		for (regnr = 0; regnr < RADIO_REGISTER_NUM; regnr++)
-			radio->registers[regnr] =
-			(buf[regnr * RADIO_REGISTER_SIZE + 1] << 8) |
-			 buf[regnr * RADIO_REGISTER_SIZE + 2];
+			radio->registers[regnr] = be16_to_cpu(get_unaligned(
+				(unsigned short *)
+				&buf[regnr * RADIO_REGISTER_SIZE + 1]));
 
 	return (retval < 0) ? -EINVAL : 0;
 }
@@ -558,7 +573,7 @@ static int si470x_get_rds_registers(struct si470x_device *radio)
 		(void *) &buf, sizeof(buf), &size, usb_timeout);
 	if (size != sizeof(buf))
 		printk(KERN_WARNING DRIVER_NAME ": si470x_get_rds_register: "
-		       "return size differs: %d != %uld\n", size, sizeof(buf));
+			"return size differs: %d != %zu\n", size, sizeof(buf));
 	if (retval < 0)
 		printk(KERN_WARNING DRIVER_NAME ": si470x_get_rds_registers: "
 			"usb_interrupt_msg returned %d\n", retval);
@@ -566,8 +581,8 @@ static int si470x_get_rds_registers(struct si470x_device *radio)
 	if (retval >= 0)
 		for (regnr = 0; regnr < RDS_REGISTER_NUM; regnr++)
 			radio->registers[STATUSRSSI + regnr] =
-			(buf[regnr * RADIO_REGISTER_SIZE + 1] << 8) |
-			 buf[regnr * RADIO_REGISTER_SIZE + 2];
+				be16_to_cpu(get_unaligned((unsigned short *)
+				&buf[regnr * RADIO_REGISTER_SIZE + 1]));
 
 	return (retval < 0) ? -EINVAL : 0;
 }
@@ -600,7 +615,7 @@ static int si470x_set_chan(struct si470x_device *radio, unsigned short chan)
 		(!timed_out));
 	if (timed_out)
 		printk(KERN_WARNING DRIVER_NAME
-			": seek does not finish after %d ms\n", tune_timeout);
+			": seek does not finish after %u ms\n", tune_timeout);
 
 	/* stop tuning */
 	radio->registers[CHANNEL] &= ~CHANNEL_TUNE;
@@ -763,6 +778,11 @@ static int si470x_rds_on(struct si470x_device *radio)
  */
 static void si470x_rds(struct si470x_device *radio)
 {
+	unsigned char blocknum;
+	unsigned short bler; /* rds block errors */
+	unsigned short rds;
+	unsigned char tmpbuf[3];
+
 	/* get rds blocks */
 	if (si470x_get_rds_registers(radio) < 0)
 		return;
@@ -775,69 +795,58 @@ static void si470x_rds(struct si470x_device *radio)
 		return;
 	}
 
-	/* copy four RDS blocks to internal buffer */
-	if (spin_trylock(&radio->lock)) {
-		unsigned char blocknum;
-		unsigned short bler; /* rds block errors */
-		unsigned short rds;
-		unsigned char tmpbuf[3];
-		unsigned char i;
-
-		/* process each rds block */
-		for (blocknum = 0; blocknum < 4; blocknum++) {
-			switch (blocknum) {
-			default:
-				bler = (radio->registers[STATUSRSSI] &
-						STATUSRSSI_BLERA) >> 9;
-				rds = radio->registers[RDSA];
-				break;
-			case 1:
-				bler = (radio->registers[READCHAN] &
-						READCHAN_BLERB) >> 14;
-				rds = radio->registers[RDSB];
-				break;
-			case 2:
-				bler = (radio->registers[READCHAN] &
-						READCHAN_BLERC) >> 12;
-				rds = radio->registers[RDSC];
-				break;
-			case 3:
-				bler = (radio->registers[READCHAN] &
-						READCHAN_BLERD) >> 10;
-				rds = radio->registers[RDSD];
-				break;
-			};
-
-			/* Fill the V4L2 RDS buffer */
-			tmpbuf[0] = rds & 0x00ff;	/* LSB */
-			tmpbuf[1] = (rds & 0xff00) >> 8;/* MSB */
-			tmpbuf[2] = blocknum;		/* offset name */
-			tmpbuf[2] |= blocknum << 3;	/* received offset */
-			if (bler > max_rds_errors)
-				tmpbuf[2] |= 0x80; /* uncorrectable errors */
-			else if (bler > 0)
-				tmpbuf[2] |= 0x40; /* corrected error(s) */
-
-			/* copy RDS block to internal buffer */
-			for (i = 0; i < 3; i++) {
-				radio->buffer[radio->wr_index] = tmpbuf[i];
-				radio->wr_index++;
-			}
-
-			/* wrap write pointer */
-			if (radio->wr_index >= radio->buf_size)
-				radio->wr_index = 0;
-
-			/* check for overflow */
-			if (radio->wr_index == radio->rd_index) {
-				/* increment and wrap read pointer */
-				radio->rd_index += 3;
-				if (radio->rd_index >= radio->buf_size)
-					radio->rd_index = 0;
-			}
+	/* copy all four RDS blocks to internal buffer */
+	mutex_lock(&radio->lock);
+	for (blocknum = 0; blocknum < 4; blocknum++) {
+		switch (blocknum) {
+		default:
+			bler = (radio->registers[STATUSRSSI] &
+					STATUSRSSI_BLERA) >> 9;
+			rds = radio->registers[RDSA];
+			break;
+		case 1:
+			bler = (radio->registers[READCHAN] &
+					READCHAN_BLERB) >> 14;
+			rds = radio->registers[RDSB];
+			break;
+		case 2:
+			bler = (radio->registers[READCHAN] &
+					READCHAN_BLERC) >> 12;
+			rds = radio->registers[RDSC];
+			break;
+		case 3:
+			bler = (radio->registers[READCHAN] &
+					READCHAN_BLERD) >> 10;
+			rds = radio->registers[RDSD];
+			break;
+		};
+
+		/* Fill the V4L2 RDS buffer */
+		put_unaligned(cpu_to_le16(rds), (unsigned short *) &tmpbuf);
+		tmpbuf[2] = blocknum;		/* offset name */
+		tmpbuf[2] |= blocknum << 3;	/* received offset */
+		if (bler > max_rds_errors)
+			tmpbuf[2] |= 0x80; /* uncorrectable errors */
+		else if (bler > 0)
+			tmpbuf[2] |= 0x40; /* corrected error(s) */
+
+		/* copy RDS block to internal buffer */
+		memcpy(&radio->buffer[radio->wr_index], &tmpbuf, 3);
+		radio->wr_index += 3;
+
+		/* wrap write pointer */
+		if (radio->wr_index >= radio->buf_size)
+			radio->wr_index = 0;
+
+		/* check for overflow */
+		if (radio->wr_index == radio->rd_index) {
+			/* increment and wrap read pointer */
+			radio->rd_index += 3;
+			if (radio->rd_index >= radio->buf_size)
+				radio->rd_index = 0;
 		}
-		spin_unlock(&radio->lock);
 	}
+	mutex_unlock(&radio->lock);
 
 	/* wake up read queue */
 	if (radio->wr_index != radio->rd_index)
@@ -845,30 +854,19 @@ static void si470x_rds(struct si470x_device *radio)
 }
 
 
-/*
- * si470x_timer - rds timer function
- */
-static void si470x_timer(unsigned long data)
-{
-	struct si470x_device *radio = (struct si470x_device *) data;
-
-	schedule_work(&radio->work);
-}
-
-
 /*
  * si470x_work - rds work function
  */
 static void si470x_work(struct work_struct *work)
 {
 	struct si470x_device *radio = container_of(work, struct si470x_device,
-		work);
+		work.work);
 
 	if ((radio->registers[SYSCONFIG1] & SYSCONFIG1_RDS) == 0)
 		return;
 
 	si470x_rds(radio);
-	mod_timer(&radio->timer, jiffies + msecs_to_jiffies(rds_poll_time));
+	schedule_delayed_work(&radio->work, msecs_to_jiffies(rds_poll_time));
 }
 
 
@@ -885,49 +883,49 @@ static ssize_t si470x_fops_read(struct file *file, char __user *buf,
 {
 	struct si470x_device *radio = video_get_drvdata(video_devdata(file));
 	int retval = 0;
+	unsigned int block_count = 0;
 
 	/* switch on rds reception */
 	if ((radio->registers[SYSCONFIG1] & SYSCONFIG1_RDS) == 0) {
 		si470x_rds_on(radio);
-		schedule_work(&radio->work);
+		schedule_delayed_work(&radio->work,
+			msecs_to_jiffies(rds_poll_time));
 	}
 
 	/* block if no new data available */
 	while (radio->wr_index == radio->rd_index) {
 		if (file->f_flags & O_NONBLOCK)
 			return -EWOULDBLOCK;
-		interruptible_sleep_on(&radio->read_queue);
+		if (wait_event_interruptible(radio->read_queue,
+			radio->wr_index != radio->rd_index) < 0)
+			return -EINTR;
 	}
 
 	/* calculate block count from byte count */
 	count /= 3;
 
 	/* copy RDS block out of internal buffer and to user buffer */
-	if (spin_trylock(&radio->lock)) {
-		unsigned int block_count = 0;
-		while (block_count < count) {
-			if (radio->rd_index == radio->wr_index)
-				break;
-
-			/* always transfer rds complete blocks */
-			if (copy_to_user(buf,
-					&radio->buffer[radio->rd_index], 3))
-				/* retval = -EFAULT; */
-				break;
+	mutex_lock(&radio->lock);
+	while (block_count < count) {
+		if (radio->rd_index == radio->wr_index)
+			break;
 
-			/* increment and wrap read pointer */
-			radio->rd_index += 3;
-			if (radio->rd_index >= radio->buf_size)
-				radio->rd_index = 0;
+		/* always transfer rds complete blocks */
+		if (copy_to_user(buf, &radio->buffer[radio->rd_index], 3))
+			/* retval = -EFAULT; */
+			break;
 
-			/* increment counters */
-			block_count++;
-			buf += 3;
-			retval += 3;
-		}
+		/* increment and wrap read pointer */
+		radio->rd_index += 3;
+		if (radio->rd_index >= radio->buf_size)
+			radio->rd_index = 0;
 
-		spin_unlock(&radio->lock);
+		/* increment counters */
+		block_count++;
+		buf += 3;
+		retval += 3;
 	}
+	mutex_unlock(&radio->lock);
 
 	return retval;
 }
@@ -944,7 +942,8 @@ static unsigned int si470x_fops_poll(struct file *file,
 	/* switch on rds reception */
 	if ((radio->registers[SYSCONFIG1] & SYSCONFIG1_RDS) == 0) {
 		si470x_rds_on(radio);
-		schedule_work(&radio->work);
+		schedule_delayed_work(&radio->work,
+			msecs_to_jiffies(rds_poll_time));
 	}
 
 	poll_wait(file, &radio->read_queue, pts);
@@ -984,8 +983,7 @@ static int si470x_fops_release(struct inode *inode, struct file *file)
 	radio->users--;
 	if (radio->users == 0) {
 		/* stop rds reception */
-		del_timer_sync(&radio->timer);
-		flush_scheduled_work();
+		cancel_delayed_work_sync(&radio->work);
 
 		/* cancel read processes */
 		wake_up_interruptible(&radio->read_queue);
@@ -1362,73 +1360,82 @@ static int si470x_usb_driver_probe(struct usb_interface *intf,
 		const struct usb_device_id *id)
 {
 	struct si470x_device *radio;
+	int retval = -ENOMEM;
 
-	/* memory and interface allocations */
-	radio = kmalloc(sizeof(struct si470x_device), GFP_KERNEL);
+	/* private data allocation */
+	radio = kzalloc(sizeof(struct si470x_device), GFP_KERNEL);
 	if (!radio)
-		return -ENOMEM;
+		goto err_initial;
+
+	/* video device allocation */
 	radio->videodev = video_device_alloc();
-	if (!radio->videodev) {
-		kfree(radio);
-		return -ENOMEM;
-	}
+	if (!radio->videodev)
+		goto err_radio;
+
+	/* initial configuration */
 	memcpy(radio->videodev, &si470x_viddev_template,
 			sizeof(si470x_viddev_template));
 	radio->users = 0;
 	radio->usbdev = interface_to_usbdev(intf);
+	mutex_init(&radio->lock);
 	video_set_drvdata(radio->videodev, radio);
-	if (video_register_device(radio->videodev, VFL_TYPE_RADIO, radio_nr)) {
-		printk(KERN_WARNING DRIVER_NAME
-				": Could not register video device\n");
-		video_device_release(radio->videodev);
-		kfree(radio);
-		return -EIO;
-	}
-	usb_set_intfdata(intf, radio);
 
 	/* show some infos about the specific device */
-	if (si470x_get_all_registers(radio) < 0) {
-		video_device_release(radio->videodev);
-		kfree(radio);
-		return -EIO;
-	}
-	printk(KERN_INFO DRIVER_NAME ": DeviceID=0x%4.4x ChipID=0x%4.4x\n",
+	retval = -EIO;
+	if (si470x_get_all_registers(radio) < 0)
+		goto err_all;
+	printk(KERN_INFO DRIVER_NAME ": DeviceID=0x%4.4hx ChipID=0x%4.4hx\n",
 			radio->registers[DEVICEID], radio->registers[CHIPID]);
 
 	/* check if firmware is current */
 	if ((radio->registers[CHIPID] & CHIPID_FIRMWARE)
-			< RADIO_SW_VERSION_CURRENT)
+			< RADIO_SW_VERSION_CURRENT) {
+		printk(KERN_WARNING DRIVER_NAME
+			": This driver is known to work with "
+			"firmware version %hu,\n", RADIO_SW_VERSION_CURRENT);
+		printk(KERN_WARNING DRIVER_NAME
+			": but the device has firmware version %hu.\n",
+			radio->registers[CHIPID] & CHIPID_FIRMWARE);
+		printk(KERN_WARNING DRIVER_NAME
+			": If you have some trouble using this driver,\n");
 		printk(KERN_WARNING DRIVER_NAME
-			": This driver is known to work with chip version %d, "
-			"but the device has firmware %d.\n"
-			DRIVER_NAME
-			"If you have some trouble using this driver, please "
-			"report to V4L ML at video4linux-list@redhat.com\n",
-			radio->registers[CHIPID] & CHIPID_FIRMWARE,
-			RADIO_SW_VERSION_CURRENT);
+			": please report to V4L ML at "
+			"video4linux-list@redhat.com\n");
+	}
 
 	/* set initial frequency */
 	si470x_set_freq(radio, 87.5 * FREQ_MUL); /* available in all regions */
 
-	/* rds initialization */
+	/* rds buffer allocation */
 	radio->buf_size = rds_buf * 3;
 	radio->buffer = kmalloc(radio->buf_size, GFP_KERNEL);
-	if (!radio->buffer) {
-		video_device_release(radio->videodev);
-		kfree(radio);
-		return -ENOMEM;
-	}
+	if (!radio->buffer)
+		goto err_all;
+
+	/* rds buffer configuration */
 	radio->wr_index = 0;
 	radio->rd_index = 0;
 	init_waitqueue_head(&radio->read_queue);
 
-	/* prepare polling via eventd */
-	INIT_WORK(&radio->work, si470x_work);
-	init_timer(&radio->timer);
-	radio->timer.function = si470x_timer;
-	radio->timer.data = (unsigned long) radio;
+	/* prepare rds work function */
+	INIT_DELAYED_WORK(&radio->work, si470x_work);
+
+	/* register video device */
+	if (video_register_device(radio->videodev, VFL_TYPE_RADIO, radio_nr)) {
+		printk(KERN_WARNING DRIVER_NAME
+				": Could not register video device\n");
+		goto err_all;
+	}
+	usb_set_intfdata(intf, radio);
 
 	return 0;
+err_all:
+	video_device_release(radio->videodev);
+	kfree(radio->buffer);
+err_radio:
+	kfree(radio);
+err_initial:
+	return retval;
 }
 
 
@@ -1439,14 +1446,11 @@ static void si470x_usb_driver_disconnect(struct usb_interface *intf)
 {
 	struct si470x_device *radio = usb_get_intfdata(intf);
 
+	cancel_delayed_work_sync(&radio->work);
 	usb_set_intfdata(intf, NULL);
-	if (radio) {
-	       del_timer_sync(&radio->timer);
-	       flush_scheduled_work();
-		video_unregister_device(radio->videodev);
-		kfree(radio->buffer);
-		kfree(radio);
-	}
+	video_unregister_device(radio->videodev);
+	kfree(radio->buffer);
+	kfree(radio);
 }