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

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

[Andre Przywara]

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.

Cheers,
Andre.

 drivers/pinctrl/core.c   | 5 +++--
 drivers/pinctrl/pinmux.c | 5 +++--
 2 files changed, 6 insertions(+), 4 deletions(-)

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 pinctrl_dev *pctldev)
 	void **slot;
 	int i = 0;
 
-	indices = devm_kzalloc(pctldev->dev, sizeof(*indices) *
-			       pctldev->num_groups, GFP_KERNEL);
+	indices = kcalloc(pctldev->num_groups, sizeof(*indices),
+			  GFP_KERNEL);
 	if (!indices)
 		return;
 
@@ -704,6 +704,7 @@ static void pinctrl_generic_free_groups(struct pinctrl_dev *pctldev)
 		radix_tree_delete(&pctldev->pin_group_tree, indices[i]);
 		devm_kfree(pctldev->dev, group);
 	}
+	kfree(indices);
 
 	pctldev->num_groups = 0;
 }
diff --git a/drivers/pinctrl/pinmux.c b/drivers/pinctrl/pinmux.c
index 29ad315..6a70339 100644
--- a/drivers/pinctrl/pinmux.c
+++ b/drivers/pinctrl/pinmux.c
@@ -836,8 +836,8 @@ void pinmux_generic_free_functions(struct pinctrl_dev *pctldev)
 	void **slot;
 	int i = 0;
 
-	indices = devm_kzalloc(pctldev->dev, sizeof(*indices) *
-			       pctldev->num_functions, GFP_KERNEL);
+	indices = kcalloc(pctldev->num_functions, sizeof(*indices),
+			  GFP_KERNEL);
 	if (!indices)
 		return;
 
@@ -850,6 +850,7 @@ void pinmux_generic_free_functions(struct pinctrl_dev *pctldev)
 		radix_tree_delete(&pctldev->pin_function_tree, indices[i]);
 		devm_kfree(pctldev->dev, function);
 	}
+	kfree(indices);
 
 	pctldev->num_functions = 0;
 }
