Hi Laurent,

On Tue, Sep 19, 2017 at 07:12:14PM +0300, Laurent Pinchart wrote:

Hi Sakari,

On Tuesday, 19 September 2017 17:47:22 EEST Sakari Ailus wrote:

quoted

On Tue, Sep 19, 2017 at 03:46:18PM +0300, Laurent Pinchart wrote:

quoted

On Tuesday, 19 September 2017 15:43:26 EEST Sakari Ailus wrote:

quoted

On Tue, Sep 19, 2017 at 02:40:29PM +0300, Laurent Pinchart wrote:

quoted

quoted

@@ -2256,7 +2210,9 @@ static int isp_probe(struct platform_device

*pdev)
 	if (ret)
 		return ret;

-	ret = isp_fwnodes_parse(&pdev->dev, &isp->notifier);
+	ret = v4l2_async_notifier_parse_fwnode_endpoints(
+		&pdev->dev, &isp->notifier, sizeof(struct isp_async_subdev),
+		isp_fwnode_parse);
 	if (ret < 0)

The documentation in patch 05/25 states that
v4l2_async_notifier_release() should be called even if
v4l2_async_notifier_parse_fwnode_endpoints() fails. I don't think that's
needed here, so you might want to update the documentation (and possibly
the implementation of the function).

It is. If parsing fails, async sub-devices may have been already set up.
This happens e.g. when the parsing fails after the first one has been
successfully set up already.

But for v4l2_async_notifier_parse_fwnode_endpoints() we could clean up
internally when an error occurs. Otherwise you need to call
v4l2_async_notifier_release() here.

The functions that set up async sub-devices can be called multiple times
(on separate references). This is quite alike setting up a control handler
really, so I adopted the same pattern.

If there is a failure, how many async sub-devices should be cleaned up, if
there have been async sub-devices already set up before calling this
function?

I'm not opposed to that pattern, I just thought that cleanup could be 
automated for v4l2_async_notifier_parse_fwnode_endpoints() failures, as 
opposed to v4l2_async_notifier_parse_fwnode_endpoints_by_port() failures.

As this patch, written by the author of 
v4l2_async_notifier_parse_fwnode_endpoints() and part of the same patch 
series, is missing a call to v4l2_async_notifier_release(), I expect driver 
authors to make the same mistake and was thus wondering how to prevent that.

I believe that the issue here is that initialization of the notifier is done 
implicitly by the first call to a parsing function. With explicit notification 
it should be clear to driver authors that they need to call the cleanup 
function:

ret = init()
if (ret)
	return ret;

ret = parse()
if (ret) {
	cleanup();
	return ret;
}

ret = parse()
if (ret) {
	cleanup();
	return ret;
}

ret = parse()
if (ret) {
	cleanup();
	return ret;
}

But with an implicit initialization it's easy to miss cleanup when 
v4l2_async_notifier_parse_fwnode_endpoints() fails, as that function can be 
considered as an initilization function that performs cleanup internally.

I'm not sure what the best pattern would be. At the very least you need to fix 
this patch, but that wouldn't prevent future mistakes.

The patch was actually originally written before this was apparent. I agree
that it is not a common pattern to require cleanup function to be called on
a failure.

I'll fix the patch for v14.

Going forward, I see no reason why we couldn't automate much of this for
drivers: the bindings are generic after all. The notifier could be
registered through registration of the v4l2_device. It could be simply
released immediately if there would be no async sub-devices around.

It's not a trivial change, and I'd think well out of scope of this set:
we'll need to move link creation to the framework and detach async
sub-deviecs from parsing the endpoint properties as well.

-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ailus-X3B1VOXEql0@public.gmane.org
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html