From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757272AbdELPf3 (ORCPT ); Fri, 12 May 2017 11:35:29 -0400 Received: from muru.com ([72.249.23.125]:47396 "EHLO muru.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756841AbdELPf2 (ORCPT ); Fri, 12 May 2017 11:35:28 -0400 Date: Fri, 12 May 2017 08:35:23 -0700 From: Tony Lindgren To: Linus Walleij Cc: Andre Przywara , Tejun Heo , Icenowy Zheng , Adam Borowski , Greg Kroah-Hartman , Maxime Ripard , Chen-Yu Tsai , linux-sunxi , "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: <20170512153522.GG3489@atomide.com> References: <1493855857-4453-1-git-send-email-andre.przywara@arm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.8.2 (2017-04-18) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org * Linus Walleij [170512 02:28]: > On Thu, May 11, 2017 at 4:20 PM, Andre Przywara wrote: > >> On Thu, May 4, 2017 at 1:57 AM, 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_probe_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 actually > >>> 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. > >>> > >>> 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]. > >>> > >>> Signed-off-by: Andre Przywara > >>> > >>> [1] https://storage.kernelci.org/net-next/master/v4.11-rc8-2122-gc08bac03d289/arm64/defconfig/lab-baylibre-seattle/boot-sun50i-a64-pine64-plus.html > >>> --- > >>> Hi, > >>> > >>> not sure this is the right fix, I am open to suggestions. > >> > >> I have queued this as a tentative v4.12-rc1 fix, but a bit undertain. > >> > >> Tejun, do I read your comments on the patch as an ACK? > > > > Tejun and I were wondering why we need this "create an array with the > > indices" in the first place. If we can just call radix_tree_delete() > > directly from the radix_tree_for_each_slot() loop, we can have a much > > better fix (omitting the memory allocation at all) > > OK I pulled the patch out again for now. > > > Linus, can you shed some light if this array creation serves some purpose? > > Tony [author of this function] can you look at this? > > The code in pinctrl_generic_free_groups() does look a bit weird, > allocating these indices just to remove the radix tree. > Do you think we can clean it up? Yup indeed it seems totally pointless. Also the same code can be removed from pinmux_generic_free_functions(). It must be left over code from my initial attempts to to add generic pinctrl groups and functions when I still though we need to keep a static array around for the indices to keep pinctrl happy. Then I probably did some robotic compile fixes after updating things to use just the radix tree and added indices locally to both functions.. Regards, Tony