Re: [PATCH v2 1/6] device property: Helper to match multiple connections

[Bjorn Andersson <bjorn.andersson@linaro.org>]

On Sun 20 Feb 03:16 PST 2022, Andy Shevchenko wrote:

> On Fri, Feb 18, 2022 at 11:00:45AM -0800, Bjorn Andersson wrote:
> > On Wed 09 Feb 04:30 PST 2022, Andy Shevchenko wrote:
> > > On Mon, Feb 07, 2022 at 07:19:39PM -0800, Bjorn Andersson wrote:
> 
> ...
> 
> > > > +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;
> > > > +
> > > > +	if (!fwnode || !match || !matches)
> > > 
> > > !matches case may be still useful to get the count and allocate memory by
> > > caller. Please, consider this case.
> > > 
> > 
> > As discussed in previous version, and described in the commit message,
> > the returned value of "match" is a opaque pointer to something which
> > has to be passed back to the caller in order to be cleaned up.
> > 
> > E.g. the typec mux code returns a pointer to a typec_mux/switch object
> > with a refcounted struct device within, or an ERR_PTR().
> > 
> > So unfortunately we can must gather the results into matches and pass it
> > back to the caller to take consume or clean up.
> 
> 
> It's fine. You have **matches, means pointer of an opaque pointer.
> What I'm talking about is memory allocation for and array of _pointers_.
> That's what caller very much aware of and can allocate on heap. So, please
> consider this case.
> 

I'm sorry, but I'm not sure what you're looking for.


I still interpret your comment as that it would be nice to be able to do
something like:

count = fwnode_connection_find_matches(fwnode, "orientation-switch",
				       NULL, typec_switch_match, NULL, 0);

based on the returned value the caller could allocate an array of
"count" pointers and then call the function again to actually fill out
the count elements.


The problem with this is that, typec_switch_match() does:

void *typec_switch_match(fwnode, id, data) {
	struct device *dev = find_struct_device(fwnode, id);
	if (!dev)
		return NULL;
	get_device(dev);
	return container_of(dev, struct typec_switch, dev);
}

So if we call the match function and if that finds a "dev" it will
return a struct typec_switch with a refcounted struct device within.

We can see if that's NULL or not and will be able to return a "count",
but we have no way of releasing the reference acquired - we must return
the void pointer back to the client, so that it can release it.


My claim is that this is not a problem, because this works fine with any
reasonable size of fwnode graphs we might run into - and the client will
in general have a sense of the worst case number of matches (in this
series its 3, as there's 3 types of lanes that can be switched/muxed
coming out of a USB connector).


But that's perhaps not what you're referring to? Or perhaps I'm missing
something else?

Regards,
Bjorn