[PATCH 0/2] max9286: Refactor V4L2 support to prevent EPROBE_DEFER failures

[PATCH 0/2] max9286: Refactor V4L2 support to prevent EPROBE_DEFER failures

[Kieran Bingham]

Currently the V4L2 notifiers are registered during DT parsing.

Move this handling and all other V4L2 code to two distinct implementations and
contained higher in the source code.  This simplifies the hardware
initialisation functions, and makes error paths and cleanup much easier to
parse.

I hope to squash these two patches into the max9286 and then post a v7, along
with Jacopo's RDACM20 split work, with an aim to hopefully getting upstream
integration.


max9286 v7 changlog currently looks like:

    v7:
     [Kieran]
     - Ensure powerdown lines are optional
     - Add a 4ms power-up delay
     - Add max9286_check_config_link() to core
     - Add GPIO chip controller for GPIO0OUT and GPIO1OUT
     - Fix GPIO registration
    
     [Jacopo]
     - Remove redundanct MAXIM_I2C_SPEED macros
     - Move notifiers operations
     - Add delay after reverse channel reconfiguration
     - Move link setup to completion
     - Fix up max9286_check_config_link() implementation
     - Remove redundant dual configuration of reverse channel


Kieran Bingham (2):
  max9286: Split out async registration
  max9286: Collect all V4L2 registrations

 drivers/media/i2c/max9286.c | 205 ++++++++++++++++++++++--------------
 1 file changed, 127 insertions(+), 78 deletions(-)

-- 
2.20.1

[PATCH 1/2] max9286: Split out async registration

[Kieran Bingham]

Move all the V4L2 Subdev Async registration so that it can only happen once
we know we will not need to -EPROBE_DEFER...

Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
---
 drivers/media/i2c/max9286.c | 88 +++++++++++++++++++++++--------------
 1 file changed, 55 insertions(+), 33 deletions(-)

diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
index 1b4ff3533795..03c5fa232b6d 100644
--- a/drivers/media/i2c/max9286.c
+++ b/drivers/media/i2c/max9286.c
@@ -503,6 +503,49 @@ static const struct v4l2_async_notifier_operations max9286_notify_ops = {
 	.unbind = max9286_notify_unbind,
 };
 
+static int max9286_v4l2_async_register(struct max9286_priv *priv)
+{
+	struct device *dev = &priv->client->dev;
+	struct max9286_source *source = NULL;
+	int ret;
+
+	v4l2_async_notifier_init(&priv->notifier);
+
+	for_each_source(priv, source) {
+		unsigned int i = to_index(priv, source);
+
+		dev_err(dev, "Registering v4l2-async for source %d\n", i);
+
+		source->asd.match_type = V4L2_ASYNC_MATCH_FWNODE;
+		source->asd.match.fwnode = source->fwnode;
+
+		ret = v4l2_async_notifier_add_subdev(&priv->notifier,
+						     &source->asd);
+		if (ret) {
+			dev_err(dev, "Failed to add subdev for source %d", i);
+			v4l2_async_notifier_cleanup(&priv->notifier);
+			return ret;
+		}
+	}
+
+	priv->notifier.ops = &max9286_notify_ops;
+
+	ret = v4l2_async_subdev_notifier_register(&priv->sd, &priv->notifier);
+	if (ret) {
+		dev_err(dev, "Failed to register subdev_notifier");
+		v4l2_async_notifier_cleanup(&priv->notifier);
+		return ret;
+	}
+
+	return 0;
+}
+
+static void max9286_v4l2_async_unregister(struct max9286_priv *priv)
+{
+	v4l2_async_notifier_unregister(&priv->notifier);
+	v4l2_async_notifier_cleanup(&priv->notifier);
+}
+
 static int max9286_s_stream(struct v4l2_subdev *sd, int enable)
 {
 	struct max9286_priv *priv = sd_to_max9286(sd);
@@ -870,6 +913,13 @@ static int max9286_init(struct device *dev)
 		goto err_regulator;
 	}
 
+	/* Register v4l2 async notifiers */
+	ret = max9286_v4l2_async_register(priv);
+	if (ret) {
+		dev_err(dev, "Unable to register V4L2 async notifiers\n");
+		goto err_regulator;
+	}
+
 	v4l2_i2c_subdev_init(&priv->sd, client, &max9286_subdev_ops);
 	priv->sd.internal_ops = &max9286_subdev_internal_ops;
 	priv->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
@@ -884,7 +934,7 @@ static int max9286_init(struct device *dev)
 	priv->sd.ctrl_handler = &priv->ctrls;
 	ret = priv->ctrls.error;
 	if (ret)
-		goto err_regulator;
+		goto err_async;
 
 	priv->sd.entity.function = MEDIA_ENT_F_VID_IF_BRIDGE;
 
@@ -927,6 +977,8 @@ static int max9286_init(struct device *dev)
 	max9286_i2c_mux_close(priv);
 err_put_node:
 	fwnode_handle_put(ep);
+err_async:
+	max9286_v4l2_async_unregister(priv);
 err_regulator:
 	regulator_disable(priv->regulator);
 	priv->poc_enabled = false;
@@ -938,14 +990,6 @@ static void max9286_cleanup_dt(struct max9286_priv *priv)
 {
 	struct max9286_source *source;
 
-	/*
-	 * Not strictly part of the DT, but the notifier is registered during
-	 * max9286_parse_dt(), and the notifier holds references to the fwnodes
-	 * thus the cleanup is here to mirror the registration.
-	 */
-	v4l2_async_notifier_unregister(&priv->notifier);
-	v4l2_async_notifier_cleanup(&priv->notifier);
-
 	for_each_source(priv, source) {
 		fwnode_handle_put(source->fwnode);
 		source->fwnode = NULL;
@@ -958,7 +1002,6 @@ static int max9286_parse_dt(struct max9286_priv *priv)
 	struct device_node *i2c_mux;
 	struct device_node *node = NULL;
 	unsigned int i2c_mux_mask = 0;
-	int ret;
 
 	of_node_get(dev->of_node);
 	i2c_mux = of_find_node_by_name(dev->of_node, "i2c-mux");
@@ -986,8 +1029,6 @@ static int max9286_parse_dt(struct max9286_priv *priv)
 	of_node_put(node);
 	of_node_put(i2c_mux);
 
-	v4l2_async_notifier_init(&priv->notifier);
-
 	/* Parse the endpoints */
 	for_each_endpoint_of_node(dev->of_node, node) {
 		struct max9286_source *source;
@@ -1056,34 +1097,14 @@ static int max9286_parse_dt(struct max9286_priv *priv)
 			continue;
 		}
 
-		source->asd.match_type = V4L2_ASYNC_MATCH_FWNODE;
-		source->asd.match.fwnode = source->fwnode;
-
-		ret = v4l2_async_notifier_add_subdev(&priv->notifier,
-						     &source->asd);
-		if (ret) {
-			v4l2_async_notifier_cleanup(&priv->notifier);
-			of_node_put(node);
-			return ret;
-		}
-
 		priv->source_mask |= BIT(ep.port);
 		priv->nsources++;
 	}
 	of_node_put(node);
 
-	/* Do not register the subdev notifier if there are no devices. */
-	if (!priv->nsources)
-		return 0;
-
 	priv->route_mask = priv->source_mask;
-	priv->notifier.ops = &max9286_notify_ops;
-
-	ret = v4l2_async_subdev_notifier_register(&priv->sd, &priv->notifier);
-	if (ret)
-		v4l2_async_notifier_cleanup(&priv->notifier);
 
-	return ret;
+	return 0;
 }
 
 static int max9286_probe(struct i2c_client *client)
@@ -1182,6 +1203,7 @@ static int max9286_remove(struct i2c_client *client)
 
 	fwnode_handle_put(priv->sd.fwnode);
 	v4l2_async_unregister_subdev(&priv->sd);
+	max9286_v4l2_async_unregister(priv);
 
 	if (priv->poc_enabled)
 		regulator_disable(priv->regulator);
-- 
2.20.1

[PATCH 2/2] max9286: Collect all V4L2 registrations

[Kieran Bingham]

Move all interactions with V4L2 to a dedicated pair of functions
for register and unregister to improve readability of the code.

Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
---
 drivers/media/i2c/max9286.c | 139 +++++++++++++++++++++---------------
 1 file changed, 83 insertions(+), 56 deletions(-)

diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
index 03c5fa232b6d..b1366f122a8a 100644
--- a/drivers/media/i2c/max9286.c
+++ b/drivers/media/i2c/max9286.c
@@ -735,6 +735,80 @@ static const struct v4l2_subdev_internal_ops max9286_subdev_internal_ops = {
 	.open = max9286_open,
 };
 
+static int max9286_v4l2_register(struct max9286_priv *priv)
+{
+	struct device *dev = &priv->client->dev;
+	struct fwnode_handle *ep;
+	int ret;
+	int i;
+
+	/* Register v4l2 async notifiers for connected Camera subdevices */
+	ret = max9286_v4l2_async_register(priv);
+	if (ret) {
+		dev_err(dev, "Unable to register V4L2 async notifiers\n");
+		return ret;
+	}
+
+	/* Configure V4L2 for the MAX9286 itself */
+
+	v4l2_i2c_subdev_init(&priv->sd, priv->client, &max9286_subdev_ops);
+	priv->sd.internal_ops = &max9286_subdev_internal_ops;
+	priv->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
+
+	v4l2_ctrl_handler_init(&priv->ctrls, 1);
+	/*
+	 * FIXME: Compute the real pixel rate. The 50 MP/s value comes from the
+	 * hardcoded frequency in the BSP CSI-2 receiver driver.
+	 */
+	v4l2_ctrl_new_std(&priv->ctrls, NULL, V4L2_CID_PIXEL_RATE,
+			  50000000, 50000000, 1, 50000000);
+	priv->sd.ctrl_handler = &priv->ctrls;
+	ret = priv->ctrls.error;
+	if (ret)
+		goto err_async;
+
+	priv->sd.entity.function = MEDIA_ENT_F_VID_IF_BRIDGE;
+
+	priv->pads[MAX9286_SRC_PAD].flags = MEDIA_PAD_FL_SOURCE;
+	for (i = 0; i < MAX9286_SRC_PAD; i++)
+		priv->pads[i].flags = MEDIA_PAD_FL_SINK;
+	ret = media_entity_pads_init(&priv->sd.entity, MAX9286_N_PADS,
+				     priv->pads);
+	if (ret)
+		goto err_async;
+
+	ep = fwnode_graph_get_endpoint_by_id(dev_fwnode(dev), MAX9286_SRC_PAD,
+					     0, 0);
+	if (!ep) {
+		dev_err(dev, "Unable to retrieve endpoint on \"port@4\"\n");
+		ret = -ENOENT;
+		goto err_async;
+	}
+	priv->sd.fwnode = ep;
+
+	ret = v4l2_async_register_subdev(&priv->sd);
+	if (ret < 0) {
+		dev_err(dev, "Unable to register subdevice\n");
+		goto err_put_node;
+	}
+
+	return 0;
+
+err_put_node:
+	fwnode_handle_put(ep);
+err_async:
+	max9286_v4l2_async_unregister(priv);
+
+	return ret;
+}
+
+static void max9286_v4l2_unregister(struct max9286_priv *priv)
+{
+	fwnode_handle_put(priv->sd.fwnode);
+	v4l2_async_unregister_subdev(&priv->sd);
+	max9286_v4l2_async_unregister(priv);
+}
+
 /* -----------------------------------------------------------------------------
  * Probe/Remove
  */
