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;
Date:   Fri, 12 May 2023 20:49:25 +0200
From:   Uwe =?utf-8?Q?Kleine-K=C3=B6nig?= <u.kleine-koenig@pengutronix.de>
To:     Johan Hovold <johan@kernel.org>
Cc:     "Rafael J. Wysocki" <rafael@kernel.org>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        linux-kernel@vger.kernel.org, kernel@pengutronix.de
Subject: Re: [PATCH] driver core: Call pm_runtime_put_sync() only after
 device_remove()
Message-ID: <20230512184925.d7w3j4r7oajtpsxi@pengutronix.de>
References: <20230511073428.10264-1-u.kleine-koenig@pengutronix.de>
 <CAJZ5v0gNPt=rq+pQtmoGL5nxzDQboOK4d6h7=NoY=LueVhZjAQ@mail.gmail.com>
 <20230511103923.hvibdyo5ges4bab2@pengutronix.de>
 <ZFzWCey825wSlr2v@hovoldconsulting.com>
 <CAJZ5v0jvJT4JkHtO3RCUEzkfawxLCwR=QO2Y2CsL=cYN9s4hXw@mail.gmail.com>
 <ZF3tUQFTeILXV_VT@hovoldconsulting.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
        protocol="application/pgp-signature"; boundary="nbpmey6zhsjg25lq"
Content-Disposition: inline
In-Reply-To: <ZF3tUQFTeILXV_VT@hovoldconsulting.com>
Precedence: bulk


--nbpmey6zhsjg25lq
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Fri, May 12, 2023 at 09:40:01AM +0200, Johan Hovold wrote:
> On Thu, May 11, 2023 at 04:44:25PM +0200, Rafael J. Wysocki wrote:
> > On Thu, May 11, 2023 at 1:48=E2=80=AFPM Johan Hovold <johan@kernel.org>=
 wrote:
>=20
> > > No, this seems like very bad idea and even violates the documentation
> > > which clearly states that the usage counter is balanced before calling
> > > remove() so that drivers can use pm_runtime_suspend() to put devices
> > > into suspended state.
> >=20
> > I missed that, sorry.
> >=20
> > > There's is really no good reason to even try to change as this is in =
no
> > > way a fast path.
> >=20
> > Still, I think that while the "put" part needs to be done before
> > device_remove(), the actual state change can be carried out later.
> >=20
> > So something like
> >=20
> >     pm_runtime_put_noidle(dev);
> >=20
> >     device_remove(dev);
> >=20
> >     pm_runtime_suspend(dev);
> >=20
> > would generally work, wouldn't it?
>=20
> No, as drivers typically disable runtime pm in their remove callbacks,
> that pm_runtime_suspend() would amount to a no-op (and calling the
> driver pm ops post unbind and the driver having freed its data would
> not work either).

However if a driver author also cares for the CONFIG_PM=3Dn case, calling
pm_runtime_suspend() doesn't have the intended effect and so it's
unfortunately complicated to rely on runtime-pm to power down your
device and you have to do it by hand anyhow (unless you let your driver
depend on CONFIG_PM). So I'm not convinced that "A driver can call
pm_runtime_suspend() to power down" is a useful thing to have.

In the end something like 72362dcdf654 ("can: mcp251xfd:
mcp251xfd_unregister(): simplify runtime PM handling") might be an
approach. But IMHO it's more complicated than it should be and honestly
I'm not sure if it's safe and correct this way.

Best regards
Uwe

--=20
Pengutronix e.K.                           | Uwe Kleine-K=C3=B6nig         =
   |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

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

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

iQEzBAABCgAdFiEEP4GsaTp6HlmJrf7Tj4D7WH0S/k4FAmReijQACgkQj4D7WH0S
/k658ggAsjeo7koFj/nx0LObSxTtW1vC0fPsg5HchvVtxqz0ao1nfod+ObPuJiTW
hLsNGqOClxHV7e3W4zddJBm5dP39jPENLn5AOlCfYx2Z5S597ASE8WO/Rj34/RNq
v60rAe/FwHq8ymexsT4NFV62YY7PtLM4NLldmoc3uffZfTWzm0vLlQMFHlurXUHe
bJFBQqwNzQIOqlz0hzb8U4CKPZm+jv9RO6V2DSQKi4Sde/08tK9mY70N86P/cbOV
9LOAys+3svuHLNzWNk8tXtUFbZVmzCiQ8laI5WKDDNAIo6tTXEB2Oo9yaIv/XVkR
2fYQFjGfs4P7iBoI37KpcAZ6LM5mzQ==
=Atyz
-----END PGP SIGNATURE-----

--nbpmey6zhsjg25lq--