[PATCH 0/2] Introduce v4l2_async_nf_unregister_cleanup

[Laurent Pinchart]

On Thu, May 02, 2024 at 04:24:04PM +0000, Sakari Ailus wrote:
> On Thu, May 02, 2024 at 07:08:30PM +0300, Laurent Pinchart wrote:
> > On Thu, May 02, 2024 at 04:01:45PM +0000, Sakari Ailus wrote:
> > > On Thu, May 02, 2024 at 06:56:26PM +0300, Laurent Pinchart wrote:
> > > > On Thu, May 02, 2024 at 05:22:20PM +0200, Julien Massot wrote:
> > > > > Many drivers has
> > > > >   v4l2_async_nf_unregister(&notifier);
> > > > >   v4l2_async_nf_cleanup(&notifier);
> > > > > 
> > > > > 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.
> > > 
> > > This still avoids the driver authors feeling they need to implement wrapper
> > > functions for v4l2_async_nf_{unregister,cleanup}. I'd be in favour merging
> > > this.
> > > 
> > > I don't see this getting in the way of adding use counts as the code will
> > > need to be changed in any case.
> > 
> > Fixing the lifetime issues would essentially revert 2/2 and move the
> > v4l2_async_nf_cleanup() call to .remove(). I don't think providing a
> > helper that forces the cleanup at .remove() time is a good idea, it
> > gives a false sense of doing things right to drivers. This is the same
> > reason why devm_kzalloc() is so harmful, it gave the wrong message, and
> > created (or participated in) all those lifetime issues.
> 
> I still prefer having devm_*alloc() functions than having the drivers open
> coding the same -- with the same result. The frameworks won't enable doing
> this right at the moment and I don't think drivers (or us!) should be
> penalised for that.

I don't really see where the penalty is. What's the urgency to switch
from calling v4l2_async_nf_unregister() and v4l2_async_nf_cleanup() to a
helper that we know goes in the wrong direction ?

> The driver authors will only change what they do, with
> these patches or without, when told so. But we don't really have an
> alternative today.

There's already a .release() callback that can be used, and some drivers
use it.

> A similar situation exists with clk_unprepare() and clk_disable().

-- 
Regards,

Laurent Pinchart