Date:   Fri, 18 Dec 2020 22:19:05 +0000
From:   Daniel Scally <djrscally@...il.com>
To:     Laurent Pinchart <laurent.pinchart@...asonboard.com>
Cc:     linux-kernel@...r.kernel.org, linux-acpi@...r.kernel.org,
        linux-media@...r.kernel.org, devel@...ica.org, rjw@...ysocki.net,
        lenb@...nel.org, gregkh@...uxfoundation.org, yong.zhi@...el.com,
        sakari.ailus@...ux.intel.com, bingbu.cao@...el.com,
        tian.shu.qiu@...el.com, mchehab@...nel.org, robert.moore@...el.com,
        erik.kaneda@...el.com, pmladek@...e.com, rostedt@...dmis.org,
        sergey.senozhatsky@...il.com, andriy.shevchenko@...ux.intel.com,
        linux@...musvillemoes.dk,
        laurent.pinchart+renesas@...asonboard.com,
        jacopo+renesas@...ndi.org, kieran.bingham+renesas@...asonboard.com,
        linus.walleij@...aro.org, heikki.krogerus@...ux.intel.com,
        kitakar@...il.com, jorhand@...ux.microsoft.com
Subject: Re: [PATCH v2 04/12] software_node: Enforce parent before child
 ordering of nodes arrays

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.shevchenko@...ux.intel.com>
>> Signed-off-by: Daniel Scally <djrscally@...il.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.pinchart@...asonboard.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);
> 

Powered by blists - more mailing lists