[PATCH 0/5] Add debug output to v4l2-async

[PATCH 0/5] Add debug output to v4l2-async

[Jacopo Mondi]

Hello Sakari,
   while testing rcar-vin setup on top of your RFC (included in the series)
that moves the framework to perform endpoint matching, I realized how hard is
to follow what happens with asynchronous notifiers, sub-notifiers and
sub-devices.

In order to better understand what happens and ease debug of v4l2-async
operations I have introduced some dev_dbg() output, protected by a Kconfig
option.

Before being able to properly identify (sub-)notifiers and subdevices I had to
extend fwnode_* framework to support a new .get_name() operation, and modify
v4l2_async to make sure notifiers always have a valid fwnode_handle field to
be identified with.

I have tested this only with not yet mainlined drivers (rcar-vin, rcar-csi2,
max9286) each of them performing non-trivial endpoint matching. Also I only
tested this on OF based systems (not tested on ACPI).
This considered, I am copying linux-media anyhow for feedbacks.

Thanks
   j

Jacopo Mondi (4):
  device property: Add fwnode_get_name() operation
  include: v4l2_async: Add 'owner' field to notifier
  v4l2: async: Postpone subdev_notifier registration
  v4l2: async: Add debug output to v4l2-async module

Sakari Ailus (1):
  v4l: async: Use endpoint node, not device node, for fwnode match

 drivers/acpi/property.c                        |   6 +
 drivers/base/property.c                        |  12 ++
 drivers/media/platform/am437x/am437x-vpfe.c    |   2 +-
 drivers/media/platform/atmel/atmel-isc.c       |   2 +-
 drivers/media/platform/atmel/atmel-isi.c       |   2 +-
 drivers/media/platform/davinci/vpif_capture.c  |   2 +-
 drivers/media/platform/exynos4-is/media-dev.c  |  14 ++-
 drivers/media/platform/pxa_camera.c            |   2 +-
 drivers/media/platform/qcom/camss-8x16/camss.c |   2 +-
 drivers/media/platform/rcar_drif.c             |   2 +-
 drivers/media/platform/stm32/stm32-dcmi.c      |   2 +-
 drivers/media/platform/ti-vpe/cal.c            |   2 +-
 drivers/media/platform/xilinx/xilinx-vipp.c    |  16 ++-
 drivers/media/v4l2-core/Kconfig                |   8 ++
 drivers/media/v4l2-core/v4l2-async.c           | 152 +++++++++++++++++++++----
 drivers/media/v4l2-core/v4l2-fwnode.c          |   2 +-
 drivers/of/property.c                          |   6 +
 include/linux/fwnode.h                         |   2 +
 include/linux/property.h                       |   1 +
 include/media/v4l2-async.h                     |   4 +
 20 files changed, 200 insertions(+), 41 deletions(-)

--
2.7.4

[PATCH 1/5] v4l: async: Use endpoint node, not device node, for fwnode match

[Jacopo Mondi]

From: Sakari Ailus <sakari.ailus@linux.intel.com>

V4L2 async framework can use both device's fwnode and endpoints's fwnode
for matching the async sub-device with the sub-device. In order to proceed
moving towards endpoint matching assign the endpoint to the async
sub-device.

As most async sub-device drivers (and the related hardware) only supports
a single endpoint, use the first endpoint found. This works for all
current drivers --- we only ever supported a single async sub-device per
device to begin with.

For async devices that have no endpoints, continue to use the fwnode
related to the device. This includes e.g. lens devices.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/platform/am437x/am437x-vpfe.c    |  2 +-
 drivers/media/platform/atmel/atmel-isc.c       |  2 +-
 drivers/media/platform/atmel/atmel-isi.c       |  2 +-
 drivers/media/platform/davinci/vpif_capture.c  |  2 +-
 drivers/media/platform/exynos4-is/media-dev.c  | 14 ++++++++++----
 drivers/media/platform/pxa_camera.c            |  2 +-
 drivers/media/platform/qcom/camss-8x16/camss.c |  2 +-
 drivers/media/platform/rcar_drif.c             |  2 +-
 drivers/media/platform/stm32/stm32-dcmi.c      |  2 +-
 drivers/media/platform/ti-vpe/cal.c            |  2 +-
 drivers/media/platform/xilinx/xilinx-vipp.c    | 16 +++++++++++++---
 drivers/media/v4l2-core/v4l2-async.c           |  8 ++++++--
 drivers/media/v4l2-core/v4l2-fwnode.c          |  2 +-
 13 files changed, 39 insertions(+), 19 deletions(-)

diff --git a/drivers/media/platform/am437x/am437x-vpfe.c b/drivers/media/platform/am437x/am437x-vpfe.c
index 0997c64..892d9e9 100644
--- a/drivers/media/platform/am437x/am437x-vpfe.c
+++ b/drivers/media/platform/am437x/am437x-vpfe.c
@@ -2493,7 +2493,7 @@ vpfe_get_pdata(struct platform_device *pdev)
 		if (flags & V4L2_MBUS_VSYNC_ACTIVE_HIGH)
 			sdinfo->vpfe_param.vdpol = 1;
 
