2.6.24-rc1: OOPS at acpi_battery_update

2.6.24-rc1: OOPS at acpi_battery_update

[Romano Giannetti]

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




-- 
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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: 2.6.24-rc1: OOPS at acpi_battery_update

[Andrew Morton]

On Mon, 29 Oct 2007 11:11:04 +0100
Romano Giannetti <romanol@upcomillas.es> wrote:

> 
> 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
> 

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.

[PATCH] ACPI: Battery: remove cycle from battery removal.

[Alexey Starikovskiy]

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

Signed-off-by: Alexey Starikovskiy <astarikovskiy@suse.de>
Tested-by: Rolf Eike Beer <eike-kernel@sf-tec.de>
---

 drivers/acpi/battery.c |    8 +++++---
 1 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c
index c2ce0ad..ca37db4 100644
--- a/drivers/acpi/battery.c
+++ b/drivers/acpi/battery.c
@@ -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_update(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)

Re: 2.6.24-rc1: OOPS at acpi_battery_update

[Rafael J. Wysocki]

On Friday, 2 November 2007 00:14, Andrew Morton wrote:
> On Mon, 29 Oct 2007 11:11:04 +0100
> Romano Giannetti <romanol@upcomillas.es> wrote:
> 
> > 
> > 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
> > 
> 
> 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

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

[Romano Giannetti]

On Fri, 2007-11-02 at 17:08 +0100, Rafael J. Wysocki wrote:
> On Friday, 2 November 2007 00:14, Andrew Morton wrote:
> > On Mon, 29 Oct 2007 11:11:04 +0100
[OOPS removed]
> > 
> > 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.

> 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 

-- 
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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Rafael J. Wysocki]

On Sunday, 4 of November 2007, Romano Giannetti wrote:
> 
> On Fri, 2007-11-02 at 17:08 +0100, Rafael J. Wysocki wrote:
> > On Friday, 2 November 2007 00:14, Andrew Morton wrote:
> > > On Mon, 29 Oct 2007 11:11:04 +0100
> [OOPS removed]
> > > 
> > > 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.
> 
> > 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.

Re: 2.6.24-rc1: OOPS at acpi_battery_update

[Michael (rabenkind) Brandstetter]

Am Sonntag, 4. November 2007 14:17:23 schrieben Sie:
> On Sunday, 4 of November 2007, Romano Giannetti wrote:
> > On Fri, 2007-11-02 at 17:08 +0100, Rafael J. Wysocki wrote:
> > > On Friday, 2 November 2007 00:14, Andrew Morton wrote:
> > > > On Mon, 29 Oct 2007 11:11:04 +0100

Hello!

> > [OOPS removed]
> >
> > > > 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.

> > Never noticed it before. But it's happening just sometime (I've had it
> > just two times), so I cannot be sure.
> >
> > > 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.

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 :))
-- 
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

Re: 2.6.24-rc1: OOPS at acpi_battery_update

[Rafael J. Wysocki]

On Monday, 5 of November 2007, Michael (rabenkind) Brandstetter wrote:
> Am Sonntag, 4. November 2007 14:17:23 schrieben Sie:
> > On Sunday, 4 of November 2007, Romano Giannetti wrote:
> > > On Fri, 2007-11-02 at 17:08 +0100, Rafael J. Wysocki wrote:
> > > > On Friday, 2 November 2007 00:14, Andrew Morton wrote:
> > > > > On Mon, 29 Oct 2007 11:11:04 +0100
> 
> Hello!
> 
> > > [OOPS removed]
> > >
> > > > > 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.
> 
> > > Never noticed it before. But it's happening just sometime (I've had it
> > > just two times), so I cannot be sure.
> > >
> > > > 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.
> 
> 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

Re: 2.6.24-rc1: OOPS at acpi_battery_update

[Johannes Weiner]

Hi,

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

	Hannes

Re: 2.6.24-rc1: OOPS at acpi_battery_update

[Alexey Starikovskiy]

Rafael J. Wysocki wrote:
> On Thursday, 8 of November 2007, Johannes Weiner wrote:
>> 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.

Re: 2.6.24-rc1: OOPS at acpi_battery_update

[Rafael J. Wysocki]

