| Message ID | 20231117101140.1338288-1-sakari.ailus@linux.intel.com |
|---|---|
| Headers | show |
| Series | Unify sub-device state access functions | expand |
Hi Sakari, Thank you for the patch. On Fri, Nov 17, 2023 at 12:11:39PM +0200, Sakari Ailus wrote: > Return NULL from sub-device pad state access functions > (v4l2_subdev_state_get_{format,crop,compose}) for non-existent pads. While > this behaviour differs from older set of pad state information access > functions, we've had a WARN_ON() there for a long time and callers also do > validate the pad index nowadays. Therefore problems are not expected. > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> > --- > drivers/media/v4l2-core/v4l2-subdev.c | 36 +++++---------------------- > 1 file changed, 6 insertions(+), 30 deletions(-) > > diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c > index 41fe9fa5c21a..985873b7981e 100644 > --- a/drivers/media/v4l2-core/v4l2-subdev.c > +++ b/drivers/media/v4l2-core/v4l2-subdev.c > @@ -1545,16 +1545,8 @@ __v4l2_subdev_state_get_format(struct v4l2_subdev_state *state, > if (stream) > return NULL; > > - /* > - * Set the pad to 0 on error as this is aligned with the > - * behaviour of the pad state information access functions. The > - * purpose of setting pad to 0 here is to avoid accessing memory > - * outside the pads array, but still issuing warning of the > - * invalid access while making the caller's error handling > - * easier. > - */ > - if (WARN_ON_ONCE(pad >= state->sd->entity.num_pads)) > - pad = 0; > + if (pad >= state->sd->entity.num_pads) > + return NULL; > > return &state->pads[pad].format; > } > @@ -1587,16 +1579,8 @@ __v4l2_subdev_state_get_crop(struct v4l2_subdev_state *state, unsigned int pad, > if (stream) > return NULL; > > - /* > - * Set the pad to 0 on error as this is aligned with the > - * behaviour of the pad state information access functions. The > - * purpose of setting pad to 0 here is to avoid accessing memory > - * outside the pads array, but still issuing warning of the > - * invalid access while making the caller's error handling > - * easier. > - */ > - if (WARN_ON_ONCE(pad >= state->sd->entity.num_pads)) > - pad = 0; > + if (pad >= state->sd->entity.num_pads) > + return NULL; > > return &state->pads[pad].crop; > } > @@ -1629,16 +1613,8 @@ __v4l2_subdev_state_get_compose(struct v4l2_subdev_state *state, > if (stream) > return NULL; > > - /* > - * Set the pad to 0 on error as this is aligned with the > - * behaviour of the pad state information access functions. The > - * purpose of setting pad to 0 here is to avoid accessing memory > - * outside the pads array, but still issuing warning of the > - * invalid access while making the caller's error handling > - * easier. > - */ > - if (WARN_ON_ONCE(pad >= state->sd->entity.num_pads)) > - pad = 0; > + if (pad >= state->sd->entity.num_pads) > + return NULL; > > return &state->pads[pad].compose; > }
Hi folks, This set unifies historical and stream-aware sub-device state access functions into one set of functions called v4l2_subdev_state_get_{format,crop,compose}. No functional change is intended. This happened after Tomi suggested adding a num_pads field to struct v4l2_subdev_state. I'd like to merge these somewhat soon, assuming there's a rough consensus on them, as this is a fairly hot set, touching 136 files. Note: the big Coccinelle-generated patch likely won't make it to the list. You can find it here: <URL:https://pilvi.retiisi.eu/s/j3LjF7FmGAmadDL/download/v4-0006-media-v4l-subdev-Switch-to-stream-aware-state-fun.patch>. since v9: - Rebase on Starfive ISP driver patches. - Fix kerneldoc issues in sub-device state access functions. since v7: - Include one more patch, to move the sub-device state access functions out a section conditional to sub-device UAPI. The patch has been rebased before the driver changes. since v6: - More driver changes missing in v6. since v5: - Fix Coccinelle-added space (not tab) indenting in the big patch. since v4: - Rework macros for two/three argument state information access functions. There's now a common function for generating the name while renames have been done according to the discussion on the list. - Don't warn about incorrect pad or stream being set for pad information access functions (dropped the last patch). The pad index is nowadays validated by the caller and so resulting NULL is now a driver bug, in other words we no longer need to prepare for driver not validating the field (as this is now done by the framework). - There are no changes to the big patch i.e. it's the same as in v4. since v3: - Drop the patch asserting state mutex acquisition for stream-unaware drivers. - Add a patch to warn about non-zero stream when access per-pad state information. - Added a comment on why pad 0 information was returned on invalid pad. - Add a patch to return NULL from per-pad state information access functions if pad is invalid. (The added comment is removed by this patch as well.) - Use only __VA_ARGS__ to implement variable number of arguments for subdev state information access functions. This should now work for all supported GCC versions. Add a comment to explain the macro. - Rename the subdev state information access functions to include "state" again, i.e. v4l2_subdev_state_get_{format,crop,compose}. - Try to avoid confusing git am by adding '> ' prefix for the manually created diff. since v2: - Add back a missing patch (adding sub-device pointer to the state). - Address unused variable warnings in a few sensor drivers. - Address kernel-doc build warnings (see lkp bot mail to v2). - Add a patch to assert holding the state lock also for stream-unaware users. - Elaborate manual changes in the commit message of the big Coccinelle-generated patch. since v1: - Fix missing changes in ds90ub913, TI CAL and IMX drivers. - Rename state access functions as v4l2_subdev_state_get_*(), i.e. added "state_" to the function names. - Instead of requiring stream-unaware users to specify the stream value, provide a wrapper macro to produce that, using variadic args. - Put struct v4l2_subdev pointer to sub-device state (instead of num_pads integer field). Sakari Ailus (9): media: v4l: subdev: Store the sub-device in the sub-device state media: v4l: subdev: Also return pads array information on stream functions media: v4l: subdev: Rename sub-device state information access functions media: v4l: subdev: v4l2_subdev_state_get_format always returns format now media: v4l: subdev: Make stream argument optional in state access functions media: v4l: subdev: Always compile sub-device state access functions media: v4l: subdev: Switch to stream-aware state functions media: v4l: subdev: Remove stream-unaware sub-device state access media: v4l: subdev: Return NULL from pad access functions on error drivers/media/i2c/adv7180.c | 4 +- drivers/media/i2c/adv7183.c | 2 +- drivers/media/i2c/adv748x/adv748x-afe.c | 6 +- drivers/media/i2c/adv748x/adv748x-csi2.c | 2 +- drivers/media/i2c/adv748x/adv748x-hdmi.c | 6 +- drivers/media/i2c/adv7511-v4l2.c | 4 +- drivers/media/i2c/adv7604.c | 4 +- drivers/media/i2c/adv7842.c | 4 +- drivers/media/i2c/ar0521.c | 5 +- drivers/media/i2c/ccs/ccs-core.c | 22 +- drivers/media/i2c/ds90ub913.c | 3 +- drivers/media/i2c/ds90ub953.c | 3 +- drivers/media/i2c/ds90ub960.c | 12 +- drivers/media/i2c/et8ek8/et8ek8_driver.c | 3 +- drivers/media/i2c/hi556.c | 13 +- drivers/media/i2c/hi846.c | 11 +- drivers/media/i2c/hi847.c | 9 +- drivers/media/i2c/imx208.c | 9 +- drivers/media/i2c/imx214.c | 4 +- drivers/media/i2c/imx219.c | 12 +- drivers/media/i2c/imx258.c | 9 +- drivers/media/i2c/imx274.c | 12 +- drivers/media/i2c/imx290.c | 8 +- drivers/media/i2c/imx296.c | 18 +- drivers/media/i2c/imx319.c | 7 +- drivers/media/i2c/imx334.c | 4 +- drivers/media/i2c/imx335.c | 4 +- drivers/media/i2c/imx355.c | 7 +- drivers/media/i2c/imx412.c | 4 +- drivers/media/i2c/imx415.c | 6 +- drivers/media/i2c/isl7998x.c | 6 +- drivers/media/i2c/max9286.c | 4 +- drivers/media/i2c/mt9m001.c | 6 +- drivers/media/i2c/mt9m111.c | 6 +- drivers/media/i2c/mt9m114.c | 58 ++--- drivers/media/i2c/mt9p031.c | 6 +- drivers/media/i2c/mt9t112.c | 2 +- drivers/media/i2c/mt9v011.c | 2 +- drivers/media/i2c/mt9v032.c | 10 +- drivers/media/i2c/mt9v111.c | 4 +- drivers/media/i2c/og01a1b.c | 10 +- drivers/media/i2c/ov01a10.c | 2 +- drivers/media/i2c/ov02a10.c | 6 +- drivers/media/i2c/ov08d10.c | 9 +- drivers/media/i2c/ov08x40.c | 7 +- drivers/media/i2c/ov13858.c | 10 +- drivers/media/i2c/ov13b10.c | 10 +- drivers/media/i2c/ov2640.c | 6 +- drivers/media/i2c/ov2659.c | 6 +- drivers/media/i2c/ov2680.c | 10 +- drivers/media/i2c/ov2685.c | 4 +- drivers/media/i2c/ov2740.c | 4 +- drivers/media/i2c/ov4689.c | 2 +- drivers/media/i2c/ov5640.c | 9 +- drivers/media/i2c/ov5645.c | 4 +- drivers/media/i2c/ov5647.c | 12 +- drivers/media/i2c/ov5648.c | 6 +- drivers/media/i2c/ov5670.c | 13 +- drivers/media/i2c/ov5675.c | 9 +- drivers/media/i2c/ov5693.c | 4 +- drivers/media/i2c/ov5695.c | 8 +- drivers/media/i2c/ov6650.c | 12 +- drivers/media/i2c/ov7251.c | 4 +- drivers/media/i2c/ov7670.c | 7 +- drivers/media/i2c/ov772x.c | 2 +- drivers/media/i2c/ov7740.c | 7 +- drivers/media/i2c/ov8856.c | 9 +- drivers/media/i2c/ov8858.c | 6 +- drivers/media/i2c/ov8865.c | 8 +- drivers/media/i2c/ov9282.c | 6 +- drivers/media/i2c/ov9640.c | 2 +- drivers/media/i2c/ov9650.c | 7 +- drivers/media/i2c/ov9734.c | 9 +- drivers/media/i2c/rj54n1cb0c.c | 2 +- drivers/media/i2c/s5c73m3/s5c73m3-core.c | 37 ++-- drivers/media/i2c/s5k5baf.c | 35 ++- drivers/media/i2c/s5k6a3.c | 8 +- drivers/media/i2c/saa6752hs.c | 2 +- drivers/media/i2c/st-mipid02.c | 11 +- drivers/media/i2c/st-vgxy61.c | 5 +- drivers/media/i2c/tc358746.c | 12 +- drivers/media/i2c/tda1997x.c | 6 +- drivers/media/i2c/tvp5150.c | 2 +- drivers/media/i2c/tw9910.c | 2 +- drivers/media/pci/intel/ipu3/ipu3-cio2.c | 10 +- drivers/media/pci/intel/ivsc/mei_csi.c | 4 +- drivers/media/platform/atmel/atmel-isi.c | 2 +- drivers/media/platform/cadence/cdns-csi2rx.c | 4 +- drivers/media/platform/cadence/cdns-csi2tx.c | 3 +- .../platform/microchip/microchip-csi2dc.c | 15 +- .../platform/microchip/microchip-isc-base.c | 2 +- .../platform/microchip/microchip-isc-scaler.c | 16 +- drivers/media/platform/nxp/imx-mipi-csis.c | 10 +- drivers/media/platform/nxp/imx7-media-csi.c | 16 +- .../platform/nxp/imx8-isi/imx8-isi-crossbar.c | 10 +- .../platform/nxp/imx8-isi/imx8-isi-pipe.c | 18 +- .../platform/nxp/imx8-isi/imx8-isi-video.c | 2 +- drivers/media/platform/nxp/imx8mq-mipi-csi2.c | 13 +- .../media/platform/qcom/camss/camss-csid.c | 3 +- .../media/platform/qcom/camss/camss-csiphy.c | 3 +- .../media/platform/qcom/camss/camss-ispif.c | 3 +- drivers/media/platform/qcom/camss/camss-vfe.c | 10 +- drivers/media/platform/renesas/rcar-isp.c | 4 +- .../platform/renesas/rcar-vin/rcar-csi2.c | 4 +- .../platform/renesas/rzg2l-cru/rzg2l-csi2.c | 6 +- .../platform/renesas/rzg2l-cru/rzg2l-ip.c | 6 +- .../media/platform/renesas/vsp1/vsp1_brx.c | 2 +- .../media/platform/renesas/vsp1/vsp1_entity.c | 8 +- .../media/platform/renesas/vsp1/vsp1_rwpf.c | 3 +- .../platform/rockchip/rkisp1/rkisp1-csi.c | 16 +- .../platform/rockchip/rkisp1/rkisp1-isp.c | 103 ++++----- .../platform/rockchip/rkisp1/rkisp1-resizer.c | 53 ++--- .../samsung/exynos4-is/fimc-capture.c | 12 +- .../platform/samsung/exynos4-is/fimc-isp.c | 24 +-- .../platform/samsung/exynos4-is/fimc-lite.c | 16 +- .../platform/samsung/exynos4-is/mipi-csis.c | 3 +- .../samsung/s3c-camif/camif-capture.c | 8 +- .../platform/sunxi/sun4i-csi/sun4i_v4l2.c | 8 +- .../sunxi/sun6i-csi/sun6i_csi_bridge.c | 8 +- .../sunxi/sun6i-mipi-csi2/sun6i_mipi_csi2.c | 8 +- .../sun8i_a83t_mipi_csi2.c | 8 +- drivers/media/platform/ti/cal/cal-camerarx.c | 18 +- drivers/media/platform/ti/cal/cal-video.c | 2 +- drivers/media/platform/ti/omap3isp/ispccdc.c | 7 +- drivers/media/platform/ti/omap3isp/ispccp2.c | 3 +- drivers/media/platform/ti/omap3isp/ispcsi2.c | 3 +- .../media/platform/ti/omap3isp/isppreview.c | 6 +- .../media/platform/ti/omap3isp/ispresizer.c | 5 +- drivers/media/platform/video-mux.c | 18 +- .../media/platform/xilinx/xilinx-csi2rxss.c | 5 +- drivers/media/platform/xilinx/xilinx-tpg.c | 9 +- drivers/media/platform/xilinx/xilinx-vip.c | 4 +- .../media/test-drivers/vimc/vimc-debayer.c | 10 +- drivers/media/test-drivers/vimc/vimc-scaler.c | 9 +- drivers/media/test-drivers/vimc/vimc-sensor.c | 6 +- drivers/media/v4l2-core/v4l2-subdev.c | 181 +++++++++------- .../media/atomisp/i2c/atomisp-gc0310.c | 2 +- .../media/atomisp/i2c/atomisp-gc2235.c | 2 +- .../media/atomisp/i2c/atomisp-mt9m114.c | 2 +- .../media/atomisp/i2c/atomisp-ov2722.c | 2 +- .../staging/media/atomisp/pci/atomisp_csi2.c | 3 +- .../media/atomisp/pci/atomisp_subdev.c | 6 +- .../staging/media/atomisp/pci/atomisp_tpg.c | 2 +- .../media/deprecated/atmel/atmel-isc-base.c | 2 +- drivers/staging/media/imx/imx-ic-prp.c | 4 +- drivers/staging/media/imx/imx-ic-prpencvf.c | 4 +- drivers/staging/media/imx/imx-media-csi.c | 8 +- drivers/staging/media/imx/imx-media-utils.c | 2 +- drivers/staging/media/imx/imx-media-vdic.c | 2 +- drivers/staging/media/imx/imx6-mipi-csi2.c | 2 +- drivers/staging/media/ipu3/ipu3-v4l2.c | 14 +- drivers/staging/media/omap4iss/iss_csi2.c | 3 +- drivers/staging/media/omap4iss/iss_ipipe.c | 3 +- drivers/staging/media/omap4iss/iss_ipipeif.c | 3 +- drivers/staging/media/omap4iss/iss_resizer.c | 3 +- .../staging/media/starfive/camss/stf-isp.c | 25 ++- .../media/sunxi/sun6i-isp/sun6i_isp_proc.c | 8 +- drivers/staging/media/tegra-video/vi.c | 2 +- include/media/v4l2-subdev.h | 204 +++++++----------- 159 files changed, 802 insertions(+), 909 deletions(-)