Re: [PATCH 01/13] media: v4l2-async: Clean v4l2_async_notifier_add_fwnode_remote_subdev semantics

[Ezequiel Garcia <ezequiel@collabora.com>]

On Thu, 2021-01-14 at 15:47 +0200, Sakari Ailus wrote:
> Hi Laurent, Ezequiel,
> 
> On Thu, Jan 14, 2021 at 03:59:10AM +0200, Laurent Pinchart wrote:
> > > diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c
> > > index 68da1eed753d..235dcf0c4122 100644
> > > --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c
> > > +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c
> > > @@ -252,6 +252,7 @@ static int rkisp1_subdev_notifier(struct rkisp1_device *rkisp1)
> > >                         .bus_type = V4L2_MBUS_CSI2_DPHY
> > >                 };
> > >                 struct rkisp1_sensor_async *rk_asd = NULL;
> > > +               struct v4l2_async_subdev *asd;
> > >                 struct fwnode_handle *ep;
> > >  
> > >                 ep = fwnode_graph_get_endpoint_by_id(dev_fwnode(rkisp1->dev),
> > > @@ -264,21 +265,16 @@ static int rkisp1_subdev_notifier(struct rkisp1_device *rkisp1)
> > >                 if (ret)
> > >                         goto err_parse;
> > >  
> > > -               rk_asd = kzalloc(sizeof(*rk_asd), GFP_KERNEL);
> > > -               if (!rk_asd) {
> > > -                       ret = -ENOMEM;
> > > +               asd = v4l2_async_notifier_add_fwnode_remote_subdev(ntf, ep,
> > > +                                                       sizeof(*rk_asd));
> > > +               if (IS_ERR(asd))
> 
> The problem with registering the sub-device already here is that the driver
> can proceed to use the information in the async sub-device object which is
> initialised below.
> 

Note that this interface is not really registering sub-devices.

> There might not be practical problems but there's also no guarantee it
> would work. The same problem is actually present in the rest of the
> functions registering the object after allocating it.
> 

I'd say the v4l2_async_notifier_add_{}_subdev interface is about adding a
v4l2-async subdevice descriptor to a v4l2-async notifier.

Until the v4l2-async notifier is not registered by v4l2_async_notifier_register()
(which is expected to happen after the structures that embed the descriptors
are filled, if such thing is needed), then I don't think the driver
would have access to the descriptors.

The access to the v4l2-async subdevice descriptor (struct v4l2_async_subdev)
should be done via v4l2_async_notifier_operations.bound and .complete ops.

I think this usage model is safe, and quite clear from the interface itself,
so don't think there's any issue with this change.

And OTOH, this is about making v4l2_async_notifier_add_fwnode_remote_subdev
consistent with v4l2_async_notifier_add_fwnode_subdev et al.

Thanks,
Ezequiel