ACPI: battery: Fix CONFIG_ACPI_SYSFS_POWER=n

Disabling CONFIG_ACPI_SYSFS_POWER changes the behaviour of acpi_battery_update(). It will call acpi_battery_get_info() even if the battery is not present. I haven't noticed this causing any problem, but it does look like a bad idea.

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

index 58b4517..908edce 100644
@@ -501,13 +501,13 @@ static int acpi_battery_update(struct acpi_battery *battery)
 	result = acpi_battery_get_status(battery);
 	if (result)
 		return result;
-#ifdef CONFIG_ACPI_SYSFS_POWER
 	if (!acpi_battery_present(battery)) {
+#ifdef CONFIG_ACPI_SYSFS_POWER
 		sysfs_remove_battery(battery);
+#endif
 		battery->update_time = 0;
 		return 0;
 	}
-#endif
 	if (!battery->update_time ||
 	    old_present != acpi_battery_present(battery)) {
 		result = acpi_battery_get_info(battery);

1.6.3.2

Disabling CONFIG_ACPI_SYSFS_POWER changes the behaviour of acpi_battery_update(). It will call acpi_battery_get_info() even if the battery is not present. I haven't noticed this causing any problem, but it does look like a bad idea.

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

index 58b4517..908edce 100644
@@ -501,13 +501,13 @@ static int acpi_battery_update(struct acpi_battery *battery)
 	result = acpi_battery_get_status(battery);
 	if (result)
 		return result;
-#ifdef CONFIG_ACPI_SYSFS_POWER
 	if (!acpi_battery_present(battery)) {
+#ifdef CONFIG_ACPI_SYSFS_POWER
 		sysfs_remove_battery(battery);
+#endif
 		battery->update_time = 0;
 		return 0;
 	}
-#endif
 	if (!battery->update_time ||
 	    old_present != acpi_battery_present(battery)) {
 		result = acpi_battery_get_info(battery);

1.6.3.2

Calling kobject_uevent() directly is a layering violation. In particular, it means we'll miss updating the generic LED trigger.

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

index 908edce..7d31a23 100644
@@ -811,7 +811,7 @@ static void acpi_battery_notify(struct acpi_device *device, u32 event)
 #ifdef CONFIG_ACPI_SYSFS_POWER
 	/* acpi_batter_update could remove power_supply object */
 	if (battery->bat.dev)
-		kobject_uevent(&battery->bat.dev->kobj, KOBJ_CHANGE);
+		power_supply_changed(&battery->bat);
 #endif
 }
 

1.6.3.2

Keeping this device around lets userspace know that we have a battery bay, even if there is nothing in it at the moment. This is what every other battery driver does, so ACPI should do it as well.

For example, this means gnome-power-manager will now allow configuring behaviour for "on battery power" if the battery bay was empty on startup. (Thanks to Maxim Levitsky for pointing this out).

There is no reason to preserve the old behaviour. We now correctly provide the "present" attribute, which will return "0" when the battery is removed. HAL was already trying to check this attribute when the old behaviour was implemented. This should not break any version of HAL.

---
 drivers/acpi/battery.c |   36 ++++++++++++++++--------------------
 1 files changed, 16 insertions(+), 20 deletions(-)

index 7d31a23..f424033 100644
@@ -487,8 +487,6 @@ static int sysfs_add_battery(struct acpi_battery *battery)
 
 static void sysfs_remove_battery(struct acpi_battery *battery)
 {
-	if (!battery->bat.dev)
-		return;
 	device_remove_file(battery->bat.dev, &alarm_attr);
 	power_supply_unregister(&battery->bat);
 	battery->bat.dev = NULL;

@@ -502,9 +500,6 @@ static int acpi_battery_update(struct acpi_battery *battery)
 	if (result)
 		return result;
 	if (!acpi_battery_present(battery)) {
-#ifdef CONFIG_ACPI_SYSFS_POWER
-		sysfs_remove_battery(battery);
-#endif
 		battery->update_time = 0;
 		return 0;
 	}

@@ -515,10 +510,6 @@ static int acpi_battery_update(struct acpi_battery *battery)
 			return result;
 		acpi_battery_init_alarm(battery);
 	}
-#ifdef CONFIG_ACPI_SYSFS_POWER
-	if (!battery->bat.dev)
-		sysfs_add_battery(battery);
-#endif
 	return acpi_battery_get_state(battery);
 }
 

