Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
Date:   Fri, 4 Mar 2022 15:41:33 +0200
From:   Sakari Ailus <sakari.ailus@linux.intel.com>
To:     Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc:     Bjorn Andersson <bjorn.andersson@linaro.org>,
        Rob Herring <robh+dt@kernel.org>,
        Daniel Scally <djrscally@gmail.com>,
        Heikki Krogerus <heikki.krogerus@linux.intel.com>,
        "Rafael J. Wysocki" <rafael@kernel.org>,
        Hans de Goede <hdegoede@redhat.com>,
        linux-usb@vger.kernel.org, devicetree@vger.kernel.org,
        linux-kernel@vger.kernel.org, linux-acpi@vger.kernel.org,
        Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Subject: Re: [PATCH v3 1/6] device property: Helper to match multiple
 connections
Message-ID: <YiIXDZnquRZj8dU5@paasikivi.fi.intel.com>
References: <20220303223351.141238-1-bjorn.andersson@linaro.org>
 <YiIL/ejgxhfRhTDP@smile.fi.intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <YiIL/ejgxhfRhTDP@smile.fi.intel.com>
Precedence: bulk

Hi Andy, Bj?rn,

On Fri, Mar 04, 2022 at 02:54:21PM +0200, Andy Shevchenko wrote:
> On Thu, Mar 03, 2022 at 02:33:46PM -0800, Bjorn Andersson wrote:
> > In some cases multiple connections with the same connection id
> > needs to be resolved from a fwnode graph.
> > 
> > One such example is when separate hardware is used for performing muxing
> > and/or orientation switching of the SuperSpeed and SBU lines in a USB
> > Type-C connector. In this case the connector needs to belong to a graph
> > with multiple matching remote endpoints, and the Type-C controller needs
> > to be able to resolve them both.
> > 
> > Add a new API that allows this kind of lookup.
> 
> ...
> 
> > +static unsigned int fwnode_graph_devcon_matches(struct fwnode_handle *fwnode,
> > +						const char *con_id, void *data,
> > +						devcon_match_fn_t match,
> > +						void **matches,
> > +						unsigned int matches_len)
> > +{
> > +	struct fwnode_handle *node;
> > +	struct fwnode_handle *ep;
> > +	unsigned int count = 0;
> > +	void *ret;
> > +
> > +	fwnode_graph_for_each_endpoint(fwnode, ep) {
> 
> > +		if (count >= matches_len && matches) {
> > +			fwnode_handle_put(ep);
> > +			return count;
> > +		}
> 
> Wouldn't be the same as
> 
> 		if (count >= matches_len) {
> 			fwnode_handle_put(ep);
> 			break;
> 		}

Don't you need to check for non-NULL matches here?

I find return above easier to read.

> 
> ?
> 
> > +		node = fwnode_graph_get_remote_port_parent(ep);
> > +		if (!fwnode_device_is_available(node)) {
> > +			fwnode_handle_put(node);
> > +			continue;
> > +		}
> > +
> > +		ret = match(node, con_id, data);
> > +		fwnode_handle_put(node);
> > +		if (ret) {
> > +			if (matches)
> > +				matches[count] = ret;
> > +			count++;
> > +		}
> > +	}
> > +	return count;
> > +}
> 
> ...
> 
> > +static unsigned int fwnode_devcon_matches(struct fwnode_handle *fwnode,
> > +					  const char *con_id, void *data,
> > +					  devcon_match_fn_t match,
> > +					  void **matches,
> > +					  unsigned int matches_len)
> > +{
> > +	struct fwnode_handle *node;
> > +	unsigned int count = 0;
> > +	unsigned int i;
> > +	void *ret;
> > +
> > +	for (i = 0; ; i++) {
> 
> > +		if (count >= matches_len && matches)
> > +			return count;
> 
> Ditto.
> 
> > +		node = fwnode_find_reference(fwnode, con_id, i);
> > +		if (IS_ERR(node))
> > +			break;
> > +
> > +		ret = match(node, NULL, data);
> > +		fwnode_handle_put(node);
> > +		if (ret) {
> > +			if (matches)
> > +				matches[count] = ret;
> > +			count++;
> > +		}
> > +	}
> > +
> > +	return count;
> > +}
> 
> ...
> 
> > +int fwnode_connection_find_matches(struct fwnode_handle *fwnode,
> > +				   const char *con_id, void *data,
> > +				   devcon_match_fn_t match,
> > +				   void **matches, unsigned int matches_len)
> > +{
> > +	unsigned int count_graph;
> > +	unsigned int count_ref;
> > +
> > +	if (!fwnode || !match)
> > +		return -EINVAL;
> 
> > +	count_graph = fwnode_graph_devcon_matches(fwnode, con_id, data, match,
> > +						  matches, matches_len);
> 
> > +	matches += count_graph;
> > +	matches_len -= count_graph;
> 
> No, won't work when matches == NULL.
> 
> Also, matches_len is expected to be 0 in that case (or at least being ignored,
> check with vsnprintf() behaviour in similar case).
> 
> So, something like this, perhaps
> 
> 	if (matches && matches_len) {
> 		matches += count_graph;
> 		matches_len -= count_graph;
> 	}

Good find!

> 
> > +	count_ref = fwnode_devcon_matches(fwnode, con_id, data, match,
> > +					  matches, matches_len);
> > +
> > +	return count_graph + count_ref;
> > +}
> 
> 

-- 
Regards,

Sakari Ailus