All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/2] software node: implement software_node_unregister()
@ 2020-05-24 15:30 Greg Kroah-Hartman
  2020-05-24 15:30 ` [PATCH 2/2] kobject: send KOBJ_REMOVE uevent when the object is removed from sysfs Greg Kroah-Hartman
                   ` (5 more replies)
  0 siblings, 6 replies; 20+ messages in thread
From: Greg Kroah-Hartman @ 2020-05-24 15:30 UTC (permalink / raw)
  To: linux-kernel
  Cc: linux, Greg Kroah-Hartman, Naresh Kamboju, kernel test robot,
	stable, Andy Shevchenko, Brendan Higgins, Dmitry Torokhov,
	Heikki Krogerus, Petr Mladek, Rafael J . Wysocki, Randy Dunlap,
	Rasmus Villemoes, Sakari Ailus, Sergey Senozhatsky,
	Steven Rostedt

Sometimes it is better to unregister individual nodes instead of trying
to do them all at once with software_node_unregister_nodes(), so create
software_node_unregister() so that you can unregister them one at a
time.

This is especially important when creating nodes in a hierarchy, with
parent -> children representations.  Children always need to be removed
before a parent is, as the swnode logic assumes this is going to be the
case.

Fix up the lib/test_printf.c fwnode_pointer() test which to use this new
function as it had the problem of tearing things down in the backwards
order.

Fixes: f1ce39df508d ("lib/test_printf: Add tests for %pfw printk modifier")
Reported-by: Naresh Kamboju <naresh.kamboju@linaro.org>
Reported-by: kernel test robot <rong.a.chen@intel.com>
Cc: stable <stable@vger.kernel.org>
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Brendan Higgins <brendanhiggins@google.com>
Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com>
Cc: Petr Mladek <pmladek@suse.com>
Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Cc: Randy Dunlap <rdunlap@infradead.org>
Cc: Rasmus Villemoes <linux@rasmusvillemoes.dk>
Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
Cc: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
Cc: Steven Rostedt <rostedt@goodmis.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/base/swnode.c    | 27 +++++++++++++++++++++------
 include/linux/property.h |  1 +
 lib/test_printf.c        |  4 +++-
 3 files changed, 25 insertions(+), 7 deletions(-)

diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
index de8d3543e8fe..770b1f47a625 100644
--- a/drivers/base/swnode.c
+++ b/drivers/base/swnode.c
@@ -712,17 +712,18 @@ EXPORT_SYMBOL_GPL(software_node_register_nodes);
  * @nodes: Zero terminated array of software nodes to be unregistered
  *
  * Unregister multiple software nodes at once.
+ *
+ * NOTE: Be careful using this call if the nodes had parent pointers set up in
+ * them before registering.  If so, it is wiser to remove the nodes
+ * individually, in the correct order (child before parent) instead of relying
+ * on the sequential order of the list of nodes in the array.
  */
 void software_node_unregister_nodes(const struct software_node *nodes)
 {
-	struct swnode *swnode;
 	int i;
 
-	for (i = 0; nodes[i].name; i++) {
-		swnode = software_node_to_swnode(&nodes[i]);
-		if (swnode)
-			fwnode_remove_software_node(&swnode->fwnode);
-	}
+	for (i = 0; nodes[i].name; i++)
+		software_node_unregister(&nodes[i]);
 }
 EXPORT_SYMBOL_GPL(software_node_unregister_nodes);
 
@@ -741,6 +742,20 @@ int software_node_register(const struct software_node *node)
 }
 EXPORT_SYMBOL_GPL(software_node_register);
 
+/**
+ * software_node_unregister - Unregister static software node
+ * @node: The software node to be unregistered
+ */
+void software_node_unregister(const struct software_node *node)
+{
+	struct swnode *swnode;
+
+	swnode = software_node_to_swnode(node);
+	if (swnode)
+		fwnode_remove_software_node(&swnode->fwnode);
+}
+EXPORT_SYMBOL_GPL(software_node_unregister);
+
 struct fwnode_handle *
 fwnode_create_software_node(const struct property_entry *properties,
 			    const struct fwnode_handle *parent)
diff --git a/include/linux/property.h b/include/linux/property.h
index d86de017c689..0d4099b4ce1f 100644
--- a/include/linux/property.h
+++ b/include/linux/property.h
@@ -441,6 +441,7 @@ int software_node_register_nodes(const struct software_node *nodes);
 void software_node_unregister_nodes(const struct software_node *nodes);
 
 int software_node_register(const struct software_node *node);
+void software_node_unregister(const struct software_node *node);
 
 int software_node_notify(struct device *dev, unsigned long action);
 
diff --git a/lib/test_printf.c b/lib/test_printf.c
index 6b1622f4d7c2..fc63b8959d42 100644
--- a/lib/test_printf.c
+++ b/lib/test_printf.c
@@ -637,7 +637,9 @@ static void __init fwnode_pointer(void)
 	test(second_name, "%pfwP", software_node_fwnode(&softnodes[1]));
 	test(third_name, "%pfwP", software_node_fwnode(&softnodes[2]));
 
-	software_node_unregister_nodes(softnodes);
+	software_node_unregister(&softnodes[2]);
+	software_node_unregister(&softnodes[1]);
+	software_node_unregister(&softnodes[0]);
 }
 
 static void __init
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH 2/2] kobject: send KOBJ_REMOVE uevent when the object is removed from sysfs
  2020-05-24 15:30 [PATCH 1/2] software node: implement software_node_unregister() Greg Kroah-Hartman
@ 2020-05-24 15:30 ` Greg Kroah-Hartman
  2020-05-25 11:07   ` Rafael J. Wysocki
  2020-05-25 22:49   ` Dmitry Torokhov
  2020-05-24 16:43 ` [PATCH 1/2] software node: implement software_node_unregister() Guenter Roeck
                   ` (4 subsequent siblings)
  5 siblings, 2 replies; 20+ messages in thread
From: Greg Kroah-Hartman @ 2020-05-24 15:30 UTC (permalink / raw)
  To: linux-kernel; +Cc: linux, Greg Kroah-Hartman, Rafael J. Wysocki

It is possible for a KOBJ_REMOVE uevent to be sent to userspace way
after the files are actually gone from sysfs, due to how reference
counting for kobjects work.  This should not be a problem, but it would
be good to properly send the information when things are going away, not
at some later point in time in the future.

Before this move, if a kobject's parent was torn down before the child,
when the call to kobject_uevent() happened, the parent walk to try to
reconstruct the full path of the kobject could be a total mess and cause
crashes.  It's not good to try to tear down a kobject tree from top
down, but let's at least try to not to crash if a user does so.

Cc: "Rafael J. Wysocki" <rafael@kernel.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 lib/kobject.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/lib/kobject.c b/lib/kobject.c
index 83198cb37d8d..2509e22ad74a 100644
--- a/lib/kobject.c
+++ b/lib/kobject.c
@@ -620,6 +620,13 @@ void kobject_del(struct kobject *kobj)
 	if (ktype)
 		sysfs_remove_groups(kobj, ktype->default_groups);
 
+	/* send "remove" if the caller did not do it but sent "add" */
+	if (kobj->state_add_uevent_sent && !kobj->state_remove_uevent_sent) {
+		pr_debug("kobject: '%s' (%p): auto cleanup 'remove' event\n",
+			 kobject_name(kobj), kobj);
+		kobject_uevent(kobj, KOBJ_REMOVE);
+	}
+
 	sysfs_remove_dir(kobj);
 	sysfs_put(sd);
 
@@ -673,13 +680,6 @@ static void kobject_cleanup(struct kobject *kobj)
 		pr_debug("kobject: '%s' (%p): does not have a release() function, it is broken and must be fixed. See Documentation/kobject.txt.\n",
 			 kobject_name(kobj), kobj);
 
-	/* send "remove" if the caller did not do it but sent "add" */
-	if (kobj->state_add_uevent_sent && !kobj->state_remove_uevent_sent) {
-		pr_debug("kobject: '%s' (%p): auto cleanup 'remove' event\n",
-			 kobject_name(kobj), kobj);
-		kobject_uevent(kobj, KOBJ_REMOVE);
-	}
-
 	/* remove from sysfs if the caller did not do it */
 	if (kobj->state_in_sysfs) {
 		pr_debug("kobject: '%s' (%p): auto cleanup kobject_del\n",
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* Re: [PATCH 1/2] software node: implement software_node_unregister()
  2020-05-24 15:30 [PATCH 1/2] software node: implement software_node_unregister() Greg Kroah-Hartman
  2020-05-24 15:30 ` [PATCH 2/2] kobject: send KOBJ_REMOVE uevent when the object is removed from sysfs Greg Kroah-Hartman
@ 2020-05-24 16:43 ` Guenter Roeck
  2020-05-25  2:44   ` Randy Dunlap
  2020-05-25  7:59 ` Petr Mladek
                   ` (3 subsequent siblings)
  5 siblings, 1 reply; 20+ messages in thread
From: Guenter Roeck @ 2020-05-24 16:43 UTC (permalink / raw)
  To: Greg Kroah-Hartman, linux-kernel
  Cc: Naresh Kamboju, kernel test robot, stable, Andy Shevchenko,
	Brendan Higgins, Dmitry Torokhov, Heikki Krogerus, Petr Mladek,
	Rafael J . Wysocki, Randy Dunlap, Rasmus Villemoes, Sakari Ailus,
	Sergey Senozhatsky, Steven Rostedt

