[PATCH/RFC,1/7] dsbr100: implement proper locking

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

On Wednesday 05 May 2010 19:05:24 David Ellingsworth wrote:
> Signed-off-by: David Ellingsworth <david@identd.dyndns.org>

Does this actually fix bugs or does this just clean up the locking?

I ask because I am planning to make it easier to do locking for ioctl calls.
Basically there will be pre-hook and post-hook callbacks that are called before
and after an ioctl and where you can do the locking. I am waiting for a patch
series related to priority handling to go in first, then I will work on this
change.

So from my perspective I am OK with this if it fixes bugs, but if it is
just a clean up, then it might be better to wait until my new callbacks are in.

I won't block this, though. It's just FYI.

Regards,

	Hans

> ---
>  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 = {
>