From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To: Sakari Ailus <sakari.ailus@linux.intel.com>
Cc: "Julien Massot" <julien.massot@collabora.com>,
"Mauro Carvalho Chehab" <mchehab@kernel.org>,
"Tomi Valkeinen" <tomi.valkeinen@ideasonboard.com>,
"Jacopo Mondi" <jacopo+renesas@jmondi.org>,
"Kieran Bingham" <kieran.bingham+renesas@ideasonboard.com>,
"Niklas Söderlund" <niklas.soderlund+renesas@ragnatech.se>,
"Benjamin Mugnier" <benjamin.mugnier@foss.st.com>,
"Sylvain Petinot" <sylvain.petinot@foss.st.com>,
"Yong Zhi" <yong.zhi@intel.com>,
"Bingbu Cao" <bingbu.cao@intel.com>,
"Dan Scally" <djrscally@gmail.com>,
"Tianshu Qiu" <tian.shu.qiu@intel.com>,
"Eugen Hristev" <eugen.hristev@collabora.com>,
"Nicolas Ferre" <nicolas.ferre@microchip.com>,
"Alexandre Belloni" <alexandre.belloni@bootlin.com>,
"Claudiu Beznea" <claudiu.beznea@tuxon.dev>,
"Maxime Ripard" <mripard@kernel.org>,
"Rui Miguel Silva" <rmfrfs@gmail.com>,
"Martin Kepplinger" <martink@posteo.de>,
"Purism Kernel Team" <kernel@puri.sm>,
"Shawn Guo" <shawnguo@kernel.org>,
"Sascha Hauer" <s.hauer@pengutronix.de>,
"Pengutronix Kernel Team" <kernel@pengutronix.de>,
"Fabio Estevam" <festevam@gmail.com>,
"Robert Foss" <rfoss@kernel.org>,
"Todor Tomov" <todor.too@gmail.com>,
"Bryan O'Donoghue" <bryan.odonoghue@linaro.org>,
"Bjorn Andersson" <andersson@kernel.org>,
"Konrad Dybcio" <konrad.dybcio@linaro.org>,
"Fabrizio Castro" <fabrizio.castro.jz@renesas.com>,
"Dafna Hirschfeld" <dafna@fastmail.com>,
"Heiko Stuebner" <heiko@sntech.de>,
"Sylwester Nawrocki" <s.nawrocki@samsung.com>,
"Krzysztof Kozlowski" <krzysztof.kozlowski@linaro.org>,
"Alim Akhtar" <alim.akhtar@samsung.com>,
"Hugues Fruchet" <hugues.fruchet@foss.st.com>,
"Alain Volmat" <alain.volmat@foss.st.com>,
"Maxime Coquelin" <mcoquelin.stm32@gmail.com>,
"Alexandre Torgue" <alexandre.torgue@foss.st.com>,
"Chen-Yu Tsai" <wens@csie.org>,
"Jernej Skrabec" <jernej.skrabec@gmail.com>,
"Samuel Holland" <samuel@sholland.org>,
"Yong Deng" <yong.deng@magewell.com>,
"Paul Kocialkowski" <paul.kocialkowski@bootlin.com>,
"Benoit Parrot" <bparrot@ti.com>, "Jai Luthra" <j-luthra@ti.com>,
"Philipp Zabel" <p.zabel@pengutronix.de>,
"Michal Simek" <michal.simek@amd.com>,
"Greg Kroah-Hartman" <gregkh@linuxfoundation.org>,
"Thierry Reding" <thierry.reding@gmail.com>,
"Jonathan Hunter" <jonathanh@nvidia.com>,
"Sowjanya Komatineni" <skomatineni@nvidia.com>,
"Luca Ceresoli" <luca.ceresoli@bootlin.com>,
linux-media@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org, imx@lists.linux.dev,
linux-arm-msm@vger.kernel.org, linux-renesas-soc@vger.kernel.org,
linux-rockchip@lists.infradead.org,
linux-samsung-soc@vger.kernel.org,
linux-stm32@st-md-mailman.stormreply.com,
linux-sunxi@lists.linux.dev, linux-staging@lists.linux.dev,
linux-tegra@vger.kernel.org
Subject: Re: [PATCH 0/2] Introduce v4l2_async_nf_unregister_cleanup
Date: Sat, 4 May 2024 17:18:45 +0300 [thread overview]
Message-ID: <20240504141845.GC24548@pendragon.ideasonboard.com> (raw)
In-Reply-To: <ZjO-JDBdHXVLbz5H@kekkonen.localdomain>
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(¬ifier);
> > > > > v4l2_async_nf_cleanup(¬ifier);
> > > > >
> > > > > 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
prev parent reply other threads:[~2024-05-04 14:18 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-05-02 15:22 [PATCH 0/2] Introduce v4l2_async_nf_unregister_cleanup Julien Massot
2024-05-02 15:22 ` [PATCH 1/2] media: v4l: async: Add v4l2_async_nf_unregister_cleanup Julien Massot
2024-05-02 15:22 ` [PATCH 2/2] media: convert all drivers to use v4l2_async_nf_unregister_cleanup Julien Massot
2024-05-02 15:56 ` [PATCH 0/2] Introduce v4l2_async_nf_unregister_cleanup Laurent Pinchart
2024-05-02 16:01 ` Sakari Ailus
2024-05-02 16:08 ` Laurent Pinchart
2024-05-02 16:24 ` Sakari Ailus
2024-05-04 14:18 ` Laurent Pinchart [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20240504141845.GC24548@pendragon.ideasonboard.com \
--to=laurent.pinchart@ideasonboard.com \
--cc=alain.volmat@foss.st.com \
--cc=alexandre.belloni@bootlin.com \
--cc=alexandre.torgue@foss.st.com \
--cc=alim.akhtar@samsung.com \
--cc=andersson@kernel.org \
--cc=benjamin.mugnier@foss.st.com \
--cc=bingbu.cao@intel.com \
--cc=bparrot@ti.com \
--cc=bryan.odonoghue@linaro.org \
--cc=claudiu.beznea@tuxon.dev \
--cc=dafna@fastmail.com \
--cc=djrscally@gmail.com \
--cc=eugen.hristev@collabora.com \
--cc=fabrizio.castro.jz@renesas.com \
--cc=festevam@gmail.com \
--cc=gregkh@linuxfoundation.org \
--cc=heiko@sntech.de \
--cc=hugues.fruchet@foss.st.com \
--cc=imx@lists.linux.dev \
--cc=j-luthra@ti.com \
--cc=jacopo+renesas@jmondi.org \
--cc=jernej.skrabec@gmail.com \
--cc=jonathanh@nvidia.com \
--cc=julien.massot@collabora.com \
--cc=kernel@pengutronix.de \
--cc=kernel@puri.sm \
--cc=kieran.bingham+renesas@ideasonboard.com \
--cc=konrad.dybcio@linaro.org \
--cc=krzysztof.kozlowski@linaro.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-arm-msm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-media@vger.kernel.org \
--cc=linux-renesas-soc@vger.kernel.org \
--cc=linux-rockchip@lists.infradead.org \
--cc=linux-samsung-soc@vger.kernel.org \
--cc=linux-staging@lists.linux.dev \
--cc=linux-stm32@st-md-mailman.stormreply.com \
--cc=linux-sunxi@lists.linux.dev \
--cc=linux-tegra@vger.kernel.org \
--cc=luca.ceresoli@bootlin.com \
--cc=martink@posteo.de \
--cc=mchehab@kernel.org \
--cc=mcoquelin.stm32@gmail.com \
--cc=michal.simek@amd.com \
--cc=mripard@kernel.org \
--cc=nicolas.ferre@microchip.com \
--cc=niklas.soderlund+renesas@ragnatech.se \
--cc=p.zabel@pengutronix.de \
--cc=paul.kocialkowski@bootlin.com \
--cc=rfoss@kernel.org \
--cc=rmfrfs@gmail.com \
--cc=s.hauer@pengutronix.de \
--cc=s.nawrocki@samsung.com \
--cc=sakari.ailus@linux.intel.com \
--cc=samuel@sholland.org \
--cc=shawnguo@kernel.org \
--cc=skomatineni@nvidia.com \
--cc=sylvain.petinot@foss.st.com \
--cc=thierry.reding@gmail.com \
--cc=tian.shu.qiu@intel.com \
--cc=todor.too@gmail.com \
--cc=tomi.valkeinen@ideasonboard.com \
--cc=wens@csie.org \
--cc=yong.deng@magewell.com \
--cc=yong.zhi@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).