On 5/24/20 8:30 AM, Greg Kroah-Hartman wrote:
> Sometimes it is better to unregister individual nodes instead of trying
> to do them all at once with software_node_unregister_nodes(), so create
> software_node_unregister() so that you can unregister them one at a
> time.
> 
> This is especially important when creating nodes in a hierarchy, with
> parent -> children representations.  Children always need to be removed
> before a parent is, as the swnode logic assumes this is going to be the
> case.
> 
> Fix up the lib/test_printf.c fwnode_pointer() test which to use this new
> function as it had the problem of tearing things down in the backwards
> order.
> 
> Fixes: f1ce39df508d ("lib/test_printf: Add tests for %pfw printk modifier")
> Reported-by: Naresh Kamboju <naresh.kamboju@linaro.org>
> Reported-by: kernel test robot <rong.a.chen@intel.com>
> Cc: stable <stable@vger.kernel.org>
> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Cc: Brendan Higgins <brendanhiggins@google.com>
> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> Cc: Petr Mladek <pmladek@suse.com>
> Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Cc: Randy Dunlap <rdunlap@infradead.org>
> Cc: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
> Cc: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> Cc: Steven Rostedt <rostedt@goodmis.org>
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Both patches pass my boot tests on arm64 and arm64be (I didn't test any others).
So, FWIW,

Tested-by: Guenter Roeck <linux@roeck-us.net>

I wasn't sure it the two patches replace or fix commit 4ef12f719802 ("kobject:
Make sure the parent does not get released before its children"), so I tried
to re-apply 4ef12f719802 on top of the two patches. Unfortunately that still
results in crashes and UAF messages.

Guenter

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 1/2] software node: implement software_node_unregister()
  2020-05-24 16:43 ` [PATCH 1/2] software node: implement software_node_unregister() Guenter Roeck
@ 2020-05-25  2:44   ` Randy Dunlap
  0 siblings, 0 replies; 20+ messages in thread
From: Randy Dunlap @ 2020-05-25  2:44 UTC (permalink / raw)
  To: Guenter Roeck, Greg Kroah-Hartman, linux-kernel
  Cc: Naresh Kamboju, kernel test robot, stable, Andy Shevchenko,
	Brendan Higgins, Dmitry Torokhov, Heikki Krogerus, Petr Mladek,
	Rafael J . Wysocki, Rasmus Villemoes, Sakari Ailus,
	Sergey Senozhatsky, Steven Rostedt

On 5/24/20 9:43 AM, Guenter Roeck wrote:
> On 5/24/20 8:30 AM, Greg Kroah-Hartman wrote:
>> Sometimes it is better to unregister individual nodes instead of trying
>> to do them all at once with software_node_unregister_nodes(), so create
>> software_node_unregister() so that you can unregister them one at a
>> time.
>>
>> This is especially important when creating nodes in a hierarchy, with
>> parent -> children representations.  Children always need to be removed
>> before a parent is, as the swnode logic assumes this is going to be the
>> case.
>>
>> Fix up the lib/test_printf.c fwnode_pointer() test which to use this new
>> function as it had the problem of tearing things down in the backwards
>> order.
>>
>> Fixes: f1ce39df508d ("lib/test_printf: Add tests for %pfw printk modifier")
>> Reported-by: Naresh Kamboju <naresh.kamboju@linaro.org>
>> Reported-by: kernel test robot <rong.a.chen@intel.com>
>> Cc: stable <stable@vger.kernel.org>
>> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>> Cc: Brendan Higgins <brendanhiggins@google.com>
>> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>> Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com>
>> Cc: Petr Mladek <pmladek@suse.com>
>> Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>> Cc: Randy Dunlap <rdunlap@infradead.org>
>> Cc: Rasmus Villemoes <linux@rasmusvillemoes.dk>
>> Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
>> Cc: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
>> Cc: Steven Rostedt <rostedt@goodmis.org>
>> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> 
> Both patches pass my boot tests on arm64 and arm64be (I didn't test any others).
> So, FWIW,
> 
> Tested-by: Guenter Roeck <linux@roeck-us.net>
> 
> I wasn't sure it the two patches replace or fix commit 4ef12f719802 ("kobject:
> Make sure the parent does not get released before its children"), so I tried
> to re-apply 4ef12f719802 on top of the two patches. Unfortunately that still
> results in crashes and UAF messages.

Yes, that kobject patch has been reverted:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=e6764aa0e5530066dd969eccea2a1a7d177859a8

and these 2 patches are to be used instead.

thanks.
-- 
~Randy


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 1/2] software node: implement software_node_unregister()
  2020-05-24 15:30 [PATCH 1/2] software node: implement software_node_unregister() Greg Kroah-Hartman
  2020-05-24 15:30 ` [PATCH 2/2] kobject: send KOBJ_REMOVE uevent when the object is removed from sysfs Greg Kroah-Hartman
  2020-05-24 16:43 ` [PATCH 1/2] software node: implement software_node_unregister() Guenter Roeck
@ 2020-05-25  7:59 ` Petr Mladek
  2020-05-25 11:24 ` Heikki Krogerus
                   ` (2 subsequent siblings)
  5 siblings, 0 replies; 20+ messages in thread
From: Petr Mladek @ 2020-05-25  7:59 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: linux-kernel, linux, Naresh Kamboju, kernel test robot, stable,
	Andy Shevchenko, Brendan Higgins, Dmitry Torokhov,
	Heikki Krogerus, Rafael J . Wysocki, Randy Dunlap,
	Rasmus Villemoes, Sakari Ailus, Sergey Senozhatsky,
	Steven Rostedt

On Sun 2020-05-24 17:30:40, Greg Kroah-Hartman wrote:
> Sometimes it is better to unregister individual nodes instead of trying
> to do them all at once with software_node_unregister_nodes(), so create
> software_node_unregister() so that you can unregister them one at a
> time.
> 
> This is especially important when creating nodes in a hierarchy, with
> parent -> children representations.  Children always need to be removed
> before a parent is, as the swnode logic assumes this is going to be the
> case.
> 
> Fix up the lib/test_printf.c fwnode_pointer() test which to use this new
> function as it had the problem of tearing things down in the backwards
> order.
> 
> Fixes: f1ce39df508d ("lib/test_printf: Add tests for %pfw printk modifier")
> Reported-by: Naresh Kamboju <naresh.kamboju@linaro.org>
> Reported-by: kernel test robot <rong.a.chen@intel.com>
> Cc: stable <stable@vger.kernel.org>
> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Cc: Brendan Higgins <brendanhiggins@google.com>
> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> Cc: Petr Mladek <pmladek@suse.com>
> Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Cc: Randy Dunlap <rdunlap@infradead.org>
> Cc: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
> Cc: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> Cc: Steven Rostedt <rostedt@goodmis.org>
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

I could confirm that this fixed lib/test_printf.c. The patch looks reasonable.

Tested-by: Petr Mladek <pmladek@suse.com>

Best Regards,
Petr

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 2/2] kobject: send KOBJ_REMOVE uevent when the object is removed from sysfs
  2020-05-24 15:30 ` [PATCH 2/2] kobject: send KOBJ_REMOVE uevent when the object is removed from sysfs Greg Kroah-Hartman
@ 2020-05-25 11:07   ` Rafael J. Wysocki
  2020-05-25 22:49   ` Dmitry Torokhov
  1 sibling, 0 replies; 20+ messages in thread
From: Rafael J. Wysocki @ 2020-05-25 11:07 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Linux Kernel Mailing List, Guenter Roeck, Rafael J. Wysocki

On Sun, May 24, 2020 at 5:31 PM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> It is possible for a KOBJ_REMOVE uevent to be sent to userspace way
> after the files are actually gone from sysfs, due to how reference
> counting for kobjects work.  This should not be a problem, but it would
> be good to properly send the information when things are going away, not
> at some later point in time in the future.
>
> Before this move, if a kobject's parent was torn down before the child,
> when the call to kobject_uevent() happened, the parent walk to try to
> reconstruct the full path of the kobject could be a total mess and cause
> crashes.  It's not good to try to tear down a kobject tree from top
> down, but let's at least try to not to crash if a user does so.
>
> Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

LGTM

Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

