Re: [PATCH v2] device property: preserve usecount for node passed to of_fwnode_graph_get_port_parent()

[Rob Herring <robh@kernel.org>]

On Wed, Oct 4, 2017 at 7:30 AM, Sakari Ailus <sakari.ailus@iki.fi> wrote:
> Hi Rob,
>
> On Tue, Oct 03, 2017 at 08:40:10AM -0500, Rob Herring wrote:
>> On Sun, Aug 27, 2017 at 5:40 PM, Sakari Ailus
>> <sakari.ailus@linux.intel.com> wrote:
>> > Hi Rob,
>> >
>> > On Tue, Aug 22, 2017 at 02:42:10PM -0500, Rob Herring wrote:
>> >> On Tue, Aug 22, 2017 at 10:00 AM, Niklas Söderlund
>> >> <niklas.soderlund@ragnatech.se> wrote:
>> >> > Hi Rob,
>> >> >
>> >> > On 2017-08-22 09:49:35 -0500, Rob Herring wrote:
>> >> >> On Mon, Aug 21, 2017 at 7:19 PM, Niklas Söderlund
>> >> >> <niklas.soderlund+renesas@ragnatech.se> wrote:
>> >> >> > Using CONFIG_OF_DYNAMIC=y uncovered an imbalance in the usecount of the
>> >> >> > node being passed to of_fwnode_graph_get_port_parent(). Preserve the
>> >> >> > usecount by using of_get_parent() instead of of_get_next_parent() which
>> >> >> > don't decrement the usecount of the node passed to it.
>> >> >> >
>> >> >> > Fixes: 3b27d00e7b6d7c88 ("device property: Move fwnode graph ops to firmware specific locations")
>> >> >> > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
>> >> >> > Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>
>> >> >> > ---
>> >> >> >  drivers/of/property.c | 2 +-
>> >> >> >  1 file changed, 1 insertion(+), 1 deletion(-)
>> >> >>
>> >> >> Isn't this already fixed with this fix:
>> >> >>
>> >> >> commit c0a480d1acf7dc184f9f3e7cf724483b0d28dc2e
>> >> >> Author: Tony Lindgren <tony@atomide.com>
>> >> >> Date:   Fri Jul 28 01:23:15 2017 -0700
>> >> >>
>> >> >> device property: Fix usecount for of_graph_get_port_parent()
>> >> >
>> >> > No, that commit fixes it for of_graph_get_port_parent() while this
>> >> > commit fixes it for of_fwnode_graph_get_port_parent(). But in essence it
>> >> > is the same issue but needs two separate fixes.
>> >>
>> >> Ah, because one takes the port node and one takes the endpoint node.
>> >> That won't confuse anyone.
>> >
>> > Yes, I think we've had this for some time in naming of a few graph
>> > functions and increasingly so lately. It began with
>> > of_graph_get_remote_port_parent(), which likely was named so to avoid the
>> > name getting really long. The function itself gets a remove of the endpoint
>> > given as an argument, then the port related to the entpoint and finally the
>> > parent node of the port node (which is not the "ports" node). That's a lot
>> > of work for a single interface function.
>>
>> That's because returning the "ports" node would be pointless. The
>> remote device could have:
>>
>> ports {
>>     port {
>>         endpoint {
>>         };
>>     };
>> };
>>
>> Or no ports node. Both are valid and should be treated equivalently.
>
> Indeed --- otherwise you could simply use of_get_parent()...
>
>>
>> >
>> > What comes to of_fwnode_graph_get_port_parent() --- it's the OF callback
>> > function for the fwnode graph API that, as the name suggests, gets the
>> > parent of the port node, no more, no less. The function is used in the
>> > fwnode graph API implementation and is not available in the API as such.
>>
>> If this operates on the local node, then you should already have the
>> relevant parent. If you are walking the remote node, then the exact
>> port structure should be opaque. If you need the remote endpoint and
>> its properties, that's fine. Otherwise, you should really only need
>> the remote parent device because the remote's graph structure is
>> specific to the remote device and any parsing should be done within
>> the remote's driver.
>
> This function is used to implement fwnode_graph_get_remote_port_parent() as
> well as fwnode_graph_get_port_parent(). These are used by V4L2 to match
> remote devices currently. That said, we've thought about using endpoints
> for matching: the connection is made between two hardware interfaces, not
> devices.
>
>>
>> > The fwnode graph API itself only implements functionality already available
>> > in the OF graph API under the corresponding name.
>>
>> There are graph APIs I want to get rid of, but since there are still
>> users I haven't. Adding those APIs to the fwnode API will just make it
>> harder.
>
> I remember we've discussed that but I'm not sure we arrived to a conclusion
> back then. And that explicitly parsing a port / endpoint pair was
> preferred.
>
> In V4L2 the endpoints are currently iterated over. The endpoint numbers are
> also not verified in drivers (apart from perhaps one or two exceptions),
> they're currently simply ignored. Only port numbers are actually used.
>
> I think it boils down to whether (or not) there's a material chance of
> breaking something by effectively adding the endpoint number checks. Should
> we assume it will not?
>
>>
>> >> Can we please align this mess. I've tried to make the graph parsing
>> >> not a free for all, open coded mess. There's no reason to have the
>> >> port node handle and then need the parent device. Either you started
>> >> with the parent device to parse local ports and endpoints or you got
>> >> the remote endpoint with .graph_get_remote_endpoint(). Most of the
>> >> time you don't even need the endpoint node handles. You really just
>> >> need to know what is the remote device connected to port X, endpoint Y
>> >> of my local device.
>> >
>> > Perhaps most of the time, yes. V4L2 devices store bus (e.g. MIPI CSI-2)
>> > specific information in the endpoint nodes.
>> >
>> > The current OF graph API is geared towards providing convenience functions
>> > to the extent that a single function performs actions a driver would
>> > typically need. More recently functions implementing a single operation has
>> > been added; the primitives that just perform a single operation would
>> > likely be easier to manage.
>>
>> Then we have each driver open coding walking the graph.
>>
>> > The convenience functions have been, well, convenient as getting and
>> > putting nodes could have been somewhat ignored in drivers. If the OF graph
>> > API usage can be moved out of the drivers we'll likely have way fewer users
>> > and thus there's no real need for convenience functions. That has other
>> > benefits, too, such as parsing the graph correctly: most V4L2 drivers have
>> > issues in this area.
>> >
>> > The OF graph API (or the fwnode equivalent) is used effectively in all V4L2
>> > drivers that support OF (there are about 20 of them); we're moving these to
>> > the V4L2 framework but it'll take some time. That should make it easier for
>> > cleaning things up. Based on a quick look V4L2 and V4L2 drivers together
>> > represent a large proportion of all users in the kernel.
>>
>> I certainly will care less when there's only subsystems using the API
>> versus each driver.
>>
>> I'd guess it is more equal because most of the DRM drivers are
>> converted to not use the graph API directly.
>
> Ack.
>
>>
>> >
>> > What do you think?
>>
>> I'll apply this fix, but please keep the above in mind when reworking
>> the V4L2 drivers.
>
> Thanks!
>
> --
> Kind regards,
>
> Sakari Ailus
> e-mail: sakari.ailus@iki.fi