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

Groups > linux.kernel > #1640347 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  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

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

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?

Yours,
Linus Walleij

[toc] | [next] | [standalone]

#1640545

* 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

[toc] | [prev] | [next] | [standalone]

#1640626

* Tony Lindgren <tony@atomide.com> [170512 08:39]:
> * 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:
> > > 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..

Hmm no that, can't be, I think I figured it out.. See the patch
below.

Regards,

Tony

8< ---------------------------
From tony Mon Sep 17 00:00:00 2001
From: Tony Lindgren <tony@atomide.com>
Date: Fri, 12 May 2017 08:47:57 -0700
Subject: [PATCH] pinctrl: core: Fix warning by removing bogus code

Andre Przywara <andre.przywara@arm.com> noticed that we can get the
following warning with -EPROBE_DEFER:

"WARNING: CPU: 1 PID: 89 at drivers/base/dd.c:349
driver_probe_device+0x2ac/0x2e8"

Let's fix the issue by removing the indices as suggested by
Tejun Heo <tj@kernel.org>. All we have to do here is kill the radix
tree.

I probably ended up with the indices after grepping for removal
of all entries using radix_tree_for_each_slot() and the first
match found was gmap_radix_tree_free(). Anyways, no need for
indices here, and we can just do remove all the entries using
radix_tree_for_each_slot() along how the item_kill_tree() test
case does.