> ---
>  lib/kobject.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/lib/kobject.c b/lib/kobject.c
> index 83198cb37d8d..2509e22ad74a 100644
> --- a/lib/kobject.c
> +++ b/lib/kobject.c
> @@ -620,6 +620,13 @@ void kobject_del(struct kobject *kobj)
>         if (ktype)
>                 sysfs_remove_groups(kobj, ktype->default_groups);
>
> +       /* send "remove" if the caller did not do it but sent "add" */
> +       if (kobj->state_add_uevent_sent && !kobj->state_remove_uevent_sent) {
> +               pr_debug("kobject: '%s' (%p): auto cleanup 'remove' event\n",
> +                        kobject_name(kobj), kobj);
> +               kobject_uevent(kobj, KOBJ_REMOVE);
> +       }
> +
>         sysfs_remove_dir(kobj);
>         sysfs_put(sd);
>
> @@ -673,13 +680,6 @@ static void kobject_cleanup(struct kobject *kobj)
>                 pr_debug("kobject: '%s' (%p): does not have a release() function, it is broken and must be fixed. See Documentation/kobject.txt.\n",
>                          kobject_name(kobj), kobj);
>
> -       /* send "remove" if the caller did not do it but sent "add" */
> -       if (kobj->state_add_uevent_sent && !kobj->state_remove_uevent_sent) {
> -               pr_debug("kobject: '%s' (%p): auto cleanup 'remove' event\n",
> -                        kobject_name(kobj), kobj);
> -               kobject_uevent(kobj, KOBJ_REMOVE);
> -       }
> -
>         /* remove from sysfs if the caller did not do it */
>         if (kobj->state_in_sysfs) {
>                 pr_debug("kobject: '%s' (%p): auto cleanup kobject_del\n",
> --
> 2.26.2
>

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 1/2] software node: implement software_node_unregister()
  2020-05-24 15:30 [PATCH 1/2] software node: implement software_node_unregister() Greg Kroah-Hartman
                   ` (2 preceding siblings ...)
  2020-05-25  7:59 ` Petr Mladek
@ 2020-05-25 11:24 ` Heikki Krogerus
  2020-05-26  0:12 ` Randy Dunlap
  2020-05-28 20:25 ` Brendan Higgins
  5 siblings, 0 replies; 20+ messages in thread
From: Heikki Krogerus @ 2020-05-25 11:24 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: linux-kernel, linux, Naresh Kamboju, kernel test robot, stable,
	Andy Shevchenko, Brendan Higgins, Dmitry Torokhov, Petr Mladek,
	Rafael J . Wysocki, Randy Dunlap, Rasmus Villemoes, Sakari Ailus,
	Sergey Senozhatsky, Steven Rostedt

On Sun, May 24, 2020 at 05:30:40PM +0200, Greg Kroah-Hartman wrote:
> Sometimes it is better to unregister individual nodes instead of trying
> to do them all at once with software_node_unregister_nodes(), so create
> software_node_unregister() so that you can unregister them one at a
> time.
> 
> This is especially important when creating nodes in a hierarchy, with
> parent -> children representations.  Children always need to be removed
> before a parent is, as the swnode logic assumes this is going to be the
> case.
> 
> Fix up the lib/test_printf.c fwnode_pointer() test which to use this new
> function as it had the problem of tearing things down in the backwards
> order.
> 
> Fixes: f1ce39df508d ("lib/test_printf: Add tests for %pfw printk modifier")
> Reported-by: Naresh Kamboju <naresh.kamboju@linaro.org>
> Reported-by: kernel test robot <rong.a.chen@intel.com>
> Cc: stable <stable@vger.kernel.org>
> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Cc: Brendan Higgins <brendanhiggins@google.com>
> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> Cc: Petr Mladek <pmladek@suse.com>
> Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Cc: Randy Dunlap <rdunlap@infradead.org>
> Cc: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
> Cc: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> Cc: Steven Rostedt <rostedt@goodmis.org>
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

FWIW:

Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>

> ---
>  drivers/base/swnode.c    | 27 +++++++++++++++++++++------
>  include/linux/property.h |  1 +
>  lib/test_printf.c        |  4 +++-
>  3 files changed, 25 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
> index de8d3543e8fe..770b1f47a625 100644
> --- a/drivers/base/swnode.c
> +++ b/drivers/base/swnode.c
> @@ -712,17 +712,18 @@ EXPORT_SYMBOL_GPL(software_node_register_nodes);
>   * @nodes: Zero terminated array of software nodes to be unregistered
>   *
>   * Unregister multiple software nodes at once.
> + *
> + * NOTE: Be careful using this call if the nodes had parent pointers set up in
> + * them before registering.  If so, it is wiser to remove the nodes
> + * individually, in the correct order (child before parent) instead of relying
> + * on the sequential order of the list of nodes in the array.
>   */
>  void software_node_unregister_nodes(const struct software_node *nodes)
>  {
> -	struct swnode *swnode;
>  	int i;
>  
> -	for (i = 0; nodes[i].name; i++) {
> -		swnode = software_node_to_swnode(&nodes[i]);
> -		if (swnode)
> -			fwnode_remove_software_node(&swnode->fwnode);
> -	}
> +	for (i = 0; nodes[i].name; i++)
> +		software_node_unregister(&nodes[i]);
>  }
>  EXPORT_SYMBOL_GPL(software_node_unregister_nodes);
>  
> @@ -741,6 +742,20 @@ int software_node_register(const struct software_node *node)
>  }
>  EXPORT_SYMBOL_GPL(software_node_register);
>  
> +/**
> + * software_node_unregister - Unregister static software node
> + * @node: The software node to be unregistered
> + */
> +void software_node_unregister(const struct software_node *node)
> +{
> +	struct swnode *swnode;
> +
> +	swnode = software_node_to_swnode(node);
> +	if (swnode)
> +		fwnode_remove_software_node(&swnode->fwnode);
> +}
> +EXPORT_SYMBOL_GPL(software_node_unregister);
> +
>  struct fwnode_handle *
>  fwnode_create_software_node(const struct property_entry *properties,
>  			    const struct fwnode_handle *parent)
> diff --git a/include/linux/property.h b/include/linux/property.h
> index d86de017c689..0d4099b4ce1f 100644
> --- a/include/linux/property.h
> +++ b/include/linux/property.h
> @@ -441,6 +441,7 @@ int software_node_register_nodes(const struct software_node *nodes);
>  void software_node_unregister_nodes(const struct software_node *nodes);
>  
>  int software_node_register(const struct software_node *node);
> +void software_node_unregister(const struct software_node *node);
>  
>  int software_node_notify(struct device *dev, unsigned long action);
>  
> diff --git a/lib/test_printf.c b/lib/test_printf.c
> index 6b1622f4d7c2..fc63b8959d42 100644
> --- a/lib/test_printf.c
> +++ b/lib/test_printf.c
> @@ -637,7 +637,9 @@ static void __init fwnode_pointer(void)
>  	test(second_name, "%pfwP", software_node_fwnode(&softnodes[1]));
>  	test(third_name, "%pfwP", software_node_fwnode(&softnodes[2]));
>  
> -	software_node_unregister_nodes(softnodes);
> +	software_node_unregister(&softnodes[2]);
> +	software_node_unregister(&softnodes[1]);
> +	software_node_unregister(&softnodes[0]);
>  }
>  
>  static void __init
> -- 
> 2.26.2

thanks,

-- 
heikki

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 2/2] kobject: send KOBJ_REMOVE uevent when the object is removed from sysfs
  2020-05-24 15:30 ` [PATCH 2/2] kobject: send KOBJ_REMOVE uevent when the object is removed from sysfs Greg Kroah-Hartman
  2020-05-25 11:07   ` Rafael J. Wysocki
@ 2020-05-25 22:49   ` Dmitry Torokhov
  2020-05-26  5:58     ` Greg Kroah-Hartman
  1 sibling, 1 reply; 20+ messages in thread
From: Dmitry Torokhov @ 2020-05-25 22:49 UTC (permalink / raw)
  To: Greg Kroah-Hartman; +Cc: lkml, Guenter Roeck, Rafael J. Wysocki

On Sun, May 24, 2020 at 8:34 AM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> It is possible for a KOBJ_REMOVE uevent to be sent to userspace way
> after the files are actually gone from sysfs, due to how reference
> counting for kobjects work.  This should not be a problem, but it would
> be good to properly send the information when things are going away, not
> at some later point in time in the future.
>
> Before this move, if a kobject's parent was torn down before the child,

^^^^ And this is the root of the problem and what has to be fixed.

> when the call to kobject_uevent() happened, the parent walk to try to
> reconstruct the full path of the kobject could be a total mess and cause
> crashes.  It's not good to try to tear down a kobject tree from top
> down, but let's at least try to not to crash if a user does so.

One can try, but if we keep proper reference counting then kobject
core should take care of actually releasing objects in the right
order. I do not think you should keep this patch, and instead see if
we can push call to kobject_put(kobj->parent) into kobject_cleanup().

Thanks.

-- 
Dmitry

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 1/2] software node: implement software_node_unregister()
  2020-05-24 15:30 [PATCH 1/2] software node: implement software_node_unregister() Greg Kroah-Hartman
                   ` (3 preceding siblings ...)
  2020-05-25 11:24 ` Heikki Krogerus
@ 2020-05-26  0:12 ` Randy Dunlap
  2020-05-28 20:25 ` Brendan Higgins
  5 siblings, 0 replies; 20+ messages in thread
From: Randy Dunlap @ 2020-05-26  0:12 UTC (permalink / raw)
  To: Greg Kroah-Hartman, linux-kernel
  Cc: linux, Naresh Kamboju, kernel test robot, stable,
	Andy Shevchenko, Brendan Higgins, Dmitry Torokhov,
	Heikki Krogerus, Petr Mladek, Rafael J . Wysocki,
	Rasmus Villemoes, Sakari Ailus, Sergey Senozhatsky,
	Steven Rostedt

On 5/24/20 8:30 AM, Greg Kroah-Hartman wrote:
> Sometimes it is better to unregister individual nodes instead of trying
> to do them all at once with software_node_unregister_nodes(), so create
> software_node_unregister() so that you can unregister them one at a
> time.
> 
> This is especially important when creating nodes in a hierarchy, with
> parent -> children representations.  Children always need to be removed
> before a parent is, as the swnode logic assumes this is going to be the
> case.
> 
> Fix up the lib/test_printf.c fwnode_pointer() test which to use this new
> function as it had the problem of tearing things down in the backwards
> order.
> 
> Fixes: f1ce39df508d ("lib/test_printf: Add tests for %pfw printk modifier")
> Reported-by: Naresh Kamboju <naresh.kamboju@linaro.org>
> Reported-by: kernel test robot <rong.a.chen@intel.com>
> Cc: stable <stable@vger.kernel.org>
> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Cc: Brendan Higgins <brendanhiggins@google.com>
> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> Cc: Petr Mladek <pmladek@suse.com>
> Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Cc: Randy Dunlap <rdunlap@infradead.org>
> Cc: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
> Cc: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> Cc: Steven Rostedt <rostedt@goodmis.org>
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Reported-by: Randy Dunlap <rdunlap@infradead.org>
Acked-by: Randy Dunlap <rdunlap@infradead.org>
Tested-by: Randy Dunlap <rdunlap@infradead.org>

thanks.

> ---
>  drivers/base/swnode.c    | 27 +++++++++++++++++++++------
>  include/linux/property.h |  1 +
>  lib/test_printf.c        |  4 +++-
>  3 files changed, 25 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
> index de8d3543e8fe..770b1f47a625 100644
> --- a/drivers/base/swnode.c
> +++ b/drivers/base/swnode.c
> @@ -712,17 +712,18 @@ EXPORT_SYMBOL_GPL(software_node_register_nodes);
>   * @nodes: Zero terminated array of software nodes to be unregistered
>   *
>   * Unregister multiple software nodes at once.
> + *
> + * NOTE: Be careful using this call if the nodes had parent pointers set up in
> + * them before registering.  If so, it is wiser to remove the nodes
> + * individually, in the correct order (child before parent) instead of relying
> + * on the sequential order of the list of nodes in the array.
>   */
>  void software_node_unregister_nodes(const struct software_node *nodes)
>  {
> -	struct swnode *swnode;
>  	int i;
>  
> -	for (i = 0; nodes[i].name; i++) {
> -		swnode = software_node_to_swnode(&nodes[i]);
> -		if (swnode)
> -			fwnode_remove_software_node(&swnode->fwnode);
> -	}
> +	for (i = 0; nodes[i].name; i++)
> +		software_node_unregister(&nodes[i]);
>  }
>  EXPORT_SYMBOL_GPL(software_node_unregister_nodes);
>  
> @@ -741,6 +742,20 @@ int software_node_register(const struct software_node *node)
>  }
>  EXPORT_SYMBOL_GPL(software_node_register);
>  
> +/**
> + * software_node_unregister - Unregister static software node
> + * @node: The software node to be unregistered
> + */
> +void software_node_unregister(const struct software_node *node)
> +{
> +	struct swnode *swnode;
> +
> +	swnode = software_node_to_swnode(node);
> +	if (swnode)
> +		fwnode_remove_software_node(&swnode->fwnode);
> +}
> +EXPORT_SYMBOL_GPL(software_node_unregister);
> +
>  struct fwnode_handle *
>  fwnode_create_software_node(const struct property_entry *properties,
>  			    const struct fwnode_handle *parent)
> diff --git a/include/linux/property.h b/include/linux/property.h
> index d86de017c689..0d4099b4ce1f 100644
> --- a/include/linux/property.h
> +++ b/include/linux/property.h
> @@ -441,6 +441,7 @@ int software_node_register_nodes(const struct software_node *nodes);
>  void software_node_unregister_nodes(const struct software_node *nodes);
>  
>  int software_node_register(const struct software_node *node);
> +void software_node_unregister(const struct software_node *node);
>  
>  int software_node_notify(struct device *dev, unsigned long action);
>  
> diff --git a/lib/test_printf.c b/lib/test_printf.c
> index 6b1622f4d7c2..fc63b8959d42 100644
> --- a/lib/test_printf.c
> +++ b/lib/test_printf.c
> @@ -637,7 +637,9 @@ static void __init fwnode_pointer(void)
>  	test(second_name, "%pfwP", software_node_fwnode(&softnodes[1]));
>  	test(third_name, "%pfwP", software_node_fwnode(&softnodes[2]));
>  
> -	software_node_unregister_nodes(softnodes);
> +	software_node_unregister(&softnodes[2]);
> +	software_node_unregister(&softnodes[1]);
> +	software_node_unregister(&softnodes[0]);
>  }
>  
>  static void __init
> 


-- 
~Randy

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 2/2] kobject: send KOBJ_REMOVE uevent when the object is removed from sysfs
  2020-05-25 22:49   ` Dmitry Torokhov
@ 2020-05-26  5:58     ` Greg Kroah-Hartman
  2020-05-26  8:26       ` Rafael J. Wysocki
  0 siblings, 1 reply; 20+ messages in thread
From: Greg Kroah-Hartman @ 2020-05-26  5:58 UTC (permalink / raw)
  To: Dmitry Torokhov; +Cc: lkml, Guenter Roeck, Rafael J. Wysocki

On Mon, May 25, 2020 at 03:49:01PM -0700, Dmitry Torokhov wrote:
> On Sun, May 24, 2020 at 8:34 AM Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> >
> > It is possible for a KOBJ_REMOVE uevent to be sent to userspace way
> > after the files are actually gone from sysfs, due to how reference
> > counting for kobjects work.  This should not be a problem, but it would
> > be good to properly send the information when things are going away, not
> > at some later point in time in the future.
> >
> > Before this move, if a kobject's parent was torn down before the child,
> 
> ^^^^ And this is the root of the problem and what has to be fixed.

I fixed that in patch one of this series.  Turns out the user of the
kobject was not even expecting that to happen.

> > when the call to kobject_uevent() happened, the parent walk to try to
> > reconstruct the full path of the kobject could be a total mess and cause
> > crashes.  It's not good to try to tear down a kobject tree from top
> > down, but let's at least try to not to crash if a user does so.
> 
> One can try, but if we keep proper reference counting then kobject
> core should take care of actually releasing objects in the right
> order. I do not think you should keep this patch, and instead see if
> we can push call to kobject_put(kobj->parent) into kobject_cleanup().

I tried that, but there was a _lot_ of underflow errors reported, so
there's something else happening.  Or my attempt was incorrect :)

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 2/2] kobject: send KOBJ_REMOVE uevent when the object is removed from sysfs
  2020-05-26  5:58     ` Greg Kroah-Hartman
@ 2020-05-26  8:26       ` Rafael J. Wysocki
  2020-05-27  7:50         ` Heikki Krogerus
  0 siblings, 1 reply; 20+ messages in thread
From: Rafael J. Wysocki @ 2020-05-26  8:26 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Dmitry Torokhov, lkml, Guenter Roeck, Rafael J. Wysocki, Heikki Krogerus

On Tue, May 26, 2020 at 7:58 AM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Mon, May 25, 2020 at 03:49:01PM -0700, Dmitry Torokhov wrote:
> > On Sun, May 24, 2020 at 8:34 AM Greg Kroah-Hartman
> > <gregkh@linuxfoundation.org> wrote:
> > >
> > > It is possible for a KOBJ_REMOVE uevent to be sent to userspace way
> > > after the files are actually gone from sysfs, due to how reference
> > > counting for kobjects work.  This should not be a problem, but it would
> > > be good to properly send the information when things are going away, not
> > > at some later point in time in the future.
> > >
> > > Before this move, if a kobject's parent was torn down before the child,
> >
> > ^^^^ And this is the root of the problem and what has to be fixed.
>
> I fixed that in patch one of this series.  Turns out the user of the
> kobject was not even expecting that to happen.
>
> > > when the call to kobject_uevent() happened, the parent walk to try to
> > > reconstruct the full path of the kobject could be a total mess and cause
> > > crashes.  It's not good to try to tear down a kobject tree from top
> > > down, but let's at least try to not to crash if a user does so.
> >
> > One can try, but if we keep proper reference counting then kobject
> > core should take care of actually releasing objects in the right
> > order. I do not think you should keep this patch, and instead see if
> > we can push call to kobject_put(kobj->parent) into kobject_cleanup().
>
> I tried that, but there was a _lot_ of underflow errors reported, so
> there's something else happening.  Or my attempt was incorrect :)

So it looks like there is something in there that's been overlooked so far.

I'll try to look at the Guenter's traces and figure out what went
wrong after the Heikki's patch.

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 2/2] kobject: send KOBJ_REMOVE uevent when the object is removed from sysfs
  2020-05-26  8:26       ` Rafael J. Wysocki
