From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from perceval.ideasonboard.com (perceval.ideasonboard.com [213.167.242.64]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 25E801F1306; Fri, 8 Aug 2025 06:30:30 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=213.167.242.64 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1754634634; cv=none; b=geodAv2WxrTvjSx5F+XDmoTy68puTiqH1EQZGu+XoiliyH8QgAtwSsHo1VCkfFVutyvbVi4QpFyIyZ4mNipJ/a9OT7VzVYzzYd6AVuVZnF7/Z9M/e5jr8j4O3A5gt7ZUYvVBZYZLTxAHp1dyi69/B00My0RJ0IYR1XePs0Dvzf8= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1754634634; c=relaxed/simple; bh=2sZclBUIchMgOI2BfV6AOvaczH7sNa7CyFdTx87lVcE=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=WJsST49+5uog5kV2/lr94XjSVUpPOiviZ1BtMsrxc9O1FQHuOMbY9YkiqDxgbRyUDi79p6AUiDG/tsnP1EUqXTWNuBYyNzM19FN1bNNM+pOFKaLqdrjGOtEz5Kzbp/uqIsy/rXsiKC/xlWl5DxuhonkOhx8TnSoEuWpdTPKzqoo= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=ideasonboard.com; spf=pass smtp.mailfrom=ideasonboard.com; dkim=pass (1024-bit key) header.d=ideasonboard.com header.i=@ideasonboard.com header.b=oumdgnpI; arc=none smtp.client-ip=213.167.242.64 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=ideasonboard.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=ideasonboard.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=ideasonboard.com header.i=@ideasonboard.com header.b="oumdgnpI" Received: from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi [81.175.209.231]) by perceval.ideasonboard.com (Postfix) with UTF8SMTPSA id 5463C1648; Fri, 8 Aug 2025 08:29:36 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1754634577; bh=2sZclBUIchMgOI2BfV6AOvaczH7sNa7CyFdTx87lVcE=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=oumdgnpIcDsNg0djHYTq38XzQUF1TkgCzet42UojtjhitVip2QZCrbg2zqqcByvlX baY/CA6Dhgx81mz9UqbNHHcJrX7kQLHr2Qwr0A9FfGL8DNPlEjbIJ+qzZwZn4Bu7WR q6Pt/CjukSiWCSaMGBxXRzbtMbKI+d/wrxnUndSo= Date: Fri, 8 Aug 2025 09:30:11 +0300 From: Laurent Pinchart To: Hans Verkuil Cc: Jacopo Mondi , Mauro Carvalho Chehab , Devarsh Thakkar , Benoit Parrot , Hans Verkuil , Mike Isely , Hans de Goede , Parthiban Veerasooran , Christian Gromm , Greg Kroah-Hartman , Alex Shi , Yanteng Si , Dongliang Mu , Jonathan Corbet , Tomasz Figa , Marek Szyprowski , Andy Walls , Michael Tretter , Pengutronix Kernel Team , Bin Liu , Matthias Brugger , AngeloGioacchino Del Regno , Dmitry Osipenko , Thierry Reding , Jonathan Hunter , Mirela Rabulea , Shawn Guo , Sascha Hauer , Fabio Estevam , Kieran Bingham , Michal Simek , Ming Qian , Zhou Peng , Xavier Roumegue , Philipp Zabel , Vikash Garodia , Dikshita Agarwal , Abhinav Kumar , Bryan O'Donoghue , Sylwester Nawrocki , Jernej Skrabec , Chen-Yu Tsai , Samuel Holland , Daniel Almeida , Neil Armstrong , Kevin Hilman , Jerome Brunet , Martin Blumenstingl , Nas Chung , Jackson Lee , Minghsiu Tsai , Houlong Wei , Andrew-CT Chen , Tiffany Lin , Yunfei Dong , Geert Uytterhoeven , Magnus Damm , Mikhail Ulyanov , Jacob Chen , Ezequiel Garcia , Heiko Stuebner , Detlev Casanova , Krzysztof Kozlowski , Alim Akhtar , Sylwester Nawrocki , =?utf-8?Q?=C5=81ukasz?= Stelmach , Andrzej Pietrasiewicz , Jacek Anaszewski , Andrzej Hajda , Fabien Dessenne , Hugues Fruchet , Jean-Christophe Trotin , Maxime Coquelin , Alexandre Torgue , Nicolas Dufresne , Benjamin Gaignard , Steve Longerbeam , Maxime Ripard , Paul Kocialkowski , Niklas =?utf-8?Q?S=C3=B6derlund?= , Robert Foss , Todor Tomov , Vladimir Zapolskiy , Corentin Labbe , Sakari Ailus , Bingbu Cao , Tianshu Qiu , Stanislaw Gruszka , linux-media@vger.kernel.org, linux-kernel@vger.kernel.org, linux-staging@lists.linux.dev, linux-doc@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-mediatek@lists.infradead.org, linux-tegra@vger.kernel.org, imx@lists.linux.dev, linux-renesas-soc@vger.kernel.org, linux-arm-msm@vger.kernel.org, linux-samsung-soc@vger.kernel.org, linux-sunxi@lists.linux.dev, linux-usb@vger.kernel.org, linux-amlogic@lists.infradead.org, linux-rockchip@lists.infradead.org, linux-stm32@st-md-mailman.stormreply.com, mjpeg-users@lists.sourceforge.net Subject: Re: [PATCH 27/65] media: Reset file->private_data to NULL in v4l2_fh_del() Message-ID: <20250808063011.GJ11583@pendragon.ideasonboard.com> References: <20250802-media-private-data-v1-0-eb140ddd6a9d@ideasonboard.com> <20250802-media-private-data-v1-27-eb140ddd6a9d@ideasonboard.com> <20250807085003.GE11583@pendragon.ideasonboard.com> <20250807170004.GG11583@pendragon.ideasonboard.com> <20250807202553.GB28610@pendragon.ideasonboard.com> <7605f778-6b20-47e4-bd65-7a0d85fff736@kernel.org> Precedence: bulk X-Mailing-List: linux-usb@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <7605f778-6b20-47e4-bd65-7a0d85fff736@kernel.org> On Thu, Aug 07, 2025 at 10:51:27PM +0200, Hans Verkuil wrote: > On 07/08/2025 22:25, Laurent Pinchart wrote: > > On Thu, Aug 07, 2025 at 08:00:06PM +0300, Laurent Pinchart wrote: > >> On Thu, Aug 07, 2025 at 11:50:07AM +0300, Laurent Pinchart wrote: > >>> On Wed, Aug 06, 2025 at 02:45:14PM +0200, Hans Verkuil wrote: > >>>> On 02/08/2025 11:22, Jacopo Mondi wrote: > >>>>> From: Laurent Pinchart > >>>>> > >>>>> Multiple drivers that use v4l2_fh and call v4l2_fh_del() manually reset > >>>>> the file->private_data pointer to NULL in their video device .release() > >>>>> file operation handler. Move the code to the v4l2_fh_del() function to > >>>>> avoid direct access to file->private_data in drivers. This requires > >>>>> adding a file pointer argument to the function. > >>>>> > >>>>> Changes to drivers have been generated with the following coccinelle > >>>>> semantic patch: > >>>>> > >>>>> @@ > >>>>> expression fh; > >>>>> identifier filp; > >>>>> identifier release; > >>>>> type ret; > >>>>> @@ > >>>>> ret release(..., struct file *filp, ...) > >>>>> { > >>>>> <... > >>>>> - filp->private_data = NULL; > >>>>> ... > >>>>> - v4l2_fh_del(fh); > >>>>> + v4l2_fh_del(fh, filp); > >>>>> ...> > >>>>> } > >>>>> > >>>>> @@ > >>>>> expression fh; > >>>>> identifier filp; > >>>>> identifier release; > >>>>> type ret; > >>>>> @@ > >>>>> ret release(..., struct file *filp, ...) > >>>>> { > >>>>> <... > >>>>> - v4l2_fh_del(fh); > >>>>> + v4l2_fh_del(fh, filp); > >>>>> ... > >>>>> - filp->private_data = NULL; > >>>>> ...> > >>>>> } > >>>>> > >>>>> @@ > >>>>> expression fh; > >>>>> identifier filp; > >>>>> identifier release; > >>>>> type ret; > >>>>> @@ > >>>>> ret release(..., struct file *filp, ...) > >>>>> { > >>>>> <... > >>>>> - v4l2_fh_del(fh); > >>>>> + v4l2_fh_del(fh, filp); > >>>>> ...> > >>>>> } > >>>>> > >>>>> Manual changes have been applied to Documentation/ to update the usage > >>>>> patterns, to drivers/media/v4l2-core/v4l2-fh.c to update the > >>>>> v4l2_fh_del() prototype and reset file->private_data, and to > >>>>> include/media/v4l2-fh.h to update the v4l2_fh_del() function prototype > >>>>> and its documentation. > >>>>> > >>>>> Additionally, white space issues have been fixed manually in > >>>>> drivers/usb/gadget/function/uvc_v4l2.c > >>>>> > >>>>> Signed-off-by: Laurent Pinchart > >>>>> Signed-off-by: Jacopo Mondi > >>>>> --- > >>>>> Documentation/driver-api/media/v4l2-fh.rst | 4 ++-- > >>>>> Documentation/translations/zh_CN/video4linux/v4l2-framework.txt | 4 ++-- > >>>>> drivers/media/pci/cx18/cx18-fileops.c | 4 ++-- > >>>>> drivers/media/pci/ivtv/ivtv-fileops.c | 4 ++-- > >>>>> drivers/media/pci/saa7164/saa7164-encoder.c | 2 +- > >>>>> drivers/media/pci/saa7164/saa7164-vbi.c | 2 +- > >>>>> drivers/media/platform/allegro-dvt/allegro-core.c | 2 +- > >>>>> drivers/media/platform/amlogic/meson-ge2d/ge2d.c | 2 +- > >>>>> drivers/media/platform/amphion/vpu_v4l2.c | 4 ++-- > >>>>> drivers/media/platform/chips-media/coda/coda-common.c | 4 ++-- > >>>>> drivers/media/platform/chips-media/wave5/wave5-helper.c | 2 +- > >>>>> drivers/media/platform/imagination/e5010-jpeg-enc.c | 4 ++-- > >>>>> drivers/media/platform/m2m-deinterlace.c | 2 +- > >>>>> drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.c | 4 ++-- > >>>>> drivers/media/platform/mediatek/mdp/mtk_mdp_m2m.c | 4 ++-- > >>>>> drivers/media/platform/mediatek/mdp3/mtk-mdp3-m2m.c | 4 ++-- > >>>>> .../media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_drv.c | 4 ++-- > >>>>> .../media/platform/mediatek/vcodec/encoder/mtk_vcodec_enc_drv.c | 4 ++-- > >>>>> drivers/media/platform/nvidia/tegra-vde/v4l2.c | 2 +- > >>>>> drivers/media/platform/nxp/dw100/dw100.c | 2 +- > >>>>> drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c | 4 ++-- > >>>>> drivers/media/platform/nxp/imx-pxp.c | 2 +- > >>>>> drivers/media/platform/nxp/imx8-isi/imx8-isi-m2m.c | 2 +- > >>>>> drivers/media/platform/nxp/mx2_emmaprp.c | 2 +- > >>>>> drivers/media/platform/qcom/iris/iris_vidc.c | 3 +-- > >>>>> drivers/media/platform/qcom/venus/core.c | 2 +- > >>>>> drivers/media/platform/renesas/rcar_fdp1.c | 2 +- > >>>>> drivers/media/platform/renesas/rcar_jpu.c | 4 ++-- > >>>>> drivers/media/platform/renesas/vsp1/vsp1_video.c | 2 +- > >>>>> drivers/media/platform/rockchip/rga/rga.c | 2 +- > >>>>> drivers/media/platform/rockchip/rkvdec/rkvdec.c | 2 +- > >>>>> drivers/media/platform/samsung/exynos-gsc/gsc-m2m.c | 4 ++-- > >>>>> drivers/media/platform/samsung/exynos4-is/fimc-m2m.c | 4 ++-- > >>>>> drivers/media/platform/samsung/s5p-g2d/g2d.c | 2 +- > >>>>> drivers/media/platform/samsung/s5p-jpeg/jpeg-core.c | 4 ++-- > >>>>> drivers/media/platform/samsung/s5p-mfc/s5p_mfc.c | 4 ++-- > >>>>> drivers/media/platform/st/sti/bdisp/bdisp-v4l2.c | 4 ++-- > >>>>> drivers/media/platform/st/sti/delta/delta-v4l2.c | 4 ++-- > >>>>> drivers/media/platform/st/sti/hva/hva-v4l2.c | 4 ++-- > >>>>> drivers/media/platform/st/stm32/dma2d/dma2d.c | 2 +- > >>>>> drivers/media/platform/sunxi/sun8i-di/sun8i-di.c | 2 +- > >>>>> drivers/media/platform/sunxi/sun8i-rotate/sun8i_rotate.c | 2 +- > >>>>> drivers/media/platform/ti/omap3isp/ispvideo.c | 5 ++--- > >>>>> drivers/media/platform/ti/vpe/vpe.c | 2 +- > >>>>> drivers/media/platform/verisilicon/hantro_drv.c | 4 ++-- > >>>>> drivers/media/test-drivers/vicodec/vicodec-core.c | 2 +- > >>>>> drivers/media/test-drivers/vim2m.c | 2 +- > >>>>> drivers/media/test-drivers/visl/visl-core.c | 2 +- > >>>>> drivers/media/usb/pvrusb2/pvrusb2-v4l2.c | 3 +-- > >>>>> drivers/media/v4l2-core/v4l2-fh.c | 7 ++++--- > >>>>> drivers/media/v4l2-core/v4l2-subdev.c | 5 ++--- > >>>>> drivers/staging/media/imx/imx-media-csc-scaler.c | 4 ++-- > >>>>> drivers/staging/media/meson/vdec/vdec.c | 2 +- > >>>>> drivers/staging/media/sunxi/cedrus/cedrus.c | 2 +- > >>>>> drivers/staging/most/video/video.c | 4 ++-- > >>>>> drivers/usb/gadget/function/uvc_v4l2.c | 3 +-- > >>>>> include/media/v4l2-fh.h | 5 ++++- > >>>>> 57 files changed, 89 insertions(+), 90 deletions(-) > >>>>> > >>>> > >>>> > >>>> > >>>>> diff --git a/drivers/media/v4l2-core/v4l2-fh.c b/drivers/media/v4l2-core/v4l2-fh.c > >>>>> index b59b1084d8cdf1b62da12879e21dbe56c2109648..df3ba9d4674bd25626cfcddc2d0cb28c233e3cc3 100644 > >>>>> --- a/drivers/media/v4l2-core/v4l2-fh.c > >>>>> +++ b/drivers/media/v4l2-core/v4l2-fh.c > >>>>> @@ -67,7 +67,7 @@ int v4l2_fh_open(struct file *filp) > >>>>> } > >>>>> EXPORT_SYMBOL_GPL(v4l2_fh_open); > >>>>> > >>>>> -void v4l2_fh_del(struct v4l2_fh *fh) > >>>>> +void v4l2_fh_del(struct v4l2_fh *fh, struct file *filp) > >>>> > >>>> Instead of adding a second argument, perhaps it is better to > >>>> just provide the filp pointer. After all, you can get the v4l2_fh > >>>> from filp->private_data. > >>>> > >>>> It simplifies the code a bit. > >>> > >>> That's an interesting idea. I'll give it a try. > >> > >> We end up with code like (e.g. in v4l2_fh_release(), with similar > >> constructs in lots of drivers) > >> > >> if (fh) { > >> v4l2_fh_del(filp); > >> v4l2_fh_exit(fh); > >> kfree(fh); > >> } > >> > >> compared to > >> > >> if (fh) { > >> v4l2_fh_del(fh, filp); > >> v4l2_fh_exit(fh); > >> kfree(fh); > >> } > >> > >> with the existing patch. I find the fact that v4l2_fh_del() takes a > >> different pointer than v4l2_fh_exit() a bit disturbing. If you think > >> it's better I'll drop the fh argument in v2. > > > > I gave it a try, and looking at the function prototype, its > > documentation, the imbalance with v4l2_fh_add(), and the code in the > > callers, I think keeping both arguments would look cleaner. Please tell > > me if you feel strongly about this, I can still submit a patch to drop > > the argument. It can very easily be scripted with coccinelle and doesn't > > conflict with the rest of the series, so it could also be done later. > > Looking at all the drivers that call v4l2_fh_del/exit I always see v4l2_fh_del() > directly followed by v4l2_fh_exit(). I think it would make a lot of sense to just > combine the two as a single function: v4l2_fh_del_exit(filp). > > That simplifies the code and solves the imbalance. I'll try that as a separate patch on top of this one. Keeping the two separate will ease review (both patches will be simpler, and will be generated by coccinelle), and will also make it easier to drop the second patch if we decide it's not the way to go. > >>>>> { > >>>>> unsigned long flags; > >>>>> > >>>>> @@ -75,6 +75,8 @@ void v4l2_fh_del(struct v4l2_fh *fh) > >>>>> list_del_init(&fh->list); > >>>>> spin_unlock_irqrestore(&fh->vdev->fh_lock, flags); > >>>>> v4l2_prio_close(fh->vdev->prio, fh->prio); > >>>>> + > >>>>> + filp->private_data = NULL; > >>>>> } > >>>>> EXPORT_SYMBOL_GPL(v4l2_fh_del); > >>>>> > >>>>> @@ -94,10 +96,9 @@ int v4l2_fh_release(struct file *filp) > >>>>> struct v4l2_fh *fh = file_to_v4l2_fh(filp); > >>>>> > >>>>> if (fh) { > >>>>> - v4l2_fh_del(fh); > >>>>> + v4l2_fh_del(fh, filp); > >>>>> v4l2_fh_exit(fh); > >>>>> kfree(fh); > >>>>> - filp->private_data = NULL; > >>>>> } > >>>>> return 0; > >>>>> } > >>>> > >>>> > >>>> > >>>>> diff --git a/include/media/v4l2-fh.h b/include/media/v4l2-fh.h > >>>>> index d8fcf49f10e09452b73499f4a9bd1285bc2835a5..5e4c761635120608e0b588e0b0daf63e69588d38 100644 > >>>>> --- a/include/media/v4l2-fh.h > >>>>> +++ b/include/media/v4l2-fh.h > >>>>> @@ -114,12 +114,15 @@ int v4l2_fh_open(struct file *filp); > >>>>> * v4l2_fh_del - Remove file handle from the list of file handles. > >>>>> * > >>>>> * @fh: pointer to &struct v4l2_fh > >>>>> + * @filp: pointer to &struct file associated with @fh > >>>>> + * > >>>>> + * The function resets filp->private_data to NULL. > >>>>> * > >>>>> * .. note:: > >>>>> * Must be called in v4l2_file_operations->release\(\) handler if the driver > >>>>> * uses &struct v4l2_fh. > >>>>> */ > >>>>> -void v4l2_fh_del(struct v4l2_fh *fh); > >>>>> +void v4l2_fh_del(struct v4l2_fh *fh, struct file *filp); > >>>>> > >>>>> /** > >>>>> * v4l2_fh_exit - Release resources related to a file handle. -- Regards, Laurent Pinchart