Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1640545

Re: [PATCH] pinctrl: use non-devm kmalloc versions for free functions

Show all headers | View raw

* Linus Walleij <linus.walleij@linaro.org> [170512 02:28]:
> On Thu, May 11, 2017 at 4:20 PM, Andre Przywara <andre.przywara@arm.com> wrote:
> >> On Thu, May 4, 2017 at 1:57 AM, Andre Przywara <andre.przywara@arm.com> 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 : [<ffff000008559234>] lr : [<ffff000008559090>] 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 <andre.przywara@arm.com>
> >>>
> >>> [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

Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread

Thread

Re: [PATCH] pinctrl: use non-devm kmalloc versions for free functions Linus Walleij <linus.walleij@linaro.org> - 2017-05-12 11:30 +0200
  Re: [PATCH] pinctrl: use non-devm kmalloc versions for free functions Tony Lindgren <tony@atomide.com> - 2017-05-12 17:40 +0200
    Re: [PATCH] pinctrl: use non-devm kmalloc versions for free functions Tony Lindgren <tony@atomide.com> - 2017-05-12 19:20 +0200
      Re: [PATCH] pinctrl: use non-devm kmalloc versions for free functions André Przywara <andre.przywara@arm.com> - 2017-05-13 02:30 +0200
      Re: [PATCH] pinctrl: use non-devm kmalloc versions for free functions Linus Walleij <linus.walleij@linaro.org> - 2017-05-22 17:50 +0200

csiph-web