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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxxxx>
> 
> 
> Only a couple of minorish comments below.
> 
> With those fixed:
> 
> Reviewed-by: Kieran Bingham <kieran.bingham+renesas@xxxxxxxxxxxxxxxx>
> 
> 
> 
> > ---
> >  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



[Index of Archives]     [Linux Samsung SOC]     [Linux Wireless]     [Linux Kernel]     [ATH6KL]     [Linux Bluetooth]     [Linux Netdev]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Samba]     [Device Mapper]

  Powered by Linux