* [PATCH 1/5] media: v4l2-async: Document asd allocation requirements
2020-08-11 20:59 [PATCH 0/5] media: Fix asd dynamic allocation Laurent Pinchart
@ 2020-08-11 20:59 ` Laurent Pinchart
2020-09-08 5:08 ` Laurent Pinchart
2020-09-16 15:44 ` Kieran Bingham
2020-08-11 20:59 ` [PATCH 2/5] media: rcar_drif: Fix fwnode reference leak when parsing DT Laurent Pinchart
` (4 subsequent siblings)
5 siblings, 2 replies; 17+ messages in thread
From: Laurent Pinchart @ 2020-08-11 20:59 UTC (permalink / raw)
To: linux-media
Cc: linux-renesas-soc, Jacopo Mondi, Kieran Bingham,
Niklas Söderlund, Ramesh Shanmugasundaram, Sakari Ailus,
Steve Longerbeam
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
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH 1/5] media: v4l2-async: Document asd allocation requirements
2020-08-11 20:59 ` [PATCH 1/5] media: v4l2-async: Document asd allocation requirements Laurent Pinchart
@ 2020-09-08 5:08 ` Laurent Pinchart
2020-09-16 15:44 ` Kieran Bingham
1 sibling, 0 replies; 17+ messages in thread
From: Laurent Pinchart @ 2020-09-08 5:08 UTC (permalink / raw)
To: Laurent Pinchart
Cc: linux-media, linux-renesas-soc, Jacopo Mondi, Kieran Bingham,
Niklas Söderlund, Ramesh Shanmugasundaram, Sakari Ailus,
Steve Longerbeam
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/5] media: v4l2-async: Document asd allocation requirements
2020-08-11 20:59 ` [PATCH 1/5] media: v4l2-async: Document asd allocation requirements Laurent Pinchart
2020-09-08 5:08 ` Laurent Pinchart
@ 2020-09-16 15:44 ` Kieran Bingham
1 sibling, 0 replies; 17+ messages in thread
From: Kieran Bingham @ 2020-09-16 15:44 UTC (permalink / raw)
To: Laurent Pinchart, linux-media
Cc: linux-renesas-soc, Jacopo Mondi, Niklas Söderlund,
Ramesh Shanmugasundaram, Sakari Ailus, Steve Longerbeam
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);
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 2/5] media: rcar_drif: Fix fwnode reference leak when parsing DT
2020-08-11 20:59 [PATCH 0/5] media: Fix asd dynamic allocation Laurent Pinchart
2020-08-11 20:59 ` [PATCH 1/5] media: v4l2-async: Document asd allocation requirements Laurent Pinchart
@ 2020-08-11 20:59 ` Laurent Pinchart
2020-09-16 15:46 ` Kieran Bingham
2020-08-11 20:59 ` [PATCH 3/5] media: rcar_drif: Allocate v4l2_async_subdev dynamically Laurent Pinchart
` (3 subsequent siblings)
5 siblings, 1 reply; 17+ messages in thread
From: Laurent Pinchart @ 2020-08-11 20:59 UTC (permalink / raw)
To: linux-media
Cc: linux-renesas-soc, Jacopo Mondi, Kieran Bingham,
Niklas Söderlund, Ramesh Shanmugasundaram, Sakari Ailus,
Steve Longerbeam
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
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH 2/5] media: rcar_drif: Fix fwnode reference leak when parsing DT
2020-08-11 20:59 ` [PATCH 2/5] media: rcar_drif: Fix fwnode reference leak when parsing DT Laurent Pinchart
@ 2020-09-16 15:46 ` Kieran Bingham
0 siblings, 0 replies; 17+ messages in thread
From: Kieran Bingham @ 2020-09-16 15:46 UTC (permalink / raw)
To: Laurent Pinchart, linux-media
Cc: linux-renesas-soc, Jacopo Mondi, Niklas Söderlund,
Ramesh Shanmugasundaram, Sakari Ailus, Steve Longerbeam
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 */
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 3/5] media: rcar_drif: Allocate v4l2_async_subdev dynamically
2020-08-11 20:59 [PATCH 0/5] media: Fix asd dynamic allocation Laurent Pinchart
2020-08-11 20:59 ` [PATCH 1/5] media: v4l2-async: Document asd allocation requirements Laurent Pinchart
2020-08-11 20:59 ` [PATCH 2/5] media: rcar_drif: Fix fwnode reference leak when parsing DT Laurent Pinchart
@ 2020-08-11 20:59 ` Laurent Pinchart
2020-09-16 15:53 ` Kieran Bingham
2020-08-11 20:59 ` [PATCH 4/5] media: rcar-csi2: " Laurent Pinchart
` (2 subsequent siblings)
5 siblings, 1 reply; 17+ messages in thread
From: Laurent Pinchart @ 2020-08-11 20:59 UTC (permalink / raw)
To: linux-media
Cc: linux-renesas-soc, Jacopo Mondi, Kieran Bingham,
Niklas Söderlund, Ramesh Shanmugasundaram, Sakari Ailus,
Steve Longerbeam
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
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH 3/5] media: rcar_drif: Allocate v4l2_async_subdev dynamically
2020-08-11 20:59 ` [PATCH 3/5] media: rcar_drif: Allocate v4l2_async_subdev dynamically Laurent Pinchart
@ 2020-09-16 15:53 ` Kieran Bingham
0 siblings, 0 replies; 17+ messages in thread
From: Kieran Bingham @ 2020-09-16 15:53 UTC (permalink / raw)
To: Laurent Pinchart, linux-media
Cc: linux-renesas-soc, Jacopo Mondi, Niklas Söderlund,
Ramesh Shanmugasundaram, Sakari Ailus, Steve Longerbeam
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 */
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 4/5] media: rcar-csi2: Allocate v4l2_async_subdev dynamically
2020-08-11 20:59 [PATCH 0/5] media: Fix asd dynamic allocation Laurent Pinchart
` (2 preceding siblings ...)
2020-08-11 20:59 ` [PATCH 3/5] media: rcar_drif: Allocate v4l2_async_subdev dynamically Laurent Pinchart
@ 2020-08-11 20:59 ` Laurent Pinchart
2020-08-11 21:43 ` Niklas Söderlund
2020-08-11 20:59 ` [PATCH 5/5] media: i2c: max9286: " Laurent Pinchart
2020-08-31 19:44 ` [PATCH 0/5] media: Fix asd dynamic allocation Ramesh Shanmugasundaram
5 siblings, 1 reply; 17+ messages in thread
From: Laurent Pinchart @ 2020-08-11 20:59 UTC (permalink / raw)
To: linux-media
Cc: linux-renesas-soc, Jacopo Mondi, Kieran Bingham,
Niklas Söderlund, Ramesh Shanmugasundaram, Sakari Ailus,
Steve Longerbeam
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
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH 4/5] media: rcar-csi2: Allocate v4l2_async_subdev dynamically
2020-08-11 20:59 ` [PATCH 4/5] media: rcar-csi2: " Laurent Pinchart
@ 2020-08-11 21:43 ` Niklas Söderlund
2020-08-11 22:14 ` Laurent Pinchart
0 siblings, 1 reply; 17+ messages in thread
From: Niklas Söderlund @ 2020-08-11 21:43 UTC (permalink / raw)
To: Laurent Pinchart
Cc: linux-media, linux-renesas-soc, Jacopo Mondi, Kieran Bingham,
Ramesh Shanmugasundaram, Sakari Ailus, Steve Longerbeam
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 4/5] media: rcar-csi2: Allocate v4l2_async_subdev dynamically
2020-08-11 21:43 ` Niklas Söderlund
@ 2020-08-11 22:14 ` Laurent Pinchart
0 siblings, 0 replies; 17+ messages in thread
From: Laurent Pinchart @ 2020-08-11 22:14 UTC (permalink / raw)
To: Niklas Söderlund
Cc: Laurent Pinchart, linux-media, linux-renesas-soc, Jacopo Mondi,
Kieran Bingham, Ramesh Shanmugasundaram, Sakari Ailus,
Steve Longerbeam
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 5/5] media: i2c: max9286: Allocate v4l2_async_subdev dynamically
2020-08-11 20:59 [PATCH 0/5] media: Fix asd dynamic allocation Laurent Pinchart
` (3 preceding siblings ...)
2020-08-11 20:59 ` [PATCH 4/5] media: rcar-csi2: " Laurent Pinchart
@ 2020-08-11 20:59 ` Laurent Pinchart
2020-09-08 5:08 ` Laurent Pinchart
2020-09-14 12:55 ` Jacopo Mondi
2020-08-31 19:44 ` [PATCH 0/5] media: Fix asd dynamic allocation Ramesh Shanmugasundaram
5 siblings, 2 replies; 17+ messages in thread
From: Laurent Pinchart @ 2020-08-11 20:59 UTC (permalink / raw)
To: linux-media
Cc: linux-renesas-soc, Jacopo Mondi, Kieran Bingham,
Niklas Söderlund, Ramesh Shanmugasundaram, Sakari Ailus,
Steve Longerbeam
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
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH 5/5] media: i2c: max9286: Allocate v4l2_async_subdev dynamically
2020-08-11 20:59 ` [PATCH 5/5] media: i2c: max9286: " Laurent Pinchart
@ 2020-09-08 5:08 ` Laurent Pinchart
2020-09-14 12:55 ` Jacopo Mondi
1 sibling, 0 replies; 17+ messages in thread
From: Laurent Pinchart @ 2020-09-08 5:08 UTC (permalink / raw)
To: Laurent Pinchart
Cc: linux-media, linux-renesas-soc, Jacopo Mondi, Kieran Bingham,
Niklas Söderlund, Ramesh Shanmugasundaram, Sakari Ailus,
Steve Longerbeam
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 5/5] media: i2c: max9286: Allocate v4l2_async_subdev dynamically
2020-08-11 20:59 ` [PATCH 5/5] media: i2c: max9286: " Laurent Pinchart
2020-09-08 5:08 ` Laurent Pinchart
@ 2020-09-14 12:55 ` Jacopo Mondi
1 sibling, 0 replies; 17+ messages in thread
From: Jacopo Mondi @ 2020-09-14 12:55 UTC (permalink / raw)
To: Laurent Pinchart
Cc: linux-media, linux-renesas-soc, Jacopo Mondi, Kieran Bingham,
Niklas Söderlund, Ramesh Shanmugasundaram, Sakari Ailus,
Steve Longerbeam
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
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 0/5] media: Fix asd dynamic allocation
2020-08-11 20:59 [PATCH 0/5] media: Fix asd dynamic allocation Laurent Pinchart
` (4 preceding siblings ...)
2020-08-11 20:59 ` [PATCH 5/5] media: i2c: max9286: " Laurent Pinchart
@ 2020-08-31 19:44 ` Ramesh Shanmugasundaram
2020-09-01 10:46 ` Chris Paterson
5 siblings, 1 reply; 17+ messages in thread
From: Ramesh Shanmugasundaram @ 2020-08-31 19:44 UTC (permalink / raw)
To: Laurent Pinchart
Cc: linux-media, linux-renesas-soc, Jacopo Mondi, Kieran Bingham,
Niklas Söderlund, Sakari Ailus, Steve Longerbeam,
Chris Paterson
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
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* RE: [PATCH 0/5] media: Fix asd dynamic allocation
2020-08-31 19:44 ` [PATCH 0/5] media: Fix asd dynamic allocation Ramesh Shanmugasundaram
@ 2020-09-01 10:46 ` Chris Paterson
2020-09-10 15:53 ` Fabrizio Castro
0 siblings, 1 reply; 17+ messages in thread
From: Chris Paterson @ 2020-09-01 10:46 UTC (permalink / raw)
To: Fabrizio Castro, Ramesh Shanmugasundaram, Laurent Pinchart
Cc: linux-media, linux-renesas-soc, Jacopo Mondi, Kieran Bingham,
Niklas Söderlund, Sakari Ailus, Steve Longerbeam
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
> >
^ permalink raw reply [flat|nested] 17+ messages in thread
* RE: [PATCH 0/5] media: Fix asd dynamic allocation
2020-09-01 10:46 ` Chris Paterson
@ 2020-09-10 15:53 ` Fabrizio Castro
0 siblings, 0 replies; 17+ messages in thread
From: Fabrizio Castro @ 2020-09-10 15:53 UTC (permalink / raw)
To: Chris Paterson, Ramesh Shanmugasundaram, Laurent Pinchart
Cc: linux-media, linux-renesas-soc, Jacopo Mondi, Kieran Bingham,
Niklas Söderlund, Sakari Ailus, Steve Longerbeam
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
^ permalink raw reply [flat|nested] 17+ messages in thread