From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752844AbdEDMEA (ORCPT ); Thu, 4 May 2017 08:04:00 -0400 Received: from mail.free-electrons.com ([62.4.15.54]:37588 "EHLO mail.free-electrons.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751991AbdEDMD6 (ORCPT ); Thu, 4 May 2017 08:03:58 -0400 Date: Thu, 4 May 2017 14:03:14 +0200 From: Maxime Ripard To: Andre Przywara Cc: Linus Walleij , Tejun Heo , Icenowy Zheng , Adam Borowski , Greg Kroah-Hartman , Chen-Yu Tsai , linux-sunxi@googlegroups.com, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH] pinctrl: use non-devm kmalloc versions for free functions Message-ID: <20170504120314.lfmcv4cpsuz2igte@lukather> References: <1493855857-4453-1-git-send-email-andre.przywara@arm.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="omjmwxutqcevmafb" Content-Disposition: inline In-Reply-To: <1493855857-4453-1-git-send-email-andre.przywara@arm.com> User-Agent: NeoMutt/20170113 (1.7.2) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org --omjmwxutqcevmafb Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable Hi Andre, On Thu, May 04, 2017 at 12:57:37AM +0100, Andre Przywara wrote: > When a pinctrl driver gets interrupted during its probe process > (returning -EPROBE_DEFER), the devres system cleans up all allocated > resources. During this process it calls pinmux_generic_free_functions() > and pinctrl_generic_free_groups(), which in turn use managed kmalloc > calls for temporarily allocating some memory. Now those calls seem to > get added to the devres list, but are apparently not covered by the > cleanup process, because this is actually just running and iterating the > existing list. This leads to those mallocs being left with the device, > which the devres manager complains about when the driver eventually gets > probed again: > [ 0.825239] ------------[ cut here ]------------ > [ 0.825256] WARNING: CPU: 1 PID: 89 at drivers/base/dd.c:349 driver_pr= obe_device+0x2ac/0x2e8 > [ 0.825258] Modules linked in: > [ 0.825262] > [ 0.825270] CPU: 1 PID: 89 Comm: kworker/1:1 Not tainted 4.11.0 #307 > [ 0.825272] Hardware name: Pine64+ (DT) > [ 0.825283] Workqueue: events deferred_probe_work_func > [ 0.825288] task: ffff80007c19c100 task.stack: ffff80007c16c000 > [ 0.825292] PC is at driver_probe_device+0x2ac/0x2e8 > [ 0.825296] LR is at driver_probe_device+0x108/0x2e8 > [ 0.825300] pc : [] lr : [] pstate= : 20000045 > .... > This warning is triggered because the devres list is not empty. In this > case the allocations were using 0 bytes, so no real leaks, but still this > ugly warning. > Looking more closely at these *cleanup* functions, devm_kzalloc() is actu= ally > not needed, because the memory is just allocated temporarily and can be > freed just before returning from this function. > So fix this issue by using the bog standard kcalloc() call instead of > devm_kzalloc() and kfree()ing the memory at the end. >=20 > This fixes above warnings on boot, which can be observed on *some* builds > for the Pine64, where the pinctrl driver gets loaded early, but it missing > resources, so gets deferred and is loaded again (successfully) later. > kernelci caught this as well [1]. >=20 > Signed-off-by: Andre Przywara >=20 > [1] https://storage.kernelci.org/net-next/master/v4.11-rc8-2122-gc08bac03= d289/arm64/defconfig/lab-baylibre-seattle/boot-sun50i-a64-pine64-plus.html > --- > Hi, >=20 > not sure this is the right fix, I am open to suggestions. >=20 > Cheers, > Andre. >=20 > drivers/pinctrl/core.c | 5 +++-- > drivers/pinctrl/pinmux.c | 5 +++-- > 2 files changed, 6 insertions(+), 4 deletions(-) >=20 > diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c > index 32822b0d..5198415 100644 > --- a/drivers/pinctrl/core.c > +++ b/drivers/pinctrl/core.c > @@ -690,8 +690,8 @@ static void pinctrl_generic_free_groups(struct pinctr= l_dev *pctldev) > void **slot; > int i =3D 0; > =20 > - indices =3D devm_kzalloc(pctldev->dev, sizeof(*indices) * > - pctldev->num_groups, GFP_KERNEL); > + indices =3D kcalloc(pctldev->num_groups, sizeof(*indices), > + GFP_KERNEL); > if (!indices) > return; > =20 > @@ -704,6 +704,7 @@ static void pinctrl_generic_free_groups(struct pinctr= l_dev *pctldev) > radix_tree_delete(&pctldev->pin_group_tree, indices[i]); > devm_kfree(pctldev->dev, group); > } > + kfree(indices); We use devm_kfree for other allocations done here, maybe we can just have the same thing here? We would be consistant, and we would still keep the resource tracking. Maxime --=20 Maxime Ripard, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com --omjmwxutqcevmafb Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIcBAABCAAGBQJZCxh+AAoJEBx+YmzsjxAgIf0P/jdPoJnvOWrprx2iG9RjEfiB oMNXmG6fdK536TArOaFLUZ6LDMQWjjAhRLwEQ7l3DD8tvTjchmGM+cltdb1649Dc cxL6FuAuxDiWkbCy++NxyLvUnDUCp7SLwIT4ZEJN6Dj8VGVBWBjv8aOPjotYIFRE KuuwvAJ9grFq0SABkmgfyTvi4sCFd4WNYVBmGrErkgswI8BCa4ESO+u20amjW5l8 NigFq63/xmZb87h0sxyqilcDVnX+mZwnnLEETW0AVqu4jUe/Gp0NuVlAw3ssv7y4 ixrBJB6btqnTBShzk8sawYLi9npVLUWH/uWjDPHEEcsfaHTUsRWDWMQcG66qhJ4I nzWMxBAE/yLUftajgtHnVwa9smDOwXSNk5JIbTEMrfkvub3T4IuYtNlvTS7F690C 8uAsBq0w+/tgyCBTkiQ3QbYpDKXQLzV4ZLzqy82iDnnMcO3oQq/vVQGSlizEu6R3 k3tUbBlFjWj4SI5wnYTa6spjVWa0FuiC7dg5rePifbqcwylfUMIUxkHNd75zn2VP X5XNBG2SizmMAuZ8Fw6A/WECLqJNvdx36obgaP/jfDjqpKyqK56FA/VT56M2uUSa la0aRdXT0E2EonRZLATIZAqclKom4gRZye7J+ZybbsmHF3R0EApgxJTKRxe51EYZ Wz2aHA2x49E4sbGm7+n7 =5M73 -----END PGP SIGNATURE----- --omjmwxutqcevmafb--