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

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

 



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@xxxxxxxxxxxx>
>>
>>
>> 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@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.

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



[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]

  Powered by Linux