Hi Julien, On Thu, May 02, 2024 at 05:22:20PM +0200, Julien Massot wrote: > Many drivers has > v4l2_async_nf_unregister(¬ifier); > v4l2_async_nf_cleanup(¬ifier); > > Introduce a helper function to call both functions in one line. Does this really go in the right direction ? For other objects (video devices, media devices, ...), the unregistration should be done at .remove() time, and the cleanup at .release() time (the operation called when the last reference to the object is released). This is needed to ensure proper lifetime management of the objects, and avoid a use-after-free for objects that can be reached from userspace. It could be argued that the notifier isn't exposed to userspace, but can we guarantee that no driver will have a need to access the notifier in a code path triggered by a userspace operation ? I think it would be safer to adopt the same split for the nofifier unregistration and cleanup. In my opinion using the same rule across different APIs also make it easier for driver authors and for reviewers to get it right. As shown by your series, lots of drivers call v4l2_async_nf_cleanup() and .remove() time instead of .release(). That's because most drivers get lifetime management wrong and don't even implement .release(). That's something Sakari is addressing with ongoing work. This patch series seems to go in the opposite direction. > --- > Julien Massot (2): > media: v4l: async: Add v4l2_async_nf_unregister_cleanup > media: convert all drivers to use v4l2_async_nf_unregister_cleanup > > drivers/media/i2c/ds90ub913.c | 10 ++-------- > drivers/media/i2c/ds90ub953.c | 10 ++-------- > drivers/media/i2c/ds90ub960.c | 10 ++-------- > drivers/media/i2c/max9286.c | 3 +-- > drivers/media/i2c/st-mipid02.c | 6 ++---- > drivers/media/i2c/tc358746.c | 3 +-- > drivers/media/pci/intel/ipu3/ipu3-cio2.c | 6 ++---- > drivers/media/pci/intel/ipu6/ipu6-isys.c | 8 +------- > drivers/media/pci/intel/ivsc/mei_csi.c | 6 ++---- > drivers/media/platform/atmel/atmel-isi.c | 3 +-- > drivers/media/platform/cadence/cdns-csi2rx.c | 6 ++---- > drivers/media/platform/intel/pxa_camera.c | 3 +-- > drivers/media/platform/marvell/mcam-core.c | 6 ++---- > drivers/media/platform/microchip/microchip-csi2dc.c | 3 +-- > drivers/media/platform/microchip/microchip-isc-base.c | 6 ++---- > drivers/media/platform/nxp/imx-mipi-csis.c | 6 ++---- > drivers/media/platform/nxp/imx7-media-csi.c | 3 +-- > drivers/media/platform/nxp/imx8-isi/imx8-isi-core.c | 3 +-- > drivers/media/platform/nxp/imx8mq-mipi-csi2.c | 6 ++---- > drivers/media/platform/qcom/camss/camss.c | 3 +-- > drivers/media/platform/renesas/rcar-csi2.c | 6 ++---- > drivers/media/platform/renesas/rcar-isp.c | 6 ++---- > drivers/media/platform/renesas/rcar-vin/rcar-core.c | 9 +++------ > drivers/media/platform/renesas/rcar_drif.c | 3 +-- > drivers/media/platform/renesas/renesas-ceu.c | 4 +--- > drivers/media/platform/renesas/rzg2l-cru/rzg2l-core.c | 3 +-- > drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c | 6 ++---- > drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c | 3 +-- > drivers/media/platform/samsung/exynos4-is/media-dev.c | 3 +-- > drivers/media/platform/st/stm32/stm32-dcmi.c | 3 +-- > .../media/platform/st/stm32/stm32-dcmipp/dcmipp-core.c | 3 +-- > drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.c | 3 +-- > .../media/platform/sunxi/sun6i-csi/sun6i_csi_bridge.c | 3 +-- > .../platform/sunxi/sun6i-mipi-csi2/sun6i_mipi_csi2.c | 3 +-- > .../sunxi/sun8i-a83t-mipi-csi2/sun8i_a83t_mipi_csi2.c | 3 +-- > drivers/media/platform/ti/am437x/am437x-vpfe.c | 3 +-- > drivers/media/platform/ti/cal/cal.c | 8 +------- > drivers/media/platform/ti/davinci/vpif_capture.c | 3 +-- > drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c | 10 ++-------- > drivers/media/platform/ti/omap3isp/isp.c | 3 +-- > drivers/media/platform/video-mux.c | 3 +-- > drivers/media/platform/xilinx/xilinx-vipp.c | 3 +-- > drivers/staging/media/deprecated/atmel/atmel-isc-base.c | 6 ++---- > drivers/staging/media/sunxi/sun6i-isp/sun6i_isp_proc.c | 3 +-- > drivers/staging/media/tegra-video/vi.c | 3 +-- > include/media/v4l2-async.h | 17 +++++++++++++++++ > 46 files changed, 80 insertions(+), 153 deletions(-) > --- > base-commit: 843a9f4a7a85988f2f3af98adf21797c2fd05ab1 > change-id: 20240502-master-5deee133b4f5 -- Regards, Laurent Pinchart