-- 
2.8.2

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

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 3690 bytes --]

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_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.
> 
> Cheers,
> Andre.
> 
>  drivers/pinctrl/core.c   | 5 +++--
>  drivers/pinctrl/pinmux.c | 5 +++--
>  2 files changed, 6 insertions(+), 4 deletions(-)
> 
> 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 pinctrl_dev *pctldev)
>  	void **slot;
>  	int i = 0;
>  
> -	indices = devm_kzalloc(pctldev->dev, sizeof(*indices) *
> -			       pctldev->num_groups, GFP_KERNEL);
> +	indices = kcalloc(pctldev->num_groups, sizeof(*indices),
> +			  GFP_KERNEL);
>  	if (!indices)
>  		return;
>  
> @@ -704,6 +704,7 @@ static void pinctrl_generic_free_groups(struct pinctrl_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

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

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

[Tejun Heo]

Hello,

On Thu, May 04, 2017 at 02:03:14PM +0200, Maxime Ripard wrote:
> > @@ -704,6 +704,7 @@ static void pinctrl_generic_free_groups(struct pinctrl_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.

It doesn't make any sense to use the managed functions from the
release functions and if you're always matching devm_kmalloc() with
devm_kfree(), the only thing it'd do is confusing its readers.

And the function in question just looks weird to me - give up on
freeing resources if memory allocation fails?  And why call
devm_kfree() on objects which are being released anyway?  Or if the
group can be released w/o the device being detached, why use devm
allocations on the members at all?

As for removal, can't it just call radix_tree_iter_delete() while
iterating?  Why allocate memory to iterate and store all indices and
to look them up again and delete them?

Thanks.

-- 
tejun

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

[André Przywara]

On 04/05/17 17:00, Tejun Heo wrote:

Hi,

> Hello,
> 
> On Thu, May 04, 2017 at 02:03:14PM +0200, Maxime Ripard wrote:
>>> @@ -704,6 +704,7 @@ static void pinctrl_generic_free_groups(struct pinctrl_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.
> 
> It doesn't make any sense to use the managed functions from the
> release functions and if you're always matching devm_kmalloc() with
> devm_kfree(), the only thing it'd do is confusing its readers.

I agree, the idea of this patch is to get rid of managed functions. Also
my understanding is that devm_kfree needs to match some devm_kmalloc().

> And the function in question just looks weird to me - give up on
> freeing resources if memory allocation fails?  And why call
> devm_kfree() on objects which are being released anyway?  Or if the
> group can be released w/o the device being detached, why use devm
> allocations on the members at all?
> 
> As for removal, can't it just call radix_tree_iter_delete() while
> iterating?  Why allocate memory to iterate and store all indices and
> to look them up again and delete them?

Yeah, I was wondering about this as well. It looks like it just wants to
clean up everything; the implementation of ida_destroy() seems to have
the same intention and uses radix_tree_iter_delete().

Using this algorithm would avoid the memory allocation in the first
place, so fix the issue as well and simplify the code.

So is there anything we miss here why we store all indices and iterate
twice over the tree?

Cheers,
Andre.

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

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1236 bytes --]

On Thu, May 04, 2017 at 12:00:32PM -0400, Tejun Heo wrote:
> Hello,
> 
> On Thu, May 04, 2017 at 02:03:14PM +0200, Maxime Ripard wrote:
> > > @@ -704,6 +704,7 @@ static void pinctrl_generic_free_groups(struct pinctrl_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.
> 
> It doesn't make any sense to use the managed functions from the
> release functions and if you're always matching devm_kmalloc() with
> devm_kfree(), the only thing it'd do is confusing its readers.

I wouldn't say that being able to recover and free whatever memory
leak we might have not making sense, but ok. That was one of the
options, let's discard it.

The other one is: refactor the rest of the allocations so that you
don't have a mix of devm_kmalloc / devm_kfree and kmalloc / kfree for
the same purpose in the same function.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

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

[Tejun Heo]

Hello, Maxime.

On Fri, May 05, 2017 at 09:55:18PM +0200, Maxime Ripard wrote:
> > It doesn't make any sense to use the managed functions from the
> > release functions and if you're always matching devm_kmalloc() with
> > devm_kfree(), the only thing it'd do is confusing its readers.
> 
> I wouldn't say that being able to recover and free whatever memory
> leak we might have not making sense, but ok. That was one of the
> options, let's discard it.
>
> The other one is: refactor the rest of the allocations so that you
> don't have a mix of devm_kmalloc / devm_kfree and kmalloc / kfree for
> the same purpose in the same function.

So, I think the issues here are

1. Use of devm functions in a devres release function.  This just
   doesn't make sense.

2. If a resource is always allocated and freed in the same function,
   there's no reason to use devres for it.  I understand that there
   can be exceptions for this, e.g. consistency, but it doesn't seem
   to apply here.

3. Last but not least, allocating memory to destroy a radix tree.  It
   should be able to do that without allocating any memory.

Thanks.

-- 
tejun

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

[Linus Walleij]

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?

Yours,
Linus Walleij

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

[Icenowy Zheng]

于 2017年5月11日 GMT+08:00 下午10:01:54, Linus Walleij <linus.walleij@linaro.org> 写到:
>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.

I think 4.11 has also this issue.

>
>Tejun, do I read your comments on the patch as an ACK?
>
>Yours,
>Linus Walleij

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

[Andre Przywara]

Hi Linus,

On 11/05/17 15:01, Linus Walleij 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)

Linus, can you shed some light if this array creation serves some purpose?

Cheers,
Andre.

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

[Tejun Heo]

Hello,

On Thu, May 11, 2017 at 03:20:58PM +0100, Andre Przywara wrote:
> > 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)

Yeah, it doesn't make sense to allocate to destroy a radix tree.  It'd
be much better to cleanup the code so that it doesn't need allocation
in the first place.

Thanks.

-- 
tejun

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

[Linus Walleij]

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

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

[Tony Lindgren]

* 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

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

[Tony Lindgren]

* 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

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

[André Przywara]

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;
>  }
> 

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

[Linus Walleij]

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