From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752312AbdLFLdJ (ORCPT ); Wed, 6 Dec 2017 06:33:09 -0500 Received: from mailout1.w1.samsung.com ([210.118.77.11]:56515 "EHLO mailout1.w1.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751546AbdLFLdF (ORCPT ); Wed, 6 Dec 2017 06:33:05 -0500 DKIM-Filter: OpenDKIM Filter v2.11.0 mailout1.w1.samsung.com 20171206113302euoutp01177547286859b30b87bc2fa7ae4bcda6~9sUpcFn821565715657euoutp01R X-AuditID: cbfec7f2-f793b6d000003243-c9-5a27d56d945f From: =?utf-8?Q?=C5=81ukasz_Stelmach?= To: Krzysztof Kozlowski Cc: Stephan Mueller , robh+dt@kernel.org, Herbert Xu , "David S. Miller" , Kukjin Kim , linux-crypto@vger.kernel.org, linux-samsung-soc@vger.kernel.org, linux-kernel@vger.kernel.org, Marek Szyprowski , =?utf-8?Q?Bart=C5=82omiej_=C5=BBo?= =?utf-8?Q?=C5=82nierkiewicz?= Subject: Re: [PATCH 2/3] crypto: exynos - Improve performance of PRNG Date: Wed, 06 Dec 2017 12:32:50 +0100 In-reply-to: Message-id: <87vahk9jml.fsf%l.stelmach@samsung.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux) MIME-version: 1.0 Content-type: multipart/signed; boundary="=-=-="; micalg="pgp-sha256"; protocol="application/pgp-signature" X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFtrMKsWRmVeSWpSXmKPExsWy7djP87q5V9WjDM626VpsnLGe1WLO+RYW i+5XMhb9j18zW5w/v4Hd4v69n0wWl3fNYbOYcX4fk8XaI3fZLVr3HmG3mHp6LYsDt8enK1eY PLasvMnkse2AqsemVZ1sHn1bVjF6fN4kF8AWxWWTkpqTWZZapG+XwJWxful8xoKlihXnP/Qx NzDuku5i5OSQEDCReD5zDzOELSZx4d56NhBbSGApo8TMntQuRi4g+zOjxMtpy9lgGh7+WccO UbSMUeJKsxBE0RdGiRu7loFNYhOwl+g/so8FxBYR0JS4/vc7K0gRs8BiZol/C46xgiSEBVwl Zl3eDNbAIqAq8a7pEJjNKRAsceDccqANHBy8AsYSTz6Vg4RFBSwl7vXdBTuCV0BQ4sfke2Dz mQVyJT41/WcHmS8h0M8u0Tt3JdQ7LhILtlxhhbCFJV4d38IOYctIXJ7czQLVwChxeP53qMQU RonFCx0gbGuJP6smskFs4JOYtG06M8hBEgK8Eh1tQhAlHhIvP26FanWUmHn4DjMkJJYwSnRf 2c88gVF2FpJjZyE5dhbQKGZgwKzfpQ8R1pZYtvA1M4RtK7Fu3XuWBYysqxhFUkuLc9NTi431 ihNzi0vz0vWS83M3MQKTz+l/xz/tYPx6wuoQowAHoxIPb8AMtSgh1sSy4srcQ4wqQGMebVh9 gVGKJS8/L1VJhPfyZfUoId6UxMqq1KL8+KLSnNTiQ4zSHCxK4ry2UW2RQgLpiSWp2ampBalF MFkmDk6pBkalX8J9Ntb1Bptesqo3nUk+oiGlrbbu+cLnezwL1G+xL5RRzFbf6Pd81UzrWTld TlZfG1b/e1pomfVXPaLXpKMzXO/J2bz9hneXMZRF/48K2sG+4+wnh+o7Xglyyv0nzy6rkRDc oRI1ScPt3Mugm5E5f28EXLjuOXNGs86q6bwPd9+c//br5JdKLMUZiYZazEXFiQApLKxLRgMA AA== X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFupkkeLIzCtJLcpLzFFi42I5/e/4Nd3cq+pRBovvCVlsnLGe1WLO+RYW i+5XMhb9j18zW5w/v4Hd4v69n0wWl3fNYbOYcX4fk8XaI3fZLVr3HmG3mHp6LYsDt8enK1eY PLasvMnkse2AqsemVZ1sHn1bVjF6fN4kF8AWxWWTkpqTWZZapG+XwJWxful8xoKlihXnP/Qx NzDuku5i5OSQEDCRePhnHTuELSZx4d56ti5GLg4hgSWMEleufWeBcL4xSkyY0cAIUsUmYC/R f2QfC4gtIqApcf3vd1aQImaBpcwSD56uZwJJCAu4Ssy6vJkZxBYSCJD49KULLM4ioCrxrukQ WJxTIFiiv+EW0GoODl4BY4knn8pBwqIClhL3+u6ygdi8AoISPybfA9vFLJAtceHiG5YJjPyz kKRmIUnNAprEDHTS+l36EGFtiWULXzND2LYS69a9Z1nAyLqKUSS1tDg3PbfYSK84Mbe4NC9d Lzk/dxMjMFK2Hfu5ZQdj17vgQ4wCHIxKPLwrZqlFCbEmlhVX5h5iVAEa82jD6guMUix5+Xmp SiK8ly+rRwnxpiRWVqUW5ccXleakFh9ilOZgURLn7d2zOlJIID2xJDU7NbUgtQgmy8TBKdXA uC7V7DzvcsEPaosn3pyz9WaXYXpr/zzbtl+r1xXVtS5KDzedNf/42yMN9xdxqyTVzn0jLDyB /c2zLWIPeJ5sfy2pvGuzHOevhSY/kjqu/tIQ6LLV8e57Ku3laC4/OV+3I5K1/km48LGQiTfs 32y4ejYxUuL+lr0h35/sCj21fNX6S51ruP2MU5VYijMSDbWYi4oTAWbqf1ucAgAA X-CMS-MailID: 20171206113301eucas1p23da9decc34cc646b0bf4eb88953ef94a X-Msg-Generator: CA CMS-TYPE: 201P X-CMS-RootMailID: 20171206113301eucas1p23da9decc34cc646b0bf4eb88953ef94a X-RootMTR: 20171206113301eucas1p23da9decc34cc646b0bf4eb88953ef94a References: Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org --=-=-= Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable It was <2017-12-05 wto 19:06>, when Krzysztof Kozlowski wrote: > On Tue, Dec 5, 2017 at 6:53 PM, Krzysztof Kozlowski wro= te: >> On Tue, Dec 05, 2017 at 05:43:10PM +0100, =C5=81ukasz Stelmach wrote: >>> It was <2017-12-05 wto 14:54>, when Stephan Mueller wrote: >>>> Am Dienstag, 5. Dezember 2017, 13:35:57 CET schrieb =C5=81ukasz Stelma= ch: >>>>> Use memcpy_fromio() instead of custom exynos_rng_copy_random() functi= on >>>>> to retrieve generated numbers from the registers of PRNG. >>>>> >>>>> Remove unnecessary invocation of cpu_relax(). >>>>> >>>>> Signed-off-by: =C5=81ukasz Stelmach >>>>> --- >>>>> drivers/crypto/exynos-rng.c | 36 +++++------------------------------- >>>>> 1 file changed, 5 insertions(+), 31 deletions(-) >>>>> >>>>> diff --git a/drivers/crypto/exynos-rng.c b/drivers/crypto/exynos-rng.c >>>>> index 894ef93ef5ec..002e9d2a83cc 100644 >>>>> --- a/drivers/crypto/exynos-rng.c >>>>> +++ b/drivers/crypto/exynos-rng.c >>> >>> [...] >>> >>>>> @@ -171,6 +143,8 @@ static int exynos_rng_get_random(struct exynos_rn= g_dev >>>>> *rng, { >>>>> int retry =3D EXYNOS_RNG_WAIT_RETRIES; >>>>> >>>>> + *read =3D min_t(size_t, dlen, EXYNOS_RNG_SEED_SIZE); >>>>> + >>>>> if (rng->type =3D=3D EXYNOS_PRNG_TYPE4) { >>>>> exynos_rng_writel(rng, EXYNOS_RNG_CONTROL_START, >>>>> EXYNOS_RNG_CONTROL); >>>>> @@ -180,8 +154,8 @@ static int exynos_rng_get_random(struct exynos_rn= g_dev >>>>> *rng, } >>>>> >>>>> while (!(exynos_rng_readl(rng, >>>>> - EXYNOS_RNG_STATUS) & EXYNOS_RNG_STATUS_RNG_DONE) &= & --retry) >>>>> - cpu_relax(); >>>>> + EXYNOS_RNG_STATUS) & EXYNOS_RNG_STATUS_RNG_DONE) && >>>>> + --retry); [...] >>> The busy loop is not very busy. Every time I checked the loop (w/o >>> cpu_relax()) was executed twice (retry was 98) and the operation was >>> reliable. I don't see why do we need a memory barrier here. On the other >>> hand, I am not sure the whole exynos_rng_get_random() shouldn't be ran >>> under a mutex or a spinlock (I don't see anything like this in the upper >>> layers of the crypto framework). >>> >>> The *_relaxed() I/O operations do not enforce memory >> >> The cpu_relax() is a common pattern for busy-loop. If you want to break >> this pattern - please explain why only this part of kernel should not >> follow it (and rest of kernel should). >> >> The other part - this code is already using relaxed versions which might >> get you into difficult to debug issues. You mentioned that loop works >> reliable after removing the cpu_relax... yeah, it might for 99.999% but >> that's not the argument. I remember few emails from Arnd Bergmann >> mentioning explicitly to avoid using relaxed versions "just because", >> unless it is necessary or really understood. >> >> The code first writes to control register, then checks for status so you >> should have these operations strictly ordered. Therefore I think >> cpu_relax() should not be removed. > > ... or just convert it to readl_poll_timeout() because it makes code > more readable, takes care of timeout and you do not have care about > specific implementation (whether there should or should not be > cpu_relax). OK. This appears to perform reasonably. do { cpu_relax(); } while (!(exynos_rng_readl(rng, EXYNOS_RNG_STATUS) & EXYNOS_RNG_STATUS_RNG_DONE) && --retry); =2D-=20 =C5=81ukasz Stelmach Samsung R&D Institute Poland Samsung Electronics --=-=-= Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQEcBAEBCAAGBQJaJ9VjAAoJELCuHpyYpYAQox8H/0gxJhOv+YMqdBCn/VGntXph 1hU7ptg2MCYA9HsfkK2sRGFs/p9N6uCM78jRBX0Gpad7fE6eHf9K8D76QcOQ7q7Y MSX+nuD6YA3ZZjdaODgmStEHF9B6sBv0yAjEYGHULHw/ZeD15jMneavwFz0MbYbz eE4eJIcPY7nvY5+8gJ16IikTuEIsfftuXit0PJehif2ceItDjzRQbRHCD9uTpl46 y/oEiZuGxvnzSV4tqrqmZgNorOL2XMOOwJa8xA/EQ/dsjtV08g2yfLeEbmWcXNFr j2NsRh5QU7Kihcdm7j0Vu6RfBNBQUJYcAfuNr+AWxJnEc52dwlTqVqM0pp+MKpI= =rcok -----END PGP SIGNATURE----- --=-=-=--