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:   Thu, 11 May 2023 13:48:25 +0200
From:   Johan Hovold <johan@kernel.org>
To:     Uwe =?utf-8?Q?Kleine-K=C3=B6nig?= <u.kleine-koenig@pengutronix.de>
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: <ZFzWCey825wSlr2v@hovoldconsulting.com>
References: <20230511073428.10264-1-u.kleine-koenig@pengutronix.de>
 <CAJZ5v0gNPt=rq+pQtmoGL5nxzDQboOK4d6h7=NoY=LueVhZjAQ@mail.gmail.com>
 <20230511103923.hvibdyo5ges4bab2@pengutronix.de>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha256;
        protocol="application/pgp-signature"; boundary="YjvbUq+RuprNibir"
Content-Disposition: inline
In-Reply-To: <20230511103923.hvibdyo5ges4bab2@pengutronix.de>
Precedence: bulk


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

On Thu, May 11, 2023 at 12:39:23PM +0200, Uwe Kleine-K=C3=B6nig wrote:
> On Thu, May 11, 2023 at 12:18:09PM +0200, Rafael J. Wysocki wrote:
> > On Thu, May 11, 2023 at 9:34=E2=80=AFAM Uwe Kleine-K=C3=B6nig
> > <u.kleine-koenig@pengutronix.de> wrote:
> > >
> > > Many drivers that use runtime PM call pm_runtime_get_sync() or one of
> > > its variants in their remove callback. So calling pm_runtime_put_sync=
()
> > > directly before calling the remove callback results (under some
> > > conditions) in the driver's suspend routine being called just to resu=
me
> > > it again afterwards.
> > >
> > > So delay the pm_runtime_put_sync() call until after device_remove().
> > >
> > > Confirmed on a stm32mp157a that doing
> > >
> > >         echo 4400e000.can > /sys/bus/platform/drivers/m_can_platform/=
unbind
> > >
> > > (starting with a runtime-pm suspended 4400e000.can) results in one ca=
ll
> > > less of m_can_runtime_resume() and m_can_runtime_suspend() each after
> > > this change was applied.
> > >
> > > Signed-off-by: Uwe Kleine-K=C3=B6nig <u.kleine-koenig@pengutronix.de>
> >=20
> > I'm not against this change, although I kind of expect it to trigger
> > some fallout that will need to be addressed.  So caveat emtor.
> >=20
> > Anyway
> >=20
> > Reviewed-by: Rafael J. Wysocki <rafael@kernel.org>
>=20
> Thanks for your review tag. I wondered if there will be some fallout,
> and don't know what to expect yet. Sounds like getting it into next soon
> is a good idea?!

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.

There's is really no good reason to even try to change as this is in no
way a fast path.

NAK.

Johan

--YjvbUq+RuprNibir
Content-Type: application/pgp-signature; name="signature.asc"

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

iHUEABYIAB0WIQQHbPq+cpGvN/peuzMLxc3C7H1lCAUCZFzWBQAKCRALxc3C7H1l
CEmdAP48YvGUiT2voAzj2gqP4ohmaHTEH7q7zfd6Qe6cRNqedQD8DCJURAE0CXXO
Z/Kg3dnErUpngBFKmBFtaZScllflIwM=
=+8jj
-----END PGP SIGNATURE-----

--YjvbUq+RuprNibir--