2.6.24-rc1: OOPS at acpi_battery_update

PATCH New Romano Giannetti 8 participants 19 messages Oct 29, 2007 – Nov 13, 2007 ACPI

lore

Share & tools

lore

Romano Giannetti

author

2007-10-29 10:11 UTC #0 lore raw

Hi,

sometime on resuming from s2ram my laptop spew the following oops.
Config, dmesg etc are at:

http://www.dea.icai.upcomillas.es/romano/linux/info/2624rc1_6/

[    3.475386] Oops: 0000 [#1] SMP 
[    3.475602] Process kacpi_notify (pid: 50, ti=c2122000 task=c210c030 task.ti=c2122000)
[    3.475608] Stack: c35c5060 c02f2a58 00000046 00000000 6b6b6b6b c2b20298 c2123efc c02f29ac 
[    3.475626]        c02166cb f896f3ca 00000000 f896ff63 00000246 00000000 c2b20298 ffffa512 
[    3.475644]        c2123f04 c02f2a58 c2123f34 f896f3ed c2123f1c c34fbdb8 c2123f20 c02f3e22 
[    3.475662] Call Trace:
[    3.475667]  [show_trace_log_lvl+26/48] show_trace_log_lvl+0x1a/0x30
[    3.475678]  [show_stack_log_lvl+177/224] show_stack_log_lvl+0xb1/0xe0
[    3.475695]  [die+282/560] die+0x11a/0x230
[    3.475687]  [show_registers+193/464] show_registers+0xc1/0x1d0
[    3.475703]  [do_page_fault+415/1648] do_page_fault+0x19f/0x670
[    3.475713]  [error_code+114/120] error_code+0x72/0x78
[    3.475722]  [__mutex_unlock_slowpath+172/336] __mutex_unlock_slowpath+0xac/0x150
[    3.475731]  [mutex_unlock+8/16] mutex_unlock+0x8/0x10
[    3.475739]  [<f896f3ed>] acpi_battery_update+0x1ce/0x23c [battery]
[    3.475753]  [<f896f927>] acpi_battery_notify+0x21/0x78 [battery]
[    3.475764]  [acpi_ev_notify_dispatch+79/90] acpi_ev_notify_dispatch+0x4f/0x5a
[    3.475792]  [worker_thread+157/256] worker_thread+0x9d/0x100
[    3.475774]  [acpi_os_execute_notify+36/47] acpi_os_execute_notify+0x24/0x2f
[    3.475784]  [run_workqueue+288/464] run_workqueue+0x120/0x1d0
[    3.475809]  [kernel_thread_helper+7/16] kernel_thread_helper+0x7/0x10
[    3.475801]  [kthread+66/112] kthread+0x42/0x70
[    3.475821] Code: 8d b4 26 00 00 00 00 55 89 e5 83 ec 18 89 5d f8 89 c3 89 75 fc 0f b6 40 04 89 d6 84 c0 7f 24 8d 43 20 3b 43 20 0f 84 e4 00 00 00 <3b> 76 10 0f 85 93 00 00 00 3b 36 90 74 47 8b 5d f8 8b 75 fc 89 
[    3.475818]  =======================
[    3.475909] EIP: [debug_mutex_wake_waiter+36/352] debug_mutex_wake_waiter+0x24/0x160 SS:ESP 0068:c2123ebc

Signature · 12 lines

Sorry for the disclaimer --- ¡I cannot stop it!



--
La presente comunicación tiene carácter confidencial y es para el exclusivo uso del destinatario indicado en la misma. Si Ud. no es el destinatario indicado, le informamos que cualquier forma de distribución, reproducción o uso de esta comunicación y/o de la información contenida en la misma están estrictamente prohibidos por la ley. Si Ud. ha recibido esta comunicación por error, por favor, notifíquelo inmediatamente al remitente contestando a este mensaje y proceda a continuación a destruirlo. Gracias por su colaboración.

This communication contains confidential information. It is for the exclusive use of the intended addressee. If you are not the intended addressee, please note that any form of distribution, copying or use of this communication or the information in it is strictly prohibited by law. If you have received this communication in error, please immediately notify the sender by reply e-mail and destroy this message. Thank you for your cooperation. 
-
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Andrew Morton

2007-11-01 23:15 UTC #1 lore raw

Quoted 25 lines · On Mon, 29 Oct 2007 11:11:04 +0100 Romano Giannetti <[email protected]>…

Hi,

sometime on resuming from s2ram my laptop spew the following oops.
Config, dmesg etc are at:

http://www.dea.icai.upcomillas.es/romano/linux/info/2624rc1_6/

[    3.475386] Oops: 0000 [#1] SMP 
[    3.475602] Process kacpi_notify (pid: 50, ti=c2122000 task=c210c030 task.ti=c2122000)
[    3.475608] Stack: c35c5060 c02f2a58 00000046 00000000 6b6b6b6b c2b20298 c2123efc c02f29ac 
[    3.475626]        c02166cb f896f3ca 00000000 f896ff63 00000246 00000000 c2b20298 ffffa512 
[    3.475644]        c2123f04 c02f2a58 c2123f34 f896f3ed c2123f1c c34fbdb8 c2123f20 c02f3e22 
[    3.475662] Call Trace:
[    3.475667]  [show_trace_log_lvl+26/48] show_trace_log_lvl+0x1a/0x30
[    3.475678]  [show_stack_log_lvl+177/224] show_stack_log_lvl+0xb1/0xe0
[    3.475695]  [die+282/560] die+0x11a/0x230
[    3.475687]  [show_registers+193/464] show_registers+0xc1/0x1d0
[    3.475703]  [do_page_fault+415/1648] do_page_fault+0x19f/0x670
[    3.475713]  [error_code+114/120] error_code+0x72/0x78
[    3.475722]  [__mutex_unlock_slowpath+172/336] __mutex_unlock_slowpath+0xac/0x150
[    3.475731]  [mutex_unlock+8/16] mutex_unlock+0x8/0x10
[    3.475739]  [<f896f3ed>] acpi_battery_update+0x1ce/0x23c [battery]
[    3.475753]  [<f896f927>] acpi_battery_notify+0x21/0x78 [battery]
[    3.475764]  [acpi_ev_notify_dispatch+79/90] acpi_ev_notify_dispatch+0x4f/0x5a

Quoted 9 lines

[    3.475792]  [worker_thread+157/256] worker_thread+0x9d/0x100
[    3.475774]  [acpi_os_execute_notify+36/47] acpi_os_execute_notify+0x24/0x2f
[    3.475784]  [run_workqueue+288/464] run_workqueue+0x120/0x1d0
[    3.475809]  [kernel_thread_helper+7/16] kernel_thread_helper+0x7/0x10
[    3.475801]  [kthread+66/112] kthread+0x42/0x70
[    3.475821] Code: 8d b4 26 00 00 00 00 55 89 e5 83 ec 18 89 5d f8 89 c3 89 75 fc 0f b6 40 04 89 d6 84 c0 7f 24 8d 43 20 3b 43 20 0f 84 e4 00 00 00 <3b> 76 10 0f 85 93 00 00 00 3b 36 90 74 47 8b 5d f8 8b 75 fc 89 
[    3.475818]  =======================
[    3.475909] EIP: [debug_mutex_wake_waiter+36/352] debug_mutex_wake_waiter+0x24/0x160 SS:ESP 0068:c2123ebc

Did any earlier kernels do this? In other words, do you believe that this is a bug which we added after 2.6.23 was released?

Thanks.

Rafael J. Wysocki

2007-11-02 15:52 UTC #2 lore raw

Quoted 38 lines · Andrew Morton

Quoted 25 lines · On Mon, 29 Oct 2007 11:11:04 +0100 Romano Giannetti <[email protected]>…

Hi,

sometime on resuming from s2ram my laptop spew the following oops.
Config, dmesg etc are at:

http://www.dea.icai.upcomillas.es/romano/linux/info/2624rc1_6/

[    3.475386] Oops: 0000 [#1] SMP 
[    3.475602] Process kacpi_notify (pid: 50, ti=c2122000 task=c210c030 task.ti=c2122000)
[    3.475608] Stack: c35c5060 c02f2a58 00000046 00000000 6b6b6b6b c2b20298 c2123efc c02f29ac 
[    3.475626]        c02166cb f896f3ca 00000000 f896ff63 00000246 00000000 c2b20298 ffffa512 
[    3.475644]        c2123f04 c02f2a58 c2123f34 f896f3ed c2123f1c c34fbdb8 c2123f20 c02f3e22 
[    3.475662] Call Trace:
[    3.475667]  [show_trace_log_lvl+26/48] show_trace_log_lvl+0x1a/0x30
[    3.475678]  [show_stack_log_lvl+177/224] show_stack_log_lvl+0xb1/0xe0
[    3.475695]  [die+282/560] die+0x11a/0x230
[    3.475687]  [show_registers+193/464] show_registers+0xc1/0x1d0
[    3.475703]  [do_page_fault+415/1648] do_page_fault+0x19f/0x670
[    3.475713]  [error_code+114/120] error_code+0x72/0x78
[    3.475722]  [__mutex_unlock_slowpath+172/336] __mutex_unlock_slowpath+0xac/0x150
[    3.475731]  [mutex_unlock+8/16] mutex_unlock+0x8/0x10
[    3.475739]  [<f896f3ed>] acpi_battery_update+0x1ce/0x23c [battery]
[    3.475753]  [<f896f927>] acpi_battery_notify+0x21/0x78 [battery]
[    3.475764]  [acpi_ev_notify_dispatch+79/90] acpi_ev_notify_dispatch+0x4f/0x5a

Quoted 9 lines

[    3.475792]  [worker_thread+157/256] worker_thread+0x9d/0x100
[    3.475774]  [acpi_os_execute_notify+36/47] acpi_os_execute_notify+0x24/0x2f
[    3.475784]  [run_workqueue+288/464] run_workqueue+0x120/0x1d0
[    3.475809]  [kernel_thread_helper+7/16] kernel_thread_helper+0x7/0x10
[    3.475801]  [kthread+66/112] kthread+0x42/0x70
[    3.475821] Code: 8d b4 26 00 00 00 00 55 89 e5 83 ec 18 89 5d f8 89 c3 89 75 fc 0f b6 40 04 89 d6 84 c0 7f 24 8d 43 20 3b 43 20 0f 84 e4 00 00 00 <3b> 76 10 0f 85 93 00 00 00 3b 36 90 74 47 8b 5d f8 8b 75 fc 89 
[    3.475818]  =======================
[    3.475909] EIP: [debug_mutex_wake_waiter+36/352] debug_mutex_wake_waiter+0x24/0x160 SS:ESP 0068:c2123ebc

Quoted 2 lines

Did any earlier kernels do this? In other words, do you believe that this is a bug which we added after 2.6.23 was released?

This might be the same as http://bugzilla.kernel.org/show_bug.cgi?id=9283

Romano Giannetti

author

2007-11-04 09:34 UTC #3 lore raw

Re: *SPAM* Re: 2.6.24-rc1: OOPS at acpi_battery_update

Quoted 2 lines · Rafael J. Wysocki

Quoted 1 line · Andrew Morton
On Mon, 29 Oct 2007 11:11:04 +0100

[OOPS removed]

Quoted 3 lines

Quoted 3 lines
Did any earlier kernels do this? In other words, do you believe that this is a bug which we added after 2.6.23 was released?

Never noticed it before. But it's happening just sometime (I've had it just two times), so I cannot be sure.

Quoted 1 line

This might be the same as http://bugzilla.kernel.org/show_bug.cgi?id=9283

Hmmm... there is too little info for my level in that bug report to understand if it's the same.

Romano

Signature · 12 lines

Sorry for the disclaimer --- ¡I cannot stop it!



--
La presente comunicación tiene carácter confidencial y es para el exclusivo uso del destinatario indicado en la misma. Si Ud. no es el destinatario indicado, le informamos que cualquier forma de distribución, reproducción o uso de esta comunicación y/o de la información contenida en la misma están estrictamente prohibidos por la ley. Si Ud. ha recibido esta comunicación por error, por favor, notifíquelo inmediatamente al remitente contestando a este mensaje y proceda a continuación a destruirlo. Gracias por su colaboración.

This communication contains confidential information. It is for the exclusive use of the intended addressee. If you are not the intended addressee, please note that any form of distribution, copying or use of this communication or the information in it is strictly prohibited by law. If you have received this communication in error, please immediately notify the sender by reply e-mail and destroy this message. Thank you for your cooperation. 
-
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Rafael J. Wysocki

2007-11-04 13:00 UTC #4 lore raw

Re: *SPAM* Re: 2.6.24-rc1: OOPS at acpi_battery_update

Quoted 9 lines · Romano Giannetti

Quoted 2 lines · Rafael J. Wysocki
Quoted 1 line · Andrew Morton
On Mon, 29 Oct 2007 11:11:04 +0100
[OOPS removed]
Quoted 3 lines
Quoted 3 lines
Did any earlier kernels do this? In other words, do you believe that this is a bug which we added after 2.6.23 was released?

Quoted 7 lines

Never noticed it before. But it's happening just sometime (I've had it just two times), so I cannot be sure.
Quoted 1 line
This might be the same as http://bugzilla.kernel.org/show_bug.cgi?id=9283
Hmmm... there is too little info for my level in that bug report to understand if it's the same.

Then, please apply the patch from

http://bugzilla.kernel.org/attachment.cgi?id=13376&action=view

and see if the problem goes away.

Michael (rabenkind) Brandstetter

2007-11-04 23:36 UTC #5 lore raw

Am Sonntag, 4. November 2007 14:17:23 schrieben Sie:

Quoted 4 lines · Romano Giannetti

Quoted 3 lines · Rafael J. Wysocki
Quoted 2 lines · Andrew Morton
Quoted 1 line
On Mon, 29 Oct 2007 11:11:04 +0100

Hello!

Quoted 4 lines

Quoted 4 lines
[OOPS removed]
Quoted 2 lines
Quoted 2 lines
Did any earlier kernels do this? In other words, do you believe that this is a bug which we added after 2.6.23 was released?

Yes the kernels prior to 2.6.24-rc1 did not have this bug.

The oops always appear when i plug in or remove the battery.

Quoted 6 lines

Quoted 6 lines
Never noticed it before. But it's happening just sometime (I've had it just two times), so I cannot be sure.
Quoted 2 lines
This might be the same as
http://bugzilla.kernel.org/show_bug.cgi?id=9283

