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 <andriy.shevche...@linux.intel.com>
>> Signed-off-by: Daniel Scally <djrsca...@gmail.com>
>> ---
>> 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 <laurent.pinch...@ideasonboard.com>
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);
>