@@ -809,9 +800,7 @@ static void acpi_battery_notify(struct acpi_device *device, u32 event)
 					dev_name(&device->dev), event,
 					acpi_battery_present(battery));
 #ifdef CONFIG_ACPI_SYSFS_POWER
-	/* acpi_batter_update could remove power_supply object */
-	if (battery->bat.dev)
-		power_supply_changed(&battery->bat);
+	power_supply_changed(&battery->bat);
 #endif
 }
 

@@ -832,17 +821,24 @@ static int acpi_battery_add(struct acpi_device *device)
 	acpi_battery_update(battery);
 #ifdef CONFIG_ACPI_PROCFS_POWER
 	result = acpi_battery_add_fs(device);
+	if (result)
+		goto fail;
 #endif
-	if (!result) {
-		printk(KERN_INFO PREFIX "%s Slot [%s] (battery %s)\n",
-			ACPI_BATTERY_DEVICE_NAME, acpi_device_bid(device),
-			device->status.battery_present ? "present" : "absent");
-	} else {
+#ifdef CONFIG_ACPI_SYSFS_POWER
+	result = sysfs_add_battery(battery);
+	if (result)
+		goto fail;
+#endif
+	printk(KERN_INFO PREFIX "%s Slot [%s] (battery %s)\n",
+		ACPI_BATTERY_DEVICE_NAME, acpi_device_bid(device),
+		device->status.battery_present ? "present" : "absent");
+	return 0;
+
+fail:
 #ifdef CONFIG_ACPI_PROCFS_POWER
-		acpi_battery_remove_fs(device);
+	acpi_battery_remove_fs(device);
 #endif
-		kfree(battery);
-	}
+	kfree(battery);
 	return result;
 }
 

1.6.3.2

Hi,

So this is newer version of this patchset.
When this will be merged?

Last patch doesn't apply, seems that parts of yours patch were applied. I applied it manually.
Will soon test.

Best regards,
Maxim Levitsky

I will submit "To: len" next time round.

You're right, it no longer applies cleanly to acpi-test. ["patch" is happy to apply it and warn about "fuzz", but git-am is more strict. It doesn't look like there's a real conflict.]

I look forward to your results. Please tell me the diff you end up with for the last patch and what tree you applied it on top of. That way, if I add "Tested-by: Maxim..." I can be certain we're talking about the same patch :-).

Thanks!
Alan

I applied it on top of both vanilla and linux-next. Here it really doesn't apply (with or without 'fuzz')

I did that few days ago (applied patch manually, without much thinking)

Unfortunately, resulting kernel oopses if battery is present, and otherwise, if I plug batter later on, many battery statistics are missing (probably some locks are held)

I will soon investigate this issue, more deeply.

Best regards,
Maxim Levitsky

I've applied the patches from the mailing list, initially on -rc8 using git-am, then rebased them onto acpi-test without conflicts. I don't have GIT hosting, so here's a git bundle. If you're still having problems, you should be able to get an _exact_ copy of my tree with

$ git fetch alan.bundle battery-check:battery-check
$ git checkout battery-check

so long as you have the acpi-test commit I based it on.

Thanks
Alan

Indeed, it works.

Just one issue though, if I plug in the battery after system boot, it is detected a an 'energy battery' (energy_battery_props are used)