Fixes: c7059c5ac70a ("pinctrl: core: Add generic pinctrl functions
for managing groups")
Fixes: a76edc89b100 ("pinctrl: core: Add generic pinctrl functions
for managing groups")
Reported-by: Andre Przywara <andre.przywara@arm.com>
Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 drivers/pinctrl/core.c   | 20 +++-----------------
 drivers/pinctrl/pinmux.c | 21 ++++-----------------
 2 files changed, 7 insertions(+), 34 deletions(-)

diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
--- a/drivers/pinctrl/core.c
+++ b/drivers/pinctrl/core.c
@@ -680,30 +680,16 @@ EXPORT_SYMBOL_GPL(pinctrl_generic_remove_group);
  * pinctrl_generic_free_groups() - removes all pin groups
  * @pctldev: pin controller device
  *
- * Note that the caller must take care of locking.
+ * Note that the caller must take care of locking. The pinctrl groups
+ * are allocated with devm_kzalloc() so no need to free them here.
  */
 static void pinctrl_generic_free_groups(struct pinctrl_dev *pctldev)
 {
 	struct radix_tree_iter iter;
-	struct group_desc *group;
-	unsigned long *indices;
 	void **slot;
-	int i = 0;
-
-	indices = devm_kzalloc(pctldev->dev, sizeof(*indices) *
-			       pctldev->num_groups, GFP_KERNEL);
-	if (!indices)
-		return;
 
 	radix_tree_for_each_slot(slot, &pctldev->pin_group_tree, &iter, 0)
-		indices[i++] = iter.index;
-
-	for (i = 0; i < pctldev->num_groups; i++) {
-		group = radix_tree_lookup(&pctldev->pin_group_tree,
-					  indices[i]);
-		radix_tree_delete(&pctldev->pin_group_tree, indices[i]);
-		devm_kfree(pctldev->dev, group);
-	}
+		radix_tree_delete(&pctldev->pin_group_tree, iter.index);
 
 	pctldev->num_groups = 0;
 }
diff --git a/drivers/pinctrl/pinmux.c b/drivers/pinctrl/pinmux.c
--- a/drivers/pinctrl/pinmux.c
+++ b/drivers/pinctrl/pinmux.c
@@ -826,30 +826,17 @@ EXPORT_SYMBOL_GPL(pinmux_generic_remove_function);
  * pinmux_generic_free_functions() - removes all functions
  * @pctldev: pin controller device
  *
- * Note that the caller must take care of locking.
+ * Note that the caller must take care of locking. The pinctrl
+ * functions are allocated with devm_kzalloc() so no need to free
+ * them here.
  */
 void pinmux_generic_free_functions(struct pinctrl_dev *pctldev)
 {
 	struct radix_tree_iter iter;
-	struct function_desc *function;
-	unsigned long *indices;
 	void **slot;
-	int i = 0;
-
-	indices = devm_kzalloc(pctldev->dev, sizeof(*indices) *
-			       pctldev->num_functions, GFP_KERNEL);
-	if (!indices)
-		return;
 
 	radix_tree_for_each_slot(slot, &pctldev->pin_function_tree, &iter, 0)
-		indices[i++] = iter.index;
-
-	for (i = 0; i < pctldev->num_functions; i++) {
-		function = radix_tree_lookup(&pctldev->pin_function_tree,
-					     indices[i]);
-		radix_tree_delete(&pctldev->pin_function_tree, indices[i]);
-		devm_kfree(pctldev->dev, function);
-	}
+		radix_tree_delete(&pctldev->pin_function_tree, iter.index);
 
 	pctldev->num_functions = 0;
 }
-- 
2.13.0

[toc] | [prev] | [next] | [standalone]

#1640815

On 12/05/17 18:14, Tony Lindgren wrote:
> * Tony Lindgren <tony@atomide.com> [170512 08:39]:
>> * 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:
>>>> 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..
> 
> Hmm no that, can't be, I think I figured it out.. See the patch
> below.
> 
> Regards,
> 
> Tony
> 
> 8< ---------------------------
> From tony Mon Sep 17 00:00:00 2001
> From: Tony Lindgren <tony@atomide.com>
> Date: Fri, 12 May 2017 08:47:57 -0700
> Subject: [PATCH] pinctrl: core: Fix warning by removing bogus code
> 
> Andre Przywara <andre.przywara@arm.com> noticed that we can get the
> following warning with -EPROBE_DEFER:
> 
> "WARNING: CPU: 1 PID: 89 at drivers/base/dd.c:349
> driver_probe_device+0x2ac/0x2e8"
> 
> Let's fix the issue by removing the indices as suggested by
> Tejun Heo <tj@kernel.org>. All we have to do here is kill the radix
> tree.
> 
> I probably ended up with the indices after grepping for removal
> of all entries using radix_tree_for_each_slot() and the first
> match found was gmap_radix_tree_free(). Anyways, no need for
> indices here, and we can just do remove all the entries using
> radix_tree_for_each_slot() along how the item_kill_tree() test
> case does.

Yeah, I was hoping for exactly that!

> Fixes: c7059c5ac70a ("pinctrl: core: Add generic pinctrl functions
> for managing groups")
> Fixes: a76edc89b100 ("pinctrl: core: Add generic pinctrl functions
> for managing groups")
> Reported-by: Andre Przywara <andre.przywara@arm.com>

Reviewed-by: Andre Przywara <andre.przywara@arm.com>
Tested-by: Andre Przywara <andre.przywara@arm.com>

Thanks!
Andre

> Signed-off-by: Tony Lindgren <tony@atomide.com>
> ---
>  drivers/pinctrl/core.c   | 20 +++-----------------
>  drivers/pinctrl/pinmux.c | 21 ++++-----------------
>  2 files changed, 7 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
> --- a/drivers/pinctrl/core.c
> +++ b/drivers/pinctrl/core.c
> @@ -680,30 +680,16 @@ EXPORT_SYMBOL_GPL(pinctrl_generic_remove_group);
>   * pinctrl_generic_free_groups() - removes all pin groups
>   * @pctldev: pin controller device
>   *
> - * Note that the caller must take care of locking.
> + * Note that the caller must take care of locking. The pinctrl groups
> + * are allocated with devm_kzalloc() so no need to free them here.
>   */
>  static void pinctrl_generic_free_groups(struct pinctrl_dev *pctldev)
>  {
>  	struct radix_tree_iter iter;
> -	struct group_desc *group;
> -	unsigned long *indices;
>  	void **slot;
> -	int i = 0;
> -
> -	indices = devm_kzalloc(pctldev->dev, sizeof(*indices) *
> -			       pctldev->num_groups, GFP_KERNEL);
> -	if (!indices)
> -		return;
>  
>  	radix_tree_for_each_slot(slot, &pctldev->pin_group_tree, &iter, 0)
> -		indices[i++] = iter.index;
> -
> -	for (i = 0; i < pctldev->num_groups; i++) {
> -		group = radix_tree_lookup(&pctldev->pin_group_tree,
> -					  indices[i]);
> -		radix_tree_delete(&pctldev->pin_group_tree, indices[i]);
> -		devm_kfree(pctldev->dev, group);
> -	}
> +		radix_tree_delete(&pctldev->pin_group_tree, iter.index);
>  
>  	pctldev->num_groups = 0;
>  }
> diff --git a/drivers/pinctrl/pinmux.c b/drivers/pinctrl/pinmux.c
> --- a/drivers/pinctrl/pinmux.c
> +++ b/drivers/pinctrl/pinmux.c
> @@ -826,30 +826,17 @@ EXPORT_SYMBOL_GPL(pinmux_generic_remove_function);
>   * pinmux_generic_free_functions() - removes all functions
>   * @pctldev: pin controller device
>   *
> - * Note that the caller must take care of locking.
> + * Note that the caller must take care of locking. The pinctrl
> + * functions are allocated with devm_kzalloc() so no need to free
> + * them here.
>   */
>  void pinmux_generic_free_functions(struct pinctrl_dev *pctldev)
>  {
>  	struct radix_tree_iter iter;
> -	struct function_desc *function;
> -	unsigned long *indices;
>  	void **slot;
> -	int i = 0;
> -
> -	indices = devm_kzalloc(pctldev->dev, sizeof(*indices) *
> -			       pctldev->num_functions, GFP_KERNEL);
> -	if (!indices)
> -		return;
>  
>  	radix_tree_for_each_slot(slot, &pctldev->pin_function_tree, &iter, 0)
> -		indices[i++] = iter.index;
> -
> -	for (i = 0; i < pctldev->num_functions; i++) {
> -		function = radix_tree_lookup(&pctldev->pin_function_tree,
> -					     indices[i]);
> -		radix_tree_delete(&pctldev->pin_function_tree, indices[i]);
> -		devm_kfree(pctldev->dev, function);
> -	}
> +		radix_tree_delete(&pctldev->pin_function_tree, iter.index);
>  
>  	pctldev->num_functions = 0;
>  }
> 

[toc] | [prev] | [next] | [standalone]

#1647055

On Fri, May 12, 2017 at 7:14 PM, Tony Lindgren <tony@atomide.com> wrote:

> From tony Mon Sep 17 00:00:00 2001
> From: Tony Lindgren <tony@atomide.com>
> Date: Fri, 12 May 2017 08:47:57 -0700
> Subject: [PATCH] pinctrl: core: Fix warning by removing bogus code
>
> Andre Przywara <andre.przywara@arm.com> noticed that we can get the
> following warning with -EPROBE_DEFER:
>
> "WARNING: CPU: 1 PID: 89 at drivers/base/dd.c:349
> driver_probe_device+0x2ac/0x2e8"
>
> Let's fix the issue by removing the indices as suggested by
> Tejun Heo <tj@kernel.org>. All we have to do here is kill the radix
> tree.
>
> I probably ended up with the indices after grepping for removal
> of all entries using radix_tree_for_each_slot() and the first
> match found was gmap_radix_tree_free(). Anyways, no need for
> indices here, and we can just do remove all the entries using
> radix_tree_for_each_slot() along how the item_kill_tree() test
> case does.
>
> Fixes: c7059c5ac70a ("pinctrl: core: Add generic pinctrl functions
> for managing groups")
> Fixes: a76edc89b100 ("pinctrl: core: Add generic pinctrl functions
> for managing groups")
> Reported-by: Andre Przywara <andre.przywara@arm.com>
> Signed-off-by: Tony Lindgren <tony@atomide.com>

Thanks! This nice inline patch applied for fixes with André's tags.

Yours,
Linus Walleij

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web