| Message ID | 20190213220754.14664-1-niklas.soderlund+renesas@ragnatech.se (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Kieran Bingham |
| Headers | show |
| Series | rcar-vin: Fix lockdep warning at stream on | expand |
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; > >
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
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 >
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;
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(-)