[PATCH] [2.6.24-rc] ACPI: register power_supply subdevice only when battery is present

From: Andrey Borzenkov
Date: Sun Oct 28 2007 - 03:37:46 EST

Next message: WANG Cong: "[Git Patch] arch/um/os-Linux/: varied improvements and fixes"
Previous message: Amit Shah: "Re: sysfs: WARNING: at fs/sysfs/dir.c:424 sysfs_add_one()"
In reply to: Andrey Borzenkov: "Re: [PATCH] 2.6.24-rc1: ensure "present" sysfs attribute even if battery is absent"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Sunday 28 October 2007, Andrey Borzenkov wrote:
> On Saturday 27 October 2007, Anton Vorontsov wrote:
> > On Sat, Oct 27, 2007 at 08:54:30PM +0400, Andrey Borzenkov wrote:
> > > I am not exactly sure about this one ... what other power_supply class
> > > drivers do? Should I fix HAL instead (but then, I do not know whether
> > > HAL is the only application that is using this interface).
> >
> > Well, PROP_PRESENT wasn't my idea, currently it's used by pmu and
> > olpc drivers becuase it's not trivial to register/unregister their
> > batteries on physical insertion/removal. I have some plans to teach
> > at least pmu batteries to not use PROP_PRESENT. I don't have any
> > OLPC, thus I can't convert it.
> >
> > To sum this: the good way to handle "missing" batteries is to
> > unregister them, so they'll not show up in the /sys/class/power_supply.
>
> Well, in this case HAL behaviour makes sense (default to present == true
> if "present" attribute is missing)
>
> > Is that possible with the ACPI?
>
> At least looking in ACPI specs, this looks possible. What currently is
> presented as battery object is actually battery bay according to ACPI spec:
>
> Unlike most other devices, when a battery is inserted or removed from the
> system, the device itself (the
> battery bay) is still considered to be present in the system.
>
> When battery is inserted/removed, ACPI notifies us and we can check whether
> battery is actually present and update registration accordingly. From the
> sysfs structure POV probably nothing has to be changed; just when and how
> power_supply is registered
> under /sys/devices/LNXSYSTM:00/device:00/PNP0C0A:0n
>
> Alexey, does it make sense (or doable)?

or would you take attached patch as prototype? :) This works on my system and
does not crash it immediately. Here is event sequence I get:

UEVENT[1193556388.161539] add /module/battery (module)
ACTION=add
DEVPATH=/module/battery
SUBSYSTEM=module
SEQNUM=3243

UEVENT[1193556388.177069] add /bus/acpi/drivers/battery (drivers)
ACTION=add
DEVPATH=/bus/acpi/drivers/battery
SUBSYSTEM=drivers
SEQNUM=3244

UEVENT[1193556388.212721]
add /devices/LNXSYSTM:00/device:00/PNP0C0A:00/power_supply/BAT1
(power_supply)
ACTION=add
DEVPATH=/devices/LNXSYSTM:00/device:00/PNP0C0A:00/power_supply/BAT1
SUBSYSTEM=power_supply
SEQNUM=3245

UEVENT[1193556388.223113]
change /devices/LNXSYSTM:00/device:00/PNP0C0A:00/power_supply/BAT1
(power_supply)
ACTION=change
DEVPATH=/devices/LNXSYSTM:00/device:00/PNP0C0A:00/power_supply/BAT1
SUBSYSTEM=power_supply
POWER_SUPPLY_NAME=BAT1
POWER_SUPPLY_TYPE=Battery
POWER_SUPPLY_STATUS=Full
POWER_SUPPLY_PRESENT=1
POWER_SUPPLY_TECHNOLOGY=Unknown
POWER_SUPPLY_VOLTAGE_MIN_DESIGN=10800000
POWER_SUPPLY_VOLTAGE_NOW=0
POWER_SUPPLY_CURRENT_NOW=0
POWER_SUPPLY_ENERGY_FULL_DESIGN=38880000
POWER_SUPPLY_ENERGY_FULL=37530000
POWER_SUPPLY_ENERGY_NOW=0
POWER_SUPPLY_MODEL_NAME=XM2038P04
POWER_SUPPLY_MANUFACTURER=
SEQNUM=3246

physically remove battery

