On Tue, Jul 12, 2011 at 09:03:27AM +0100, Stefan Hajnoczi wrote:
> This patch makes power_supply_register() safer for callers that are not
> being careful. When the function fails it leaves the caller's psy.dev
> pointer set to the stale power supply device. A correct caller would
> handle the error return and never use psy.dev but the example of
> drivers/acpi/battery.c shows otherwise.
>
> Clear the psy.dev pointer when power_supply_register() fails so the
> caller either sees a valid pointer on success or NULL on failure.
>
> Signed-off-by: Stefan Hajnoczi <stefanha@xxxxxxxxxxxxxxxxxx>
> ---
> drivers/power/power_supply_core.c | 1 +
> 1 files changed, 1 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/power/power_supply_core.c b/drivers/power/power_supply_core.c
> index 329b46b..33d4068 100644
> --- a/drivers/power/power_supply_core.c
> +++ b/drivers/power/power_supply_core.c
> @@ -194,6 +194,7 @@ create_triggers_failed:
> kobject_set_name_failed:
> device_add_failed:
> put_device(dev);
> + psy->dev = NULL; /* make it crystal-clear that we failed */
> success:
> return rc;
> }
I think this may easily cause races. I.e.
- ACPI calls power_supply_register, it allocates dev, sets
psy->dev;
- Someone calls acpi_battery_notify() or acpi_battery_update(),
which tests for psy->dev;
- power_supply_register fails, it frees dev, and then clears psy->dev;
but it's too late, as acpi_battery_notify/acpi_battery_update thinks
that we're fine.
I believe the whole ACPI battery logic is overcomplicated, and
needs a bit of rework. In the meantime, we could move 'psy->dev =
dev;' assignment into the end of the function, where _register
could not fail, i.e. something like this:
diff --git a/drivers/power/power_supply_core.c b/drivers/power/power_supply_core.c
index 329b46b..9f85b70 100644
--- a/drivers/power/power_supply_core.c
+++ b/drivers/power/power_supply_core.c
@@ -169,7 +169,6 @@ int power_supply_register(struct device *parent, struct power_supply *psy)
dev->parent = parent;
dev->release = power_supply_dev_release;
dev_set_drvdata(dev, psy);
- psy->dev = dev;
INIT_WORK(&psy->changed_work, power_supply_changed_work);
@@ -185,6 +184,8 @@ int power_supply_register(struct device *parent, struct power_supply *psy)
if (rc)
goto create_triggers_failed;
+ psy->dev = dev;
+
power_supply_changed(psy);
goto success;
But still, I don't see how this will save us from the same issue
when ACPI calls power_supply_unregister, which doesn't clear psy->dev:
static void acpi_battery_refresh(struct acpi_battery *battery)
{
if (!battery->bat.dev)
return;
acpi_battery_get_info(battery);
/* The battery may have changed its reporting units. */
sysfs_remove_battery(battery);
sysfs_add_battery(battery);
}
Really, ACPI battery needs some proper fixing and locking. :-/
--
Anton Vorontsov
Email: cbouatmailru@xxxxxxxxx
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
