[PATCH/RFC v2 0/8] dsbr100: driver cleanup and fixes

[PATCH/RFC v2 0/8] dsbr100: driver cleanup and fixes

[David Ellingsworth]

This patch series addresses several issues in the dsbr100 driver.
This series is based on the v4l-dvb master git branch and has been
compile tested only. It should be tested before applying.

This is the second version of this series. An additional patch has
been added to cleanup/clarify the return values from dsbr100_start
and dsbr100_stop.

The following patches are included in this series:
   [PATCH/RFC v2 1/8] dsbr100: implement proper locking
   [PATCH/RFC v2 2/8] dsbr100: fix potential use after free
   [PATCH/RFC v2 3/8] dsbr100: only change frequency upon success
   [PATCH/RFC v2 4/8] dsbr100: remove disconnected indicator
   [PATCH/RFC v2 5/8] dsbr100: cleanup return value of start/stop handlers
   [PATCH/RFC v2 6/8] dsbr100: properly initialize the radio
   [PATCH/RFC v2 7/8] dsbr100: cleanup usb probe routine
   [PATCH/RFC v2 8/8] dsbr100: simplify access to radio device

Regards,

David Ellingsworth

[PATCH/RFC v2 1/8] dsbr100: implement proper locking

[David Ellingsworth]

Signed-off-by: David Ellingsworth <david@identd.dyndns.org>
---
 drivers/media/radio/dsbr100.c |   77 +++++++++++++++++-----------------------
 1 files changed, 33 insertions(+), 44 deletions(-)

diff --git a/drivers/media/radio/dsbr100.c b/drivers/media/radio/dsbr100.c
index ed9cd7a..673eda8 100644
--- a/drivers/media/radio/dsbr100.c
+++ b/drivers/media/radio/dsbr100.c
@@ -182,7 +182,7 @@ static int dsbr100_start(struct dsbr100_device *radio)
 	int retval;
 	int request;
 
-	mutex_lock(&radio->lock);
+	BUG_ON(!mutex_is_locked(&radio->lock));
 
 	retval = usb_control_msg(radio->usbdev,
 		usb_rcvctrlpipe(radio->usbdev, 0),
@@ -207,11 +207,9 @@ static int dsbr100_start(struct dsbr100_device *radio)
 	}
 
 	radio->status = STARTED;
-	mutex_unlock(&radio->lock);
 	return (radio->transfer_buffer)[0];
 
 usb_control_msg_failed:
-	mutex_unlock(&radio->lock);
 	dev_err(&radio->usbdev->dev,
 		"%s - usb_control_msg returned %i, request %i\n",
 			__func__, retval, request);
@@ -225,7 +223,7 @@ static int dsbr100_stop(struct dsbr100_device *radio)
 	int retval;
 	int request;
 
-	mutex_lock(&radio->lock);
+	BUG_ON(!mutex_is_locked(&radio->lock));
 
 	retval = usb_control_msg(radio->usbdev,
 		usb_rcvctrlpipe(radio->usbdev, 0),
@@ -250,11 +248,9 @@ static int dsbr100_stop(struct dsbr100_device *radio)
 	}
 
 	radio->status = STOPPED;
-	mutex_unlock(&radio->lock);
 	return (radio->transfer_buffer)[0];
 
 usb_control_msg_failed:
-	mutex_unlock(&radio->lock);
 	dev_err(&radio->usbdev->dev,
 		"%s - usb_control_msg returned %i, request %i\n",
 			__func__, retval, request);
@@ -269,7 +265,7 @@ static int dsbr100_setfreq(struct dsbr100_device *radio)
 	int request;
 	int freq = (radio->curfreq / 16 * 80) / 1000 + 856;
 
-	mutex_lock(&radio->lock);
+	BUG_ON(!mutex_is_locked(&radio->lock));
 
 	retval = usb_control_msg(radio->usbdev,
 		usb_rcvctrlpipe(radio->usbdev, 0),
@@ -306,12 +302,10 @@ static int dsbr100_setfreq(struct dsbr100_device *radio)
 	}
 
 	radio->stereo = !((radio->transfer_buffer)[0] & 0x01);
-	mutex_unlock(&radio->lock);
 	return (radio->transfer_buffer)[0];
 
 usb_control_msg_failed:
 	radio->stereo = -1;
-	mutex_unlock(&radio->lock);
 	dev_err(&radio->usbdev->dev,
 		"%s - usb_control_msg returned %i, request %i\n",
 			__func__, retval, request);
