[PATCH] rcar-vin: Fix lockdep warning at stream on

[PATCH] rcar-vin: Fix lockdep warning at stream on

[Niklas Söderlund]

Changes to v4l2-fwnode in commit [1] triggered a lockdep warning in
rcar-vin. The first attempt to solve this warning in the rcar-vin driver
was incomplete and only pushed the warning to happen at at stream on
time instead of at probe time.

This change reverts the incomplete fix and properly fix the warning by
removing the need to hold the rcar-vin specific group lock when calling
v4l2_async_notifier_parse_fwnode_endpoints_by_port(). And instead takes
it in the callback where it's really needed.

1. commit eae2aed1eab9bf08 ("media: v4l2-fwnode: Switch to v4l2_async_notifier_add_subdev")

Fixes: 6458afc8c49148f0 ("media: rcar-vin: remove unneeded locking in async callbacks")
Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 drivers/media/platform/rcar-vin/rcar-core.c | 43 +++++++++++++++------
 1 file changed, 32 insertions(+), 11 deletions(-)

diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
index 594d804340047511..abbb5820223965e3 100644
--- a/drivers/media/platform/rcar-vin/rcar-core.c
+++ b/drivers/media/platform/rcar-vin/rcar-core.c
@@ -546,7 +546,9 @@ static void rvin_parallel_notify_unbind(struct v4l2_async_notifier *notifier,
 
 	vin_dbg(vin, "unbind parallel subdev %s\n", subdev->name);
 
+	mutex_lock(&vin->lock);
 	rvin_parallel_subdevice_detach(vin);
+	mutex_unlock(&vin->lock);
 }
 
 static int rvin_parallel_notify_bound(struct v4l2_async_notifier *notifier,
@@ -556,7 +558,9 @@ static int rvin_parallel_notify_bound(struct v4l2_async_notifier *notifier,
 	struct rvin_dev *vin = v4l2_dev_to_vin(notifier->v4l2_dev);
 	int ret;
 
+	mutex_lock(&vin->lock);
 	ret = rvin_parallel_subdevice_attach(vin, subdev);
+	mutex_unlock(&vin->lock);
 	if (ret)
 		return ret;
 
@@ -664,6 +668,7 @@ static int rvin_group_notify_complete(struct v4l2_async_notifier *notifier)
 	}
 
 	/* Create all media device links between VINs and CSI-2's. */
+	mutex_lock(&vin->group->lock);
 	for (route = vin->info->routes; route->mask; route++) {
 		struct media_pad *source_pad, *sink_pad;
 		struct media_entity *source, *sink;
@@ -699,6 +704,7 @@ static int rvin_group_notify_complete(struct v4l2_async_notifier *notifier)
 			break;
 		}
 	}
+	mutex_unlock(&vin->group->lock);
 
 	return ret;
 }