On Thursday, 8 of November 2007, Johannes Weiner wrote:
> Hi,
> 
> is there any reason, why acpi_battery_get_property() should call
> acpi_battery_update() at all?

Alex?

Re: 2.6.24-rc1: OOPS at acpi_battery_update

[Alexey Starikovskiy]

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

Rafael J. Wysocki wrote:
> On Thursday, 8 of November 2007, Johannes Weiner wrote:
>> 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.

[-- Attachment #2: remove_cycle_at_battery_removal.patch --]
[-- Type: text/x-patch, Size: 1733 bytes --]

ACPI: Battery: remove cycle from battery removal.

From: Alexey Starikovskiy <astarikovskiy@suse.de>

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

Signed-off-by: Alexey Starikovskiy <astarikovskiy@suse.de>
Tested-by: Rolf Eike Beer <eike-kernel@sf-tec.de>
---

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

diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c
index c2ce0ad..192c244 100644
--- a/drivers/acpi/battery.c
+++ b/drivers/acpi/battery.c
@@ -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)

Re: 2.6.24-rc1: OOPS at acpi_battery_update

[Johannes Weiner]

Hi,

On Thu, Nov 08, 2007 at 07:11:53PM +0300, Alexey Starikovskiy wrote:
> Rafael J. Wysocki wrote:
> >On Thursday, 8 of November 2007, Johannes Weiner wrote:
> >>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

Re: 2.6.24-rc1: OOPS at acpi_battery_update

[Johannes Weiner]

Hi Alex,

On Thu, Nov 08, 2007 at 07:35:23PM +0300, Alexey Starikovskiy wrote:
> Rafael J. Wysocki wrote:
> >On Thursday, 8 of November 2007, Johannes Weiner wrote:
> >>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 <hannes@saeurebad.de>

Thanks!

	Hannes

Re: 2.6.24-rc1: OOPS at acpi_battery_update

[Andrew Morton]

A> On Thu, 08 Nov 2007 19:35:23 +0300 Alexey Starikovskiy <astarikovskiy@suse.de> wrote:
> [remove_cycle_at_battery_removal.patch  text/x-patch (1.7KB)]
> ACPI: Battery: remove cycle from battery removal.
> 
> From: Alexey Starikovskiy <astarikovskiy@suse.de>
> 
> get_property() should not call battery_update() on absent battery to
> avoid cycle and oops.
> 
> Signed-off-by: Alexey Starikovskiy <astarikovskiy@suse.de>
> Tested-by: Rolf Eike Beer <eike-kernel@sf-tec.de>

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.

Re: 2.6.24-rc1: OOPS at acpi_battery_update

[Alexey Starikovskiy]

Andrew Morton wrote:
> A> On Thu, 08 Nov 2007 19:35:23 +0300 Alexey Starikovskiy <astarikovskiy@suse.de> wrote:
>> [remove_cycle_at_battery_removal.patch  text/x-patch (1.7KB)]
>> ACPI: Battery: remove cycle from battery removal.
>>
>> From: Alexey Starikovskiy <astarikovskiy@suse.de>
>>
>> get_property() should not call battery_update() on absent battery to
>> avoid cycle and oops.
>>
>> Signed-off-by: Alexey Starikovskiy <astarikovskiy@suse.de>
>> Tested-by: Rolf Eike Beer <eike-kernel@sf-tec.de>
> 
> 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

Re: 2.6.24-rc1: OOPS at acpi_battery_update

[Andrew Morton]

On Fri, 09 Nov 2007 12:36:43 +0300 Alexey Starikovskiy <astarikovskiy@suse.de> wrote:

> Andrew Morton wrote:
> > A> On Thu, 08 Nov 2007 19:35:23 +0300 Alexey Starikovskiy <astarikovskiy@suse.de> wrote:
> >> [remove_cycle_at_battery_removal.patch  text/x-patch (1.7KB)]
> >> ACPI: Battery: remove cycle from battery removal.
> >>
> >> From: Alexey Starikovskiy <astarikovskiy@suse.de>
> >>
> >> get_property() should not call battery_update() on absent battery to
> >> avoid cycle and oops.
> >>
> >> Signed-off-by: Alexey Starikovskiy <astarikovskiy@suse.de>
> >> Tested-by: Rolf Eike Beer <eike-kernel@sf-tec.de>
> > 
> > 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.

