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

[Sakari Ailus <sakari.ailus@linux.intel.com>]

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.

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.

> >  			goto err_parse;
> > -		}
> >  
> > +		rk_asd = container_of(asd, struct rkisp1_sensor_async, asd);
> 
> It could be nice to turn v4l2_async_notifier_add_fwnode_remote_subdev()
> into a macro that would take the asd structure type, and cast the
> result, to avoid container_of() in the caller. That can be done on top
> of this series.
> 
> >  		rk_asd->mbus_type = vep.bus_type;
> >  		rk_asd->mbus_flags = vep.bus.mipi_csi2.flags;
> >  		rk_asd->lanes = vep.bus.mipi_csi2.num_data_lanes;
> >  
> > -		ret = v4l2_async_notifier_add_fwnode_remote_subdev(ntf, ep,
> > -								   &rk_asd->asd);
> > -		if (ret)
> > -			goto err_parse;
> > -
> >  		dev_dbg(rkisp1->dev, "registered ep id %d with %d lanes\n",
> >  			vep.base.id, rk_asd->lanes);
> >  

-- 
Kind regards,

Sakari Ailus