@ 2020-05-27  7:50         ` Heikki Krogerus
  2020-05-27  8:34           ` Rafael J. Wysocki
  0 siblings, 1 reply; 20+ messages in thread
From: Heikki Krogerus @ 2020-05-27  7:50 UTC (permalink / raw)
  To: Rafael J. Wysocki
  Cc: Greg Kroah-Hartman, Dmitry Torokhov, lkml, Guenter Roeck

On Tue, May 26, 2020 at 10:26:23AM +0200, Rafael J. Wysocki wrote:
> On Tue, May 26, 2020 at 7:58 AM Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> >
> > On Mon, May 25, 2020 at 03:49:01PM -0700, Dmitry Torokhov wrote:
> > > On Sun, May 24, 2020 at 8:34 AM Greg Kroah-Hartman
> > > <gregkh@linuxfoundation.org> wrote:
> > > >
> > > > It is possible for a KOBJ_REMOVE uevent to be sent to userspace way
> > > > after the files are actually gone from sysfs, due to how reference
> > > > counting for kobjects work.  This should not be a problem, but it would
> > > > be good to properly send the information when things are going away, not
> > > > at some later point in time in the future.
> > > >
> > > > Before this move, if a kobject's parent was torn down before the child,
> > >
> > > ^^^^ And this is the root of the problem and what has to be fixed.
> >
> > I fixed that in patch one of this series.  Turns out the user of the
> > kobject was not even expecting that to happen.
> >
> > > > when the call to kobject_uevent() happened, the parent walk to try to
> > > > reconstruct the full path of the kobject could be a total mess and cause
> > > > crashes.  It's not good to try to tear down a kobject tree from top
> > > > down, but let's at least try to not to crash if a user does so.
> > >
> > > One can try, but if we keep proper reference counting then kobject
> > > core should take care of actually releasing objects in the right
> > > order. I do not think you should keep this patch, and instead see if
> > > we can push call to kobject_put(kobj->parent) into kobject_cleanup().
> >
> > I tried that, but there was a _lot_ of underflow errors reported, so
> > there's something else happening.  Or my attempt was incorrect :)
> 
> So it looks like there is something in there that's been overlooked so far.
> 
> I'll try to look at the Guenter's traces and figure out what went
> wrong after the Heikki's patch.

At least one problem with that patch was that I was releasing the
parent reference unconditionally.

thanks,

