On Thu, Jan 14, 2021 at 11:46:11AM -0300, Ezequiel Garcia wrote:
> 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.
Not directly, but this will happen as a by-product of registering the async
sub-device and other functions that will be called. All this takes place
synchronously, meaming that by the time this function returns, the
character devices that are the user space interface have already been
created.
>
> > 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.
As I said, you might not have a practical problem but there's no
_guarantee_ that it'll be fine.
There are three things being done here, allocating memory for the async
sub-device, initialising it and finally registering it. These need to take
place in that order.
--
Kind regards,
Sakari Ailus