@@ -887,8 +961,6 @@ static int max9286_init(struct device *dev)
 {
 	struct max9286_priv *priv;
 	struct i2c_client *client;
-	struct fwnode_handle *ep;
-	unsigned int i;
 	int ret;
 
 	/* Skip non-max9286 devices. */
@@ -913,58 +985,20 @@ static int max9286_init(struct device *dev)
 		goto err_regulator;
 	}
 
-	/* Register v4l2 async notifiers */
-	ret = max9286_v4l2_async_register(priv);
-	if (ret) {
-		dev_err(dev, "Unable to register V4L2 async notifiers\n");
-		goto err_regulator;
-	}
-
-	v4l2_i2c_subdev_init(&priv->sd, client, &max9286_subdev_ops);
-	priv->sd.internal_ops = &max9286_subdev_internal_ops;
-	priv->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
-
-	v4l2_ctrl_handler_init(&priv->ctrls, 1);
 	/*
-	 * FIXME: Compute the real pixel rate. The 50 MP/s value comes from the
-	 * hardcoded frequency in the BSP CSI-2 receiver driver.
+	 * Register all V4L2 interactions for the MAX9286 and notifiers for
+	 * any subdevices connected.
 	 */
-	v4l2_ctrl_new_std(&priv->ctrls, NULL, V4L2_CID_PIXEL_RATE,
-			  50000000, 50000000, 1, 50000000);
-	priv->sd.ctrl_handler = &priv->ctrls;
-	ret = priv->ctrls.error;
-	if (ret)
-		goto err_async;
-
-	priv->sd.entity.function = MEDIA_ENT_F_VID_IF_BRIDGE;
-
-	priv->pads[MAX9286_SRC_PAD].flags = MEDIA_PAD_FL_SOURCE;
-	for (i = 0; i < MAX9286_SRC_PAD; i++)
-		priv->pads[i].flags = MEDIA_PAD_FL_SINK;
-	ret = media_entity_pads_init(&priv->sd.entity, MAX9286_N_PADS,
-				     priv->pads);
-	if (ret)
-		goto err_regulator;
-
-	ep = fwnode_graph_get_endpoint_by_id(dev_fwnode(dev), MAX9286_SRC_PAD,
-					     0, 0);
-	if (!ep) {
-		dev_err(dev, "Unable to retrieve endpoint on \"port@4\"\n");
-		ret = -ENOENT;
+	ret = max9286_v4l2_register(priv);
+	if (ret) {
+		dev_err(dev, "Failed to register with V4L2\n");
 		goto err_regulator;
 	}
-	priv->sd.fwnode = ep;
-
-	ret = v4l2_async_register_subdev(&priv->sd);
-	if (ret < 0) {
-		dev_err(dev, "Unable to register subdevice\n");
-		goto err_put_node;
-	}
 
 	ret = max9286_i2c_mux_init(priv);
 	if (ret) {
 		dev_err(dev, "Unable to initialize I2C multiplexer\n");
-		goto err_subdev_unregister;
+		goto err_v4l2_register;
 	}
 
 	/* Leave the mux channels disabled until they are selected. */
@@ -972,13 +1006,8 @@ static int max9286_init(struct device *dev)
 
 	return 0;
 
-err_subdev_unregister:
-	v4l2_async_unregister_subdev(&priv->sd);
-	max9286_i2c_mux_close(priv);
-err_put_node:
-	fwnode_handle_put(ep);
-err_async:
-	max9286_v4l2_async_unregister(priv);
+err_v4l2_register:
+	max9286_v4l2_unregister(priv);
 err_regulator:
 	regulator_disable(priv->regulator);
 	priv->poc_enabled = false;
@@ -1201,9 +1230,7 @@ static int max9286_remove(struct i2c_client *client)
 
 	i2c_mux_del_adapters(priv->mux);
 
-	fwnode_handle_put(priv->sd.fwnode);
-	v4l2_async_unregister_subdev(&priv->sd);
-	max9286_v4l2_async_unregister(priv);
+	max9286_v4l2_unregister(priv);
 
 	if (priv->poc_enabled)
 		regulator_disable(priv->regulator);
-- 
2.20.1

Re: [PATCH 1/2] max9286: Split out async registration

[Kieran Bingham]

Hi Me,

On 12/02/2020 17:37, Kieran Bingham wrote:
> Move all the V4L2 Subdev Async registration so that it can only happen once
> we know we will not need to -EPROBE_DEFER...
> 
> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> ---
>  drivers/media/i2c/max9286.c | 88 +++++++++++++++++++++++--------------
>  1 file changed, 55 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> index 1b4ff3533795..03c5fa232b6d 100644
> --- a/drivers/media/i2c/max9286.c
> +++ b/drivers/media/i2c/max9286.c
> @@ -503,6 +503,49 @@ static const struct v4l2_async_notifier_operations max9286_notify_ops = {
>  	.unbind = max9286_notify_unbind,
>  };
>  
> +static int max9286_v4l2_async_register(struct max9286_priv *priv)
> +{
> +	struct device *dev = &priv->client->dev;
> +	struct max9286_source *source = NULL;
> +	int ret;
> +
> +	v4l2_async_notifier_init(&priv->notifier);
> +
> +	for_each_source(priv, source) {
> +		unsigned int i = to_index(priv, source);
> +
> +		dev_err(dev, "Registering v4l2-async for source %d\n", i);

Debug line, can be removed.

> +
> +		source->asd.match_type = V4L2_ASYNC_MATCH_FWNODE;
> +		source->asd.match.fwnode = source->fwnode;
> +
> +		ret = v4l2_async_notifier_add_subdev(&priv->notifier,
> +						     &source->asd);
> +		if (ret) {
> +			dev_err(dev, "Failed to add subdev for source %d", i);
> +			v4l2_async_notifier_cleanup(&priv->notifier);
> +			return ret;
> +		}
> +	}
> +
> +	priv->notifier.ops = &max9286_notify_ops;
> +
> +	ret = v4l2_async_subdev_notifier_register(&priv->sd, &priv->notifier);
> +	if (ret) {
> +		dev_err(dev, "Failed to register subdev_notifier");
> +		v4l2_async_notifier_cleanup(&priv->notifier);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static void max9286_v4l2_async_unregister(struct max9286_priv *priv)
> +{
> +	v4l2_async_notifier_unregister(&priv->notifier);
> +	v4l2_async_notifier_cleanup(&priv->notifier);
> +}
> +
>  static int max9286_s_stream(struct v4l2_subdev *sd, int enable)
>  {
>  	struct max9286_priv *priv = sd_to_max9286(sd);
> @@ -870,6 +913,13 @@ static int max9286_init(struct device *dev)
>  		goto err_regulator;
>  	}
>  
> +	/* Register v4l2 async notifiers */
> +	ret = max9286_v4l2_async_register(priv);
> +	if (ret) {
> +		dev_err(dev, "Unable to register V4L2 async notifiers\n");
> +		goto err_regulator;
> +	}
> +
>  	v4l2_i2c_subdev_init(&priv->sd, client, &max9286_subdev_ops);
>  	priv->sd.internal_ops = &max9286_subdev_internal_ops;
>  	priv->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> @@ -884,7 +934,7 @@ static int max9286_init(struct device *dev)
>  	priv->sd.ctrl_handler = &priv->ctrls;
>  	ret = priv->ctrls.error;
>  	if (ret)
> -		goto err_regulator;
> +		goto err_async;
>  
>  	priv->sd.entity.function = MEDIA_ENT_F_VID_IF_BRIDGE;
>  
> @@ -927,6 +977,8 @@ static int max9286_init(struct device *dev)
>  	max9286_i2c_mux_close(priv);
>  err_put_node:
>  	fwnode_handle_put(ep);
> +err_async:
> +	max9286_v4l2_async_unregister(priv);
>  err_regulator:
>  	regulator_disable(priv->regulator);
>  	priv->poc_enabled = false;
> @@ -938,14 +990,6 @@ static void max9286_cleanup_dt(struct max9286_priv *priv)
>  {
>  	struct max9286_source *source;
>  
> -	/*
> -	 * Not strictly part of the DT, but the notifier is registered during
> -	 * max9286_parse_dt(), and the notifier holds references to the fwnodes
> -	 * thus the cleanup is here to mirror the registration.
> -	 */
> -	v4l2_async_notifier_unregister(&priv->notifier);
> -	v4l2_async_notifier_cleanup(&priv->notifier);
> -
>  	for_each_source(priv, source) {
>  		fwnode_handle_put(source->fwnode);
>  		source->fwnode = NULL;
> @@ -958,7 +1002,6 @@ static int max9286_parse_dt(struct max9286_priv *priv)
>  	struct device_node *i2c_mux;
>  	struct device_node *node = NULL;
>  	unsigned int i2c_mux_mask = 0;
> -	int ret;
>  
>  	of_node_get(dev->of_node);
>  	i2c_mux = of_find_node_by_name(dev->of_node, "i2c-mux");
> @@ -986,8 +1029,6 @@ static int max9286_parse_dt(struct max9286_priv *priv)
>  	of_node_put(node);
>  	of_node_put(i2c_mux);
>  
> -	v4l2_async_notifier_init(&priv->notifier);
> -
>  	/* Parse the endpoints */
>  	for_each_endpoint_of_node(dev->of_node, node) {
>  		struct max9286_source *source;
> @@ -1056,34 +1097,14 @@ static int max9286_parse_dt(struct max9286_priv *priv)
>  			continue;
>  		}
>  
> -		source->asd.match_type = V4L2_ASYNC_MATCH_FWNODE;
> -		source->asd.match.fwnode = source->fwnode;
> -
> -		ret = v4l2_async_notifier_add_subdev(&priv->notifier,
> -						     &source->asd);
> -		if (ret) {
> -			v4l2_async_notifier_cleanup(&priv->notifier);
> -			of_node_put(node);
> -			return ret;
> -		}
> -
>  		priv->source_mask |= BIT(ep.port);
>  		priv->nsources++;
>  	}
>  	of_node_put(node);
>  
> -	/* Do not register the subdev notifier if there are no devices. */
> -	if (!priv->nsources)
> -		return 0;
> -
>  	priv->route_mask = priv->source_mask;
> -	priv->notifier.ops = &max9286_notify_ops;
> -
> -	ret = v4l2_async_subdev_notifier_register(&priv->sd, &priv->notifier);
> -	if (ret)
> -		v4l2_async_notifier_cleanup(&priv->notifier);
>  
> -	return ret;
> +	return 0;
>  }
>  
>  static int max9286_probe(struct i2c_client *client)
> @@ -1182,6 +1203,7 @@ static int max9286_remove(struct i2c_client *client)
>  
>  	fwnode_handle_put(priv->sd.fwnode);
>  	v4l2_async_unregister_subdev(&priv->sd);
> +	max9286_v4l2_async_unregister(priv);
>  
>  	if (priv->poc_enabled)
>  		regulator_disable(priv->regulator);
> 

Re: [PATCH 1/2] max9286: Split out async registration

[Jacopo Mondi]

[-- Attachment #1: Type: text/plain, Size: 7473 bytes --]

Hi Kieran,
  very nice thanks for handling this

Just a few minors

On Wed, Feb 12, 2020 at 05:37:26PM +0000, Kieran Bingham wrote:
> Move all the V4L2 Subdev Async registration so that it can only happen once
> we know we will not need to -EPROBE_DEFER...
>
> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> ---
>  drivers/media/i2c/max9286.c | 88 +++++++++++++++++++++++--------------
>  1 file changed, 55 insertions(+), 33 deletions(-)
>
> diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> index 1b4ff3533795..03c5fa232b6d 100644
> --- a/drivers/media/i2c/max9286.c
> +++ b/drivers/media/i2c/max9286.c
> @@ -503,6 +503,49 @@ static const struct v4l2_async_notifier_operations max9286_notify_ops = {
>  	.unbind = max9286_notify_unbind,
>  };
>
> +static int max9286_v4l2_async_register(struct max9286_priv *priv)

Could you capture in the function name this actually deals with
notifiers ? Like max9286_notifier_register() or similar...

> +{
> +	struct device *dev = &priv->client->dev;
> +	struct max9286_source *source = NULL;
> +	int ret;
> +

Do we want to init and register the notifier if there are no sources
connected ? I would keep

	if (!priv->nsources)
		return 0;

here or in the caller.

> +	v4l2_async_notifier_init(&priv->notifier);
> +
> +	for_each_source(priv, source) {
> +		unsigned int i = to_index(priv, source);
> +
> +		dev_err(dev, "Registering v4l2-async for source %d\n", i);

dev_err ?

> +
> +		source->asd.match_type = V4L2_ASYNC_MATCH_FWNODE;
> +		source->asd.match.fwnode = source->fwnode;
> +
> +		ret = v4l2_async_notifier_add_subdev(&priv->notifier,
> +						     &source->asd);
> +		if (ret) {
> +			dev_err(dev, "Failed to add subdev for source %d", i);
> +			v4l2_async_notifier_cleanup(&priv->notifier);

v4l2_async_notifier_cleanup() does fwnode_handle_put() on the async
subdevs registered to the notifier but not yet completed. All the other
sources have to be put as well I think.

How to do so might be not nice, as you would need to keep track of which
sources have been registered to the notifier already and put the other
ones in the error path.

> +			return ret;
> +		}
> +	}
> +
> +	priv->notifier.ops = &max9286_notify_ops;
> +
> +	ret = v4l2_async_subdev_notifier_register(&priv->sd, &priv->notifier);
> +	if (ret) {
> +		dev_err(dev, "Failed to register subdev_notifier");
> +		v4l2_async_notifier_cleanup(&priv->notifier);

Here it's fine to call notifier_cleanup()

> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static void max9286_v4l2_async_unregister(struct max9286_priv *priv)
> +{
> +	v4l2_async_notifier_unregister(&priv->notifier);
> +	v4l2_async_notifier_cleanup(&priv->notifier);

I would first cleanup() then unregister() even if they deal with two
different sets of asds (done and registred-but-not-yet-done ones).

> +}
> +
>  static int max9286_s_stream(struct v4l2_subdev *sd, int enable)
>  {
>  	struct max9286_priv *priv = sd_to_max9286(sd);
> @@ -870,6 +913,13 @@ static int max9286_init(struct device *dev)
>  		goto err_regulator;
>  	}
>
> +	/* Register v4l2 async notifiers */
> +	ret = max9286_v4l2_async_register(priv);
> +	if (ret) {
> +		dev_err(dev, "Unable to register V4L2 async notifiers\n");
> +		goto err_regulator;
> +	}
> +
>  	v4l2_i2c_subdev_init(&priv->sd, client, &max9286_subdev_ops);
>  	priv->sd.internal_ops = &max9286_subdev_internal_ops;
>  	priv->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> @@ -884,7 +934,7 @@ static int max9286_init(struct device *dev)
>  	priv->sd.ctrl_handler = &priv->ctrls;
>  	ret = priv->ctrls.error;
>  	if (ret)
> -		goto err_regulator;
> +		goto err_async;
>
>  	priv->sd.entity.function = MEDIA_ENT_F_VID_IF_BRIDGE;
>
> @@ -927,6 +977,8 @@ static int max9286_init(struct device *dev)
>  	max9286_i2c_mux_close(priv);
>  err_put_node:
>  	fwnode_handle_put(ep);
> +err_async:
> +	max9286_v4l2_async_unregister(priv);
>  err_regulator:
>  	regulator_disable(priv->regulator);
>  	priv->poc_enabled = false;
> @@ -938,14 +990,6 @@ static void max9286_cleanup_dt(struct max9286_priv *priv)
>  {
>  	struct max9286_source *source;
>
> -	/*
> -	 * Not strictly part of the DT, but the notifier is registered during
> -	 * max9286_parse_dt(), and the notifier holds references to the fwnodes
> -	 * thus the cleanup is here to mirror the registration.
> -	 */
> -	v4l2_async_notifier_unregister(&priv->notifier);
> -	v4l2_async_notifier_cleanup(&priv->notifier);
> -
>  	for_each_source(priv, source) {
>  		fwnode_handle_put(source->fwnode);

Wasn't this a double fwnode_handle_put() ? We called
notifier_cleanup() and then put all sources again manually.

I don't see one more get() when the asd gets registered to the
notifier with v4l2_async_notifier_add_subdev() so I'm afraid this
would result in a duplicated put(). Am I wrong ?

It was there already, but I think it happens in this patch as
well: if max9286_init() fails calls max9286_v4l2_unregister() which
then calls max9286_v4l2_async_unregister() which put all the
not-yet-completed subdevs by calling v4l2_async_notifier_cleanup().
Then in the probe() function error path we then call
max9286_cleanup_dt() which puts again all the registered sources
regardless of their completed status.

I would call max9286_cleanup_dt() only if max9286_init() has not been
called yet. If we get to register subdevs to the notifier, I would
then provide a function that calls v4l2_async_notifier_cleanup() and
then manually puts all sources not yet registered. I'm afraid this
would need to keep a status flag in the source, unless you have a more
elegant solution.

Thanks
   j

>  		source->fwnode = NULL;
> @@ -958,7 +1002,6 @@ static int max9286_parse_dt(struct max9286_priv *priv)
>  	struct device_node *i2c_mux;
>  	struct device_node *node = NULL;
>  	unsigned int i2c_mux_mask = 0;
> -	int ret;
>
>  	of_node_get(dev->of_node);
>  	i2c_mux = of_find_node_by_name(dev->of_node, "i2c-mux");
> @@ -986,8 +1029,6 @@ static int max9286_parse_dt(struct max9286_priv *priv)
>  	of_node_put(node);
>  	of_node_put(i2c_mux);
>
> -	v4l2_async_notifier_init(&priv->notifier);
> -
>  	/* Parse the endpoints */
>  	for_each_endpoint_of_node(dev->of_node, node) {
>  		struct max9286_source *source;
> @@ -1056,34 +1097,14 @@ static int max9286_parse_dt(struct max9286_priv *priv)
>  			continue;
>  		}
>
> -		source->asd.match_type = V4L2_ASYNC_MATCH_FWNODE;
> -		source->asd.match.fwnode = source->fwnode;
> -
> -		ret = v4l2_async_notifier_add_subdev(&priv->notifier,
> -						     &source->asd);
> -		if (ret) {
> -			v4l2_async_notifier_cleanup(&priv->notifier);
> -			of_node_put(node);
> -			return ret;
> -		}
> -
>  		priv->source_mask |= BIT(ep.port);
>  		priv->nsources++;
>  	}
>  	of_node_put(node);
>
> -	/* Do not register the subdev notifier if there are no devices. */
> -	if (!priv->nsources)
> -		return 0;
> -
>  	priv->route_mask = priv->source_mask;
> -	priv->notifier.ops = &max9286_notify_ops;
> -
> -	ret = v4l2_async_subdev_notifier_register(&priv->sd, &priv->notifier);
> -	if (ret)
> -		v4l2_async_notifier_cleanup(&priv->notifier);
>
> -	return ret;
> +	return 0;
>  }
>
>  static int max9286_probe(struct i2c_client *client)
> @@ -1182,6 +1203,7 @@ static int max9286_remove(struct i2c_client *client)
>
>  	fwnode_handle_put(priv->sd.fwnode);
>  	v4l2_async_unregister_subdev(&priv->sd);
> +	max9286_v4l2_async_unregister(priv);
>
>  	if (priv->poc_enabled)
>  		regulator_disable(priv->regulator);
> --
> 2.20.1
>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/2] max9286: Split out async registration

[Kieran Bingham]

Hi Jacopo,

On 13/02/2020 09:46, Jacopo Mondi wrote:
> Hi Kieran,
>   very nice thanks for handling this

:-)

> Just a few minors

:-s hehe

> 
> On Wed, Feb 12, 2020 at 05:37:26PM +0000, Kieran Bingham wrote:
>> Move all the V4L2 Subdev Async registration so that it can only happen once
>> we know we will not need to -EPROBE_DEFER...
>>
>> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
>> ---
>>  drivers/media/i2c/max9286.c | 88 +++++++++++++++++++++++--------------
>>  1 file changed, 55 insertions(+), 33 deletions(-)
>>
>> diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
>> index 1b4ff3533795..03c5fa232b6d 100644
>> --- a/drivers/media/i2c/max9286.c
>> +++ b/drivers/media/i2c/max9286.c
>> @@ -503,6 +503,49 @@ static const struct v4l2_async_notifier_operations max9286_notify_ops = {
>>  	.unbind = max9286_notify_unbind,
>>  };
>>
>> +static int max9286_v4l2_async_register(struct max9286_priv *priv)
> 
> Could you capture in the function name this actually deals with
> notifiers ? Like max9286_notifier_register() or similar...

I'd like to keep the 'v4l2' in there somewhere...

	max9286_v4l2_notifier_register() ?

But then maybe even that could be confused with the notifiers/async
handling for the max9286 itself.

My aim was that max9286_v4l2_async_{un,}register() dealt with subdevices
connected to the max9286 only ...

For ~20 lines of code, it could be inlined up a level into
max9286_v4l2_register() but I do perhaps like trying to keep the
symmetry clean somehow.

<scratch that - below tells me not to inline>


>> +{
>> +	struct device *dev = &priv->client->dev;
>> +	struct max9286_source *source = NULL;
>> +	int ret;
>> +
> 
> Do we want to init and register the notifier if there are no sources
> connected ? I would keep
> 
> 	if (!priv->nsources)
> 		return 0;
> 
> here or in the caller.

Ah yes, I had thought about that but clearly not acted upon it much.

If we know there is nothing to notify us, I guess we won't expect any
need to register the notifications!

Although this would certainly mean keeping the sources registration in
their own functions.


> 
>> +	v4l2_async_notifier_init(&priv->notifier);
>> +
>> +	for_each_source(priv, source) {
>> +		unsigned int i = to_index(priv, source);
>> +
>> +		dev_err(dev, "Registering v4l2-async for source %d\n", i);
> 
> dev_err ?
> 

Already removed, left over debug print.


>> +
>> +		source->asd.match_type = V4L2_ASYNC_MATCH_FWNODE;
>> +		source->asd.match.fwnode = source->fwnode;
>> +
>> +		ret = v4l2_async_notifier_add_subdev(&priv->notifier,
>> +						     &source->asd);
>> +		if (ret) {
>> +			dev_err(dev, "Failed to add subdev for source %d", i);
>> +			v4l2_async_notifier_cleanup(&priv->notifier);
> 
> v4l2_async_notifier_cleanup() does fwnode_handle_put() on the async
> subdevs registered to the notifier but not yet completed. All the other
> sources have to be put as well I think.
> 
> How to do so might be not nice, as you would need to keep track of which
> sources have been registered to the notifier already and put the other
> ones in the error path.

Or can we move all fwnode refcounting back to cleanup_dt perhaps?

I'll have a look.

> 
>> +			return ret;
>> +		}
>> +	}
>> +
>> +	priv->notifier.ops = &max9286_notify_ops;
>> +
>> +	ret = v4l2_async_subdev_notifier_register(&priv->sd, &priv->notifier);
>> +	if (ret) {
>> +		dev_err(dev, "Failed to register subdev_notifier");
>> +		v4l2_async_notifier_cleanup(&priv->notifier);
> 
> Here it's fine to call notifier_cleanup()
> 
>> +		return ret;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static void max9286_v4l2_async_unregister(struct max9286_priv *priv)
>> +{
>> +	v4l2_async_notifier_unregister(&priv->notifier);
>> +	v4l2_async_notifier_cleanup(&priv->notifier);
> 
> I would first cleanup() then unregister() even if they deal with two
> different sets of asds (done and registred-but-not-yet-done ones).


Looking at max9286_v4l2_async_register(), the
v4l2_async_subdev_notifier_register() call is last.

Therefore that would make it the first thing to cleanup in the reverse
procedure?


> 
>> +}
>> +
>>  static int max9286_s_stream(struct v4l2_subdev *sd, int enable)
>>  {
>>  	struct max9286_priv *priv = sd_to_max9286(sd);
>> @@ -870,6 +913,13 @@ static int max9286_init(struct device *dev)
>>  		goto err_regulator;
>>  	}
>>
>> +	/* Register v4l2 async notifiers */
>> +	ret = max9286_v4l2_async_register(priv);
>> +	if (ret) {
>> +		dev_err(dev, "Unable to register V4L2 async notifiers\n");
>> +		goto err_regulator;
>> +	}
>> +
>>  	v4l2_i2c_subdev_init(&priv->sd, client, &max9286_subdev_ops);
>>  	priv->sd.internal_ops = &max9286_subdev_internal_ops;
>>  	priv->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
>> @@ -884,7 +934,7 @@ static int max9286_init(struct device *dev)
>>  	priv->sd.ctrl_handler = &priv->ctrls;
>>  	ret = priv->ctrls.error;
>>  	if (ret)
>> -		goto err_regulator;
>> +		goto err_async;
>>
>>  	priv->sd.entity.function = MEDIA_ENT_F_VID_IF_BRIDGE;
>>
>> @@ -927,6 +977,8 @@ static int max9286_init(struct device *dev)
>>  	max9286_i2c_mux_close(priv);
>>  err_put_node:
>>  	fwnode_handle_put(ep);
>> +err_async:
>> +	max9286_v4l2_async_unregister(priv);
>>  err_regulator:
>>  	regulator_disable(priv->regulator);
>>  	priv->poc_enabled = false;
>> @@ -938,14 +990,6 @@ static void max9286_cleanup_dt(struct max9286_priv *priv)
>>  {
>>  	struct max9286_source *source;
>>
>> -	/*
>> -	 * Not strictly part of the DT, but the notifier is registered during
>> -	 * max9286_parse_dt(), and the notifier holds references to the fwnodes
>> -	 * thus the cleanup is here to mirror the registration.
>> -	 */
>> -	v4l2_async_notifier_unregister(&priv->notifier);
>> -	v4l2_async_notifier_cleanup(&priv->notifier);
>> -
>>  	for_each_source(priv, source) {
>>  		fwnode_handle_put(source->fwnode);
> 
> Wasn't this a double fwnode_handle_put() ? We called
> notifier_cleanup() and then put all sources again manually.
> 
> I don't see one more get() when the asd gets registered to the
> notifier with v4l2_async_notifier_add_subdev() so I'm afraid this
> would result in a duplicated put(). Am I wrong ?

Agh, all the implicit transfers of ownerships for refcnting with fwnodes
is horrible.

I hadn't actually noticed that _notifier_cleanup() was doing
fwnode_handle_puts() ...

But indeed, all of that was pre-existing before this patch series. Not
something I was looking to modify as part of this patch.

Lets fix that issue on top.

(which is going to get squashed in all the same, but I'm not going to
change /this/ patch for it)


> It was there already, but I think it happens in this patch as
> well: if max9286_init() fails calls max9286_v4l2_unregister() which
> then calls max9286_v4l2_async_unregister() which put all the
> not-yet-completed subdevs by calling v4l2_async_notifier_cleanup().
> Then in the probe() function error path we then call
> max9286_cleanup_dt() which puts again all the registered sources
> regardless of their completed status.

> I would call max9286_cleanup_dt() only if max9286_init() has not been
> called yet. If we get to register subdevs to the notifier, I would
> then provide a function that calls v4l2_async_notifier_cleanup() and
> then manually puts all sources not yet registered. I'm afraid this
> would need to keep a status flag in the source, unless you have a more
> elegant solution.

It seems like we also have the issue where we need to cleanup partially
registered sources, (i.e. if some have registered, and some haven't) ...
so how about a per-source flag to note that the device /got/ registered,
and thus 'ownership' moved to V4L2 v4l2_async_notifier_cleanup() to _put().

Then we can maintain a single cleanup function still, and it will be
handled on a per-node basis.

--
KB



> 
> Thanks
>    j
> 
>>  		source->fwnode = NULL;
>> @@ -958,7 +1002,6 @@ static int max9286_parse_dt(struct max9286_priv *priv)
>>  	struct device_node *i2c_mux;
>>  	struct device_node *node = NULL;
>>  	unsigned int i2c_mux_mask = 0;
>> -	int ret;
>>
>>  	of_node_get(dev->of_node);
>>  	i2c_mux = of_find_node_by_name(dev->of_node, "i2c-mux");
>> @@ -986,8 +1029,6 @@ static int max9286_parse_dt(struct max9286_priv *priv)
>>  	of_node_put(node);
>>  	of_node_put(i2c_mux);
>>
>> -	v4l2_async_notifier_init(&priv->notifier);
>> -
>>  	/* Parse the endpoints */
>>  	for_each_endpoint_of_node(dev->of_node, node) {
>>  		struct max9286_source *source;
>> @@ -1056,34 +1097,14 @@ static int max9286_parse_dt(struct max9286_priv *priv)
>>  			continue;
>>  		}
>>
>> -		source->asd.match_type = V4L2_ASYNC_MATCH_FWNODE;
>> -		source->asd.match.fwnode = source->fwnode;
>> -
>> -		ret = v4l2_async_notifier_add_subdev(&priv->notifier,
>> -						     &source->asd);
>> -		if (ret) {
>> -			v4l2_async_notifier_cleanup(&priv->notifier);
>> -			of_node_put(node);
>> -			return ret;
>> -		}
>> -
>>  		priv->source_mask |= BIT(ep.port);
>>  		priv->nsources++;
>>  	}
>>  	of_node_put(node);
>>
>> -	/* Do not register the subdev notifier if there are no devices. */
>> -	if (!priv->nsources)
>> -		return 0;
>> -
>>  	priv->route_mask = priv->source_mask;
>> -	priv->notifier.ops = &max9286_notify_ops;
>> -
>> -	ret = v4l2_async_subdev_notifier_register(&priv->sd, &priv->notifier);
>> -	if (ret)
>> -		v4l2_async_notifier_cleanup(&priv->notifier);
>>
>> -	return ret;
>> +	return 0;
>>  }
>>
>>  static int max9286_probe(struct i2c_client *client)
>> @@ -1182,6 +1203,7 @@ static int max9286_remove(struct i2c_client *client)
>>
>>  	fwnode_handle_put(priv->sd.fwnode);
>>  	v4l2_async_unregister_subdev(&priv->sd);
>> +	max9286_v4l2_async_unregister(priv);
>>
>>  	if (priv->poc_enabled)
>>  		regulator_disable(priv->regulator);
>> --
>> 2.20.1
>>

Re: [PATCH 1/2] max9286: Split out async registration

[Kieran Bingham]

Hi Jacopo,

On 13/02/2020 10:07, Kieran Bingham wrote:
> Hi Jacopo,
> 
> On 13/02/2020 09:46, Jacopo Mondi wrote:
>> Hi Kieran,
>>   very nice thanks for handling this
> 
> :-)
> 
>> Just a few minors
> 
> :-s hehe
> 
>>
>> On Wed, Feb 12, 2020 at 05:37:26PM +0000, Kieran Bingham wrote:
>>> Move all the V4L2 Subdev Async registration so that it can only happen once
>>> we know we will not need to -EPROBE_DEFER...
>>>
>>> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
>>> ---
>>>  drivers/media/i2c/max9286.c | 88 +++++++++++++++++++++++--------------
>>>  1 file changed, 55 insertions(+), 33 deletions(-)
>>>
>>> diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
>>> index 1b4ff3533795..03c5fa232b6d 100644
>>> --- a/drivers/media/i2c/max9286.c
>>> +++ b/drivers/media/i2c/max9286.c
>>> @@ -503,6 +503,49 @@ static const struct v4l2_async_notifier_operations max9286_notify_ops = {
>>>  	.unbind = max9286_notify_unbind,
>>>  };
>>>
>>> +static int max9286_v4l2_async_register(struct max9286_priv *priv)
>>
>> Could you capture in the function name this actually deals with
>> notifiers ? Like max9286_notifier_register() or similar...
> 
> I'd like to keep the 'v4l2' in there somewhere...
> 
> 	max9286_v4l2_notifier_register() ?
> 
> But then maybe even that could be confused with the notifiers/async
> handling for the max9286 itself.
> 
> My aim was that max9286_v4l2_async_{un,}register() dealt with subdevices
> connected to the max9286 only ...
> 
> For ~20 lines of code, it could be inlined up a level into
> max9286_v4l2_register() but I do perhaps like trying to keep the
> symmetry clean somehow.
> 
> <scratch that - below tells me not to inline>
> 
> 
>>> +{
>>> +	struct device *dev = &priv->client->dev;
>>> +	struct max9286_source *source = NULL;
>>> +	int ret;
>>> +
>>
>> Do we want to init and register the notifier if there are no sources
>> connected ? I would keep
>>
>> 	if (!priv->nsources)
>> 		return 0;
>>
>> here or in the caller.
> 
> Ah yes, I had thought about that but clearly not acted upon it much.
> 
> If we know there is nothing to notify us, I guess we won't expect any
> need to register the notifications!
> 
> Although this would certainly mean keeping the sources registration in
> their own functions.
> 
> 
>>
>>> +	v4l2_async_notifier_init(&priv->notifier);
>>> +
>>> +	for_each_source(priv, source) {
>>> +		unsigned int i = to_index(priv, source);
>>> +
>>> +		dev_err(dev, "Registering v4l2-async for source %d\n", i);
>>
>> dev_err ?
>>
> 
> Already removed, left over debug print.
> 
> 
>>> +
>>> +		source->asd.match_type = V4L2_ASYNC_MATCH_FWNODE;
>>> +		source->asd.match.fwnode = source->fwnode;
>>> +
>>> +		ret = v4l2_async_notifier_add_subdev(&priv->notifier,
>>> +						     &source->asd);
>>> +		if (ret) {
>>> +			dev_err(dev, "Failed to add subdev for source %d", i);
>>> +			v4l2_async_notifier_cleanup(&priv->notifier);
>>
>> v4l2_async_notifier_cleanup() does fwnode_handle_put() on the async
>> subdevs registered to the notifier but not yet completed. All the other
>> sources have to be put as well I think.
>>
>> How to do so might be not nice, as you would need to keep track of which
>> sources have been registered to the notifier already and put the other
>> ones in the error path.
> 
> Or can we move all fwnode refcounting back to cleanup_dt perhaps?
> 
> I'll have a look.
> 
>>
>>> +			return ret;
>>> +		}
>>> +	}
>>> +
>>> +	priv->notifier.ops = &max9286_notify_ops;
>>> +
>>> +	ret = v4l2_async_subdev_notifier_register(&priv->sd, &priv->notifier);
>>> +	if (ret) {
>>> +		dev_err(dev, "Failed to register subdev_notifier");
>>> +		v4l2_async_notifier_cleanup(&priv->notifier);
>>
>> Here it's fine to call notifier_cleanup()
>>
>>> +		return ret;
>>> +	}
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static void max9286_v4l2_async_unregister(struct max9286_priv *priv)
>>> +{
>>> +	v4l2_async_notifier_unregister(&priv->notifier);
>>> +	v4l2_async_notifier_cleanup(&priv->notifier);
>>
>> I would first cleanup() then unregister() even if they deal with two
>> different sets of asds (done and registred-but-not-yet-done ones).
> 
> 
> Looking at max9286_v4l2_async_register(), the
> v4l2_async_subdev_notifier_register() call is last.
> 
> Therefore that would make it the first thing to cleanup in the reverse
> procedure?
> 
> 
>>
>>> +}
>>> +
>>>  static int max9286_s_stream(struct v4l2_subdev *sd, int enable)
>>>  {
>>>  	struct max9286_priv *priv = sd_to_max9286(sd);
>>> @@ -870,6 +913,13 @@ static int max9286_init(struct device *dev)
>>>  		goto err_regulator;
>>>  	}
>>>
>>> +	/* Register v4l2 async notifiers */
>>> +	ret = max9286_v4l2_async_register(priv);
>>> +	if (ret) {
>>> +		dev_err(dev, "Unable to register V4L2 async notifiers\n");
>>> +		goto err_regulator;
>>> +	}
>>> +
>>>  	v4l2_i2c_subdev_init(&priv->sd, client, &max9286_subdev_ops);
>>>  	priv->sd.internal_ops = &max9286_subdev_internal_ops;
>>>  	priv->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
>>> @@ -884,7 +934,7 @@ static int max9286_init(struct device *dev)
>>>  	priv->sd.ctrl_handler = &priv->ctrls;
>>>  	ret = priv->ctrls.error;
>>>  	if (ret)
>>> -		goto err_regulator;
>>> +		goto err_async;
>>>
>>>  	priv->sd.entity.function = MEDIA_ENT_F_VID_IF_BRIDGE;
>>>
>>> @@ -927,6 +977,8 @@ static int max9286_init(struct device *dev)
>>>  	max9286_i2c_mux_close(priv);
>>>  err_put_node:
>>>  	fwnode_handle_put(ep);
>>> +err_async:
>>> +	max9286_v4l2_async_unregister(priv);
>>>  err_regulator:
>>>  	regulator_disable(priv->regulator);
>>>  	priv->poc_enabled = false;
>>> @@ -938,14 +990,6 @@ static void max9286_cleanup_dt(struct max9286_priv *priv)
>>>  {
>>>  	struct max9286_source *source;
>>>
>>> -	/*
>>> -	 * Not strictly part of the DT, but the notifier is registered during
>>> -	 * max9286_parse_dt(), and the notifier holds references to the fwnodes
>>> -	 * thus the cleanup is here to mirror the registration.
>>> -	 */
>>> -	v4l2_async_notifier_unregister(&priv->notifier);
>>> -	v4l2_async_notifier_cleanup(&priv->notifier);
>>> -
>>>  	for_each_source(priv, source) {
>>>  		fwnode_handle_put(source->fwnode);
>>
>> Wasn't this a double fwnode_handle_put() ? We called
>> notifier_cleanup() and then put all sources again manually.
>>
>> I don't see one more get() when the asd gets registered to the
>> notifier with v4l2_async_notifier_add_subdev() so I'm afraid this
>> would result in a duplicated put(). Am I wrong ?
> 
> Agh, all the implicit transfers of ownerships for refcnting with fwnodes
> is horrible.
> 
> I hadn't actually noticed that _notifier_cleanup() was doing
> fwnode_handle_puts() ...
> 
> But indeed, all of that was pre-existing before this patch series. Not
> something I was looking to modify as part of this patch.
> 
> Lets fix that issue on top.
> 
> (which is going to get squashed in all the same, but I'm not going to
> change /this/ patch for it)

Perhaps the simplest option in this case actually is to do a
fwnode_handle_get() on a successful async_notifier_add_subdev().

That will keep the refcnting balanced.

>> It was there already, but I think it happens in this patch as
>> well: if max9286_init() fails calls max9286_v4l2_unregister() which
>> then calls max9286_v4l2_async_unregister() which put all the
>> not-yet-completed subdevs by calling v4l2_async_notifier_cleanup().
>> Then in the probe() function error path we then call
>> max9286_cleanup_dt() which puts again all the registered sources
>> regardless of their completed status.
> 
>> I would call max9286_cleanup_dt() only if max9286_init() has not been
>> called yet. If we get to register subdevs to the notifier, I would
>> then provide a function that calls v4l2_async_notifier_cleanup() and
>> then manually puts all sources not yet registered. I'm afraid this
>> would need to keep a status flag in the source, unless you have a more
>> elegant solution.
> 
> It seems like we also have the issue where we need to cleanup partially
> registered sources, (i.e. if some have registered, and some haven't) ...
> so how about a per-source flag to note that the device /got/ registered,
> and thus 'ownership' moved to V4L2 v4l2_async_notifier_cleanup() to _put().
> 
> Then we can maintain a single cleanup function still, and it will be
> handled on a per-node basis.

And I think the extra 'fwnode_handle_get()' would resolve this too?


> 
> --
> KB
> 
> 
> 
>>
>> Thanks
>>    j
>>
>>>  		source->fwnode = NULL;
>>> @@ -958,7 +1002,6 @@ static int max9286_parse_dt(struct max9286_priv *priv)
>>>  	struct device_node *i2c_mux;
>>>  	struct device_node *node = NULL;
>>>  	unsigned int i2c_mux_mask = 0;
>>> -	int ret;
>>>
>>>  	of_node_get(dev->of_node);
>>>  	i2c_mux = of_find_node_by_name(dev->of_node, "i2c-mux");
>>> @@ -986,8 +1029,6 @@ static int max9286_parse_dt(struct max9286_priv *priv)
>>>  	of_node_put(node);
>>>  	of_node_put(i2c_mux);
>>>
>>> -	v4l2_async_notifier_init(&priv->notifier);
>>> -
>>>  	/* Parse the endpoints */
>>>  	for_each_endpoint_of_node(dev->of_node, node) {
>>>  		struct max9286_source *source;
>>> @@ -1056,34 +1097,14 @@ static int max9286_parse_dt(struct max9286_priv *priv)
>>>  			continue;
>>>  		}
>>>
>>> -		source->asd.match_type = V4L2_ASYNC_MATCH_FWNODE;
>>> -		source->asd.match.fwnode = source->fwnode;
>>> -
>>> -		ret = v4l2_async_notifier_add_subdev(&priv->notifier,
>>> -						     &source->asd);
>>> -		if (ret) {
>>> -			v4l2_async_notifier_cleanup(&priv->notifier);
>>> -			of_node_put(node);
>>> -			return ret;
>>> -		}
>>> -
>>>  		priv->source_mask |= BIT(ep.port);
>>>  		priv->nsources++;
>>>  	}
>>>  	of_node_put(node);
>>>
>>> -	/* Do not register the subdev notifier if there are no devices. */
>>> -	if (!priv->nsources)
>>> -		return 0;
>>> -
>>>  	priv->route_mask = priv->source_mask;
>>> -	priv->notifier.ops = &max9286_notify_ops;
>>> -
>>> -	ret = v4l2_async_subdev_notifier_register(&priv->sd, &priv->notifier);
>>> -	if (ret)
>>> -		v4l2_async_notifier_cleanup(&priv->notifier);
>>>
>>> -	return ret;
>>> +	return 0;
>>>  }
>>>
>>>  static int max9286_probe(struct i2c_client *client)
>>> @@ -1182,6 +1203,7 @@ static int max9286_remove(struct i2c_client *client)
>>>
>>>  	fwnode_handle_put(priv->sd.fwnode);
>>>  	v4l2_async_unregister_subdev(&priv->sd);
>>> +	max9286_v4l2_async_unregister(priv);
>>>
>>>  	if (priv->poc_enabled)
>>>  		regulator_disable(priv->regulator);
>>> --
>>> 2.20.1
>>>
> 

Re: [PATCH 1/2] max9286: Split out async registration

[Jacopo Mondi]

[-- Attachment #1: Type: text/plain, Size: 11051 bytes --]

Hi Kieran,

On Thu, Feb 13, 2020 at 10:07:18AM +0000, Kieran Bingham wrote:
> Hi Jacopo,
>
> On 13/02/2020 09:46, Jacopo Mondi wrote:
> > Hi Kieran,
> >   very nice thanks for handling this
>
> :-)
>
> > Just a few minors
>
> :-s hehe
>

Turned out to be lengthier than expected :)

> >
> > On Wed, Feb 12, 2020 at 05:37:26PM +0000, Kieran Bingham wrote:
> >> Move all the V4L2 Subdev Async registration so that it can only happen once
> >> we know we will not need to -EPROBE_DEFER...
> >>
> >> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> >> ---
> >>  drivers/media/i2c/max9286.c | 88 +++++++++++++++++++++++--------------
> >>  1 file changed, 55 insertions(+), 33 deletions(-)
> >>
> >> diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> >> index 1b4ff3533795..03c5fa232b6d 100644
> >> --- a/drivers/media/i2c/max9286.c
> >> +++ b/drivers/media/i2c/max9286.c
> >> @@ -503,6 +503,49 @@ static const struct v4l2_async_notifier_operations max9286_notify_ops = {
> >>  	.unbind = max9286_notify_unbind,
> >>  };
> >>
> >> +static int max9286_v4l2_async_register(struct max9286_priv *priv)
> >
> > Could you capture in the function name this actually deals with
> > notifiers ? Like max9286_notifier_register() or similar...
>
> I'd like to keep the 'v4l2' in there somewhere...
>
> 	max9286_v4l2_notifier_register() ?
>
> But then maybe even that could be confused with the notifiers/async
> handling for the max9286 itself.
>
> My aim was that max9286_v4l2_async_{un,}register() dealt with subdevices
> connected to the max9286 only ...

To me async_register() calls for dealing with registering our own
subdev to the async framework, not collecting remote asds and adding
it our subnotifier. As you wish, it's really just a suggestion.

>
> For ~20 lines of code, it could be inlined up a level into
> max9286_v4l2_register() but I do perhaps like trying to keep the
> symmetry clean somehow.
>
> <scratch that - below tells me not to inline>
>
>
> >> +{
> >> +	struct device *dev = &priv->client->dev;
> >> +	struct max9286_source *source = NULL;
> >> +	int ret;
> >> +
> >
> > Do we want to init and register the notifier if there are no sources
> > connected ? I would keep
> >
> > 	if (!priv->nsources)
> > 		return 0;
> >
> > here or in the caller.
>
> Ah yes, I had thought about that but clearly not acted upon it much.
>
> If we know there is nothing to notify us, I guess we won't expect any
> need to register the notifications!
>
> Although this would certainly mean keeping the sources registration in
> their own functions.
>
>
> >
> >> +	v4l2_async_notifier_init(&priv->notifier);
> >> +
> >> +	for_each_source(priv, source) {
> >> +		unsigned int i = to_index(priv, source);
> >> +
> >> +		dev_err(dev, "Registering v4l2-async for source %d\n", i);
> >
> > dev_err ?
> >
>
> Already removed, left over debug print.
>
>
> >> +
> >> +		source->asd.match_type = V4L2_ASYNC_MATCH_FWNODE;
> >> +		source->asd.match.fwnode = source->fwnode;
> >> +
> >> +		ret = v4l2_async_notifier_add_subdev(&priv->notifier,
> >> +						     &source->asd);
> >> +		if (ret) {
> >> +			dev_err(dev, "Failed to add subdev for source %d", i);
> >> +			v4l2_async_notifier_cleanup(&priv->notifier);
> >
> > v4l2_async_notifier_cleanup() does fwnode_handle_put() on the async
> > subdevs registered to the notifier but not yet completed. All the other
> > sources have to be put as well I think.
> >
> > How to do so might be not nice, as you would need to keep track of which
> > sources have been registered to the notifier already and put the other
> > ones in the error path.
>
> Or can we move all fwnode refcounting back to cleanup_dt perhaps?
>
> I'll have a look.
>
> >
> >> +			return ret;
> >> +		}
> >> +	}
> >> +
> >> +	priv->notifier.ops = &max9286_notify_ops;
> >> +
> >> +	ret = v4l2_async_subdev_notifier_register(&priv->sd, &priv->notifier);
> >> +	if (ret) {
> >> +		dev_err(dev, "Failed to register subdev_notifier");
> >> +		v4l2_async_notifier_cleanup(&priv->notifier);
> >
> > Here it's fine to call notifier_cleanup()
> >
> >> +		return ret;
> >> +	}
> >> +
> >> +	return 0;
> >> +}
> >> +
> >> +static void max9286_v4l2_async_unregister(struct max9286_priv *priv)
> >> +{
> >> +	v4l2_async_notifier_unregister(&priv->notifier);
> >> +	v4l2_async_notifier_cleanup(&priv->notifier);
> >
> > I would first cleanup() then unregister() even if they deal with two
> > different sets of asds (done and registred-but-not-yet-done ones).
>
>
> Looking at max9286_v4l2_async_register(), the
> v4l2_async_subdev_notifier_register() call is last.
>
> Therefore that would make it the first thing to cleanup in the reverse
> procedure?
>

Ok then, reverse order for symmetry is fine.

>
> >
> >> +}
> >> +
> >>  static int max9286_s_stream(struct v4l2_subdev *sd, int enable)
> >>  {
> >>  	struct max9286_priv *priv = sd_to_max9286(sd);
> >> @@ -870,6 +913,13 @@ static int max9286_init(struct device *dev)
> >>  		goto err_regulator;
> >>  	}
> >>
> >> +	/* Register v4l2 async notifiers */
> >> +	ret = max9286_v4l2_async_register(priv);
> >> +	if (ret) {
> >> +		dev_err(dev, "Unable to register V4L2 async notifiers\n");
> >> +		goto err_regulator;
> >> +	}
> >> +
> >>  	v4l2_i2c_subdev_init(&priv->sd, client, &max9286_subdev_ops);
> >>  	priv->sd.internal_ops = &max9286_subdev_internal_ops;
> >>  	priv->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> >> @@ -884,7 +934,7 @@ static int max9286_init(struct device *dev)
> >>  	priv->sd.ctrl_handler = &priv->ctrls;
> >>  	ret = priv->ctrls.error;
> >>  	if (ret)
> >> -		goto err_regulator;
> >> +		goto err_async;
> >>
> >>  	priv->sd.entity.function = MEDIA_ENT_F_VID_IF_BRIDGE;
> >>
> >> @@ -927,6 +977,8 @@ static int max9286_init(struct device *dev)
> >>  	max9286_i2c_mux_close(priv);
> >>  err_put_node:
> >>  	fwnode_handle_put(ep);
> >> +err_async:
> >> +	max9286_v4l2_async_unregister(priv);
> >>  err_regulator:
> >>  	regulator_disable(priv->regulator);
> >>  	priv->poc_enabled = false;
> >> @@ -938,14 +990,6 @@ static void max9286_cleanup_dt(struct max9286_priv *priv)
> >>  {
> >>  	struct max9286_source *source;
> >>
> >> -	/*
> >> -	 * Not strictly part of the DT, but the notifier is registered during
> >> -	 * max9286_parse_dt(), and the notifier holds references to the fwnodes
> >> -	 * thus the cleanup is here to mirror the registration.
> >> -	 */
> >> -	v4l2_async_notifier_unregister(&priv->notifier);
> >> -	v4l2_async_notifier_cleanup(&priv->notifier);
> >> -
> >>  	for_each_source(priv, source) {
> >>  		fwnode_handle_put(source->fwnode);
> >
> > Wasn't this a double fwnode_handle_put() ? We called
> > notifier_cleanup() and then put all sources again manually.
> >
> > I don't see one more get() when the asd gets registered to the
> > notifier with v4l2_async_notifier_add_subdev() so I'm afraid this
> > would result in a duplicated put(). Am I wrong ?
>
> Agh, all the implicit transfers of ownerships for refcnting with fwnodes
> is horrible.
>

Yup :(

> I hadn't actually noticed that _notifier_cleanup() was doing
> fwnode_handle_puts() ...
>
> But indeed, all of that was pre-existing before this patch series. Not
> something I was looking to modify as part of this patch.
>
> Lets fix that issue on top.
>

Fine with me

> (which is going to get squashed in all the same, but I'm not going to
> change /this/ patch for it)
>
>
> > It was there already, but I think it happens in this patch as
> > well: if max9286_init() fails calls max9286_v4l2_unregister() which
> > then calls max9286_v4l2_async_unregister() which put all the
> > not-yet-completed subdevs by calling v4l2_async_notifier_cleanup().
> > Then in the probe() function error path we then call
> > max9286_cleanup_dt() which puts again all the registered sources
> > regardless of their completed status.
>
> > I would call max9286_cleanup_dt() only if max9286_init() has not been
> > called yet. If we get to register subdevs to the notifier, I would
> > then provide a function that calls v4l2_async_notifier_cleanup() and
> > then manually puts all sources not yet registered. I'm afraid this
> > would need to keep a status flag in the source, unless you have a more
> > elegant solution.
>
> It seems like we also have the issue where we need to cleanup partially
> registered sources, (i.e. if some have registered, and some haven't) ...
> so how about a per-source flag to note that the device /got/ registered,
> and thus 'ownership' moved to V4L2 v4l2_async_notifier_cleanup() to _put().
>
> Then we can maintain a single cleanup function still, and it will be
> handled on a per-node basis.

That was my suggestion but indeed requires some state keeping in
place, so I hoped we could come up with something more elegant :)

But indeed, keeping a flag in the source to tell ownership has been
passed to the async framework would probably be enough, and would
allow us to possibly have a single cleanup function, yes.

>
> --
> KB
>
>
>
> >
> > Thanks
> >    j
> >
> >>  		source->fwnode = NULL;
> >> @@ -958,7 +1002,6 @@ static int max9286_parse_dt(struct max9286_priv *priv)
> >>  	struct device_node *i2c_mux;
> >>  	struct device_node *node = NULL;
> >>  	unsigned int i2c_mux_mask = 0;
> >> -	int ret;
> >>
> >>  	of_node_get(dev->of_node);
> >>  	i2c_mux = of_find_node_by_name(dev->of_node, "i2c-mux");
> >> @@ -986,8 +1029,6 @@ static int max9286_parse_dt(struct max9286_priv *priv)
> >>  	of_node_put(node);
> >>  	of_node_put(i2c_mux);
> >>
> >> -	v4l2_async_notifier_init(&priv->notifier);
> >> -
> >>  	/* Parse the endpoints */
> >>  	for_each_endpoint_of_node(dev->of_node, node) {
> >>  		struct max9286_source *source;
> >> @@ -1056,34 +1097,14 @@ static int max9286_parse_dt(struct max9286_priv *priv)
> >>  			continue;
> >>  		}
> >>
> >> -		source->asd.match_type = V4L2_ASYNC_MATCH_FWNODE;
> >> -		source->asd.match.fwnode = source->fwnode;
> >> -
> >> -		ret = v4l2_async_notifier_add_subdev(&priv->notifier,
> >> -						     &source->asd);
> >> -		if (ret) {
> >> -			v4l2_async_notifier_cleanup(&priv->notifier);
> >> -			of_node_put(node);
> >> -			return ret;
> >> -		}
> >> -
> >>  		priv->source_mask |= BIT(ep.port);
> >>  		priv->nsources++;
> >>  	}
> >>  	of_node_put(node);
> >>
> >> -	/* Do not register the subdev notifier if there are no devices. */
> >> -	if (!priv->nsources)
> >> -		return 0;
> >> -
> >>  	priv->route_mask = priv->source_mask;
> >> -	priv->notifier.ops = &max9286_notify_ops;
> >> -
> >> -	ret = v4l2_async_subdev_notifier_register(&priv->sd, &priv->notifier);
> >> -	if (ret)
> >> -		v4l2_async_notifier_cleanup(&priv->notifier);
> >>
> >> -	return ret;
> >> +	return 0;
> >>  }
> >>
> >>  static int max9286_probe(struct i2c_client *client)
> >> @@ -1182,6 +1203,7 @@ static int max9286_remove(struct i2c_client *client)
> >>
> >>  	fwnode_handle_put(priv->sd.fwnode);
> >>  	v4l2_async_unregister_subdev(&priv->sd);
> >> +	max9286_v4l2_async_unregister(priv);
> >>
> >>  	if (priv->poc_enabled)
> >>  		regulator_disable(priv->regulator);
> >> --
> >> 2.20.1
> >>
>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH] max9286: balance v4l2_async refcnting

[Kieran Bingham]

When we add fwnodes to V4L2 notifiers through
v4l2_async_notifier_add_subdev they are stored internally in V4L2 core,
and have a reference count released upon any call to
v4l2_async_notifier_cleanup().

Ensure that any source successfully added to a notifier gets its fwnode
reference count increased accordingly.

Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
---
 drivers/media/i2c/max9286.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
index f3311210a666..62615e6ab710 100644
--- a/drivers/media/i2c/max9286.c
+++ b/drivers/media/i2c/max9286.c
@@ -665,6 +665,11 @@ static int max9286_v4l2_async_register(struct max9286_priv *priv)
 			v4l2_async_notifier_cleanup(&priv->notifier);
 			return ret;
 		}
+
+		/* Balance the refernce counting handled through
+		 * v4l2_async_notifier_cleanup()
+		 */
+		fwnode_handle_get(source->fwnode);
 	}
 
 	priv->notifier.ops = &max9286_notify_ops;
-- 
2.20.1

Re: [PATCH 1/2] max9286: Split out async registration

[Kieran Bingham]

On 13/02/2020 10:20, Jacopo Mondi wrote:
> Hi Kieran,
> 
> On Thu, Feb 13, 2020 at 10:07:18AM +0000, Kieran Bingham wrote:
>> Hi Jacopo,
>>
>> On 13/02/2020 09:46, Jacopo Mondi wrote:
>>> Hi Kieran,
>>>   very nice thanks for handling this
>>
>> :-)
>>
>>> Just a few minors
>>
>> :-s hehe
>>
> 
> Turned out to be lengthier than expected :)
> 
>>>
>>> On Wed, Feb 12, 2020 at 05:37:26PM +0000, Kieran Bingham wrote:
>>>> Move all the V4L2 Subdev Async registration so that it can only happen once
>>>> we know we will not need to -EPROBE_DEFER...
>>>>
>>>> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
>>>> ---
>>>>  drivers/media/i2c/max9286.c | 88 +++++++++++++++++++++++--------------
>>>>  1 file changed, 55 insertions(+), 33 deletions(-)
>>>>
>>>> diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
>>>> index 1b4ff3533795..03c5fa232b6d 100644
>>>> --- a/drivers/media/i2c/max9286.c
>>>> +++ b/drivers/media/i2c/max9286.c
>>>> @@ -503,6 +503,49 @@ static const struct v4l2_async_notifier_operations max9286_notify_ops = {
>>>>  	.unbind = max9286_notify_unbind,
>>>>  };
>>>>
>>>> +static int max9286_v4l2_async_register(struct max9286_priv *priv)
>>>
>>> Could you capture in the function name this actually deals with
>>> notifiers ? Like max9286_notifier_register() or similar...
>>
>> I'd like to keep the 'v4l2' in there somewhere...
>>
>> 	max9286_v4l2_notifier_register() ?
>>
>> But then maybe even that could be confused with the notifiers/async
>> handling for the max9286 itself.
>>
>> My aim was that max9286_v4l2_async_{un,}register() dealt with subdevices
>> connected to the max9286 only ...
> 
> To me async_register() calls for dealing with registering our own
> subdev to the async framework, not collecting remote asds and adding
> it our subnotifier. As you wish, it's really just a suggestion.


So would you like to see the async registration between the max9286 and
the connected CSI2 receiver handled in this pair of functions too?

That would mean moveing the subdevice registrations later too - to after
all of the max9286 device creations but I think that's fine.


As long as the case between "if (priv->nsources)" is handled correctly
between the max9286 subdevices and the parent device it should be fine.



>> For ~20 lines of code, it could be inlined up a level into
>> max9286_v4l2_register() but I do perhaps like trying to keep the
>> symmetry clean somehow.
>>
>> <scratch that - below tells me not to inline>
>>
>>
>>>> +{
>>>> +	struct device *dev = &priv->client->dev;
>>>> +	struct max9286_source *source = NULL;
>>>> +	int ret;
>>>> +
>>>
>>> Do we want to init and register the notifier if there are no sources
>>> connected ? I would keep
>>>
>>> 	if (!priv->nsources)
>>> 		return 0;
>>>
>>> here or in the caller.
>>
>> Ah yes, I had thought about that but clearly not acted upon it much.
>>
>> If we know there is nothing to notify us, I guess we won't expect any
>> need to register the notifications!
>>
>> Although this would certainly mean keeping the sources registration in
>> their own functions.
>>
>>
>>>
>>>> +	v4l2_async_notifier_init(&priv->notifier);
>>>> +
>>>> +	for_each_source(priv, source) {
>>>> +		unsigned int i = to_index(priv, source);
>>>> +
>>>> +		dev_err(dev, "Registering v4l2-async for source %d\n", i);
>>>
>>> dev_err ?
>>>
>>
>> Already removed, left over debug print.
>>
>>
>>>> +
>>>> +		source->asd.match_type = V4L2_ASYNC_MATCH_FWNODE;
>>>> +		source->asd.match.fwnode = source->fwnode;
>>>> +
>>>> +		ret = v4l2_async_notifier_add_subdev(&priv->notifier,
>>>> +						     &source->asd);
>>>> +		if (ret) {
>>>> +			dev_err(dev, "Failed to add subdev for source %d", i);
>>>> +			v4l2_async_notifier_cleanup(&priv->notifier);
>>>
>>> v4l2_async_notifier_cleanup() does fwnode_handle_put() on the async
>>> subdevs registered to the notifier but not yet completed. All the other
>>> sources have to be put as well I think.
>>>
>>> How to do so might be not nice, as you would need to keep track of which
>>> sources have been registered to the notifier already and put the other
>>> ones in the error path.
>>
>> Or can we move all fwnode refcounting back to cleanup_dt perhaps?
>>
>> I'll have a look.
>>
>>>
>>>> +			return ret;
>>>> +		}
>>>> +	}
>>>> +
>>>> +	priv->notifier.ops = &max9286_notify_ops;
>>>> +
>>>> +	ret = v4l2_async_subdev_notifier_register(&priv->sd, &priv->notifier);
>>>> +	if (ret) {
>>>> +		dev_err(dev, "Failed to register subdev_notifier");
>>>> +		v4l2_async_notifier_cleanup(&priv->notifier);
>>>
>>> Here it's fine to call notifier_cleanup()
>>>
>>>> +		return ret;
>>>> +	}
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +static void max9286_v4l2_async_unregister(struct max9286_priv *priv)
>>>> +{
>>>> +	v4l2_async_notifier_unregister(&priv->notifier);
>>>> +	v4l2_async_notifier_cleanup(&priv->notifier);
>>>
>>> I would first cleanup() then unregister() even if they deal with two
>>> different sets of asds (done and registred-but-not-yet-done ones).
>>
>>
>> Looking at max9286_v4l2_async_register(), the
>> v4l2_async_subdev_notifier_register() call is last.
>>
>> Therefore that would make it the first thing to cleanup in the reverse
>> procedure?
>>
> 
> Ok then, reverse order for symmetry is fine.
> 
>>
>>>
>>>> +}
>>>> +
>>>>  static int max9286_s_stream(struct v4l2_subdev *sd, int enable)
>>>>  {
>>>>  	struct max9286_priv *priv = sd_to_max9286(sd);
>>>> @@ -870,6 +913,13 @@ static int max9286_init(struct device *dev)
>>>>  		goto err_regulator;
>>>>  	}
>>>>
>>>> +	/* Register v4l2 async notifiers */
>>>> +	ret = max9286_v4l2_async_register(priv);
>>>> +	if (ret) {
>>>> +		dev_err(dev, "Unable to register V4L2 async notifiers\n");
>>>> +		goto err_regulator;
>>>> +	}
>>>> +
>>>>  	v4l2_i2c_subdev_init(&priv->sd, client, &max9286_subdev_ops);
>>>>  	priv->sd.internal_ops = &max9286_subdev_internal_ops;
>>>>  	priv->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
>>>> @@ -884,7 +934,7 @@ static int max9286_init(struct device *dev)
>>>>  	priv->sd.ctrl_handler = &priv->ctrls;
>>>>  	ret = priv->ctrls.error;
>>>>  	if (ret)
>>>> -		goto err_regulator;
>>>> +		goto err_async;
>>>>
>>>>  	priv->sd.entity.function = MEDIA_ENT_F_VID_IF_BRIDGE;
>>>>
>>>> @@ -927,6 +977,8 @@ static int max9286_init(struct device *dev)
>>>>  	max9286_i2c_mux_close(priv);
>>>>  err_put_node:
>>>>  	fwnode_handle_put(ep);
>>>> +err_async:
>>>> +	max9286_v4l2_async_unregister(priv);
>>>>  err_regulator:
>>>>  	regulator_disable(priv->regulator);
>>>>  	priv->poc_enabled = false;
>>>> @@ -938,14 +990,6 @@ static void max9286_cleanup_dt(struct max9286_priv *priv)
>>>>  {
>>>>  	struct max9286_source *source;
>>>>
>>>> -	/*
>>>> -	 * Not strictly part of the DT, but the notifier is registered during
>>>> -	 * max9286_parse_dt(), and the notifier holds references to the fwnodes
>>>> -	 * thus the cleanup is here to mirror the registration.
>>>> -	 */
>>>> -	v4l2_async_notifier_unregister(&priv->notifier);
>>>> -	v4l2_async_notifier_cleanup(&priv->notifier);
>>>> -
>>>>  	for_each_source(priv, source) {
>>>>  		fwnode_handle_put(source->fwnode);
>>>
>>> Wasn't this a double fwnode_handle_put() ? We called
>>> notifier_cleanup() and then put all sources again manually.
>>>
>>> I don't see one more get() when the asd gets registered to the
>>> notifier with v4l2_async_notifier_add_subdev() so I'm afraid this
>>> would result in a duplicated put(). Am I wrong ?
>>
>> Agh, all the implicit transfers of ownerships for refcnting with fwnodes
>> is horrible.
>>
> 
> Yup :(
> 
>> I hadn't actually noticed that _notifier_cleanup() was doing
>> fwnode_handle_puts() ...
>>
>> But indeed, all of that was pre-existing before this patch series. Not
>> something I was looking to modify as part of this patch.
>>
>> Lets fix that issue on top.
>>
> 
> Fine with me
> 
>> (which is going to get squashed in all the same, but I'm not going to
>> change /this/ patch for it)
>>
>>
>>> It was there already, but I think it happens in this patch as
>>> well: if max9286_init() fails calls max9286_v4l2_unregister() which
>>> then calls max9286_v4l2_async_unregister() which put all the
>>> not-yet-completed subdevs by calling v4l2_async_notifier_cleanup().
>>> Then in the probe() function error path we then call
>>> max9286_cleanup_dt() which puts again all the registered sources
>>> regardless of their completed status.
>>
>>> I would call max9286_cleanup_dt() only if max9286_init() has not been
>>> called yet. If we get to register subdevs to the notifier, I would
>>> then provide a function that calls v4l2_async_notifier_cleanup() and
>>> then manually puts all sources not yet registered. I'm afraid this
>>> would need to keep a status flag in the source, unless you have a more
>>> elegant solution.
>>
>> It seems like we also have the issue where we need to cleanup partially
>> registered sources, (i.e. if some have registered, and some haven't) ...
>> so how about a per-source flag to note that the device /got/ registered,
>> and thus 'ownership' moved to V4L2 v4l2_async_notifier_cleanup() to _put().
>>
>> Then we can maintain a single cleanup function still, and it will be
>> handled on a per-node basis.
> 
> That was my suggestion but indeed requires some state keeping in
> place, so I hoped we could come up with something more elegant :)
> 
> But indeed, keeping a flag in the source to tell ownership has been
> passed to the async framework would probably be enough, and would
> allow us to possibly have a single cleanup function, yes.


Hopefully my proposed patch to simply increase the refcnt on
successfully added subdevs will do the trick.



> 
>>
>> --
>> KB
>>
>>
>>
>>>
>>> Thanks
>>>    j
>>>
>>>>  		source->fwnode = NULL;
>>>> @@ -958,7 +1002,6 @@ static int max9286_parse_dt(struct max9286_priv *priv)
>>>>  	struct device_node *i2c_mux;
>>>>  	struct device_node *node = NULL;
>>>>  	unsigned int i2c_mux_mask = 0;
>>>> -	int ret;
>>>>
>>>>  	of_node_get(dev->of_node);
>>>>  	i2c_mux = of_find_node_by_name(dev->of_node, "i2c-mux");
>>>> @@ -986,8 +1029,6 @@ static int max9286_parse_dt(struct max9286_priv *priv)
>>>>  	of_node_put(node);
>>>>  	of_node_put(i2c_mux);
>>>>
>>>> -	v4l2_async_notifier_init(&priv->notifier);
>>>> -
>>>>  	/* Parse the endpoints */
>>>>  	for_each_endpoint_of_node(dev->of_node, node) {
>>>>  		struct max9286_source *source;
>>>> @@ -1056,34 +1097,14 @@ static int max9286_parse_dt(struct max9286_priv *priv)
>>>>  			continue;
>>>>  		}
>>>>
>>>> -		source->asd.match_type = V4L2_ASYNC_MATCH_FWNODE;
>>>> -		source->asd.match.fwnode = source->fwnode;
>>>> -
>>>> -		ret = v4l2_async_notifier_add_subdev(&priv->notifier,
>>>> -						     &source->asd);
>>>> -		if (ret) {
>>>> -			v4l2_async_notifier_cleanup(&priv->notifier);
>>>> -			of_node_put(node);
>>>> -			return ret;
>>>> -		}
>>>> -
>>>>  		priv->source_mask |= BIT(ep.port);
>>>>  		priv->nsources++;
>>>>  	}
>>>>  	of_node_put(node);
>>>>
>>>> -	/* Do not register the subdev notifier if there are no devices. */
>>>> -	if (!priv->nsources)
>>>> -		return 0;
>>>> -
>>>>  	priv->route_mask = priv->source_mask;
>>>> -	priv->notifier.ops = &max9286_notify_ops;
>>>> -
>>>> -	ret = v4l2_async_subdev_notifier_register(&priv->sd, &priv->notifier);
>>>> -	if (ret)
>>>> -		v4l2_async_notifier_cleanup(&priv->notifier);
>>>>
>>>> -	return ret;
>>>> +	return 0;
>>>>  }
>>>>
>>>>  static int max9286_probe(struct i2c_client *client)
>>>> @@ -1182,6 +1203,7 @@ static int max9286_remove(struct i2c_client *client)
>>>>
>>>>  	fwnode_handle_put(priv->sd.fwnode);
>>>>  	v4l2_async_unregister_subdev(&priv->sd);
>>>> +	max9286_v4l2_async_unregister(priv);
>>>>
>>>>  	if (priv->poc_enabled)
>>>>  		regulator_disable(priv->regulator);
>>>> --
>>>> 2.20.1
>>>>
>>

Re: [PATCH 1/2] max9286: Split out async registration

[Jacopo Mondi]

[-- Attachment #1: Type: text/plain, Size: 2266 bytes --]

Hi Kieran,

On Thu, Feb 13, 2020 at 10:27:11AM +0000, Kieran Bingham wrote:
> On 13/02/2020 10:20, Jacopo Mondi wrote:
> > Hi Kieran,
> >
> > On Thu, Feb 13, 2020 at 10:07:18AM +0000, Kieran Bingham wrote:
> >> Hi Jacopo,
> >>
> >> On 13/02/2020 09:46, Jacopo Mondi wrote:
> >>> Hi Kieran,
> >>>   very nice thanks for handling this
> >>
> >> :-)
> >>
> >>> Just a few minors
> >>
> >> :-s hehe
> >>
> >
> > Turned out to be lengthier than expected :)
> >
> >>>
> >>> On Wed, Feb 12, 2020 at 05:37:26PM +0000, Kieran Bingham wrote:
> >>>> Move all the V4L2 Subdev Async registration so that it can only happen once
> >>>> we know we will not need to -EPROBE_DEFER...
> >>>>
> >>>> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> >>>> ---
> >>>>  drivers/media/i2c/max9286.c | 88 +++++++++++++++++++++++--------------
> >>>>  1 file changed, 55 insertions(+), 33 deletions(-)
> >>>>
> >>>> diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> >>>> index 1b4ff3533795..03c5fa232b6d 100644
> >>>> --- a/drivers/media/i2c/max9286.c
> >>>> +++ b/drivers/media/i2c/max9286.c
> >>>> @@ -503,6 +503,49 @@ static const struct v4l2_async_notifier_operations max9286_notify_ops = {
> >>>>  	.unbind = max9286_notify_unbind,
> >>>>  };
> >>>>
> >>>> +static int max9286_v4l2_async_register(struct max9286_priv *priv)
> >>>
> >>> Could you capture in the function name this actually deals with
> >>> notifiers ? Like max9286_notifier_register() or similar...
> >>
> >> I'd like to keep the 'v4l2' in there somewhere...
> >>
> >> 	max9286_v4l2_notifier_register() ?
> >>
> >> But then maybe even that could be confused with the notifiers/async
> >> handling for the max9286 itself.
> >>
> >> My aim was that max9286_v4l2_async_{un,}register() dealt with subdevices
> >> connected to the max9286 only ...
> >
> > To me async_register() calls for dealing with registering our own
> > subdev to the async framework, not collecting remote asds and adding
> > it our subnotifier. As you wish, it's really just a suggestion.
>
>
> So would you like to see the async registration between the max9286 and
> the connected CSI2 receiver handled in this pair of functions too?
>

No no, I was just bikeshedding on function names, no worries :)


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] max9286: balance v4l2_async refcnting

[Kieran Bingham]

On 13/02/2020 10:21, Kieran Bingham wrote:
> When we add fwnodes to V4L2 notifiers through
> v4l2_async_notifier_add_subdev they are stored internally in V4L2 core,
> and have a reference count released upon any call to
> v4l2_async_notifier_cleanup().
> 
> Ensure that any source successfully added to a notifier gets its fwnode
> reference count increased accordingly.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> ---
>  drivers/media/i2c/max9286.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> index f3311210a666..62615e6ab710 100644
> --- a/drivers/media/i2c/max9286.c
> +++ b/drivers/media/i2c/max9286.c
> @@ -665,6 +665,11 @@ static int max9286_v4l2_async_register(struct max9286_priv *priv)
>  			v4l2_async_notifier_cleanup(&priv->notifier);
>  			return ret;
>  		}
> +
> +		/* Balance the refernce counting handled through

I've correctly moved this to a new line and fixed up the reference
spelling :)

> +		 * v4l2_async_notifier_cleanup()
> +		 */
> +		fwnode_handle_get(source->fwnode);
>  	}
>  
>  	priv->notifier.ops = &max9286_notify_ops;
> 

