Registering software_nodes with the .parent member set to point to a
currently unregistered software_node has the potential for problems,
so enforce parent -> child ordering in arrays passed in to
software_node_register_nodes().
Software nodes that are children of another software node should be
unregistered before their parent. To allow easy unregistering of an array
of software_nodes ordered parent to child, reverse the order in which
software_node_unregister_nodes() unregisters software_nodes.
Suggested-by: Andy Shevchenko <[email protected]>
Signed-off-by: Daniel Scally <[email protected]>
---
Changes in v2:
- Squashed the patches that originally touched these separately
- Updated documentation
drivers/base/swnode.c | 43 ++++++++++++++++++++++++++++++-------------
1 file changed, 30 insertions(+), 13 deletions(-)
diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
index 615a0c93e116..cfd1faea48a7 100644
--- a/drivers/base/swnode.c
+++ b/drivers/base/swnode.c
@@ -692,7 +692,10 @@ swnode_register(const struct software_node *node, struct swnode *parent,
* software_node_register_nodes - Register an array of software nodes
* @nodes: Zero terminated array of software nodes to be registered
*
- * Register multiple software nodes at once.
+ * Register multiple software nodes at once. If any node in the array
+ * has it's .parent pointer set, then it's parent **must** have been
+ * registered before it is; either outside of this function or by
+ * ordering the array such that parent comes before child.
*/
int software_node_register_nodes(const struct software_node *nodes)
{
@@ -700,33 +703,47 @@ int software_node_register_nodes(const struct software_node *nodes)
int i;
for (i = 0; nodes[i].name; i++) {
- ret = software_node_register(&nodes[i]);
- if (ret) {
- software_node_unregister_nodes(nodes);
- return ret;
+ const struct software_node *parent = nodes[i].parent;
+
+ if (parent && !software_node_to_swnode(parent)) {
+ ret = -EINVAL;
+ goto err_unregister_nodes;
}
+
+ ret = software_node_register(&nodes[i]);
+ if (ret)
+ goto err_unregister_nodes;
}
return 0;
+
+err_unregister_nodes:
+ software_node_unregister_nodes(nodes);
+ return ret;
}
EXPORT_SYMBOL_GPL(software_node_register_nodes);
/**
* software_node_unregister_nodes - Unregister an array of software nodes
- * @nodes: Zero terminated array of software nodes to be unregistered
+ * @nodes: Zero terminated array of software nodes to be unregistered.
*
- * Unregister multiple software nodes at once.
+ * Unregister multiple software nodes at once. If parent pointers are set up
+ * in any of the software nodes then the array MUST be ordered such that
+ * parents come before their children.
*
- * 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.
+ * NOTE: If you are uncertain whether the array is ordered such that
+ * parents will be unregistered before their children, it is wiser to
+ * remove the nodes individually, in the correct order (child before
+ * parent).
*/
void software_node_unregister_nodes(const struct software_node *nodes)
{
- int i;
+ unsigned int i = 0;
+
+ while (nodes[i].name)
+ i++;
- for (i = 0; nodes[i].name; i++)
+ while (i--)
software_node_unregister(&nodes[i]);
}
EXPORT_SYMBOL_GPL(software_node_unregister_nodes);
--
2.25.1
Hi Daniel,
Thank you for the patch.
On Thu, Dec 17, 2020 at 11:43:29PM +0000, Daniel Scally wrote:
> Registering software_nodes with the .parent member set to point to a
> currently unregistered software_node has the potential for problems,
> so enforce parent -> child ordering in arrays passed in to
> software_node_register_nodes().
>
> Software nodes that are children of another software node should be
> unregistered before their parent. To allow easy unregistering of an array
> of software_nodes ordered parent to child, reverse the order in which
> software_node_unregister_nodes() unregisters software_nodes.
>
> Suggested-by: Andy Shevchenko <[email protected]>
> Signed-off-by: Daniel Scally <[email protected]>
> ---
> Changes in v2:
>
> - Squashed the patches that originally touched these separately
> - Updated documentation
>
> drivers/base/swnode.c | 43 ++++++++++++++++++++++++++++++-------------
> 1 file changed, 30 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
> index 615a0c93e116..cfd1faea48a7 100644
> --- a/drivers/base/swnode.c
> +++ b/drivers/base/swnode.c
> @@ -692,7 +692,10 @@ swnode_register(const struct software_node *node, struct swnode *parent,
> * software_node_register_nodes - Register an array of software nodes
> * @nodes: Zero terminated array of software nodes to be registered
> *
> - * Register multiple software nodes at once.
> + * Register multiple software nodes at once. If any node in the array
> + * has it's .parent pointer set, then it's parent **must** have been
> + * registered before it is; either outside of this function or by
> + * ordering the array such that parent comes before child.
> */
> int software_node_register_nodes(const struct software_node *nodes)
> {
> @@ -700,33 +703,47 @@ int software_node_register_nodes(const struct software_node *nodes)
> int i;
>
> for (i = 0; nodes[i].name; i++) {
> - ret = software_node_register(&nodes[i]);
> - if (ret) {
> - software_node_unregister_nodes(nodes);
> - return ret;
> + const struct software_node *parent = nodes[i].parent;
> +
> + if (parent && !software_node_to_swnode(parent)) {
> + ret = -EINVAL;
> + goto err_unregister_nodes;
> }
> +
> + ret = software_node_register(&nodes[i]);
> + if (ret)
> + goto err_unregister_nodes;
> }
>
> return 0;
> +
> +err_unregister_nodes:
> + software_node_unregister_nodes(nodes);
> + return ret;
> }
> EXPORT_SYMBOL_GPL(software_node_register_nodes);
>
> /**
> * software_node_unregister_nodes - Unregister an array of software nodes
> - * @nodes: Zero terminated array of software nodes to be unregistered
> + * @nodes: Zero terminated array of software nodes to be unregistered.
Not sure if this is needed.
> *
> - * Unregister multiple software nodes at once.
> + * Unregister multiple software nodes at once. If parent pointers are set up
> + * in any of the software nodes then the array MUST be ordered such that
I'd either replace **must** above with MUST, or use **must** here. I'm
not sure if kerneldoc handles emphasis with **must**, if it does that
seems a bit nicer to me, but it's really up to you.
Reviewed-by: Laurent Pinchart <[email protected]>
> + * parents come before their children.
> *
> - * 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.
> + * NOTE: If you are uncertain whether the array is ordered such that
> + * parents will be unregistered before their children, it is wiser to
> + * remove the nodes individually, in the correct order (child before
> + * parent).
> */
> void software_node_unregister_nodes(const struct software_node *nodes)
> {
> - int i;
> + unsigned int i = 0;
> +
> + while (nodes[i].name)
> + i++;
>
> - for (i = 0; nodes[i].name; i++)
> + while (i--)
> software_node_unregister(&nodes[i]);
> }
> EXPORT_SYMBOL_GPL(software_node_unregister_nodes);
--
Regards,
Laurent Pinchart
On Thu, Dec 17, 2020 at 11:43:29PM +0000, Daniel Scally wrote:
> Registering software_nodes with the .parent member set to point to a
> currently unregistered software_node has the potential for problems,
> so enforce parent -> child ordering in arrays passed in to
> software_node_register_nodes().
>
> Software nodes that are children of another software node should be
> unregistered before their parent. To allow easy unregistering of an array
> of software_nodes ordered parent to child, reverse the order in which
> software_node_unregister_nodes() unregisters software_nodes.
...
> + * Register multiple software nodes at once. If any node in the array
> + * has it's .parent pointer set, then it's parent **must** have been
it's => its in both cases?
> + * registered before it is; either outside of this function or by
> + * ordering the array such that parent comes before child.
> */
...
> + const struct software_node *parent = nodes[i].parent;
> +
> + if (parent && !software_node_to_swnode(parent)) {
Can we have parent of swnode in an array not being an swnode?
Either comment that parent for swnode can be swnode only (Heikki, was it an
idea?) or check if parent is of swnode type and only that apply this
requirement.
> + ret = -EINVAL;
> + goto err_unregister_nodes;
> }
...
> + * Unregister multiple software nodes at once. If parent pointers are set up
> + * in any of the software nodes then the array MUST be ordered such that
> + * parents come before their children.
Shouldn't be consistent with above, i.e. **must** ?
--
With Best Regards,
Andy Shevchenko
Hi Laurent
On 18/12/2020 16:02, Laurent Pinchart wrote:
> Hi Daniel,
>
> Thank you for the patch.
>
> On Thu, Dec 17, 2020 at 11:43:29PM +0000, Daniel Scally wrote:
>> Registering software_nodes with the .parent member set to point to a
>> currently unregistered software_node has the potential for problems,
>> so enforce parent -> child ordering in arrays passed in to
>> software_node_register_nodes().
>>
>> Software nodes that are children of another software node should be
>> unregistered before their parent. To allow easy unregistering of an array
>> of software_nodes ordered parent to child, reverse the order in which
>> software_node_unregister_nodes() unregisters software_nodes.
>>
>> Suggested-by: Andy Shevchenko <[email protected]>
>> Signed-off-by: Daniel Scally <[email protected]>
>> ---
>> Changes in v2:
>>
>> - Squashed the patches that originally touched these separately
>> - Updated documentation
>>
>> drivers/base/swnode.c | 43 ++++++++++++++++++++++++++++++-------------
>> 1 file changed, 30 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
>> index 615a0c93e116..cfd1faea48a7 100644
>> --- a/drivers/base/swnode.c
>> +++ b/drivers/base/swnode.c
>> @@ -692,7 +692,10 @@ swnode_register(const struct software_node *node, struct swnode *parent,
>> * software_node_register_nodes - Register an array of software nodes
>> * @nodes: Zero terminated array of software nodes to be registered
>> *
>> - * Register multiple software nodes at once.
>> + * Register multiple software nodes at once. If any node in the array
>> + * has it's .parent pointer set, then it's parent **must** have been
>> + * registered before it is; either outside of this function or by
>> + * ordering the array such that parent comes before child.
>> */
>> int software_node_register_nodes(const struct software_node *nodes)
>> {
>> @@ -700,33 +703,47 @@ int software_node_register_nodes(const struct software_node *nodes)
>> int i;
>>
>> for (i = 0; nodes[i].name; i++) {
>> - ret = software_node_register(&nodes[i]);
>> - if (ret) {
>> - software_node_unregister_nodes(nodes);
>> - return ret;
>> + const struct software_node *parent = nodes[i].parent;
>> +
>> + if (parent && !software_node_to_swnode(parent)) {
>> + ret = -EINVAL;
>> + goto err_unregister_nodes;
>> }
>> +
>> + ret = software_node_register(&nodes[i]);
>> + if (ret)
>> + goto err_unregister_nodes;
>> }
>>
>> return 0;
>> +
>> +err_unregister_nodes:
>> + software_node_unregister_nodes(nodes);
>> + return ret;
>> }
>> EXPORT_SYMBOL_GPL(software_node_register_nodes);
>>
>> /**
>> * software_node_unregister_nodes - Unregister an array of software nodes
>> - * @nodes: Zero terminated array of software nodes to be unregistered
>> + * @nodes: Zero terminated array of software nodes to be unregistered.
>
> Not sure if this is needed.
Hah, of course. Hangover from the last version (when I had made that
line two sentences)
>
>> *
>> - * Unregister multiple software nodes at once.
>> + * Unregister multiple software nodes at once. If parent pointers are set up
>> + * in any of the software nodes then the array MUST be ordered such that
>
> I'd either replace **must** above with MUST, or use **must** here. I'm
> not sure if kerneldoc handles emphasis with **must**, if it does that
> seems a bit nicer to me, but it's really up to you.
Honestly I haven't delved into kerneldoc yet, but either way I think
**must** is better in both places - will change.
> Reviewed-by: Laurent Pinchart <[email protected]>
Thank you!
>
>> + * parents come before their children.
>> *
>> - * 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.
>> + * NOTE: If you are uncertain whether the array is ordered such that
>> + * parents will be unregistered before their children, it is wiser to
>> + * remove the nodes individually, in the correct order (child before
>> + * parent).
>> */
>> void software_node_unregister_nodes(const struct software_node *nodes)
>> {
>> - int i;
>> + unsigned int i = 0;
>> +
>> + while (nodes[i].name)
>> + i++;
>>
>> - for (i = 0; nodes[i].name; i++)
>> + while (i--)
>> software_node_unregister(&nodes[i]);
>> }
>> EXPORT_SYMBOL_GPL(software_node_unregister_nodes);
>
On 18/12/2020 20:29, Andy Shevchenko wrote:
>> + * Register multiple software nodes at once. If any node in the array
>> + * has it's .parent pointer set, then it's parent **must** have been
>
> it's => its in both cases?
Done, ty
>> + * registered before it is; either outside of this function or by
>> + * ordering the array such that parent comes before child.
>> */
>
> ...
>
>> + const struct software_node *parent = nodes[i].parent;
>> +
>> + if (parent && !software_node_to_swnode(parent)) {
>
> Can we have parent of swnode in an array not being an swnode?
> Either comment that parent for swnode can be swnode only (Heikki, was it an
> idea?) or check if parent is of swnode type and only that apply this
> requirement.
.parent can be a pointer to software_node only yes; I can add that to
the document comment.
>> + ret = -EINVAL;
>> + goto err_unregister_nodes;
>> }
>
> ...
>
>> + * Unregister multiple software nodes at once. If parent pointers are set up
>> + * in any of the software nodes then the array MUST be ordered such that
>> + * parents come before their children.
>
> Shouldn't be consistent with above, i.e. **must** ?
Done also