by Andy Shevchenko

[permalink] [raw]

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

On Sun, Feb 20, 2022 at 08:55:10PM -0800, Bjorn Andersson wrote:
> 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.

Yes, that's what I want from the generic fwnode APIs.
(Keyword: generic)

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

As you stated, the problem is in the typec_switch_match(). So, it's not related
to the fwnode, but how you are using it.

> 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.

fwnode (as being an abstraction on top of the others) has no knowledge
about this. And more important should not know that.

> 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.

The caller (if it wants to!) may create different callbacks for count and real
matching, no?

> 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).

--
With Best Regards,
Andy Shevchenko

2022-02-22 04:59:21

by Bjorn Andersson

[permalink] [raw]

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

On Mon 21 Feb 09:19 PST 2022, Andy Shevchenko wrote:

> On Sun, Feb 20, 2022 at 08:55:10PM -0800, Bjorn Andersson wrote:
> > 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.
>
> Yes, that's what I want from the generic fwnode APIs.
> (Keyword: generic)
>
> > The problem with this is that, typec_switch_match() does:
>
> As you stated, the problem is in the typec_switch_match(). So, it's not related
> to the fwnode, but how you are using it.
>
> > 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.
>
> fwnode (as being an abstraction on top of the others) has no knowledge
> about this. And more important should not know that.
>
> > 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.
>
> The caller (if it wants to!) may create different callbacks for count and real
> matching, no?
>

Ahh, yeah you're right, we can shift this responsibility onto the caller
and thereby allow them to implement the count as well. Makes sense!

Thanks,
Bjorn

> > 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).
>
> --
> With Best Regards,
> Andy Shevchenko
>
>