-- 
heikki

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 2/2] kobject: send KOBJ_REMOVE uevent when the object is removed from sysfs
  2020-05-27  7:50         ` Heikki Krogerus
@ 2020-05-27  8:34           ` Rafael J. Wysocki
  2020-05-27  9:01             ` Rafael J. Wysocki
  0 siblings, 1 reply; 20+ messages in thread
From: Rafael J. Wysocki @ 2020-05-27  8:34 UTC (permalink / raw)
  To: Heikki Krogerus
  Cc: Rafael J. Wysocki, Greg Kroah-Hartman, Dmitry Torokhov, lkml,
	Guenter Roeck

On Wed, May 27, 2020 at 9:50 AM Heikki Krogerus
<heikki.krogerus@linux.intel.com> wrote:
>
> On Tue, May 26, 2020 at 10:26:23AM +0200, Rafael J. Wysocki wrote:
> > On Tue, May 26, 2020 at 7:58 AM Greg Kroah-Hartman
> > <gregkh@linuxfoundation.org> wrote:
> > >
> > > On Mon, May 25, 2020 at 03:49:01PM -0700, Dmitry Torokhov wrote:
> > > > On Sun, May 24, 2020 at 8:34 AM Greg Kroah-Hartman
> > > > <gregkh@linuxfoundation.org> wrote:
> > > > >
> > > > > It is possible for a KOBJ_REMOVE uevent to be sent to userspace way
> > > > > after the files are actually gone from sysfs, due to how reference
> > > > > counting for kobjects work.  This should not be a problem, but it would
> > > > > be good to properly send the information when things are going away, not
> > > > > at some later point in time in the future.
> > > > >
> > > > > Before this move, if a kobject's parent was torn down before the child,
> > > >
> > > > ^^^^ And this is the root of the problem and what has to be fixed.
> > >
> > > I fixed that in patch one of this series.  Turns out the user of the
> > > kobject was not even expecting that to happen.
> > >
> > > > > when the call to kobject_uevent() happened, the parent walk to try to
> > > > > reconstruct the full path of the kobject could be a total mess and cause
> > > > > crashes.  It's not good to try to tear down a kobject tree from top
> > > > > down, but let's at least try to not to crash if a user does so.
> > > >
> > > > One can try, but if we keep proper reference counting then kobject
> > > > core should take care of actually releasing objects in the right
> > > > order. I do not think you should keep this patch, and instead see if
> > > > we can push call to kobject_put(kobj->parent) into kobject_cleanup().
> > >
> > > I tried that, but there was a _lot_ of underflow errors reported, so
> > > there's something else happening.  Or my attempt was incorrect :)
> >
> > So it looks like there is something in there that's been overlooked so far.
> >
> > I'll try to look at the Guenter's traces and figure out what went
> > wrong after the Heikki's patch.
>
> At least one problem with that patch was that I was releasing the
> parent reference unconditionally.

That actually may be sufficient to explain all of the problems introduced by it.

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 2/2] kobject: send KOBJ_REMOVE uevent when the object is removed from sysfs
  2020-05-27  8:34           ` Rafael J. Wysocki
@ 2020-05-27  9:01             ` Rafael J. Wysocki
  2020-05-27 14:34               ` Rafael J. Wysocki
  2020-05-27 22:25               ` Guenter Roeck
  0 siblings, 2 replies; 20+ messages in thread
From: Rafael J. Wysocki @ 2020-05-27  9:01 UTC (permalink / raw)
  To: linux-kernel, Guenter Roeck
  Cc: Rafael J. Wysocki, Heikki Krogerus, Greg Kroah-Hartman, Dmitry Torokhov

On Wednesday, May 27, 2020 10:34:51 AM CEST Rafael J. Wysocki wrote:
> On Wed, May 27, 2020 at 9:50 AM Heikki Krogerus
> <heikki.krogerus@linux.intel.com> wrote:
> >
> > On Tue, May 26, 2020 at 10:26:23AM +0200, Rafael J. Wysocki wrote:
> > > On Tue, May 26, 2020 at 7:58 AM Greg Kroah-Hartman
> > > <gregkh@linuxfoundation.org> wrote:
> > > >
> > > > On Mon, May 25, 2020 at 03:49:01PM -0700, Dmitry Torokhov wrote:
> > > > > On Sun, May 24, 2020 at 8:34 AM Greg Kroah-Hartman
> > > > > <gregkh@linuxfoundation.org> wrote:
> > > > > >
> > > > > > It is possible for a KOBJ_REMOVE uevent to be sent to userspace way
> > > > > > after the files are actually gone from sysfs, due to how reference
> > > > > > counting for kobjects work.  This should not be a problem, but it would
> > > > > > be good to properly send the information when things are going away, not
> > > > > > at some later point in time in the future.
> > > > > >
> > > > > > Before this move, if a kobject's parent was torn down before the child,
> > > > >
> > > > > ^^^^ And this is the root of the problem and what has to be fixed.
> > > >
> > > > I fixed that in patch one of this series.  Turns out the user of the
> > > > kobject was not even expecting that to happen.
> > > >
> > > > > > when the call to kobject_uevent() happened, the parent walk to try to
> > > > > > reconstruct the full path of the kobject could be a total mess and cause
> > > > > > crashes.  It's not good to try to tear down a kobject tree from top
> > > > > > down, but let's at least try to not to crash if a user does so.
> > > > >
> > > > > One can try, but if we keep proper reference counting then kobject
> > > > > core should take care of actually releasing objects in the right
> > > > > order. I do not think you should keep this patch, and instead see if
> > > > > we can push call to kobject_put(kobj->parent) into kobject_cleanup().
> > > >
> > > > I tried that, but there was a _lot_ of underflow errors reported, so
> > > > there's something else happening.  Or my attempt was incorrect :)
> > >
> > > So it looks like there is something in there that's been overlooked so far.
> > >
> > > I'll try to look at the Guenter's traces and figure out what went
> > > wrong after the Heikki's patch.
> >
> > At least one problem with that patch was that I was releasing the
> > parent reference unconditionally.
> 
> That actually may be sufficient to explain all of the problems introduced by it.

So Guenter, can you please test the patch below to see if it still introduces
the problems seen by you on ARM?

---
From: Heikki Krogerus <heikki.krogerus@linux.intel.com>
Subject: [PATCH] kobject: Make sure the parent does not get released before its children

In the function kobject_cleanup(), kobject_del(kobj) is
called before the kobj->release(). That makes it possible to
release the parent of the kobject before the kobject itself.

To fix that, adding function __kboject_del() that does
everything that kobject_del() does except release the parent
reference. kobject_cleanup() then calls __kobject_del()
instead of kobject_del(), and separately decrements the
reference count of the parent kobject after kobj->release()
has been called.

Reported-by: Naresh Kamboju <naresh.kamboju@linaro.org>
Reported-by: kernel test robot <rong.a.chen@intel.com>
Fixes: 7589238a8cf3 ("Revert "software node: Simplify software_node_release() function"")
Suggested-by: "Rafael J. Wysocki" <rafael@kernel.org>
Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
[ rjw: Drop parent reference only when called __kobject_del() ]
Signed-off-by: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>
---
 lib/kobject.c |   34 +++++++++++++++++++++++-----------
 1 file changed, 23 insertions(+), 11 deletions(-)

Index: linux-pm/lib/kobject.c
===================================================================
--- linux-pm.orig/lib/kobject.c
+++ linux-pm/lib/kobject.c
@@ -599,14 +599,7 @@ out:
 }
 EXPORT_SYMBOL_GPL(kobject_move);
 
-/**
- * kobject_del() - Unlink kobject from hierarchy.
- * @kobj: object.
- *
- * This is the function that should be called to delete an object
- * successfully added via kobject_add().
- */
-void kobject_del(struct kobject *kobj)
+static void __kobject_del(struct kobject *kobj)
 {
 	struct kernfs_node *sd;
 	const struct kobj_type *ktype;
@@ -625,9 +618,23 @@ void kobject_del(struct kobject *kobj)
 
 	kobj->state_in_sysfs = 0;
 	kobj_kset_leave(kobj);
-	kobject_put(kobj->parent);
 	kobj->parent = NULL;
 }
+
+/**
+ * kobject_del() - Unlink kobject from hierarchy.
+ * @kobj: object.
+ *
+ * This is the function that should be called to delete an object
+ * successfully added via kobject_add().
+ */
+void kobject_del(struct kobject *kobj)
+{
+	struct kobject *parent = kobj->parent;
+
+	__kobject_del(kobj);
+	kobject_put(parent);
+}
 EXPORT_SYMBOL(kobject_del);
 
 /**
@@ -663,7 +670,9 @@ EXPORT_SYMBOL(kobject_get_unless_zero);
  */
 static void kobject_cleanup(struct kobject *kobj)
 {
+	struct kobject *parent = kobj->parent;
 	struct kobj_type *t = get_ktype(kobj);
+	bool state_in_sysfs = kobj->state_in_sysfs;
 	const char *name = kobj->name;
 
 	pr_debug("kobject: '%s' (%p): %s, parent %p\n",
@@ -681,10 +690,10 @@ static void kobject_cleanup(struct kobje
 	}
 
 	/* remove from sysfs if the caller did not do it */
-	if (kobj->state_in_sysfs) {
+	if (state_in_sysfs) {
 		pr_debug("kobject: '%s' (%p): auto cleanup kobject_del\n",
 			 kobject_name(kobj), kobj);
-		kobject_del(kobj);
+		__kobject_del(kobj);
 	}
 
 	if (t && t->release) {
@@ -698,6 +707,9 @@ static void kobject_cleanup(struct kobje
 		pr_debug("kobject: '%s': free name\n", name);
 		kfree_const(name);
 	}
+
+	if (state_in_sysfs)
+		kobject_put(parent);
 }
 
 #ifdef CONFIG_DEBUG_KOBJECT_RELEASE




^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 2/2] kobject: send KOBJ_REMOVE uevent when the object is removed from sysfs
  2020-05-27  9:01             ` Rafael J. Wysocki
