Regression 2.6.39-rc1 for sony-laptop

Regression 2.6.39-rc1 for sony-laptop

[Andrea Gelmini]

[-- Attachment #1: Type: text/plain, Size: 4317 bytes --]

Hi all,
   and thanks a lot for your time.
   We current git tree I've got this:
[   10.299221] sony-laptop: Sony Programmable IO Control Driver v0.6.
[   10.299237] sony-laptop: detected Type3 model
[   10.300885] input: Sony Vaio Keys as
/devices/LNXSYSTM:00/device:00/PNP0A08:00/device:45/SNY6001:00/input/input3
[   10.301516] input: Sony Vaio Jogdial as /devices/virtual/input/input4
[   10.302366] sony-laptop: device allocated minor is 58
[   10.310838] sony-laptop: Sony Notebook Control Driver v0.6.
[   10.310940] BUG: unable to handle kernel NULL pointer dereference at   (null)
[   10.310996] IP: [<f816c0b0>] sony_find_snc_handle+0x10/0x70 [sony_laptop]
[   10.311023] *pde = 00000000
[   10.311023] Oops: 0000 [#1] PREEMPT SMP
[   10.311083] last sysfs file: /sys/bus/platform/drivers/sony-laptop/uevent
[   10.311083] Modules linked in: sony_laptop(+) psmouse loop
processor snd_hda_codec rfkill evdev i915 fbcon font bitblit
softcursor drm_kms_helper intel_agp cfbcopyarea video sky2 intel_gtt
thermal thermal_sys button uhci_hcd cfbimgblt cfbfillrect
[   10.311083] Pid: 1793, comm: modprobe Not tainted 2.6.39-rc1-test+
#1 Sony Corporation VGN-SZ1VP/VAIO
[   10.311083] EIP: 0060:[<f816c0b0>] EFLAGS: 00010296 CPU: 0
[   10.311083] EIP is at sony_find_snc_handle+0x10/0x70 [sony_laptop]
[   10.311083] EAX: 0000012f EBX: 00000000 ECX: 00000000 EDX: 00000286
[   10.311083] ESI: f47d9e78 EDI: f500b400 EBP: 00000000 ESP: f47d9e18
[   10.311083]  DS: 007b ES: 007b FS: 00d8 GS: 0033 SS: 0068
[   10.311083] Process modprobe (pid: 1793, ti=f47d8000 task=f4bc0e40
task.ti=f47d8000)
[   10.311083] Stack:
[   10.311083]  00000000 00000000 00000000 00000000 f816e694 f817126d
f8171250 f817124c
[   10.311083]  f4943460 0000447f f47d9e74 f5083bd0 f4935b10 00000000
c110ef27 c110decf
[   10.311083]  f47d9e90 f4935ae0 f47d9e90 c110e873 00000000 41ed9559
f44c34e0 f5083bd0
[   10.311083] Call Trace:
[   10.311083]  [<f816e694>] ? sony_nc_add+0x2a4/0x930 [sony_laptop]
[   10.311083]  [<c110ef27>] ? sysfs_do_create_link+0xb7/0x1d0
[   10.311083]  [<c110decf>] ? __sysfs_add_one+0x1f/0xf0
[   10.311083]  [<c110e873>] ? sysfs_addrm_finish+0x13/0xc0
[   10.311083]  [<c11e9862>] ? acpi_device_probe+0x37/0xec
[   10.311083]  [<c1247c97>] ? driver_probe_device+0x77/0x190
[   10.311083]  [<c11e9c58>] ? acpi_match_device_ids+0x27/0x49
[   10.311083]  [<c1247e29>] ? __driver_attach+0x79/0x80
[   10.311083]  [<c1247db0>] ? driver_probe_device+0x190/0x190
[   10.311083]  [<c1247522>] ? bus_for_each_dev+0x52/0x80
[   10.311083]  [<c1247b26>] ? driver_attach+0x16/0x20
[   10.311083]  [<c1247db0>] ? driver_probe_device+0x190/0x190
[   10.311083]  [<c1246f07>] ? bus_add_driver+0x177/0x240
[   10.311083]  [<c11e979f>] ? acpi_device_hid+0x14/0x14
[   10.311083]  [<c12483b3>] ? driver_register+0x63/0x120
[   10.311083]  [<f827104d>] ? sony_laptop_init+0x4d/0x1000 [sony_laptop]
[   10.311083]  [<c1001123>] ? do_one_initcall+0x33/0x170
[   10.311083]  [<f8271000>] ? 0xf8270fff
[   10.311083]  [<c1065b49>] ? sys_init_module+0xe9/0x350
[   10.311083]  [<c10c1b51>] ? sys_write+0x41/0x80
[   10.311083]  [<c137a110>] ? sysenter_do_call+0x12/0x26
[   10.311083] Code: 2c 17 f8 5b 3b 15 28 2c 17 f8 ba 41 00 00 00 0f
45 c2 c3 90 8d b4 26 00 00 00 00 53 31 db 83 ec 0c 8b 0d dc 2c 17 f8
8d 74 26 00 <0f> b7 14 59 39 c2 74 20 43 83 fb 10 75 f2 8b 15 a8 2c 17
f8 85
[   10.311083] EIP: [<f816c0b0>] sony_find_snc_handle+0x10/0x70
[sony_laptop] SS:ESP 0068:f47d9e18
[   10.311083] CR2: 0000000000000000
[   10.354377] ---[ end trace 1c42dff203c3e1bc ]---

Git bisect blames this commit:

commit 9af0e0fb70ed8e2387323c19496a7e174363f7b6
Author: Anssi Hannula <anssi.hannula@iki.fi>
Date:   Sun Feb 20 20:07:20 2011 +0200

    hp-wmi: check query return value in hp_wmi_perform_query

    Check BIOS provided return value code in hp_wmi_perform_query and print
    a warning on error. Printing is suppressed for HPWMI_RET_UNKNOWN_CMDTYPE
    which is returned when the command type is unsupported.

    Signed-off-by: Anssi Hannula <anssi.hannula@iki.fi>
    Signed-off-by: Matthew Garrett <mjg@redhat.com>

 drivers/platform/x86/hp-wmi.c |   16 ++++++++++++++++
 1 files changed, 16 insertions(+), 0 deletions(-)

In attachment other info.
If you need more details, please tell me.

Thanks a lot for your time,
Andrea

[-- Attachment #2: config.bz2 --]
[-- Type: application/x-bzip2, Size: 19968 bytes --]

[-- Attachment #3: dmesg.txt.bz2 --]
[-- Type: application/x-bzip2, Size: 14474 bytes --]

[-- Attachment #4: dmidecode.txt.bz2 --]
[-- Type: application/x-bzip2, Size: 1989 bytes --]

Re: Regression 2.6.39-rc1 for sony-laptop

[Matthew Garrett]

On Fri, Apr 01, 2011 at 07:16:43PM +0200, Andrea Gelmini wrote:

> Git bisect blames this commit:

Pretty convinced that git bisect's got that wrong, given that it doesn't 
touch any Sony code. I'll take a look at the Sony patches, thanks for 
the report!

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: Regression 2.6.39-rc1 for sony-laptop

[Anssi Hannula]

On 01.04.2011 20:22, Matthew Garrett wrote:
> On Fri, Apr 01, 2011 at 07:16:43PM +0200, Andrea Gelmini wrote:
> 
>> Git bisect blames this commit:
> 
> Pretty convinced that git bisect's got that wrong, given that it doesn't 
> touch any Sony code. I'll take a look at the Sony patches, thanks for 
> the report!

The same issue was posted yesterday on platform-driver-x86@ as "NULL
pointer dereference in sony-laptop" by Alessandro Guido, with a
preliminary patch from Mattia Dongili:

http://thread.gmane.org/gmane.linux.drivers.platform.x86.devel/1720

-- 
Anssi Hannula

Re: Regression 2.6.39-rc1 for sony-laptop

[Andrea Gelmini]

2011/4/1 Matthew Garrett <mjg59@srcf.ucam.org>:
> 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 <malattia@linux.it>
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 <malattia@linux.it>
    Signed-off-by: Matthew Garrett <mjg@redhat.com>

 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.

Thanks a lot for your time,
Andrea

Re: Regression 2.6.39-rc1 for sony-laptop

[Andrea Gelmini]

2011/4/1 Anssi Hannula <anssi.hannula@iki.fi>:
> The same issue was posted yesterday on platform-driver-x86@ as "NULL
> pointer dereference in sony-laptop" by Alessandro Guido, with a
> preliminary patch from Mattia Dongili:

Hi Anssi,
   and thanks a lot for your quick answer.

Ciao,
Andrea

Re: Regression 2.6.39-rc1 for sony-laptop

[Mattia Dongili]

On Sat, Apr 02, 2011 at 11:44:57AM +0200, Andrea Gelmini wrote:
> 2011/4/1 Matthew Garrett <mjg59@srcf.ucam.org>:
> > 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 <malattia@linux.it>
> 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 <malattia@linux.it>
>     Signed-off-by: Matthew Garrett <mjg@redhat.com>
> 
>  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 <malattia@linux.it>
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 <malattia@linux.it>

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",
-- 
mattia
:wq!

Re: Regression 2.6.39-rc1 for sony-laptop

[Dan Carpenter]

On 4/2/11, Mattia Dongili <malattia@linux.it> 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

Re: Regression 2.6.39-rc1 for sony-laptop

[Mattia Dongili]

On Sat, Apr 02, 2011 at 02:55:38PM +0300, Dan Carpenter wrote:
> On 4/2/11, Mattia Dongili <malattia@linux.it> 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).

-- 
mattia
:wq!

Re: Regression 2.6.39-rc1 for sony-laptop

[Dan Carpenter]

On 4/2/11, Mattia Dongili <malattia@linux.it> wrote:
> On Sat, Apr 02, 2011 at 02:55:38PM +0300, Dan Carpenter wrote:
>> On 4/2/11, Mattia Dongili <malattia@linux.it> 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

sony-laptop: fix early NULL pointer dereference

[Mattia Dongili]

Author: Mattia Dongili <malattia@linux.it>
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 <malattia@linux.it>
---

Hi Matthew,
if it's not too late, can you pick this one up instead of the previous
one (89ec2feafaedd759e53346d641f60863a14cfb9e)?
If it's too late I'll try and do a round of return value fixes later.

diff --git a/drivers/platform/x86/sony-laptop.c b/drivers/platform/x86/sony-laptop.c
index b2ce172..de79c18 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 -EINVAL;
+
 	for (i = 0; i < 0x10; i++) {
 		if (handles->cap[i] == handle) {
 			dprintk("found handle 0x%.4x (offset: 0x%.2x)\n",
-- 
mattia
:wq!

Re: sony-laptop: fix early NULL pointer dereference

[Thiago Farina]

On Mon, Apr 4, 2011 at 8:44 PM, Mattia Dongili <malattia@linux.it> wrote:
> Author: Mattia Dongili <malattia@linux.it>
> Date:   Fri Apr 1 10:01:41 2011 +0900
>
I think most of the patches doesn't include these above lines nor the
subject line in the description. Also when submitting a patch to the
list, please don't forget to put [PATCH] on the subject line.

git format-patch should output the right format.

>    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 <malattia@linux.it>
> ---
>
> Hi Matthew,
> if it's not too late, can you pick this one up instead of the previous
> one (89ec2feafaedd759e53346d641f60863a14cfb9e)?
> If it's too late I'll try and do a round of return value fixes later.
>
> diff --git a/drivers/platform/x86/sony-laptop.c b/drivers/platform/x86/sony-laptop.c
> index b2ce172..de79c18 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 */
This comment is useless, it is just repeating what the codes does ;) I
think you can just remove it.

> +       if (!handles)
> +               return -EINVAL;
> +
>        for (i = 0; i < 0x10; i++) {
>                if (handles->cap[i] == handle) {
>                        dprintk("found handle 0x%.4x (offset: 0x%.2x)\n",
> --
> mattia
> :wq!
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>

Re: sony-laptop: fix early NULL pointer dereference

[Dan Carpenter]

On 4/5/11, Thiago Farina <tfransosi@gmail.com> wrote:
> On Mon, Apr 4, 2011 at 8:44 PM, Mattia Dongili <malattia@linux.it> wrote:
>> Author: Mattia Dongili <malattia@linux.it>
>> Date:   Fri Apr 1 10:01:41 2011 +0900
>>
> I think most of the patches doesn't include these above lines nor the
> subject line in the description.

Just ignore those lines.  It's a git thing.

> Also when submitting a patch to the
> list, please don't forget to put [PATCH] on the subject line.
>

Yeah.  [patch v2].  But it's too late to fix that so don't worry about it.

> git format-patch should output the right format.
>
>>    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 <malattia@linux.it>
>> ---
>>
>> Hi Matthew,
>> if it's not too late, can you pick this one up instead of the previous
>> one (89ec2feafaedd759e53346d641f60863a14cfb9e)?
>> If it's too late I'll try and do a round of return value fixes later.

Don't worry about it.  Probably the real fix is to make checkpatch.pl
complain if you return -1 instead of a proper error code.

>>
>> diff --git a/drivers/platform/x86/sony-laptop.c
>> b/drivers/platform/x86/sony-laptop.c
>> index b2ce172..de79c18 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 */
> This comment is useless, it is just repeating what the codes does ;) I
> think you can just remove it.

Whatever...  Let's just merge this fix and let's move on.

regards,
dan carpenter

>
>> +       if (!handles)
>> +               return -EINVAL;
>> +
>>        for (i = 0; i < 0x10; i++) {
>>                if (handles->cap[i] == handle) {
>>                        dprintk("found handle 0x%.4x (offset: 0x%.2x)\n",