by Bjorn Andersson

[permalink] [raw]

Subject: Re: [PATCH 3/8] device property: Helper to match multiple connections

On Fri 07 Jan 06:33 PST 2022, Sakari Ailus wrote:

> On Wed, Jan 05, 2022 at 12:43:28PM -0800, Bjorn Andersson wrote:
> > On Fri 31 Dec 01:09 PST 2021, Sakari Ailus wrote:
> >
> > > Hi Bj?rn,
> > >
> > > (And thanks to Heikki for cc'ing me!)
> > >
> > > On Thu, Dec 30, 2021 at 11:26:34AM +0200, Heikki Krogerus wrote:
> > > > +Andy, Dan and Sakari
> > > >
> > > > On Mon, Dec 27, 2021 at 09:21:11PM -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-C
> > > > > connector. In this case the connector needs to belong to a graph with
> > > > > multiple matching remote endpoints, and the TypeC controller needs to be
> > > > > able to resolve them both.
> > > > >
> > > > > Add a new API that allows this kind of lookup.
> > > > >
> > > > > Signed-off-by: Bjorn Andersson <[email protected]>
> > > > > ---
> > > > > drivers/base/property.c | 94 ++++++++++++++++++++++++++++++++++++++++
> > > > > include/linux/property.h | 5 +++
> > > > > 2 files changed, 99 insertions(+)
> > > > >
> > > > > diff --git a/drivers/base/property.c b/drivers/base/property.c
> > > > > index cbe4fa298413..0aa0296fd991 100644
> > > > > --- a/drivers/base/property.c
> > > > > +++ b/drivers/base/property.c
> > > > > @@ -1180,6 +1180,36 @@ fwnode_graph_devcon_match(struct fwnode_handle *fwnode, const char *con_id,
> > > > > return NULL;
> > > > > }
> > > > >
> > > > > +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) {
> > > > > + fwnode_handle_put(ep);
> > > > > + return count;
> > > > > + }
> > > > > +
> > > > > + node = fwnode_graph_get_remote_port_parent(ep);
> > > > > + if (!fwnode_device_is_available(node))
> > >
> > > The reference to node needs to be put here.
> > >
> >
> > You're right, thanks!
> >
> > > > > + continue;
> > > > > +
> > > > > + ret = match(node, con_id, data);
> > > > > + fwnode_handle_put(node);
> > > > > +
> > > > > + if (ret)
> > > > > + matches[count++] = ret;
> > > > > + }
> > > > > + return count;
> > > > > +}
> > > > > +
> > > > > static void *
> > > > > fwnode_devcon_match(struct fwnode_handle *fwnode, const char *con_id,
> > > > > void *data, devcon_match_fn_t match)
> > > > > @@ -1202,6 +1232,35 @@ fwnode_devcon_match(struct fwnode_handle *fwnode, const char *con_id,
> > > > > return NULL;
> > > > > }
> > > > >
> > > > > +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;
> > > > > + void *ret;
> > > > > + int i;
> > >
> > > unsigned int, please.
> > >
> >
> > Sounds good.
> >
> > > > > +
> > > > > + for (i = 0; ; i++) {
> > > > > + if (count >= matches_len)
> > > > > + return count;
> > > > > +
> > > > > + node = fwnode_find_reference(fwnode, con_id, i);
> > > > > + if (IS_ERR(node))
> > > > > + break;
> > > > > +
> > > > > + ret = match(node, NULL, data);
> > > > > + fwnode_handle_put(node);
> > > > > +
> > > > > + if (ret)
> > > > > + matches[count++] = ret;
> > > > > + }
> > > > > +
> > > > > + return count;
> > > > > +}
> > > > > +
> > > > > /**
> > > > > * fwnode_connection_find_match - Find connection from a device node
> > > > > * @fwnode: Device node with the connection
> > > > > @@ -1229,3 +1288,38 @@ void *fwnode_connection_find_match(struct fwnode_handle *fwnode,
> > > > > return fwnode_devcon_match(fwnode, con_id, data, match);
> > > > > }
> > > > > EXPORT_SYMBOL_GPL(fwnode_connection_find_match);
> > > > > +
> > > > > +/**
> > > > > + * fwnode_connection_find_matches - Find connections from a device node
> > > > > + * @fwnode: Device node with the connection
> > > > > + * @con_id: Identifier for the connection
> > > > > + * @data: Data for the match function
> > > > > + * @match: Function to check and convert the connection description
> > > > > + * @matches: Array of pointers to fill with matches
> > > > > + * @matches_len: Length of @matches
> > > > > + *
> > > > > + * Find up to @matches_len connections with unique identifier @con_id between
> > > > > + * @fwnode and other device nodes. @match will be used to convert the
> > > > > + * connection description to data the caller is expecting to be returned
> > > > > + * through the @matches array.
> > >
> > > If the caller allocates the matches array, how does it know how large it
> > > should be? Is there a need to provide a way to count the matches before
> > > writing them to an array? Most similar functions do that by just setting the
> > > array (matches) to NULL.
> > >
> >
> > This is a very relevant comment and I did look for ways to handle this
> > as I came up with the patch.
> >
> > I think the typical mechanism would be to allow @matches to be NULL, in
> > which case we iterate over objects and return the number of matches, so
> > that the caller can allocate an appropriately sized array and call the
> > API again.
> >
> > But the "match" function simply returns a pointer to something and
> > looking at the example of the typec_{mux,switch} this pointer points to
> > a member of an object which has a struct device which is refcounted.
> >
> > As such, we can't simply discard the returned object. We have to pass it
> > back to the caller, whom knows what "match" did and is able to reverse
> > that.
> >
> > I looked at changing the callback and I looked at using krealloc() to
> > grow an array dynamically.
>
> krealloc() may also fail...
>

Exactly.

> >
> >
> > But looking at the use case in mind; finding entities that might need to
> > react to a USB Type-C event I have a need for 2 matches, and 3 seems
> > plausible. Beyond that the largest of_graph I have ever dealt with has 6
> > endpoints.
> >
> > While it isn't relevant to use this API for my 6-endpoint case, it would
> > result in @matches having to be 48 bytes of pointers. And once the call
> > returns, the actual number of pointers needed is known and the long-term
> > storage can be re-allocated as necessary based on the return value.
> >
> > As such, I dropped the idea of making something fancier and more
> > dynamic, for the sake of simplicity. Perhaps I'm missing some cool use
> > case where this is infeasible?
>
> Another option would be to use a fixed-size array for the purpose. Assuming
> this will remain a small number, a single global macro could be used to set
> the maximum number that could be also easily increased if needed.
>
> On the other hand, if this number remains specific to the caller as it
> would seem, then I guess a caller-set value (as implemented now) remains a
> fine option, too.
>

Sounds good.

I will try to capture these arguments in the commit message as I post
the next version.

Thanks,
Bjorn

> --
> Kind regards,
>
> Sakari Ailus