[PATCH 0/5] media: Fix asd dynamic allocation

[PATCH 0/5] media: Fix asd dynamic allocation

[Laurent Pinchart]

Hello,

This small patch series addresses a bug related to the usage of
v4l2_async_notifier_add_subdev() that is widespread among drivers.

The issue is explained in 1/4, which improves the documentation of the
v4l2_async_notifier_add_subdev() function by stating explicitly that the
asd argument needs to be allocated dynamically. Among the 13 drivers
that use that function, only one gets it right.

The rest of the patches fix the problem in several Renesas-related
drivers, with an unrelated fwnode reference leak fix for the rcar-drif
driver in 2/5 that made the v4l2_async_notifier_add_subdev() fix easier
to implement in that driver.

I'm lacking hardware to test 2/5 and 3/5. Ramesh, would you be able to
test that ? What development board do you use to test the DRIF driver ?
I don't see any DT integration upstream.

I also haven't dug up my MAX9286 development kit to test 5/5. I would
thus appreciate if someone could test it, but worst case I can do so
myself.

Laurent Pinchart (5):
  media: v4l2-async: Document asd allocation requirements
  media: rcar_drif: Fix fwnode reference leak when parsing DT
  media: rcar_drif: Allocate v4l2_async_subdev dynamically
  media: rcar-csi2: Allocate v4l2_async_subdev dynamically
  media: i2c: max9286: Allocate v4l2_async_subdev dynamically

 drivers/media/i2c/max9286.c                 | 38 +++++++++++----------
 drivers/media/platform/rcar-vin/rcar-csi2.c | 24 ++++++-------
 drivers/media/platform/rcar_drif.c          | 30 +++++-----------
 include/media/v4l2-async.h                  |  5 +--
 4 files changed, 42 insertions(+), 55 deletions(-)

-- 
Regards,

Laurent Pinchart

[PATCH 1/5] media: v4l2-async: Document asd allocation requirements

[Laurent Pinchart]

The v4l2_async_notifier_add_subdev() function requires the asd pointer
it receives to be allocated dynamically, but doesn't explicitly say so.
Only one driver out of 13 get its right (atmel-sama5d2-isc.c, but with
memory leaks in the error paths), clearly showing we have an issue.

Update the v4l2_async_notifier_add_subdev() documentation to clearly
state the allocation requirement. Whether this will be enough to avoid
new offending code isn't certain, but it's a good first step
nonetheless.

Fixes: 9ca465312132 ("media: v4l: fwnode: Support generic parsing of graph endpoints in a device")
Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
 include/media/v4l2-async.h | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/include/media/v4l2-async.h b/include/media/v4l2-async.h
index 8319284c93cb..d6e31234826f 100644
--- a/include/media/v4l2-async.h
+++ b/include/media/v4l2-async.h
@@ -154,8 +154,9 @@ void v4l2_async_notifier_init(struct v4l2_async_notifier *notifier);
  * @notifier: pointer to &struct v4l2_async_notifier
  * @asd: pointer to &struct v4l2_async_subdev
  *
- * Call this function before registering a notifier to link the
- * provided asd to the notifiers master @asd_list.
+ * Call this function before registering a notifier to link the provided @asd to
+ * the notifiers master @asd_list. The @asd must be allocated with k*alloc() as
+ * it will be freed by the framework when the notifier is destroyed.
  */
 int v4l2_async_notifier_add_subdev(struct v4l2_async_notifier *notifier,
 				   struct v4l2_async_subdev *asd);
-- 
Regards,

Laurent Pinchart

[PATCH 2/5] media: rcar_drif: Fix fwnode reference leak when parsing DT

[Laurent Pinchart]

The fwnode reference corresponding to the endpoint is leaked in an error
path of the rcar_drif_parse_subdevs() function. Fix it, and reorganize
fwnode reference handling in the function to release references early,
simplifying error paths.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
 drivers/media/platform/rcar_drif.c | 16 +++++-----------
 1 file changed, 5 insertions(+), 11 deletions(-)

diff --git a/drivers/media/platform/rcar_drif.c b/drivers/media/platform/rcar_drif.c
index 3d2451ac347d..3f1e5cb8b197 100644
--- a/drivers/media/platform/rcar_drif.c
+++ b/drivers/media/platform/rcar_drif.c
@@ -1227,28 +1227,22 @@ static int rcar_drif_parse_subdevs(struct rcar_drif_sdr *sdr)
 	if (!ep)
 		return 0;
 
+	/* Get the endpoint properties */
+	rcar_drif_get_ep_properties(sdr, ep);
+
 	fwnode = fwnode_graph_get_remote_port_parent(ep);
+	fwnode_handle_put(ep);
 	if (!fwnode) {
 		dev_warn(sdr->dev, "bad remote port parent\n");
-		fwnode_handle_put(ep);
 		return -EINVAL;
 	}
 
 	sdr->ep.asd.match.fwnode = fwnode;
 	sdr->ep.asd.match_type = V4L2_ASYNC_MATCH_FWNODE;
 	ret = v4l2_async_notifier_add_subdev(notifier, &sdr->ep.asd);
-	if (ret) {
-		fwnode_handle_put(fwnode);
-		return ret;
-	}
-
-	/* Get the endpoint properties */
-	rcar_drif_get_ep_properties(sdr, ep);
-
 	fwnode_handle_put(fwnode);
-	fwnode_handle_put(ep);
 
-	return 0;
+	return ret;
 }
 
 /* Check if the given device is the primary bond */
-- 
Regards,

Laurent Pinchart

[PATCH 3/5] media: rcar_drif: Allocate v4l2_async_subdev dynamically

[Laurent Pinchart]

v4l2_async_notifier_add_subdev() requires the asd to be allocated
dynamically, but the rcar-drif driver embeds it in the
rcar_drif_graph_ep structure. This causes memory corruption when the
notifier is destroyed at remove time with v4l2_async_notifier_cleanup().

Fix this issue by registering the asd with
v4l2_async_notifier_add_fwnode_subdev(), which allocates it dynamically
internally.

Fixes: d079f94c9046 ("media: platform: Switch to v4l2_async_notifier_add_subdev")
Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
 drivers/media/platform/rcar_drif.c | 18 ++++++------------
 1 file changed, 6 insertions(+), 12 deletions(-)

