From mboxrd@z Thu Jan 1 00:00:00 1970 From: Darren Hart Subject: Re: [PATCH v4 11/20] power_supply: Change ownership from driver to core Date: Wed, 25 Feb 2015 22:14:38 -0800 Message-ID: <20150226061438.GB7331@fury.dvhart.com> References: <1424692061-30624-1-git-send-email-k.kozlowski@samsung.com> <1424692061-30624-12-git-send-email-k.kozlowski@samsung.com> <20150226004522.GB885@earth> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Return-path: Content-Disposition: inline In-Reply-To: <20150226004522.GB885@earth> Sender: linux-tegra-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: Sebastian Reichel Cc: Krzysztof Kozlowski , Dmitry Eremin-Solenikov , David Woodhouse , linux-pm-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, "Rafael J. Wysocki" , Len Brown , Jiri Kosina , David Herrmann , Cezary Jackiewicz , Support Opensource , Milo Kim , Julian Andres Klode , Marc Dietrich , Greg Kroah-Hartman , linux-acpi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-input-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, platform-driver-x86-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, patches-yzvPICuk2AATkU/dhu1WVueM+bqZidxxQQ4Iyu8u01E@public.gmane.org, ac100-oU9gvf+ajcQ97yFScArB1dHuzzzSOjJt@public.gmane.org, linux-tegra-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, devel-gWbeCf7V1WCQmaza687I9mD2FQJk+8+b@public.gmane.org, Linus Walleij , Samuel Ortiz List-Id: linux-pm@vger.kernel.org On Thu, Feb 26, 2015 at 01:45:22AM +0100, Sebastian Reichel wrote: > Hi, > > On Mon, Feb 23, 2015 at 12:47:32PM +0100, Krzysztof Kozlowski wrote: > > Change the ownership of power_supply structure from each driver > > implementing the class to the power supply core. > > > > The patch changes power_supply_register() function thus all drivers > > implementing power supply class are adjusted. > > > > Each driver provides the implementation of power supply. However it > > should not be the owner of power supply class instance because it is > > exposed by core to other subsystems with power_supply_get_by_name(). > > These other subsystems have no knowledge when the driver will unregister > > the power supply. This leads to several issues when driver is unbound - > > mostly because user of power supply accesses freed memory. > > > > Instead let the core own the instance of struct 'power_supply'. Other > > users of this power supply will still access valid memory because it > > will be freed when device reference count reaches 0. Currently this > > means "it will leak" but power_supply_put() call in next patches will > > solve it. > > > > This solves invalid memory references in following race condition > > scenario: > > > > Thread 1: charger manager > > Thread 2: power supply driver, used by charger manager > > > > THREAD 1 (charger manager) THREAD 2 (power supply driver) > > ========================== ============================== > > psy = power_supply_get_by_name() > > Driver unbind, .remove > > power_supply_unregister() > > Device fully removed > > psy->get_property() > > > > The 'get_property' call is executed in invalid context because the driver was > > unbound and struct 'power_supply' memory was freed. > > > > This could be observed easily with charger manager driver (here compiled > > with max17040 fuel gauge): > > > > $ cat /sys/devices/virtual/power_supply/cm-battery/capacity & > > $ echo "1-0036" > /sys/bus/i2c/drivers/max17040/unbind > > [ 55.725123] Unable to handle kernel NULL pointer dereference at virtual address 00000000 > > [ 55.732584] pgd = d98d4000 > > [ 55.734060] [00000000] *pgd=5afa2831, *pte=00000000, *ppte=00000000 > > [ 55.740318] Internal error: Oops: 80000007 [#1] PREEMPT SMP ARM > > [ 55.746210] Modules linked in: > > [ 55.749259] CPU: 1 PID: 2936 Comm: cat Tainted: G W 3.19.0-rc1-next-20141226-00048-gf79f475f3c44-dirty #1496 > > [ 55.760190] Hardware name: SAMSUNG EXYNOS (Flattened Device Tree) > > [ 55.766270] task: d9b76f00 ti: daf54000 task.ti: daf54000 > > [ 55.771647] PC is at 0x0 > > [ 55.774182] LR is at charger_get_property+0x2f4/0x36c > > [ 55.779201] pc : [<00000000>] lr : [] psr: 60000013 > > [ 55.779201] sp : daf55e90 ip : 00000003 fp : 00000000 > > [ 55.790657] r10: 00000000 r9 : c06e2878 r8 : d9b26c68 > > [ 55.795865] r7 : dad81610 r6 : daec7410 r5 : daf55ebc r4 : 00000000 > > [ 55.802367] r3 : 00000000 r2 : daf55ebc r1 : 0000002a r0 : d9b26c68 > > [ 55.808879] Flags: nZCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment user > > [ 55.815994] Control: 10c5387d Table: 598d406a DAC: 00000015 > > [ 55.821723] Process cat (pid: 2936, stack limit = 0xdaf54210) > > [ 55.827451] Stack: (0xdaf55e90 to 0xdaf56000) > > [ 55.831795] 5e80: 60000013 c01459c4 0000002a c06f8ef8 > > [ 55.839956] 5ea0: db651000 c06f8ef8 daebac00 c04cb668 daebac08 c0346864 00000000 c01459c4 > > [ 55.848115] 5ec0: d99eaa80 c06f8ef8 00000fff 00001000 db651000 c027f25c c027f240 d99eaa80 > > [ 55.856274] 5ee0: d9a06c00 c0146218 daf55f18 00001000 d99eaa80 db4c18c0 00000001 00000001 > > [ 55.864468] 5f00: daf55f80 c0144c78 c0144c54 c0107f90 00015000 d99eaab0 00000000 00000000 > > [ 55.872603] 5f20: 000051c7 00000000 db4c18c0 c04a9370 00015000 00001000 daf55f80 00001000 > > [ 55.880763] 5f40: daf54000 00015000 00000000 c00e53dc db4c18c0 c00e548c 0000000d 00008124 > > [ 55.888937] 5f60: 00000001 00000000 00000000 db4c18c0 db4c18c0 00001000 00015000 c00e5550 > > [ 55.897099] 5f80: 00000000 00000000 00001000 00001000 00015000 00000003 00000003 c000f364 > > [ 55.905239] 5fa0: 00000000 c000f1a0 00001000 00015000 00000003 00015000 00001000 0001333c > > [ 55.913399] 5fc0: 00001000 00015000 00000003 00000003 00000002 00000000 00000000 00000000 > > [ 55.921560] 5fe0: 7fffe000 be999850 0000a225 b6f3c19c 60000010 00000003 00000000 00000000 > > [ 55.929744] [] (charger_get_property) from [] (power_supply_show_property+0x48/0x20c) > > [ 55.939286] [] (power_supply_show_property) from [] (dev_attr_show+0x1c/0x48) > > [ 55.948130] [] (dev_attr_show) from [] (sysfs_kf_seq_show+0x84/0x104) > > [ 55.956298] [] (sysfs_kf_seq_show) from [] (kernfs_seq_show+0x24/0x28) > > [ 55.964536] [] (kernfs_seq_show) from [] (seq_read+0x1b0/0x484) > > [ 55.972172] [] (seq_read) from [] (__vfs_read+0x18/0x4c) > > [ 55.979188] [] (__vfs_read) from [] (vfs_read+0x7c/0x100) > > [ 55.986304] [] (vfs_read) from [] (SyS_read+0x40/0x8c) > > [ 55.993164] [] (SyS_read) from [] (ret_fast_syscall+0x0/0x48) > > [ 56.000626] Code: bad PC value > > [ 56.011652] ---[ end trace 7b64343fbdae8ef1 ]--- > > > > Signed-off-by: Krzysztof Kozlowski > > Reviewed-by: Bartlomiej Zolnierkiewicz > > > > [for the nvec part] > > Reviewed-by: Marc Dietrich > > --- > > arch/x86/platform/olpc/olpc-xo1-sci.c | 4 +- > > arch/x86/platform/olpc/olpc-xo15-sci.c | 4 +- > > drivers/acpi/ac.c | 32 +++--- > > drivers/acpi/battery.c | 55 +++++----- > > drivers/acpi/sbs.c | 68 +++++++----- > > drivers/hid/hid-input.c | 51 +++++---- > > drivers/hid/hid-sony.c | 43 ++++---- > > drivers/hid/hid-wiimote-modules.c | 41 +++---- > > drivers/hid/hid-wiimote.h | 3 +- > > drivers/hid/wacom.h | 8 +- > > drivers/hid/wacom_sys.c | 71 ++++++------ > > drivers/platform/x86/compal-laptop.c | 29 +++-- > > drivers/power/[...] | lots of changes > > drivers/staging/nvec/nvec_power.c | 29 +++-- > > include/linux/hid.h | 6 +- > > include/linux/mfd/abx500/ux500_chargalg.h | 11 +- > > include/linux/mfd/rt5033.h | 2 +- > > include/linux/mfd/wm8350/supply.h | 6 +- > > include/linux/power/charger-manager.h | 3 +- > > include/linux/power_supply.h | 39 ++++--- > > 80 files changed, 2172 insertions(+), 1756 deletions(-) > > I would like to merge this via the power supply subsystem. Some of > the files being patched are not under my maintainence, though. It > would be nice if I get an Acked-By from their maintainers. > > As far as I can see this patch is currently missing feedback from: > > arch/x86/platform/olpc: x86 Maintainers > drivers/acpi: Rafael J. Wysocki, Len Brown > drivers/hid: Jiri Kosina > drivers/platform/x86: Darren Hart For compal-laptop.c: Acked-by: Darren Hart -- Darren Hart Intel Open Source Technology Center