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