UEVENT[1193556483.326303]
remove /devices/LNXSYSTM:00/device:00/PNP0C0A:00/power_supply/BAT1
(power_supply)
ACTION=remove
DEVPATH=/devices/LNXSYSTM:00/device:00/PNP0C0A:00/power_supply/BAT1
SUBSYSTEM=power_supply
POWER_SUPPLY_NAME=BAT1
POWER_SUPPLY_TYPE=Battery
POWER_SUPPLY_PRESENT=0
SEQNUM=3252

battery back

UEVENT[1193556510.339045]
add /devices/LNXSYSTM:00/device:00/PNP0C0A:00/power_supply/BAT1
(power_supply)
ACTION=add
DEVPATH=/devices/LNXSYSTM:00/device:00/PNP0C0A:00/power_supply/BAT1
SUBSYSTEM=power_supply
SEQNUM=3253

UEVENT[1193556510.339387]
change /devices/LNXSYSTM:00/device:00/PNP0C0A:00/power_supply/BAT1
(power_supply)
ACTION=change
DEVPATH=/devices/LNXSYSTM:00/device:00/PNP0C0A:00/power_supply/BAT1
SUBSYSTEM=power_supply
POWER_SUPPLY_NAME=BAT1
POWER_SUPPLY_TYPE=Battery
POWER_SUPPLY_STATUS=Charging
POWER_SUPPLY_PRESENT=1
POWER_SUPPLY_TECHNOLOGY=Unknown
POWER_SUPPLY_VOLTAGE_MIN_DESIGN=10800000
POWER_SUPPLY_VOLTAGE_NOW=11340000
POWER_SUPPLY_CURRENT_NOW=13197000
POWER_SUPPLY_ENERGY_FULL_DESIGN=38880000
POWER_SUPPLY_ENERGY_FULL=37530000
POWER_SUPPLY_ENERGY_NOW=37519000
POWER_SUPPLY_MODEL_NAME=XM2038P04
POWER_SUPPLY_MANUFACTURER=
SEQNUM=3254

We'll probably need to teach user space to distinguish between battery and
battery bay (and not to crash when battery is removed :) ) but that is all
doable once we settle on kernel implementation.
Subject: [PATCH] [2.6.24-rc] ACPI: register power_supply subdevice only when battery is present
From: Andrey Borzenkov <arvidjaar@xxxxxxx>

Make sure no power_supply object is present unless we actualy detect
presence of battery. This fixes ghost batteries detected by HAL

Signed-off-by: Andrey Borzenkov <arvidjaar@xxxxxxx>

---

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

diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c
index d33cb49..f37e509 100644
--- a/drivers/acpi/battery.c
+++ b/drivers/acpi/battery.c
@@ -390,14 +390,64 @@ static int acpi_battery_init_alarm(struct acpi_battery *battery)
return acpi_battery_set_alarm(battery);
}

