Gitweb:     
http://git.kernel.org/git/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=5caf51342c81c7cb2a8c3998e3f606ccfa79cee2
Commit:     5caf51342c81c7cb2a8c3998e3f606ccfa79cee2
Parent:     ea75baf4b0f117564bd50827a49c4b14d61d24e9
Author:     Tobias Lorenz <[EMAIL PROTECTED]>
AuthorDate: Mon Feb 4 22:26:08 2008 -0300
Committer:  Mauro Carvalho Chehab <[EMAIL PROTECTED]>
CommitDate: Mon Feb 18 11:15:18 2008 -0300

    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: Tobias Lorenz <[EMAIL PROTECTED]>
    Signed-off-by: Mauro Carvalho Chehab <[EMAIL PROTECTED]>
---
 drivers/media/radio/radio-si470x.c |  330 ++++++++++++++++++------------------
 1 files changed, 167 insertions(+), 163 deletions(-)

diff --git a/drivers/media/radio/radio-si470x.c 
b/drivers/media/radio/radio-si470x.c
index d428c3a..c72a9e7 100644
--- a/drivers/media/radio/radio-si470x.c
+++ b/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 <[EMAIL PROTECTED]>
+ *             Oliver Neukum <[EMAIL PROTECTED]>
+ *             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 <[EMAIL PROTECTED]>"
 #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)
@@ -846,29 +855,18 @@ 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 [EMAIL PROTECTED]",
-                       radio->registers[CHIPID] & CHIPID_FIRMWARE,
-                       RADIO_SW_VERSION_CURRENT);
+                       ": please report to V4L ML at "
+                       "[EMAIL PROTECTED]");
+       }
 
        /* 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);
 }
 
 
-
To unsubscribe from this list: send the line "unsubscribe git-commits-head" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to