Re: [patch] revert: "ACPI: drivers/acpi: elide a non-zero test ona result that is never 0"

From: Julia Lawall
Date: Fri Mar 28 2008 - 10:11:21 EST

Next message: Ingo Molnar: "Re: agpgart: when telling user you'll corrupt his data, at leastdo it at KERN_CRIT"
Previous message: Ingo Molnar: "Re: [patch 09/10] Hugetlb common code update for System z."
In reply to: Ingo Molnar: "[patch] revert: "ACPI: drivers/acpi: elide a non-zero test on aresult that is never 0""
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Zhang Rui already found the problem with the patch.

The calling context has to cope with both the results of the normal
definition of the function and a definition in a header file that returns
a completely different kind of result.

Sorry for not having studied the issue more thoroughly in the beginning.

julia

On Fri, 28 Mar 2008, Ingo Molnar wrote:

>
> * Len Brown <lenb@xxxxxxxxxx> wrote:
>
> > I've not used "-b" until now. I added it because Julia's patch was
> > simple, but since it changed indenting of a couple of blocks its
> > diffstat was otherwise large.
>
> > Julia Lawall (1):
> > ACPI: drivers/acpi: elide a non-zero test on a result that is never 0
>
> overnight randconfig qa on x86.git/latest triggered a bootup crash after
> just 7 iterations, which i bisected down to the commit above. Find the
> revert below.
>
> Observation: the patch was just 3 days old when it went upstream and
> given that it touches 50 lines of code executed on most PC hardware
> during bootup so i dont think it was in the trivial category.
>
> Even if the fix is right (which is does look to be at first sight),
> there's clearly some side-effect here and the whitespace changes mixed
> into the functional changes make it hard to validate this change.
>
> now that i had a second look, one side-effect seems to be:
>
> + acpi_driver_data(device) = cdev;
>
> this used to be executed before even with a NULL cdev and isnt executed
> now. (In any case, the revert below is the right thing to do i believe,
> the patch should have its clock reset and should restart its testing
> cycle at the tail of the development queue.)
>
> Ingo
>
> ---------------------->
> Subject: revert "ACPI: drivers/acpi: elide a non-zero test on a result that is never 0"
> From: Ingo Molnar <mingo@xxxxxxx>
> Date: Fri Mar 28 14:28:03 CET 2008
>
> revert:
>
> ACPI: drivers/acpi: elide a non-zero test on a result that is never 0
>
> as randconfig testing found that it causes a crash during bootup:
>
> initcall 0x78878534 ran for 13 msecs: acpi_button_init+0x0/0x51()
> Calling initcall 0x78878585: acpi_fan_init+0x0/0x2c()
> BUG: unable to handle kernel NULL pointer dereference at 00000000
> IP: [<782b8ad0>] acpi_fan_add+0x7d/0xfd
> *pde = 00000000
> Oops: 0000 [#1]
> Modules linked in:
>
> Pid: 1, comm: swapper Not tainted (2.6.25-rc7-sched-devel.git-x86-latest.git #14)
> EIP: 0060:[<782b8ad0>] EFLAGS: 00010246 CPU: 0
> EIP is at acpi_fan_add+0x7d/0xfd
> EAX: b787c718 EBX: b787c400 ECX: b782ceb4 EDX: 00000007
> ESI: 00000000 EDI: b787c6f4 EBP: b782cee0 ESP: b782cecc
> DS: 007b ES: 007b FS: 0000 GS: 0000 SS: 0068
> Process swapper (pid: 1, ti=b782c000 task=b7846000 task.ti=b782c000)
> Stack: b787c459 00000000 b787c400 78790888 b787c60c b782cef8 782b6fb8 ffffffda
> b787c60c 00000000 78790958 b782cf0c 783005d7 b787c60c 78790958 78790584
> b782cf1c 783007f6 b782cf28 00000000 b782cf40 782ffc4a 78790958 b794d558
> Call Trace:
> [<782b6fb8>] ? acpi_device_probe+0x3e/0xdb
> [<783005d7>] ? driver_probe_device+0x82/0xfc
> [<783007f6>] ? __driver_attach+0x3a/0x70
> [<782ffc4a>] ? bus_for_each_dev+0x3e/0x60
> [<7830048c>] ? driver_attach+0x14/0x16
> [<783007bc>] ? __driver_attach+0x0/0x70
> [<7830006a>] ? bus_add_driver+0x9d/0x1b0
> [<783008c3>] ? driver_register+0x47/0xa3
> [<7813db00>] ? timespec_to_ktime+0x9/0xc
> [<782b7331>] ? acpi_bus_register_driver+0x3a/0x3c
> [<78878592>] ? acpi_fan_init+0xd/0x2c
> [<78863656>] ? kernel_init+0xac/0x1f9
> [<788635aa>] ? kernel_init+0x0/0x1f9
> [<78114563>] ? kernel_thread_helper+0x7/0x10
> =======================
> Code: 6e 78 e8 57 44 e7 ff 58 e9 93 00 00 00 8b 55 f0 8d bb f4 02 00 00 80 4b 2d 10 8b 03 e8 87 cb ff ff 8d 83 18 03 00 00 80 63 2d ef <ff> 35 00 00 00 00 50 68 e8 9c 6e 78 e8 22 44 e7 ff b9 b6 9c 6e
> EIP: [<782b8ad0>] acpi_fan_add+0x7d/0xfd SS:ESP 0068:b782cecc
> ---[ end trace 778e504de7e3b1e3 ]---
> Kernel panic - not syncing: Attempted to kill init!
>
> Signed-off-by: Ingo Molnar <mingo@xxxxxxx>
> ---
> drivers/acpi/fan.c | 34 ++++++++++++++++++----------------
> drivers/acpi/processor_core.c | 30 ++++++++++++++++--------------
> drivers/acpi/video.c | 28 +++++++++++++++-------------
> 3 files changed, 49 insertions(+), 43 deletions(-)
>
> Index: linux-x86.q/drivers/acpi/fan.c
> ===================================================================
> --- linux-x86.q.orig/drivers/acpi/fan.c
> +++ linux-x86.q/drivers/acpi/fan.c
> @@ -260,22 +260,24 @@ static int acpi_fan_add(struct acpi_devi
> result = PTR_ERR(cdev);
> goto end;
> }
> - printk(KERN_INFO PREFIX
> - "%s is registered as cooling_device%d\n",
> - device->dev.bus_id, cdev->id);
> -
> - acpi_driver_data(device) = cdev;
> - result = sysfs_create_link(&device->dev.kobj,
> - &cdev->device.kobj,
> - "thermal_cooling");
> - if (result)
> - return result;
> -
> - result = sysfs_create_link(&cdev->device.kobj,
> - &device->dev.kobj,
> - "device");
> - if (result)
> - return result;
> + if (cdev) {
> + printk(KERN_INFO PREFIX
> + "%s is registered as cooling_device%d\n",
> + device->dev.bus_id, cdev->id);
> +
> + acpi_driver_data(device) = cdev;
> + result = sysfs_create_link(&device->dev.kobj,
> + &cdev->device.kobj,
> + "thermal_cooling");
> + if (result)
> + return result;
> +
> + result = sysfs_create_link(&cdev->device.kobj,
> + &device->dev.kobj,
> + "device");
> + if (result)
> + return result;
> + }
>
> result = acpi_fan_add_fs(device);
> if (result)
> Index: linux-x86.q/drivers/acpi/processor_core.c
> ===================================================================
> --- linux-x86.q.orig/drivers/acpi/processor_core.c
> +++ linux-x86.q/drivers/acpi/processor_core.c
> @@ -674,20 +674,22 @@ static int __cpuinit acpi_processor_star
> result = PTR_ERR(pr->cdev);
> goto end;
> }
> - printk(KERN_INFO PREFIX
> - "%s is registered as cooling_device%d\n",
> - device->dev.bus_id, pr->cdev->id);
> -
> - result = sysfs_create_link(&device->dev.kobj,
> - &pr->cdev->device.kobj,
> - "thermal_cooling");
> - if (result)
> - return result;
> - result = sysfs_create_link(&pr->cdev->device.kobj,
> - &device->dev.kobj,
> - "device");
> - if (result)
> - return result;
> + if (pr->cdev) {
> + printk(KERN_INFO PREFIX
> + "%s is registered as cooling_device%d\n",
> + device->dev.bus_id, pr->cdev->id);
> +
> + result = sysfs_create_link(&device->dev.kobj,
> + &pr->cdev->device.kobj,
> + "thermal_cooling");
> + if (result)
> + return result;
> + result = sysfs_create_link(&pr->cdev->device.kobj,
> + &device->dev.kobj,
> + "device");
> + if (result)
> + return result;
> + }
>
> if (pr->flags.throttling) {
> printk(KERN_INFO PREFIX "%s [%s] (supports",
> Index: linux-x86.q/drivers/acpi/video.c
> ===================================================================
> --- linux-x86.q.orig/drivers/acpi/video.c
> +++ linux-x86.q/drivers/acpi/video.c
> @@ -734,19 +734,21 @@ static void acpi_video_device_find_cap(s
> if (IS_ERR(device->cdev))
> return;
>
> - printk(KERN_INFO PREFIX
> - "%s is registered as cooling_device%d\n",
> - device->dev->dev.bus_id, device->cdev->id);
> - result = sysfs_create_link(&device->dev->dev.kobj,
> - &device->cdev->device.kobj,
> - "thermal_cooling");
> - if (result)
> - printk(KERN_ERR PREFIX "Create sysfs link\n");
> - result = sysfs_create_link(&device->cdev->device.kobj,
> - &device->dev->dev.kobj,
> - "device");
> - if (result)
> - printk(KERN_ERR PREFIX "Create sysfs link\n");
> + if (device->cdev) {
> + printk(KERN_INFO PREFIX
> + "%s is registered as cooling_device%d\n",
> + device->dev->dev.bus_id, device->cdev->id);
> + result = sysfs_create_link(&device->dev->dev.kobj,
> + &device->cdev->device.kobj,
> + "thermal_cooling");
> + if (result)
> + printk(KERN_ERR PREFIX "Create sysfs link\n");
> + result = sysfs_create_link(&device->cdev->device.kobj,
> + &device->dev->dev.kobj,
> + "device");
> + if (result)
> + printk(KERN_ERR PREFIX "Create sysfs link\n");
> + }
> }
> if (device->cap._DCS && device->cap._DSS){
> static int count = 0;
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Next message: Ingo Molnar: "Re: agpgart: when telling user you'll corrupt his data, at leastdo it at KERN_CRIT"
Previous message: Ingo Molnar: "Re: [patch 09/10] Hugetlb common code update for System z."
In reply to: Ingo Molnar: "[patch] revert: "ACPI: drivers/acpi: elide a non-zero test on aresult that is never 0""
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]