Hi Laurent,
Thanks for the patch.
On Thu, 2021-01-14 at 05:07 +0200, Laurent Pinchart wrote:
> The functions that add an async subdev to an async subdev notifier take
> as an argument the size of the container structure they need to
> allocate. This is error prone, as passing an invalid size will not be
> caught by the compiler. Wrap those functions in macros that take a
> container type instead of a size, and cast the returned pointer to the
> desired type. The compiler will catch mistakes if the incorrect type is
> passed to the macro, as the assignment types won't match.
>
With a majority of drivers allocating a struct v4l2_async_subdev, it's
tempting to have two versions of the APIs, one that just allocates
struct v4l2_async_subdev, and one to embed it in a larger struct.
But that means duplicating the API, which sounds like an overkill.
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@xxxxxxxxxxxxxxxx>
> ---
>
> This patch is based on top of Ezequiel's "[PATCH 00/13] V4L2 Async
> notifier API cleanup" series. It makes errors such as the one fixed by
> "[PATCH] media: ti-vpe: cal: fix write to unallocated memory" impossible
> to occur in the first place.
>
> drivers/media/i2c/max9286.c | 2 +-
> drivers/media/i2c/st-mipid02.c | 2 +-
> drivers/media/pci/intel/ipu3/ipu3-cio2.c | 10 ++---
> drivers/media/platform/am437x/am437x-vpfe.c | 2 +-
> drivers/media/platform/atmel/atmel-isi.c | 2 +-
> .../media/platform/atmel/atmel-sama5d2-isc.c | 2 +-
> drivers/media/platform/cadence/cdns-csi2rx.c | 3 +-
> drivers/media/platform/davinci/vpif_capture.c | 2 +-
> drivers/media/platform/exynos4-is/media-dev.c | 3 +-
> .../media/platform/marvell-ccic/cafe-driver.c | 2 +-
> .../media/platform/marvell-ccic/mmp-driver.c | 4 +-
> drivers/media/platform/omap3isp/isp.c | 16 +++-----
> drivers/media/platform/pxa_camera.c | 4 +-
> drivers/media/platform/qcom/camss/camss.c | 11 ++----
> drivers/media/platform/rcar-vin/rcar-core.c | 5 ++-
> drivers/media/platform/rcar-vin/rcar-csi2.c | 2 +-
> drivers/media/platform/rcar_drif.c | 2 +-
> drivers/media/platform/renesas-ceu.c | 20 ++++------
> .../platform/rockchip/rkisp1/rkisp1-dev.c | 10 ++---
> drivers/media/platform/stm32/stm32-dcmi.c | 3 +-
> .../platform/sunxi/sun4i-csi/sun4i_csi.c | 4 +-
> drivers/media/platform/ti-vpe/cal.c | 12 +++---
> drivers/media/platform/video-mux.c | 2 +-
> drivers/media/platform/xilinx/xilinx-vipp.c | 10 ++---
> drivers/media/v4l2-core/v4l2-async.c | 38 +++++++++----------
> drivers/media/v4l2-core/v4l2-fwnode.c | 4 +-
> drivers/staging/media/imx/imx-media-csi.c | 2 +-
> drivers/staging/media/imx/imx-media-of.c | 2 +-
> drivers/staging/media/imx/imx6-mipi-csi2.c | 2 +-
> drivers/staging/media/imx/imx7-media-csi.c | 2 +-
> drivers/staging/media/imx/imx7-mipi-csis.c | 2 +-
> drivers/staging/media/tegra-video/vi.c | 10 ++---
> include/media/v4l2-async.h | 36 ++++++++++++------
> 33 files changed, 116 insertions(+), 117 deletions(-)
>
[..]
> diff --git a/drivers/media/platform/renesas-ceu.c b/drivers/media/platform/renesas-ceu.c
> index 18485812a21e..ae4ad255a46a 100644
> --- a/drivers/media/platform/renesas-ceu.c
> +++ b/drivers/media/platform/renesas-ceu.c
> @@ -1473,7 +1473,6 @@ static int ceu_parse_platform_data(struct ceu_device *ceudev,
> const struct ceu_platform_data *pdata)
> {
> const struct ceu_async_subdev *async_sd;
> - struct v4l2_async_subdev *asd;
> struct ceu_subdev *ceu_sd;
> unsigned int i;
> int ret;
> @@ -1489,16 +1488,15 @@ static int ceu_parse_platform_data(struct ceu_device *ceudev,
>
> /* Setup the ceu subdevice and the async subdevice. */
> async_sd = &pdata->subdevs[i];
> - asd = v4l2_async_notifier_add_i2c_subdev(&ceudev->notifier,
> + async_sd = v4l2_async_notifier_add_i2c_subdev(&ceudev->notifier,
Should be ceu_sd = v4l2_async_notifier_add_i2c_subdev(... here.
> async_sd->i2c_adapter_id,
> async_sd->i2c_address,
> - sizeof(*ceu_sd));
> - if (IS_ERR(asd)) {
> - ret = PTR_ERR(asd);
> + struct ceu_subdev);
> + if (IS_ERR(ceu_sd)) {
> + ret = PTR_ERR(ceu_sd);
> v4l2_async_notifier_cleanup(&ceudev->notifier);
> return ret;
> }
> - ceu_sd = to_ceu_subdev(asd);
> ceu_sd->mbus_flags = async_sd->flags;
> ceudev->subdevs[i] = ceu_sd;
> }
> @@ -1513,7 +1511,6 @@ static int ceu_parse_dt(struct ceu_device *ceudev)
> {
> struct device_node *of = ceudev->dev->of_node;
> struct device_node *ep;
> - struct v4l2_async_subdev *asd;
> struct ceu_subdev *ceu_sd;
> unsigned int i;
> int num_ep;
> @@ -1555,14 +1552,13 @@ static int ceu_parse_dt(struct ceu_device *ceudev)
> }
>
> /* Setup the ceu subdevice and the async subdevice. */
> - asd = v4l2_async_notifier_add_fwnode_remote_subdev(
> + ceu_sd = v4l2_async_notifier_add_fwnode_remote_subdev(
> &ceudev->notifier, of_fwnode_handle(ep),
> - sizeof(*ceu_sd));
> - if (IS_ERR(asd)) {
> - ret = PTR_ERR(asd);
> + struct ceu_subdev);
> + if (IS_ERR(ceu_sd)) {
> + ret = PTR_ERR(ceu_sd);
> goto error_cleanup;
> }
> - ceu_sd = to_ceu_subdev(asd);
> ceu_sd->mbus_flags = fw_ep.bus.parallel.flags;
> ceudev->subdevs[i] = ceu_sd;
>
> diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c
> index 235dcf0c4122..447721e78d1e 100644
> --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c
> +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c
> @@ -251,8 +251,7 @@ static int rkisp1_subdev_notifier(struct rkisp1_device *rkisp1)
> struct v4l2_fwnode_endpoint vep = {
> .bus_type = V4L2_MBUS_CSI2_DPHY
> };
> - struct rkisp1_sensor_async *rk_asd = NULL;
> - struct v4l2_async_subdev *asd;
> + struct rkisp1_sensor_async *rk_asd;
> struct fwnode_handle *ep;
>
> ep = fwnode_graph_get_endpoint_by_id(dev_fwnode(rkisp1->dev),
> @@ -265,12 +264,11 @@ static int rkisp1_subdev_notifier(struct rkisp1_device *rkisp1)
> if (ret)
> goto err_parse;
>
> - asd = v4l2_async_notifier_add_fwnode_remote_subdev(ntf, ep,
> - sizeof(*rk_asd));
> - if (IS_ERR(asd))
> + rk_asd = v4l2_async_notifier_add_fwnode_remote_subdev(ntf, ep,
> + struct rkisp1_sensor_async);
> + if (IS_ERR(rk_asd))
> goto err_parse;
>
> - rk_asd = container_of(asd, struct rkisp1_sensor_async, asd);
> 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;
> diff --git a/drivers/media/platform/stm32/stm32-dcmi.c b/drivers/media/platform/stm32/stm32-dcmi.c
> index 142f63d07dcd..bbcc2254fa2e 100644
> --- a/drivers/media/platform/stm32/stm32-dcmi.c
> +++ b/drivers/media/platform/stm32/stm32-dcmi.c
> @@ -1820,7 +1820,8 @@ static int dcmi_graph_init(struct stm32_dcmi *dcmi)
> v4l2_async_notifier_init(&dcmi->notifier);
>
> asd = v4l2_async_notifier_add_fwnode_remote_subdev(
> - &dcmi->notifier, of_fwnode_handle(ep), sizeof(*asd));
> + &dcmi->notifier, of_fwnode_handle(ep),
> + struct v4l2_async_subdev);
>
> of_node_put(ep);
>
> diff --git a/drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.c b/drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.c
> index 3f94b8c966f3..8d40a7acba9c 100644
> --- a/drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.c
> +++ b/drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.c
> @@ -135,8 +135,8 @@ static int sun4i_csi_notifier_init(struct sun4i_csi *csi)
>
> csi->bus = vep.bus.parallel;
>
> - asd = v4l2_async_notifier_add_fwnode_remote_subdev(&csi->notifier,
> - ep, sizeof(*asd));
> + asd = v4l2_async_notifier_add_fwnode_remote_subdev(&csi->notifier, ep,
> + struct v4l2_async_subdev);
> if (IS_ERR(asd)) {
> ret = PTR_ERR(asd);
> goto out;
> diff --git a/drivers/media/platform/ti-vpe/cal.c b/drivers/media/platform/ti-vpe/cal.c
> index 293cbac905b3..9169068a23a9 100644
> --- a/drivers/media/platform/ti-vpe/cal.c
> +++ b/drivers/media/platform/ti-vpe/cal.c
> @@ -685,23 +685,21 @@ static int cal_async_notifier_register(struct cal_dev *cal)
> for (i = 0; i < cal->data->num_csi2_phy; ++i) {
> struct cal_camerarx *phy = cal->phy[i];
> struct cal_v4l2_async_subdev *casd;
> - struct v4l2_async_subdev *asd;
> struct fwnode_handle *fwnode;
>
> if (!phy->sensor_node)
> continue;
>
> fwnode = of_fwnode_handle(phy->sensor_node);
> - asd = v4l2_async_notifier_add_fwnode_subdev(&cal->notifier,
> - fwnode,
> - sizeof(*asd));
> - if (IS_ERR(asd)) {
> + casd = v4l2_async_notifier_add_fwnode_subdev(&cal->notifier,
> + fwnode,
> + struct v4l2_async_subdev);
Should be passing struct cal_v4l2_async_subdev instead of struct v4l2_async_subdev.
> + if (IS_ERR(casd)) {
> phy_err(phy, "Failed to add subdev to notifier\n");
> - ret = PTR_ERR(asd);
> + ret = PTR_ERR(casd);
> goto error;
> }
>
> - casd = to_cal_asd(asd);
> casd->phy = phy;
> }
>
[..]
The rest of the conversions look good.
> diff --git a/include/media/v4l2-async.h b/include/media/v4l2-async.h
> index 121f7d9dcbe6..72c3f4fe1148 100644
> --- a/include/media/v4l2-async.h
> +++ b/include/media/v4l2-async.h
> @@ -189,9 +189,12 @@ int __v4l2_async_notifier_add_subdev(struct v4l2_async_notifier *notifier,
> * is released later at notifier cleanup time.
> */
> struct v4l2_async_subdev *
> -v4l2_async_notifier_add_fwnode_subdev(struct v4l2_async_notifier *notifier,
> - struct fwnode_handle *fwnode,
> - unsigned int asd_struct_size);
> +__v4l2_async_notifier_add_fwnode_subdev(struct v4l2_async_notifier *notifier,
> + struct fwnode_handle *fwnode,
> + unsigned int asd_struct_size);
> +#define v4l2_async_notifier_add_fwnode_subdev(__notifier, __fwnode, __type) \
> +((__type *)__v4l2_async_notifier_add_fwnode_subdev(__notifier, __fwnode, \
> + sizeof(__type)))
>
> /**
> * v4l2_async_notifier_add_fwnode_remote_subdev - Allocate and add a fwnode
> @@ -215,9 +218,12 @@ v4l2_async_notifier_add_fwnode_subdev(struct v4l2_async_notifier *notifier,
> * exception that the fwnode refers to a local endpoint, not the remote one.
> */
> struct v4l2_async_subdev *
> -v4l2_async_notifier_add_fwnode_remote_subdev(struct v4l2_async_notifier *notif,
> - struct fwnode_handle *endpoint,
> - unsigned int asd_struct_size);
> +__v4l2_async_notifier_add_fwnode_remote_subdev(struct v4l2_async_notifier *notif,
> + struct fwnode_handle *endpoint,
> + unsigned int asd_struct_size);
> +#define v4l2_async_notifier_add_fwnode_remote_subdev(__notifier, __ep, __type) \
> +((__type *)__v4l2_async_notifier_add_fwnode_remote_subdev(__notifier, __ep, \
> + sizeof(__type)))
>
> /**
> * v4l2_async_notifier_add_i2c_subdev - Allocate and add an i2c async
> @@ -235,9 +241,12 @@ v4l2_async_notifier_add_fwnode_remote_subdev(struct v4l2_async_notifier *notif,
> * Same as above but for I2C matched sub-devices.
> */
> struct v4l2_async_subdev *
> -v4l2_async_notifier_add_i2c_subdev(struct v4l2_async_notifier *notifier,
> - int adapter_id, unsigned short address,
> - unsigned int asd_struct_size);
> +__v4l2_async_notifier_add_i2c_subdev(struct v4l2_async_notifier *notifier,
> + int adapter_id, unsigned short address,
> + unsigned int asd_struct_size);
> +#define v4l2_async_notifier_add_i2c_subdev(__notifier, __adap, __addr, __type) \
> +((__type *)__v4l2_async_notifier_add_i2c_subdev(__notifier, __adap, __addr, \
> + sizeof(__type)))
>
> /**
> * v4l2_async_notifier_add_devname_subdev - Allocate and add a device-name
> @@ -254,9 +263,12 @@ v4l2_async_notifier_add_i2c_subdev(struct v4l2_async_notifier *notifier,
> * Same as above but for device-name matched sub-devices.
> */
> struct v4l2_async_subdev *
> -v4l2_async_notifier_add_devname_subdev(struct v4l2_async_notifier *notifier,
> - const char *device_name,
> - unsigned int asd_struct_size);
> +__v4l2_async_notifier_add_devname_subdev(struct v4l2_async_notifier *notifier,
> + const char *device_name,
> + unsigned int asd_struct_size);
> +#define v4l2_async_notifier_add_devname_subdev(__notifier, __name, __type) \
> +((__type *)__v4l2_async_notifier_add_devname_subdev(__notifier, __name, \
> + sizeof(__type)))
>
I'm not thrilled about having a API that returns a type like this,
as it makes it a bit confusing. But of course, being able to catch
the error at compile-times I think justifies it.
Would you like me to fold this patch in my v2 cleanup series?
Ezequiel