Quoted 8 lines

Quoted 2 lines
Hmmm... there is too little info for my level in that bug report to understand if it's the same.
Then, please apply the patch from
http://bugzilla.kernel.org/attachment.cgi?id=13376&action=view
and see if the problem goes away.

The bug went away on my Dell Latitude C610. I applied the patch on two boxes (2.6.24-rc1-git12) and the haldaemon is not killed and no oops when i remove the battery or plug it in.

But on the Portege 3110CT the bug remains.

greetz Michael :))

Signature · 7 lines

Duisburger Linux User Group
Free Software Foundation Europe

  /"\        ASCII Ribbon Campaign
  \ /        No HTML/RTF in email
   x         No Word docs in email
  / \        Respect for open standards

Rafael J. Wysocki

2007-11-05 00:01 UTC #6 lore raw

Quoted 30 lines · Michael (rabenkind) Brandstetter

Am Sonntag, 4. November 2007 14:17:23 schrieben Sie:
Quoted 4 lines · Romano Giannetti
Quoted 3 lines · Rafael J. Wysocki
Quoted 2 lines · Andrew Morton
Quoted 1 line
On Mon, 29 Oct 2007 11:11:04 +0100
Hello!
Quoted 4 lines
Quoted 4 lines
[OOPS removed]
Quoted 2 lines
Quoted 2 lines
Did any earlier kernels do this? In other words, do you believe that this is a bug which we added after 2.6.23 was released?
Yes the kernels prior to 2.6.24-rc1 did not have this bug.
The oops always appear when i plug in or remove the battery.
Quoted 13 lines
Quoted 8 lines
Never noticed it before. But it's happening just sometime (I've had it just two times), so I cannot be sure.
Quoted 2 lines
This might be the same as
http://bugzilla.kernel.org/show_bug.cgi?id=9283
Hmmm... there is too little info for my level in that bug report to understand if it's the same.
Then, please apply the patch from
http://bugzilla.kernel.org/attachment.cgi?id=13376&action=view

