On Wednesday 05 May 2010 19:05:24 David Ellingsworth wrote:
> Signed-off-by: David Ellingsworth <david@xxxxxxxxxxxxxxxxx>
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 = {
>
--
Hans Verkuil - video4linux developer - sponsored by TANDBERG, part of Cisco
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
