lkml.org 
[lkml]   [2011]   [Jul]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [PATCH 1/3] power_supply: scrub device pointer if registration fails
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@linux.vnet.ibm.com>
> ---
> 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@gmail.com


\
 
 \ /
  Last update: 2011-07-12 17:11    [W:0.136 / U:0.312 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site