Patchwork [SRU,Oneiric/Precise,Quantal] UBUNTU: SAUCE: Unlock the rc_dev lock when the raw device is missing

login
register
mail settings
Submitter Herton Ronaldo Krzesinski
Date Aug. 7, 2012, 1:50 p.m.
Message ID <1344347451-4467-1-git-send-email-herton.krzesinski@canonical.com>
Download mbox | patch
Permalink /patch/175635/
State New
Headers show

Comments

Herton Ronaldo Krzesinski - Aug. 7, 2012, 1:50 p.m.
From: Douglas Bagnall <douglas@paradise.net.nz>

BugLink: http://bugs.launchpad.net/bugs/1015836

As pointed out by Ben Hutchings, after commit 720bb6436, the lock was
being taken and not released when an rc_dev has a NULL raw device.

Signed-off-by: Douglas Bagnall <douglas@paradise.net.nz>
Reported-by: Ben Hutchings <ben@decadent.org.uk>
Signed-off-by: Herton Ronaldo Krzesinski <herton.krzesinski@canonical.com>
---
 drivers/media/rc/rc-main.c |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Commit 720bb6436 missed an mutex_unlock on exit, thanks to Ben Hutchings
for pointing that out. Marked as "SAUCE" since it's not upstream yet,
but it should be included upstream.
Tim Gardner - Aug. 7, 2012, 1:56 p.m.

Stefan Bader - Aug. 7, 2012, 2:01 p.m.
Looking at the complete code it looks good enough to have this
pre-stable/upstream. Just the additional commit message after the sob's
looks a bit strange...


On 07.08.2012 15:50, Herton Ronaldo Krzesinski wrote:
> From: Douglas Bagnall <douglas@paradise.net.nz>
> 
> BugLink: http://bugs.launchpad.net/bugs/1015836
> 
> As pointed out by Ben Hutchings, after commit 720bb6436, the lock was
> being taken and not released when an rc_dev has a NULL raw device.
> 
> Signed-off-by: Douglas Bagnall <douglas@paradise.net.nz>
> Reported-by: Ben Hutchings <ben@decadent.org.uk>
> Signed-off-by: Herton Ronaldo Krzesinski <herton.krzesinski@canonical.com>
> ---
>  drivers/media/rc/rc-main.c |    5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> Commit 720bb6436 missed an mutex_unlock on exit, thanks to Ben Hutchings
> for pointing that out. Marked as "SAUCE" since it's not upstream yet,
> but it should be included upstream.
> 
> diff --git a/drivers/media/rc/rc-main.c b/drivers/media/rc/rc-main.c
> index cabc19c..dcd45d0 100644
> --- a/drivers/media/rc/rc-main.c
> +++ b/drivers/media/rc/rc-main.c
> @@ -778,9 +778,10 @@ static ssize_t show_protocols(struct device *device,
>  	} else if (dev->raw) {
>  		enabled = dev->raw->enabled_protocols;
>  		allowed = ir_raw_get_allowed_protocols();
> -	} else
> +	} else {
> +		mutex_unlock(&dev->lock);
>  		return -ENODEV;
> -
> +	}
>  	IR_dprintk(1, "allowed - 0x%llx, enabled - 0x%llx\n",
>  		   (long long)allowed,
>  		   (long long)enabled);
>
Tim Gardner - Aug. 7, 2012, 2:03 p.m.
On 08/07/2012 08:01 AM, Stefan Bader wrote:
> Looking at the complete code it looks good enough to have this
> pre-stable/upstream. Just the additional commit message after the sob's
> looks a bit strange...

Everything after the first '---' until a valid "^diff" is elided by 'git
am'.

> On 07.08.2012 15:50, Herton Ronaldo Krzesinski wrote:
>> From: Douglas Bagnall <douglas@paradise.net.nz>
>>
>> BugLink: http://bugs.launchpad.net/bugs/1015836
>>
>> As pointed out by Ben Hutchings, after commit 720bb6436, the lock was
>> being taken and not released when an rc_dev has a NULL raw device.
>>
>> Signed-off-by: Douglas Bagnall <douglas@paradise.net.nz>
>> Reported-by: Ben Hutchings <ben@decadent.org.uk>
>> Signed-off-by: Herton Ronaldo Krzesinski <herton.krzesinski@canonical.com>
>> ---
>>  drivers/media/rc/rc-main.c |    5 +++--
>>  1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> Commit 720bb6436 missed an mutex_unlock on exit, thanks to Ben Hutchings
>> for pointing that out. Marked as "SAUCE" since it's not upstream yet,
>> but it should be included upstream.
>>
>> diff --git a/drivers/media/rc/rc-main.c b/drivers/media/rc/rc-main.c
>> index cabc19c..dcd45d0 100644
>> --- a/drivers/media/rc/rc-main.c
>> +++ b/drivers/media/rc/rc-main.c
>> @@ -778,9 +778,10 @@ static ssize_t show_protocols(struct device *device,
>>  	} else if (dev->raw) {
>>  		enabled = dev->raw->enabled_protocols;
>>  		allowed = ir_raw_get_allowed_protocols();
>> -	} else
>> +	} else {
>> +		mutex_unlock(&dev->lock);
>>  		return -ENODEV;
>> -
>> +	}
>>  	IR_dprintk(1, "allowed - 0x%llx, enabled - 0x%llx\n",
>>  		   (long long)allowed,
>>  		   (long long)enabled);
>>
> 
> 
> 
>
Herton Ronaldo Krzesinski - Aug. 7, 2012, 2:08 p.m.
On Tue, Aug 07, 2012 at 04:01:56PM +0200, Stefan Bader wrote:
> Looking at the complete code it looks good enough to have this
> pre-stable/upstream. Just the additional commit message after the sob's
> looks a bit strange...

It was just me commenting about the patch, it's not part of the original
changelog of the patch sent upstream, didn't want that included when the
patch is applied, so it's below diffstat.
Stefan Bader - Aug. 7, 2012, 2:10 p.m.
On 07.08.2012 16:08, Herton Ronaldo Krzesinski wrote:
> On Tue, Aug 07, 2012 at 04:01:56PM +0200, Stefan Bader wrote:
>> Looking at the complete code it looks good enough to have this
>> pre-stable/upstream. Just the additional commit message after the sob's
>> looks a bit strange...
> 
> It was just me commenting about the patch, it's not part of the original
> changelog of the patch sent upstream, didn't want that included when the
> patch is applied, so it's below diffstat.
> 
Ah ok, I was not sure it is intended that way.

Patch

diff --git a/drivers/media/rc/rc-main.c b/drivers/media/rc/rc-main.c
index cabc19c..dcd45d0 100644
--- a/drivers/media/rc/rc-main.c
+++ b/drivers/media/rc/rc-main.c
@@ -778,9 +778,10 @@  static ssize_t show_protocols(struct device *device,
 	} else if (dev->raw) {
 		enabled = dev->raw->enabled_protocols;
 		allowed = ir_raw_get_allowed_protocols();
-	} else
+	} else {
+		mutex_unlock(&dev->lock);
 		return -ENODEV;
-
+	}
 	IR_dprintk(1, "allowed - 0x%llx, enabled - 0x%llx\n",
 		   (long long)allowed,
 		   (long long)enabled);