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.pinch...@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.pinch...@ideasonboard.com>
> >>>>> Signed-off-by: Jacopo Mondi <jacopo.mo...@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
_______________________________________________
Mjpeg-users mailing list
Mjpeg-users@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/mjpeg-users
