Re: [PATCH 27/65] media: Reset file->private_data to NULL in v4l2_fh_del()

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

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 <laurent.pinchart@ideasonboard.com>
> >>>>>
> >>>>> 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 <laurent.pinchart@ideasonboard.com>
> >>>>> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> >>>>> ---
> >>>>>  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(-)
> >>>>>
> >>>>
> >>>> <snip>
> >>>>
> >>>>> 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;
> >>>>>  }
> >>>>
> >>>> <snip>
> >>>>
> >>>>> 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