Date: Sat, 4 May 2024 17:18:45 +0300
From: Laurent Pinchart <laurent.pinchart@...asonboard.com>
To: Sakari Ailus <sakari.ailus@...ux.intel.com>
Cc: Julien Massot <julien.massot@...labora.com>,
	Mauro Carvalho Chehab <mchehab@...nel.org>,
	Tomi Valkeinen <tomi.valkeinen@...asonboard.com>,
	Jacopo Mondi <jacopo+renesas@...ndi.org>,
	Kieran Bingham <kieran.bingham+renesas@...asonboard.com>,
	Niklas Söderlund <niklas.soderlund+renesas@...natech.se>,
	Benjamin Mugnier <benjamin.mugnier@...s.st.com>,
	Sylvain Petinot <sylvain.petinot@...s.st.com>,
	Yong Zhi <yong.zhi@...el.com>, Bingbu Cao <bingbu.cao@...el.com>,
	Dan Scally <djrscally@...il.com>,
	Tianshu Qiu <tian.shu.qiu@...el.com>,
	Eugen Hristev <eugen.hristev@...labora.com>,
	Nicolas Ferre <nicolas.ferre@...rochip.com>,
	Alexandre Belloni <alexandre.belloni@...tlin.com>,
	Claudiu Beznea <claudiu.beznea@...on.dev>,
	Maxime Ripard <mripard@...nel.org>,
	Rui Miguel Silva <rmfrfs@...il.com>,
	Martin Kepplinger <martink@...teo.de>,
	Purism Kernel Team <kernel@...i.sm>,
	Shawn Guo <shawnguo@...nel.org>,
	Sascha Hauer <s.hauer@...gutronix.de>,
	Pengutronix Kernel Team <kernel@...gutronix.de>,
	Fabio Estevam <festevam@...il.com>, Robert Foss <rfoss@...nel.org>,
	Todor Tomov <todor.too@...il.com>,
	Bryan O'Donoghue <bryan.odonoghue@...aro.org>,
	Bjorn Andersson <andersson@...nel.org>,
	Konrad Dybcio <konrad.dybcio@...aro.org>,
	Fabrizio Castro <fabrizio.castro.jz@...esas.com>,
	Dafna Hirschfeld <dafna@...tmail.com>,
	Heiko Stuebner <heiko@...ech.de>,
	Sylwester Nawrocki <s.nawrocki@...sung.com>,
	Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>,
	Alim Akhtar <alim.akhtar@...sung.com>,
	Hugues Fruchet <hugues.fruchet@...s.st.com>,
	Alain Volmat <alain.volmat@...s.st.com>,
	Maxime Coquelin <mcoquelin.stm32@...il.com>,
	Alexandre Torgue <alexandre.torgue@...s.st.com>,
	Chen-Yu Tsai <wens@...e.org>,
	Jernej Skrabec <jernej.skrabec@...il.com>,
	Samuel Holland <samuel@...lland.org>,
	Yong Deng <yong.deng@...ewell.com>,
	Paul Kocialkowski <paul.kocialkowski@...tlin.com>,
	Benoit Parrot <bparrot@...com>, Jai Luthra <j-luthra@...com>,
	Philipp Zabel <p.zabel@...gutronix.de>,
	Michal Simek <michal.simek@....com>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Thierry Reding <thierry.reding@...il.com>,
	Jonathan Hunter <jonathanh@...dia.com>,
	Sowjanya Komatineni <skomatineni@...dia.com>,
	Luca Ceresoli <luca.ceresoli@...tlin.com>,
	linux-media@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-arm-kernel@...ts.infradead.org, imx@...ts.linux.dev,
	linux-arm-msm@...r.kernel.org, linux-renesas-soc@...r.kernel.org,
	linux-rockchip@...ts.infradead.org,
	linux-samsung-soc@...r.kernel.org,
	linux-stm32@...md-mailman.stormreply.com,
	linux-sunxi@...ts.linux.dev, linux-staging@...ts.linux.dev,
	linux-tegra@...r.kernel.org
Subject: Re: [PATCH 0/2] Introduce v4l2_async_nf_unregister_cleanup

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

Powered by blists - more mailing lists