@@ -324,7 +318,7 @@ static void dsbr100_getstat(struct dsbr100_device *radio)
 {
 	int retval;
 
-	mutex_lock(&radio->lock);
+	BUG_ON(!mutex_is_locked(&radio->lock));
 
 	retval = usb_control_msg(radio->usbdev,
 		usb_rcvctrlpipe(radio->usbdev, 0),
@@ -340,8 +334,6 @@ static void dsbr100_getstat(struct dsbr100_device *radio)
 	} else {
 		radio->stereo = !(radio->transfer_buffer[0] & 0x01);
 	}
-
-	mutex_unlock(&radio->lock);
 }
 
 /* USB subsystem interface begins here */
@@ -385,10 +377,6 @@ static int vidioc_g_tuner(struct file *file, void *priv,
 {
 	struct dsbr100_device *radio = video_drvdata(file);
 
-	/* safety check */
-	if (radio->removed)
-		return -EIO;
-
 	if (v->index > 0)
 		return -EINVAL;
 
@@ -410,12 +398,6 @@ static int vidioc_g_tuner(struct file *file, void *priv,
 static int vidioc_s_tuner(struct file *file, void *priv,
 				struct v4l2_tuner *v)
 {
-	struct dsbr100_device *radio = video_drvdata(file);
-
-	/* safety check */
-	if (radio->removed)
-		return -EIO;
-
 	if (v->index > 0)
 		return -EINVAL;
 
@@ -428,17 +410,12 @@ static int vidioc_s_frequency(struct file *file, void *priv,
 	struct dsbr100_device *radio = video_drvdata(file);
 	int retval;
 
-	/* safety check */
-	if (radio->removed)
-		return -EIO;
-
-	mutex_lock(&radio->lock);
 	radio->curfreq = f->frequency;
-	mutex_unlock(&radio->lock);
 
 	retval = dsbr100_setfreq(radio);
 	if (retval < 0)
 		dev_warn(&radio->usbdev->dev, "Set frequency failed\n");
+
 	return 0;
 }
 
@@ -447,10 +424,6 @@ static int vidioc_g_frequency(struct file *file, void *priv,
 {
 	struct dsbr100_device *radio = video_drvdata(file);
 
-	/* safety check */
-	if (radio->removed)
-		return -EIO;
-
 	f->type = V4L2_TUNER_RADIO;
 	f->frequency = radio->curfreq;
 	return 0;
@@ -472,10 +445,6 @@ static int vidioc_g_ctrl(struct file *file, void *priv,
 {
 	struct dsbr100_device *radio = video_drvdata(file);
 
-	/* safety check */
-	if (radio->removed)
-		return -EIO;
-
 	switch (ctrl->id) {
 	case V4L2_CID_AUDIO_MUTE:
 		ctrl->value = radio->status;
@@ -490,10 +459,6 @@ static int vidioc_s_ctrl(struct file *file, void *priv,
 	struct dsbr100_device *radio = video_drvdata(file);
 	int retval;
 
-	/* safety check */
-	if (radio->removed)
-		return -EIO;
-
 	switch (ctrl->id) {
 	case V4L2_CID_AUDIO_MUTE:
 		if (ctrl->value) {
@@ -513,6 +478,7 @@ static int vidioc_s_ctrl(struct file *file, void *priv,
 		}
 		return 0;
 	}
+
 	return -EINVAL;
 }
 
@@ -548,12 +514,34 @@ static int vidioc_s_audio(struct file *file, void *priv,
 	return 0;
 }
 
+static long usb_dsbr100_ioctl(struct file *file, unsigned int cmd,
+				unsigned long arg)
+{
+	struct dsbr100_device *radio = video_drvdata(file);
+	long retval = 0;
+
+	mutex_lock(&radio->lock);
+
+	if (radio->removed) {
+		retval = -EIO;
+		goto unlock;
+	}
+
+	retval = video_ioctl2(file, cmd, arg);
+
+unlock:
+	mutex_unlock(&radio->lock);
+	return retval;
+}
+
 /* Suspend device - stop device. */
 static int usb_dsbr100_suspend(struct usb_interface *intf, pm_message_t message)
 {
 	struct dsbr100_device *radio = usb_get_intfdata(intf);
 	int retval;
 
+	mutex_lock(&radio->lock);
+
 	if (radio->status == STARTED) {
 		retval = dsbr100_stop(radio);
 		if (retval < 0)
@@ -564,12 +552,10 @@ static int usb_dsbr100_suspend(struct usb_interface *intf, pm_message_t message)
 		 * we set status equal to STARTED.
 		 * On resume we will check status and run radio if needed.
 		 */
-
-		mutex_lock(&radio->lock);
 		radio->status = STARTED;
-		mutex_unlock(&radio->lock);
 	}
 
+	mutex_unlock(&radio->lock);
 	dev_info(&intf->dev, "going into suspend..\n");
 
 	return 0;
@@ -581,12 +567,15 @@ static int usb_dsbr100_resume(struct usb_interface *intf)
 	struct dsbr100_device *radio = usb_get_intfdata(intf);
 	int retval;
 
+	mutex_lock(&radio->lock);
+
 	if (radio->status == STARTED) {
 		retval = dsbr100_start(radio);
 		if (retval < 0)
 			dev_warn(&intf->dev, "dsbr100_start failed\n");
 	}
 
+	mutex_unlock(&radio->lock);
 	dev_info(&intf->dev, "coming out of suspend..\n");
 
 	return 0;
@@ -605,7 +594,7 @@ static void usb_dsbr100_video_device_release(struct video_device *videodev)
 /* File system interface */
 static const struct v4l2_file_operations usb_dsbr100_fops = {
 	.owner		= THIS_MODULE,
-	.ioctl		= video_ioctl2,
+	.unlocked_ioctl	= usb_dsbr100_ioctl,
 };
 
 static const struct v4l2_ioctl_ops usb_dsbr100_ioctl_ops = {
-- 
1.7.1

[PATCH/RFC v2 2/8] dsbr100: fix potential use after free

[David Ellingsworth]

Signed-off-by: David Ellingsworth <david@identd.dyndns.org>
---
 drivers/media/radio/dsbr100.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/media/radio/dsbr100.c b/drivers/media/radio/dsbr100.c
index 673eda8..2f96e13 100644
--- a/drivers/media/radio/dsbr100.c
+++ b/drivers/media/radio/dsbr100.c
@@ -354,8 +354,8 @@ static void usb_dsbr100_disconnect(struct usb_interface *intf)
 	radio->removed = 1;
 	mutex_unlock(&radio->lock);
 
-	video_unregister_device(&radio->videodev);
 	v4l2_device_disconnect(&radio->v4l2_dev);
+	video_unregister_device(&radio->videodev);
 }
 
 
-- 
1.7.1

[PATCH/RFC v2 3/8] dsbr100: only change frequency upon success

[David Ellingsworth]

Signed-off-by: David Ellingsworth <david@identd.dyndns.org>
---
 drivers/media/radio/dsbr100.c |   11 +++++------
 1 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/media/radio/dsbr100.c b/drivers/media/radio/dsbr100.c
index 2f96e13..b62fe40 100644
--- a/drivers/media/radio/dsbr100.c
+++ b/drivers/media/radio/dsbr100.c
@@ -259,11 +259,12 @@ usb_control_msg_failed:
 }
 
 /* set a frequency, freq is defined by v4l's TUNER_LOW, i.e. 1/16th kHz */
-static int dsbr100_setfreq(struct dsbr100_device *radio)
+static int dsbr100_setfreq(struct dsbr100_device *radio, int freq)
 {
 	int retval;
 	int request;
-	int freq = (radio->curfreq / 16 * 80) / 1000 + 856;
+
+	freq = (freq / 16 * 80) / 1000 + 856;
 
 	BUG_ON(!mutex_is_locked(&radio->lock));
 
@@ -302,6 +303,7 @@ static int dsbr100_setfreq(struct dsbr100_device *radio)
 	}
 
 	radio->stereo = !((radio->transfer_buffer)[0] & 0x01);
+	radio->curfreq = freq;
 	return (radio->transfer_buffer)[0];
 
 usb_control_msg_failed:
@@ -408,11 +410,8 @@ static int vidioc_s_frequency(struct file *file, void *priv,
 				struct v4l2_frequency *f)
 {
 	struct dsbr100_device *radio = video_drvdata(file);
-	int retval;
-
-	radio->curfreq = f->frequency;
+	int retval = dsbr100_setfreq(radio, f->frequency);
 
-	retval = dsbr100_setfreq(radio);
 	if (retval < 0)
 		dev_warn(&radio->usbdev->dev, "Set frequency failed\n");
 
-- 
1.7.1

[PATCH/RFC v2 4/8] dsbr100: remove disconnected indicator

[David Ellingsworth]

Signed-off-by: David Ellingsworth <david@identd.dyndns.org>
---
 drivers/media/radio/dsbr100.c |    6 ++----
 1 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/media/radio/dsbr100.c b/drivers/media/radio/dsbr100.c
index b62fe40..c949ace 100644
--- a/drivers/media/radio/dsbr100.c
+++ b/drivers/media/radio/dsbr100.c
@@ -151,7 +151,6 @@ struct dsbr100_device {
 	struct mutex lock;	/* buffer locking */
 	int curfreq;
 	int stereo;
-	int removed;
 	int status;
 };
 
@@ -353,7 +352,7 @@ static void usb_dsbr100_disconnect(struct usb_interface *intf)
 	usb_set_intfdata (intf, NULL);
 
 	mutex_lock(&radio->lock);
-	radio->removed = 1;
+	radio->usbdev = NULL;
 	mutex_unlock(&radio->lock);
 
 	v4l2_device_disconnect(&radio->v4l2_dev);
@@ -521,7 +520,7 @@ static long usb_dsbr100_ioctl(struct file *file, unsigned int cmd,
 
 	mutex_lock(&radio->lock);
 
-	if (radio->removed) {
+	if (!radio->usbdev) {
 		retval = -EIO;
 		goto unlock;
 	}
@@ -649,7 +648,6 @@ static int usb_dsbr100_probe(struct usb_interface *intf,
 
 	mutex_init(&radio->lock);
 
-	radio->removed = 0;
 	radio->usbdev = interface_to_usbdev(intf);
 	radio->curfreq = FREQ_MIN * FREQ_MUL;
 	radio->status = STOPPED;
-- 
1.7.1

[PATCH/RFC v2 5/8] dsbr100: cleanup return value of start/stop handlers

[David Ellingsworth]

The value returned from the device in radio->transfer_buffer[0]
isn't clearly defined, but is used as a return code. This value
is an unsigned 8-bit integer that is implicitly converted to an
int. Since this value can never be less than 0, return 0 instead.
This simplifies the verification of calling dsbr100_start and
dsbr100_stop.

Signed-off-by: David Ellingsworth <david@identd.dyndns.org>
---
 drivers/media/radio/dsbr100.c |   12 ++++++------
 1 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/media/radio/dsbr100.c b/drivers/media/radio/dsbr100.c
index c949ace..0e009b7 100644
--- a/drivers/media/radio/dsbr100.c
+++ b/drivers/media/radio/dsbr100.c
@@ -206,7 +206,7 @@ static int dsbr100_start(struct dsbr100_device *radio)
 	}
 
 	radio->status = STARTED;
-	return (radio->transfer_buffer)[0];
+	return 0;
 
 usb_control_msg_failed:
 	dev_err(&radio->usbdev->dev,
@@ -247,7 +247,7 @@ static int dsbr100_stop(struct dsbr100_device *radio)
 	}
 
 	radio->status = STOPPED;
-	return (radio->transfer_buffer)[0];
+	return 0;
 
 usb_control_msg_failed:
 	dev_err(&radio->usbdev->dev,
@@ -461,14 +461,14 @@ static int vidioc_s_ctrl(struct file *file, void *priv,
 	case V4L2_CID_AUDIO_MUTE:
 		if (ctrl->value) {
 			retval = dsbr100_stop(radio);
-			if (retval < 0) {
+			if (retval) {
 				dev_warn(&radio->usbdev->dev,
 					 "Radio did not respond properly\n");
 				return -EBUSY;
 			}
 		} else {
 			retval = dsbr100_start(radio);
-			if (retval < 0) {
+			if (retval) {
 				dev_warn(&radio->usbdev->dev,
 					 "Radio did not respond properly\n");
 				return -EBUSY;
@@ -542,7 +542,7 @@ static int usb_dsbr100_suspend(struct usb_interface *intf, pm_message_t message)
 
 	if (radio->status == STARTED) {
 		retval = dsbr100_stop(radio);
-		if (retval < 0)
+		if (retval)
 			dev_warn(&intf->dev, "dsbr100_stop failed\n");
 
 		/* After dsbr100_stop() status set to STOPPED.
@@ -569,7 +569,7 @@ static int usb_dsbr100_resume(struct usb_interface *intf)
 
 	if (radio->status == STARTED) {
 		retval = dsbr100_start(radio);
-		if (retval < 0)
+		if (retval)
 			dev_warn(&intf->dev, "dsbr100_start failed\n");
 	}
 
-- 
1.7.1

[PATCH/RFC v2 6/8] dsbr100: properly initialize the radio

[David Ellingsworth]

This patch adds a flag to indicate if the radio has been
initialized or not. If the flag has not been set upon open,
the radio is initialized to a known state.

It combines the STARTED/STOPPED indicators into a single
MUTED flag adn defines a couple of macros for determining
the status of the radio.

Signed-off-by: David Ellingsworth <david@identd.dyndns.org>
---
 drivers/media/radio/dsbr100.c |   53 +++++++++++++++++++++++++++++++---------
 1 files changed, 41 insertions(+), 12 deletions(-)

diff --git a/drivers/media/radio/dsbr100.c b/drivers/media/radio/dsbr100.c
index 0e009b7..96e6128 100644
--- a/drivers/media/radio/dsbr100.c
+++ b/drivers/media/radio/dsbr100.c
@@ -125,10 +125,13 @@ devices, that would be 76 and 91.  */
 #define FREQ_MAX 108.0
 #define FREQ_MUL 16000
 
-/* defines for radio->status */
-#define STARTED	0
-#define STOPPED	1
+enum dsbr100_status {
+	INITIALIZED	= 1 << 0,
+	MUTED		= 1 << 1
+};
 
+#define radio_initialized(r) (r->status & INITIALIZED)
+#define radio_muted(r) (r->status & MUTED)
 #define videodev_to_radio(d) container_of(d, struct dsbr100_device, videodev)
 
 static int usb_dsbr100_probe(struct usb_interface *intf,
@@ -151,7 +154,7 @@ struct dsbr100_device {
 	struct mutex lock;	/* buffer locking */
 	int curfreq;
 	int stereo;
-	int status;
+	enum dsbr100_status status;
 };
 
 static struct usb_device_id usb_dsbr100_device_table [] = {
@@ -205,7 +208,7 @@ static int dsbr100_start(struct dsbr100_device *radio)
 		goto usb_control_msg_failed;
 	}
 
-	radio->status = STARTED;
+	radio->status &= ~MUTED;
 	return 0;
 
 usb_control_msg_failed:
@@ -246,7 +249,7 @@ static int dsbr100_stop(struct dsbr100_device *radio)
 		goto usb_control_msg_failed;
 	}
 
-	radio->status = STOPPED;
+	radio->status |= MUTED;
 	return 0;
 
 usb_control_msg_failed:
@@ -532,6 +535,32 @@ unlock:
 	return retval;
 }
 
+static int usb_dsbr100_open(struct file *file)
+{
+	struct dsbr100_device *radio = video_drvdata(file);
+	int retval = 0;
+
+	mutex_lock(&radio->lock);
+
+	if (!radio->usbdev) {
+		retval = -EIO;
+		goto unlock;
+	}
+
+	if (unlikely(!radio_initialized(radio))) {
+		retval = dsbr100_stop(radio);
+
+		if (retval)
+			goto unlock;
+
+		radio->status |= INITIALIZED;
+	}
+
+unlock:
+	mutex_unlock(&radio->lock);
+	return retval;
+}
+
 /* Suspend device - stop device. */
 static int usb_dsbr100_suspend(struct usb_interface *intf, pm_message_t message)
 {
@@ -540,17 +569,17 @@ static int usb_dsbr100_suspend(struct usb_interface *intf, pm_message_t message)
 
 	mutex_lock(&radio->lock);
 
-	if (radio->status == STARTED) {
+	if (!radio_muted(radio)) {
 		retval = dsbr100_stop(radio);
 		if (retval)
 			dev_warn(&intf->dev, "dsbr100_stop failed\n");
 
-		/* After dsbr100_stop() status set to STOPPED.
+		/* After dsbr100_stop() status set to MUTED.
 		 * If we want driver to start radio on resume
-		 * we set status equal to STARTED.
+		 * we set status equal to not MUTED
 		 * On resume we will check status and run radio if needed.
 		 */
-		radio->status = STARTED;
+		radio->status &= ~MUTED;
 	}
 
 	mutex_unlock(&radio->lock);
@@ -567,7 +596,7 @@ static int usb_dsbr100_resume(struct usb_interface *intf)
 
 	mutex_lock(&radio->lock);
 
-	if (radio->status == STARTED) {
+	if (radio_initialized(radio) && !radio_muted(radio)) {
 		retval = dsbr100_start(radio);
 		if (retval)
 			dev_warn(&intf->dev, "dsbr100_start failed\n");
@@ -593,6 +622,7 @@ static void usb_dsbr100_video_device_release(struct video_device *videodev)
 static const struct v4l2_file_operations usb_dsbr100_fops = {
 	.owner		= THIS_MODULE,
 	.unlocked_ioctl	= usb_dsbr100_ioctl,
+	.open		= usb_dsbr100_open,
 };
 
 static const struct v4l2_ioctl_ops usb_dsbr100_ioctl_ops = {
@@ -650,7 +680,6 @@ static int usb_dsbr100_probe(struct usb_interface *intf,
 
 	radio->usbdev = interface_to_usbdev(intf);
 	radio->curfreq = FREQ_MIN * FREQ_MUL;
-	radio->status = STOPPED;
 
 	video_set_drvdata(&radio->videodev, radio);
 
-- 
1.7.1

[PATCH/RFC v2 7/8] dsbr100: cleanup usb probe routine

[David Ellingsworth]

This patch simplifies the error paths within the
usb_dsbr100_probe routine. It also removes an unnecessary
local variable.

Signed-off-by: David Ellingsworth <david@identd.dyndns.org>
---
 drivers/media/radio/dsbr100.c |   39 ++++++++++++++++++++-------------------
 1 files changed, 20 insertions(+), 19 deletions(-)

diff --git a/drivers/media/radio/dsbr100.c b/drivers/media/radio/dsbr100.c
index 96e6128..81e6aa5 100644
--- a/drivers/media/radio/dsbr100.c
+++ b/drivers/media/radio/dsbr100.c
@@ -645,33 +645,28 @@ static int usb_dsbr100_probe(struct usb_interface *intf,
 				const struct usb_device_id *id)
 {
 	struct dsbr100_device *radio;
-	struct v4l2_device *v4l2_dev;
 	int retval;
 
 	radio = kzalloc(sizeof(struct dsbr100_device), GFP_KERNEL);
-
 	if (!radio)
 		return -ENOMEM;
 
 	radio->transfer_buffer = kmalloc(TB_LEN, GFP_KERNEL);
-
 	if (!(radio->transfer_buffer)) {
-		kfree(radio);
-		return -ENOMEM;
+		retval = -ENOMEM;
+		goto err_nobuf;
 	}
 
-	v4l2_dev = &radio->v4l2_dev;
-
-	retval = v4l2_device_register(&intf->dev, v4l2_dev);
+	retval = v4l2_device_register(&intf->dev, &radio->v4l2_dev);
 	if (retval < 0) {
-		v4l2_err(v4l2_dev, "couldn't register v4l2_device\n");
-		kfree(radio->transfer_buffer);
-		kfree(radio);
-		return retval;
+		v4l2_err(&radio->v4l2_dev, "couldn't register v4l2_device\n");
+		goto err_v4l2;
 	}
 
-	strlcpy(radio->videodev.name, v4l2_dev->name, sizeof(radio->videodev.name));
-	radio->videodev.v4l2_dev = v4l2_dev;
+	strlcpy(radio->videodev.name, radio->v4l2_dev.name,
+		sizeof(radio->videodev.name));
+
+	radio->videodev.v4l2_dev = &radio->v4l2_dev;
 	radio->videodev.fops = &usb_dsbr100_fops;
 	radio->videodev.ioctl_ops = &usb_dsbr100_ioctl_ops;
 	radio->videodev.release = usb_dsbr100_video_device_release;
@@ -685,14 +680,20 @@ static int usb_dsbr100_probe(struct usb_interface *intf,
 
 	retval = video_register_device(&radio->videodev, VFL_TYPE_RADIO, radio_nr);
 	if (retval < 0) {
-		v4l2_err(v4l2_dev, "couldn't register video device\n");
-		v4l2_device_unregister(v4l2_dev);
-		kfree(radio->transfer_buffer);
-		kfree(radio);
-		return -EIO;
+		v4l2_err(&radio->v4l2_dev, "couldn't register video device\n");
+		goto err_vdev;
 	}
+
 	usb_set_intfdata(intf, radio);
 	return 0;
+
+err_vdev:
+	v4l2_device_unregister(&radio->v4l2_dev);
+err_v4l2:
+	kfree(radio->transfer_buffer);
+err_nobuf:
+	kfree(radio);
+	return retval;
 }
 
 static int __init dsbr100_init(void)
-- 
1.7.1

[PATCH/RFC v2 8/8] dsbr100: simplify access to radio device

[David Ellingsworth]

This patch replaces calls to video_drvdata with
references to struct file->private_data which is
set during usb_dsbr100_open. This value is passed
by video_ioctl2 via the *priv argument and is
accessible via file->private_data otherwise.

Signed-off-by: David Ellingsworth <david@identd.dyndns.org>
---
 drivers/media/radio/dsbr100.c |   15 ++++++++-------
 1 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/drivers/media/radio/dsbr100.c b/drivers/media/radio/dsbr100.c
index 81e6aa5..a8c3d5a 100644
--- a/drivers/media/radio/dsbr100.c
+++ b/drivers/media/radio/dsbr100.c
@@ -366,7 +366,7 @@ static void usb_dsbr100_disconnect(struct usb_interface *intf)
 static int vidioc_querycap(struct file *file, void *priv,
 					struct v4l2_capability *v)
 {
-	struct dsbr100_device *radio = video_drvdata(file);
+	struct dsbr100_device *radio = priv;
 
 	strlcpy(v->driver, "dsbr100", sizeof(v->driver));
 	strlcpy(v->card, "D-Link R-100 USB FM Radio", sizeof(v->card));
@@ -379,7 +379,7 @@ static int vidioc_querycap(struct file *file, void *priv,
 static int vidioc_g_tuner(struct file *file, void *priv,
 				struct v4l2_tuner *v)
 {
-	struct dsbr100_device *radio = video_drvdata(file);
+	struct dsbr100_device *radio = priv;
 
 	if (v->index > 0)
 		return -EINVAL;
@@ -411,7 +411,7 @@ static int vidioc_s_tuner(struct file *file, void *priv,
 static int vidioc_s_frequency(struct file *file, void *priv,
 				struct v4l2_frequency *f)
 {
-	struct dsbr100_device *radio = video_drvdata(file);
+	struct dsbr100_device *radio = priv;
 	int retval = dsbr100_setfreq(radio, f->frequency);
 
 	if (retval < 0)
@@ -423,7 +423,7 @@ static int vidioc_s_frequency(struct file *file, void *priv,
 static int vidioc_g_frequency(struct file *file, void *priv,
 				struct v4l2_frequency *f)
 {
-	struct dsbr100_device *radio = video_drvdata(file);
+	struct dsbr100_device *radio = priv;
 
 	f->type = V4L2_TUNER_RADIO;
 	f->frequency = radio->curfreq;
@@ -444,7 +444,7 @@ static int vidioc_queryctrl(struct file *file, void *priv,
 static int vidioc_g_ctrl(struct file *file, void *priv,
 				struct v4l2_control *ctrl)
 {
-	struct dsbr100_device *radio = video_drvdata(file);
+	struct dsbr100_device *radio = priv;
 
 	switch (ctrl->id) {
 	case V4L2_CID_AUDIO_MUTE:
@@ -457,7 +457,7 @@ static int vidioc_g_ctrl(struct file *file, void *priv,
 static int vidioc_s_ctrl(struct file *file, void *priv,
 				struct v4l2_control *ctrl)
 {
-	struct dsbr100_device *radio = video_drvdata(file);
+	struct dsbr100_device *radio = priv;
 	int retval;
 
 	switch (ctrl->id) {
@@ -518,7 +518,7 @@ static int vidioc_s_audio(struct file *file, void *priv,
 static long usb_dsbr100_ioctl(struct file *file, unsigned int cmd,
 				unsigned long arg)
 {
-	struct dsbr100_device *radio = video_drvdata(file);
+	struct dsbr100_device *radio = file->private_data;
 	long retval = 0;
 
 	mutex_lock(&radio->lock);
@@ -556,6 +556,7 @@ static int usb_dsbr100_open(struct file *file)
 		radio->status |= INITIALIZED;
 	}
 
+	file->private_data = radio;
 unlock:
 	mutex_unlock(&radio->lock);
 	return retval;
-- 
1.7.1

Re: [PATCH/RFC v2 0/8] dsbr100: driver cleanup and fixes

[David Ellingsworth]

On Thu, May 27, 2010 at 12:39 PM, David Ellingsworth
<david@identd.dyndns.org> wrote:
> This patch series addresses several issues in the dsbr100 driver.
> This series is based on the v4l-dvb master git branch and has been
> compile tested only. It should be tested before applying.
>
> This is the second version of this series. An additional patch has
> been added to cleanup/clarify the return values from dsbr100_start
> and dsbr100_stop.
>
> The following patches are included in this series:
>   [PATCH/RFC v2 1/8] dsbr100: implement proper locking
>   [PATCH/RFC v2 2/8] dsbr100: fix potential use after free
>   [PATCH/RFC v2 3/8] dsbr100: only change frequency upon success
>   [PATCH/RFC v2 4/8] dsbr100: remove disconnected indicator
>   [PATCH/RFC v2 5/8] dsbr100: cleanup return value of start/stop handlers
>   [PATCH/RFC v2 6/8] dsbr100: properly initialize the radio
>   [PATCH/RFC v2 7/8] dsbr100: cleanup usb probe routine
>   [PATCH/RFC v2 8/8] dsbr100: simplify access to radio device
>

Mauro,

This series has not received any comments and the original author
seems to have abandoned this driver. Please review these patches for
approval. All changes are relatively straight forward. The second
patch in this series is a bug fix for the normal case where the device
is unplugged while closed. The current implementation will cause a
NULL pointer dereference. The fact that no one has reported this bug
is probably due to the lack of people using this driver. The rest of
the changes mainly provide general cleanups and reduced overhead.

Regards,

David Ellingsworth

Re: [PATCH/RFC v2 0/8] dsbr100: driver cleanup and fixes

[David Ellingsworth]

Alexey,

Can you review/test this patch series? Patches 2/8, 3/8, and 5/8 are
bug fixes the rest are mainly cleanups. Patch 2/8 should fix a crash
in the normal case if the device is disconnected while not in use.

Regards,

David Ellingsworth

On Thu, May 27, 2010 at 12:39 PM, David Ellingsworth
<david@identd.dyndns.org> wrote:
> This patch series addresses several issues in the dsbr100 driver.
> This series is based on the v4l-dvb master git branch and has been
> compile tested only. It should be tested before applying.
>
> This is the second version of this series. An additional patch has
> been added to cleanup/clarify the return values from dsbr100_start
> and dsbr100_stop.
>
> The following patches are included in this series:
>   [PATCH/RFC v2 1/8] dsbr100: implement proper locking
>   [PATCH/RFC v2 2/8] dsbr100: fix potential use after free
>   [PATCH/RFC v2 3/8] dsbr100: only change frequency upon success
>   [PATCH/RFC v2 4/8] dsbr100: remove disconnected indicator
>   [PATCH/RFC v2 5/8] dsbr100: cleanup return value of start/stop handlers
>   [PATCH/RFC v2 6/8] dsbr100: properly initialize the radio
>   [PATCH/RFC v2 7/8] dsbr100: cleanup usb probe routine
>   [PATCH/RFC v2 8/8] dsbr100: simplify access to radio device
>
> Regards,
>
> David Ellingsworth
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH/RFC v2 0/8] dsbr100: driver cleanup and fixes

[Douglas Schilling Landgraf]

Hi David,

On Tue, Sep 14, 2010 at 11:56 AM, David Ellingsworth
<david@identd.dyndns.org> wrote:
> Alexey,
>
> Can you review/test this patch series? Patches 2/8, 3/8, and 5/8 are
> bug fixes the rest are mainly cleanups. Patch 2/8 should fix a crash
> in the normal case if the device is disconnected while not in use.

I will also check your patches soon. I have this old hardware at home.

Cheers
Douglas

Re: [PATCH/RFC v2 0/8] dsbr100: driver cleanup and fixes

[David Ellingsworth]

> I will also check your patches soon. I have this old hardware at home.
>

The sooner the better. These patches have been waiting for review
since May. I'd rather not have to rebase them and resend them a third
time.

Regards,

David Ellingsworth

Re: [PATCH/RFC v2 0/8] dsbr100: driver cleanup and fixes

[Bjørn Mork]

David Ellingsworth <david@identd.dyndns.org> writes:

>> I will also check your patches soon. I have this old hardware at home.
>>
>
> The sooner the better. These patches have been waiting for review
> since May. I'd rather not have to rebase them and resend them a third
> time.

The current review process for drivers abandoned by the original author
is not working.

I really, really fail too see the problem with just letting a clean
compile-tested patchset like yours through after, let's say, a week
without any comments at all.  That's probably the only way it is ever
going to be tested by someone with the actual hardware.  Worst case is
that some of the patches will have to be reverted in the next release
(and stable point release).  That's not going to be problematic at all,
given that the patchset only touches a single driver in maintenance
mode.

Please Mauro, can you implement some sort of deadline for your review
cycles?  Half a year is nowhere close to acceptable for non-
controversial stuff like this. 

Spotting the non-controversial patches is easy BTW:  Just look for
those with no comments at all...


Bjørn