+static ssize_t acpi_battery_alarm_show(struct device *dev,
+ struct device_attribute *attr,
+ char *buf)
+{
+ struct acpi_battery *battery = to_acpi_battery(dev_get_drvdata(dev));
+ return sprintf(buf, "%d\n", battery->alarm * 1000);
+}
+
+static ssize_t acpi_battery_alarm_store(struct device *dev,
+ struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ unsigned long x;
+ struct acpi_battery *battery = to_acpi_battery(dev_get_drvdata(dev));
+ if (sscanf(buf, "%ld\n", &x) == 1)
+ battery->alarm = x/1000;
+ if (acpi_battery_present(battery))
+ acpi_battery_set_alarm(battery);
+ return count;
+}
+
+static struct device_attribute alarm_attr = {
+ .attr = {.name = "alarm", .mode = 0644, .owner = THIS_MODULE},
+ .show = acpi_battery_alarm_show,
+ .store = acpi_battery_alarm_store,
+};
+
+static int sysfs_add_battery(struct acpi_battery *battery)
+{
+ int result = 0;
+
+ result = power_supply_register(&battery->device->dev, &battery->bat);
+ result = device_create_file(battery->bat.dev, &alarm_attr);
+
+ return result;
+}
+
+static int sysfs_remove_battery(struct acpi_battery *battery)
+{
+ if (battery->bat.dev) {
+ device_remove_file(battery->bat.dev, &alarm_attr);
+ power_supply_unregister(&battery->bat);
+ battery->bat.dev = NULL;
+ }
+
+ return 0;
+}
+
static int acpi_battery_update(struct acpi_battery *battery)
{
- int saved_present = acpi_battery_present(battery);
int result = acpi_battery_get_status(battery);
- if (result || !acpi_battery_present(battery))
+ if (result)
return result;
- if (saved_present != acpi_battery_present(battery) ||
- !battery->update_time) {
+ if (!acpi_battery_present(battery)) {
+ sysfs_remove_battery(battery);
+ return 0;
+ }
+ if (!battery->bat.dev) {
battery->update_time = 0;
result = acpi_battery_get_info(battery);
if (result)
@@ -411,6 +461,7 @@ static int acpi_battery_update(struct acpi_battery *battery)
battery->bat.num_properties =
ARRAY_SIZE(energy_battery_props);
}
+ sysfs_add_battery(battery);
acpi_battery_init_alarm(battery);
}
return acpi_battery_get_state(battery);
@@ -693,33 +744,6 @@ static void acpi_battery_remove_fs(struct acpi_device *device)

#endif

-static ssize_t acpi_battery_alarm_show(struct device *dev,
- struct device_attribute *attr,
- char *buf)
-{
- struct acpi_battery *battery = to_acpi_battery(dev_get_drvdata(dev));
- return sprintf(buf, "%d\n", battery->alarm * 1000);
-}
-
-static ssize_t acpi_battery_alarm_store(struct device *dev,
- struct device_attribute *attr,
- const char *buf, size_t count)
-{
- unsigned long x;
- struct acpi_battery *battery = to_acpi_battery(dev_get_drvdata(dev));
- if (sscanf(buf, "%ld\n", &x) == 1)
- battery->alarm = x/1000;
- if (acpi_battery_present(battery))
- acpi_battery_set_alarm(battery);
- return count;
-}
-
-static struct device_attribute alarm_attr = {
- .attr = {.name = "alarm", .mode = 0644, .owner = THIS_MODULE},
- .show = acpi_battery_alarm_show,
- .store = acpi_battery_alarm_store,
-};
-
/* --------------------------------------------------------------------------
Driver Interface
-------------------------------------------------------------------------- */
@@ -737,7 +761,9 @@ static void acpi_battery_notify(acpi_handle handle, u32 event, void *data)
acpi_bus_generate_netlink_event(device->pnp.device_class,
device->dev.bus_id, event,
acpi_battery_present(battery));
- kobject_uevent(&battery->bat.dev->kobj, KOBJ_CHANGE);
+ /* acpi_batter_update could remove power_supply object */
+ if (battery->bat.dev)
+ kobject_uevent(&battery->bat.dev->kobj, KOBJ_CHANGE);
}

static int acpi_battery_add(struct acpi_device *device)
@@ -755,17 +781,15 @@ static int acpi_battery_add(struct acpi_device *device)
strcpy(acpi_device_class(device), ACPI_BATTERY_CLASS);
acpi_driver_data(device) = battery;
mutex_init(&battery->lock);
+ battery->bat.name = acpi_device_bid(device);
+ battery->bat.type = POWER_SUPPLY_TYPE_BATTERY;
+ battery->bat.get_property = acpi_battery_get_property;
acpi_battery_update(battery);
#ifdef CONFIG_ACPI_PROCFS
result = acpi_battery_add_fs(device);
if (result)
goto end;
#endif
- battery->bat.name = acpi_device_bid(device);
- battery->bat.type = POWER_SUPPLY_TYPE_BATTERY;
- battery->bat.get_property = acpi_battery_get_property;
- result = power_supply_register(&battery->device->dev, &battery->bat);
- result = device_create_file(battery->bat.dev, &alarm_attr);
status = acpi_install_notify_handler(device->handle,
ACPI_ALL_NOTIFY,
acpi_battery_notify, battery);

Attachment: signature.asc
Description: This is a digitally signed message part.

Next message: WANG Cong: "[Git Patch] arch/um/os-Linux/: varied improvements and fixes"
Previous message: Amit Shah: "Re: sysfs: WARNING: at fs/sysfs/dir.c:424 sysfs_add_one()"
In reply to: Andrey Borzenkov: "Re: [PATCH] 2.6.24-rc1: ensure "present" sysfs attribute even if battery is absent"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]