On Tue, Jul 12, 2011 at 07:09:42PM +0400, Anton Vorontsov wrote:
> 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:
Aha! I didn't do this is because I don't know the code and was afraid
some other function somewhere would use psy->dev. If you think it is
safer this way I'll resend the patch.
> 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. :-/
Yeah.
Stefan
--
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
