Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH v2 06/12] software_node: Add support for fwnode_graph*()
 family of functions
To:     Sakari Ailus <sakari.ailus@linux.intel.com>
Cc:     linux-kernel@vger.kernel.org, linux-acpi@vger.kernel.org,
        linux-media@vger.kernel.org, devel@acpica.org, rjw@rjwysocki.net,
        lenb@kernel.org, gregkh@linuxfoundation.org, yong.zhi@intel.com,
        bingbu.cao@intel.com, tian.shu.qiu@intel.com, mchehab@kernel.org,
        robert.moore@intel.com, erik.kaneda@intel.com, pmladek@suse.com,
        rostedt@goodmis.org, sergey.senozhatsky@gmail.com,
        andriy.shevchenko@linux.intel.com, linux@rasmusvillemoes.dk,
        laurent.pinchart+renesas@ideasonboard.com,
        jacopo+renesas@jmondi.org, kieran.bingham+renesas@ideasonboard.com,
        linus.walleij@linaro.org, heikki.krogerus@linux.intel.com,
        kitakar@gmail.com, jorhand@linux.microsoft.com
References: <20201217234337.1983732-1-djrscally@gmail.com>
 <20201217234337.1983732-7-djrscally@gmail.com>
 <20201221093408.GH26370@paasikivi.fi.intel.com>
From:   Daniel Scally <djrscally@gmail.com>
Message-ID: <b6228b89-cbc8-9b8a-6a68-a401e2e803fc@gmail.com>
Date:   Mon, 21 Dec 2020 10:01:50 +0000
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.10.0
MIME-Version: 1.0
In-Reply-To: <20201221093408.GH26370@paasikivi.fi.intel.com>
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 7bit
Content-Language: en-US
Precedence: bulk

Hi Sakari - thanks for the reviews in previous emails

On 21/12/2020 09:34, Sakari Ailus wrote:
> Hi Daniel and Heikki,
>
> On Thu, Dec 17, 2020 at 11:43:31PM +0000, Daniel Scally wrote:
>
> +static struct fwnode_handle *
> +software_node_graph_get_port_parent(struct fwnode_handle *fwnode)
> +{
> +	struct swnode *swnode = to_swnode(fwnode);
> +	struct fwnode_handle *parent;
> +
> +	if (!strcmp(swnode->parent->node->name, "ports"))
> +		parent = &swnode->parent->parent->fwnode;
> +	else
> +		parent = &swnode->parent->fwnode;
> If you happen to call this function on a non-port node for whatever reason,
> you may end up accessing a pointer that's NULL, can't you?

Yes, actually.

> Instead I'd do
> something like:
>
> swnode = swnode->parent;
> if (swnode && !strcmp(swnode->node->name, "ports"))
> 	swnode = swnode->parent;
>
> return swnode ? software_node_get(&swnode->fwnode) : NULL;
>
> You can also drop parent as a by-product of this.
Yes ok, that looks good to me - thanks.
>> +
>> +	return software_node_get(parent);
>> +}
>> +
>> +static int
>> +software_node_graph_parse_endpoint(const struct fwnode_handle *fwnode,
>> +				   struct fwnode_endpoint *endpoint)
>> +{
>> +	struct swnode *swnode = to_swnode(fwnode);
>> +	int ret;
>> +
>> +	ret = kstrtou32(swnode->parent->node->name + 5, 10, &endpoint->port);
>> +	if (ret)
>> +		return ret;
>> +
>> +	endpoint->id = swnode->id;
>> +	endpoint->local_fwnode = fwnode;
>> +
>> +	return 0;
>> +}
>> +
>>  static const struct fwnode_operations software_node_ops = {
>>  	.get = software_node_get,
>>  	.put = software_node_put,
>> @@ -551,7 +655,11 @@ static const struct fwnode_operations software_node_ops = {
>>  	.get_parent = software_node_get_parent,
>>  	.get_next_child_node = software_node_get_next_child,
>>  	.get_named_child_node = software_node_get_named_child_node,
>> -	.get_reference_args = software_node_get_reference_args
>> +	.get_reference_args = software_node_get_reference_args,
>> +	.graph_get_next_endpoint = software_node_graph_get_next_endpoint,
>> +	.graph_get_remote_endpoint = software_node_graph_get_remote_endpoint,
>> +	.graph_get_port_parent = software_node_graph_get_port_parent,
>> +	.graph_parse_endpoint = software_node_graph_parse_endpoint,
>>  };
>>  
>>  /* -------------------------------------------------------------------------- */