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
