From mboxrd@z Thu Jan 1 00:00:00 1970 From: Sebastian Reichel Subject: Re: [PATCH v4 11/20] power_supply: Change ownership from driver to core Date: Thu, 26 Feb 2015 01:45:22 +0100 Message-ID: <20150226004522.GB885@earth> References: <1424692061-30624-1-git-send-email-k.kozlowski@samsung.com> <1424692061-30624-12-git-send-email-k.kozlowski@samsung.com> Mime-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="3uo+9/B/ebqu+fSQ" Return-path: Content-Disposition: inline In-Reply-To: <1424692061-30624-12-git-send-email-k.kozlowski@samsung.com> Sender: linux-kernel-owner@vger.kernel.org To: Krzysztof Kozlowski Cc: Dmitry Eremin-Solenikov , David Woodhouse , linux-pm@vger.kernel.org, linux-kernel@vger.kernel.org, "Rafael J. Wysocki" , Len Brown , Jiri Kosina , David Herrmann , Cezary Jackiewicz , Darren Hart , Support Opensource , Milo Kim , Julian Andres Klode , Marc Dietrich , Greg Kroah-Hartman , linux-acpi@vger.kernel.org, linux-input@vger.kernel.org, platform-driver-x86@vger.kernel.org, patches@opensource.wolfsonmicro.com, ac100@lists.launchpad.net, linux-tegra@vger.kernel.org, devel@driverdev.osuosl.org, Linus Walleij , Samuel Ortiz , Lee List-Id: linux-tegra@vger.kernel.org --3uo+9/B/ebqu+fSQ Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable 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. >=20 > The patch changes power_supply_register() function thus all drivers > implementing power supply class are adjusted. >=20 > 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. >=20 > 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. >=20 > This solves invalid memory references in following race condition > scenario: >=20 > Thread 1: charger manager > Thread 2: power supply driver, used by charger manager >=20 > THREAD 1 (charger manager) THREAD 2 (power supply driver) > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D > psy =3D power_supply_get_by_name() > Driver unbind, .remove > power_supply_unregister() > Device fully removed > psy->get_property() >=20 > The 'get_property' call is executed in invalid context because the driver= was > unbound and struct 'power_supply' memory was freed. >=20 > This could be observed easily with charger manager driver (here compiled > with max17040 fuel gauge): >=20 > $ 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 virtua= l address 00000000 > [ 55.732584] pgd =3D d98d4000 > [ 55.734060] [00000000] *pgd=3D5afa2831, *pte=3D00000000, *ppte=3D00000= 000 > [ 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 Segme= nt user > [ 55.815994] Control: 10c5387d Table: 598d406a DAC: 00000015 > [ 55.821723] Process cat (pid: 2936, stack limit =3D 0xdaf54210) > [ 55.827451] Stack: (0xdaf55e90 to 0xdaf56000) > [ 55.831795] 5e80: 60000013 c01459c= 4 0000002a c06f8ef8 > [ 55.839956] 5ea0: db651000 c06f8ef8 daebac00 c04cb668 daebac08 c034686= 4 00000000 c01459c4 > [ 55.848115] 5ec0: d99eaa80 c06f8ef8 00000fff 00001000 db651000 c027f25= c c027f240 d99eaa80 > [ 55.856274] 5ee0: d9a06c00 c0146218 daf55f18 00001000 d99eaa80 db4c18c= 0 00000001 00000001 > [ 55.864468] 5f00: daf55f80 c0144c78 c0144c54 c0107f90 00015000 d99eaab= 0 00000000 00000000 > [ 55.872603] 5f20: 000051c7 00000000 db4c18c0 c04a9370 00015000 0000100= 0 daf55f80 00001000 > [ 55.880763] 5f40: daf54000 00015000 00000000 c00e53dc db4c18c0 c00e548= c 0000000d 00008124 > [ 55.888937] 5f60: 00000001 00000000 00000000 db4c18c0 db4c18c0 0000100= 0 00015000 c00e5550 > [ 55.897099] 5f80: 00000000 00000000 00001000 00001000 00015000 0000000= 3 00000003 c000f364 > [ 55.905239] 5fa0: 00000000 c000f1a0 00001000 00015000 00000003 0001500= 0 00001000 0001333c > [ 55.913399] 5fc0: 00001000 00015000 00000003 00000003 00000002 0000000= 0 00000000 00000000 > [ 55.921560] 5fe0: 7fffe000 be999850 0000a225 b6f3c19c 60000010 0000000= 3 00000000 00000000 > [ 55.929744] [] (charger_get_property) from [] (pow= er_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_s= eq_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/0= x8c) > [ 55.993164] [] (SyS_read) from [] (ret_fast_syscal= l+0x0/0x48) > [ 56.000626] Code: bad PC value > [ 56.011652] ---[ end trace 7b64343fbdae8ef1 ]--- >=20 > Signed-off-by: Krzysztof Kozlowski > Reviewed-by: Bartlomiej Zolnierkiewicz >=20 > [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 -- Sebastian --3uo+9/B/ebqu+fSQ Content-Type: application/pgp-signature; name="signature.asc" Content-Description: Digital signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBCgAGBQJU7myfAAoJENju1/PIO/qaFmkQAIFRUAvn/d9O/g5qiUAegRFo txL+BJeAfIaHOZM011AzdIxFUdYP+Xpwz52BgttkFL892o6fHUiC5OsdFHeCOP96 8xNKLhIdQvH676Zu0yxGPC12cdFPUC89WqqtWpI4GmLl7bMR+j2LLpc0p1FWwn1n BTo+gxP2rCYpHSXjP9xbj1CMr+oESzgTVhW4zPzgSi5pBfQyVvB1s/U8jFXkKqoO yBv3SHYbLfpQlC7zJ/EIy7gDOBRIxGwq+D93JpNd5VmJduUygOzmeSDGpT+Vhf08 tnxzzzRfnoThd2g5XkixMIZ+UnQFH2sVR83WbZvKNc3r7ruo0CrmAdMVsrYNobfa gJ6HQ+wuAYyXsfdj7Knu+t6g5wxteflYmGyrVhrS9SFwSdd7i3k5uyzHiIRSO3dW WCSQA0nxF08s+FZ/YQqFShwsFbsBwQVYcNwKDi0znUi4Kf4N8T7zV4ZK5+xfNZIX VF/K0EWsO6kMXpnmli1n8Bt96U8L1E/zd/PZVHhwjsGqK4dglpRrjXKycGPN9RO9 Im8jfR/pNQi0SdB0vD/Ol5fo5bOr32PYhBKycQ23NtxVPBILa5JD1ICRxlUD5ysS ESoMvDp1O53xYgjMGMwgWlgW23vH5af9SgosfmGSrCNMewTrgLBbv5qcpIeQbcSW rOFYeCvolMKkfuM6jhXZ =vQht -----END PGP SIGNATURE----- --3uo+9/B/ebqu+fSQ--