Re: [PATCH v6 16/17] media: v4l2: async: Remove notifier subdevs array

[Steve Longerbeam]

Hi Jacopo,


On 08/04/2018 03:33 AM, jacopo mondi wrote:
> Hi Steve,
>
> On Mon, Jul 23, 2018 at 09:44:57AM -0700, Steve Longerbeam wrote:
> > On 07/23/2018 05:35 AM, Sakari Ailus wrote:
> > > Hi Steve,
> > >
> > > Thanks for the update.
> > >
> > > On Mon, Jul 09, 2018 at 03:39:16PM -0700, Steve Longerbeam wrote:
> > > > All platform drivers have been converted to use
> > > > v4l2_async_notifier_add_subdev(), in place of adding
> > > > asd's to the notifier subdevs array. So the subdevs
> > > > array can now be removed from struct v4l2_async_notifier,
> > > > and remove the backward compatibility support for that
> > > > array in v4l2-async.c.
> > > >
> > > > Signed-off-by: Steve Longerbeam <steve_longerbeam@xxxxxxxxxx>
> > > This set removes the subdevs and num_subdevs fieldsfrom the notifier (as
> > > discussed previously) but it doesn't include the corresponding
> > > driver changes. Is there a patch missing from the set?
> > Hi Sakari, yes somehow patch 15/17 (the large patch to all drivers)
> > got dropped by the ML, maybe because the cc-list was too big?
> >
> > I will resend with only linux-media and cc: you.
> For the Renesas CEU and Renesas R-Car VIN you can add my:
>
> Tested-by: Jacopo Mondi <jacopo+renesas@xxxxxxxxxx>

Thanks for testing!

> I would have a very small comment on the renesas-ceu.c patch. I'm copying
> the hunk here below as the patch didn't reach the mailing list
>
> > @@ -1562,40 +1557,46 @@ static int ceu_parse_dt(struct ceu_device *ceudev)
> >                         dev_err(ceudev->dev,
> >                                 "No subdevice connected on endpoint %u.\n", i);
> >                         ret = -ENODEV;
> > -                       goto error_put_node;
> > +                       goto error_cleanup;
> >                 }
> >
> >                 ret = v4l2_fwnode_endpoint_parse(of_fwnode_handle(ep), &fw_ep);
> >                 if (ret) {
> >                 if (ret) {
> >                         dev_err(ceudev->dev,
> >                                 "Unable to parse endpoint #%u.\n", i);
> > -                       goto error_put_node;
> > +                       goto error_cleanup;
> >                 }
> >
> >                 if (fw_ep.bus_type != V4L2_MBUS_PARALLEL) {
> >                         dev_err(ceudev->dev,
> >                                 "Only parallel input supported.\n");
> >                         ret = -EINVAL;
> > -                       goto error_put_node;
> > +                       goto error_cleanup;
> >                 }
> >
> >                 /* Setup the ceu subdevice and the async subdevice. */
> >                 ceu_sd = &ceudev->subdevs[i];
> >                 INIT_LIST_HEAD(&ceu_sd->asd.list);
> >
> > +               remote = of_graph_get_remote_port_parent(ep);
> >                 ceu_sd->mbus_flags = fw_ep.bus.parallel.flags;
> >                 ceu_sd->asd.match_type = V4L2_ASYNC_MATCH_FWNODE;
> > -               ceu_sd->asd.match.fwnode =
> > -                       fwnode_graph_get_remote_port_parent(
> > -                                       of_fwnode_handle(ep));
> > +               ceu_sd->asd.match.fwnode = of_fwnode_handle(remote);
> > +
> > +               ret = v4l2_async_notifier_add_subdev(&ceudev->notifier,
> > +                                                    &ceu_sd->asd);
> > +               if (ret) {
> > +                       of_node_put(remote);
>                          ^^^ The 'remote' device node is only put in
>                          the error path

If v4l2_async_notifier_add_subdev() succeeds, then the reference
is kept on the remote node until the asd is freed in
v4l2_async_notifier_cleanup(). Otherwise if
v4l2_async_notifier_add_subdev() fails, the asd is not
added to the notifier asd_list so the caller is responsible
for putting the remote node. So the only case where
the remote needs to be put is in the line you marked above.
Otherwise in the other error-out paths, the remote nodes
for all asd's that have been added will be put below in the
error_cleanup path.

Steve


> > +                       goto error_cleanup;
> > +               }
> >
> > -               ceudev->asds[i] = &ceu_sd->asd;
> >                 of_node_put(ep);
> >         }
> >
> >         return num_ep;
> >
> > -error_put_node:
> > +error_cleanup:
> > +       v4l2_async_notifier_cleanup(&ceudev->notifier);
> >         of_node_put(ep);
> >         return ret;
> > }
> Thanks
>     j