@ 2020-05-27 14:34               ` Rafael J. Wysocki
  2020-05-27 22:25               ` Guenter Roeck
  1 sibling, 0 replies; 20+ messages in thread
From: Rafael J. Wysocki @ 2020-05-27 14:34 UTC (permalink / raw)
  To: linux-kernel, Guenter Roeck
  Cc: Rafael J. Wysocki, Heikki Krogerus, Greg Kroah-Hartman, Dmitry Torokhov

On Wednesday, May 27, 2020 11:01:16 AM CEST Rafael J. Wysocki wrote:
> On Wednesday, May 27, 2020 10:34:51 AM CEST Rafael J. Wysocki wrote:
> > On Wed, May 27, 2020 at 9:50 AM Heikki Krogerus
> > <heikki.krogerus@linux.intel.com> wrote:
> > >
> > > On Tue, May 26, 2020 at 10:26:23AM +0200, Rafael J. Wysocki wrote:
> > > > On Tue, May 26, 2020 at 7:58 AM Greg Kroah-Hartman
> > > > <gregkh@linuxfoundation.org> wrote:
> > > > >
> > > > > On Mon, May 25, 2020 at 03:49:01PM -0700, Dmitry Torokhov wrote:
> > > > > > On Sun, May 24, 2020 at 8:34 AM Greg Kroah-Hartman
> > > > > > <gregkh@linuxfoundation.org> wrote:
> > > > > > >
> > > > > > > It is possible for a KOBJ_REMOVE uevent to be sent to userspace way
> > > > > > > after the files are actually gone from sysfs, due to how reference
> > > > > > > counting for kobjects work.  This should not be a problem, but it would
> > > > > > > be good to properly send the information when things are going away, not
> > > > > > > at some later point in time in the future.
> > > > > > >
> > > > > > > Before this move, if a kobject's parent was torn down before the child,
> > > > > >
> > > > > > ^^^^ And this is the root of the problem and what has to be fixed.
> > > > >
> > > > > I fixed that in patch one of this series.  Turns out the user of the
> > > > > kobject was not even expecting that to happen.
> > > > >
> > > > > > > when the call to kobject_uevent() happened, the parent walk to try to
> > > > > > > reconstruct the full path of the kobject could be a total mess and cause
> > > > > > > crashes.  It's not good to try to tear down a kobject tree from top
> > > > > > > down, but let's at least try to not to crash if a user does so.
> > > > > >
> > > > > > One can try, but if we keep proper reference counting then kobject
> > > > > > core should take care of actually releasing objects in the right
> > > > > > order. I do not think you should keep this patch, and instead see if
> > > > > > we can push call to kobject_put(kobj->parent) into kobject_cleanup().
> > > > >
> > > > > I tried that, but there was a _lot_ of underflow errors reported, so
> > > > > there's something else happening.  Or my attempt was incorrect :)
> > > >
> > > > So it looks like there is something in there that's been overlooked so far.
> > > >
> > > > I'll try to look at the Guenter's traces and figure out what went
> > > > wrong after the Heikki's patch.
> > >
> > > At least one problem with that patch was that I was releasing the
> > > parent reference unconditionally.
> > 
> > That actually may be sufficient to explain all of the problems introduced by it.
> 
> So Guenter, can you please test the patch below

Which can also be done without adding the second local var in kobject_cleanup(),
as follows.

> to see if it still introduces the problems seen by you on ARM?

---
From: Heikki Krogerus <heikki.krogerus@linux.intel.com>
Subject: [PATCH] kobject: Make sure the parent does not get released before its children

In the function kobject_cleanup(), kobject_del(kobj) is
called before the kobj->release(). That makes it possible to
release the parent of the kobject before the kobject itself.

To fix that, adding function __kboject_del() that does
everything that kobject_del() does except release the parent
reference. kobject_cleanup() then calls __kobject_del()
instead of kobject_del(), and separately decrements the
reference count of the parent kobject after kobj->release()
has been called.

Reported-by: Naresh Kamboju <naresh.kamboju@linaro.org>
Reported-by: kernel test robot <rong.a.chen@intel.com>
Fixes: 7589238a8cf3 ("Revert "software node: Simplify software_node_release() function"")
Suggested-by: "Rafael J. Wysocki" <rafael@kernel.org>
Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
[ rjw: Drop parent reference only when called __kobject_del() ]
Signed-off-by: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>
---
 lib/kobject.c |   33 +++++++++++++++++++++++----------
 1 file changed, 23 insertions(+), 10 deletions(-)

Index: linux-pm/lib/kobject.c
===================================================================
--- linux-pm.orig/lib/kobject.c
+++ linux-pm/lib/kobject.c
@@ -599,14 +599,7 @@ out:
 }
 EXPORT_SYMBOL_GPL(kobject_move);
 
-/**
- * kobject_del() - Unlink kobject from hierarchy.
- * @kobj: object.
- *
- * This is the function that should be called to delete an object
- * successfully added via kobject_add().
- */
-void kobject_del(struct kobject *kobj)
+static void __kobject_del(struct kobject *kobj)
 {
 	struct kernfs_node *sd;
 	const struct kobj_type *ktype;
@@ -625,9 +618,23 @@ void kobject_del(struct kobject *kobj)
 
 	kobj->state_in_sysfs = 0;
 	kobj_kset_leave(kobj);
-	kobject_put(kobj->parent);
 	kobj->parent = NULL;
 }