Re: [PATCH] max9286: balance v4l2_async refcnting

[Jacopo Mondi]

[-- Attachment #1: Type: text/plain, Size: 1453 bytes --]

Hi Kieran!

On Thu, Feb 13, 2020 at 01:06:31PM +0000, Kieran Bingham wrote:
> On 13/02/2020 10:21, Kieran Bingham wrote:
> > When we add fwnodes to V4L2 notifiers through
> > v4l2_async_notifier_add_subdev they are stored internally in V4L2 core,
> > and have a reference count released upon any call to
> > v4l2_async_notifier_cleanup().
> >
> > Ensure that any source successfully added to a notifier gets its fwnode
> > reference count increased accordingly.
> >
> > Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> > ---
> >  drivers/media/i2c/max9286.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> >
> > diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> > index f3311210a666..62615e6ab710 100644
> > --- a/drivers/media/i2c/max9286.c
> > +++ b/drivers/media/i2c/max9286.c
> > @@ -665,6 +665,11 @@ static int max9286_v4l2_async_register(struct max9286_priv *priv)
> >  			v4l2_async_notifier_cleanup(&priv->notifier);
> >  			return ret;
> >  		}
> > +
> > +		/* Balance the refernce counting handled through
>
> I've correctly moved this to a new line and fixed up the reference
> spelling :)

Good, thanks!

I think you could squash all of these in the next max9286 iteration!

Acked-by: Jacopo Mondi <jacopo@jmondi.org>

Thanks
   j
>
> > +		 * v4l2_async_notifier_cleanup()
> > +		 */
> > +		fwnode_handle_get(source->fwnode);
> >  	}
> >
> >  	priv->notifier.ops = &max9286_notify_ops;
> >
>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] max9286: balance v4l2_async refcnting

