From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To: Ezequiel Garcia <ezequiel@collabora.com>
Cc: "Laurent Pinchart" <laurent.pinchart+renesas@ideasonboard.com>,
linux-media@vger.kernel.org, linux-renesas-soc@vger.kernel.org,
"Sakari Ailus" <sakari.ailus@linux.intel.com>,
"Tomi Valkeinen" <tomi.valkeinen@ideasonboard.com>,
"Niklas Söderlund" <niklas.soderlund@ragnatech.se>
Subject: Re: [PATCH] media: v4l2-async: Improve v4l2_async_notifier_add_*_subdev() API
Date: Fri, 15 Jan 2021 23:43:57 +0200 [thread overview]
Message-ID: <YAIMnbS293x/H9ZL@pendragon.ideasonboard.com> (raw)
In-Reply-To: <b7842e88b93247eec12fbc917ef448c68a98410e.camel@collabora.com>
Hi Ezequiel,
On Thu, Jan 14, 2021 at 10:29:02AM -0300, Ezequiel Garcia wrote:
> 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@ideasonboard.com>
> > ---
> >
> > 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.
Oops. I haven't compile-tested the whole series, I had meant to send it
as an RFC to get feedback on the idea before performing tests, and I
then forgot to add the RFC :-S
> > 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.
Indeed. The good thing is that the compiler now complains :-)
> > + 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?
Please do if you can.
--
Regards,
Laurent Pinchart
next prev parent reply other threads:[~2021-01-15 21:45 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-01-14 3:07 [PATCH] media: v4l2-async: Improve v4l2_async_notifier_add_*_subdev() API Laurent Pinchart
2021-01-14 13:09 ` Niklas Söderlund
2021-01-14 13:29 ` Ezequiel Garcia
2021-01-15 21:43 ` Laurent Pinchart [this message]
2021-01-15 9:56 ` Sakari Ailus
2021-01-15 21:48 ` Laurent Pinchart
2021-01-16 23:26 ` Sakari Ailus
2021-01-15 11:46 ` Jacopo Mondi
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=YAIMnbS293x/H9ZL@pendragon.ideasonboard.com \
--to=laurent.pinchart@ideasonboard.com \
--cc=ezequiel@collabora.com \
--cc=laurent.pinchart+renesas@ideasonboard.com \
--cc=linux-media@vger.kernel.org \
--cc=linux-renesas-soc@vger.kernel.org \
--cc=niklas.soderlund@ragnatech.se \
--cc=sakari.ailus@linux.intel.com \
--cc=tomi.valkeinen@ideasonboard.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.