ACPI seems to be ok:

                Device (BAT0)
                {
                    Name (_HID, EisaId ("PNP0C0A"))
                    Name (_UID, 0x01)
                    Name (_PCL, Package (0x01)
                    {
                        \_SB
                    })
                    Name (PBIF, Package (0x0D)
                    {
                        0x01, 
                        0x0FA0, 
                        0x0FA0, 
                        0x01, 
                        0x39D0, 
                        0x0190, 
                        0x78, 
                        0x0108, 
                        0x0EC4, 
                        "Li_Ion 4000mA ", 
                        "", 
                        "Lion", 
                        "Acer   "
                    })
                    Name (PBST, Package (0x04)
                    {
                        0x01, 
                        0xFFFFFFFF, 
                        0xFFFFFFFF, 
                        0x39D0
                    })
                    Method (_STA, 0, NotSerialized)
                    {
                        If (ECON)
                        {
                            If (\_SB.PCI0.LPC.EC0.BAL1)
                            {
                                Sleep (0x14)
                                Return (0x1F)
                            }
                            Else
                            {
                                Sleep (0x14)
                                Return (0x0F)
                            }
                        }
                        Else
                        {
                            Sleep (0x14)
                            Return (0x1F)
                        }
                    }

                    Method (_BIF, 0, NotSerialized)
                    {
                        If (ECON)
                        {
                            Sleep (0x14)
                            Store (\_SB.PCI0.LPC.EC0.BDC0, Index (PBIF, 0x01))
                            Sleep (0x14)
                            Store (\_SB.PCI0.LPC.EC0.BFC0, Index (PBIF, 0x02))
                            Sleep (0x14)
                            Store (\_SB.PCI0.LPC.EC0.BDV0, Index (PBIF, 0x04))
                            Sleep (0x14)
                            Store (\_SB.PCI0.LPC.EC0.BDC0, Local2)
                            Divide (Local2, 0x64, Local6, Local2)
                            Multiply (Local2, 0x05, Local3)
                            Store (Local3, Index (PBIF, 0x05))
                            Multiply (Local2, 0x03, Local4)
                            Store (Local4, Index (PBIF, 0x06))
                            Store (\_SB.PCI0.LPC.EC0.BTY0, Local1)
                            Sleep (0x14)
                            If (LEqual (Local1, 0x01))
                            {
                                Store ("GC86508SAT0 ", Index (PBIF, 0x09))
                                Store ("SANYO ", Index (PBIF, 0x0C))
                            }
                            Else
                            {
                                If (LEqual (Local1, 0x02))
                                {
                                    Store ("GC86503SY90 ", Index (PBIF, 0x09))
                                    Store ("SONY ", Index (PBIF, 0x0C))
                                }
                                Else
                                {
                                    If (LEqual (Local1, 0x04))
                                    {
                                        Store ("GC86503PAG0 ", Index (PBIF, 0x09))
                                        Store ("PANASONIC ", Index (PBIF, 0x0C))
                                    }
                                    Else
                                    {
                                        If (LEqual (Local1, 0x05))
                                        {
                                            Store ("GC86508SM60 ", Index (PBIF, 0x09))
                                            Store ("SAMSUNG ", Index (PBIF, 0x0C))
                                        }
                                        Else
                                        {
                                            Store ("BCL3100LiON ", Index (PBIF, 0x09))
                                            Store ("COMPAL ", Index (PBIF, 0x0C))
                                        }
                                    }
                                }
                            }
                        }

                        Return (PBIF)
                    }

                    Method (_BST, 0, NotSerialized)
                    {
                        If (ECON)
                        {
                            Sleep (0x14)
                            Store (\_SB.PCI0.LPC.EC0.BST0, Local0)
                            And (Local0, 0x07, Local0)
                            Store (Local0, Index (PBST, 0x00))
                            Sleep (0x14)
                            Store (\_SB.PCI0.LPC.EC0.GAU0, Local2)
                            Sleep (0x14)
                            Store (\_SB.PCI0.LPC.EC0.BPV0, Local3)
                            Sleep (0x14)
                            Store (\_SB.PCI0.LPC.EC0.BFC0, Local1)
                            Sleep (0x14)
                            If (Local2)
                            {
                                Multiply (Local2, Local1, Local2)
                                Divide (Local2, 0x64, Local6, Local2)
                                If (Local6)
                                {
                                    Increment (Local2)
                                }
                            }

                            Store (\_SB.PCI0.LPC.EC0.BAC0, Local1)
                            If (And (Local1, 0x8000, Local1))
                            {
                                Store (\_SB.PCI0.LPC.EC0.BAC0, Local1)
                                Subtract (0xFFFF, Local1, Local1)
                            }
                            Else
                            {
                                Store (0x00, Local1)
                            }

                            Sleep (0x14)
                            Store (Local1, Index (PBST, 0x01))
                            Store (Local2, Index (PBST, 0x02))
                            Store (Local3, Index (PBST, 0x03))
                        }

                        Return (PBST)
                    }
                }

PBIF at offset 0 is set correctly to 1.

Best regards,
Maxim Levitsky

apparently I don't. Can you re-send this as a plain patch?

thanks,
-Len

Sure. I'll resend the first two patches in this series (the patch 3/3 was wrong and should not be applied).

Thanks
Alan