* [PATCH] software node: recursively unregister child swnodes
@ 2020-06-04 19:36 jorhand
2020-06-04 20:15 ` Greg Kroah-Hartman
0 siblings, 1 reply; 6+ messages in thread
From: jorhand @ 2020-06-04 19:36 UTC (permalink / raw)
To: Heikki Krogerus, Rafael J . Wysocki
Cc: Greg Kroah-Hartman, linux-kernel, linux-next, Jordan Hand
From: Jordan Hand <jorhand@linux.microsoft.com>
If a child swnode is unregistered after it's parent, it can lead to
undefined behavior.
When a swnode is unregistered, recursively free it's children to avoid
this condition.
Signed-off-by: Jordan Hand <jorhand@linux.microsoft.com>
---
drivers/base/swnode.c | 13 ++++++++-----
1 file changed, 8 insertions(+), 5 deletions(-)
diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
index e5eb27375416..e63093b1542b 100644
--- a/drivers/base/swnode.c
+++ b/drivers/base/swnode.c
@@ -619,6 +619,8 @@ static void software_node_release(struct kobject *kobj)
property_entries_free(swnode->node->properties);
kfree(swnode->node);
}
+
+ list_del(&kobj->entry);
ida_destroy(&swnode->child_ids);
kfree(swnode);
}
@@ -712,11 +714,6 @@ 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)
{
@@ -839,10 +836,16 @@ EXPORT_SYMBOL_GPL(fwnode_create_software_node);
void fwnode_remove_software_node(struct fwnode_handle *fwnode)
{
struct swnode *swnode = to_swnode(fwnode);
+ struct swnode *child = NULL;
if (!swnode)
return;
+ while (!list_empty(&swnode->children)) {
+ child = list_first_entry_or_null(&swnode->children, struct swnode, entry);
+ fwnode_remove_software_node(&child->fwnode);
+ }
+
kobject_put(&swnode->kobj);
}
EXPORT_SYMBOL_GPL(fwnode_remove_software_node);
--
2.17.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] software node: recursively unregister child swnodes
2020-06-04 19:36 [PATCH] software node: recursively unregister child swnodes jorhand
@ 2020-06-04 20:15 ` Greg Kroah-Hartman
2020-06-04 20:57 ` Jordan Hand
0 siblings, 1 reply; 6+ messages in thread
From: Greg Kroah-Hartman @ 2020-06-04 20:15 UTC (permalink / raw)
To: jorhand; +Cc: Heikki Krogerus, Rafael J . Wysocki, linux-kernel, linux-next
On Thu, Jun 04, 2020 at 12:36:23PM -0700, jorhand@linux.microsoft.com wrote:
> From: Jordan Hand <jorhand@linux.microsoft.com>
>
> If a child swnode is unregistered after it's parent, it can lead to
> undefined behavior.
Crashing the system is not really "undefined" :)
> When a swnode is unregistered, recursively free it's children to avoid
> this condition.
Are you sure? Why would you be unregistering a child after it's parent?
Why not just do not do that?
>
> Signed-off-by: Jordan Hand <jorhand@linux.microsoft.com>
> ---
> drivers/base/swnode.c | 13 ++++++++-----
> 1 file changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
> index e5eb27375416..e63093b1542b 100644
> --- a/drivers/base/swnode.c
> +++ b/drivers/base/swnode.c
> @@ -619,6 +619,8 @@ static void software_node_release(struct kobject *kobj)
> property_entries_free(swnode->node->properties);
> kfree(swnode->node);
> }
> +
> + list_del(&kobj->entry);
> ida_destroy(&swnode->child_ids);
> kfree(swnode);
> }
> @@ -712,11 +714,6 @@ 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)
> {
> @@ -839,10 +836,16 @@ EXPORT_SYMBOL_GPL(fwnode_create_software_node);
> void fwnode_remove_software_node(struct fwnode_handle *fwnode)
> {
> struct swnode *swnode = to_swnode(fwnode);
> + struct swnode *child = NULL;
>
> if (!swnode)
> return;
>
> + while (!list_empty(&swnode->children)) {
> + child = list_first_entry_or_null(&swnode->children, struct swnode, entry);
> + fwnode_remove_software_node(&child->fwnode);
You should document that you just changed the behaivor here, as you are
now really doing fwnode_remove_software_node_and_all_children().
but again, why? Who wants to unregister a child before a parent?
thanks,
greg k-h
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] software node: recursively unregister child swnodes
2020-06-04 20:15 ` Greg Kroah-Hartman
@ 2020-06-04 20:57 ` Jordan Hand
2020-06-04 23:28 ` Jordan Hand
2020-06-05 7:54 ` Greg Kroah-Hartman
0 siblings, 2 replies; 6+ messages in thread
From: Jordan Hand @ 2020-06-04 20:57 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Heikki Krogerus, Rafael J . Wysocki, linux-kernel, linux-next
On 6/4/20 1:15 PM, Greg Kroah-Hartman wrote:
> On Thu, Jun 04, 2020 at 12:36:23PM -0700, jorhand@linux.microsoft.com wrote:
>> From: Jordan Hand <jorhand@linux.microsoft.com>
>>
>> If a child swnode is unregistered after it's parent, it can lead to
>> undefined behavior.
>
> Crashing the system is not really "undefined" :)
Fair point :)
>
>> When a swnode is unregistered, recursively free it's children to avoid
>> this condition.
>
> Are you sure? Why would you be unregistering a child after it's parent?
> Why not just do not do that?
>
The main motivation for doing this was to support
`software_node_unregister_nodes` so that the passed list of nodes does
not need to be ordered in any particular way.
I suppose another way to do this would be to add a new function
`fwnode_remove_software_node_recursive` and just call that from
`software_node_unregister_nodes`.
That said, I suppose just ordering the nodes so that children come
before parents would also be fine. My thinking was just that accepting
any node ordering is simpler.
Thanks,
Jordan
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] software node: recursively unregister child swnodes
2020-06-04 20:57 ` Jordan Hand
@ 2020-06-04 23:28 ` Jordan Hand
2020-06-05 7:54 ` Greg Kroah-Hartman
1 sibling, 0 replies; 6+ messages in thread
From: Jordan Hand @ 2020-06-04 23:28 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Heikki Krogerus, Rafael J . Wysocki, linux-kernel, linux-next
On 6/4/20 1:57 PM, Jordan Hand wrote:
> On 6/4/20 1:15 PM, Greg Kroah-Hartman wrote:
>> On Thu, Jun 04, 2020 at 12:36:23PM -0700, jorhand@linux.microsoft.com
>
> That said, I suppose just ordering the nodes so that children come
> before parents would also be fine. My thinking was just that accepting
> any node ordering is simpler.
>
Oh, actually I just tried that out and software_node_register_nodes
doesn't allow this (parents must be added before children).
So I still think software_node_register_nodes and
software_node_unregister_nodes are made more useful if children are
removed recursively. I'll make some changes for v2 to make the change
less far-reaching + better documentation.
Thanks,
Jordan
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] software node: recursively unregister child swnodes
2020-06-04 20:57 ` Jordan Hand
2020-06-04 23:28 ` Jordan Hand
@ 2020-06-05 7:54 ` Greg Kroah-Hartman
2020-06-05 16:20 ` Jordan Hand
1 sibling, 1 reply; 6+ messages in thread
From: Greg Kroah-Hartman @ 2020-06-05 7:54 UTC (permalink / raw)
To: Jordan Hand; +Cc: Heikki Krogerus, Rafael J . Wysocki, linux-kernel, linux-next
On Thu, Jun 04, 2020 at 01:57:01PM -0700, Jordan Hand wrote:
> On 6/4/20 1:15 PM, Greg Kroah-Hartman wrote:
> > On Thu, Jun 04, 2020 at 12:36:23PM -0700, jorhand@linux.microsoft.com wrote:
> > > From: Jordan Hand <jorhand@linux.microsoft.com>
> > >
> > > If a child swnode is unregistered after it's parent, it can lead to
> > > undefined behavior.
> >
> > Crashing the system is not really "undefined" :)
>
> Fair point :)
>
> >
> > > When a swnode is unregistered, recursively free it's children to avoid
> > > this condition.
> >
> > Are you sure? Why would you be unregistering a child after it's parent?
> > Why not just do not do that?
> >
>
> The main motivation for doing this was to support
> `software_node_unregister_nodes` so that the passed list of nodes does not
> need to be ordered in any particular way.
>
> I suppose another way to do this would be to add a new function
> `fwnode_remove_software_node_recursive` and just call that from
> `software_node_unregister_nodes`.
>
> That said, I suppose just ordering the nodes so that children come before
> parents would also be fine. My thinking was just that accepting any node
> ordering is simpler.
Right now, the way the driver model and sysfs/kobjects work is that all
objects must be removed in child-first order. The problem of your
change where you want to try to remove the devices in parent-first order
is that you do not really know if you still have a reference to a child
device somewhere else, which would prevent this all from happening
correctly, right?
So if you "know" it is safe to drop a child, that's great, and expected.
Don't work to make this one tiny user of the kobjects (which I'm still
not quite sure why they are kobjects and not devices), do things in a
different way from the rest of the kernel without a strong reason to do
so.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] software node: recursively unregister child swnodes
2020-06-05 7:54 ` Greg Kroah-Hartman
@ 2020-06-05 16:20 ` Jordan Hand
0 siblings, 0 replies; 6+ messages in thread
From: Jordan Hand @ 2020-06-05 16:20 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Heikki Krogerus, Rafael J . Wysocki, linux-kernel, linux-next
On 6/5/20 12:54 AM, Greg Kroah-Hartman wrote:
> Right now, the way the driver model and sysfs/kobjects work is that all
> objects must be removed in child-first order. The problem of your
> change where you want to try to remove the devices in parent-first order
> is that you do not really know if you still have a reference to a child
> device somewhere else, which would prevent this all from happening
> correctly, right?
>
> So if you "know" it is safe to drop a child, that's great, and expected.
> Don't work to make this one tiny user of the kobjects (which I'm still
> not quite sure why they are kobjects and not devices), do things in a
> different way from the rest of the kernel without a strong reason to do
> so.
>
> thanks,
>
> greg k-h
>
I see, thanks for taking the time to explain, the reason for the
existing behavior is more clear to me now. I agree it is better to have
the caller remove the nodes in the correct order rather than having the
swnode infrastructure try to have some special behavior.
Thanks,
Jordan
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-06-05 16:20 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-04 19:36 [PATCH] software node: recursively unregister child swnodes jorhand
2020-06-04 20:15 ` Greg Kroah-Hartman
2020-06-04 20:57 ` Jordan Hand
2020-06-04 23:28 ` Jordan Hand
2020-06-05 7:54 ` Greg Kroah-Hartman
2020-06-05 16:20 ` Jordan Hand
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).