-		rem = of_graph_get_remote_port_parent(endpoint);
+		rem = of_graph_get_remote_endpoint(endpoint);
 		if (!rem) {
 			dev_err(&pdev->dev, "Remote device at %pOF not found\n",
 				endpoint);
diff --git a/drivers/media/platform/atmel/atmel-isc.c b/drivers/media/platform/atmel/atmel-isc.c
index 13f1c1c..c8bb60e 100644
--- a/drivers/media/platform/atmel/atmel-isc.c
+++ b/drivers/media/platform/atmel/atmel-isc.c
@@ -2044,7 +2044,7 @@ static int isc_parse_dt(struct device *dev, struct isc_device *isc)
 		if (!epn)
 			break;
 
-		rem = of_graph_get_remote_port_parent(epn);
+		rem = of_graph_get_remote_endpoint(epn);
 		if (!rem) {
 			dev_notice(dev, "Remote device at %pOF not found\n",
 				   epn);
diff --git a/drivers/media/platform/atmel/atmel-isi.c b/drivers/media/platform/atmel/atmel-isi.c
index e900995..eafdf91 100644
--- a/drivers/media/platform/atmel/atmel-isi.c
+++ b/drivers/media/platform/atmel/atmel-isi.c
@@ -1119,7 +1119,7 @@ static int isi_graph_parse(struct atmel_isi *isi, struct device_node *node)
 		if (!ep)
 			return -EINVAL;
 
-		remote = of_graph_get_remote_port_parent(ep);
+		remote = of_graph_get_remote_endpoint(ep);
 		if (!remote) {
 			of_node_put(ep);
 			return -EINVAL;
diff --git a/drivers/media/platform/davinci/vpif_capture.c b/drivers/media/platform/davinci/vpif_capture.c
index a89367a..e150d75 100644
--- a/drivers/media/platform/davinci/vpif_capture.c
+++ b/drivers/media/platform/davinci/vpif_capture.c
@@ -1572,7 +1572,7 @@ vpif_capture_get_pdata(struct platform_device *pdev)
 		if (flags & V4L2_MBUS_VSYNC_ACTIVE_HIGH)
 			chan->vpif_if.vd_pol = 1;
 
-		rem = of_graph_get_remote_port_parent(endpoint);
+		rem = of_graph_get_remote_endpoint(endpoint);
 		if (!rem) {
 			dev_dbg(&pdev->dev, "Remote device at %pOF not found\n",
 				endpoint);
diff --git a/drivers/media/platform/exynos4-is/media-dev.c b/drivers/media/platform/exynos4-is/media-dev.c
index c15596b..c6b0220 100644
--- a/drivers/media/platform/exynos4-is/media-dev.c
+++ b/drivers/media/platform/exynos4-is/media-dev.c
@@ -409,7 +409,7 @@ static int fimc_md_parse_port_node(struct fimc_md *fmd,
 
 	pd->mux_id = (endpoint.base.port - 1) & 0x1;
 
-	rem = of_graph_get_remote_port_parent(ep);
+	rem = of_graph_get_remote_endpoint(ep);
 	of_node_put(ep);
 	if (rem == NULL) {
 		v4l2_info(&fmd->v4l2_dev, "Remote device at %pOF not found\n",
@@ -1360,11 +1360,17 @@ static int subdev_notifier_bound(struct v4l2_async_notifier *notifier,
 	int i;
 
 	/* Find platform data for this sensor subdev */
-	for (i = 0; i < ARRAY_SIZE(fmd->sensor); i++)
-		if (fmd->sensor[i].asd.match.fwnode.fwnode ==
-		    of_fwnode_handle(subdev->dev->of_node))
+	for (i = 0; i < ARRAY_SIZE(fmd->sensor); i++) {
+		struct fwnode_handle *fwnode =
+			fwnode_graph_get_port_parent(
+				of_fwnode_handle(subdev->dev->of_node));
+
+		if (fmd->sensor[i].asd.match.fwnode.fwnode == fwnode)
 			si = &fmd->sensor[i];
 
+		fwnode_handle_put(fwnode);
+	}
+
 	if (si == NULL)
 		return -EINVAL;
 
diff --git a/drivers/media/platform/pxa_camera.c b/drivers/media/platform/pxa_camera.c
index 9d3f0cb..33d7ef8 100644
--- a/drivers/media/platform/pxa_camera.c
+++ b/drivers/media/platform/pxa_camera.c
@@ -2331,7 +2331,7 @@ static int pxa_camera_pdata_from_dt(struct device *dev,
 		pcdev->platform_flags |= PXA_CAMERA_PCLK_EN;
 
 	asd->match_type = V4L2_ASYNC_MATCH_FWNODE;
-	remote = of_graph_get_remote_port(np);
+	remote = of_graph_get_remote_endpoint(np);
 	if (remote) {
 		asd->match.fwnode.fwnode = of_fwnode_handle(remote);
 		of_node_put(remote);
diff --git a/drivers/media/platform/qcom/camss-8x16/camss.c b/drivers/media/platform/qcom/camss-8x16/camss.c
index 390a42c..73cac63 100644
--- a/drivers/media/platform/qcom/camss-8x16/camss.c
+++ b/drivers/media/platform/qcom/camss-8x16/camss.c
@@ -332,7 +332,7 @@ static int camss_of_parse_ports(struct device *dev,
 			return ret;
 		}
 
-		remote = of_graph_get_remote_port_parent(node);
+		remote = of_graph_get_remote_endpoint(node);
 		of_node_put(node);
 
 		if (!remote) {
diff --git a/drivers/media/platform/rcar_drif.c b/drivers/media/platform/rcar_drif.c
index 63c94f4..f6e0a08 100644
--- a/drivers/media/platform/rcar_drif.c
+++ b/drivers/media/platform/rcar_drif.c
@@ -1228,7 +1228,7 @@ static int rcar_drif_parse_subdevs(struct rcar_drif_sdr *sdr)
 		return 0;
 
 	notifier->subdevs[notifier->num_subdevs] = &sdr->ep.asd;
-	fwnode = fwnode_graph_get_remote_port_parent(ep);
+	fwnode = fwnode_graph_get_remote_endpoint(ep);
 	if (!fwnode) {
 		dev_warn(sdr->dev, "bad remote port parent\n");
 		fwnode_handle_put(ep);
diff --git a/drivers/media/platform/stm32/stm32-dcmi.c b/drivers/media/platform/stm32/stm32-dcmi.c
index ac4c450..18e0aa8 100644
--- a/drivers/media/platform/stm32/stm32-dcmi.c
+++ b/drivers/media/platform/stm32/stm32-dcmi.c
@@ -1511,7 +1511,7 @@ static int dcmi_graph_parse(struct stm32_dcmi *dcmi, struct device_node *node)
 		if (!ep)
 			return -EINVAL;
 
-		remote = of_graph_get_remote_port_parent(ep);
+		remote = of_graph_get_remote_endpoint(ep);
 		if (!remote) {
 			of_node_put(ep);
 			return -EINVAL;
diff --git a/drivers/media/platform/ti-vpe/cal.c b/drivers/media/platform/ti-vpe/cal.c
index 8b586c8..9b29706 100644
--- a/drivers/media/platform/ti-vpe/cal.c
+++ b/drivers/media/platform/ti-vpe/cal.c
@@ -1699,7 +1699,7 @@ static int of_cal_create_instance(struct cal_ctx *ctx, int inst)
 		goto cleanup_exit;
 	}
 
-	sensor_node = of_graph_get_remote_port_parent(ep_node);
+	sensor_node = of_graph_get_remote_endpoint(ep_node);
 	if (!sensor_node) {
 		ctx_dbg(3, ctx, "can't get remote parent\n");
 		goto cleanup_exit;
diff --git a/drivers/media/platform/xilinx/xilinx-vipp.c b/drivers/media/platform/xilinx/xilinx-vipp.c
index d881cf0..17d4ac0 100644
--- a/drivers/media/platform/xilinx/xilinx-vipp.c
+++ b/drivers/media/platform/xilinx/xilinx-vipp.c
@@ -82,6 +82,8 @@ static int xvip_graph_build_one(struct xvip_composite_device *xdev,
 	dev_dbg(xdev->dev, "creating links for entity %s\n", local->name);
 
 	while (1) {
+		struct fwnode_handle *fwnode;
+
 		/* Get the next endpoint and parse its link. */
 		next = of_graph_get_next_endpoint(entity->node, ep);
 		if (next == NULL)
@@ -121,8 +123,11 @@ static int xvip_graph_build_one(struct xvip_composite_device *xdev,
 			continue;
 		}
 
+		fwnode = fwnode_graph_get_port_parent(link.remote_node);
+		fwnode_handle_put(fwnode);
+
 		/* Skip DMA engines, they will be processed separately. */
-		if (link.remote_node == of_fwnode_handle(xdev->dev->of_node)) {
+		if (fwnode == of_fwnode_handle(xdev->dev->of_node)) {
 			dev_dbg(xdev->dev, "skipping DMA port %pOF:%u\n",
 				to_of_node(link.local_node),
 				link.local_port);
@@ -367,20 +372,25 @@ static int xvip_graph_parse_one(struct xvip_composite_device *xdev,
 	dev_dbg(xdev->dev, "parsing node %pOF\n", node);
 
 	while (1) {
+		struct fwnode_handle *fwnode;
+
 		ep = of_graph_get_next_endpoint(node, ep);
 		if (ep == NULL)
 			break;
 
 		dev_dbg(xdev->dev, "handling endpoint %pOF\n", ep);
 
-		remote = of_graph_get_remote_port_parent(ep);
+		remote = of_graph_get_remote_endpoint(ep);
 		if (remote == NULL) {
 			ret = -EINVAL;
 			break;
 		}
 
+		fwnode = fwnode_graph_get_port_parent(of_fwnode_handle(remote));
+		fwnode_handle_put(fwnode);
+
 		/* Skip entities that we have already processed. */
-		if (remote == xdev->dev->of_node ||
+		if (fwnode == xdev->dev->of_node ||
 		    xvip_graph_find_entity(xdev, remote)) {
 			of_node_put(remote);
 			continue;
diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
index a7c3464..a6bddff 100644
--- a/drivers/media/v4l2-core/v4l2-async.c
+++ b/drivers/media/v4l2-core/v4l2-async.c
@@ -539,8 +539,12 @@ int v4l2_async_register_subdev(struct v4l2_subdev *sd)
 	 * (struct v4l2_subdev.dev), and async sub-device does not
 	 * exist independently of the device at any point of time.
 	 */
-	if (!sd->fwnode && sd->dev)
-		sd->fwnode = dev_fwnode(sd->dev);
+	if (!sd->fwnode && sd->dev) {
+		sd->fwnode = fwnode_graph_get_next_endpoint(
+			dev_fwnode(sd->dev), NULL);
+		if (!sd->fwnode)
+			sd->fwnode = dev_fwnode(sd->dev);
+	}
 
 	mutex_lock(&list_lock);
 
diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c
index 681b192..a5a47e1 100644
--- a/drivers/media/v4l2-core/v4l2-fwnode.c
+++ b/drivers/media/v4l2-core/v4l2-fwnode.c
@@ -360,7 +360,7 @@ static int v4l2_async_notifier_fwnode_parse_endpoint(
 
 	asd->match_type = V4L2_ASYNC_MATCH_FWNODE;
 	asd->match.fwnode.fwnode =
-		fwnode_graph_get_remote_port_parent(endpoint);
+		fwnode_graph_get_remote_endpoint(endpoint);
 	if (!asd->match.fwnode.fwnode) {
 		dev_warn(dev, "bad remote port parent\n");
 		ret = -EINVAL;
-- 
2.7.4

[PATCH 2/5] device property: Add fwnode_get_name() operation

[Jacopo Mondi]

Add operation to retrieve the device name from a fwnode handle.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/acpi/property.c  |  6 ++++++
 drivers/base/property.c  | 12 ++++++++++++
 drivers/of/property.c    |  6 ++++++
 include/linux/fwnode.h   |  2 ++
 include/linux/property.h |  1 +
 5 files changed, 27 insertions(+)

diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
index e26ea20..1e3971c 100644
--- a/drivers/acpi/property.c
+++ b/drivers/acpi/property.c
@@ -1186,6 +1186,11 @@ acpi_fwnode_property_read_string_array(const struct fwnode_handle *fwnode,
 				   val, nval);
 }
 
+static const char *acpi_fwnode_get_name(const struct fwnode_handle *fwnode)
+{
+	return acpi_dev_name(to_acpi_device_node(fwnode));
+}
+
 static struct fwnode_handle *
 acpi_fwnode_get_named_child_node(const struct fwnode_handle *fwnode,
 				 const char *childname)
@@ -1281,6 +1286,7 @@ static int acpi_fwnode_graph_parse_endpoint(const struct fwnode_handle *fwnode,
 			acpi_fwnode_property_read_string_array,		\
 		.get_parent = acpi_node_get_parent,			\
 		.get_next_child_node = acpi_get_next_subnode,		\
+		.get_name = acpi_fwnode_get_name,			\
 		.get_named_child_node = acpi_fwnode_get_named_child_node, \
 		.get_reference_args = acpi_fwnode_get_reference_args,	\
 		.graph_get_next_endpoint =				\
diff --git a/drivers/base/property.c b/drivers/base/property.c
index 851b1b6..a87b4a9 100644
--- a/drivers/base/property.c
+++ b/drivers/base/property.c
@@ -950,6 +950,18 @@ int device_add_properties(struct device *dev,
 EXPORT_SYMBOL_GPL(device_add_properties);
 
 /**
+ * fwnode_get_name - Return the fwnode_handle name
+ * @fwnode: Firmware node to get name from
+ *
+ * Returns a pointer to the firmware node name
+ */
+const char *fwnode_get_name(const struct fwnode_handle *fwnode)
+{
+	return fwnode_call_ptr_op(fwnode, get_name);
+}
+EXPORT_SYMBOL(fwnode_get_name);
+
+/**
  * fwnode_get_next_parent - Iterate to the node's parent
  * @fwnode: Firmware whose parent is retrieved
  *
diff --git a/drivers/of/property.c b/drivers/of/property.c
index 8ad33a4..6c195a8 100644
--- a/drivers/of/property.c
+++ b/drivers/of/property.c
@@ -875,6 +875,11 @@ of_fwnode_property_read_string_array(const struct fwnode_handle *fwnode,
 		of_property_count_strings(node, propname);
 }
 
+static const char *of_fwnode_get_name(const struct fwnode_handle *fwnode)
+{
+	return of_node_full_name(to_of_node(fwnode));
+}
+
 static struct fwnode_handle *
 of_fwnode_get_parent(const struct fwnode_handle *fwnode)
 {
@@ -988,6 +993,7 @@ const struct fwnode_operations of_fwnode_ops = {
 	.property_present = of_fwnode_property_present,
 	.property_read_int_array = of_fwnode_property_read_int_array,
 	.property_read_string_array = of_fwnode_property_read_string_array,
+	.get_name = of_fwnode_get_name,
 	.get_parent = of_fwnode_get_parent,
 	.get_next_child_node = of_fwnode_get_next_child_node,
 	.get_named_child_node = of_fwnode_get_named_child_node,
diff --git a/include/linux/fwnode.h b/include/linux/fwnode.h
index 411a84c..5d3a8c6 100644
--- a/include/linux/fwnode.h
+++ b/include/linux/fwnode.h
@@ -57,6 +57,7 @@ struct fwnode_reference_args {
  *				 otherwise.
  * @property_read_string_array: Read an array of string properties. Return zero
  *				on success, a negative error code otherwise.
+ * @get_name: Return the fwnode name.
  * @get_parent: Return the parent of an fwnode.
  * @get_next_child_node: Return the next child node in an iteration.
  * @get_named_child_node: Return a child node with a given name.
@@ -81,6 +82,7 @@ struct fwnode_operations {
 	(*property_read_string_array)(const struct fwnode_handle *fwnode_handle,
 				      const char *propname, const char **val,
 				      size_t nval);
+	const char *(*get_name)(const struct fwnode_handle *fwnode);
 	struct fwnode_handle *(*get_parent)(const struct fwnode_handle *fwnode);
 	struct fwnode_handle *
 	(*get_next_child_node)(const struct fwnode_handle *fwnode,
diff --git a/include/linux/property.h b/include/linux/property.h
index f6189a3..0fc464f 100644
--- a/include/linux/property.h
+++ b/include/linux/property.h
@@ -78,6 +78,7 @@ int fwnode_property_get_reference_args(const struct fwnode_handle *fwnode,
 				       unsigned int nargs, unsigned int index,
 				       struct fwnode_reference_args *args);
 
+const char *fwnode_get_name(const struct fwnode_handle *fwnode);
 struct fwnode_handle *fwnode_get_parent(const struct fwnode_handle *fwnode);
 struct fwnode_handle *fwnode_get_next_parent(
 	struct fwnode_handle *fwnode);
-- 
2.7.4

[PATCH 3/5] include: v4l2_async: Add 'owner' field to notifier

[Jacopo Mondi]

Notifiers can be registered as root notifiers (identified by a 'struct
v4l2_device *') or subdevice notifiers (identified by a 'struct
v4l2_subdev *'). In order to identify a notifier no matter if it is root
or not, add a 'struct fwnode_handle *owner' field, whose name can be
printed out for debug purposes.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/media/v4l2-core/v4l2-async.c | 2 ++
 include/media/v4l2-async.h           | 2 ++
 2 files changed, 4 insertions(+)

diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
index a6bddff..0a1bf1d 100644
--- a/drivers/media/v4l2-core/v4l2-async.c
+++ b/drivers/media/v4l2-core/v4l2-async.c
@@ -447,6 +447,7 @@ int v4l2_async_notifier_register(struct v4l2_device *v4l2_dev,
 		return -EINVAL;
 
 	notifier->v4l2_dev = v4l2_dev;
+	notifier->owner = dev_fwnode(v4l2_dev->dev);
 
 	ret = __v4l2_async_notifier_register(notifier);
 	if (ret)
@@ -465,6 +466,7 @@ int v4l2_async_subdev_notifier_register(struct v4l2_subdev *sd,
 		return -EINVAL;
 
 	notifier->sd = sd;
+	notifier->owner = dev_fwnode(sd->dev);
 
 	ret = __v4l2_async_notifier_register(notifier);
 	if (ret)
diff --git a/include/media/v4l2-async.h b/include/media/v4l2-async.h
index 6152434..a15c01d 100644
--- a/include/media/v4l2-async.h
+++ b/include/media/v4l2-async.h
@@ -106,6 +106,7 @@ struct v4l2_async_notifier_operations {
  * @v4l2_dev:	v4l2_device of the root notifier, NULL otherwise
  * @sd:		sub-device that registered the notifier, NULL otherwise
  * @parent:	parent notifier
+ * @owner:	reference to notifier fwnode_handle, mostly useful for debug
  * @waiting:	list of struct v4l2_async_subdev, waiting for their drivers
  * @done:	list of struct v4l2_subdev, already probed
  * @list:	member in a global list of notifiers
@@ -118,6 +119,7 @@ struct v4l2_async_notifier {
 	struct v4l2_device *v4l2_dev;
 	struct v4l2_subdev *sd;
 	struct v4l2_async_notifier *parent;
+	struct fwnode_handle *owner;
 	struct list_head waiting;
 	struct list_head done;
 	struct list_head list;
-- 
2.7.4

[PATCH 4/5] v4l2: async: Postpone subdev_notifier registration

[Jacopo Mondi]

Currently, subdevice notifiers are tested against all available
subdevices as soon as they get registered. It often happens anyway
that the subdevice they are connected to is not yet initialized, as
it usually gets registered later in drivers' code. This makes debug
of v4l2_async particularly painful, as identifying a notifier with
an unitialized subdevice is tricky as they don't have a valid
'struct device *' or 'struct fwnode_handle *' to be identified with.

In order to make sure that the notifier's subdevices is initialized
when the notifier is tesed against available subdevices post-pone the
actual notifier registration at subdevice registration time.

It is worth noting that post-poning registration of a subdevice notifier
does not impact on the completion of the notifiers chain, as even if a
subdev notifier completes as soon as it gets registered, the complete()
call chain cannot be upscaled as long as the subdevice the notifiers
belongs to is not registered.

Also, it is now safe to access a notifier 'struct device *' as we're now
sure it is properly initialized when the notifier is actually
registered.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/media/v4l2-core/v4l2-async.c | 65 +++++++++++++++++++++++-------------
 include/media/v4l2-async.h           |  2 ++
 2 files changed, 43 insertions(+), 24 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
index 0a1bf1d..c13a781 100644
--- a/drivers/media/v4l2-core/v4l2-async.c
+++ b/drivers/media/v4l2-core/v4l2-async.c
@@ -25,6 +25,13 @@
 #include <media/v4l2-fwnode.h>
 #include <media/v4l2-subdev.h>

+static struct device *v4l2_async_notifier_dev(
+					struct v4l2_async_notifier *notifier)
+{
+	return notifier->v4l2_dev ? notifier->v4l2_dev->dev :
+				    notifier->sd->dev;
+}
+
 static int v4l2_async_notifier_call_bound(struct v4l2_async_notifier *n,
 					  struct v4l2_subdev *subdev,
 					  struct v4l2_async_subdev *asd)
@@ -124,19 +131,6 @@ static struct v4l2_async_subdev *v4l2_async_find_match(
 	return NULL;
 }

-/* Find the sub-device notifier registered by a sub-device driver. */
-static struct v4l2_async_notifier *v4l2_async_find_subdev_notifier(
-	struct v4l2_subdev *sd)
-{
-	struct v4l2_async_notifier *n;
-
-	list_for_each_entry(n, &notifier_list, list)
-		if (n->sd == sd)
-			return n;
-
-	return NULL;
-}
-
 /* Get v4l2_device related to the notifier if one can be found. */
 static struct v4l2_device *v4l2_async_notifier_find_v4l2_dev(
 	struct v4l2_async_notifier *notifier)
@@ -160,7 +154,7 @@ static bool v4l2_async_notifier_can_complete(

 	list_for_each_entry(sd, &notifier->done, async_list) {
 		struct v4l2_async_notifier *subdev_notifier =
-			v4l2_async_find_subdev_notifier(sd);
+							sd->subdev_notifier;

 		if (subdev_notifier &&
 		    !v4l2_async_notifier_can_complete(subdev_notifier))
@@ -228,7 +222,7 @@ static int v4l2_async_match_notify(struct v4l2_async_notifier *notifier,
 	/*
 	 * See if the sub-device has a notifier. If not, return here.
 	 */
-	subdev_notifier = v4l2_async_find_subdev_notifier(sd);
+	subdev_notifier = sd->subdev_notifier;
 	if (!subdev_notifier || subdev_notifier->parent)
 		return 0;

@@ -294,7 +288,7 @@ static void v4l2_async_notifier_unbind_all_subdevs(

 	list_for_each_entry_safe(sd, tmp, &notifier->done, async_list) {
 		struct v4l2_async_notifier *subdev_notifier =
-			v4l2_async_find_subdev_notifier(sd);
+							sd->subdev_notifier;

 		if (subdev_notifier)
 			v4l2_async_notifier_unbind_all_subdevs(subdev_notifier);
@@ -371,8 +365,7 @@ static bool v4l2_async_notifier_fwnode_has_async_subdev(

 static int __v4l2_async_notifier_register(struct v4l2_async_notifier *notifier)
 {
-	struct device *dev =
-		notifier->v4l2_dev ? notifier->v4l2_dev->dev : NULL;
+	struct device *dev = v4l2_async_notifier_dev(notifier);
 	struct v4l2_async_subdev *asd;
 	int ret;
 	int i;
@@ -383,6 +376,8 @@ static int __v4l2_async_notifier_register(struct v4l2_async_notifier *notifier)
 	INIT_LIST_HEAD(&notifier->waiting);
 	INIT_LIST_HEAD(&notifier->done);

+	notifier->owner = dev_fwnode(dev);
+
 	mutex_lock(&list_lock);

 	for (i = 0; i < notifier->num_subdevs; i++) {
@@ -421,6 +416,7 @@ static int __v4l2_async_notifier_register(struct v4l2_async_notifier *notifier)

 	/* Keep also completed notifiers on the list */
 	list_add(&notifier->list, &notifier_list);
+	notifier->registered = true;

 	mutex_unlock(&list_lock);

@@ -447,7 +443,7 @@ int v4l2_async_notifier_register(struct v4l2_device *v4l2_dev,
 		return -EINVAL;

 	notifier->v4l2_dev = v4l2_dev;
-	notifier->owner = dev_fwnode(v4l2_dev->dev);
+	notifier->registered = false;

 	ret = __v4l2_async_notifier_register(notifier);
 	if (ret)
@@ -466,7 +462,11 @@ int v4l2_async_subdev_notifier_register(struct v4l2_subdev *sd,
 		return -EINVAL;

 	notifier->sd = sd;
-	notifier->owner = dev_fwnode(sd->dev);
+	sd->subdev_notifier = notifier;
+	notifier->registered = false;
+
+	if (!sd->dev || !sd->fwnode)
+		return 0;

 	ret = __v4l2_async_notifier_register(notifier);
 	if (ret)
@@ -482,12 +482,15 @@ static void __v4l2_async_notifier_unregister(
 	if (!notifier || (!notifier->v4l2_dev && !notifier->sd))
 		return;

-	v4l2_async_notifier_unbind_all_subdevs(notifier);
+	if (notifier->registered) {
+		v4l2_async_notifier_unbind_all_subdevs(notifier);
+		list_del(&notifier->list);
+	}

 	notifier->sd = NULL;
 	notifier->v4l2_dev = NULL;
-
-	list_del(&notifier->list);
+	notifier->owner = NULL;
+	notifier->registered = false;
 }

 void v4l2_async_notifier_unregister(struct v4l2_async_notifier *notifier)
@@ -548,6 +551,20 @@ int v4l2_async_register_subdev(struct v4l2_subdev *sd)
 			sd->fwnode = dev_fwnode(sd->dev);
 	}

+	/*
+	 * If the subdevice has an unregisterd notifier, it's now time
+	 * to register it.
+	 */
+	subdev_notifier = sd->subdev_notifier;
+	if (subdev_notifier && !subdev_notifier->registered) {
+		ret = __v4l2_async_notifier_register(subdev_notifier);
+		if (ret) {
+			sd->fwnode = NULL;
+			subdev_notifier->owner = NULL;
+			return ret;
+		}
+	}
+
 	mutex_lock(&list_lock);

 	INIT_LIST_HEAD(&sd->async_list);
@@ -589,7 +606,7 @@ int v4l2_async_register_subdev(struct v4l2_subdev *sd)
 	 * Complete failed. Unbind the sub-devices bound through registering
 	 * this async sub-device.
 	 */
-	subdev_notifier = v4l2_async_find_subdev_notifier(sd);
+	subdev_notifier = sd->subdev_notifier;
 	if (subdev_notifier)
 		v4l2_async_notifier_unbind_all_subdevs(subdev_notifier);

diff --git a/include/media/v4l2-async.h b/include/media/v4l2-async.h
index a15c01d..6ab04ad 100644
--- a/include/media/v4l2-async.h
+++ b/include/media/v4l2-async.h
@@ -110,6 +110,7 @@ struct v4l2_async_notifier_operations {
  * @waiting:	list of struct v4l2_async_subdev, waiting for their drivers
  * @done:	list of struct v4l2_subdev, already probed
  * @list:	member in a global list of notifiers
+ * @registered: notifier registered complete flag
  */
 struct v4l2_async_notifier {
 	const struct v4l2_async_notifier_operations *ops;
@@ -123,6 +124,7 @@ struct v4l2_async_notifier {
 	struct list_head waiting;
 	struct list_head done;
 	struct list_head list;
+	bool registered;
 };

 /**
--
2.7.4

[PATCH 5/5] v4l2: async: Add debug output to v4l2-async module

[Jacopo Mondi]

The v4l2-async module operations are quite complex to follow, due to the
asynchronous nature of subdevices and notifiers registration and
matching procedures. In order to help with debugging of failed or
erroneous matching between a subdevice and the notifier collected
async_subdevice it gets matched against, introduce a few dev_dbg() calls
in v4l2_async core operations.

Protect the debug operations with a Kconfig defined symbol, to make sure
when debugging is disabled, no additional code or data is added to the
module.

Notifiers are identified by the name of the subdevice or v4l2_dev they are
registered by, while subdevice matching which now happens on endpoints,
need a longer description built walking the fwnode graph backwards
collecting parent nodes names (otherwise we would have had printouts
like: "Matching "endpoint" with "endpoint"" which are not that useful).

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

---
For fwnodes backed by OF, I may have used the "%pOF" format modifier to
get the full node name instead of parsing the fwnode graph by myself with
"v4l2_async_fwnode_full_name()". Unfortunately I'm not aware of anything
like "%pOF" for ACPI backed fwnodes. Also, walking the fwnode graph by
myself allows me to reduce the depth, to reduce the debug messages output
length which is anyway long enough to result disturbing on a 80columns
terminal window.
---

 drivers/media/v4l2-core/Kconfig      |  8 ++++
 drivers/media/v4l2-core/v4l2-async.c | 81 ++++++++++++++++++++++++++++++++++++
 2 files changed, 89 insertions(+)

diff --git a/drivers/media/v4l2-core/Kconfig b/drivers/media/v4l2-core/Kconfig
index a35c336..8331736 100644
--- a/drivers/media/v4l2-core/Kconfig
+++ b/drivers/media/v4l2-core/Kconfig
@@ -17,6 +17,14 @@ config VIDEO_ADV_DEBUG
 	  V4L devices.
 	  In doubt, say N.

+config VIDEO_V4L2_ASYNC_DEBUG
+	bool "Enable debug functionalities for V4L2 async module"
+	depends on VIDEO_V4L2
+	default n
+	---help---
+	  Say Y here to enable debug output in V4L2 async module.
+	  In doubt, say N.
+
 config VIDEO_FIXED_MINOR_RANGES
 	bool "Enable old-style fixed minor ranges on drivers/video devices"
 	default n
diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
index c13a781..307e1a5 100644
--- a/drivers/media/v4l2-core/v4l2-async.c
+++ b/drivers/media/v4l2-core/v4l2-async.c
@@ -8,6 +8,10 @@
  * published by the Free Software Foundation.
  */

+#if defined(CONFIG_VIDEO_V4L2_ASYNC_DEBUG)
+#define DEBUG
+#endif
+
 #include <linux/device.h>
 #include <linux/err.h>
 #include <linux/i2c.h>
@@ -25,6 +29,52 @@
 #include <media/v4l2-fwnode.h>
 #include <media/v4l2-subdev.h>

+#if defined(CONFIG_VIDEO_V4L2_ASYNC_DEBUG)
+#define V4L2_ASYNC_FWNODE_NAME_LEN	512
+
+static void __v4l2_async_fwnode_full_name(char *name,
+					  unsigned int len,
+					  unsigned int max_depth,
+					  struct fwnode_handle *fwnode)
+{
+	unsigned int buf_len = len < V4L2_ASYNC_FWNODE_NAME_LEN ?
+			       len : V4L2_ASYNC_FWNODE_NAME_LEN;
+	char __tmp[V4L2_ASYNC_FWNODE_NAME_LEN];
+	struct fwnode_handle *parent;
+
+	memset(name, 0, buf_len);
+	buf_len -= snprintf(__tmp, buf_len, "%s", fwnode_get_name(fwnode));
+
+	parent = fwnode;
+	while ((parent = fwnode_get_parent(parent)) && buf_len &&
+		--max_depth) {
+		buf_len -= snprintf(name, buf_len, "%s/%s",
+				    fwnode_get_name(parent), __tmp);
+		strcpy(__tmp, name);
+	}
+}
+
+static void v4l2_async_fwnode_full_name(char *name,
+					unsigned int len,
+					struct fwnode_handle *fwnode)
+{
+	/*
+	 * Usually 4 as nesting level is sufficient to identify an
+	 * endpoint firmware node uniquely.
+	 */
+	__v4l2_async_fwnode_full_name(name, len, 4, fwnode);
+}
+
+#else /* CONFIG_VIDEO_V4L2_ASYNC_DEBUG */
+#define V4L2_ASYNC_FWNODE_NAME_LEN	0
+
+static void v4l2_async_fwnode_full_name(char *name,
+					unsigned int len,
+					struct fwnode_handle *fwnode)
+{
+}
+#endif /* CONFIG_VIDEO_V4L2_ASYNC_DEBUG */
+
 static struct device *v4l2_async_notifier_dev(
 					struct v4l2_async_notifier *notifier)
 {
@@ -54,9 +104,12 @@ static void v4l2_async_notifier_call_unbind(struct v4l2_async_notifier *n,

 static int v4l2_async_notifier_call_complete(struct v4l2_async_notifier *n)
 {
+	struct device *dev = v4l2_async_notifier_dev(n);
 	if (!n->ops || !n->ops->complete)
 		return 0;

+	dev_dbg(dev, "Complete notifier \"%s\"\n", fwnode_get_name(n->owner));
+
 	return n->ops->complete(n);
 }

@@ -100,8 +153,17 @@ static struct v4l2_async_subdev *v4l2_async_find_match(
 	struct v4l2_async_notifier *notifier, struct v4l2_subdev *sd)
 {
 	bool (*match)(struct v4l2_subdev *, struct v4l2_async_subdev *);
+	struct device *dev = v4l2_async_notifier_dev(notifier);
+	char asd_full_name[V4L2_ASYNC_FWNODE_NAME_LEN];
+	char sd_full_name[V4L2_ASYNC_FWNODE_NAME_LEN];
 	struct v4l2_async_subdev *asd;

+	v4l2_async_fwnode_full_name(sd_full_name, V4L2_ASYNC_FWNODE_NAME_LEN,
+				    sd->fwnode);
+
+	dev_dbg(dev, "Match notifier \"%s\" with subdevice \"%s\"\n",
+		fwnode_get_name(notifier->owner), sd_full_name);
+
 	list_for_each_entry(asd, &notifier->waiting, list) {
 		/* bus_type has been verified valid before */
 		switch (asd->match_type) {
@@ -115,6 +177,11 @@ static struct v4l2_async_subdev *v4l2_async_find_match(
 			match = match_i2c;
 			break;
 		case V4L2_ASYNC_MATCH_FWNODE:
+			v4l2_async_fwnode_full_name(asd_full_name,
+						    V4L2_ASYNC_FWNODE_NAME_LEN,
+						    asd->match.fwnode.fwnode);
+			dev_dbg(dev, "Test sd \"%s\" with async_sd \"%s\"\n",
+				sd_full_name, asd_full_name);
 			match = match_fwnode;
 			break;
 		default:
@@ -198,9 +265,16 @@ static int v4l2_async_match_notify(struct v4l2_async_notifier *notifier,
 				   struct v4l2_subdev *sd,
 				   struct v4l2_async_subdev *asd)
 {
+	struct device *dev = v4l2_async_notifier_dev(notifier);
 	struct v4l2_async_notifier *subdev_notifier;
+	char sd_full_name[V4L2_ASYNC_FWNODE_NAME_LEN];
 	int ret;

+	v4l2_async_fwnode_full_name(sd_full_name, V4L2_ASYNC_FWNODE_NAME_LEN,
+				    sd->fwnode);
+	dev_dbg(dev, "Matched sd: \"%s\" with notifier \"%s\"\n",
+		sd_full_name, fwnode_get_name(notifier->owner));
+
 	ret = v4l2_device_register_subdev(v4l2_dev, sd);
 	if (ret < 0)
 		return ret;
@@ -240,6 +314,7 @@ static int v4l2_async_match_notify(struct v4l2_async_notifier *notifier,
 static int v4l2_async_notifier_try_all_subdevs(
 	struct v4l2_async_notifier *notifier)
 {
+	struct device *dev = v4l2_async_notifier_dev(notifier);
 	struct v4l2_device *v4l2_dev =
 		v4l2_async_notifier_find_v4l2_dev(notifier);
 	struct v4l2_subdev *sd;
@@ -247,6 +322,9 @@ static int v4l2_async_notifier_try_all_subdevs(
 	if (!v4l2_dev)
 		return 0;

+	dev_dbg(dev, "Testing notifier \"%s\" against all subdevices\n",
+		fwnode_get_name(notifier->owner));
+
 again:
 	list_for_each_entry(sd, &subdev_list, async_list) {
 		struct v4l2_async_subdev *asd;
@@ -378,6 +456,9 @@ static int __v4l2_async_notifier_register(struct v4l2_async_notifier *notifier)

 	notifier->owner = dev_fwnode(dev);

+	dev_dbg(dev, "Registering notifier \"%s\"\n",
+		fwnode_get_name(notifier->owner));
+
 	mutex_lock(&list_lock);

 	for (i = 0; i < notifier->num_subdevs; i++) {
--
2.7.4

Re: [PATCH 2/5] device property: Add fwnode_get_name() operation

[Sakari Ailus]

Hi Jacopo,

Thanks for the patch.

Could you cc the next version to linux-acpi@vger.kernel.org, please? Cc
Mika, too.

On Wed, Dec 13, 2017 at 07:26:17PM +0100, Jacopo Mondi wrote:
> Add operation to retrieve the device name from a fwnode handle.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  drivers/acpi/property.c  |  6 ++++++
>  drivers/base/property.c  | 12 ++++++++++++
>  drivers/of/property.c    |  6 ++++++
>  include/linux/fwnode.h   |  2 ++
>  include/linux/property.h |  1 +
>  5 files changed, 27 insertions(+)
> 
> diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
> index e26ea20..1e3971c 100644
> --- a/drivers/acpi/property.c
> +++ b/drivers/acpi/property.c
> @@ -1186,6 +1186,11 @@ acpi_fwnode_property_read_string_array(const struct fwnode_handle *fwnode,
>  				   val, nval);
>  }
>  
> +static const char *acpi_fwnode_get_name(const struct fwnode_handle *fwnode)
> +{
> +	return acpi_dev_name(to_acpi_device_node(fwnode));

This works for device nodes but will fail miserably for non-device ACPI
nodes.

The ACPI nodes don't currently have a name such as the DT nodes do, it
would certainly help debugging if they did.

What you'll at least need to do is to check to_acpi_device_node() does not
return NULL.

acpi_dev_name() should be made const as well if the function is still used
in v2.

> +}
> +
>  static struct fwnode_handle *
>  acpi_fwnode_get_named_child_node(const struct fwnode_handle *fwnode,
>  				 const char *childname)
> @@ -1281,6 +1286,7 @@ static int acpi_fwnode_graph_parse_endpoint(const struct fwnode_handle *fwnode,
>  			acpi_fwnode_property_read_string_array,		\
>  		.get_parent = acpi_node_get_parent,			\
>  		.get_next_child_node = acpi_get_next_subnode,		\
> +		.get_name = acpi_fwnode_get_name,			\
>  		.get_named_child_node = acpi_fwnode_get_named_child_node, \
>  		.get_reference_args = acpi_fwnode_get_reference_args,	\
>  		.graph_get_next_endpoint =				\
> diff --git a/drivers/base/property.c b/drivers/base/property.c
> index 851b1b6..a87b4a9 100644
> --- a/drivers/base/property.c
> +++ b/drivers/base/property.c
> @@ -950,6 +950,18 @@ int device_add_properties(struct device *dev,
>  EXPORT_SYMBOL_GPL(device_add_properties);
>  
>  /**
> + * fwnode_get_name - Return the fwnode_handle name
> + * @fwnode: Firmware node to get name from
> + *
> + * Returns a pointer to the firmware node name
> + */
> +const char *fwnode_get_name(const struct fwnode_handle *fwnode)
> +{
> +	return fwnode_call_ptr_op(fwnode, get_name);
> +}
> +EXPORT_SYMBOL(fwnode_get_name);
> +
> +/**
>   * fwnode_get_next_parent - Iterate to the node's parent
>   * @fwnode: Firmware whose parent is retrieved
>   *
> diff --git a/drivers/of/property.c b/drivers/of/property.c
> index 8ad33a4..6c195a8 100644
> --- a/drivers/of/property.c
> +++ b/drivers/of/property.c
> @@ -875,6 +875,11 @@ of_fwnode_property_read_string_array(const struct fwnode_handle *fwnode,
>  		of_property_count_strings(node, propname);
>  }
>  
> +static const char *of_fwnode_get_name(const struct fwnode_handle *fwnode)
> +{
> +	return of_node_full_name(to_of_node(fwnode));
> +}
> +
>  static struct fwnode_handle *
>  of_fwnode_get_parent(const struct fwnode_handle *fwnode)
>  {
> @@ -988,6 +993,7 @@ const struct fwnode_operations of_fwnode_ops = {
>  	.property_present = of_fwnode_property_present,
>  	.property_read_int_array = of_fwnode_property_read_int_array,
>  	.property_read_string_array = of_fwnode_property_read_string_array,
> +	.get_name = of_fwnode_get_name,
>  	.get_parent = of_fwnode_get_parent,
>  	.get_next_child_node = of_fwnode_get_next_child_node,
>  	.get_named_child_node = of_fwnode_get_named_child_node,
> diff --git a/include/linux/fwnode.h b/include/linux/fwnode.h
> index 411a84c..5d3a8c6 100644
> --- a/include/linux/fwnode.h
> +++ b/include/linux/fwnode.h
> @@ -57,6 +57,7 @@ struct fwnode_reference_args {
>   *				 otherwise.
>   * @property_read_string_array: Read an array of string properties. Return zero
>   *				on success, a negative error code otherwise.
> + * @get_name: Return the fwnode name.
>   * @get_parent: Return the parent of an fwnode.
>   * @get_next_child_node: Return the next child node in an iteration.
>   * @get_named_child_node: Return a child node with a given name.
> @@ -81,6 +82,7 @@ struct fwnode_operations {
>  	(*property_read_string_array)(const struct fwnode_handle *fwnode_handle,
>  				      const char *propname, const char **val,
>  				      size_t nval);
> +	const char *(*get_name)(const struct fwnode_handle *fwnode);
>  	struct fwnode_handle *(*get_parent)(const struct fwnode_handle *fwnode);
>  	struct fwnode_handle *
>  	(*get_next_child_node)(const struct fwnode_handle *fwnode,
> diff --git a/include/linux/property.h b/include/linux/property.h
> index f6189a3..0fc464f 100644
> --- a/include/linux/property.h
> +++ b/include/linux/property.h
> @@ -78,6 +78,7 @@ int fwnode_property_get_reference_args(const struct fwnode_handle *fwnode,
>  				       unsigned int nargs, unsigned int index,
>  				       struct fwnode_reference_args *args);
>  
> +const char *fwnode_get_name(const struct fwnode_handle *fwnode);
>  struct fwnode_handle *fwnode_get_parent(const struct fwnode_handle *fwnode);
>  struct fwnode_handle *fwnode_get_next_parent(
>  	struct fwnode_handle *fwnode);

-- 
Kind regards,

Sakari Ailus
sakari.ailus@linux.intel.com

Re: [PATCH 3/5] include: v4l2_async: Add 'owner' field to notifier

[Sakari Ailus]

Hi Jacopo,

On Wed, Dec 13, 2017 at 07:26:18PM +0100, Jacopo Mondi wrote:
> Notifiers can be registered as root notifiers (identified by a 'struct
> v4l2_device *') or subdevice notifiers (identified by a 'struct
> v4l2_subdev *'). In order to identify a notifier no matter if it is root
> or not, add a 'struct fwnode_handle *owner' field, whose name can be
> printed out for debug purposes.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

You'll have struct device either through the v4l2_device or v4l2_subdev. Do
you need an additional field for this?

-- 
Sakari Ailus
sakari.ailus@linux.intel.com

Re: [PATCH 4/5] v4l2: async: Postpone subdev_notifier registration

[Sakari Ailus]

Hi Jacopo,

On Wed, Dec 13, 2017 at 07:26:19PM +0100, Jacopo Mondi wrote:
> Currently, subdevice notifiers are tested against all available
> subdevices as soon as they get registered. It often happens anyway
> that the subdevice they are connected to is not yet initialized, as
> it usually gets registered later in drivers' code. This makes debug
> of v4l2_async particularly painful, as identifying a notifier with
> an unitialized subdevice is tricky as they don't have a valid
> 'struct device *' or 'struct fwnode_handle *' to be identified with.
> 
> In order to make sure that the notifier's subdevices is initialized
> when the notifier is tesed against available subdevices post-pone the
> actual notifier registration at subdevice registration time.
> 
> It is worth noting that post-poning registration of a subdevice notifier
> does not impact on the completion of the notifiers chain, as even if a
> subdev notifier completes as soon as it gets registered, the complete()
> call chain cannot be upscaled as long as the subdevice the notifiers
> belongs to is not registered.

Let me rephrase to make sure I understand the problem correctly ---

A sub-device notifier is registered but the notifier's sub-device is not
registered yet, and finding a match for this notifier leads, to, well
problems. Is that the reason for this patch?

I think there could be simpler solutions to address this.

I wonder if we could simply check for sub-device notifier's v4l2_dev field,
and fail in matching if it's not set. v4l2_device_register_subdev() would
still need to proceed with calling v4l2_async_notifier_try_all_subdevs()
and v4l2_async_notifier_try_complete() if that was the case.

What do you think?

> 
> Also, it is now safe to access a notifier 'struct device *' as we're now
> sure it is properly initialized when the notifier is actually
> registered.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  drivers/media/v4l2-core/v4l2-async.c | 65 +++++++++++++++++++++++-------------
>  include/media/v4l2-async.h           |  2 ++
>  2 files changed, 43 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
> index 0a1bf1d..c13a781 100644
> --- a/drivers/media/v4l2-core/v4l2-async.c
> +++ b/drivers/media/v4l2-core/v4l2-async.c
> @@ -25,6 +25,13 @@
>  #include <media/v4l2-fwnode.h>
>  #include <media/v4l2-subdev.h>
> 
> +static struct device *v4l2_async_notifier_dev(
> +					struct v4l2_async_notifier *notifier)
> +{
> +	return notifier->v4l2_dev ? notifier->v4l2_dev->dev :
> +				    notifier->sd->dev;
> +}
> +
>  static int v4l2_async_notifier_call_bound(struct v4l2_async_notifier *n,
>  					  struct v4l2_subdev *subdev,
>  					  struct v4l2_async_subdev *asd)
> @@ -124,19 +131,6 @@ static struct v4l2_async_subdev *v4l2_async_find_match(
>  	return NULL;
>  }
> 
> -/* Find the sub-device notifier registered by a sub-device driver. */
> -static struct v4l2_async_notifier *v4l2_async_find_subdev_notifier(
> -	struct v4l2_subdev *sd)
> -{
> -	struct v4l2_async_notifier *n;
> -
> -	list_for_each_entry(n, &notifier_list, list)
> -		if (n->sd == sd)
> -			return n;
> -
> -	return NULL;
> -}
> -
>  /* Get v4l2_device related to the notifier if one can be found. */
>  static struct v4l2_device *v4l2_async_notifier_find_v4l2_dev(
>  	struct v4l2_async_notifier *notifier)
> @@ -160,7 +154,7 @@ static bool v4l2_async_notifier_can_complete(
> 
>  	list_for_each_entry(sd, &notifier->done, async_list) {
>  		struct v4l2_async_notifier *subdev_notifier =
> -			v4l2_async_find_subdev_notifier(sd);
> +							sd->subdev_notifier;
> 
>  		if (subdev_notifier &&
>  		    !v4l2_async_notifier_can_complete(subdev_notifier))
> @@ -228,7 +222,7 @@ static int v4l2_async_match_notify(struct v4l2_async_notifier *notifier,
>  	/*
>  	 * See if the sub-device has a notifier. If not, return here.
>  	 */
> -	subdev_notifier = v4l2_async_find_subdev_notifier(sd);
> +	subdev_notifier = sd->subdev_notifier;
>  	if (!subdev_notifier || subdev_notifier->parent)
>  		return 0;
> 
> @@ -294,7 +288,7 @@ static void v4l2_async_notifier_unbind_all_subdevs(
> 
>  	list_for_each_entry_safe(sd, tmp, &notifier->done, async_list) {
>  		struct v4l2_async_notifier *subdev_notifier =
> -			v4l2_async_find_subdev_notifier(sd);
> +							sd->subdev_notifier;
> 
>  		if (subdev_notifier)
>  			v4l2_async_notifier_unbind_all_subdevs(subdev_notifier);
> @@ -371,8 +365,7 @@ static bool v4l2_async_notifier_fwnode_has_async_subdev(
> 
>  static int __v4l2_async_notifier_register(struct v4l2_async_notifier *notifier)
>  {
> -	struct device *dev =
> -		notifier->v4l2_dev ? notifier->v4l2_dev->dev : NULL;
> +	struct device *dev = v4l2_async_notifier_dev(notifier);
>  	struct v4l2_async_subdev *asd;
>  	int ret;
>  	int i;
> @@ -383,6 +376,8 @@ static int __v4l2_async_notifier_register(struct v4l2_async_notifier *notifier)
>  	INIT_LIST_HEAD(&notifier->waiting);
>  	INIT_LIST_HEAD(&notifier->done);
> 
> +	notifier->owner = dev_fwnode(dev);
> +
>  	mutex_lock(&list_lock);
> 
>  	for (i = 0; i < notifier->num_subdevs; i++) {
> @@ -421,6 +416,7 @@ static int __v4l2_async_notifier_register(struct v4l2_async_notifier *notifier)
> 
>  	/* Keep also completed notifiers on the list */
>  	list_add(&notifier->list, &notifier_list);
> +	notifier->registered = true;
> 
>  	mutex_unlock(&list_lock);
> 
> @@ -447,7 +443,7 @@ int v4l2_async_notifier_register(struct v4l2_device *v4l2_dev,
>  		return -EINVAL;
> 
>  	notifier->v4l2_dev = v4l2_dev;
> -	notifier->owner = dev_fwnode(v4l2_dev->dev);
> +	notifier->registered = false;
> 
>  	ret = __v4l2_async_notifier_register(notifier);
>  	if (ret)
> @@ -466,7 +462,11 @@ int v4l2_async_subdev_notifier_register(struct v4l2_subdev *sd,
>  		return -EINVAL;
> 
>  	notifier->sd = sd;
> -	notifier->owner = dev_fwnode(sd->dev);
> +	sd->subdev_notifier = notifier;
> +	notifier->registered = false;
> +
> +	if (!sd->dev || !sd->fwnode)
> +		return 0;
> 
>  	ret = __v4l2_async_notifier_register(notifier);
>  	if (ret)
> @@ -482,12 +482,15 @@ static void __v4l2_async_notifier_unregister(
>  	if (!notifier || (!notifier->v4l2_dev && !notifier->sd))
>  		return;
> 
> -	v4l2_async_notifier_unbind_all_subdevs(notifier);
> +	if (notifier->registered) {
> +		v4l2_async_notifier_unbind_all_subdevs(notifier);
> +		list_del(&notifier->list);
> +	}
> 
>  	notifier->sd = NULL;
>  	notifier->v4l2_dev = NULL;
> -
> -	list_del(&notifier->list);
> +	notifier->owner = NULL;
> +	notifier->registered = false;
>  }
> 
>  void v4l2_async_notifier_unregister(struct v4l2_async_notifier *notifier)
> @@ -548,6 +551,20 @@ int v4l2_async_register_subdev(struct v4l2_subdev *sd)
>  			sd->fwnode = dev_fwnode(sd->dev);
>  	}
> 
> +	/*
> +	 * If the subdevice has an unregisterd notifier, it's now time
> +	 * to register it.
> +	 */
> +	subdev_notifier = sd->subdev_notifier;
> +	if (subdev_notifier && !subdev_notifier->registered) {
> +		ret = __v4l2_async_notifier_register(subdev_notifier);
> +		if (ret) {
> +			sd->fwnode = NULL;
> +			subdev_notifier->owner = NULL;
> +			return ret;
> +		}
> +	}
> +
>  	mutex_lock(&list_lock);
> 
>  	INIT_LIST_HEAD(&sd->async_list);
> @@ -589,7 +606,7 @@ int v4l2_async_register_subdev(struct v4l2_subdev *sd)
>  	 * Complete failed. Unbind the sub-devices bound through registering
>  	 * this async sub-device.
>  	 */
> -	subdev_notifier = v4l2_async_find_subdev_notifier(sd);
> +	subdev_notifier = sd->subdev_notifier;
>  	if (subdev_notifier)
>  		v4l2_async_notifier_unbind_all_subdevs(subdev_notifier);
> 
> diff --git a/include/media/v4l2-async.h b/include/media/v4l2-async.h
> index a15c01d..6ab04ad 100644
> --- a/include/media/v4l2-async.h
> +++ b/include/media/v4l2-async.h
> @@ -110,6 +110,7 @@ struct v4l2_async_notifier_operations {
>   * @waiting:	list of struct v4l2_async_subdev, waiting for their drivers
>   * @done:	list of struct v4l2_subdev, already probed
>   * @list:	member in a global list of notifiers
> + * @registered: notifier registered complete flag
>   */
>  struct v4l2_async_notifier {
>  	const struct v4l2_async_notifier_operations *ops;
> @@ -123,6 +124,7 @@ struct v4l2_async_notifier {
>  	struct list_head waiting;
>  	struct list_head done;
>  	struct list_head list;
> +	bool registered;
>  };
> 
>  /**
> --
> 2.7.4
> 

-- 
Sakari Ailus
sakari.ailus@linux.intel.com

Re: [PATCH 5/5] v4l2: async: Add debug output to v4l2-async module

[Sakari Ailus]

Hi Jacopo,

On Wed, Dec 13, 2017 at 07:26:20PM +0100, Jacopo Mondi wrote:
> The v4l2-async module operations are quite complex to follow, due to the
> asynchronous nature of subdevices and notifiers registration and
> matching procedures. In order to help with debugging of failed or
> erroneous matching between a subdevice and the notifier collected
> async_subdevice it gets matched against, introduce a few dev_dbg() calls
> in v4l2_async core operations.
> 
> Protect the debug operations with a Kconfig defined symbol, to make sure
> when debugging is disabled, no additional code or data is added to the
> module.
> 
> Notifiers are identified by the name of the subdevice or v4l2_dev they are
> registered by, while subdevice matching which now happens on endpoints,
> need a longer description built walking the fwnode graph backwards
> collecting parent nodes names (otherwise we would have had printouts
> like: "Matching "endpoint" with "endpoint"" which are not that useful).
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> 
> ---
> For fwnodes backed by OF, I may have used the "%pOF" format modifier to
> get the full node name instead of parsing the fwnode graph by myself with
> "v4l2_async_fwnode_full_name()". Unfortunately I'm not aware of anything
> like "%pOF" for ACPI backed fwnodes. Also, walking the fwnode graph by
> myself allows me to reduce the depth, to reduce the debug messages output
> length which is anyway long enough to result disturbing on a 80columns
> terminal window.

ACPI doesn't have such at the moment. I think printing the full path would
still be better. There isn't that much more to print after all.

> ---
> 
>  drivers/media/v4l2-core/Kconfig      |  8 ++++
>  drivers/media/v4l2-core/v4l2-async.c | 81 ++++++++++++++++++++++++++++++++++++
>  2 files changed, 89 insertions(+)
> 
> diff --git a/drivers/media/v4l2-core/Kconfig b/drivers/media/v4l2-core/Kconfig
> index a35c336..8331736 100644
> --- a/drivers/media/v4l2-core/Kconfig
> +++ b/drivers/media/v4l2-core/Kconfig
> @@ -17,6 +17,14 @@ config VIDEO_ADV_DEBUG
>  	  V4L devices.
>  	  In doubt, say N.
> 
> +config VIDEO_V4L2_ASYNC_DEBUG
> +	bool "Enable debug functionalities for V4L2 async module"
> +	depends on VIDEO_V4L2

I'm not sure I'd add a Kconfig option. This is adding a fairly simple
function only to the kernel.

> +	default n
> +	---help---
> +	  Say Y here to enable debug output in V4L2 async module.
> +	  In doubt, say N.
> +
>  config VIDEO_FIXED_MINOR_RANGES
>  	bool "Enable old-style fixed minor ranges on drivers/video devices"
>  	default n
> diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
> index c13a781..307e1a5 100644
> --- a/drivers/media/v4l2-core/v4l2-async.c
> +++ b/drivers/media/v4l2-core/v4l2-async.c
> @@ -8,6 +8,10 @@
>   * published by the Free Software Foundation.
>   */
> 
> +#if defined(CONFIG_VIDEO_V4L2_ASYNC_DEBUG)
> +#define DEBUG

Do you need this?

> +#endif
> +
>  #include <linux/device.h>
>  #include <linux/err.h>
>  #include <linux/i2c.h>
> @@ -25,6 +29,52 @@
>  #include <media/v4l2-fwnode.h>
>  #include <media/v4l2-subdev.h>
> 
> +#if defined(CONFIG_VIDEO_V4L2_ASYNC_DEBUG)
> +#define V4L2_ASYNC_FWNODE_NAME_LEN	512
> +
> +static void __v4l2_async_fwnode_full_name(char *name,
> +					  unsigned int len,
> +					  unsigned int max_depth,
> +					  struct fwnode_handle *fwnode)
> +{
> +	unsigned int buf_len = len < V4L2_ASYNC_FWNODE_NAME_LEN ?
> +			       len : V4L2_ASYNC_FWNODE_NAME_LEN;
> +	char __tmp[V4L2_ASYNC_FWNODE_NAME_LEN];

That's a bit too much to allocate from the stack I think.

> +	struct fwnode_handle *parent;
> +
> +	memset(name, 0, buf_len);
> +	buf_len -= snprintf(__tmp, buf_len, "%s", fwnode_get_name(fwnode));
> +
> +	parent = fwnode;
> +	while ((parent = fwnode_get_parent(parent)) && buf_len &&
> +		--max_depth) {
> +		buf_len -= snprintf(name, buf_len, "%s/%s",
> +				    fwnode_get_name(parent), __tmp);
> +		strcpy(__tmp, name);
> +	}
> +}
> +
> +static void v4l2_async_fwnode_full_name(char *name,
> +					unsigned int len,
> +					struct fwnode_handle *fwnode)
> +{
> +	/*
> +	 * Usually 4 as nesting level is sufficient to identify an
> +	 * endpoint firmware node uniquely.
> +	 */
> +	__v4l2_async_fwnode_full_name(name, len, 4, fwnode);
> +}
> +
> +#else /* CONFIG_VIDEO_V4L2_ASYNC_DEBUG */
> +#define V4L2_ASYNC_FWNODE_NAME_LEN	0
> +
> +static void v4l2_async_fwnode_full_name(char *name,
> +					unsigned int len,
> +					struct fwnode_handle *fwnode)
> +{
> +}
> +#endif /* CONFIG_VIDEO_V4L2_ASYNC_DEBUG */
> +
>  static struct device *v4l2_async_notifier_dev(
>  					struct v4l2_async_notifier *notifier)
>  {
> @@ -54,9 +104,12 @@ static void v4l2_async_notifier_call_unbind(struct v4l2_async_notifier *n,
> 
>  static int v4l2_async_notifier_call_complete(struct v4l2_async_notifier *n)
>  {
> +	struct device *dev = v4l2_async_notifier_dev(n);
>  	if (!n->ops || !n->ops->complete)
>  		return 0;
> 
> +	dev_dbg(dev, "Complete notifier \"%s\"\n", fwnode_get_name(n->owner));
> +
>  	return n->ops->complete(n);
>  }
> 
> @@ -100,8 +153,17 @@ static struct v4l2_async_subdev *v4l2_async_find_match(
>  	struct v4l2_async_notifier *notifier, struct v4l2_subdev *sd)
>  {
>  	bool (*match)(struct v4l2_subdev *, struct v4l2_async_subdev *);
> +	struct device *dev = v4l2_async_notifier_dev(notifier);
> +	char asd_full_name[V4L2_ASYNC_FWNODE_NAME_LEN];
> +	char sd_full_name[V4L2_ASYNC_FWNODE_NAME_LEN];
>  	struct v4l2_async_subdev *asd;
> 
> +	v4l2_async_fwnode_full_name(sd_full_name, V4L2_ASYNC_FWNODE_NAME_LEN,
> +				    sd->fwnode);
> +
> +	dev_dbg(dev, "Match notifier \"%s\" with subdevice \"%s\"\n",
> +		fwnode_get_name(notifier->owner), sd_full_name);
> +
>  	list_for_each_entry(asd, &notifier->waiting, list) {
>  		/* bus_type has been verified valid before */
>  		switch (asd->match_type) {
> @@ -115,6 +177,11 @@ static struct v4l2_async_subdev *v4l2_async_find_match(
>  			match = match_i2c;
>  			break;
>  		case V4L2_ASYNC_MATCH_FWNODE:
> +			v4l2_async_fwnode_full_name(asd_full_name,
> +						    V4L2_ASYNC_FWNODE_NAME_LEN,
> +						    asd->match.fwnode.fwnode);
> +			dev_dbg(dev, "Test sd \"%s\" with async_sd \"%s\"\n",
> +				sd_full_name, asd_full_name);
>  			match = match_fwnode;
>  			break;
>  		default:
> @@ -198,9 +265,16 @@ static int v4l2_async_match_notify(struct v4l2_async_notifier *notifier,
>  				   struct v4l2_subdev *sd,
>  				   struct v4l2_async_subdev *asd)
>  {
> +	struct device *dev = v4l2_async_notifier_dev(notifier);
>  	struct v4l2_async_notifier *subdev_notifier;
> +	char sd_full_name[V4L2_ASYNC_FWNODE_NAME_LEN];
>  	int ret;
> 
> +	v4l2_async_fwnode_full_name(sd_full_name, V4L2_ASYNC_FWNODE_NAME_LEN,
> +				    sd->fwnode);
> +	dev_dbg(dev, "Matched sd: \"%s\" with notifier \"%s\"\n",
> +		sd_full_name, fwnode_get_name(notifier->owner));
> +
>  	ret = v4l2_device_register_subdev(v4l2_dev, sd);
>  	if (ret < 0)
>  		return ret;
> @@ -240,6 +314,7 @@ static int v4l2_async_match_notify(struct v4l2_async_notifier *notifier,
>  static int v4l2_async_notifier_try_all_subdevs(
>  	struct v4l2_async_notifier *notifier)
>  {
> +	struct device *dev = v4l2_async_notifier_dev(notifier);
>  	struct v4l2_device *v4l2_dev =
>  		v4l2_async_notifier_find_v4l2_dev(notifier);
>  	struct v4l2_subdev *sd;
> @@ -247,6 +322,9 @@ static int v4l2_async_notifier_try_all_subdevs(
>  	if (!v4l2_dev)
>  		return 0;
> 
> +	dev_dbg(dev, "Testing notifier \"%s\" against all subdevices\n",
> +		fwnode_get_name(notifier->owner));
> +
>  again:
>  	list_for_each_entry(sd, &subdev_list, async_list) {
>  		struct v4l2_async_subdev *asd;
> @@ -378,6 +456,9 @@ static int __v4l2_async_notifier_register(struct v4l2_async_notifier *notifier)
> 
>  	notifier->owner = dev_fwnode(dev);
> 
> +	dev_dbg(dev, "Registering notifier \"%s\"\n",
> +		fwnode_get_name(notifier->owner));
> +
>  	mutex_lock(&list_lock);
> 
>  	for (i = 0; i < notifier->num_subdevs; i++) {
> --
> 2.7.4
> 

-- 
Sakari Ailus
sakari.ailus@linux.intel.com

Re: [PATCH 4/5] v4l2: async: Postpone subdev_notifier registration

[Kieran Bingham]

Hi Jacopo,

Thank you for the patch,

This seems like a good thing to do at a glance here, but I'll leave contextual
judgement like that to Sakari.

A few minor grammatical nits here and a question on locking.

On 13/12/17 18:26, Jacopo Mondi wrote:
> Currently, subdevice notifiers are tested against all available
> subdevices as soon as they get registered. It often happens anyway
> that the subdevice they are connected to is not yet initialized, as
> it usually gets registered later in drivers' code. This makes debug
> of v4l2_async particularly painful, as identifying a notifier with
> an unitialized subdevice is tricky as they don't have a valid

uninitialized

> 'struct device *' or 'struct fwnode_handle *' to be identified with.
> 
> In order to make sure that the notifier's subdevices is initialized
> when the notifier is tesed against available subdevices post-pone the
> actual notifier registration at subdevice registration time.
> 
> It is worth noting that post-poning registration of a subdevice notifier

postponing is not hyphenated.

> does not impact on the completion of the notifiers chain, as even if a
> subdev notifier completes as soon as it gets registered, the complete()
> call chain cannot be upscaled as long as the subdevice the notifiers

Upscaled? Is this the right word here ? perhaps 'processed'?

"the complete() call chain cannot be process before the subdevice to which the
notifiers belong has been registered"

> belongs to is not registered.
> 
> Also, it is now safe to access a notifier 'struct device *' as we're now
> sure it is properly initialized when the notifier is actually
> registered.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  drivers/media/v4l2-core/v4l2-async.c | 65 +++++++++++++++++++++++-------------
>  include/media/v4l2-async.h           |  2 ++
>  2 files changed, 43 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
> index 0a1bf1d..c13a781 100644
> --- a/drivers/media/v4l2-core/v4l2-async.c
> +++ b/drivers/media/v4l2-core/v4l2-async.c
> @@ -25,6 +25,13 @@
>  #include <media/v4l2-fwnode.h>
>  #include <media/v4l2-subdev.h>
> 
> +static struct device *v4l2_async_notifier_dev(
> +					struct v4l2_async_notifier *notifier)
> +{
> +	return notifier->v4l2_dev ? notifier->v4l2_dev->dev :
> +				    notifier->sd->dev;
> +}
> +
>  static int v4l2_async_notifier_call_bound(struct v4l2_async_notifier *n,
>  					  struct v4l2_subdev *subdev,
>  					  struct v4l2_async_subdev *asd)
> @@ -124,19 +131,6 @@ static struct v4l2_async_subdev *v4l2_async_find_match(
>  	return NULL;
>  }
> 
> -/* Find the sub-device notifier registered by a sub-device driver. */
> -static struct v4l2_async_notifier *v4l2_async_find_subdev_notifier(
> -	struct v4l2_subdev *sd)
> -{
> -	struct v4l2_async_notifier *n;
> -
> -	list_for_each_entry(n, &notifier_list, list)
> -		if (n->sd == sd)
> -			return n;
> -
> -	return NULL;
> -}
> -
>  /* Get v4l2_device related to the notifier if one can be found. */
>  static struct v4l2_device *v4l2_async_notifier_find_v4l2_dev(
>  	struct v4l2_async_notifier *notifier)
> @@ -160,7 +154,7 @@ static bool v4l2_async_notifier_can_complete(
> 
>  	list_for_each_entry(sd, &notifier->done, async_list) {
>  		struct v4l2_async_notifier *subdev_notifier =
> -			v4l2_async_find_subdev_notifier(sd);
> +							sd->subdev_notifier;
> 
>  		if (subdev_notifier &&
>  		    !v4l2_async_notifier_can_complete(subdev_notifier))
> @@ -228,7 +222,7 @@ static int v4l2_async_match_notify(struct v4l2_async_notifier *notifier,
>  	/*
>  	 * See if the sub-device has a notifier. If not, return here.
>  	 */
> -	subdev_notifier = v4l2_async_find_subdev_notifier(sd);
> +	subdev_notifier = sd->subdev_notifier;
>  	if (!subdev_notifier || subdev_notifier->parent)
>  		return 0;
> 
> @@ -294,7 +288,7 @@ static void v4l2_async_notifier_unbind_all_subdevs(
> 
>  	list_for_each_entry_safe(sd, tmp, &notifier->done, async_list) {
>  		struct v4l2_async_notifier *subdev_notifier =
> -			v4l2_async_find_subdev_notifier(sd);
> +							sd->subdev_notifier;
> 
>  		if (subdev_notifier)
>  			v4l2_async_notifier_unbind_all_subdevs(subdev_notifier);
> @@ -371,8 +365,7 @@ static bool v4l2_async_notifier_fwnode_has_async_subdev(
> 
>  static int __v4l2_async_notifier_register(struct v4l2_async_notifier *notifier)
>  {
> -	struct device *dev =
> -		notifier->v4l2_dev ? notifier->v4l2_dev->dev : NULL;
> +	struct device *dev = v4l2_async_notifier_dev(notifier);
>  	struct v4l2_async_subdev *asd;
>  	int ret;
>  	int i;
> @@ -383,6 +376,8 @@ static int __v4l2_async_notifier_register(struct v4l2_async_notifier *notifier)
>  	INIT_LIST_HEAD(&notifier->waiting);
>  	INIT_LIST_HEAD(&notifier->done);
> 
> +	notifier->owner = dev_fwnode(dev);
> +
>  	mutex_lock(&list_lock);
> 
>  	for (i = 0; i < notifier->num_subdevs; i++) {
> @@ -421,6 +416,7 @@ static int __v4l2_async_notifier_register(struct v4l2_async_notifier *notifier)
> 
>  	/* Keep also completed notifiers on the list */
>  	list_add(&notifier->list, &notifier_list);
> +	notifier->registered = true;
> 
>  	mutex_unlock(&list_lock);
> 
> @@ -447,7 +443,7 @@ int v4l2_async_notifier_register(struct v4l2_device *v4l2_dev,
>  		return -EINVAL;
> 
>  	notifier->v4l2_dev = v4l2_dev;
> -	notifier->owner = dev_fwnode(v4l2_dev->dev);
> +	notifier->registered = false;
> 
>  	ret = __v4l2_async_notifier_register(notifier);
>  	if (ret)
> @@ -466,7 +462,11 @@ int v4l2_async_subdev_notifier_register(struct v4l2_subdev *sd,
>  		return -EINVAL;
> 
>  	notifier->sd = sd;
> -	notifier->owner = dev_fwnode(sd->dev);
> +	sd->subdev_notifier = notifier;
> +	notifier->registered = false;
> +
> +	if (!sd->dev || !sd->fwnode)
> +		return 0;
> 
>  	ret = __v4l2_async_notifier_register(notifier);
>  	if (ret)
> @@ -482,12 +482,15 @@ static void __v4l2_async_notifier_unregister(
>  	if (!notifier || (!notifier->v4l2_dev && !notifier->sd))
>  		return;
> 
> -	v4l2_async_notifier_unbind_all_subdevs(notifier);
> +	if (notifier->registered) {
> +		v4l2_async_notifier_unbind_all_subdevs(notifier);
> +		list_del(&notifier->list);
> +	}
> 
>  	notifier->sd = NULL;
>  	notifier->v4l2_dev = NULL;
> -
> -	list_del(&notifier->list);
> +	notifier->owner = NULL;
> +	notifier->registered = false;

Do we need any locking of the notifier object now that it uses a flag and an
'asynchronous' registration ?

It might be OK ... but I haven't processed through the whole usage yet.

>  }
> 
>  void v4l2_async_notifier_unregister(struct v4l2_async_notifier *notifier)
> @@ -548,6 +551,20 @@ int v4l2_async_register_subdev(struct v4l2_subdev *sd)
>  			sd->fwnode = dev_fwnode(sd->dev);
>  	}
> 
> +	/*
> +	 * If the subdevice has an unregisterd notifier, it's now time

unregistered

--
Regards

Kieran

> +	 * to register it.> +	 */
> +	subdev_notifier = sd->subdev_notifier;
> +	if (subdev_notifier && !subdev_notifier->registered) {
> +		ret = __v4l2_async_notifier_register(subdev_notifier);
> +		if (ret) {
> +			sd->fwnode = NULL;
> +			subdev_notifier->owner = NULL;
> +			return ret;
> +		}
> +	}
> +
>  	mutex_lock(&list_lock);
> 
>  	INIT_LIST_HEAD(&sd->async_list);
> @@ -589,7 +606,7 @@ int v4l2_async_register_subdev(struct v4l2_subdev *sd)
>  	 * Complete failed. Unbind the sub-devices bound through registering
>  	 * this async sub-device.
>  	 */
> -	subdev_notifier = v4l2_async_find_subdev_notifier(sd);
> +	subdev_notifier = sd->subdev_notifier;
>  	if (subdev_notifier)
>  		v4l2_async_notifier_unbind_all_subdevs(subdev_notifier);
> 
> diff --git a/include/media/v4l2-async.h b/include/media/v4l2-async.h
> index a15c01d..6ab04ad 100644
> --- a/include/media/v4l2-async.h
> +++ b/include/media/v4l2-async.h
> @@ -110,6 +110,7 @@ struct v4l2_async_notifier_operations {
>   * @waiting:	list of struct v4l2_async_subdev, waiting for their drivers
>   * @done:	list of struct v4l2_subdev, already probed
>   * @list:	member in a global list of notifiers
> + * @registered: notifier registered complete flag
>   */
>  struct v4l2_async_notifier {
>  	const struct v4l2_async_notifier_operations *ops;
> @@ -123,6 +124,7 @@ struct v4l2_async_notifier {
>  	struct list_head waiting;
>  	struct list_head done;
>  	struct list_head list;
> +	bool registered;
>  };
> 
>  /**
> --
> 2.7.4
> 

Re: [PATCH 4/5] v4l2: async: Postpone subdev_notifier registration

[Kieran Bingham]

On 17/12/17 13:10, Kieran Bingham wrote:
> Hi Jacopo,
> 
> Thank you for the patch,
> 
> This seems like a good thing to do at a glance here, but I'll leave contextual
> judgement like that to Sakari.

Oh - I hit send and *then* my mail client wakes up and tells me Sakari reviewed
two days ago.

--
Kieran

Re: [PATCH 4/5] v4l2: async: Postpone subdev_notifier registration

[jacopo mondi]

Hi Sakari,

On Fri, Dec 15, 2017 at 05:20:40PM +0200, Sakari Ailus wrote:
> Hi Jacopo,
>
> On Wed, Dec 13, 2017 at 07:26:19PM +0100, Jacopo Mondi wrote:
> > Currently, subdevice notifiers are tested against all available
> > subdevices as soon as they get registered. It often happens anyway
> > that the subdevice they are connected to is not yet initialized, as
> > it usually gets registered later in drivers' code. This makes debug
> > of v4l2_async particularly painful, as identifying a notifier with
> > an unitialized subdevice is tricky as they don't have a valid
> > 'struct device *' or 'struct fwnode_handle *' to be identified with.
> >
> > In order to make sure that the notifier's subdevices is initialized
> > when the notifier is tesed against available subdevices post-pone the
> > actual notifier registration at subdevice registration time.
> >
> > It is worth noting that post-poning registration of a subdevice notifier
> > does not impact on the completion of the notifiers chain, as even if a
> > subdev notifier completes as soon as it gets registered, the complete()
> > call chain cannot be upscaled as long as the subdevice the notifiers
> > belongs to is not registered.
>
> Let me rephrase to make sure I understand the problem correctly ---
>
> A sub-device notifier is registered but the notifier's sub-device is not
> registered yet, and finding a match for this notifier leads, to, well
> problems. Is that the reason for this patch?

Almost :)
I never encountered problems registering the sub-notifier, but instead
identifying it when trying to debug what's happening in v4l2-async.

I had a lot of "(null)" notifiers, and that makes debug particularly
painful, as in example I had unexpected matches between a subdev and a
"(null)" notifier and it's pretty hard find out what's going wrong.

So I post-poned registratration (and consequentially matching with the
available subdevices) to a time where I know it can be identified.

>
> I think there could be simpler solutions to address this.
>
> I wonder if we could simply check for sub-device notifier's v4l2_dev field,
> and fail in matching if it's not set. v4l2_device_register_subdev() would
> still need to proceed with calling v4l2_async_notifier_try_all_subdevs()
> and v4l2_async_notifier_try_complete() if that was the case.
>
> What do you think?

v4l2_dev is only set in root notifiers, we maybe can check for a valid
"struct device *" and refuse notifiers without an initialized one in
"__v4l2_async_notifier_register()".

And you're actually right here, when later the subdevice owning the just
refused sub-notifier gets registered (and eventually matched) its
sub-notifier will be processed anyhow, and this makes hunk  "@@ -548,6
+551,20 @@" of my patch unnecessary.

I will test and simplify it. Thanks

>
> >
> > Also, it is now safe to access a notifier 'struct device *' as we're now
> > sure it is properly initialized when the notifier is actually
> > registered.
> >
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > ---
> >  drivers/media/v4l2-core/v4l2-async.c | 65 +++++++++++++++++++++++-------------
> >  include/media/v4l2-async.h           |  2 ++
> >  2 files changed, 43 insertions(+), 24 deletions(-)
> >
> > diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
> > index 0a1bf1d..c13a781 100644
> > --- a/drivers/media/v4l2-core/v4l2-async.c
> > +++ b/drivers/media/v4l2-core/v4l2-async.c
> > @@ -25,6 +25,13 @@
> >  #include <media/v4l2-fwnode.h>
> >  #include <media/v4l2-subdev.h>
> >
> > +static struct device *v4l2_async_notifier_dev(
> > +					struct v4l2_async_notifier *notifier)
> > +{
> > +	return notifier->v4l2_dev ? notifier->v4l2_dev->dev :
> > +				    notifier->sd->dev;
> > +}
> > +
> >  static int v4l2_async_notifier_call_bound(struct v4l2_async_notifier *n,
> >  					  struct v4l2_subdev *subdev,
> >  					  struct v4l2_async_subdev *asd)
> > @@ -124,19 +131,6 @@ static struct v4l2_async_subdev *v4l2_async_find_match(
> >  	return NULL;
> >  }
> >
> > -/* Find the sub-device notifier registered by a sub-device driver. */
> > -static struct v4l2_async_notifier *v4l2_async_find_subdev_notifier(
> > -	struct v4l2_subdev *sd)
> > -{
> > -	struct v4l2_async_notifier *n;
> > -
> > -	list_for_each_entry(n, &notifier_list, list)
> > -		if (n->sd == sd)
> > -			return n;
> > -
> > -	return NULL;
> > -}
> > -
> >  /* Get v4l2_device related to the notifier if one can be found. */
> >  static struct v4l2_device *v4l2_async_notifier_find_v4l2_dev(
> >  	struct v4l2_async_notifier *notifier)
> > @@ -160,7 +154,7 @@ static bool v4l2_async_notifier_can_complete(
> >
> >  	list_for_each_entry(sd, &notifier->done, async_list) {
> >  		struct v4l2_async_notifier *subdev_notifier =
> > -			v4l2_async_find_subdev_notifier(sd);
> > +							sd->subdev_notifier;
> >
> >  		if (subdev_notifier &&
> >  		    !v4l2_async_notifier_can_complete(subdev_notifier))
> > @@ -228,7 +222,7 @@ static int v4l2_async_match_notify(struct v4l2_async_notifier *notifier,
> >  	/*
> >  	 * See if the sub-device has a notifier. If not, return here.
> >  	 */
> > -	subdev_notifier = v4l2_async_find_subdev_notifier(sd);
> > +	subdev_notifier = sd->subdev_notifier;
> >  	if (!subdev_notifier || subdev_notifier->parent)
> >  		return 0;
> >
> > @@ -294,7 +288,7 @@ static void v4l2_async_notifier_unbind_all_subdevs(
> >
> >  	list_for_each_entry_safe(sd, tmp, &notifier->done, async_list) {
> >  		struct v4l2_async_notifier *subdev_notifier =
> > -			v4l2_async_find_subdev_notifier(sd);
> > +							sd->subdev_notifier;
> >
> >  		if (subdev_notifier)
> >  			v4l2_async_notifier_unbind_all_subdevs(subdev_notifier);
> > @@ -371,8 +365,7 @@ static bool v4l2_async_notifier_fwnode_has_async_subdev(
> >
> >  static int __v4l2_async_notifier_register(struct v4l2_async_notifier *notifier)
> >  {
> > -	struct device *dev =
> > -		notifier->v4l2_dev ? notifier->v4l2_dev->dev : NULL;
> > +	struct device *dev = v4l2_async_notifier_dev(notifier);
> >  	struct v4l2_async_subdev *asd;
> >  	int ret;
> >  	int i;
> > @@ -383,6 +376,8 @@ static int __v4l2_async_notifier_register(struct v4l2_async_notifier *notifier)
> >  	INIT_LIST_HEAD(&notifier->waiting);
> >  	INIT_LIST_HEAD(&notifier->done);
> >
> > +	notifier->owner = dev_fwnode(dev);
> > +
> >  	mutex_lock(&list_lock);
> >
> >  	for (i = 0; i < notifier->num_subdevs; i++) {
> > @@ -421,6 +416,7 @@ static int __v4l2_async_notifier_register(struct v4l2_async_notifier *notifier)
> >
> >  	/* Keep also completed notifiers on the list */
> >  	list_add(&notifier->list, &notifier_list);
> > +	notifier->registered = true;
> >
> >  	mutex_unlock(&list_lock);
> >
> > @@ -447,7 +443,7 @@ int v4l2_async_notifier_register(struct v4l2_device *v4l2_dev,
> >  		return -EINVAL;
> >
> >  	notifier->v4l2_dev = v4l2_dev;
> > -	notifier->owner = dev_fwnode(v4l2_dev->dev);
> > +	notifier->registered = false;
> >
> >  	ret = __v4l2_async_notifier_register(notifier);
> >  	if (ret)
> > @@ -466,7 +462,11 @@ int v4l2_async_subdev_notifier_register(struct v4l2_subdev *sd,
> >  		return -EINVAL;
> >
> >  	notifier->sd = sd;
> > -	notifier->owner = dev_fwnode(sd->dev);
> > +	sd->subdev_notifier = notifier;
> > +	notifier->registered = false;
> > +
> > +	if (!sd->dev || !sd->fwnode)
> > +		return 0;
> >
> >  	ret = __v4l2_async_notifier_register(notifier);
> >  	if (ret)
> > @@ -482,12 +482,15 @@ static void __v4l2_async_notifier_unregister(
> >  	if (!notifier || (!notifier->v4l2_dev && !notifier->sd))
> >  		return;
> >
> > -	v4l2_async_notifier_unbind_all_subdevs(notifier);
> > +	if (notifier->registered) {
> > +		v4l2_async_notifier_unbind_all_subdevs(notifier);
> > +		list_del(&notifier->list);
> > +	}
> >
> >  	notifier->sd = NULL;
> >  	notifier->v4l2_dev = NULL;
> > -
> > -	list_del(&notifier->list);
> > +	notifier->owner = NULL;
> > +	notifier->registered = false;
> >  }
> >
> >  void v4l2_async_notifier_unregister(struct v4l2_async_notifier *notifier)
> > @@ -548,6 +551,20 @@ int v4l2_async_register_subdev(struct v4l2_subdev *sd)
> >  			sd->fwnode = dev_fwnode(sd->dev);
> >  	}
> >
> > +	/*
> > +	 * If the subdevice has an unregisterd notifier, it's now time
> > +	 * to register it.
> > +	 */
> > +	subdev_notifier = sd->subdev_notifier;
> > +	if (subdev_notifier && !subdev_notifier->registered) {
> > +		ret = __v4l2_async_notifier_register(subdev_notifier);
> > +		if (ret) {
> > +			sd->fwnode = NULL;
> > +			subdev_notifier->owner = NULL;
> > +			return ret;
> > +		}
> > +	}
> > +
> >  	mutex_lock(&list_lock);
> >
> >  	INIT_LIST_HEAD(&sd->async_list);
> > @@ -589,7 +606,7 @@ int v4l2_async_register_subdev(struct v4l2_subdev *sd)
> >  	 * Complete failed. Unbind the sub-devices bound through registering
> >  	 * this async sub-device.
> >  	 */
> > -	subdev_notifier = v4l2_async_find_subdev_notifier(sd);
> > +	subdev_notifier = sd->subdev_notifier;
> >  	if (subdev_notifier)
> >  		v4l2_async_notifier_unbind_all_subdevs(subdev_notifier);
> >
> > diff --git a/include/media/v4l2-async.h b/include/media/v4l2-async.h
> > index a15c01d..6ab04ad 100644
> > --- a/include/media/v4l2-async.h
> > +++ b/include/media/v4l2-async.h
> > @@ -110,6 +110,7 @@ struct v4l2_async_notifier_operations {
> >   * @waiting:	list of struct v4l2_async_subdev, waiting for their drivers
> >   * @done:	list of struct v4l2_subdev, already probed
> >   * @list:	member in a global list of notifiers
> > + * @registered: notifier registered complete flag
> >   */
> >  struct v4l2_async_notifier {
> >  	const struct v4l2_async_notifier_operations *ops;
> > @@ -123,6 +124,7 @@ struct v4l2_async_notifier {
> >  	struct list_head waiting;
> >  	struct list_head done;
> >  	struct list_head list;
> > +	bool registered;
> >  };
> >
> >  /**
> > --
> > 2.7.4
> >
>
> --
> Sakari Ailus
> sakari.ailus@linux.intel.com

Re: [PATCH 5/5] v4l2: async: Add debug output to v4l2-async module

[jacopo mondi]

Hi Sakari,

On Fri, Dec 15, 2017 at 06:17:04PM +0200, Sakari Ailus wrote:
> Hi Jacopo,
>
> On Wed, Dec 13, 2017 at 07:26:20PM +0100, Jacopo Mondi wrote:
> > The v4l2-async module operations are quite complex to follow, due to the
> > asynchronous nature of subdevices and notifiers registration and
> > matching procedures. In order to help with debugging of failed or
> > erroneous matching between a subdevice and the notifier collected
> > async_subdevice it gets matched against, introduce a few dev_dbg() calls
> > in v4l2_async core operations.
> >
> > Protect the debug operations with a Kconfig defined symbol, to make sure
> > when debugging is disabled, no additional code or data is added to the
> > module.
> >
> > Notifiers are identified by the name of the subdevice or v4l2_dev they are
> > registered by, while subdevice matching which now happens on endpoints,
> > need a longer description built walking the fwnode graph backwards
> > collecting parent nodes names (otherwise we would have had printouts
> > like: "Matching "endpoint" with "endpoint"" which are not that useful).
> >
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> >
> > ---
> > For fwnodes backed by OF, I may have used the "%pOF" format modifier to
> > get the full node name instead of parsing the fwnode graph by myself with
> > "v4l2_async_fwnode_full_name()". Unfortunately I'm not aware of anything
> > like "%pOF" for ACPI backed fwnodes. Also, walking the fwnode graph by
> > myself allows me to reduce the depth, to reduce the debug messages output
> > length which is anyway long enough to result disturbing on a 80columns
> > terminal window.
>
> ACPI doesn't have such at the moment. I think printing the full path would
> still be better. There isn't that much more to print after all.

So you suggest to just use the full node name for OF. What about ACPI?

>From your other reply I got that I can print the single node name for
"device ACPI nodes" but not for "non-device ACPI nodes". Should I build
the full device name in drivers/acpi/properties.c for ACPI devices
like I'm doing here for fwnodes?

>
> > ---
> >
> >  drivers/media/v4l2-core/Kconfig      |  8 ++++
> >  drivers/media/v4l2-core/v4l2-async.c | 81 ++++++++++++++++++++++++++++++++++++
> >  2 files changed, 89 insertions(+)
> >
> > diff --git a/drivers/media/v4l2-core/Kconfig b/drivers/media/v4l2-core/Kconfig
> > index a35c336..8331736 100644
> > --- a/drivers/media/v4l2-core/Kconfig
> > +++ b/drivers/media/v4l2-core/Kconfig
> > @@ -17,6 +17,14 @@ config VIDEO_ADV_DEBUG
> >  	  V4L devices.
> >  	  In doubt, say N.
> >
> > +config VIDEO_V4L2_ASYNC_DEBUG
> > +	bool "Enable debug functionalities for V4L2 async module"
> > +	depends on VIDEO_V4L2
>
> I'm not sure I'd add a Kconfig option. This is adding a fairly simple
> function only to the kernel.

So I will use a symbol defined in the module to enable/disable debug
(maybe the "DEBUG" symbol itself?)

>
> > +	default n
> > +	---help---
> > +	  Say Y here to enable debug output in V4L2 async module.
> > +	  In doubt, say N.
> > +
> >  config VIDEO_FIXED_MINOR_RANGES
> >  	bool "Enable old-style fixed minor ranges on drivers/video devices"
> >  	default n
> > diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
> > index c13a781..307e1a5 100644
> > --- a/drivers/media/v4l2-core/v4l2-async.c
> > +++ b/drivers/media/v4l2-core/v4l2-async.c
> > @@ -8,6 +8,10 @@
> >   * published by the Free Software Foundation.
> >   */
> >
> > +#if defined(CONFIG_VIDEO_V4L2_ASYNC_DEBUG)
> > +#define DEBUG
>
> Do you need this?

No dev_dbg() otherwise, isn't it?

>
> > +#endif
> > +
> >  #include <linux/device.h>
> >  #include <linux/err.h>
> >  #include <linux/i2c.h>
> > @@ -25,6 +29,52 @@
> >  #include <media/v4l2-fwnode.h>
> >  #include <media/v4l2-subdev.h>
> >
> > +#if defined(CONFIG_VIDEO_V4L2_ASYNC_DEBUG)
> > +#define V4L2_ASYNC_FWNODE_NAME_LEN	512
> > +
> > +static void __v4l2_async_fwnode_full_name(char *name,
> > +					  unsigned int len,
> > +					  unsigned int max_depth,
> > +					  struct fwnode_handle *fwnode)
> > +{
> > +	unsigned int buf_len = len < V4L2_ASYNC_FWNODE_NAME_LEN ?
> > +			       len : V4L2_ASYNC_FWNODE_NAME_LEN;
> > +	char __tmp[V4L2_ASYNC_FWNODE_NAME_LEN];
>
> That's a bit too much to allocate from the stack I think.

For an full name do you think 128 is enough? 256 maybe?

Thanks
  j

Re: [PATCH 1/5] v4l: async: Use endpoint node, not device node, for fwnode match

[Laurent Pinchart]

Hi Sakari,

Thank you for the patch.

On Wednesday, 13 December 2017 20:26:16 EET Jacopo Mondi wrote:
> From: Sakari Ailus <sakari.ailus@linux.intel.com>
> 
> V4L2 async framework can use both device's fwnode and endpoints's fwnode
> for matching the async sub-device with the sub-device. In order to proceed
> moving towards endpoint matching assign the endpoint to the async
> sub-device.
> 
> As most async sub-device drivers (and the related hardware) only supports
> a single endpoint, use the first endpoint found. This works for all
> current drivers --- we only ever supported a single async sub-device per
> device to begin with.
> 
> For async devices that have no endpoints, continue to use the fwnode
> related to the device. This includes e.g. lens devices.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  drivers/media/platform/am437x/am437x-vpfe.c    |  2 +-
>  drivers/media/platform/atmel/atmel-isc.c       |  2 +-
>  drivers/media/platform/atmel/atmel-isi.c       |  2 +-
>  drivers/media/platform/davinci/vpif_capture.c  |  2 +-
>  drivers/media/platform/exynos4-is/media-dev.c  | 14 ++++++++++----
>  drivers/media/platform/pxa_camera.c            |  2 +-
>  drivers/media/platform/qcom/camss-8x16/camss.c |  2 +-
>  drivers/media/platform/rcar_drif.c             |  2 +-
>  drivers/media/platform/stm32/stm32-dcmi.c      |  2 +-
>  drivers/media/platform/ti-vpe/cal.c            |  2 +-
>  drivers/media/platform/xilinx/xilinx-vipp.c    | 16 +++++++++++++---
>  drivers/media/v4l2-core/v4l2-async.c           |  8 ++++++--
>  drivers/media/v4l2-core/v4l2-fwnode.c          |  2 +-
>  13 files changed, 39 insertions(+), 19 deletions(-)

[snip]

> diff --git a/drivers/media/platform/davinci/vpif_capture.c
> b/drivers/media/platform/davinci/vpif_capture.c index a89367a..e150d75
> 100644
> --- a/drivers/media/platform/davinci/vpif_capture.c
> +++ b/drivers/media/platform/davinci/vpif_capture.c
> @@ -1572,7 +1572,7 @@ vpif_capture_get_pdata(struct platform_device *pdev)
>  		if (flags & V4L2_MBUS_VSYNC_ACTIVE_HIGH)
>  			chan->vpif_if.vd_pol = 1;
> 
> -		rem = of_graph_get_remote_port_parent(endpoint);
> +		rem = of_graph_get_remote_endpoint(endpoint);
>  		if (!rem) {
>  			dev_dbg(&pdev->dev, "Remote device at %pOF not found\n",
>  				endpoint);

The node's full name is used as the subdev name, have you verified that this 
change won't break the driver ?

> diff --git a/drivers/media/platform/exynos4-is/media-dev.c
> b/drivers/media/platform/exynos4-is/media-dev.c index c15596b..c6b0220
> 100644
> --- a/drivers/media/platform/exynos4-is/media-dev.c
> +++ b/drivers/media/platform/exynos4-is/media-dev.c
> @@ -409,7 +409,7 @@ static int fimc_md_parse_port_node(struct fimc_md *fmd,
> 
>  	pd->mux_id = (endpoint.base.port - 1) & 0x1;
> 
> -	rem = of_graph_get_remote_port_parent(ep);
> +	rem = of_graph_get_remote_endpoint(ep);
>  	of_node_put(ep);
>  	if (rem == NULL) {
>  		v4l2_info(&fmd->v4l2_dev, "Remote device at %pOF not found\n",
> @@ -1360,11 +1360,17 @@ static int subdev_notifier_bound(struct
> v4l2_async_notifier *notifier, int i;
> 
>  	/* Find platform data for this sensor subdev */
> -	for (i = 0; i < ARRAY_SIZE(fmd->sensor); i++)
> -		if (fmd->sensor[i].asd.match.fwnode.fwnode ==
> -		    of_fwnode_handle(subdev->dev->of_node))
> +	for (i = 0; i < ARRAY_SIZE(fmd->sensor); i++) {
> +		struct fwnode_handle *fwnode =
> +			fwnode_graph_get_port_parent(
> +				of_fwnode_handle(subdev->dev->of_node));

Isn't fwnode_graph_get_port_parent() supposed to be called on an endpoint node 
? subdev->dev->of_node is the device's node.

> +		if (fmd->sensor[i].asd.match.fwnode.fwnode == fwnode)
>  			si = &fmd->sensor[i];
> 
> +		fwnode_handle_put(fwnode);
> +	}
> +
>  	if (si == NULL)
>  		return -EINVAL;
> 

[snip]

> diff --git a/drivers/media/platform/ti-vpe/cal.c
> b/drivers/media/platform/ti-vpe/cal.c index 8b586c8..9b29706 100644
> --- a/drivers/media/platform/ti-vpe/cal.c
> +++ b/drivers/media/platform/ti-vpe/cal.c
> @@ -1699,7 +1699,7 @@ static int of_cal_create_instance(struct cal_ctx *ctx,
> int inst) goto cleanup_exit;
>  	}
> 
> -	sensor_node = of_graph_get_remote_port_parent(ep_node);
> +	sensor_node = of_graph_get_remote_endpoint(ep_node);
>  	if (!sensor_node) {
>  		ctx_dbg(3, ctx, "can't get remote parent\n");
>  		goto cleanup_exit;

sensor_node->name is used in a debug message that will become a bit less 
useful as a result.

> diff --git a/drivers/media/platform/xilinx/xilinx-vipp.c
> b/drivers/media/platform/xilinx/xilinx-vipp.c index d881cf0..17d4ac0 100644
> --- a/drivers/media/platform/xilinx/xilinx-vipp.c
> +++ b/drivers/media/platform/xilinx/xilinx-vipp.c
> @@ -82,6 +82,8 @@ static int xvip_graph_build_one(struct
> xvip_composite_device *xdev, dev_dbg(xdev->dev, "creating links for entity
> %s\n", local->name);
> 
>  	while (1) {
> +		struct fwnode_handle *fwnode;
> +
>  		/* Get the next endpoint and parse its link. */
>  		next = of_graph_get_next_endpoint(entity->node, ep);
>  		if (next == NULL)
> @@ -121,8 +123,11 @@ static int xvip_graph_build_one(struct
> xvip_composite_device *xdev, continue;
>  		}
> 
> +		fwnode = fwnode_graph_get_port_parent(link.remote_node);
> +		fwnode_handle_put(fwnode);
> +
>  		/* Skip DMA engines, they will be processed separately. */
> -		if (link.remote_node == of_fwnode_handle(xdev->dev->of_node)) {
> +		if (fwnode == of_fwnode_handle(xdev->dev->of_node)) {
>  			dev_dbg(xdev->dev, "skipping DMA port %pOF:%u\n",
>  				to_of_node(link.local_node),
>  				link.local_port);
> @@ -367,20 +372,25 @@ static int xvip_graph_parse_one(struct
> xvip_composite_device *xdev, dev_dbg(xdev->dev, "parsing node %pOF\n",
> node);
> 
>  	while (1) {
> +		struct fwnode_handle *fwnode;
> +
>  		ep = of_graph_get_next_endpoint(node, ep);
>  		if (ep == NULL)
>  			break;
> 
>  		dev_dbg(xdev->dev, "handling endpoint %pOF\n", ep);
> 
> -		remote = of_graph_get_remote_port_parent(ep);
> +		remote = of_graph_get_remote_endpoint(ep);
>  		if (remote == NULL) {
>  			ret = -EINVAL;
>  			break;
>  		}
> 
> +		fwnode = fwnode_graph_get_port_parent(of_fwnode_handle(remote));
> +		fwnode_handle_put(fwnode);
> +
>  		/* Skip entities that we have already processed. */
> -		if (remote == xdev->dev->of_node ||
> +		if (fwnode == xdev->dev->of_node ||

The former is a fwnode_handle pointer and the latter a device_node pointer, I 
don't think that's expected. Doesn't gcc generate a warning ?

>  		    xvip_graph_find_entity(xdev, remote)) {
>  			of_node_put(remote);
>  			continue;
> diff --git a/drivers/media/v4l2-core/v4l2-async.c
> b/drivers/media/v4l2-core/v4l2-async.c index a7c3464..a6bddff 100644
> --- a/drivers/media/v4l2-core/v4l2-async.c
> +++ b/drivers/media/v4l2-core/v4l2-async.c
> @@ -539,8 +539,12 @@ int v4l2_async_register_subdev(struct v4l2_subdev *sd)
>  	 * (struct v4l2_subdev.dev), and async sub-device does not
>  	 * exist independently of the device at any point of time.
>  	 */
> -	if (!sd->fwnode && sd->dev)
> -		sd->fwnode = dev_fwnode(sd->dev);
> +	if (!sd->fwnode && sd->dev) {
> +		sd->fwnode = fwnode_graph_get_next_endpoint(
> +			dev_fwnode(sd->dev), NULL);
> +		if (!sd->fwnode)
> +			sd->fwnode = dev_fwnode(sd->dev);

Shouldn't you drop the reference to the fwnode here, as the framework doesn't 
release it (see the comment just above this piece of code) ? You'll have to 
update the comment as well to explain the new mechanism.

> +	}
> 
>  	mutex_lock(&list_lock);
> 

[snip]

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/5] device property: Add fwnode_get_name() operation

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Wednesday, 13 December 2017 20:26:17 EET Jacopo Mondi wrote:
> Add operation to retrieve the device name from a fwnode handle.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  drivers/acpi/property.c  |  6 ++++++
>  drivers/base/property.c  | 12 ++++++++++++
>  drivers/of/property.c    |  6 ++++++
>  include/linux/fwnode.h   |  2 ++
>  include/linux/property.h |  1 +
>  5 files changed, 27 insertions(+)
> 
> diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
> index e26ea20..1e3971c 100644
> --- a/drivers/acpi/property.c
> +++ b/drivers/acpi/property.c
> @@ -1186,6 +1186,11 @@ acpi_fwnode_property_read_string_array(const struct
> fwnode_handle *fwnode, val, nval);
>  }
> 
> +static const char *acpi_fwnode_get_name(const struct fwnode_handle *fwnode)
> +{
> +	return acpi_dev_name(to_acpi_device_node(fwnode));

You're returning a name here, but it's not the ACPI fwnode name, it's the name 
of the corresponding device. This isn't consistent with the OF implementation. 
As Sakari pointed out, it also won't work for non-device ACPI nodes.

> +}
> +
>  static struct fwnode_handle *
>  acpi_fwnode_get_named_child_node(const struct fwnode_handle *fwnode,
>  				 const char *childname)
> @@ -1281,6 +1286,7 @@ static int acpi_fwnode_graph_parse_endpoint(const
> struct fwnode_handle *fwnode, acpi_fwnode_property_read_string_array,		\
>  		.get_parent = acpi_node_get_parent,			\
>  		.get_next_child_node = acpi_get_next_subnode,		\
> +		.get_name = acpi_fwnode_get_name,			\
>  		.get_named_child_node = acpi_fwnode_get_named_child_node, \
>  		.get_reference_args = acpi_fwnode_get_reference_args,	\
>  		.graph_get_next_endpoint =				\
> diff --git a/drivers/base/property.c b/drivers/base/property.c
> index 851b1b6..a87b4a9 100644
> --- a/drivers/base/property.c
> +++ b/drivers/base/property.c
> @@ -950,6 +950,18 @@ int device_add_properties(struct device *dev,
>  EXPORT_SYMBOL_GPL(device_add_properties);
> 
>  /**
> + * fwnode_get_name - Return the fwnode_handle name
> + * @fwnode: Firmware node to get name from
> + *
> + * Returns a pointer to the firmware node name

Could you please document the life time constraints of the return pointer ?

> + */
> +const char *fwnode_get_name(const struct fwnode_handle *fwnode)
> +{
> +	return fwnode_call_ptr_op(fwnode, get_name);
> +}
> +EXPORT_SYMBOL(fwnode_get_name);
> +
> +/**
>   * fwnode_get_next_parent - Iterate to the node's parent
>   * @fwnode: Firmware whose parent is retrieved
>   *
> diff --git a/drivers/of/property.c b/drivers/of/property.c
> index 8ad33a4..6c195a8 100644
> --- a/drivers/of/property.c
> +++ b/drivers/of/property.c
> @@ -875,6 +875,11 @@ of_fwnode_property_read_string_array(const struct
> fwnode_handle *fwnode, of_property_count_strings(node, propname);
>  }
> 
> +static const char *of_fwnode_get_name(const struct fwnode_handle *fwnode)
> +{
> +	return of_node_full_name(to_of_node(fwnode));

If you're returning the full name shouldn't the operation be called 
*get_full_name() ?

> +}
> +
>  static struct fwnode_handle *
>  of_fwnode_get_parent(const struct fwnode_handle *fwnode)
>  {
> @@ -988,6 +993,7 @@ const struct fwnode_operations of_fwnode_ops = {
>  	.property_present = of_fwnode_property_present,
>  	.property_read_int_array = of_fwnode_property_read_int_array,
>  	.property_read_string_array = of_fwnode_property_read_string_array,
> +	.get_name = of_fwnode_get_name,
>  	.get_parent = of_fwnode_get_parent,
>  	.get_next_child_node = of_fwnode_get_next_child_node,
>  	.get_named_child_node = of_fwnode_get_named_child_node,
> diff --git a/include/linux/fwnode.h b/include/linux/fwnode.h
> index 411a84c..5d3a8c6 100644
> --- a/include/linux/fwnode.h
> +++ b/include/linux/fwnode.h
> @@ -57,6 +57,7 @@ struct fwnode_reference_args {
>   *				 otherwise.
>   * @property_read_string_array: Read an array of string properties. Return
> zero *				on success, a negative error code otherwise.
> + * @get_name: Return the fwnode name.
>   * @get_parent: Return the parent of an fwnode.
>   * @get_next_child_node: Return the next child node in an iteration.
>   * @get_named_child_node: Return a child node with a given name.
> @@ -81,6 +82,7 @@ struct fwnode_operations {
>  	(*property_read_string_array)(const struct fwnode_handle *fwnode_handle,
>  				      const char *propname, const char **val,
>  				      size_t nval);
> +	const char *(*get_name)(const struct fwnode_handle *fwnode);
>  	struct fwnode_handle *(*get_parent)(const struct fwnode_handle *fwnode);
>  	struct fwnode_handle *
>  	(*get_next_child_node)(const struct fwnode_handle *fwnode,
> diff --git a/include/linux/property.h b/include/linux/property.h
> index f6189a3..0fc464f 100644
> --- a/include/linux/property.h
> +++ b/include/linux/property.h
> @@ -78,6 +78,7 @@ int fwnode_property_get_reference_args(const struct
> fwnode_handle *fwnode, unsigned int nargs, unsigned int index,
>  				       struct fwnode_reference_args *args);
> 
> +const char *fwnode_get_name(const struct fwnode_handle *fwnode);
>  struct fwnode_handle *fwnode_get_parent(const struct fwnode_handle
> *fwnode);
>  struct fwnode_handle *fwnode_get_next_parent(struct fwnode_handle *fwnode);

-- 
Regards,

Laurent Pinchart

Re: [PATCH 3/5] include: v4l2_async: Add 'owner' field to notifier

[Laurent Pinchart]

Hello,

On Friday, 15 December 2017 16:38:16 EET Sakari Ailus wrote:
> Hi Jacopo,
> 
> On Wed, Dec 13, 2017 at 07:26:18PM +0100, Jacopo Mondi wrote:
> > Notifiers can be registered as root notifiers (identified by a 'struct
> > v4l2_device *') or subdevice notifiers (identified by a 'struct
> > v4l2_subdev *'). In order to identify a notifier no matter if it is root
> > or not, add a 'struct fwnode_handle *owner' field, whose name can be
> > printed out for debug purposes.
> > 
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> 
> You'll have struct device either through the v4l2_device or v4l2_subdev. Do
> you need an additional field for this?

I agree with this comment. If there's a reason to add a new field, its life 
time constraints should be documented. The fwnodes are refcounted and you're 
not increasing the refcount here, you should explain why you don't need to.

-- 
Regards,

Laurent Pinchart

Re: [PATCH 4/5] v4l2: async: Postpone subdev_notifier registration

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Wednesday, 13 December 2017 20:26:19 EET Jacopo Mondi wrote:
> Currently, subdevice notifiers are tested against all available
> subdevices as soon as they get registered. It often happens anyway
> that the subdevice they are connected to is not yet initialized, as
> it usually gets registered later in drivers' code. This makes debug
> of v4l2_async particularly painful, as identifying a notifier with
> an unitialized subdevice is tricky as they don't have a valid
> 'struct device *' or 'struct fwnode_handle *' to be identified with.
> 
> In order to make sure that the notifier's subdevices is initialized
> when the notifier is tesed against available subdevices post-pone the
> actual notifier registration at subdevice registration time.

Aren't you piling yet another hack on top of an already dirty framework ? How 
about fixing the root cause of the issue and ensuring that subdevs are 
properly initialized when the notifier is registered ?

> It is worth noting that post-poning registration of a subdevice notifier
> does not impact on the completion of the notifiers chain, as even if a
> subdev notifier completes as soon as it gets registered, the complete()
> call chain cannot be upscaled as long as the subdevice the notifiers
> belongs to is not registered.
> 
> Also, it is now safe to access a notifier 'struct device *' as we're now
> sure it is properly initialized when the notifier is actually
> registered.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  drivers/media/v4l2-core/v4l2-async.c | 65 +++++++++++++++++++++-----------
>  include/media/v4l2-async.h           |  2 ++
>  2 files changed, 43 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-async.c
> b/drivers/media/v4l2-core/v4l2-async.c index 0a1bf1d..c13a781 100644
> --- a/drivers/media/v4l2-core/v4l2-async.c
> +++ b/drivers/media/v4l2-core/v4l2-async.c

[snip]

> @@ -548,6 +551,20 @@ int v4l2_async_register_subdev(struct v4l2_subdev *sd)
>  			sd->fwnode = dev_fwnode(sd->dev);
>  	}
> 
> +	/*
> +	 * If the subdevice has an unregisterd notifier, it's now time
> +	 * to register it.
> +	 */
> +	subdev_notifier = sd->subdev_notifier;
> +	if (subdev_notifier && !subdev_notifier->registered) {
> +		ret = __v4l2_async_notifier_register(subdev_notifier);
> +		if (ret) {
> +			sd->fwnode = NULL;
> +			subdev_notifier->owner = NULL;
> +			return ret;
> +		}
> +	}

This is the part I like the least in this patch set. The 
v4l2_subdev::subdev_notifier field should really disappear, there's no reason 
to limit subdevs to a single notifier. Implicit registration of notifiers is a 
dirty hack in my opinion.

>  	mutex_lock(&list_lock);
> 
>  	INIT_LIST_HEAD(&sd->async_list);

[snip]

-- 
Regards,

Laurent Pinchart

Re: [PATCH 5/5] v4l2: async: Add debug output to v4l2-async module

[Laurent Pinchart]

Hello,

On Friday, 15 December 2017 18:17:04 EET Sakari Ailus wrote:
> On Wed, Dec 13, 2017 at 07:26:20PM +0100, Jacopo Mondi wrote:
> > The v4l2-async module operations are quite complex to follow, due to the
> > asynchronous nature of subdevices and notifiers registration and
> > matching procedures. In order to help with debugging of failed or
> > erroneous matching between a subdevice and the notifier collected
> > async_subdevice it gets matched against, introduce a few dev_dbg() calls
> > in v4l2_async core operations.
> > 
> > Protect the debug operations with a Kconfig defined symbol, to make sure
> > when debugging is disabled, no additional code or data is added to the
> > module.
> > 
> > Notifiers are identified by the name of the subdevice or v4l2_dev they are
> > registered by, while subdevice matching which now happens on endpoints,
> > need a longer description built walking the fwnode graph backwards
> > collecting parent nodes names (otherwise we would have had printouts
> > like: "Matching "endpoint" with "endpoint"" which are not that useful).
> > 
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > 
> > ---
> > For fwnodes backed by OF, I may have used the "%pOF" format modifier to
> > get the full node name instead of parsing the fwnode graph by myself with
> > "v4l2_async_fwnode_full_name()". Unfortunately I'm not aware of anything
> > like "%pOF" for ACPI backed fwnodes. Also, walking the fwnode graph by
> > myself allows me to reduce the depth, to reduce the debug messages output
> > length which is anyway long enough to result disturbing on a 80columns
> > terminal window.
> 
> ACPI doesn't have such at the moment. I think printing the full path would
> still be better. There isn't that much more to print after all.
> 
> > ---
> > 
> >  drivers/media/v4l2-core/Kconfig      |  8 ++++
> >  drivers/media/v4l2-core/v4l2-async.c | 81 +++++++++++++++++++++++++++++++
> >  2 files changed, 89 insertions(+)
> > 
> > diff --git a/drivers/media/v4l2-core/Kconfig
> > b/drivers/media/v4l2-core/Kconfig index a35c336..8331736 100644
> > --- a/drivers/media/v4l2-core/Kconfig
> > +++ b/drivers/media/v4l2-core/Kconfig
> > @@ -17,6 +17,14 @@ config VIDEO_ADV_DEBUG
> >  	  V4L devices.
> >  	  In doubt, say N.
> > 
> > +config VIDEO_V4L2_ASYNC_DEBUG
> > +	bool "Enable debug functionalities for V4L2 async module"
> > +	depends on VIDEO_V4L2
> 
> I'm not sure I'd add a Kconfig option. This is adding a fairly simple
> function only to the kernel.
> 
> > +	default n
> > +	---help---
> > +	  Say Y here to enable debug output in V4L2 async module.
> > +	  In doubt, say N.
> > +
> >  config VIDEO_FIXED_MINOR_RANGES
> >  	bool "Enable old-style fixed minor ranges on drivers/video devices"
> >  	default n
> > diff --git a/drivers/media/v4l2-core/v4l2-async.c
> > b/drivers/media/v4l2-core/v4l2-async.c index c13a781..307e1a5 100644
> > --- a/drivers/media/v4l2-core/v4l2-async.c
> > +++ b/drivers/media/v4l2-core/v4l2-async.c
> > @@ -8,6 +8,10 @@
> >   * published by the Free Software Foundation.
> >   */
> > 
> > +#if defined(CONFIG_VIDEO_V4L2_ASYNC_DEBUG)
> > +#define DEBUG
> 
> Do you need this?

No this isn't needed. Debugging can be enabled through dynamic debug without 
requiring the Kconfig option. A Kconfig option might be useful to avoid 
compiling the debug code in kernels that have dynamic debug enabled, but those 
are large already and the amount of debug code here is limited, so I don't 
think it's worth it.

> > +#endif
> > +
> > 
> >  #include <linux/device.h>
> >  #include <linux/err.h>
> >  #include <linux/i2c.h>

[snip]

-- 
Regards,

Laurent Pinchart

Re: [PATCH 4/5] v4l2: async: Postpone subdev_notifier registration

[Sakari Ailus]

Hi Laurent,

On Sun, Dec 17, 2017 at 07:03:17PM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
> 
> Thank you for the patch.
> 
> On Wednesday, 13 December 2017 20:26:19 EET Jacopo Mondi wrote:
> > Currently, subdevice notifiers are tested against all available
> > subdevices as soon as they get registered. It often happens anyway
> > that the subdevice they are connected to is not yet initialized, as
> > it usually gets registered later in drivers' code. This makes debug
> > of v4l2_async particularly painful, as identifying a notifier with
> > an unitialized subdevice is tricky as they don't have a valid
> > 'struct device *' or 'struct fwnode_handle *' to be identified with.
> > 
> > In order to make sure that the notifier's subdevices is initialized
> > when the notifier is tesed against available subdevices post-pone the
> > actual notifier registration at subdevice registration time.
> 
> Aren't you piling yet another hack on top of an already dirty framework ? How 
> about fixing the root cause of the issue and ensuring that subdevs are 
> properly initialized when the notifier is registered ?

The problem domain is quite complex --- there are multiple drivers working
with multiple objects each here, and things can happen in a different order
--- which needs to be supported but is sometimes missed in design.

In this case the problem is that the sub-device is only registered after
the related notifier is. If you did that the other way around, the V4L2
async framework could well find that everything is done and proceed to call
the complete callback, just before the async sub-device notifier is
registered.

Perhaps this is, once again, a sign that we should really ditch the
complete callback. I'd hope we could find consensus on that. It's a lot of
trouble to support this and I feel it's an entirely arfiticial construct
that does not really solve a problem it's intended to.

> 
> > It is worth noting that post-poning registration of a subdevice notifier
> > does not impact on the completion of the notifiers chain, as even if a
> > subdev notifier completes as soon as it gets registered, the complete()
> > call chain cannot be upscaled as long as the subdevice the notifiers
> > belongs to is not registered.
> > 
> > Also, it is now safe to access a notifier 'struct device *' as we're now
> > sure it is properly initialized when the notifier is actually
> > registered.
> > 
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > ---
> >  drivers/media/v4l2-core/v4l2-async.c | 65 +++++++++++++++++++++-----------
> >  include/media/v4l2-async.h           |  2 ++
> >  2 files changed, 43 insertions(+), 24 deletions(-)
> > 
> > diff --git a/drivers/media/v4l2-core/v4l2-async.c
> > b/drivers/media/v4l2-core/v4l2-async.c index 0a1bf1d..c13a781 100644
> > --- a/drivers/media/v4l2-core/v4l2-async.c
> > +++ b/drivers/media/v4l2-core/v4l2-async.c
> 
> [snip]
> 
> > @@ -548,6 +551,20 @@ int v4l2_async_register_subdev(struct v4l2_subdev *sd)
> >  			sd->fwnode = dev_fwnode(sd->dev);
> >  	}
> > 
> > +	/*
> > +	 * If the subdevice has an unregisterd notifier, it's now time
> > +	 * to register it.
> > +	 */
> > +	subdev_notifier = sd->subdev_notifier;
> > +	if (subdev_notifier && !subdev_notifier->registered) {
> > +		ret = __v4l2_async_notifier_register(subdev_notifier);
> > +		if (ret) {
> > +			sd->fwnode = NULL;
> > +			subdev_notifier->owner = NULL;
> > +			return ret;
> > +		}
> > +	}
> 
> This is the part I like the least in this patch set. The 
> v4l2_subdev::subdev_notifier field should really disappear, there's no reason 
> to limit subdevs to a single notifier. Implicit registration of notifiers is a 
> dirty hack in my opinion.
> 
> >  	mutex_lock(&list_lock);
> > 
> >  	INIT_LIST_HEAD(&sd->async_list);
> 
> [snip]
> 
> -- 
> Regards,
> 
> Laurent Pinchart
> 

-- 
Sakari Ailus
e-mail: sakari.ailus@iki.fi

Re: [PATCH 5/5] v4l2: async: Add debug output to v4l2-async module

[Sakari Ailus]

Hi Jacopo,

On Sun, Dec 17, 2017 at 05:42:54PM +0100, jacopo mondi wrote:
> Hi Sakari,
> 
> On Fri, Dec 15, 2017 at 06:17:04PM +0200, Sakari Ailus wrote:
> > Hi Jacopo,
> >
> > On Wed, Dec 13, 2017 at 07:26:20PM +0100, Jacopo Mondi wrote:
> > > The v4l2-async module operations are quite complex to follow, due to the
> > > asynchronous nature of subdevices and notifiers registration and
> > > matching procedures. In order to help with debugging of failed or
> > > erroneous matching between a subdevice and the notifier collected
> > > async_subdevice it gets matched against, introduce a few dev_dbg() calls
> > > in v4l2_async core operations.
> > >
> > > Protect the debug operations with a Kconfig defined symbol, to make sure
> > > when debugging is disabled, no additional code or data is added to the
> > > module.
> > >
> > > Notifiers are identified by the name of the subdevice or v4l2_dev they are
> > > registered by, while subdevice matching which now happens on endpoints,
> > > need a longer description built walking the fwnode graph backwards
> > > collecting parent nodes names (otherwise we would have had printouts
> > > like: "Matching "endpoint" with "endpoint"" which are not that useful).
> > >
> > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > >
> > > ---
> > > For fwnodes backed by OF, I may have used the "%pOF" format modifier to
> > > get the full node name instead of parsing the fwnode graph by myself with
> > > "v4l2_async_fwnode_full_name()". Unfortunately I'm not aware of anything
> > > like "%pOF" for ACPI backed fwnodes. Also, walking the fwnode graph by
> > > myself allows me to reduce the depth, to reduce the debug messages output
> > > length which is anyway long enough to result disturbing on a 80columns
> > > terminal window.
> >
> > ACPI doesn't have such at the moment. I think printing the full path would
> > still be better. There isn't that much more to print after all.
> 
> So you suggest to just use the full node name for OF. What about ACPI?
> 
> From your other reply I got that I can print the single node name for
> "device ACPI nodes" but not for "non-device ACPI nodes". Should I build
> the full device name in drivers/acpi/properties.c for ACPI devices
> like I'm doing here for fwnodes?

What I think would be nice was that ACPI would receive similar way to print
node names (as well as other information) as OF has, through printk.

I don't demand that to get the patchset in though, I'm fine if this is
limited to OF right now. It's debug info, after all that I've at least
personally been fine without.

> 
> >
> > > ---
> > >
> > >  drivers/media/v4l2-core/Kconfig      |  8 ++++
> > >  drivers/media/v4l2-core/v4l2-async.c | 81 ++++++++++++++++++++++++++++++++++++
> > >  2 files changed, 89 insertions(+)
> > >
> > > diff --git a/drivers/media/v4l2-core/Kconfig b/drivers/media/v4l2-core/Kconfig
> > > index a35c336..8331736 100644
> > > --- a/drivers/media/v4l2-core/Kconfig
> > > +++ b/drivers/media/v4l2-core/Kconfig
> > > @@ -17,6 +17,14 @@ config VIDEO_ADV_DEBUG
> > >  	  V4L devices.
> > >  	  In doubt, say N.
> > >
> > > +config VIDEO_V4L2_ASYNC_DEBUG
> > > +	bool "Enable debug functionalities for V4L2 async module"
> > > +	depends on VIDEO_V4L2
> >
> > I'm not sure I'd add a Kconfig option. This is adding a fairly simple
> > function only to the kernel.
> 
> So I will use a symbol defined in the module to enable/disable debug
> (maybe the "DEBUG" symbol itself?)

Dynamic debug can be enabled via the user space interface or the kernel
command line, I think that should be enough.

> 
> >
> > > +	default n
> > > +	---help---
> > > +	  Say Y here to enable debug output in V4L2 async module.
> > > +	  In doubt, say N.
> > > +
> > >  config VIDEO_FIXED_MINOR_RANGES
> > >  	bool "Enable old-style fixed minor ranges on drivers/video devices"
> > >  	default n
> > > diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
> > > index c13a781..307e1a5 100644
> > > --- a/drivers/media/v4l2-core/v4l2-async.c
> > > +++ b/drivers/media/v4l2-core/v4l2-async.c
> > > @@ -8,6 +8,10 @@
> > >   * published by the Free Software Foundation.
> > >   */
> > >
> > > +#if defined(CONFIG_VIDEO_V4L2_ASYNC_DEBUG)
> > > +#define DEBUG
> >
> > Do you need this?
> 
> No dev_dbg() otherwise, isn't it?

Yes. With dynamic debug.

> 
> >
> > > +#endif
> > > +
> > >  #include <linux/device.h>
> > >  #include <linux/err.h>
> > >  #include <linux/i2c.h>
> > > @@ -25,6 +29,52 @@
> > >  #include <media/v4l2-fwnode.h>
> > >  #include <media/v4l2-subdev.h>
> > >
> > > +#if defined(CONFIG_VIDEO_V4L2_ASYNC_DEBUG)
> > > +#define V4L2_ASYNC_FWNODE_NAME_LEN	512
> > > +
> > > +static void __v4l2_async_fwnode_full_name(char *name,
> > > +					  unsigned int len,
> > > +					  unsigned int max_depth,
> > > +					  struct fwnode_handle *fwnode)
> > > +{
> > > +	unsigned int buf_len = len < V4L2_ASYNC_FWNODE_NAME_LEN ?
> > > +			       len : V4L2_ASYNC_FWNODE_NAME_LEN;
> > > +	char __tmp[V4L2_ASYNC_FWNODE_NAME_LEN];
> >
> > That's a bit too much to allocate from the stack I think.
> 
> For an full name do you think 128 is enough? 256 maybe?

I think it'd be nicer if you could print the information without using a
buffer.

-- 
Kind regards,

Sakari Ailus
sakari.ailus@linux.intel.com

Re: [PATCH 4/5] v4l2: async: Postpone subdev_notifier registration

[Laurent Pinchart]

Hi Sakari,

On Monday, 18 December 2017 01:33:56 EET Sakari Ailus wrote:
> On Sun, Dec 17, 2017 at 07:03:17PM +0200, Laurent Pinchart wrote:
> > On Wednesday, 13 December 2017 20:26:19 EET Jacopo Mondi wrote:
> >> Currently, subdevice notifiers are tested against all available
> >> subdevices as soon as they get registered. It often happens anyway
> >> that the subdevice they are connected to is not yet initialized, as
> >> it usually gets registered later in drivers' code. This makes debug
> >> of v4l2_async particularly painful, as identifying a notifier with
> >> an unitialized subdevice is tricky as they don't have a valid
> >> 'struct device *' or 'struct fwnode_handle *' to be identified with.
> >> 
> >> In order to make sure that the notifier's subdevices is initialized
> >> when the notifier is tesed against available subdevices post-pone the
> >> actual notifier registration at subdevice registration time.
> > 
> > Aren't you piling yet another hack on top of an already dirty framework ?
> > How about fixing the root cause of the issue and ensuring that subdevs
> > are properly initialized when the notifier is registered ?
> 
> The problem domain is quite complex --- there are multiple drivers working
> with multiple objects each here, and things can happen in a different order
> --- which needs to be supported but is sometimes missed in design.
> 
> In this case the problem is that the sub-device is only registered after
> the related notifier is. If you did that the other way around, the V4L2
> async framework could well find that everything is done and proceed to call
> the complete callback, just before the async sub-device notifier is
> registered.

Sure, I understand that, but can't we guarantee that we initialize enough of 
the v4l2_subdev structure before registering the notifier while keeping the 
same order of notifier and subdev registration ?

> Perhaps this is, once again, a sign that we should really ditch the
> complete callback. I'd hope we could find consensus on that. It's a lot of
> trouble to support this and I feel it's an entirely arfiticial construct
> that does not really solve a problem it's intended to.

I agree. It's at least time to refactor the API, as it has grown into a 
complex piece of code with an intricate and difficult to follow execution 
path, without in my opinion any clear benefit of such an approach.

> >> It is worth noting that post-poning registration of a subdevice notifier
> >> does not impact on the completion of the notifiers chain, as even if a
> >> subdev notifier completes as soon as it gets registered, the complete()
> >> call chain cannot be upscaled as long as the subdevice the notifiers
> >> belongs to is not registered.
> >> 
> >> Also, it is now safe to access a notifier 'struct device *' as we're now
> >> sure it is properly initialized when the notifier is actually
> >> registered.
> >> 
> >> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> >> ---
> >> 
> >>  drivers/media/v4l2-core/v4l2-async.c | 65 +++++++++++++++++++-----------
> >>  include/media/v4l2-async.h           |  2 ++
> >>  2 files changed, 43 insertions(+), 24 deletions(-)
> >> 
> >> diff --git a/drivers/media/v4l2-core/v4l2-async.c
> >> b/drivers/media/v4l2-core/v4l2-async.c index 0a1bf1d..c13a781 100644
> >> --- a/drivers/media/v4l2-core/v4l2-async.c
> >> +++ b/drivers/media/v4l2-core/v4l2-async.c
> > 
> > [snip]
> > 
> >> @@ -548,6 +551,20 @@ int v4l2_async_register_subdev(struct v4l2_subdev
> >> *sd)
> >>  			sd->fwnode = dev_fwnode(sd->dev);
> >>  	}
> >> 
> >> +	/*
> >> +	 * If the subdevice has an unregisterd notifier, it's now time
> >> +	 * to register it.
> >> +	 */
> >> +	subdev_notifier = sd->subdev_notifier;
> >> +	if (subdev_notifier && !subdev_notifier->registered) {
> >> +		ret = __v4l2_async_notifier_register(subdev_notifier);
> >> +		if (ret) {
> >> +			sd->fwnode = NULL;
> >> +			subdev_notifier->owner = NULL;
> >> +			return ret;
> >> +		}
> >> +	}
> > 
> > This is the part I like the least in this patch set. The
> > v4l2_subdev::subdev_notifier field should really disappear, there's no
> > reason to limit subdevs to a single notifier. Implicit registration of
> > notifiers is a dirty hack in my opinion.
> > 
> >>  	mutex_lock(&list_lock);
> >>  	
> >>  	INIT_LIST_HEAD(&sd->async_list);
> > 
> > [snip]

-- 
Regards,

Laurent Pinchart