diff --git a/drivers/media/platform/rcar_drif.c b/drivers/media/platform/rcar_drif.c
index 3f1e5cb8b197..f318cd4b8086 100644
--- a/drivers/media/platform/rcar_drif.c
+++ b/drivers/media/platform/rcar_drif.c
@@ -185,7 +185,6 @@ struct rcar_drif_frame_buf {
 /* OF graph endpoint's V4L2 async data */
 struct rcar_drif_graph_ep {
 	struct v4l2_subdev *subdev;	/* Async matched subdev */
-	struct v4l2_async_subdev asd;	/* Async sub-device descriptor */
 };
 
 /* DMA buffer */
@@ -1109,12 +1108,6 @@ static int rcar_drif_notify_bound(struct v4l2_async_notifier *notifier,
 	struct rcar_drif_sdr *sdr =
 		container_of(notifier, struct rcar_drif_sdr, notifier);
 
-	if (sdr->ep.asd.match.fwnode !=
-	    of_fwnode_handle(subdev->dev->of_node)) {
-		rdrif_err(sdr, "subdev %s cannot bind\n", subdev->name);
-		return -EINVAL;
-	}
-
 	v4l2_set_subdev_hostdata(subdev, sdr);
 	sdr->ep.subdev = subdev;
 	rdrif_dbg(sdr, "bound asd %s\n", subdev->name);
@@ -1218,7 +1211,7 @@ static int rcar_drif_parse_subdevs(struct rcar_drif_sdr *sdr)
 {
 	struct v4l2_async_notifier *notifier = &sdr->notifier;
 	struct fwnode_handle *fwnode, *ep;
-	int ret;
+	struct v4l2_async_subdev *asd;
 
 	v4l2_async_notifier_init(notifier);
 
@@ -1237,12 +1230,13 @@ static int rcar_drif_parse_subdevs(struct rcar_drif_sdr *sdr)
 		return -EINVAL;
 	}
 
-	sdr->ep.asd.match.fwnode = fwnode;
-	sdr->ep.asd.match_type = V4L2_ASYNC_MATCH_FWNODE;
-	ret = v4l2_async_notifier_add_subdev(notifier, &sdr->ep.asd);
+	asd = v4l2_async_notifier_add_fwnode_subdev(notifier, fwnode,
+						    sizeof(*asd));
 	fwnode_handle_put(fwnode);
+	if (IS_ERR(asd))
+		return PTR_ERR(asd);
 
-	return ret;
+	return 0;
 }
 
 /* Check if the given device is the primary bond */
-- 
Regards,

Laurent Pinchart

[PATCH 4/5] media: rcar-csi2: Allocate v4l2_async_subdev dynamically

[Laurent Pinchart]

v4l2_async_notifier_add_subdev() requires the asd to be allocated
dynamically, but the rcar-csi2 driver embeds it in the rcar_csi2
structure. This causes memory corruption when the notifier is destroyed
at remove time with v4l2_async_notifier_cleanup().

Fix this issue by registering the asd with
v4l2_async_notifier_add_fwnode_subdev(), which allocates it dynamically
internally.

Fixes: 769afd212b16 ("media: rcar-csi2: add Renesas R-Car MIPI CSI-2 receiver driver")
Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
 drivers/media/platform/rcar-vin/rcar-csi2.c | 24 +++++++++------------
 1 file changed, 10 insertions(+), 14 deletions(-)

diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c
index c6cc4f473a07..a16c492b3143 100644
--- a/drivers/media/platform/rcar-vin/rcar-csi2.c
+++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
@@ -362,7 +362,6 @@ struct rcar_csi2 {
 	struct media_pad pads[NR_OF_RCAR_CSI2_PAD];
 
 	struct v4l2_async_notifier notifier;
-	struct v4l2_async_subdev asd;
 	struct v4l2_subdev *remote;
 
 	struct v4l2_mbus_framefmt mf;
@@ -811,6 +810,8 @@ static int rcsi2_parse_v4l2(struct rcar_csi2 *priv,
 
 static int rcsi2_parse_dt(struct rcar_csi2 *priv)
 {
+	struct v4l2_async_subdev *asd;
+	struct fwnode_handle *fwnode;
 	struct device_node *ep;
 	struct v4l2_fwnode_endpoint v4l2_ep = { .bus_type = 0 };
 	int ret;
@@ -834,24 +835,19 @@ static int rcsi2_parse_dt(struct rcar_csi2 *priv)
 		return ret;
 	}
 
-	priv->asd.match.fwnode =
-		fwnode_graph_get_remote_endpoint(of_fwnode_handle(ep));
-	priv->asd.match_type = V4L2_ASYNC_MATCH_FWNODE;
-
+	fwnode = fwnode_graph_get_remote_endpoint(of_fwnode_handle(ep));
 	of_node_put(ep);
 
+	dev_dbg(priv->dev, "Found '%pOF'\n", to_of_node(fwnode));
+
 	v4l2_async_notifier_init(&priv->notifier);
-
-	ret = v4l2_async_notifier_add_subdev(&priv->notifier, &priv->asd);
-	if (ret) {
-		fwnode_handle_put(priv->asd.match.fwnode);
-		return ret;
-	}
-
 	priv->notifier.ops = &rcar_csi2_notify_ops;
 
-	dev_dbg(priv->dev, "Found '%pOF'\n",
-		to_of_node(priv->asd.match.fwnode));
+	asd = v4l2_async_notifier_add_fwnode_subdev(&priv->notifier, fwnode,
+						    sizeof(*asd));
+	fwnode_handle_put(fwnode);
+	if (IS_ERR(asd))
+		return PTR_ERR(asd);
 
 	ret = v4l2_async_subdev_notifier_register(&priv->subdev,
 						  &priv->notifier);
-- 
Regards,

Laurent Pinchart

[PATCH 5/5] media: i2c: max9286: Allocate v4l2_async_subdev dynamically

[Laurent Pinchart]

v4l2_async_notifier_add_subdev() requires the asd to be allocated
dynamically, but the max9286 driver embeds it in the max9286_source
structure. This causes memory corruption when the notifier is destroyed
at remove time with v4l2_async_notifier_cleanup().

Fix this issue by registering the asd with
v4l2_async_notifier_add_fwnode_subdev(), which allocates it dynamically
internally. A new max9286_asd structure is introduced, to store a
pointer to the corresonding max9286_source that needs to be accessed
from bound and unbind callbacks. There's no need to take an extra
explicit reference to the fwnode anymore as
v4l2_async_notifier_add_fwnode_subdev() does so internally.

While at it, use %u instead of %d to print the unsigned index in the
error message from the v4l2_async_notifier_add_fwnode_subdev() error
path.

Fixes: 66d8c9d2422d ("media: i2c: Add MAX9286 driver")
Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
 drivers/media/i2c/max9286.c | 38 +++++++++++++++++++------------------
 1 file changed, 20 insertions(+), 18 deletions(-)

diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
index 47f280518fdb..5d890dddb376 100644
--- a/drivers/media/i2c/max9286.c
+++ b/drivers/media/i2c/max9286.c
@@ -135,13 +135,19 @@
 #define MAX9286_SRC_PAD			4
 
 struct max9286_source {
-	struct v4l2_async_subdev asd;
 	struct v4l2_subdev *sd;
 	struct fwnode_handle *fwnode;
 };
 
-#define asd_to_max9286_source(_asd) \
-	container_of(_asd, struct max9286_source, asd)
+struct max9286_asd {
+	struct v4l2_async_subdev base;
+	struct max9286_source *source;
+};
+
+static inline struct max9286_asd *to_max9286_asd(struct v4l2_async_subdev *asd)
+{
+	return container_of(asd, struct max9286_asd, base);
+}
 
 struct max9286_priv {
 	struct i2c_client *client;
@@ -480,7 +486,7 @@ static int max9286_notify_bound(struct v4l2_async_notifier *notifier,
 				struct v4l2_async_subdev *asd)
 {
 	struct max9286_priv *priv = sd_to_max9286(notifier->sd);
-	struct max9286_source *source = asd_to_max9286_source(asd);
+	struct max9286_source *source = to_max9286_asd(asd)->source;
 	unsigned int index = to_index(priv, source);
 	unsigned int src_pad;
 	int ret;
@@ -544,7 +550,7 @@ static void max9286_notify_unbind(struct v4l2_async_notifier *notifier,
 				  struct v4l2_async_subdev *asd)
 {
 	struct max9286_priv *priv = sd_to_max9286(notifier->sd);
-	struct max9286_source *source = asd_to_max9286_source(asd);
+	struct max9286_source *source = to_max9286_asd(asd)->source;
 	unsigned int index = to_index(priv, source);
 
 	source->sd = NULL;
@@ -569,23 +575,19 @@ static int max9286_v4l2_notifier_register(struct max9286_priv *priv)
 
 	for_each_source(priv, source) {
 		unsigned int i = to_index(priv, source);
+		struct v4l2_async_subdev *asd;
 
-		source->asd.match_type = V4L2_ASYNC_MATCH_FWNODE;
-		source->asd.match.fwnode = source->fwnode;
-
-		ret = v4l2_async_notifier_add_subdev(&priv->notifier,
-						     &source->asd);
-		if (ret) {
-			dev_err(dev, "Failed to add subdev for source %d", i);
+		asd = v4l2_async_notifier_add_fwnode_subdev(&priv->notifier,
+							    source->fwnode,
+							    sizeof(*asd));
+		if (IS_ERR(asd)) {
+			dev_err(dev, "Failed to add subdev for source %u: %ld",
+				i, PTR_ERR(asd));
 			v4l2_async_notifier_cleanup(&priv->notifier);
-			return ret;
+			return PTR_ERR(asd);
 		}
 
-		/*
-		 * Balance the reference counting handled through
-		 * v4l2_async_notifier_cleanup()
-		 */
-		fwnode_handle_get(source->fwnode);
+		to_max9286_asd(asd)->source = source;
 	}
 
 	priv->notifier.ops = &max9286_notify_ops;
-- 
Regards,

Laurent Pinchart

Re: [PATCH 4/5] media: rcar-csi2: Allocate v4l2_async_subdev dynamically

[Niklas Söderlund]

Hi Laurent,

Thanks for your work.

On 2020-08-11 23:59:38 +0300, Laurent Pinchart wrote:
> v4l2_async_notifier_add_subdev() requires the asd to be allocated
> dynamically, but the rcar-csi2 driver embeds it in the rcar_csi2
> structure. This causes memory corruption when the notifier is destroyed
> at remove time with v4l2_async_notifier_cleanup().
> 
> Fix this issue by registering the asd with
> v4l2_async_notifier_add_fwnode_subdev(), which allocates it dynamically
> internally.

This patch conflicts with [1] which I think is a nicer solution to the 
problem, provided 1/2 of that series is palatable for everyone :-)

1. [PATCH 2/2] rcar-csi2: Use V4L2 async helpers to create the notifier

> 
> Fixes: 769afd212b16 ("media: rcar-csi2: add Renesas R-Car MIPI CSI-2 receiver driver")
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> ---
>  drivers/media/platform/rcar-vin/rcar-csi2.c | 24 +++++++++------------
>  1 file changed, 10 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c
> index c6cc4f473a07..a16c492b3143 100644
> --- a/drivers/media/platform/rcar-vin/rcar-csi2.c
> +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
> @@ -362,7 +362,6 @@ struct rcar_csi2 {
>  	struct media_pad pads[NR_OF_RCAR_CSI2_PAD];
>  
>  	struct v4l2_async_notifier notifier;
> -	struct v4l2_async_subdev asd;
>  	struct v4l2_subdev *remote;
>  
>  	struct v4l2_mbus_framefmt mf;
> @@ -811,6 +810,8 @@ static int rcsi2_parse_v4l2(struct rcar_csi2 *priv,
>  
>  static int rcsi2_parse_dt(struct rcar_csi2 *priv)
>  {
> +	struct v4l2_async_subdev *asd;
> +	struct fwnode_handle *fwnode;
>  	struct device_node *ep;
>  	struct v4l2_fwnode_endpoint v4l2_ep = { .bus_type = 0 };
>  	int ret;
> @@ -834,24 +835,19 @@ static int rcsi2_parse_dt(struct rcar_csi2 *priv)
>  		return ret;
>  	}
>  
> -	priv->asd.match.fwnode =
> -		fwnode_graph_get_remote_endpoint(of_fwnode_handle(ep));
> -	priv->asd.match_type = V4L2_ASYNC_MATCH_FWNODE;
> -
> +	fwnode = fwnode_graph_get_remote_endpoint(of_fwnode_handle(ep));
>  	of_node_put(ep);
>  
> +	dev_dbg(priv->dev, "Found '%pOF'\n", to_of_node(fwnode));
> +
>  	v4l2_async_notifier_init(&priv->notifier);
> -
> -	ret = v4l2_async_notifier_add_subdev(&priv->notifier, &priv->asd);
> -	if (ret) {
> -		fwnode_handle_put(priv->asd.match.fwnode);
> -		return ret;
> -	}
> -
>  	priv->notifier.ops = &rcar_csi2_notify_ops;
>  
> -	dev_dbg(priv->dev, "Found '%pOF'\n",
> -		to_of_node(priv->asd.match.fwnode));
> +	asd = v4l2_async_notifier_add_fwnode_subdev(&priv->notifier, fwnode,
> +						    sizeof(*asd));
> +	fwnode_handle_put(fwnode);
> +	if (IS_ERR(asd))
> +		return PTR_ERR(asd);
>  
>  	ret = v4l2_async_subdev_notifier_register(&priv->subdev,
>  						  &priv->notifier);
> -- 
> Regards,
> 
> Laurent Pinchart
> 

-- 
Regards,
Niklas Söderlund

Re: [PATCH 4/5] media: rcar-csi2: Allocate v4l2_async_subdev dynamically

[Laurent Pinchart]

Hi Niklas,

On Tue, Aug 11, 2020 at 11:43:24PM +0200, Niklas Söderlund wrote:
> On 2020-08-11 23:59:38 +0300, Laurent Pinchart wrote:
> > v4l2_async_notifier_add_subdev() requires the asd to be allocated
> > dynamically, but the rcar-csi2 driver embeds it in the rcar_csi2
> > structure. This causes memory corruption when the notifier is destroyed
> > at remove time with v4l2_async_notifier_cleanup().
> > 
> > Fix this issue by registering the asd with
> > v4l2_async_notifier_add_fwnode_subdev(), which allocates it dynamically
> > internally.
> 
> This patch conflicts with [1] which I think is a nicer solution to the 
> problem, provided 1/2 of that series is palatable for everyone :-)
> 
> 1. [PATCH 2/2] rcar-csi2: Use V4L2 async helpers to create the notifier

That looks better to me too.

> > Fixes: 769afd212b16 ("media: rcar-csi2: add Renesas R-Car MIPI CSI-2 receiver driver")
> > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > ---
> >  drivers/media/platform/rcar-vin/rcar-csi2.c | 24 +++++++++------------
> >  1 file changed, 10 insertions(+), 14 deletions(-)
> > 
> > diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c
> > index c6cc4f473a07..a16c492b3143 100644
> > --- a/drivers/media/platform/rcar-vin/rcar-csi2.c
> > +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
> > @@ -362,7 +362,6 @@ struct rcar_csi2 {
> >  	struct media_pad pads[NR_OF_RCAR_CSI2_PAD];
> >  
> >  	struct v4l2_async_notifier notifier;
> > -	struct v4l2_async_subdev asd;
> >  	struct v4l2_subdev *remote;
> >  
> >  	struct v4l2_mbus_framefmt mf;
> > @@ -811,6 +810,8 @@ static int rcsi2_parse_v4l2(struct rcar_csi2 *priv,
> >  
> >  static int rcsi2_parse_dt(struct rcar_csi2 *priv)
> >  {
> > +	struct v4l2_async_subdev *asd;
> > +	struct fwnode_handle *fwnode;
> >  	struct device_node *ep;
> >  	struct v4l2_fwnode_endpoint v4l2_ep = { .bus_type = 0 };
> >  	int ret;
> > @@ -834,24 +835,19 @@ static int rcsi2_parse_dt(struct rcar_csi2 *priv)
> >  		return ret;
> >  	}
> >  
> > -	priv->asd.match.fwnode =
> > -		fwnode_graph_get_remote_endpoint(of_fwnode_handle(ep));
> > -	priv->asd.match_type = V4L2_ASYNC_MATCH_FWNODE;
> > -
> > +	fwnode = fwnode_graph_get_remote_endpoint(of_fwnode_handle(ep));
> >  	of_node_put(ep);
> >  
> > +	dev_dbg(priv->dev, "Found '%pOF'\n", to_of_node(fwnode));
> > +
> >  	v4l2_async_notifier_init(&priv->notifier);
> > -
> > -	ret = v4l2_async_notifier_add_subdev(&priv->notifier, &priv->asd);
> > -	if (ret) {
> > -		fwnode_handle_put(priv->asd.match.fwnode);
> > -		return ret;
> > -	}
> > -
> >  	priv->notifier.ops = &rcar_csi2_notify_ops;
> >  
> > -	dev_dbg(priv->dev, "Found '%pOF'\n",
> > -		to_of_node(priv->asd.match.fwnode));
> > +	asd = v4l2_async_notifier_add_fwnode_subdev(&priv->notifier, fwnode,
> > +						    sizeof(*asd));
> > +	fwnode_handle_put(fwnode);
> > +	if (IS_ERR(asd))
> > +		return PTR_ERR(asd);
> >  
> >  	ret = v4l2_async_subdev_notifier_register(&priv->subdev,
> >  						  &priv->notifier);

-- 
Regards,

Laurent Pinchart

Re: [PATCH 0/5] media: Fix asd dynamic allocation

[Ramesh Shanmugasundaram]

Hi Laurent,

Thank you for the fix. Sorry about the late reply.

> This small patch series addresses a bug related to the usage of
> v4l2_async_notifier_add_subdev() that is widespread among drivers.
>
> The issue is explained in 1/4, which improves the documentation of the
> v4l2_async_notifier_add_subdev() function by stating explicitly that the
> asd argument needs to be allocated dynamically. Among the 13 drivers
> that use that function, only one gets it right.
>
> The rest of the patches fix the problem in several Renesas-related
> drivers, with an unrelated fwnode reference leak fix for the rcar-drif
> driver in 2/5 that made the v4l2_async_notifier_add_subdev() fix easier
> to implement in that driver.
>
> I'm lacking hardware to test 2/5 and 3/5. Ramesh, would you be able to
> test that ? What development board do you use to test the DRIF driver ?
> I don't see any DT integration upstream.

I'm afraid I may not be able to test it as I do not have the hardware.
I have copied Chris P if he can help out.

I have tested DRIF driver on a H3/M3 Salvator-X/S with a add-on board
from Maxim having two MAX2175 tuners (MAX2175 evaluation kit). This
add-on board was a prototype at that time. Hence, the DT updates were
maintained internally.

Thanks,
Ramesh

> I also haven't dug up my MAX9286 development kit to test 5/5. I would
> thus appreciate if someone could test it, but worst case I can do so
> myself.
>
> Laurent Pinchart (5):
>   media: v4l2-async: Document asd allocation requirements
>   media: rcar_drif: Fix fwnode reference leak when parsing DT
>   media: rcar_drif: Allocate v4l2_async_subdev dynamically
>   media: rcar-csi2: Allocate v4l2_async_subdev dynamically
>   media: i2c: max9286: Allocate v4l2_async_subdev dynamically
>
>  drivers/media/i2c/max9286.c                 | 38 +++++++++++----------
>  drivers/media/platform/rcar-vin/rcar-csi2.c | 24 ++++++-------
>  drivers/media/platform/rcar_drif.c          | 30 +++++-----------
>  include/media/v4l2-async.h                  |  5 +--
>  4 files changed, 42 insertions(+), 55 deletions(-)
>
> --
> Regards,
>
> Laurent Pinchart
>

RE: [PATCH 0/5] media: Fix asd dynamic allocation

[Chris Paterson]

Hello,

> From: Ramesh Shanmugasundaram <rashanmu@gmail.com>
> Sent: 31 August 2020 20:45
> 
> Hi Laurent,
> 
> Thank you for the fix. Sorry about the late reply.
> 
> > This small patch series addresses a bug related to the usage of
> > v4l2_async_notifier_add_subdev() that is widespread among drivers.
> >
> > The issue is explained in 1/4, which improves the documentation of the
> > v4l2_async_notifier_add_subdev() function by stating explicitly that the
> > asd argument needs to be allocated dynamically. Among the 13 drivers
> > that use that function, only one gets it right.
> >
> > The rest of the patches fix the problem in several Renesas-related
> > drivers, with an unrelated fwnode reference leak fix for the rcar-drif
> > driver in 2/5 that made the v4l2_async_notifier_add_subdev() fix easier
> > to implement in that driver.
> >
> > I'm lacking hardware to test 2/5 and 3/5. Ramesh, would you be able to
> > test that ? What development board do you use to test the DRIF driver ?
> > I don't see any DT integration upstream.
> 
> I'm afraid I may not be able to test it as I do not have the hardware.
> I have copied Chris P if he can help out.

Thanks Ramesh.

Fabrizio will take a look as he's working on DRIF currently.
We'll also get him added to the maintainers file.

Kind regards, Chris

> 
> I have tested DRIF driver on a H3/M3 Salvator-X/S with a add-on board
> from Maxim having two MAX2175 tuners (MAX2175 evaluation kit). This
> add-on board was a prototype at that time. Hence, the DT updates were
> maintained internally.
> 
> Thanks,
> Ramesh
> 
> > I also haven't dug up my MAX9286 development kit to test 5/5. I would
> > thus appreciate if someone could test it, but worst case I can do so
> > myself.
> >
> > Laurent Pinchart (5):
> >   media: v4l2-async: Document asd allocation requirements
> >   media: rcar_drif: Fix fwnode reference leak when parsing DT
> >   media: rcar_drif: Allocate v4l2_async_subdev dynamically
> >   media: rcar-csi2: Allocate v4l2_async_subdev dynamically
> >   media: i2c: max9286: Allocate v4l2_async_subdev dynamically
> >
> >  drivers/media/i2c/max9286.c                 | 38 +++++++++++----------
> >  drivers/media/platform/rcar-vin/rcar-csi2.c | 24 ++++++-------
> >  drivers/media/platform/rcar_drif.c          | 30 +++++-----------
> >  include/media/v4l2-async.h                  |  5 +--
> >  4 files changed, 42 insertions(+), 55 deletions(-)
> >
> > --
> > Regards,
> >
> > Laurent Pinchart
> >

Re: [PATCH 1/5] media: v4l2-async: Document asd allocation requirements

[Laurent Pinchart]

Gentle ping for reviews.

On Tue, Aug 11, 2020 at 11:59:35PM +0300, Laurent Pinchart wrote:
> The v4l2_async_notifier_add_subdev() function requires the asd pointer
> it receives to be allocated dynamically, but doesn't explicitly say so.
> Only one driver out of 13 get its right (atmel-sama5d2-isc.c, but with
> memory leaks in the error paths), clearly showing we have an issue.
> 
> Update the v4l2_async_notifier_add_subdev() documentation to clearly
> state the allocation requirement. Whether this will be enough to avoid
> new offending code isn't certain, but it's a good first step
> nonetheless.
> 
> Fixes: 9ca465312132 ("media: v4l: fwnode: Support generic parsing of graph endpoints in a device")
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> ---
>  include/media/v4l2-async.h | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/include/media/v4l2-async.h b/include/media/v4l2-async.h
> index 8319284c93cb..d6e31234826f 100644
> --- a/include/media/v4l2-async.h
> +++ b/include/media/v4l2-async.h
> @@ -154,8 +154,9 @@ void v4l2_async_notifier_init(struct v4l2_async_notifier *notifier);
>   * @notifier: pointer to &struct v4l2_async_notifier
>   * @asd: pointer to &struct v4l2_async_subdev
>   *
> - * Call this function before registering a notifier to link the
> - * provided asd to the notifiers master @asd_list.
> + * Call this function before registering a notifier to link the provided @asd to
> + * the notifiers master @asd_list. The @asd must be allocated with k*alloc() as
> + * it will be freed by the framework when the notifier is destroyed.
>   */
>  int v4l2_async_notifier_add_subdev(struct v4l2_async_notifier *notifier,
>  				   struct v4l2_async_subdev *asd);

-- 
Regards,

Laurent Pinchart

Re: [PATCH 5/5] media: i2c: max9286: Allocate v4l2_async_subdev dynamically

[Laurent Pinchart]

Gentle ping for reviews.

On Tue, Aug 11, 2020 at 11:59:39PM +0300, Laurent Pinchart wrote:
> v4l2_async_notifier_add_subdev() requires the asd to be allocated
> dynamically, but the max9286 driver embeds it in the max9286_source
> structure. This causes memory corruption when the notifier is destroyed
> at remove time with v4l2_async_notifier_cleanup().
> 
> Fix this issue by registering the asd with
> v4l2_async_notifier_add_fwnode_subdev(), which allocates it dynamically
> internally. A new max9286_asd structure is introduced, to store a
> pointer to the corresonding max9286_source that needs to be accessed
> from bound and unbind callbacks. There's no need to take an extra
> explicit reference to the fwnode anymore as
> v4l2_async_notifier_add_fwnode_subdev() does so internally.
> 
> While at it, use %u instead of %d to print the unsigned index in the
> error message from the v4l2_async_notifier_add_fwnode_subdev() error
> path.
> 
> Fixes: 66d8c9d2422d ("media: i2c: Add MAX9286 driver")
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> ---
>  drivers/media/i2c/max9286.c | 38 +++++++++++++++++++------------------
>  1 file changed, 20 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> index 47f280518fdb..5d890dddb376 100644
> --- a/drivers/media/i2c/max9286.c
> +++ b/drivers/media/i2c/max9286.c
> @@ -135,13 +135,19 @@
>  #define MAX9286_SRC_PAD			4
>  
>  struct max9286_source {
> -	struct v4l2_async_subdev asd;
>  	struct v4l2_subdev *sd;
>  	struct fwnode_handle *fwnode;
>  };
>  
> -#define asd_to_max9286_source(_asd) \
> -	container_of(_asd, struct max9286_source, asd)
> +struct max9286_asd {
> +	struct v4l2_async_subdev base;
> +	struct max9286_source *source;
> +};
> +
> +static inline struct max9286_asd *to_max9286_asd(struct v4l2_async_subdev *asd)
> +{
> +	return container_of(asd, struct max9286_asd, base);
> +}
>  
>  struct max9286_priv {
>  	struct i2c_client *client;
> @@ -480,7 +486,7 @@ static int max9286_notify_bound(struct v4l2_async_notifier *notifier,
>  				struct v4l2_async_subdev *asd)
>  {
>  	struct max9286_priv *priv = sd_to_max9286(notifier->sd);
> -	struct max9286_source *source = asd_to_max9286_source(asd);
> +	struct max9286_source *source = to_max9286_asd(asd)->source;
>  	unsigned int index = to_index(priv, source);
>  	unsigned int src_pad;
>  	int ret;
> @@ -544,7 +550,7 @@ static void max9286_notify_unbind(struct v4l2_async_notifier *notifier,
>  				  struct v4l2_async_subdev *asd)
>  {
>  	struct max9286_priv *priv = sd_to_max9286(notifier->sd);
> -	struct max9286_source *source = asd_to_max9286_source(asd);
> +	struct max9286_source *source = to_max9286_asd(asd)->source;
>  	unsigned int index = to_index(priv, source);
>  
>  	source->sd = NULL;
> @@ -569,23 +575,19 @@ static int max9286_v4l2_notifier_register(struct max9286_priv *priv)
>  
>  	for_each_source(priv, source) {
>  		unsigned int i = to_index(priv, source);
> +		struct v4l2_async_subdev *asd;
>  
> -		source->asd.match_type = V4L2_ASYNC_MATCH_FWNODE;
> -		source->asd.match.fwnode = source->fwnode;
> -
> -		ret = v4l2_async_notifier_add_subdev(&priv->notifier,
> -						     &source->asd);
> -		if (ret) {
> -			dev_err(dev, "Failed to add subdev for source %d", i);
> +		asd = v4l2_async_notifier_add_fwnode_subdev(&priv->notifier,
> +							    source->fwnode,
> +							    sizeof(*asd));
> +		if (IS_ERR(asd)) {
> +			dev_err(dev, "Failed to add subdev for source %u: %ld",
> +				i, PTR_ERR(asd));
>  			v4l2_async_notifier_cleanup(&priv->notifier);
> -			return ret;
> +			return PTR_ERR(asd);
>  		}
>  
> -		/*
> -		 * Balance the reference counting handled through
> -		 * v4l2_async_notifier_cleanup()
> -		 */
> -		fwnode_handle_get(source->fwnode);
> +		to_max9286_asd(asd)->source = source;
>  	}
>  
>  	priv->notifier.ops = &max9286_notify_ops;

-- 
Regards,

Laurent Pinchart

RE: [PATCH 0/5] media: Fix asd dynamic allocation

[Fabrizio Castro]

Hello Laurent,

Thank you for your work.

I have tested your patches related to the DRIF driver, and they look ok to me.

Tested-by: Fabrizio Castro <fabrizio.castro.jz@renesas.com>

Thanks,
Fab

> From: Chris Paterson <Chris.Paterson2@renesas.com>
> Sent: 01 September 2020 11:46
>
> Hello,
>
> > From: Ramesh Shanmugasundaram <rashanmu@gmail.com>
> > Sent: 31 August 2020 20:45
> >
> > Hi Laurent,
> >
> > Thank you for the fix. Sorry about the late reply.
> >
> > > This small patch series addresses a bug related to the usage of
> > > v4l2_async_notifier_add_subdev() that is widespread among drivers.
> > >
> > > The issue is explained in 1/4, which improves the documentation of the
> > > v4l2_async_notifier_add_subdev() function by stating explicitly that the
> > > asd argument needs to be allocated dynamically. Among the 13 drivers
> > > that use that function, only one gets it right.
> > >
> > > The rest of the patches fix the problem in several Renesas-related
> > > drivers, with an unrelated fwnode reference leak fix for the rcar-drif
> > > driver in 2/5 that made the v4l2_async_notifier_add_subdev() fix easier
> > > to implement in that driver.
> > >
> > > I'm lacking hardware to test 2/5 and 3/5. Ramesh, would you be able to
> > > test that ? What development board do you use to test the DRIF driver ?
> > > I don't see any DT integration upstream.
> >
> > I'm afraid I may not be able to test it as I do not have the hardware.
> > I have copied Chris P if he can help out.
>
> Thanks Ramesh.
>
> Fabrizio will take a look as he's working on DRIF currently.
> We'll also get him added to the maintainers file.
>
> Kind regards, Chris
>
> >
> > I have tested DRIF driver on a H3/M3 Salvator-X/S with a add-on board
> > from Maxim having two MAX2175 tuners (MAX2175 evaluation kit). This
> > add-on board was a prototype at that time. Hence, the DT updates were
> > maintained internally.
> >
> > Thanks,
> > Ramesh
> >
> > > I also haven't dug up my MAX9286 development kit to test 5/5. I would
> > > thus appreciate if someone could test it, but worst case I can do so
> > > myself.
> > >
> > > Laurent Pinchart (5):
> > >   media: v4l2-async: Document asd allocation requirements
> > >   media: rcar_drif: Fix fwnode reference leak when parsing DT
> > >   media: rcar_drif: Allocate v4l2_async_subdev dynamically
> > >   media: rcar-csi2: Allocate v4l2_async_subdev dynamically
> > >   media: i2c: max9286: Allocate v4l2_async_subdev dynamically
> > >
> > >  drivers/media/i2c/max9286.c                 | 38 +++++++++++----------
> > >  drivers/media/platform/rcar-vin/rcar-csi2.c | 24 ++++++-------
> > >  drivers/media/platform/rcar_drif.c          | 30 +++++-----------
> > >  include/media/v4l2-async.h                  |  5 +--
> > >  4 files changed, 42 insertions(+), 55 deletions(-)
> > >
> > > --
> > > Regards,
> > >
> > > Laurent Pinchart
> > >


Renesas Electronics Europe GmbH, Geschaeftsfuehrer/President: Carsten Jauch, Sitz der Gesellschaft/Registered office: Duesseldorf, Arcadiastrasse 10, 40472 Duesseldorf, Germany, Handelsregister/Commercial Register: Duesseldorf, HRB 3708 USt-IDNr./Tax identification no.: DE 119353406 WEEE-Reg.-Nr./WEEE reg. no.: DE 14978647

Re: [PATCH 5/5] media: i2c: max9286: Allocate v4l2_async_subdev dynamically

[Jacopo Mondi]

Hi Laurent,

On Tue, Aug 11, 2020 at 11:59:39PM +0300, Laurent Pinchart wrote:
> v4l2_async_notifier_add_subdev() requires the asd to be allocated
> dynamically, but the max9286 driver embeds it in the max9286_source
> structure. This causes memory corruption when the notifier is destroyed
> at remove time with v4l2_async_notifier_cleanup().
>
> Fix this issue by registering the asd with
> v4l2_async_notifier_add_fwnode_subdev(), which allocates it dynamically
> internally. A new max9286_asd structure is introduced, to store a
> pointer to the corresonding max9286_source that needs to be accessed
> from bound and unbind callbacks. There's no need to take an extra
> explicit reference to the fwnode anymore as
> v4l2_async_notifier_add_fwnode_subdev() does so internally.
>
> While at it, use %u instead of %d to print the unsigned index in the
> error message from the v4l2_async_notifier_add_fwnode_subdev() error
> path.
>
> Fixes: 66d8c9d2422d ("media: i2c: Add MAX9286 driver")
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> ---
>  drivers/media/i2c/max9286.c | 38 +++++++++++++++++++------------------
>  1 file changed, 20 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> index 47f280518fdb..5d890dddb376 100644
> --- a/drivers/media/i2c/max9286.c
> +++ b/drivers/media/i2c/max9286.c
> @@ -135,13 +135,19 @@
>  #define MAX9286_SRC_PAD			4
>
>  struct max9286_source {
> -	struct v4l2_async_subdev asd;
>  	struct v4l2_subdev *sd;
>  	struct fwnode_handle *fwnode;
>  };
>
> -#define asd_to_max9286_source(_asd) \
> -	container_of(_asd, struct max9286_source, asd)
> +struct max9286_asd {
> +	struct v4l2_async_subdev base;
> +	struct max9286_source *source;
> +};
> +
> +static inline struct max9286_asd *to_max9286_asd(struct v4l2_async_subdev *asd)
> +{
> +	return container_of(asd, struct max9286_asd, base);
> +}
>
>  struct max9286_priv {
>  	struct i2c_client *client;
> @@ -480,7 +486,7 @@ static int max9286_notify_bound(struct v4l2_async_notifier *notifier,
>  				struct v4l2_async_subdev *asd)
>  {
>  	struct max9286_priv *priv = sd_to_max9286(notifier->sd);
> -	struct max9286_source *source = asd_to_max9286_source(asd);
> +	struct max9286_source *source = to_max9286_asd(asd)->source;
>  	unsigned int index = to_index(priv, source);
>  	unsigned int src_pad;
>  	int ret;
> @@ -544,7 +550,7 @@ static void max9286_notify_unbind(struct v4l2_async_notifier *notifier,
>  				  struct v4l2_async_subdev *asd)
>  {
>  	struct max9286_priv *priv = sd_to_max9286(notifier->sd);
> -	struct max9286_source *source = asd_to_max9286_source(asd);
> +	struct max9286_source *source = to_max9286_asd(asd)->source;
>  	unsigned int index = to_index(priv, source);
>
>  	source->sd = NULL;
> @@ -569,23 +575,19 @@ static int max9286_v4l2_notifier_register(struct max9286_priv *priv)
>
>  	for_each_source(priv, source) {
>  		unsigned int i = to_index(priv, source);
> +		struct v4l2_async_subdev *asd;
>
> -		source->asd.match_type = V4L2_ASYNC_MATCH_FWNODE;
> -		source->asd.match.fwnode = source->fwnode;
> -
> -		ret = v4l2_async_notifier_add_subdev(&priv->notifier,
> -						     &source->asd);
> -		if (ret) {
> -			dev_err(dev, "Failed to add subdev for source %d", i);
> +		asd = v4l2_async_notifier_add_fwnode_subdev(&priv->notifier,
> +							    source->fwnode,
> +							    sizeof(*asd));

This should be sizeof(struct max9286_asd), but suprisingly, it doesn't
fail at runtime :)

I'll send a patch for this in the meantime.

Thanks
  j

> +		if (IS_ERR(asd)) {
> +			dev_err(dev, "Failed to add subdev for source %u: %ld",
> +				i, PTR_ERR(asd));
>  			v4l2_async_notifier_cleanup(&priv->notifier);
> -			return ret;
> +			return PTR_ERR(asd);
>  		}
>
> -		/*
> -		 * Balance the reference counting handled through
> -		 * v4l2_async_notifier_cleanup()
> -		 */
> -		fwnode_handle_get(source->fwnode);
> +		to_max9286_asd(asd)->source = source;
>  	}
>
>  	priv->notifier.ops = &max9286_notify_ops;
> --
> Regards,
>
> Laurent Pinchart
>

Re: [PATCH 1/5] media: v4l2-async: Document asd allocation requirements

[Kieran Bingham]

Hi Laurent,

On 11/08/2020 21:59, Laurent Pinchart wrote:
> The v4l2_async_notifier_add_subdev() function requires the asd pointer
> it receives to be allocated dynamically, but doesn't explicitly say so.
> Only one driver out of 13 get its right (atmel-sama5d2-isc.c, but with
> memory leaks in the error paths), clearly showing we have an issue.
> 
> Update the v4l2_async_notifier_add_subdev() documentation to clearly
> state the allocation requirement. Whether this will be enough to avoid
> new offending code isn't certain, but it's a good first step
> nonetheless.
> 
> Fixes: 9ca465312132 ("media: v4l: fwnode: Support generic parsing of graph endpoints in a device")
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

Seems reasonable to me:

Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

> ---
>  include/media/v4l2-async.h | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/include/media/v4l2-async.h b/include/media/v4l2-async.h
> index 8319284c93cb..d6e31234826f 100644
> --- a/include/media/v4l2-async.h
> +++ b/include/media/v4l2-async.h
> @@ -154,8 +154,9 @@ void v4l2_async_notifier_init(struct v4l2_async_notifier *notifier);
>   * @notifier: pointer to &struct v4l2_async_notifier
>   * @asd: pointer to &struct v4l2_async_subdev
>   *
> - * Call this function before registering a notifier to link the
> - * provided asd to the notifiers master @asd_list.
> + * Call this function before registering a notifier to link the provided @asd to
> + * the notifiers master @asd_list. The @asd must be allocated with k*alloc() as
> + * it will be freed by the framework when the notifier is destroyed.
>   */
>  int v4l2_async_notifier_add_subdev(struct v4l2_async_notifier *notifier,
>  				   struct v4l2_async_subdev *asd);
> 

Re: [PATCH 2/5] media: rcar_drif: Fix fwnode reference leak when parsing DT

[Kieran Bingham]

Hi Laurent,

On 11/08/2020 21:59, Laurent Pinchart wrote:
> The fwnode reference corresponding to the endpoint is leaked in an error
> path of the rcar_drif_parse_subdevs() function. Fix it, and reorganize
> fwnode reference handling in the function to release references early,
> simplifying error paths.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

Simplified indeed.

Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

> ---
>  drivers/media/platform/rcar_drif.c | 16 +++++-----------
>  1 file changed, 5 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/media/platform/rcar_drif.c b/drivers/media/platform/rcar_drif.c
> index 3d2451ac347d..3f1e5cb8b197 100644
> --- a/drivers/media/platform/rcar_drif.c
> +++ b/drivers/media/platform/rcar_drif.c
> @@ -1227,28 +1227,22 @@ static int rcar_drif_parse_subdevs(struct rcar_drif_sdr *sdr)
>  	if (!ep)
>  		return 0;
>  
> +	/* Get the endpoint properties */
> +	rcar_drif_get_ep_properties(sdr, ep);
> +
>  	fwnode = fwnode_graph_get_remote_port_parent(ep);
> +	fwnode_handle_put(ep);
>  	if (!fwnode) {
>  		dev_warn(sdr->dev, "bad remote port parent\n");
> -		fwnode_handle_put(ep);
>  		return -EINVAL;
>  	}
>  
>  	sdr->ep.asd.match.fwnode = fwnode;
>  	sdr->ep.asd.match_type = V4L2_ASYNC_MATCH_FWNODE;
>  	ret = v4l2_async_notifier_add_subdev(notifier, &sdr->ep.asd);
> -	if (ret) {
> -		fwnode_handle_put(fwnode);
> -		return ret;
> -	}
> -
> -	/* Get the endpoint properties */
> -	rcar_drif_get_ep_properties(sdr, ep);
> -
>  	fwnode_handle_put(fwnode);
> -	fwnode_handle_put(ep);
>  
> -	return 0;
> +	return ret;
>  }
>  
>  /* Check if the given device is the primary bond */
> 

Re: [PATCH 3/5] media: rcar_drif: Allocate v4l2_async_subdev dynamically

[Kieran Bingham]

Hi Laurent,

On 11/08/2020 21:59, Laurent Pinchart wrote:
> v4l2_async_notifier_add_subdev() requires the asd to be allocated
> dynamically, but the rcar-drif driver embeds it in the
> rcar_drif_graph_ep structure. This causes memory corruption when the
> notifier is destroyed at remove time with v4l2_async_notifier_cleanup().
> 
> Fix this issue by registering the asd with
> v4l2_async_notifier_add_fwnode_subdev(), which allocates it dynamically
> internally.
> 
> Fixes: d079f94c9046 ("media: platform: Switch to v4l2_async_notifier_add_subdev")
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>


> ---
>  drivers/media/platform/rcar_drif.c | 18 ++++++------------
>  1 file changed, 6 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/media/platform/rcar_drif.c b/drivers/media/platform/rcar_drif.c
> index 3f1e5cb8b197..f318cd4b8086 100644
> --- a/drivers/media/platform/rcar_drif.c
> +++ b/drivers/media/platform/rcar_drif.c
> @@ -185,7 +185,6 @@ struct rcar_drif_frame_buf {
>  /* OF graph endpoint's V4L2 async data */
>  struct rcar_drif_graph_ep {
>  	struct v4l2_subdev *subdev;	/* Async matched subdev */
> -	struct v4l2_async_subdev asd;	/* Async sub-device descriptor */
>  };
>  
>  /* DMA buffer */
> @@ -1109,12 +1108,6 @@ static int rcar_drif_notify_bound(struct v4l2_async_notifier *notifier,
>  	struct rcar_drif_sdr *sdr =
>  		container_of(notifier, struct rcar_drif_sdr, notifier);
>  
> -	if (sdr->ep.asd.match.fwnode !=
> -	    of_fwnode_handle(subdev->dev->of_node)) {
> -		rdrif_err(sdr, "subdev %s cannot bind\n", subdev->name);
> -		return -EINVAL;
> -	}
> -
>  	v4l2_set_subdev_hostdata(subdev, sdr);
>  	sdr->ep.subdev = subdev;
>  	rdrif_dbg(sdr, "bound asd %s\n", subdev->name);
> @@ -1218,7 +1211,7 @@ static int rcar_drif_parse_subdevs(struct rcar_drif_sdr *sdr)
>  {
>  	struct v4l2_async_notifier *notifier = &sdr->notifier;
>  	struct fwnode_handle *fwnode, *ep;
> -	int ret;
> +	struct v4l2_async_subdev *asd;
>  
>  	v4l2_async_notifier_init(notifier);
>  
> @@ -1237,12 +1230,13 @@ static int rcar_drif_parse_subdevs(struct rcar_drif_sdr *sdr)
>  		return -EINVAL;
>  	}
>  
> -	sdr->ep.asd.match.fwnode = fwnode;
> -	sdr->ep.asd.match_type = V4L2_ASYNC_MATCH_FWNODE;
> -	ret = v4l2_async_notifier_add_subdev(notifier, &sdr->ep.asd);
> +	asd = v4l2_async_notifier_add_fwnode_subdev(notifier, fwnode,
> +						    sizeof(*asd));

I guess this isn't suffering from the same thing that happened on the
max9286 as there is no need for any private data to follow here.

So,

Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

>  	fwnode_handle_put(fwnode);
> +	if (IS_ERR(asd))
> +		return PTR_ERR(asd);
>  
> -	return ret;
> +	return 0;
>  }
>  
>  /* Check if the given device is the primary bond */
>