Regression 2.6.39-rc1 for sony-laptop

Commit Message

Mattia Dongili April 2, 2011, 10 a.m. UTC

On Sat, Apr 02, 2011 at 11:44:57AM +0200, Andrea Gelmini wrote:
> 2011/4/1 Matthew Garrett <[e-mail]>:
> > touch any Sony code. I'll take a look at the Sony patches, thanks for
> > the report!
> 
> Hi Matthew,
>    and thanks a lot for your quick answer.
>    Maybe I messed up with bisect trying to find the origin of two regression.
>    One was this. The other one is this one.
> 
>    Hibernation (to disk, of course) doesn't work no more.
>    Screen blanks and nothing happens. No HD activity, and so on. I also
>    waited some minutes.
> 
>    Well, bisect blame this commit:
> 
> commit 2a4f0c81adcd1f812a63bc9106be2fd26f437730
> Author: Mattia Dongili <[e-mail]>
> Date:   Sat Feb 19 11:52:30 2011 +0900
> 
>     sony-laptop: cache handles and report them via sysfs
> 
>     Avoid calling into acpi each time we need to lookup a method handle
>     and report the available handles to ease collection of information when
>     debugging issues. Also move initialization of the platform driver
>     earlier to allow adding files from other setup functions.
> 
>     Signed-off-by: Mattia Dongili <[e-mail]>
>     Signed-off-by: Matthew Garrett <[e-mail]>
> 
>  drivers/platform/x86/sony-laptop.c |   97 ++++++++++++++++++++++++++++++------
>  1 files changed, 81 insertions(+), 16 deletions(-)
> 
>    Do you have any suggestion to better track down this issue?
>    It's not possibile to simply revert it.

As already reported the following patch should fix the issue:

commit 88d25cbfda526567dabf056a868d8ff5f22a962e
Author: Mattia Dongili <[e-mail]>
Date:   Fri Apr 1 10:01:41 2011 +0900

    sony-laptop: fix early NULL pointer dereference
    
    The SNC acpi driver could get early notifications before it fully
    initializes and that could lead to dereferencing the sony_nc_handles
    structure pointer that is still NULL at that stage.
    Make sure we return early from the handle lookup function in these
    cases.
    
    Signed-off-by: Mattia Dongili <[e-mail]>

Comments

Dan Carpenter April 2, 2011, 11:55 a.m. UTC | #1

On 4/2/11, Mattia Dongili <[e-mail]> wrote:
> --- a/drivers/platform/x86/sony-laptop.c
> +++ b/drivers/platform/x86/sony-laptop.c
> @@ -810,6 +810,11 @@ static int sony_nc_handles_cleanup(struct
> platform_device *pd)
>  static int sony_find_snc_handle(int handle)
>  {
>  	int i;
> +
> +	/* not initialized yet, return early */
> +	if (!handles)
> +		return -1;

-1 is -EPERM.  That's not the right error code here.  Maybe -EINVAL?

regards,
dan carpenter
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Mattia Dongili April 2, 2011, 3:55 p.m. UTC | #2

On Sat, Apr 02, 2011 at 02:55:38PM +0300, Dan Carpenter wrote:
> On 4/2/11, Mattia Dongili <[e-mail]> wrote:
> > --- a/drivers/platform/x86/sony-laptop.c
> > +++ b/drivers/platform/x86/sony-laptop.c
> > @@ -810,6 +810,11 @@ static int sony_nc_handles_cleanup(struct
> > platform_device *pd)
> >  static int sony_find_snc_handle(int handle)
> >  {
> >  	int i;
> > +
> > +	/* not initialized yet, return early */
> > +	if (!handles)
> > +		return -1;
> 
> -1 is -EPERM.  That's not the right error code here.  Maybe -EINVAL?

this error is not propagated to userspace. If necessary I can review all
error codes in the sony-laptop internal functions (where -1 is a fairly
common return code for error conditions).

I remember a discussion on LKML recently about error codes but I'm not
sure what the outcome was (if any).

Dan Carpenter April 4, 2011, 7:37 a.m. UTC | #3

On 4/2/11, Mattia Dongili <[e-mail]> wrote:
> On Sat, Apr 02, 2011 at 02:55:38PM +0300, Dan Carpenter wrote:
>> On 4/2/11, Mattia Dongili <[e-mail]> wrote:
>> > --- a/drivers/platform/x86/sony-laptop.c
>> > +++ b/drivers/platform/x86/sony-laptop.c
>> > @@ -810,6 +810,11 @@ static int sony_nc_handles_cleanup(struct
>> > platform_device *pd)
>> >  static int sony_find_snc_handle(int handle)
>> >  {
>> >  	int i;
>> > +
>> > +	/* not initialized yet, return early */
>> > +	if (!handles)
>> > +		return -1;
>>
>> -1 is -EPERM.  That's not the right error code here.  Maybe -EINVAL?
>
> this error is not propagated to userspace. If necessary I can review all
> error codes in the sony-laptop internal functions (where -1 is a fairly
> common return code for error conditions).

That would be good, but it's going beyond the call of duty.  The main thing
is to not introduce new slop.

>
> I remember a discussion on LKML recently about error codes but I'm not
> sure what the outcome was (if any).
>

People sometimes think -1 is a generic error code, but it's not.  It has
a specific wrong meaning.  In fact, -1 is never the right error code.
It's should
either be -EPERM, or the appropriate error code.

regards,
dan carpenter
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/platform/x86/sony-laptop.c b/drivers/platform/x86/sony-laptop.c
index b2ce172..7082c55 100644
--- a/drivers/platform/x86/sony-laptop.c
+++ b/drivers/platform/x86/sony-laptop.c
@@ -810,6 +810,11 @@  static int sony_nc_handles_cleanup(struct platform_device *pd)
 static int sony_find_snc_handle(int handle)
 {
 	int i;
+
+	/* not initialized yet, return early */
+	if (!handles)
+		return -1;
+
 	for (i = 0; i < 0x10; i++) {
 		if (handles->cap[i] == handle) {
 			dprintk("found handle 0x%.4x (offset: 0x%.2x)\n",