linux-kernel - Re: [PATCH v2] media: v4l2-subdev: Refactor events

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20241028183657.GE26852@pendragon.ideasonboard.com>
Date: Mon, 28 Oct 2024 20:36:57 +0200
From: Laurent Pinchart <laurent.pinchart@...asonboard.com>
To: Tommaso Merciai <tomm.merciai@...il.com>
Cc: sakari.ailus@...ux.intel.com,
	Mauro Carvalho Chehab <mchehab@...nel.org>,
	Hans Verkuil <hverkuil@...all.nl>,
	Tomi Valkeinen <tomi.valkeinen@...asonboard.com>,
	Paweł Anikiel <panikiel@...gle.com>,
	linux-media@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] media: v4l2-subdev: Refactor events

Hi Tommaso,

On Mon, Oct 28, 2024 at 06:32:32PM +0100, Tommaso Merciai wrote:
> On Mon, Oct 21, 2024 at 10:30:34AM +0300, Laurent Pinchart wrote:
> > On Mon, Oct 21, 2024 at 08:35:53AM +0200, Tommaso Merciai wrote:
> > > On Sun, Oct 20, 2024 at 07:43:54PM +0300, Laurent Pinchart wrote:
> > > > On Sun, Oct 20, 2024 at 06:35:32PM +0200, Tommaso Merciai wrote:
> > > > > Controls can be exposed to userspace via a v4l-subdevX device, and
> > > > > userspace has to be able to subscribe to control events so that it is
> > > > > notified when the control changes value.
> > > > > If a control handler is set for the subdev then set the HAS_EVENTS
> > > > > flag automatically into v4l2_subdev_init_finalize() and use
> > > > > v4l2_ctrl_subdev_subscribe_event() and v4l2_event_subdev_unsubscribe()
> > > > > as default if subdev don't have .(un)subscribe control operations.
> > > > 
> > > > I would add here
> > > > 
> > > > This simplifies subdev drivers by avoiding the need to set the
> > > > V4L2_SUBDEV_FL_HAS_EVENTS flag and plug the event handlers, and ensures
> > > > consistency of the API exposed to userspace.
> > > > 
> > > > And you can also add
> > > > 
> > > > Suggested-by: Laurent Pinchart <laurent.pinchart@...asonboard.com>
> > > > 
> > > > > Signed-off-by: Tommaso Merciai <tomm.merciai@...il.com>
> > > > 
> > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@...asonboard.com>
> > > 
> > > Oks, Thanks again.
> > > 
> > > > Now, can we simplify sensor drivers to drop the event handlers and the
> > > > flag ? :-)
> > > 
> > > Yep, plan is add all to support v4l2_subdev_init_finalize()
> > > Removing:
> > > 
> > >  .subscribe_event = v4l2_ctrl_subdev_subscribe_event,
> > >  .unsubscribe_event = v4l2_event_subdev_unsubscribe,
> > > 
> > > if are used. And ofc V4L2_SUBDEV_FL_HAS_EVENTS.
> > 
> > What I meant is looking at the I2C sensor drivers that currently
> > 
> > - call v4l2_subdev_init_finalize()
> > - set V4L2_SUBDEV_FL_HAS_EVENTS
> > - set the .subscribe_event() and .unsubscribe_event() handlers
> > 
> > and dropping the flag and handlers from them. Is that what you plan to
> > work on ?
> 
> It's ok for you per/driver patch or you prefer a big single patch?

I'm fine either way. Maybe one large patch to address all the drivers
where the flag and handlers are simply dropped, and then one patch per
driver where changes are larger (such as adding calls to
v4l2_subdev_init_finalize()) ?

> Meanwhile I've prepared something here:
> 
> https://gitlab.freedesktop.org/linux-media/users/tmerciai/-/compare/next...v6.12.0-rc1-nxp?from_project_id=22111
> 
> Let me know if you prefer (un)squashed version.
> Thanks in advance. :)
> 
> Thanks & Regards,
> Tommaso
> 
> > 
> > > Meanwhile I think I will send v3 with your
> > > suggestions. :)
> > > 
> > > > > ---
> > > > > Changes since v1:
> > > > >  - Aligned event subscription with unsubscription as suggested by LPinchart,
> > > > >    SAilus
> > > > > 
> > > > >  drivers/media/v4l2-core/v4l2-subdev.c | 22 ++++++++++++++++++++--
> > > > >  1 file changed, 20 insertions(+), 2 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> > > > > index 3a4ba08810d2..fad8fa1f63e8 100644
> > > > > --- a/drivers/media/v4l2-core/v4l2-subdev.c
> > > > > +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> > > > > @@ -691,10 +691,25 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg,
> > > > >  		return v4l2_event_dequeue(vfh, arg, file->f_flags & O_NONBLOCK);
> > > > >  
> > > > >  	case VIDIOC_SUBSCRIBE_EVENT:
> > > > > -		return v4l2_subdev_call(sd, core, subscribe_event, vfh, arg);
> > > > > +		if (v4l2_subdev_has_op(sd, core, subscribe_event))
> > > > > +			return v4l2_subdev_call(sd, core, subscribe_event,
> > > > > +						vfh, arg);
> > > > > +
> > > > > +		if ((sd->flags & V4L2_SUBDEV_FL_HAS_EVENTS) &&
> > > > > +		    vfh->ctrl_handler)
> > > > > +			return v4l2_ctrl_subdev_subscribe_event(sd, vfh, arg);
> > > > > +
> > > > > +		return -ENOIOCTLCMD;
> > > > >  
> > > > >  	case VIDIOC_UNSUBSCRIBE_EVENT:
> > > > > -		return v4l2_subdev_call(sd, core, unsubscribe_event, vfh, arg);
> > > > > +		if (v4l2_subdev_has_op(sd, core, unsubscribe_event))
> > > > > +			return v4l2_subdev_call(sd, core, unsubscribe_event,
> > > > > +						vfh, arg);
> > > > > +
> > > > > +		if (sd->flags & V4L2_SUBDEV_FL_HAS_EVENTS)
> > > > > +			return v4l2_event_subdev_unsubscribe(sd, vfh, arg);
> > > > > +
> > > > > +		return -ENOIOCTLCMD;
> > > > >  
> > > > >  #ifdef CONFIG_VIDEO_ADV_DEBUG
> > > > >  	case VIDIOC_DBG_G_REGISTER:
> > > > > @@ -1641,6 +1656,9 @@ int __v4l2_subdev_init_finalize(struct v4l2_subdev *sd, const char *name,
> > > > >  		}
> > > > >  	}
> > > > >  
> > > > > +	if (sd->ctrl_handler)
> > > > > +		sd->flags |= V4L2_SUBDEV_FL_HAS_EVENTS;
> > > > > +
> > > > >  	state = __v4l2_subdev_state_alloc(sd, name, key);
> > > > >  	if (IS_ERR(state))
> > > > >  		return PTR_ERR(state);

-- 
Regards,

Laurent Pinchart