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
