Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Feedback-ID: i8771445c:Fastmail
Date:   Wed, 29 Mar 2023 21:46:09 +0200
From:   Maxime Ripard <maxime@cerno.tech>
To:     Jonathan Cameron <jic23@kernel.org>
Cc:     Matti Vaittinen <mazziesaccount@gmail.com>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>,
        "Rafael J. Wysocki" <rafael@kernel.org>,
        Brendan Higgins <brendan.higgins@linux.dev>,
        David Gow <davidgow@google.com>,
        Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
        Heikki Krogerus <heikki.krogerus@linux.intel.com>,
        linux-kernel@vger.kernel.org, linux-kselftest@vger.kernel.org,
        kunit-dev@googlegroups.com, Stephen Boyd <sboyd@kernel.org>,
        linux-iio@vger.kernel.org, Lars-Peter Clausen <lars@metafoo.de>
Subject: Re: [PATCH v5 1/8] drivers: kunit: Generic helpers for test device
 creation
Message-ID: <20230329194609.7u2hgidxdk34emsf@penduick>
References: <ZBtPhoelZo4U5jwC@kroah.com>
 <20230323101216.w56kz3rudlj23vab@houat>
 <ZBwoRgc2ICBJX/Lq@kroah.com>
 <8a03a6fb-39b9-cd17-cc10-ece71111357d@gmail.com>
 <20230323122925.kqdnomr7i46qnyo4@houat>
 <590189b3-42d9-ab12-fccd-37338595cb6f@gmail.com>
 <20230323163639.xtwpid2uunwnzai4@houat>
 <a0e8b1da-3645-4141-6518-e035ad80a23d@gmail.com>
 <20230324123157.bbwvfq4gsxnlnfwb@houat>
 <20230325175044.7bee9e7d@jic23-huawei>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
        protocol="application/pgp-signature"; boundary="l6s2nrpwzcaaz3jq"
Content-Disposition: inline
In-Reply-To: <20230325175044.7bee9e7d@jic23-huawei>
Precedence: bulk


--l6s2nrpwzcaaz3jq
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Sat, Mar 25, 2023 at 05:50:44PM +0000, Jonathan Cameron wrote:
> On Fri, 24 Mar 2023 13:31:57 +0100
> Maxime Ripard <maxime@cerno.tech> wrote:
>=20
> > On Fri, Mar 24, 2023 at 08:11:52AM +0200, Matti Vaittinen wrote:
> > > On 3/23/23 18:36, Maxime Ripard wrote: =20
> > > > On Thu, Mar 23, 2023 at 03:02:03PM +0200, Matti Vaittinen wrote: =
=20
> > > > > On 3/23/23 14:29, Maxime Ripard wrote: =20
> > > > > > On Thu, Mar 23, 2023 at 02:16:52PM +0200, Matti Vaittinen wrote:
> > > > > >=20
> > > > > > This is the description of what was happening:
> > > > > > https://lore.kernel.org/dri-devel/20221117165311.vovrc7usy4efiy=
tl@houat/ =20
> > > > >=20
> > > > > Thanks Maxime. Do I read this correcty. The devm_ unwinding not b=
eing done
> > > > > when root_device_register() is used is not because root_device_un=
register()
> > > > > would not trigger the unwinding - but rather because DRM code on =
top of this
> > > > > device keeps the refcount increased? =20
> > > >=20
> > > > There's a difference of behaviour between a root_device and any dev=
ice
> > > > with a bus: the root_device will only release the devm resources wh=
en
> > > > it's freed (in device_release), but a bus device will also do it in
> > > > device_del (through bus_remove_device() -> device_release_driver() =
->
> > > > device_release_driver_internal() -> __device_release_driver() ->
> > > > device_unbind_cleanup(), which are skipped (in multiple places) if
> > > > there's no bus and no driver attached to the device).
> > > >=20
> > > > It does affect DRM, but I'm pretty sure it will affect any framework
> > > > that deals with device hotplugging by deferring the framework struc=
ture
> > > > until the last (userspace) user closes its file descriptor. So I'd
> > > > assume that v4l2 and cec at least are also affected, and most likely
> > > > others. =20
> > >=20
> > > Thanks for the explanation and patience :)
> > >  =20
> > > >  =20
> > > > > If this is the case, then it sounds like a DRM specific issue to =
me. =20
> > > >=20
> > > > I mean, I guess. One could also argue that it's because IIO doesn't
> > > > properly deal with hotplugging. =20
> > >=20
> > > I must say I haven't been testing the IIO registration API. I've only=
 tested
> > > the helper API which is not backed up by any "IIO device". (This is f=
ine for
> > > the helper because it must by design be cleaned-up only after the
> > > IIO-deregistration).
> > >=20
> > > After your explanation here, I am not convinced IIO wouldn't see the =
same
> > > issue if I was testing the devm_iio_device_alloc() & co. =20
> >=20
> > It depends really. The issue DRM is trying to solve is that, when a
> > device is gone, some application might still have an open FD and could
> > still poke into the kernel, while all the resources would have been
> > free'd if it was using devm.
> >=20
> > So everything is kept around until the last fd is closed, so you still
> > have a reference to the device (even though it's been removed from its
> > bus) until that time.
> >=20
> > It could be possible that IIO just doesn't handle that case at all. I
> > guess most of the devices aren't hotpluggable, and there's not much to
> > interact with from a userspace PoV iirc, so it might be why.
>=20
> Lars-Peter Clausen (IIRC) fixed up the IIO handling of the similar cases a
> long time ago now. It's simpler that for some other subsystems as we don't
> have as many interdependencies as occur in DRM etc.
>=20
> I 'think' we are fine in general with the IIO approach to this (I think we
> did have one report of a theoretical race condition in the remove path th=
at
> was never fully addressed).
>=20
> For IIO we also have fds that can be open but all accesses to them are pr=
oxied
> through the IIO core and one of the things iio_device_unregister() or the=
 devm
> equivalent does is to set indio_dev->info =3D NULL  (+ wake up anyone wai=
ting on
> data etc). Alongside removing the callbacks, that is also used as a flag
> to indicate the device has gone.

Sorry if it came as trying to put IIO under a bad light, it certainly
wasn't my intention. I was trying to come up with possible explanations
as to why IIO's design was simpler than DRM is :)

> Note that we keep a reference to the struct indio_dev->dev (rather that t=
he
> underlying device) so that is not freed until the last fd is closed.
> Thus, although devm unwinding has occurred that doesn't mean all the data
> that was allocated with devm_xx calls is cleared up immediately.

I'm not sure I get that part though. devm unwinding can happen even if the =
refcount is > 1

Maxime

--l6s2nrpwzcaaz3jq
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iHUEABYKAB0WIQRcEzekXsqa64kGDp7j7w1vZxhRxQUCZCSVgQAKCRDj7w1vZxhR
xaenAP950SFm/YscP/OZNbX+eEpJynPuAwfDyd1PtJuqNfChOQD+LJmD189z9X+y
U5CselZugPJllYdyXVUeK0Apq+k7KAE=
=+cge
-----END PGP SIGNATURE-----

--l6s2nrpwzcaaz3jq--