[Kieran Bingham]

On 13/02/2020 13:11, Jacopo Mondi wrote:
> Hi Kieran!
> 
> On Thu, Feb 13, 2020 at 01:06:31PM +0000, Kieran Bingham wrote:
>> On 13/02/2020 10:21, Kieran Bingham wrote:
>>> When we add fwnodes to V4L2 notifiers through
>>> v4l2_async_notifier_add_subdev they are stored internally in V4L2 core,
>>> and have a reference count released upon any call to
>>> v4l2_async_notifier_cleanup().
>>>
>>> Ensure that any source successfully added to a notifier gets its fwnode
>>> reference count increased accordingly.
>>>
>>> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
>>> ---
>>>  drivers/media/i2c/max9286.c | 5 +++++
>>>  1 file changed, 5 insertions(+)
>>>
>>> diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
>>> index f3311210a666..62615e6ab710 100644
>>> --- a/drivers/media/i2c/max9286.c
>>> +++ b/drivers/media/i2c/max9286.c
>>> @@ -665,6 +665,11 @@ static int max9286_v4l2_async_register(struct max9286_priv *priv)
>>>  			v4l2_async_notifier_cleanup(&priv->notifier);
>>>  			return ret;
>>>  		}
>>> +
>>> +		/* Balance the refernce counting handled through
>>
>> I've correctly moved this to a new line and fixed up the reference
>> spelling :)
> 
> Good, thanks!
> 
> I think you could squash all of these in the next max9286 iteration!
> 
> Acked-by: Jacopo Mondi <jacopo@jmondi.org>

Thankyou, I'll squash and rebase to linux-media/master branch!


 \o/

--
Kieran


> 
> Thanks
>    j
>>
>>> +		 * v4l2_async_notifier_cleanup()
>>> +		 */
>>> +		fwnode_handle_get(source->fwnode);
>>>  	}
>>>
>>>  	priv->notifier.ops = &max9286_notify_ops;
>>>
>>