All of lore.kernel.org
 help / color / mirror / Atom feed
From: tony@atomide.com (Tony Lindgren)
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH] pinctrl: use non-devm kmalloc versions for free functions
Date: Fri, 12 May 2017 10:14:17 -0700	[thread overview]
Message-ID: <20170512171417.GH3489@atomide.com> (raw)
In-Reply-To: <20170512153522.GG3489@atomide.com>

* 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

WARNING: multiple messages have this Message-ID (diff)
From: Tony Lindgren <tony@atomide.com>
To: Linus Walleij <linus.walleij@linaro.org>
Cc: Adam Borowski <kilobyte@angband.pl>,
	Andre Przywara <andre.przywara@arm.com>,
	linux-sunxi <linux-sunxi@googlegroups.com>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	Chen-Yu Tsai <wens@csie.org>, Icenowy Zheng <icenowy@aosc.xyz>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Tejun Heo <tj@kernel.org>,
	Maxime Ripard <maxime.ripard@free-electrons.com>,
	"linux-arm-kernel@lists.infradead.org" 
	<linux-arm-kernel@lists.infradead.org>
Subject: Re: [PATCH] pinctrl: use non-devm kmalloc versions for free functions
Date: Fri, 12 May 2017 10:14:17 -0700	[thread overview]
Message-ID: <20170512171417.GH3489@atomide.com> (raw)
In-Reply-To: <20170512153522.GG3489@atomide.com>

* 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

  reply	other threads:[~2017-05-12 17:14 UTC|newest]

Thread overview: 30+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-05-03 23:57 [PATCH] pinctrl: use non-devm kmalloc versions for free functions Andre Przywara
2017-05-03 23:57 ` Andre Przywara
2017-05-04 12:03 ` Maxime Ripard
2017-05-04 12:03   ` Maxime Ripard
2017-05-04 16:00   ` Tejun Heo
2017-05-04 16:00     ` Tejun Heo
2017-05-05  0:41     ` André Przywara
2017-05-05  0:41       ` André Przywara
2017-05-05 19:55     ` Maxime Ripard
2017-05-05 19:55       ` Maxime Ripard
2017-05-05 21:49       ` Tejun Heo
2017-05-05 21:49         ` Tejun Heo
2017-05-11 14:01 ` Linus Walleij
2017-05-11 14:01   ` Linus Walleij
2017-05-11 14:02   ` [linux-sunxi] " Icenowy Zheng
2017-05-11 14:02     ` Icenowy Zheng
2017-05-11 14:20   ` Andre Przywara
2017-05-11 14:20     ` Andre Przywara
2017-05-11 14:45     ` Tejun Heo
2017-05-11 14:45       ` Tejun Heo
2017-05-12  9:25     ` Linus Walleij
2017-05-12  9:25       ` Linus Walleij
2017-05-12 15:35       ` Tony Lindgren
2017-05-12 15:35         ` Tony Lindgren
2017-05-12 17:14         ` Tony Lindgren [this message]
2017-05-12 17:14           ` Tony Lindgren
2017-05-13  0:24           ` André Przywara
2017-05-13  0:24             ` André Przywara
2017-05-22 15:37           ` Linus Walleij
2017-05-22 15:37             ` Linus Walleij

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20170512171417.GH3489@atomide.com \
    --to=tony@atomide.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.