gspca: fix vidioc_s_jpegcomp locking

gspca: fix vidioc_s_jpegcomp locking

[Jim Paris]

This locking looked wrong.

-jim

--

gspca: fix vidioc_s_jpegcomp locking

Signed-off-by: Jim Paris <jim@jtan.com>

diff -r b50857fea6df linux/drivers/media/video/gspca/gspca.c
--- a/linux/drivers/media/video/gspca/gspca.c	Tue Dec 09 16:20:31 2008 -0500
+++ b/linux/drivers/media/video/gspca/gspca.c	Tue Dec 09 16:55:39 2008 -0500
@@ -1320,10 +1320,10 @@
 	struct gspca_dev *gspca_dev = priv;
 	int ret;
 
+	if (!gspca_dev->sd_desc->set_jcomp)
+		return -EINVAL;
 	if (mutex_lock_interruptible(&gspca_dev->usb_lock))
 		return -ERESTARTSYS;
-	if (!gspca_dev->sd_desc->set_jcomp)
-		return -EINVAL;
 	ret = gspca_dev->sd_desc->set_jcomp(gspca_dev, jpegcomp);
 	mutex_unlock(&gspca_dev->usb_lock);
 	return ret;

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: gspca: fix vidioc_s_jpegcomp locking

[Hans de Goede]

Jim Paris wrote:
> This locking looked wrong.
> 

Hi,

I appreciate the effort, but please do not send patches just because something 
looks wrong. The original code is perfectly fine. It check if the sub driver 
supports set_jcomp at all, this check does not need locking.

Regards,

Hans


> -jim
> 
> --
> 
> gspca: fix vidioc_s_jpegcomp locking
> 
> Signed-off-by: Jim Paris <jim@jtan.com>
> 
> diff -r b50857fea6df linux/drivers/media/video/gspca/gspca.c
> --- a/linux/drivers/media/video/gspca/gspca.c	Tue Dec 09 16:20:31 2008 -0500
> +++ b/linux/drivers/media/video/gspca/gspca.c	Tue Dec 09 16:55:39 2008 -0500
> @@ -1320,10 +1320,10 @@
>  	struct gspca_dev *gspca_dev = priv;
>  	int ret;
>  
> +	if (!gspca_dev->sd_desc->set_jcomp)
> +		return -EINVAL;
>  	if (mutex_lock_interruptible(&gspca_dev->usb_lock))
>  		return -ERESTARTSYS;
> -	if (!gspca_dev->sd_desc->set_jcomp)
> -		return -EINVAL;
>  	ret = gspca_dev->sd_desc->set_jcomp(gspca_dev, jpegcomp);
>  	mutex_unlock(&gspca_dev->usb_lock);
>  	return ret;
> 
> --
> video4linux-list mailing list
> Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
> https://www.redhat.com/mailman/listinfo/video4linux-list
> 

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: gspca: fix vidioc_s_jpegcomp locking

[Jim Paris]

Hans de Goede wrote:
> Jim Paris wrote:
>> This locking looked wrong.
>>
>
> Hi,
>
> I appreciate the effort, but please do not send patches just because 
> something looks wrong. The original code is perfectly fine. It check if 
> the sub driver supports set_jcomp at all, this check does not need 
> locking.

Well, the patch was a request for comments.  But please double-check.
In the current code, if set_jcomp is NULL, the lock is taken but never
released.

I agree that the check does not need locking, which is why my change
moved the check outside the lock.

-jim

>> gspca: fix vidioc_s_jpegcomp locking
>>
>> Signed-off-by: Jim Paris <jim@jtan.com>
>>
>> diff -r b50857fea6df linux/drivers/media/video/gspca/gspca.c
>> --- a/linux/drivers/media/video/gspca/gspca.c	Tue Dec 09 16:20:31 2008 -0500
>> +++ b/linux/drivers/media/video/gspca/gspca.c	Tue Dec 09 16:55:39 2008 -0500
>> @@ -1320,10 +1320,10 @@
>>  	struct gspca_dev *gspca_dev = priv;
>>  	int ret;
>> +	if (!gspca_dev->sd_desc->set_jcomp)
>> +		return -EINVAL;
>>  	if (mutex_lock_interruptible(&gspca_dev->usb_lock))
>>  		return -ERESTARTSYS;
>> -	if (!gspca_dev->sd_desc->set_jcomp)
>> -		return -EINVAL;
>>  	ret = gspca_dev->sd_desc->set_jcomp(gspca_dev, jpegcomp);
>>  	mutex_unlock(&gspca_dev->usb_lock);
>>  	return ret;

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: gspca: fix vidioc_s_jpegcomp locking

[Hans de Goede]

Jim Paris wrote:
> Hans de Goede wrote:
>> Jim Paris wrote:
>>> This locking looked wrong.
>>>
>> Hi,
>>
>> I appreciate the effort, but please do not send patches just because 
>> something looks wrong. The original code is perfectly fine. It check if 
>> the sub driver supports set_jcomp at all, this check does not need 
>> locking.
> 
> Well, the patch was a request for comments.  But please double-check.
> In the current code, if set_jcomp is NULL, the lock is taken but never
> released.
> 
> I agree that the check does not need locking, which is why my change
> moved the check outside the lock.
> 

Blergh, sorry I completely misread your patch, somehow reversing what it did in 
my head.

You are completely right, the original code is wrong, and this patch needs to 
be applied.

Sorry!

Regards,

Hans

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list