+
+/**
+ * kobject_del() - Unlink kobject from hierarchy.
+ * @kobj: object.
+ *
+ * This is the function that should be called to delete an object
+ * successfully added via kobject_add().
+ */
+void kobject_del(struct kobject *kobj)
+{
+	struct kobject *parent = kobj->parent;
+
+	__kobject_del(kobj);
+	kobject_put(parent);
+}
 EXPORT_SYMBOL(kobject_del);
 
 /**
@@ -663,6 +670,7 @@ EXPORT_SYMBOL(kobject_get_unless_zero);
  */
 static void kobject_cleanup(struct kobject *kobj)
 {
+	struct kobject *parent = kobj->parent;
 	struct kobj_type *t = get_ktype(kobj);
 	const char *name = kobj->name;
 
@@ -684,7 +692,10 @@ static void kobject_cleanup(struct kobje
 	if (kobj->state_in_sysfs) {
 		pr_debug("kobject: '%s' (%p): auto cleanup kobject_del\n",
 			 kobject_name(kobj), kobj);
-		kobject_del(kobj);
+		__kobject_del(kobj);
+	} else {
+		/* avoid dropping the parent reference unnecessarily */
+		parent = NULL;
 	}
 
 	if (t && t->release) {
@@ -698,6 +709,8 @@ static void kobject_cleanup(struct kobje
 		pr_debug("kobject: '%s': free name\n", name);
 		kfree_const(name);
 	}
+
+	kobject_put(parent);
 }
 
 #ifdef CONFIG_DEBUG_KOBJECT_RELEASE




^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 2/2] kobject: send KOBJ_REMOVE uevent when the object is removed from sysfs
  2020-05-27  9:01             ` Rafael J. Wysocki
  2020-05-27 14:34               ` Rafael J. Wysocki
@ 2020-05-27 22:25               ` Guenter Roeck
  2020-05-28 10:57                 ` Rafael J. Wysocki
  1 sibling, 1 reply; 20+ messages in thread
From: Guenter Roeck @ 2020-05-27 22:25 UTC (permalink / raw)
  To: Rafael J. Wysocki
  Cc: linux-kernel, Rafael J. Wysocki, Heikki Krogerus,
	Greg Kroah-Hartman, Dmitry Torokhov

On Wed, May 27, 2020 at 11:01:16AM +0200, Rafael J. Wysocki wrote:
> 
> So Guenter, can you please test the patch below to see if it still introduces
> the problems seen by you on ARM?
> 

arm64 and arm64be boot tests pass with the patch below. Some arm boot
tests fail, but I think that is due to some other problem with -next.
Hard to say for sure at this point because -next is pretty badly broken
overall. I'll need to run some bisects to see what is going on.

Guenter

> ---
> From: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> Subject: [PATCH] kobject: Make sure the parent does not get released before its children
> 
> In the function kobject_cleanup(), kobject_del(kobj) is
> called before the kobj->release(). That makes it possible to
> release the parent of the kobject before the kobject itself.
> 
> To fix that, adding function __kboject_del() that does
> everything that kobject_del() does except release the parent
> reference. kobject_cleanup() then calls __kobject_del()
> instead of kobject_del(), and separately decrements the
> reference count of the parent kobject after kobj->release()
> has been called.
> 
> Reported-by: Naresh Kamboju <naresh.kamboju@linaro.org>
> Reported-by: kernel test robot <rong.a.chen@intel.com>
> Fixes: 7589238a8cf3 ("Revert "software node: Simplify software_node_release() function"")
> Suggested-by: "Rafael J. Wysocki" <rafael@kernel.org>
> Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> [ rjw: Drop parent reference only when called __kobject_del() ]
> Signed-off-by: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>
> ---
>  lib/kobject.c |   34 +++++++++++++++++++++++-----------
>  1 file changed, 23 insertions(+), 11 deletions(-)
> 
> Index: linux-pm/lib/kobject.c
> ===================================================================
> --- linux-pm.orig/lib/kobject.c
> +++ linux-pm/lib/kobject.c
> @@ -599,14 +599,7 @@ out:
>  }
>  EXPORT_SYMBOL_GPL(kobject_move);
>  
> -/**
> - * kobject_del() - Unlink kobject from hierarchy.
> - * @kobj: object.
> - *
> - * This is the function that should be called to delete an object
> - * successfully added via kobject_add().
> - */
> -void kobject_del(struct kobject *kobj)
> +static void __kobject_del(struct kobject *kobj)
>  {
>  	struct kernfs_node *sd;
>  	const struct kobj_type *ktype;
> @@ -625,9 +618,23 @@ void kobject_del(struct kobject *kobj)
>  
>  	kobj->state_in_sysfs = 0;
>  	kobj_kset_leave(kobj);
> -	kobject_put(kobj->parent);
>  	kobj->parent = NULL;
>  }
> +
> +/**
> + * kobject_del() - Unlink kobject from hierarchy.
> + * @kobj: object.
> + *
> + * This is the function that should be called to delete an object
> + * successfully added via kobject_add().
> + */
> +void kobject_del(struct kobject *kobj)
> +{
> +	struct kobject *parent = kobj->parent;
> +
> +	__kobject_del(kobj);
> +	kobject_put(parent);
> +}
>  EXPORT_SYMBOL(kobject_del);
>  
>  /**
> @@ -663,7 +670,9 @@ EXPORT_SYMBOL(kobject_get_unless_zero);
>   */
>  static void kobject_cleanup(struct kobject *kobj)
>  {
> +	struct kobject *parent = kobj->parent;
>  	struct kobj_type *t = get_ktype(kobj);
> +	bool state_in_sysfs = kobj->state_in_sysfs;
>  	const char *name = kobj->name;
>  
>  	pr_debug("kobject: '%s' (%p): %s, parent %p\n",
> @@ -681,10 +690,10 @@ static void kobject_cleanup(struct kobje
>  	}
>  
>  	/* remove from sysfs if the caller did not do it */
> -	if (kobj->state_in_sysfs) {
> +	if (state_in_sysfs) {
>  		pr_debug("kobject: '%s' (%p): auto cleanup kobject_del\n",
>  			 kobject_name(kobj), kobj);
> -		kobject_del(kobj);
> +		__kobject_del(kobj);
>  	}
>  
>  	if (t && t->release) {
> @@ -698,6 +707,9 @@ static void kobject_cleanup(struct kobje
>  		pr_debug("kobject: '%s': free name\n", name);
>  		kfree_const(name);
>  	}
> +
> +	if (state_in_sysfs)
> +		kobject_put(parent);
>  }
>  
>  #ifdef CONFIG_DEBUG_KOBJECT_RELEASE
> 
> 
> 

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 2/2] kobject: send KOBJ_REMOVE uevent when the object is removed from sysfs
  2020-05-27 22:25               ` Guenter Roeck
@ 2020-05-28 10:57                 ` Rafael J. Wysocki
  2020-05-28 13:56                   ` Guenter Roeck
  0 siblings, 1 reply; 20+ messages in thread
From: Rafael J. Wysocki @ 2020-05-28 10:57 UTC (permalink / raw)
  To: Guenter Roeck
  Cc: Rafael J. Wysocki, Linux Kernel Mailing List, Rafael J. Wysocki,
	Heikki Krogerus, Greg Kroah-Hartman, Dmitry Torokhov

On Thu, May 28, 2020 at 12:25 AM Guenter Roeck <linux@roeck-us.net> wrote:
>
> On Wed, May 27, 2020 at 11:01:16AM +0200, Rafael J. Wysocki wrote:
> >
> > So Guenter, can you please test the patch below to see if it still introduces
> > the problems seen by you on ARM?
> >
>
> arm64 and arm64be boot tests pass with the patch below.

Great, thanks!

> Some arm boot tests fail, but I think that is due to some other problem with -next.
> Hard to say for sure at this point because -next is pretty badly broken
> overall. I'll need to run some bisects to see what is going on.

I see.

Thanks for giving this one a go.

> > ---
> > From: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > Subject: [PATCH] kobject: Make sure the parent does not get released before its children
> >
> > In the function kobject_cleanup(), kobject_del(kobj) is
> > called before the kobj->release(). That makes it possible to
> > release the parent of the kobject before the kobject itself.
> >
> > To fix that, adding function __kboject_del() that does
> > everything that kobject_del() does except release the parent
> > reference. kobject_cleanup() then calls __kobject_del()
> > instead of kobject_del(), and separately decrements the
> > reference count of the parent kobject after kobj->release()
> > has been called.
> >
> > Reported-by: Naresh Kamboju <naresh.kamboju@linaro.org>
> > Reported-by: kernel test robot <rong.a.chen@intel.com>
> > Fixes: 7589238a8cf3 ("Revert "software node: Simplify software_node_release() function"")
> > Suggested-by: "Rafael J. Wysocki" <rafael@kernel.org>
> > Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > [ rjw: Drop parent reference only when called __kobject_del() ]
> > Signed-off-by: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>
> > ---
> >  lib/kobject.c |   34 +++++++++++++++++++++++-----------
> >  1 file changed, 23 insertions(+), 11 deletions(-)
> >
> > Index: linux-pm/lib/kobject.c
> > ===================================================================
> > --- linux-pm.orig/lib/kobject.c
> > +++ linux-pm/lib/kobject.c
> > @@ -599,14 +599,7 @@ out:
> >  }
> >  EXPORT_SYMBOL_GPL(kobject_move);
> >
> > -/**
> > - * kobject_del() - Unlink kobject from hierarchy.
> > - * @kobj: object.
> > - *
> > - * This is the function that should be called to delete an object
> > - * successfully added via kobject_add().
> > - */
> > -void kobject_del(struct kobject *kobj)
> > +static void __kobject_del(struct kobject *kobj)
> >  {
> >       struct kernfs_node *sd;
> >       const struct kobj_type *ktype;
> > @@ -625,9 +618,23 @@ void kobject_del(struct kobject *kobj)
> >
> >       kobj->state_in_sysfs = 0;
> >       kobj_kset_leave(kobj);
> > -     kobject_put(kobj->parent);
> >       kobj->parent = NULL;
> >  }
> > +
> > +/**
> > + * kobject_del() - Unlink kobject from hierarchy.
> > + * @kobj: object.
> > + *
> > + * This is the function that should be called to delete an object
> > + * successfully added via kobject_add().
> > + */
> > +void kobject_del(struct kobject *kobj)
> > +{
> > +     struct kobject *parent = kobj->parent;
> > +
> > +     __kobject_del(kobj);
> > +     kobject_put(parent);
> > +}
> >  EXPORT_SYMBOL(kobject_del);
> >
> >  /**
> > @@ -663,7 +670,9 @@ EXPORT_SYMBOL(kobject_get_unless_zero);
> >   */
> >  static void kobject_cleanup(struct kobject *kobj)
> >  {
> > +     struct kobject *parent = kobj->parent;
> >       struct kobj_type *t = get_ktype(kobj);
> > +     bool state_in_sysfs = kobj->state_in_sysfs;
> >       const char *name = kobj->name;
> >
> >       pr_debug("kobject: '%s' (%p): %s, parent %p\n",
> > @@ -681,10 +690,10 @@ static void kobject_cleanup(struct kobje
> >       }
> >
> >       /* remove from sysfs if the caller did not do it */
> > -     if (kobj->state_in_sysfs) {
> > +     if (state_in_sysfs) {
> >               pr_debug("kobject: '%s' (%p): auto cleanup kobject_del\n",
> >                        kobject_name(kobj), kobj);
> > -             kobject_del(kobj);
> > +             __kobject_del(kobj);
> >       }
> >
> >       if (t && t->release) {
> > @@ -698,6 +707,9 @@ static void kobject_cleanup(struct kobje
> >               pr_debug("kobject: '%s': free name\n", name);
> >               kfree_const(name);
> >       }
> > +
> > +     if (state_in_sysfs)
> > +             kobject_put(parent);
> >  }
> >
> >  #ifdef CONFIG_DEBUG_KOBJECT_RELEASE
> >
> >
> >

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 2/2] kobject: send KOBJ_REMOVE uevent when the object is removed from sysfs
  2020-05-28 10:57                 ` Rafael J. Wysocki
@ 2020-05-28 13:56                   ` Guenter Roeck
  2020-05-28 14:03                     ` Rafael J. Wysocki
  0 siblings, 1 reply; 20+ messages in thread
From: Guenter Roeck @ 2020-05-28 13:56 UTC (permalink / raw)
  To: Rafael J. Wysocki
  Cc: Rafael J. Wysocki, Linux Kernel Mailing List, Heikki Krogerus,
	Greg Kroah-Hartman, Dmitry Torokhov

On 5/28/20 3:57 AM, Rafael J. Wysocki wrote:
> On Thu, May 28, 2020 at 12:25 AM Guenter Roeck <linux@roeck-us.net> wrote:
>>
>> On Wed, May 27, 2020 at 11:01:16AM +0200, Rafael J. Wysocki wrote:
>>>
>>> So Guenter, can you please test the patch below to see if it still introduces
>>> the problems seen by you on ARM?
>>>
>>
>> arm64 and arm64be boot tests pass with the patch below.
> 
> Great, thanks!
> 
>> Some arm boot tests fail, but I think that is due to some other problem with -next.
>> Hard to say for sure at this point because -next is pretty badly broken
>> overall. I'll need to run some bisects to see what is going on.
> 
> I see.
> 

The failing arm boot tests are due to various dts changes (commit "arm64:
dts: vexpress: Move fixed devices out of bus node" and associated),
unrelated to this patch.

Guenter

> Thanks for giving this one a go.
> 
>>> ---
>>> From: Heikki Krogerus <heikki.krogerus@linux.intel.com>
>>> Subject: [PATCH] kobject: Make sure the parent does not get released before its children
>>>
>>> In the function kobject_cleanup(), kobject_del(kobj) is
>>> called before the kobj->release(). That makes it possible to
>>> release the parent of the kobject before the kobject itself.
>>>
>>> To fix that, adding function __kboject_del() that does
>>> everything that kobject_del() does except release the parent
>>> reference. kobject_cleanup() then calls __kobject_del()
>>> instead of kobject_del(), and separately decrements the
>>> reference count of the parent kobject after kobj->release()
>>> has been called.
>>>
>>> Reported-by: Naresh Kamboju <naresh.kamboju@linaro.org>
>>> Reported-by: kernel test robot <rong.a.chen@intel.com>
>>> Fixes: 7589238a8cf3 ("Revert "software node: Simplify software_node_release() function"")
>>> Suggested-by: "Rafael J. Wysocki" <rafael@kernel.org>
>>> Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
>>> [ rjw: Drop parent reference only when called __kobject_del() ]
>>> Signed-off-by: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>
>>> ---
>>>  lib/kobject.c |   34 +++++++++++++++++++++++-----------
>>>  1 file changed, 23 insertions(+), 11 deletions(-)
>>>
>>> Index: linux-pm/lib/kobject.c
>>> ===================================================================
>>> --- linux-pm.orig/lib/kobject.c
>>> +++ linux-pm/lib/kobject.c
>>> @@ -599,14 +599,7 @@ out:
>>>  }
>>>  EXPORT_SYMBOL_GPL(kobject_move);
>>>
>>> -/**
>>> - * kobject_del() - Unlink kobject from hierarchy.
>>> - * @kobj: object.
>>> - *
>>> - * This is the function that should be called to delete an object
>>> - * successfully added via kobject_add().
>>> - */
>>> -void kobject_del(struct kobject *kobj)
>>> +static void __kobject_del(struct kobject *kobj)
>>>  {
>>>       struct kernfs_node *sd;
>>>       const struct kobj_type *ktype;
>>> @@ -625,9 +618,23 @@ void kobject_del(struct kobject *kobj)
>>>
>>>       kobj->state_in_sysfs = 0;
>>>       kobj_kset_leave(kobj);
>>> -     kobject_put(kobj->parent);
>>>       kobj->parent = NULL;
>>>  }
>>> +
>>> +/**
>>> + * kobject_del() - Unlink kobject from hierarchy.
>>> + * @kobj: object.
>>> + *
>>> + * This is the function that should be called to delete an object
>>> + * successfully added via kobject_add().
>>> + */
>>> +void kobject_del(struct kobject *kobj)
>>> +{
>>> +     struct kobject *parent = kobj->parent;
>>> +
>>> +     __kobject_del(kobj);
>>> +     kobject_put(parent);
>>> +}
>>>  EXPORT_SYMBOL(kobject_del);
>>>
>>>  /**
>>> @@ -663,7 +670,9 @@ EXPORT_SYMBOL(kobject_get_unless_zero);
>>>   */
>>>  static void kobject_cleanup(struct kobject *kobj)
>>>  {
>>> +     struct kobject *parent = kobj->parent;
>>>       struct kobj_type *t = get_ktype(kobj);
>>> +     bool state_in_sysfs = kobj->state_in_sysfs;
>>>       const char *name = kobj->name;
>>>
>>>       pr_debug("kobject: '%s' (%p): %s, parent %p\n",
>>> @@ -681,10 +690,10 @@ static void kobject_cleanup(struct kobje
>>>       }
>>>
>>>       /* remove from sysfs if the caller did not do it */
>>> -     if (kobj->state_in_sysfs) {
>>> +     if (state_in_sysfs) {
>>>               pr_debug("kobject: '%s' (%p): auto cleanup kobject_del\n",
>>>                        kobject_name(kobj), kobj);
>>> -             kobject_del(kobj);
>>> +             __kobject_del(kobj);
>>>       }
>>>
>>>       if (t && t->release) {
>>> @@ -698,6 +707,9 @@ static void kobject_cleanup(struct kobje
>>>               pr_debug("kobject: '%s': free name\n", name);
>>>               kfree_const(name);
>>>       }
>>> +
>>> +     if (state_in_sysfs)
>>> +             kobject_put(parent);
>>>  }
>>>
>>>  #ifdef CONFIG_DEBUG_KOBJECT_RELEASE
>>>
>>>
>>>


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 2/2] kobject: send KOBJ_REMOVE uevent when the object is removed from sysfs
  2020-05-28 13:56                   ` Guenter Roeck
@ 2020-05-28 14:03                     ` Rafael J. Wysocki
  0 siblings, 0 replies; 20+ messages in thread
From: Rafael J. Wysocki @ 2020-05-28 14:03 UTC (permalink / raw)
  To: Guenter Roeck
  Cc: Rafael J. Wysocki, Rafael J. Wysocki, Linux Kernel Mailing List,
	Heikki Krogerus, Greg Kroah-Hartman, Dmitry Torokhov

On Thu, May 28, 2020 at 3:56 PM Guenter Roeck <linux@roeck-us.net> wrote:
>
> On 5/28/20 3:57 AM, Rafael J. Wysocki wrote:
> > On Thu, May 28, 2020 at 12:25 AM Guenter Roeck <linux@roeck-us.net> wrote:
> >>
> >> On Wed, May 27, 2020 at 11:01:16AM +0200, Rafael J. Wysocki wrote:
> >>>
> >>> So Guenter, can you please test the patch below to see if it still introduces
> >>> the problems seen by you on ARM?
> >>>
> >>
> >> arm64 and arm64be boot tests pass with the patch below.
> >
> > Great, thanks!
> >
> >> Some arm boot tests fail, but I think that is due to some other problem with -next.
> >> Hard to say for sure at this point because -next is pretty badly broken
> >> overall. I'll need to run some bisects to see what is going on.
> >
> > I see.
> >
>
> The failing arm boot tests are due to various dts changes (commit "arm64:
> dts: vexpress: Move fixed devices out of bus node" and associated),
> unrelated to this patch.

Thanks for the confirmation!

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 1/2] software node: implement software_node_unregister()
  2020-05-24 15:30 [PATCH 1/2] software node: implement software_node_unregister() Greg Kroah-Hartman
                   ` (4 preceding siblings ...)
  2020-05-26  0:12 ` Randy Dunlap
@ 2020-05-28 20:25 ` Brendan Higgins
  5 siblings, 0 replies; 20+ messages in thread
From: Brendan Higgins @ 2020-05-28 20:25 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Linux Kernel Mailing List, Guenter Roeck, Naresh Kamboju,
	kernel test robot, stable, Andy Shevchenko, Dmitry Torokhov,
	Heikki Krogerus, Petr Mladek, Rafael J . Wysocki, Randy Dunlap,
	Rasmus Villemoes, Sakari Ailus, Sergey Senozhatsky,
	Steven Rostedt

On Sun, May 24, 2020 at 8:31 AM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> Sometimes it is better to unregister individual nodes instead of trying
> to do them all at once with software_node_unregister_nodes(), so create
> software_node_unregister() so that you can unregister them one at a
> time.
>
> This is especially important when creating nodes in a hierarchy, with
> parent -> children representations.  Children always need to be removed
> before a parent is, as the swnode logic assumes this is going to be the
> case.
>
> Fix up the lib/test_printf.c fwnode_pointer() test which to use this new
> function as it had the problem of tearing things down in the backwards
> order.
>
> Fixes: f1ce39df508d ("lib/test_printf: Add tests for %pfw printk modifier")
> Reported-by: Naresh Kamboju <naresh.kamboju@linaro.org>
> Reported-by: kernel test robot <rong.a.chen@intel.com>
> Cc: stable <stable@vger.kernel.org>
> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Cc: Brendan Higgins <brendanhiggins@google.com>
> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> Cc: Petr Mladek <pmladek@suse.com>
> Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Cc: Randy Dunlap <rdunlap@infradead.org>
> Cc: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
> Cc: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> Cc: Steven Rostedt <rostedt@goodmis.org>
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Sorry, I was on vacation.

Seems like this has already been sufficiently reviewed and applied.
Nevertheless,

Tested-by: Brendan Higgins <brendanhiggins@google.com>

^ permalink raw reply	[flat|nested] 20+ messages in thread

end of thread, other threads:[~2020-05-28 20:25 UTC | newest]

Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-24 15:30 [PATCH 1/2] software node: implement software_node_unregister() Greg Kroah-Hartman
2020-05-24 15:30 ` [PATCH 2/2] kobject: send KOBJ_REMOVE uevent when the object is removed from sysfs Greg Kroah-Hartman
2020-05-25 11:07   ` Rafael J. Wysocki
2020-05-25 22:49   ` Dmitry Torokhov
2020-05-26  5:58     ` Greg Kroah-Hartman
2020-05-26  8:26       ` Rafael J. Wysocki
2020-05-27  7:50         ` Heikki Krogerus
2020-05-27  8:34           ` Rafael J. Wysocki
2020-05-27  9:01             ` Rafael J. Wysocki
2020-05-27 14:34               ` Rafael J. Wysocki
2020-05-27 22:25               ` Guenter Roeck
2020-05-28 10:57                 ` Rafael J. Wysocki
2020-05-28 13:56                   ` Guenter Roeck
2020-05-28 14:03                     ` Rafael J. Wysocki
2020-05-24 16:43 ` [PATCH 1/2] software node: implement software_node_unregister() Guenter Roeck
2020-05-25  2:44   ` Randy Dunlap
2020-05-25  7:59 ` Petr Mladek
2020-05-25 11:24 ` Heikki Krogerus
2020-05-26  0:12 ` Randy Dunlap
2020-05-28 20:25 ` Brendan Higgins

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.