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

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

Hi Andre,

On Thu, May 04, 2017 at 12:57:37AM +0100, Andre Przywara wrote:

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20170504/1e19f6b1/attachment.sig>

Hello,

On Thu, May 04, 2017 at 02:03:14PM +0200, Maxime Ripard wrote:

quoted

@@ -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

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

Hi,

Hello,

On Thu, May 04, 2017 at 02:03:14PM +0200, Maxime Ripard wrote:

quoted

quoted

@@ -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.

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:

quoted

quoted

@@ -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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20170505/cf11918c/attachment-0001.sig>

Hello, Maxime.

On Fri, May 05, 2017 at 09:55:18PM +0200, Maxime Ripard wrote:

quoted

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

On Thu, May 4, 2017 at 1:57 AM, Andre Przywara [off-list ref] 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

? 2017?5?11? GMT+08:00 ??10:01:54, Linus Walleij [off-list ref] ??:

On Thu, May 4, 2017 at 1:57 AM, Andre Przywara [off-list ref]
wrote:

quoted

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()

quoted

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

quoted

existing list. This leads to those mallocs being left with the

device,

quoted

which the devres manager complains about when the driver eventually

gets

quoted

probed again:
[    0.825239] ------------[ cut here ]------------
[    0.825256] WARNING: CPU: 1 PID: 89 at drivers/base/dd.c:349

driver_probe_device+0x2ac/0x2e8

quoted

[    0.825258] Modules linked in:
[    0.825262]
[    0.825270] CPU: 1 PID: 89 Comm: kworker/1:1 Not tainted 4.11.0

#307

quoted

[    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

quoted

....
This warning is triggered because the devres list is not empty. In

this

quoted

case the allocations were using 0 bytes, so no real leaks, but still

this

quoted

ugly warning.
Looking more closely at these *cleanup* functions, devm_kzalloc() is

actually

quoted

not needed, because the memory is just allocated temporarily and can

be

quoted

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

quoted

for the Pine64, where the pinctrl driver gets loaded early, but it

missing

quoted

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

quoted

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

Hello,

On Thu, May 11, 2017 at 03:20:58PM +0100, Andre Przywara wrote:

quoted

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

Hi Linus,

On 11/05/17 15:01, Linus Walleij wrote:

On Thu, May 4, 2017 at 1:57 AM, Andre Przywara [off-list ref] wrote:

quoted

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.

On Thu, May 11, 2017 at 4:20 PM, Andre Przywara [off-list ref] wrote:

quoted

On Thu, May 4, 2017 at 1:57 AM, Andre Przywara [off-list ref] wrote:

quoted

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

* Linus Walleij [off-list ref] [170512 02:28]:

On Thu, May 11, 2017 at 4:20 PM, Andre Przywara [off-list ref] wrote:

quoted

quoted

On Thu, May 4, 2017 at 1:57 AM, Andre Przywara [off-list ref] wrote:

quoted

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.

quoted

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

* Tony Lindgren [off-list ref] [170512 08:39]:

* Linus Walleij [off-list ref] [170512 02:28]:

quoted

On Thu, May 11, 2017 at 4:20 PM, Andre Przywara [off-list ref] wrote:

quoted

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 [off-list ref] 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 [off-list ref]. 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

On 12/05/17 18:14, Tony Lindgren wrote:

* Tony Lindgren [off-list ref] [170512 08:39]:

quoted

* Linus Walleij [off-list ref] [170512 02:28]:

quoted

On Thu, May 11, 2017 at 4:20 PM, Andre Przywara [off-list ref] wrote:

quoted

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 [off-list ref] 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 [off-list ref]. 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

On Fri, May 12, 2017 at 7:14 PM, Tony Lindgren [off-list ref] 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 [off-list ref] 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 [off-list ref]. 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