Quoted 7 lines

Quoted 1 line
and see if the problem goes away.
The bug went away on my Dell Latitude C610. I applied the patch on two boxes (2.6.24-rc1-git12) and the haldaemon is not killed and no oops when i remove the battery or plug it in.
But on the Portege 3110CT the bug remains.

Well, please create a bug report at http://bugzilla.kernel.org regarding this bug and place all of the relevant information in there (including what boxes are fixed by the above patch etc.) Also please add my address to the report's CC list.

Thanks,
Rafael

Johannes Weiner

2007-11-08 15:53 UTC #7 lore raw

Hi,

is there any reason, why acpi_battery_get_property() should call acpi_battery_update() at all?

Hannes

Rafael J. Wysocki

2007-11-08 15:59 UTC #8 lore raw

Quoted 4 lines · Johannes Weiner

Hi,
is there any reason, why acpi_battery_get_property() should call acpi_battery_update() at all?

Alex?

Alexey Starikovskiy

2007-11-08 16:12 UTC #9 lore raw

Quoted 7 lines · Rafael J. Wysocki

Quoted 4 lines · Johannes Weiner
Hi,
is there any reason, why acpi_battery_get_property() should call acpi_battery_update() at all?
Alex?

If someone wants to read stale values, he could comment out acpi_battery_update.