Re: 2.6.24-rc1: OOPS at acpi_battery_update

[Alexey Starikovskiy]

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

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.

Andrew Morton wrote:
> On Fri, 09 Nov 2007 12:36:43 +0300 Alexey Starikovskiy <astarikovskiy@suse.de> wrote:
>
>   
>> Andrew Morton wrote:
>>     
>>> A> On Thu, 08 Nov 2007 19:35:23 +0300 Alexey Starikovskiy <astarikovskiy@suse.de> wrote:
>>>       
>>>> [remove_cycle_at_battery_removal.patch  text/x-patch (1.7KB)]
>>>> ACPI: Battery: remove cycle from battery removal.
>>>>
>>>> From: Alexey Starikovskiy <astarikovskiy@suse.de>
>>>>
>>>> get_property() should not call battery_update() on absent battery to
>>>> avoid cycle and oops.
>>>>
>>>> Signed-off-by: Alexey Starikovskiy <astarikovskiy@suse.de>
>>>> Tested-by: Rolf Eike Beer <eike-kernel@sf-tec.de>
>>>>         
>>> 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.
>
>   


[-- Attachment #2: remove_cycle_at_battery_removal.patch --]
[-- Type: text/x-patch, Size: 1733 bytes --]

ACPI: Battery: remove cycle from battery removal.

From: Alexey Starikovskiy <astarikovskiy@suse.de>

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

Signed-off-by: Alexey Starikovskiy <astarikovskiy@suse.de>
Tested-by: Rolf Eike Beer <eike-kernel@sf-tec.de>
---

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

diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c
index c2ce0ad..192c244 100644
--- a/drivers/acpi/battery.c
+++ b/drivers/acpi/battery.c
@@ -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)

Re: 2.6.24-rc1: OOPS at acpi_battery_update

[Andrew Morton]

On Tue, 13 Nov 2007 11:35:18 +0300 Alexey Starikovskiy <aystarik@gmail.com> wrote:

> 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.

[PATCH] ACPI: Battery: remove cycle from battery removal.

[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 <astarikovskiy@suse.de>
Tested-by: Rolf Eike Beer <eike-kernel@sf-tec.de>
Acked-by: Johannes Weiner <hannes@saeurebad.de>
---

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

diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c
index c2ce0ad..192c244 100644
--- a/drivers/acpi/battery.c
+++ b/drivers/acpi/battery.c
@@ -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)

[PATCH] ACPI: Battery: remove cycle from battery removal.

[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 <astarikovskiy@suse.de>
Tested-by: Rolf Eike Beer <eike-kernel@sf-tec.de>
Acked-by: Johannes Weiner <hannes@saeurebad.de>
---

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

diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c
index c2ce0ad..192c244 100644
--- a/drivers/acpi/battery.c
+++ b/drivers/acpi/battery.c
@@ -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)

Re: [PATCH] ACPI: Battery: remove cycle from battery removal.

[Alexey Starikovskiy]

Rafael J. Wysocki wrote:
> On Wednesday, 14 of November 2007, Alexey Starikovskiy wrote:
>> 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 <astarikovskiy@suse.de>
>> Tested-by: Rolf Eike Beer <eike-kernel@sf-tec.de>
>> Acked-by: Johannes Weiner <hannes@saeurebad.de>
> 
> It looks like this patch has been merged as commit
> 9104476e4efbef8a8e32d48ced583603ff32a2db
Yes, I noticed.

Thanks,
Alex.

Re: [PATCH] ACPI: Battery: remove cycle from battery removal.

[Rafael J. Wysocki]

On Wednesday, 14 of November 2007, Alexey Starikovskiy wrote:
> 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 <astarikovskiy@suse.de>
> Tested-by: Rolf Eike Beer <eike-kernel@sf-tec.de>
> Acked-by: Johannes Weiner <hannes@saeurebad.de>

It looks like this patch has been merged as commit
9104476e4efbef8a8e32d48ced583603ff32a2db


> ---
> 
>  drivers/acpi/battery.c |   10 ++++++----
>  1 files changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c
> index c2ce0ad..192c244 100644
> --- a/drivers/acpi/battery.c
> +++ b/drivers/acpi/battery.c
> @@ -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)
> 
> -