@@ -714,6 +720,8 @@ static void rvin_group_notify_unbind(struct v4l2_async_notifier *notifier,
 		if (vin->group->vin[i])
 			rvin_v4l2_unregister(vin->group->vin[i]);
 
+	mutex_lock(&vin->group->lock);
+
 	for (i = 0; i < RVIN_CSI_MAX; i++) {
 		if (vin->group->csi[i].fwnode != asd->match.fwnode)
 			continue;
@@ -721,6 +729,8 @@ static void rvin_group_notify_unbind(struct v4l2_async_notifier *notifier,
 		vin_dbg(vin, "Unbind CSI-2 %s from slot %u\n", subdev->name, i);
 		break;
 	}
+
+	mutex_unlock(&vin->group->lock);
 }
 
 static int rvin_group_notify_bound(struct v4l2_async_notifier *notifier,
@@ -730,6 +740,8 @@ static int rvin_group_notify_bound(struct v4l2_async_notifier *notifier,
 	struct rvin_dev *vin = v4l2_dev_to_vin(notifier->v4l2_dev);
 	unsigned int i;
 
+	mutex_lock(&vin->group->lock);
+
 	for (i = 0; i < RVIN_CSI_MAX; i++) {
 		if (vin->group->csi[i].fwnode != asd->match.fwnode)
 			continue;
@@ -738,6 +750,8 @@ static int rvin_group_notify_bound(struct v4l2_async_notifier *notifier,
 		break;
 	}
 
+	mutex_unlock(&vin->group->lock);
+
 	return 0;
 }
 
@@ -752,6 +766,7 @@ static int rvin_mc_parse_of_endpoint(struct device *dev,
 				     struct v4l2_async_subdev *asd)
 {
 	struct rvin_dev *vin = dev_get_drvdata(dev);
+	int ret = 0;
 
 	if (vep->base.port != 1 || vep->base.id >= RVIN_CSI_MAX)
 		return -EINVAL;
@@ -762,38 +777,48 @@ static int rvin_mc_parse_of_endpoint(struct device *dev,
 		return -ENOTCONN;
 	}
 
+	mutex_lock(&vin->group->lock);
+
 	if (vin->group->csi[vep->base.id].fwnode) {
 		vin_dbg(vin, "OF device %pOF already handled\n",
 			to_of_node(asd->match.fwnode));
-		return -ENOTCONN;
+		ret = -ENOTCONN;
+		goto out;
 	}
 
 	vin->group->csi[vep->base.id].fwnode = asd->match.fwnode;
 
 	vin_dbg(vin, "Add group OF device %pOF to slot %u\n",
 		to_of_node(asd->match.fwnode), vep->base.id);
+out:
+	mutex_unlock(&vin->group->lock);
 
-	return 0;
+	return ret;
 }
 
 static int rvin_mc_parse_of_graph(struct rvin_dev *vin)
 {
-	unsigned int count = 0;
+	unsigned int count = 0, vin_mask = 0;
 	unsigned int i;
 	int ret;
 
 	mutex_lock(&vin->group->lock);
 
 	/* If not all VIN's are registered don't register the notifier. */
-	for (i = 0; i < RCAR_VIN_NUM; i++)
-		if (vin->group->vin[i])
+	for (i = 0; i < RCAR_VIN_NUM; i++) {
+		if (vin->group->vin[i]) {
 			count++;
+			vin_mask |= BIT(i);
+		}
+	}
 
 	if (vin->group->count != count) {
 		mutex_unlock(&vin->group->lock);
 		return 0;
 	}
 
+	mutex_unlock(&vin->group->lock);
+
 	v4l2_async_notifier_init(&vin->group->notifier);
 
 	/*
@@ -802,21 +827,17 @@ static int rvin_mc_parse_of_graph(struct rvin_dev *vin)
 	 * will only be registered once with the group notifier.
 	 */
 	for (i = 0; i < RCAR_VIN_NUM; i++) {
-		if (!vin->group->vin[i])
+		if (!(vin_mask & BIT(i)))
 			continue;
 
 		ret = v4l2_async_notifier_parse_fwnode_endpoints_by_port(
 				vin->group->vin[i]->dev, &vin->group->notifier,
 				sizeof(struct v4l2_async_subdev), 1,
 				rvin_mc_parse_of_endpoint);
-		if (ret) {
-			mutex_unlock(&vin->group->lock);
+		if (ret)
 			return ret;
-		}
 	}
 
-	mutex_unlock(&vin->group->lock);
-
 	if (list_empty(&vin->group->notifier.asd_list))
 		return 0;
 
-- 
2.20.1

Re: [PATCH] rcar-vin: Fix lockdep warning at stream on

[Kieran Bingham]

Hi Niklas,

On 13/02/2019 22:07, Niklas Söderlund wrote:
> Changes to v4l2-fwnode in commit [1] triggered a lockdep warning in
> rcar-vin. The first attempt to solve this warning in the rcar-vin driver
> was incomplete and only pushed the warning to happen at at stream on
> time instead of at probe time.
> 
> This change reverts the incomplete fix and properly fix the warning by
> removing the need to hold the rcar-vin specific group lock when calling
> v4l2_async_notifier_parse_fwnode_endpoints_by_port(). And instead takes
> it in the callback where it's really needed.
> 

It might have been more readable to provide the revert and the fix
separately, as it's hard to know which parts of this are the revert, and
which are 'new', but don't worry about that as it is fortuanately a
fairly clear separation below..


> 1. commit eae2aed1eab9bf08 ("media: v4l2-fwnode: Switch to v4l2_async_notifier_add_subdev")
> 
> Fixes: 6458afc8c49148f0 ("media: rcar-vin: remove unneeded locking in async callbacks")
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>


Only a couple of minorish comments below.

With those fixed:

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



> ---
>  drivers/media/platform/rcar-vin/rcar-core.c | 43 +++++++++++++++------
>  1 file changed, 32 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
> index 594d804340047511..abbb5820223965e3 100644
> --- a/drivers/media/platform/rcar-vin/rcar-core.c
> +++ b/drivers/media/platform/rcar-vin/rcar-core.c
> @@ -546,7 +546,9 @@ static void rvin_parallel_notify_unbind(struct v4l2_async_notifier *notifier,
>  
>  	vin_dbg(vin, "unbind parallel subdev %s\n", subdev->name);
>  
> +	mutex_lock(&vin->lock);
>  	rvin_parallel_subdevice_detach(vin);
> +	mutex_unlock(&vin->lock);
>  }
>  
>  static int rvin_parallel_notify_bound(struct v4l2_async_notifier *notifier,
> @@ -556,7 +558,9 @@ static int rvin_parallel_notify_bound(struct v4l2_async_notifier *notifier,
>  	struct rvin_dev *vin = v4l2_dev_to_vin(notifier->v4l2_dev);
>  	int ret;
>  
> +	mutex_lock(&vin->lock);
>  	ret = rvin_parallel_subdevice_attach(vin, subdev);
> +	mutex_unlock(&vin->lock);
>  	if (ret)
>  		return ret;
>  
> @@ -664,6 +668,7 @@ static int rvin_group_notify_complete(struct v4l2_async_notifier *notifier)
>  	}
>  
>  	/* Create all media device links between VINs and CSI-2's. */
> +	mutex_lock(&vin->group->lock);
>  	for (route = vin->info->routes; route->mask; route++) {
>  		struct media_pad *source_pad, *sink_pad;
>  		struct media_entity *source, *sink;
> @@ -699,6 +704,7 @@ static int rvin_group_notify_complete(struct v4l2_async_notifier *notifier)
>  			break;
>  		}
>  	}
> +	mutex_unlock(&vin->group->lock);
>  
>  	return ret;
>  }
> @@ -714,6 +720,8 @@ static void rvin_group_notify_unbind(struct v4l2_async_notifier *notifier,
>  		if (vin->group->vin[i])
>  			rvin_v4l2_unregister(vin->group->vin[i]);
>  
> +	mutex_lock(&vin->group->lock);
> +
>  	for (i = 0; i < RVIN_CSI_MAX; i++) {
>  		if (vin->group->csi[i].fwnode != asd->match.fwnode)
>  			continue;
> @@ -721,6 +729,8 @@ static void rvin_group_notify_unbind(struct v4l2_async_notifier *notifier,
>  		vin_dbg(vin, "Unbind CSI-2 %s from slot %u\n", subdev->name, i);
>  		break;
>  	}
> +
> +	mutex_unlock(&vin->group->lock);
>  }
>  
>  static int rvin_group_notify_bound(struct v4l2_async_notifier *notifier,
> @@ -730,6 +740,8 @@ static int rvin_group_notify_bound(struct v4l2_async_notifier *notifier,
>  	struct rvin_dev *vin = v4l2_dev_to_vin(notifier->v4l2_dev);
>  	unsigned int i;
>  
> +	mutex_lock(&vin->group->lock);
> +
>  	for (i = 0; i < RVIN_CSI_MAX; i++) {
>  		if (vin->group->csi[i].fwnode != asd->match.fwnode)
>  			continue;
> @@ -738,6 +750,8 @@ static int rvin_group_notify_bound(struct v4l2_async_notifier *notifier,
>  		break;
>  	}
>  
> +	mutex_unlock(&vin->group->lock);
> +
>  	return 0;
>  }

So if I'm not mistaken, everything above this is the 'revert' and the
below is the 'fix'




>  
> @@ -752,6 +766,7 @@ static int rvin_mc_parse_of_endpoint(struct device *dev,
>  				     struct v4l2_async_subdev *asd)
>  {
>  	struct rvin_dev *vin = dev_get_drvdata(dev);
> +	int ret = 0;
>  
>  	if (vep->base.port != 1 || vep->base.id >= RVIN_CSI_MAX)
>  		return -EINVAL;
> @@ -762,38 +777,48 @@ static int rvin_mc_parse_of_endpoint(struct device *dev,
>  		return -ENOTCONN;
>  	}
>  
> +	mutex_lock(&vin->group->lock);
> +
>  	if (vin->group->csi[vep->base.id].fwnode) {
>  		vin_dbg(vin, "OF device %pOF already handled\n",
>  			to_of_node(asd->match.fwnode));
> -		return -ENOTCONN;
> +		ret = -ENOTCONN;
> +		goto out;
>  	}
>  
>  	vin->group->csi[vep->base.id].fwnode = asd->match.fwnode;
>  
>  	vin_dbg(vin, "Add group OF device %pOF to slot %u\n",
>  		to_of_node(asd->match.fwnode), vep->base.id);
> +out:
> +	mutex_unlock(&vin->group->lock);

I think you could unlock before you print the debug... But perhaps
that's not a critical path.



>  
> -	return 0;
> +	return ret;
>  }
>  
>  static int rvin_mc_parse_of_graph(struct rvin_dev *vin)
>  {
> -	unsigned int count = 0;
> +	unsigned int count = 0, vin_mask = 0;

Shouldn't vin_mask have it's own line?

>  	unsigned int i;
>  	int ret;
>  
>  	mutex_lock(&vin->group->lock);
>  
>  	/* If not all VIN's are registered don't register the notifier. */
> -	for (i = 0; i < RCAR_VIN_NUM; i++)
> -		if (vin->group->vin[i])
> +	for (i = 0; i < RCAR_VIN_NUM; i++) {
> +		if (vin->group->vin[i]) {
>  			count++;
> +			vin_mask |= BIT(i);
> +		}
> +	}
>  
>  	if (vin->group->count != count) {
>  		mutex_unlock(&vin->group->lock);
>  		return 0;
>  	}
>  
> +	mutex_unlock(&vin->group->lock);
> +
>  	v4l2_async_notifier_init(&vin->group->notifier);
>  
>  	/*
> @@ -802,21 +827,17 @@ static int rvin_mc_parse_of_graph(struct rvin_dev *vin)
>  	 * will only be registered once with the group notifier.
>  	 */
>  	for (i = 0; i < RCAR_VIN_NUM; i++) {
> -		if (!vin->group->vin[i])
> +		if (!(vin_mask & BIT(i)))
>  			continue;
>  
>  		ret = v4l2_async_notifier_parse_fwnode_endpoints_by_port(
>  				vin->group->vin[i]->dev, &vin->group->notifier,
>  				sizeof(struct v4l2_async_subdev), 1,
>  				rvin_mc_parse_of_endpoint);
> -		if (ret) {
> -			mutex_unlock(&vin->group->lock);
> +		if (ret)
>  			return ret;
> -		}
>  	}
>  
> -	mutex_unlock(&vin->group->lock);
> -
>  	if (list_empty(&vin->group->notifier.asd_list))
>  		return 0;
>  
> 


-- 
Regards
--
Kieran

Re: [PATCH] rcar-vin: Fix lockdep warning at stream on

[Niklas Söderlund]

Hi Kieran,

On 2019-02-17 22:27:27 +0000, Kieran Bingham wrote:
> Hi Niklas,
> 
> On 13/02/2019 22:07, Niklas Söderlund wrote:
> > Changes to v4l2-fwnode in commit [1] triggered a lockdep warning in
> > rcar-vin. The first attempt to solve this warning in the rcar-vin driver
> > was incomplete and only pushed the warning to happen at at stream on
> > time instead of at probe time.
> > 
> > This change reverts the incomplete fix and properly fix the warning by
> > removing the need to hold the rcar-vin specific group lock when calling
> > v4l2_async_notifier_parse_fwnode_endpoints_by_port(). And instead takes
> > it in the callback where it's really needed.
> > 
> 
> It might have been more readable to provide the revert and the fix
> separately, as it's hard to know which parts of this are the revert, and
> which are 'new', but don't worry about that as it is fortuanately a
> fairly clear separation below..

I agree it would have been clearer to have it as two patches, I wanted 
to make backporting as easy as possible so I kept it in the same patch, 
I'm happy to split it into two if you think it's better.

> 
> 
> > 1. commit eae2aed1eab9bf08 ("media: v4l2-fwnode: Switch to v4l2_async_notifier_add_subdev")
> > 
> > Fixes: 6458afc8c49148f0 ("media: rcar-vin: remove unneeded locking in async callbacks")
> > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> 
> 
> Only a couple of minorish comments below.
> 
> With those fixed:
> 
> Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> 
> 
> 
> > ---
> >  drivers/media/platform/rcar-vin/rcar-core.c | 43 +++++++++++++++------
> >  1 file changed, 32 insertions(+), 11 deletions(-)
> > 
> > diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
> > index 594d804340047511..abbb5820223965e3 100644
> > --- a/drivers/media/platform/rcar-vin/rcar-core.c
> > +++ b/drivers/media/platform/rcar-vin/rcar-core.c
> > @@ -546,7 +546,9 @@ static void rvin_parallel_notify_unbind(struct v4l2_async_notifier *notifier,
> >  
> >  	vin_dbg(vin, "unbind parallel subdev %s\n", subdev->name);
> >  
> > +	mutex_lock(&vin->lock);
> >  	rvin_parallel_subdevice_detach(vin);
> > +	mutex_unlock(&vin->lock);
> >  }
> >  
> >  static int rvin_parallel_notify_bound(struct v4l2_async_notifier *notifier,
> > @@ -556,7 +558,9 @@ static int rvin_parallel_notify_bound(struct v4l2_async_notifier *notifier,
> >  	struct rvin_dev *vin = v4l2_dev_to_vin(notifier->v4l2_dev);
> >  	int ret;
> >  
> > +	mutex_lock(&vin->lock);
> >  	ret = rvin_parallel_subdevice_attach(vin, subdev);
> > +	mutex_unlock(&vin->lock);
> >  	if (ret)
> >  		return ret;
> >  
> > @@ -664,6 +668,7 @@ static int rvin_group_notify_complete(struct v4l2_async_notifier *notifier)
> >  	}
> >  
> >  	/* Create all media device links between VINs and CSI-2's. */
> > +	mutex_lock(&vin->group->lock);
> >  	for (route = vin->info->routes; route->mask; route++) {
> >  		struct media_pad *source_pad, *sink_pad;
> >  		struct media_entity *source, *sink;
> > @@ -699,6 +704,7 @@ static int rvin_group_notify_complete(struct v4l2_async_notifier *notifier)
> >  			break;
> >  		}
> >  	}
> > +	mutex_unlock(&vin->group->lock);
> >  
> >  	return ret;
> >  }
> > @@ -714,6 +720,8 @@ static void rvin_group_notify_unbind(struct v4l2_async_notifier *notifier,
> >  		if (vin->group->vin[i])
> >  			rvin_v4l2_unregister(vin->group->vin[i]);
> >  
> > +	mutex_lock(&vin->group->lock);
> > +
> >  	for (i = 0; i < RVIN_CSI_MAX; i++) {
> >  		if (vin->group->csi[i].fwnode != asd->match.fwnode)
> >  			continue;
> > @@ -721,6 +729,8 @@ static void rvin_group_notify_unbind(struct v4l2_async_notifier *notifier,
> >  		vin_dbg(vin, "Unbind CSI-2 %s from slot %u\n", subdev->name, i);
> >  		break;
> >  	}
> > +
> > +	mutex_unlock(&vin->group->lock);
> >  }
> >  
> >  static int rvin_group_notify_bound(struct v4l2_async_notifier *notifier,
> > @@ -730,6 +740,8 @@ static int rvin_group_notify_bound(struct v4l2_async_notifier *notifier,
> >  	struct rvin_dev *vin = v4l2_dev_to_vin(notifier->v4l2_dev);
> >  	unsigned int i;
> >  
> > +	mutex_lock(&vin->group->lock);
> > +
> >  	for (i = 0; i < RVIN_CSI_MAX; i++) {
> >  		if (vin->group->csi[i].fwnode != asd->match.fwnode)
> >  			continue;
> > @@ -738,6 +750,8 @@ static int rvin_group_notify_bound(struct v4l2_async_notifier *notifier,
> >  		break;
> >  	}
> >  
> > +	mutex_unlock(&vin->group->lock);
> > +
> >  	return 0;
> >  }
> 
> So if I'm not mistaken, everything above this is the 'revert' and the
> below is the 'fix'

Correct :-)

> 
> 
> 
> 
> >  
> > @@ -752,6 +766,7 @@ static int rvin_mc_parse_of_endpoint(struct device *dev,
> >  				     struct v4l2_async_subdev *asd)
> >  {
> >  	struct rvin_dev *vin = dev_get_drvdata(dev);
> > +	int ret = 0;
> >  
> >  	if (vep->base.port != 1 || vep->base.id >= RVIN_CSI_MAX)
> >  		return -EINVAL;
> > @@ -762,38 +777,48 @@ static int rvin_mc_parse_of_endpoint(struct device *dev,
> >  		return -ENOTCONN;
> >  	}
> >  
> > +	mutex_lock(&vin->group->lock);
> > +
> >  	if (vin->group->csi[vep->base.id].fwnode) {
> >  		vin_dbg(vin, "OF device %pOF already handled\n",
> >  			to_of_node(asd->match.fwnode));
> > -		return -ENOTCONN;
> > +		ret = -ENOTCONN;
> > +		goto out;
> >  	}
> >  
> >  	vin->group->csi[vep->base.id].fwnode = asd->match.fwnode;
> >  
> >  	vin_dbg(vin, "Add group OF device %pOF to slot %u\n",
> >  		to_of_node(asd->match.fwnode), vep->base.id);
> > +out:
> > +	mutex_unlock(&vin->group->lock);
> 
> I think you could unlock before you print the debug... But perhaps
> that's not a critical path.

It could be done, but then the error path would be more complex. I'm 
open to change this at your leisure.

> 
> 
> 
> >  
> > -	return 0;
> > +	return ret;
> >  }
> >  
> >  static int rvin_mc_parse_of_graph(struct rvin_dev *vin)
> >  {
> > -	unsigned int count = 0;
> > +	unsigned int count = 0, vin_mask = 0;
> 
> Shouldn't vin_mask have it's own line?

It could, I'm trying to keep the style of the rest of the file. I'm open 
to change this.

> 
> >  	unsigned int i;
> >  	int ret;
> >  
> >  	mutex_lock(&vin->group->lock);
> >  
> >  	/* If not all VIN's are registered don't register the notifier. */
> > -	for (i = 0; i < RCAR_VIN_NUM; i++)
> > -		if (vin->group->vin[i])
> > +	for (i = 0; i < RCAR_VIN_NUM; i++) {
> > +		if (vin->group->vin[i]) {
> >  			count++;
> > +			vin_mask |= BIT(i);
> > +		}
> > +	}
> >  
> >  	if (vin->group->count != count) {
> >  		mutex_unlock(&vin->group->lock);
> >  		return 0;
> >  	}
> >  
> > +	mutex_unlock(&vin->group->lock);
> > +
> >  	v4l2_async_notifier_init(&vin->group->notifier);
> >  
> >  	/*
> > @@ -802,21 +827,17 @@ static int rvin_mc_parse_of_graph(struct rvin_dev *vin)
> >  	 * will only be registered once with the group notifier.
> >  	 */
> >  	for (i = 0; i < RCAR_VIN_NUM; i++) {
> > -		if (!vin->group->vin[i])
> > +		if (!(vin_mask & BIT(i)))
> >  			continue;
> >  
> >  		ret = v4l2_async_notifier_parse_fwnode_endpoints_by_port(
> >  				vin->group->vin[i]->dev, &vin->group->notifier,
> >  				sizeof(struct v4l2_async_subdev), 1,
> >  				rvin_mc_parse_of_endpoint);
> > -		if (ret) {
> > -			mutex_unlock(&vin->group->lock);
> > +		if (ret)
> >  			return ret;
> > -		}
> >  	}
> >  
> > -	mutex_unlock(&vin->group->lock);
> > -
> >  	if (list_empty(&vin->group->notifier.asd_list))
> >  		return 0;
> >  
> > 
> 
> 
> -- 
> Regards
> --
> Kieran

-- 
Regards,
Niklas Söderlund

Re: [PATCH] rcar-vin: Fix lockdep warning at stream on

[Kieran Bingham]

Hi Niklas,

On 18/02/2019 01:43, Niklas Söderlund wrote:
> Hi Kieran,
> 
> On 2019-02-17 22:27:27 +0000, Kieran Bingham wrote:
>> Hi Niklas,
>>
>> On 13/02/2019 22:07, Niklas Söderlund wrote:
>>> Changes to v4l2-fwnode in commit [1] triggered a lockdep warning in
>>> rcar-vin. The first attempt to solve this warning in the rcar-vin driver
>>> was incomplete and only pushed the warning to happen at at stream on
>>> time instead of at probe time.
>>>
>>> This change reverts the incomplete fix and properly fix the warning by
>>> removing the need to hold the rcar-vin specific group lock when calling
>>> v4l2_async_notifier_parse_fwnode_endpoints_by_port(). And instead takes
>>> it in the callback where it's really needed.
>>>
>>
>> It might have been more readable to provide the revert and the fix
>> separately, as it's hard to know which parts of this are the revert, and
>> which are 'new', but don't worry about that as it is fortuanately a
>> fairly clear separation below..
> 
> I agree it would have been clearer to have it as two patches, I wanted 
> to make backporting as easy as possible so I kept it in the same patch, 
> I'm happy to split it into two if you think it's better.

Easing backport and stable is certainly a good enough reason for me.

Keep it as is.


> 
>>
>>
>>> 1. commit eae2aed1eab9bf08 ("media: v4l2-fwnode: Switch to v4l2_async_notifier_add_subdev")
>>>
>>> Fixes: 6458afc8c49148f0 ("media: rcar-vin: remove unneeded locking in async callbacks")
>>> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
>>
>>
>> Only a couple of minorish comments below.
>>
>> With those fixed:
>>

You've answered my queries - so you get to keep this:

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


>>
>>
>>
>>> ---
>>>  drivers/media/platform/rcar-vin/rcar-core.c | 43 +++++++++++++++------
>>>  1 file changed, 32 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
>>> index 594d804340047511..abbb5820223965e3 100644
>>> --- a/drivers/media/platform/rcar-vin/rcar-core.c
>>> +++ b/drivers/media/platform/rcar-vin/rcar-core.c
>>> @@ -546,7 +546,9 @@ static void rvin_parallel_notify_unbind(struct v4l2_async_notifier *notifier,
>>>  
>>>  	vin_dbg(vin, "unbind parallel subdev %s\n", subdev->name);
>>>  
>>> +	mutex_lock(&vin->lock);
>>>  	rvin_parallel_subdevice_detach(vin);
>>> +	mutex_unlock(&vin->lock);
>>>  }
>>>  
>>>  static int rvin_parallel_notify_bound(struct v4l2_async_notifier *notifier,
>>> @@ -556,7 +558,9 @@ static int rvin_parallel_notify_bound(struct v4l2_async_notifier *notifier,
>>>  	struct rvin_dev *vin = v4l2_dev_to_vin(notifier->v4l2_dev);
>>>  	int ret;
>>>  
>>> +	mutex_lock(&vin->lock);
>>>  	ret = rvin_parallel_subdevice_attach(vin, subdev);
>>> +	mutex_unlock(&vin->lock);
>>>  	if (ret)
>>>  		return ret;
>>>  
>>> @@ -664,6 +668,7 @@ static int rvin_group_notify_complete(struct v4l2_async_notifier *notifier)
>>>  	}
>>>  
>>>  	/* Create all media device links between VINs and CSI-2's. */
>>> +	mutex_lock(&vin->group->lock);
>>>  	for (route = vin->info->routes; route->mask; route++) {
>>>  		struct media_pad *source_pad, *sink_pad;
>>>  		struct media_entity *source, *sink;
>>> @@ -699,6 +704,7 @@ static int rvin_group_notify_complete(struct v4l2_async_notifier *notifier)
>>>  			break;
>>>  		}
>>>  	}
>>> +	mutex_unlock(&vin->group->lock);
>>>  
>>>  	return ret;
>>>  }
>>> @@ -714,6 +720,8 @@ static void rvin_group_notify_unbind(struct v4l2_async_notifier *notifier,
>>>  		if (vin->group->vin[i])
>>>  			rvin_v4l2_unregister(vin->group->vin[i]);
>>>  
>>> +	mutex_lock(&vin->group->lock);
>>> +
>>>  	for (i = 0; i < RVIN_CSI_MAX; i++) {
>>>  		if (vin->group->csi[i].fwnode != asd->match.fwnode)
>>>  			continue;
>>> @@ -721,6 +729,8 @@ static void rvin_group_notify_unbind(struct v4l2_async_notifier *notifier,
>>>  		vin_dbg(vin, "Unbind CSI-2 %s from slot %u\n", subdev->name, i);
>>>  		break;
>>>  	}
>>> +
>>> +	mutex_unlock(&vin->group->lock);
>>>  }
>>>  
>>>  static int rvin_group_notify_bound(struct v4l2_async_notifier *notifier,
>>> @@ -730,6 +740,8 @@ static int rvin_group_notify_bound(struct v4l2_async_notifier *notifier,
>>>  	struct rvin_dev *vin = v4l2_dev_to_vin(notifier->v4l2_dev);
>>>  	unsigned int i;
>>>  
>>> +	mutex_lock(&vin->group->lock);
>>> +
>>>  	for (i = 0; i < RVIN_CSI_MAX; i++) {
>>>  		if (vin->group->csi[i].fwnode != asd->match.fwnode)
>>>  			continue;
>>> @@ -738,6 +750,8 @@ static int rvin_group_notify_bound(struct v4l2_async_notifier *notifier,
>>>  		break;
>>>  	}
>>>  
>>> +	mutex_unlock(&vin->group->lock);
>>> +
>>>  	return 0;
>>>  }
>>
>> So if I'm not mistaken, everything above this is the 'revert' and the
>> below is the 'fix'
> 
> Correct :-)
> 
>>
>>
>>
>>
>>>  
>>> @@ -752,6 +766,7 @@ static int rvin_mc_parse_of_endpoint(struct device *dev,
>>>  				     struct v4l2_async_subdev *asd)
>>>  {
>>>  	struct rvin_dev *vin = dev_get_drvdata(dev);
>>> +	int ret = 0;
>>>  
>>>  	if (vep->base.port != 1 || vep->base.id >= RVIN_CSI_MAX)
>>>  		return -EINVAL;
>>> @@ -762,38 +777,48 @@ static int rvin_mc_parse_of_endpoint(struct device *dev,
>>>  		return -ENOTCONN;
>>>  	}
>>>  
>>> +	mutex_lock(&vin->group->lock);
>>> +
>>>  	if (vin->group->csi[vep->base.id].fwnode) {
>>>  		vin_dbg(vin, "OF device %pOF already handled\n",
>>>  			to_of_node(asd->match.fwnode));
>>> -		return -ENOTCONN;
>>> +		ret = -ENOTCONN;
>>> +		goto out;
>>>  	}
>>>  
>>>  	vin->group->csi[vep->base.id].fwnode = asd->match.fwnode;
>>>  
>>>  	vin_dbg(vin, "Add group OF device %pOF to slot %u\n",
>>>  		to_of_node(asd->match.fwnode), vep->base.id);
>>> +out:
>>> +	mutex_unlock(&vin->group->lock);
>>
>> I think you could unlock before you print the debug... But perhaps
>> that's not a critical path.
> 
> It could be done, but then the error path would be more complex. I'm 
> open to change this at your leisure.

Ah good point-  I had missed that. Keep it simple then.


>>>  
>>> -	return 0;
>>> +	return ret;
>>>  }
>>>  
>>>  static int rvin_mc_parse_of_graph(struct rvin_dev *vin)
>>>  {
>>> -	unsigned int count = 0;
>>> +	unsigned int count = 0, vin_mask = 0;
>>
>> Shouldn't vin_mask have it's own line?
> 
> It could, I'm trying to keep the style of the rest of the file. I'm open 
> to change this.


coding-style.rst states:

"Don’t put multiple assignments on a single line either. Kernel coding
style is super simple. Avoid tricky expressions."

But that's open to some interpretation on declarations and if you've
already set a precedent in your driver it would need the whole file
updating - so lets not do that here.

It's your driver - so that part is your choice really :)


>>
>>>  	unsigned int i;
>>>  	int ret;
>>>  
>>>  	mutex_lock(&vin->group->lock);
>>>  
>>>  	/* If not all VIN's are registered don't register the notifier. */
>>> -	for (i = 0; i < RCAR_VIN_NUM; i++)
>>> -		if (vin->group->vin[i])
>>> +	for (i = 0; i < RCAR_VIN_NUM; i++) {
>>> +		if (vin->group->vin[i]) {
>>>  			count++;
>>> +			vin_mask |= BIT(i);
>>> +		}
>>> +	}
>>>  
>>>  	if (vin->group->count != count) {
>>>  		mutex_unlock(&vin->group->lock);
>>>  		return 0;
>>>  	}
>>>  
>>> +	mutex_unlock(&vin->group->lock);
>>> +
>>>  	v4l2_async_notifier_init(&vin->group->notifier);
>>>  
>>>  	/*
>>> @@ -802,21 +827,17 @@ static int rvin_mc_parse_of_graph(struct rvin_dev *vin)
>>>  	 * will only be registered once with the group notifier.
>>>  	 */
>>>  	for (i = 0; i < RCAR_VIN_NUM; i++) {
>>> -		if (!vin->group->vin[i])
>>> +		if (!(vin_mask & BIT(i)))
>>>  			continue;
>>>  
>>>  		ret = v4l2_async_notifier_parse_fwnode_endpoints_by_port(
>>>  				vin->group->vin[i]->dev, &vin->group->notifier,
>>>  				sizeof(struct v4l2_async_subdev), 1,
>>>  				rvin_mc_parse_of_endpoint);
>>> -		if (ret) {
>>> -			mutex_unlock(&vin->group->lock);
>>> +		if (ret)
>>>  			return ret;
>>> -		}
>>>  	}
>>>  
>>> -	mutex_unlock(&vin->group->lock);
>>> -
>>>  	if (list_empty(&vin->group->notifier.asd_list))
>>>  		return 0;
>>>  
>>>
>>
>>
>> -- 
>> Regards
>> --
>> Kieran
> 

-- 
Regards
--
Kieran