Regards,
Alex.

Alexey Starikovskiy

Tested-by Rolf Eike Beer

2007-11-08 16:35 UTC #11 lore raw

In reply to Rafael J. Wysocki

Quoted 7 lines · Rafael J. Wysocki

Quoted 4 lines · Johannes Weiner
Hi,
is there any reason, why acpi_battery_get_property() should call acpi_battery_update() at all?
Alex?

Do you mean "why should it call _whole_ battery update?" ? get_state should be sufficient in order to not get stale data.

Regards,
Alex.

ACPI: Battery: remove cycle from battery removal.

From: Alexey Starikovskiy <[email protected]>

get_property() should not call battery_update() on absent battery to avoid cycle and oops.

Signed-off-by Alexey Starikovskiy <[email protected]>

Tested-by Rolf Eike Beer <[email protected]>

---

 drivers/acpi/battery.c |   10 ++++++----
 1 files changed, 6 insertions(+), 4 deletions(-)

drivers/acpi/battery.c +6 -4

index c2ce0ad..192c244 100644
@@ -132,7 +132,7 @@ static int acpi_battery_technology(struct acpi_battery *battery)
 	return POWER_SUPPLY_TECHNOLOGY_UNKNOWN;
 }
 
-static int acpi_battery_update(struct acpi_battery *battery);
+static int acpi_battery_get_state(struct acpi_battery *battery);
 
 static int acpi_battery_get_property(struct power_supply *psy,
 				     enum power_supply_property psp,

@@ -140,10 +140,11 @@ static int acpi_battery_get_property(struct power_supply *psy,
 {
 	struct acpi_battery *battery = to_acpi_battery(psy);
 
-	if ((!acpi_battery_present(battery)) &&
-	     psp != POWER_SUPPLY_PROP_PRESENT)
+	if (acpi_battery_present(battery)) {
+		/* run battery update only if it is present */
+		acpi_battery_get_state(battery);
+	} else if (psp != POWER_SUPPLY_PROP_PRESENT)
 		return -ENODEV;
-	acpi_battery_update(battery);
 	switch (psp) {
 	case POWER_SUPPLY_PROP_STATUS:
 		if (battery->state & 0x01)

@@ -457,6 +458,7 @@ static void sysfs_remove_battery(struct acpi_battery *battery)
 		return;
 	device_remove_file(battery->bat.dev, &alarm_attr);
 	power_supply_unregister(&battery->bat);
+	battery->bat.dev = NULL;
 }
 
 static int acpi_battery_update(struct acpi_battery *battery)

Johannes Weiner

2007-11-08 16:47 UTC #10 lore raw

In reply to Alexey Starikovskiy

Hi,

Quoted 10 lines · Alexey Starikovskiy

Quoted 7 lines · Rafael J. Wysocki
Quoted 4 lines · Johannes Weiner
Hi,
is there any reason, why acpi_battery_get_property() should call acpi_battery_update() at all?
Alex?
If someone wants to read stale values, he could comment out acpi_battery_update.

Please help me to understand:

When the battery is plugged in, the acpi_battery_notify() is called, which in turn calls acpi_battery_update(). The latter ensures that the sysfs files are created if not yet present.

When the battery is removed, acpi_battery_notify is called, which in turn calls acpi_battery_update(). The latter ensures that the sysfs files are removed if present.

During runtime - as far as I understood - no sysfs files have to be created/removed but the saved battery state info becomes stale.

So, would it be enough to call acpi_battery_get_state() in acpi_battery_get_property() instead of acpi_battery_update()?

If acpi_battery_get_state() does what I think it does, this would ensure that the battery state is reread and updated when acpi_battery_get_property() is called.

Hit me.

Hannes

Johannes Weiner

Acked-by

2007-11-08 17:00 UTC #12 lore raw

In reply to Alexey Starikovskiy

Hi Alex,

Quoted 10 lines · Alexey Starikovskiy

Quoted 7 lines · Rafael J. Wysocki
Quoted 4 lines · Johannes Weiner
Hi,
is there any reason, why acpi_battery_get_property() should call acpi_battery_update() at all?
Alex?
Do you mean "why should it call _whole_ battery update?" ? get_state should be sufficient in order to not get stale data.

Okay, than I understood it correct.

Acked-by Johannes Weiner <[email protected]>

Thanks!

Hannes

Andrew Morton

2007-11-09 04:35 UTC #13 lore raw

In reply to Alexey Starikovskiy

Quoted 10 lines · A> On Thu, 08 Nov 2007 19:35:23 +0300 Alexey Starikovskiy…

[remove_cycle_at_battery_removal.patch text/x-patch (1.7KB)]
ACPI: Battery: remove cycle from battery removal.
From: Alexey Starikovskiy <[email protected]>
get_property() should not call battery_update() on absent battery to avoid cycle and oops.
Signed-off-by Alexey Starikovskiy <[email protected]>
Tested-by Rolf Eike Beer <[email protected]>

A patch similar to this one but with an identical changelog was merged into Len's tree on November 2.

If it had been promptly merged into mainline then quite a lot of people's time would not have been wasted.

Alexey Starikovskiy

2007-11-09 09:37 UTC #14 lore raw

Quoted 9 lines · Andrew Morton

Quoted 8 lines · A> On Thu, 08 Nov 2007 19:35:23 +0300 Alexey Starikovskiy…
[remove_cycle_at_battery_removal.patch text/x-patch (1.7KB)]
ACPI: Battery: remove cycle from battery removal.
From: Alexey Starikovskiy <[email protected]>
get_property() should not call battery_update() on absent battery to avoid cycle and oops.

Quoted 9 lines

Quoted 2 lines
Signed-off-by Alexey Starikovskiy <[email protected]>
Tested-by Rolf Eike Beer <[email protected]>
A patch similar to this one but with an identical changelog was merged into Len's tree on November 2.
If it had been promptly merged into mainline then quite a lot of people's time would not have been wasted.

Andrew,
should I send such patches directly to you/Linus?

Regards,
Alex

Andrew Morton

2007-11-09 09:50 UTC #15 lore raw

Quoted 13 lines · On Fri, 09 Nov 2007 12:36:43 +0300 Alexey Starikovskiy <[email protected]>…

Quoted 12 lines · Andrew Morton
Quoted 10 lines · A> On Thu, 08 Nov 2007 19:35:23 +0300 Alexey Starikovskiy…
[remove_cycle_at_battery_removal.patch text/x-patch (1.7KB)]
ACPI: Battery: remove cycle from battery removal.
From: Alexey Starikovskiy <[email protected]>
get_property() should not call battery_update() on absent battery to avoid cycle and oops.
Signed-off-by Alexey Starikovskiy <[email protected]>
Tested-by Rolf Eike Beer <[email protected]>

Quoted 9 lines

Quoted 6 lines
A patch similar to this one but with an identical changelog was merged into Len's tree on November 2.
If it had been promptly merged into mainline then quite a lot of people's time would not have been wasted.
Andrew,
should I send such patches directly to you/Linus?

Well if Len doesn't object and you're confident in the changes, why not? Any time we leave bugs unfixed we drive away testers and that impacts all parts of the kernel.

Alexey Starikovskiy

Tested-by Rolf Eike Beer

2007-11-13 08:35 UTC #16 lore raw

Andrew,

I can not contact with Len for several days, while the oops on battery seems quite important. It also seem to behave well in -mm tree (as part of Len's acpi-test).
Will you send this patch to Linus without approval from Len or should I?

Thanks,
Alex.

Quoted 25 lines · Andrew Morton

Quoted 22 lines · On Fri, 09 Nov 2007 12:36:43 +0300 Alexey Starikovskiy <[email protected]>…
Quoted 20 lines · Andrew Morton
Quoted 11 lines · A> On Thu, 08 Nov 2007 19:35:23 +0300 Alexey Starikovskiy…
[remove_cycle_at_battery_removal.patch text/x-patch (1.7KB)]
ACPI: Battery: remove cycle from battery removal.
From: Alexey Starikovskiy <[email protected]>
get_property() should not call battery_update() on absent battery to avoid cycle and oops.
Signed-off-by Alexey Starikovskiy <[email protected]>
Tested-by Rolf Eike Beer <[email protected]>
A patch similar to this one but with an identical changelog was merged into Len's tree on November 2.
If it had been promptly merged into mainline then quite a lot of people's time would not have been wasted.

Quoted 9 lines

Quoted 4 lines
Andrew,
should I send such patches directly to you/Linus?
Well if Len doesn't object and you're confident in the changes, why not? Any time we leave bugs unfixed we drive away testers and that impacts all parts of the kernel.

ACPI: Battery: remove cycle from battery removal.

From: Alexey Starikovskiy <[email protected]>

get_property() should not call battery_update() on absent battery to avoid cycle and oops.

Signed-off-by Alexey Starikovskiy <[email protected]>

Tested-by Rolf Eike Beer <[email protected]>

---

 drivers/acpi/battery.c |   10 ++++++----
 1 files changed, 6 insertions(+), 4 deletions(-)

drivers/acpi/battery.c +6 -4

index c2ce0ad..192c244 100644
@@ -132,7 +132,7 @@ static int acpi_battery_technology(struct acpi_battery *battery)
 	return POWER_SUPPLY_TECHNOLOGY_UNKNOWN;
 }
 
-static int acpi_battery_update(struct acpi_battery *battery);
+static int acpi_battery_get_state(struct acpi_battery *battery);
 
 static int acpi_battery_get_property(struct power_supply *psy,
 				     enum power_supply_property psp,

@@ -140,10 +140,11 @@ static int acpi_battery_get_property(struct power_supply *psy,
 {
 	struct acpi_battery *battery = to_acpi_battery(psy);
 
-	if ((!acpi_battery_present(battery)) &&
-	     psp != POWER_SUPPLY_PROP_PRESENT)
+	if (acpi_battery_present(battery)) {
+		/* run battery update only if it is present */
+		acpi_battery_get_state(battery);
+	} else if (psp != POWER_SUPPLY_PROP_PRESENT)
 		return -ENODEV;
-	acpi_battery_update(battery);
 	switch (psp) {
 	case POWER_SUPPLY_PROP_STATUS:
 		if (battery->state & 0x01)

@@ -457,6 +458,7 @@ static void sysfs_remove_battery(struct acpi_battery *battery)
 		return;
 	device_remove_file(battery->bat.dev, &alarm_attr);
 	power_supply_unregister(&battery->bat);
+	battery->bat.dev = NULL;
 }
 
 static int acpi_battery_update(struct acpi_battery *battery)

Andrew Morton

2007-11-13 08:59 UTC #17 lore raw

Quoted 4 lines · On Tue, 13 Nov 2007 11:35:18 +0300 Alexey Starikovskiy <[email protected]>…

I can not contact with Len for several days, while the oops on battery seems quite important.
It also seem to behave well in -mm tree (as part of Len's acpi-test). Will you send this patch to Linus without approval from Len or should I?

Please send it yourself - your latest version seems a little different from the one in git-acpi and I'd just be dangerous trying to work out which one is needed.

Alexey Starikovskiy

2007-11-13 09:23 UTC #18 lore raw

PATCH 1/1 ACPI: Battery: remove cycle from battery removal.

In reply to Alexey Starikovskiy

get_property() should not call battery_update(),
it also should call get_status() only if battery is present to avoid cycle and oops.

Signed-off-by Alexey Starikovskiy <[email protected]>

Tested-by Rolf Eike Beer <[email protected]>

Acked-by Johannes Weiner <[email protected]>

---

 drivers/acpi/battery.c |   10 ++++++----
 1 files changed, 6 insertions(+), 4 deletions(-)

drivers/acpi/battery.c +6 -4

index c2ce0ad..192c244 100644
@@ -132,7 +132,7 @@ static int acpi_battery_technology(struct acpi_battery *battery)
 	return POWER_SUPPLY_TECHNOLOGY_UNKNOWN;
 }
 
-static int acpi_battery_update(struct acpi_battery *battery);
+static int acpi_battery_get_state(struct acpi_battery *battery);
 
 static int acpi_battery_get_property(struct power_supply *psy,
 				     enum power_supply_property psp,

@@ -140,10 +140,11 @@ static int acpi_battery_get_property(struct power_supply *psy,
 {
 	struct acpi_battery *battery = to_acpi_battery(psy);
 
-	if ((!acpi_battery_present(battery)) &&
-	     psp != POWER_SUPPLY_PROP_PRESENT)
+	if (acpi_battery_present(battery)) {
+		/* run battery update only if it is present */
+		acpi_battery_get_state(battery);
+	} else if (psp != POWER_SUPPLY_PROP_PRESENT)
 		return -ENODEV;
-	acpi_battery_update(battery);
 	switch (psp) {
 	case POWER_SUPPLY_PROP_STATUS:
 		if (battery->state & 0x01)

@@ -457,6 +458,7 @@ static void sysfs_remove_battery(struct acpi_battery *battery)
 		return;
 	device_remove_file(battery->bat.dev, &alarm_attr);
 	power_supply_unregister(&battery->bat);
+	battery->bat.dev = NULL;
 }
 
 static int acpi_battery_update(struct acpi_battery *battery)