From patchwork Tue Jul 12 15:09:42 2011 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Anton Vorontsov X-Patchwork-Id: 968602 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by demeter1.kernel.org (8.14.4/8.14.4) with ESMTP id p6CF9paA030708 for ; Tue, 12 Jul 2011 15:09:51 GMT Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751082Ab1GLPJs (ORCPT ); Tue, 12 Jul 2011 11:09:48 -0400 Received: from mail-wy0-f174.google.com ([74.125.82.174]:61010 "EHLO mail-wy0-f174.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751072Ab1GLPJr (ORCPT ); Tue, 12 Jul 2011 11:09:47 -0400 Received: by wyg8 with SMTP id 8so96801wyg.19 for ; Tue, 12 Jul 2011 08:09:46 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=date:from:to:cc:subject:message-id:references:mime-version :content-type:content-disposition:in-reply-to:user-agent; bh=1dpNsxtFR1Cs+fB46VoKgJj0lw7usn5HLEwxjLDlPgI=; b=u+vg0A2W2WZHZ4sLNju1WsxW5hRedWjDvq9MFDqIV2A7nnsVm6H3f14KUE3whobLoN 9xo8mz+05g8bo71XwAZjbA6efiKkR84b1yInTGNcZRSZvsLxYh7eeBr1vKndz4gA+drK mmUDcKPhdYBBanzZodThJvvyOo4ah7av+hrYo= Received: by 10.216.56.15 with SMTP id l15mr4106067wec.105.1310483386122; Tue, 12 Jul 2011 08:09:46 -0700 (PDT) Received: from localhost (mail.dev.rtsoft.ru [213.79.90.226]) by mx.google.com with ESMTPS id 53sm2563564wel.31.2011.07.12.08.09.44 (version=TLSv1/SSLv3 cipher=OTHER); Tue, 12 Jul 2011 08:09:45 -0700 (PDT) Date: Tue, 12 Jul 2011 19:09:42 +0400 From: Anton Vorontsov To: Stefan Hajnoczi Cc: Len Brown , David Woodhouse , linux-acpi@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH 1/3] power_supply: scrub device pointer if registration fails Message-ID: <20110712150942.GA1291@oksana.dev.rtsoft.ru> References: <1310457809-2731-1-git-send-email-stefanha@linux.vnet.ibm.com> <1310457809-2731-2-git-send-email-stefanha@linux.vnet.ibm.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <1310457809-2731-2-git-send-email-stefanha@linux.vnet.ibm.com> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-acpi-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-acpi@vger.kernel.org X-Greylist: IP, sender and recipient auto-whitelisted, not delayed by milter-greylist-4.2.6 (demeter1.kernel.org [140.211.167.41]); Tue, 12 Jul 2011 15:09:51 +0000 (UTC) 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 > --- > 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: 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. :-/ 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;