| Message ID | 20110712150942.GA1291@oksana.dev.rtsoft.ru (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@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: 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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;