Received-SPF: pass (google.com: domain of linux-kernel+bounces-86239-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Thu, 29 Feb 2024 08:33:13 +0100
From: Wolfram Sang <wsa+renesas@sang-engineering.com>
To: Dirk Behme <dirk.behme@de.bosch.com>
Cc: linux-renesas-soc@vger.kernel.org, Ulf Hansson <ulf.hansson@linaro.org>,
	linux-mmc@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH RFT] mmc: tmio: avoid concurrent runs of
 mmc_request_done()
Message-ID: <ZeAzOUN-8QEl4U2n@ninjato>
Mail-Followup-To: Wolfram Sang <wsa+renesas@sang-engineering.com>,
	Dirk Behme <dirk.behme@de.bosch.com>,
	linux-renesas-soc@vger.kernel.org,
	Ulf Hansson <ulf.hansson@linaro.org>, linux-mmc@vger.kernel.org,
	linux-kernel@vger.kernel.org
References: <20240228100354.3285-2-wsa+renesas@sang-engineering.com>
 <331084d4-2802-4d1d-b978-6660f546ea2d@de.bosch.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="yXWwMvQMGXoOVewE"
Content-Disposition: inline
In-Reply-To: <331084d4-2802-4d1d-b978-6660f546ea2d@de.bosch.com>


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

Hi Dirk,

> > With the to-be-fixed commit, the reset_work handler cleared 'host->mrq'
> > outside of the spinlock protected critical section. That leaves a small
> > race window during execution of 'tmio_mmc_reset()' where the done_work
> > handler could grab a pointer to the now invalid 'host->mrq'. Both would
> > use it to call mmc_request_done() causing problems (see Link).
> >=20
> > However, 'host->mrq' cannot simply be cleared earlier inside the
> > critical section. That would allow new mrqs to come in asynchronously
> > while the actual reset of the controller still needs to be done. So,
> > like 'tmio_mmc_set_ios()', an ERR_PTR is used to prevent new mrqs from
> > coming in but still avoiding concurrency between work handlers.
> >=20
> > Reported-by: Dirk Behme <dirk.behme@de.bosch.com>
> > Closes: https://lore.kernel.org/all/20240220061356.3001761-1-dirk.behme=
@de.bosch.com/
> > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> > Fixes: df3ef2d3c92c ("mmc: protect the tmio_mmc driver against a theore=
tical race")
>=20
> Tested-by: Dirk Behme <dirk.behme@de.bosch.com>
> Reviewed-by: Dirk Behme <dirk.behme@de.bosch.com>

Awesome! Thanks for the super-fast tags!

> At least the issues we observed before are not seen any more. As we are n=
ot
> exactly sure on the root cause, of course this is not a 100% proof. But as
> the change looks good, looks like it won't break something and the system
> behaves good with it I would say we are good to go.

I agree. We don't know if it is all you need. But there definitely was a
race window and closing it removes some observed anomalies. Let's hope
all of them :) I looked many times at the code and, to the best of my
knowledge, don't see side effects. 'host->mrq' stays non-NULL, so new
mrqs won't be added like before. Changing it to an ERR_PTR will only
affect the check in the done_work handler which is what we want. But, of
course, more eyes are always welcome.

> I think we could add anything like
>=20
> Cc: stable@vger.kernel.org # 3.0+

Yes, we should definitely have that. I would have added it once your
testing got good results. This affects every Renesas SDHI or Uniphier SD
instance since 3.0 (12 years). Wow! So, thanks a ton for your report and
assistance in debugging it. Very much appreciated! And, phew, I am happy
that this solution does not make the locking more complex \o/

All the best,

   Wolfram


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

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

iQIzBAABCgAdFiEEOZGx6rniZ1Gk92RdFA3kzBSgKbYFAmXgMzUACgkQFA3kzBSg
KbbIwA/8C0nJicwAl4j00mETzzdiC36BY6uQzoqil257N2l/QOBBGo3PdjzdLW41
KOggdi2aX+xRmGPuIIevXbKDYfu0GJUcTMwY3cqJ5Y+oQLC2s2e/ZSVUqc34vxv/
sNbJYcLwJE+cTN2H0GMzYnUoZS96Nq0JSr1qhMCWUOWWQ9jAlBTw/7bK07i3YWO5
6eP/6GaoxE2TJwnsrjgGflCIAtL0/VQV6VWjgGfWXMUSO3+LSBrbNITWUgUcCncj
YwLgPKZEoweyIWJ13TrpODyxxXyTu7NZlj5zkIfDSrKnobO6oN2970NVeM0fTkS7
eInBGie7rXRuADOxureLRkn+ElEZ6rlPyq4jDU5ycYbwEXNYW9+enAK7ns9ce9M6
EqjuPS0okYZpoOpvDvwjxJ5Pc0CBFPP6iVq2yYtbZPbSZElKDKeh12lwmtdGaar/
fTbt47paI8LMsKtroDeY/q5eBzh7l2Gu0SeOLHERBKN9wLfROglQ5X56qr73iaaC
QuQ4DGr8bphvbiJpBbQ65e9uy0Ti8AqClb6JgkIiAbyYcFfmRL5T7d81yZtS1PHE
27jtIjqJ2mCZavq2aAxNfTHNEpr8hSlJtLrLNoOgalww9gPbZMCNsJU+RGNgLo+y
esVZRoTz70+es1cBmUAuvXc8yInLWope+JNK3GhIOSGK6rRmOy4=
=4Qja
-----END PGP SIGNATURE-----

--yXWwMvQMGXoOVewE--