From mboxrd@z Thu Jan 1 00:00:00 1970 From: Bartlomiej Zolnierkiewicz Subject: Re: [PATCH 1/3] crypto: hw_random - Add new Exynos RNG driver Date: Fri, 24 Mar 2017 17:11:25 +0100 Message-ID: <9265537.P6AeF50kg8@amdc3058> References: <20170324142446.31129-1-krzk@kernel.org> <3940941.n3qdgn1RbN@amdc3058> <20170324154600.lavc22dbw7rtioli@kozik-lap> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Cc: linux-samsung-soc@vger.kernel.org, Herbert Xu , Arnd Bergmann , linux-kernel@vger.kernel.org, Javier Martinez Canillas , Kukjin Kim , linux-crypto@vger.kernel.org, Matt Mackall , Olof Johansson , "David S. Miller" , linux-arm-kernel@lists.infradead.org, Marek Szyprowski To: Krzysztof Kozlowski Return-path: In-reply-to: <20170324154600.lavc22dbw7rtioli@kozik-lap> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=m.gmane.org@lists.infradead.org List-Id: linux-crypto.vger.kernel.org On Friday, March 24, 2017 06:46:00 PM Krzysztof Kozlowski wrote: > On Fri, Mar 24, 2017 at 04:26:47PM +0100, Bartlomiej Zolnierkiewicz wrote: > > > > Hi, > > > > Firstly, thanks for working on this. > > > > The patch looks fine overall for me, some review comments below. > > > > On Friday, March 24, 2017 05:24:44 PM Krzysztof Kozlowski wrote: > > > Replace existing hw_ranndom/exynos-rng driver with a new, reworked one. > > > This is a driver for pseudo random number generator block which on > > > Exynos4 chipsets must be seeded with some value. On newer Exynos5420 > > > chipsets it might seed itself from true random number generator block > > > but this is not implemented yet. > > > > > > New driver is a complete rework to use the crypto ALGAPI instead of > > > hw_random API. Rationale for the change: > > > 1. hw_random interface is for true RNG devices. > > > 2. The old driver was seeding itself with jiffies which is not a > > > reliable source for randomness. > > > 3. Device generates five random numbers in each pass but old driver was > > > returning only one thus its performance was reduced. > > > > > > Compatibility with DeviceTree bindings is preserved. > > > > > > New driver does not use runtime power management but manually enables > > > and disables the clock when needed. This is preferred approach because > > > using runtime PM just to toggle clock is huge overhead. Another > > > > I'm not entirely convinced that the new approach is better. > > > > With the old approach exynos_rng_generate() can be called more > > than once before PM autosuspend kicks in and thus clk_prepare_enable()/ > > clk_disable()_unprepare() operations will be done only once. This > > would give better performance on the "burst" operations. > > > > [ The above assumes that clock operations are more costly than > > going through PM core to check the current device state. ] > > I agree that we loose the "burst" mode but: > 1. At least on Exynso4 SSS is part of TOP power domain so it will not > help to reduce any more power consumption (on Exynos5422 it is > mentioned in G2D... which seems incorrect). > 2. I think the overhead of clk operations is much smaller. These are only > two locks (prepare mutex + enable spin), simple tree traversal and > play with few SFRs. > > The power domain code in comparison to that is huge and complicated > with inter-device links and dependencies. Also the actual runtime PM > suspend would anyway fall back at then end to clk prepare/enable > locks and paths. > > We've been talking about this a lot with Marek Szyprowski (cc'ed) and > he was always (AFAIR) against attempts of runtime power > management of a single clock... OK, thanks for explanation. > > > +static int exynos_rng_probe(struct platform_device *pdev) > > > +{ > > > + struct exynos_rng_dev *rng; > > > + struct resource *res; > > > + int ret; > > > + > > > + if (exynos_rng_dev) > > > + return -EEXIST; > > > > How this condition could ever happen? > > > > The probe function will never be called twice. > > I really do not like global or file-scope variables. I do not like > drivers using them. Actually I hate them. > > From time to time I encounter a driver which was designed with that > approach - static fields and hidden assumption that there will be only > one instance. Usually that assumption is really hidden... > > ... and then it happens that I want to use two instances which of course > fails. > > This code serves as a clear documentation for this assumption - only one > instance is allowed. You can look at it as a self-documenting > requirement. For me it looks as needless case of defensive programming and when I see the code like this it always raises questions about the real intentions of the code. I find it puzzling and not helpful. > And I think the probe might be called twice, for example in case of > mistake in DTB. Even if this is possible resource allocation code in the driver will take take care of handling it just fine, Best regards, -- Bartlomiej Zolnierkiewicz Samsung R&D Institute Poland Samsung Electronics From mboxrd@z Thu Jan 1 00:00:00 1970 From: b.zolnierkie@samsung.com (Bartlomiej Zolnierkiewicz) Date: Fri, 24 Mar 2017 17:11:25 +0100 Subject: [PATCH 1/3] crypto: hw_random - Add new Exynos RNG driver In-Reply-To: <20170324154600.lavc22dbw7rtioli@kozik-lap> References: <20170324142446.31129-1-krzk@kernel.org> <3940941.n3qdgn1RbN@amdc3058> <20170324154600.lavc22dbw7rtioli@kozik-lap> Message-ID: <9265537.P6AeF50kg8@amdc3058> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On Friday, March 24, 2017 06:46:00 PM Krzysztof Kozlowski wrote: > On Fri, Mar 24, 2017 at 04:26:47PM +0100, Bartlomiej Zolnierkiewicz wrote: > > > > Hi, > > > > Firstly, thanks for working on this. > > > > The patch looks fine overall for me, some review comments below. > > > > On Friday, March 24, 2017 05:24:44 PM Krzysztof Kozlowski wrote: > > > Replace existing hw_ranndom/exynos-rng driver with a new, reworked one. > > > This is a driver for pseudo random number generator block which on > > > Exynos4 chipsets must be seeded with some value. On newer Exynos5420 > > > chipsets it might seed itself from true random number generator block > > > but this is not implemented yet. > > > > > > New driver is a complete rework to use the crypto ALGAPI instead of > > > hw_random API. Rationale for the change: > > > 1. hw_random interface is for true RNG devices. > > > 2. The old driver was seeding itself with jiffies which is not a > > > reliable source for randomness. > > > 3. Device generates five random numbers in each pass but old driver was > > > returning only one thus its performance was reduced. > > > > > > Compatibility with DeviceTree bindings is preserved. > > > > > > New driver does not use runtime power management but manually enables > > > and disables the clock when needed. This is preferred approach because > > > using runtime PM just to toggle clock is huge overhead. Another > > > > I'm not entirely convinced that the new approach is better. > > > > With the old approach exynos_rng_generate() can be called more > > than once before PM autosuspend kicks in and thus clk_prepare_enable()/ > > clk_disable()_unprepare() operations will be done only once. This > > would give better performance on the "burst" operations. > > > > [ The above assumes that clock operations are more costly than > > going through PM core to check the current device state. ] > > I agree that we loose the "burst" mode but: > 1. At least on Exynso4 SSS is part of TOP power domain so it will not > help to reduce any more power consumption (on Exynos5422 it is > mentioned in G2D... which seems incorrect). > 2. I think the overhead of clk operations is much smaller. These are only > two locks (prepare mutex + enable spin), simple tree traversal and > play with few SFRs. > > The power domain code in comparison to that is huge and complicated > with inter-device links and dependencies. Also the actual runtime PM > suspend would anyway fall back at then end to clk prepare/enable > locks and paths. > > We've been talking about this a lot with Marek Szyprowski (cc'ed) and > he was always (AFAIR) against attempts of runtime power > management of a single clock... OK, thanks for explanation. > > > +static int exynos_rng_probe(struct platform_device *pdev) > > > +{ > > > + struct exynos_rng_dev *rng; > > > + struct resource *res; > > > + int ret; > > > + > > > + if (exynos_rng_dev) > > > + return -EEXIST; > > > > How this condition could ever happen? > > > > The probe function will never be called twice. > > I really do not like global or file-scope variables. I do not like > drivers using them. Actually I hate them. > > From time to time I encounter a driver which was designed with that > approach - static fields and hidden assumption that there will be only > one instance. Usually that assumption is really hidden... > > ... and then it happens that I want to use two instances which of course > fails. > > This code serves as a clear documentation for this assumption - only one > instance is allowed. You can look at it as a self-documenting > requirement. For me it looks as needless case of defensive programming and when I see the code like this it always raises questions about the real intentions of the code. I find it puzzling and not helpful. > And I think the probe might be called twice, for example in case of > mistake in DTB. Even if this is possible resource allocation code in the driver will take take care of handling it just fine, Best regards, -- Bartlomiej Zolnierkiewicz Samsung R&D Institute Poland Samsung Electronics From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S966119AbdCXQME (ORCPT ); Fri, 24 Mar 2017 12:12:04 -0400 Received: from mailout1.samsung.com ([203.254.224.24]:58743 "EHLO mailout1.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753367AbdCXQLc (ORCPT ); Fri, 24 Mar 2017 12:11:32 -0400 X-AuditID: b6c32a36-f79c56d000001a35-8b-58d545302aac From: Bartlomiej Zolnierkiewicz To: Krzysztof Kozlowski Cc: linux-arm-kernel@lists.infradead.org, Kukjin Kim , Javier Martinez Canillas , Matt Mackall , Herbert Xu , "David S. Miller" , linux-kernel@vger.kernel.org, linux-samsung-soc@vger.kernel.org, linux-crypto@vger.kernel.org, Olof Johansson , Arnd Bergmann , Marek Szyprowski Subject: Re: [PATCH 1/3] crypto: hw_random - Add new Exynos RNG driver Date: Fri, 24 Mar 2017 17:11:25 +0100 Message-id: <9265537.P6AeF50kg8@amdc3058> User-Agent: KMail/4.13.3 (Linux/3.13.0-96-generic; KDE/4.13.3; x86_64; ; ) In-reply-to: <20170324154600.lavc22dbw7rtioli@kozik-lap> MIME-version: 1.0 Content-transfer-encoding: 7Bit Content-type: text/plain; charset=us-ascii X-Brightmail-Tracker: H4sIAAAAAAAAA02Sa0hTYRjHec9tx+XstJY9alkOCjTzEiInlUrIOoQf/GCxhKihB5U2nVtK RqBINVPx2sXMULpgG9plDZOsKVpLJF3ijRS3yiS8JKWp6VJqHgW//d7n+T//h//DS+NSK+lN p6Zd4LVpSpWcEhON7f7B+0Ni+hUhVU2+7HK5VcRW264QbOHEDnbqRz3GloxO4qzN9kzEmkYH SNZhX8TY3lfVFFtps2Bsw9sREVvbWIzYzsFZ6oiEcy6VI85s+IRxja17OJPxOsW9eJjD9XXk kZy5ZETEFZuNiCsef4a4WZNvnDhBHJXEq1KzeG3woXPilHz9PaS57Xex3manctGwVwFyo4EJ g8Fb5bjAnvDR/pQqQGJayjQheOsYJISHHoPeuglyfaL/+TApNOoQOA1ja6o5BIVd84RLRTER UKY3IhfLGH8YXF5YncCZdzjk1/esirYyx6Db+VVUgGiaYPbAgCPWVZb817ffXaRcvI05AWaL HnOxG8OC4c1PXNBsgT8V9lUbnNkFlpabpMCB0GV9gly7gFkUQdGHbtzlD8xOMLWu4VEoqaCF MFth4r1ZJLAPGFeGMIFvI3jpBMHmBQJjc+maKBLa3/es7fKA6bkiUvCUQP41qSDhoOLzNCGU o+HB1UThPH0IDKW/RaVoV9WGBFUbElRtSFCLcCPy5DU6dTKvC9UcCNIp1brMtOSgxHS1Ca3+ roDwJnS/O7YNMTSSu0s6JvsUUlKZpctWtyGgcblM8iWyXyGVJCmzL/Ha9LPaTBWva0M+NCHf LiFDyhRSJll5gT/P8xpeu97FaDfvXGRYGjdKgw1hMWUDGZs6X8f4qvC9r1dOaXZK3sR/UzBx gW1NgQfzZLi7hYivqaRiOifsDf7zGUvNhyNHhiz1Y1j7r9P7alJmHFmyx73hx702j4UNR81n efoljUSfXPg8lfDuTAshvWP9XuyTmlMz7ZFd22ff/eiJPFw/8zei7vINOaFLUYYG4Fqd8h8H MaA5WQMAAA== X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFnrFIsWRmVeSWpSXmKPExsVy+t9jAV0D16sRBl2/WCz+TjrGbjHnfAuL RfcrGYs3b9cwWfQ/fs1scf78BnaLTY+vsVrcv/eTyeLyrjlsFjPO72OyWHvkLrvFgm19jBan rn9mc+D1+P1rEqPHlpU3mTy2HVD12LSqk81j85J6jysnmlg9tvTfZffo27KK0aPv5QZGj8+b 5AK4otxsMlITU1KLFFLzkvNTMvPSbZVCQ9x0LZQU8hJzU22VInR9Q4KUFMoSc0qBPCMDNODg HOAerKRvl+CW0dE+l7FgumLFmvP32BoYb0t2MXJySAiYSFzdeJsVwhaTuHBvPVsXIxeHkMBS Romd2w+zQDhfGSUezD7LAlLFJmAlMbF9FSOILSKgKXH973dWkCJmgaPMElvXbQBLCAu4SZz7 /Yi9i5GDg0VAVeLafR+QMC9Q/eHZP9lAbFEBL4kt+9qZQGxOAQuJlXs/MEMsa2CU2HV/IjtE g6DEj8n3wBYzC8hL7Ns/lRXC1pJYv/M40wRGgVlIymYhKZuFpGwBI/MqRonUguSC4qT0XMO8 1HK94sTc4tK8dL3k/NxNjOB4fia1g/HgLvdDjAIcjEo8vCdeX4kQYk0sK67MPcQowcGsJML7 0PpqhBBvSmJlVWpRfnxRaU5q8SFGU6AHJzJLiSbnA1NNXkm8oYm5ibmxgYW5paWJkZI4b+Ps Z+FCAumJJanZqakFqUUwfUwcnFINjGHi+R98Pdcf9d9j8/Yy/+FFew6nff2nkvX2ypGZD+9f XhW0fcEX/opMy3kC1x1LTs7iteZZdLk7Rs/2/Yb8X8r/eaX3mRUH29W3nLhnzKM7tXRmq3i0 2SrDLvtqRbEpKyq/LznctM+h93T0u4uhbCVbEs7Wts1r1546X7DmRG1pZ+7bg69Ci5VYijMS DbWYi4oTAdt5RmL9AgAA X-MTR: 20000000000000000@CPGS X-CMS-MailID: 20170324161128epcas1p4b9ef08e3d7e640957d30afa84aa66a16 X-Msg-Generator: CA X-Sender-IP: 203.254.230.26 X-Local-Sender: =?UTF-8?B?QmFydGxvbWllaiBab2xuaWVya2lld2ljehtTUlBPTC1LZXJu?= =?UTF-8?B?ZWwgKFRQKRvsgrzshLHsoITsnpAbU2VuaW9yIFNvZnR3YXJlIEVuZ2luZWVy?= X-Global-Sender: =?UTF-8?B?QmFydGxvbWllaiBab2xuaWVya2lld2ljehtTUlBPTC1LZXJu?= =?UTF-8?B?ZWwgKFRQKRtTYW1zdW5nIEVsZWN0cm9uaWNzG1NlbmlvciBTb2Z0d2FyZSBF?= =?UTF-8?B?bmdpbmVlcg==?= X-Sender-Code: =?UTF-8?B?QzEwG0VIURtDMTBDRDAyQ0QwMjczOTI=?= CMS-TYPE: 101P X-HopCount: 7 X-CMS-RootMailID: 20170324161128epcas1p4b9ef08e3d7e640957d30afa84aa66a16 X-RootMTR: 20170324161128epcas1p4b9ef08e3d7e640957d30afa84aa66a16 References: <20170324142446.31129-1-krzk@kernel.org> <3940941.n3qdgn1RbN@amdc3058> <20170324154600.lavc22dbw7rtioli@kozik-lap> Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Friday, March 24, 2017 06:46:00 PM Krzysztof Kozlowski wrote: > On Fri, Mar 24, 2017 at 04:26:47PM +0100, Bartlomiej Zolnierkiewicz wrote: > > > > Hi, > > > > Firstly, thanks for working on this. > > > > The patch looks fine overall for me, some review comments below. > > > > On Friday, March 24, 2017 05:24:44 PM Krzysztof Kozlowski wrote: > > > Replace existing hw_ranndom/exynos-rng driver with a new, reworked one. > > > This is a driver for pseudo random number generator block which on > > > Exynos4 chipsets must be seeded with some value. On newer Exynos5420 > > > chipsets it might seed itself from true random number generator block > > > but this is not implemented yet. > > > > > > New driver is a complete rework to use the crypto ALGAPI instead of > > > hw_random API. Rationale for the change: > > > 1. hw_random interface is for true RNG devices. > > > 2. The old driver was seeding itself with jiffies which is not a > > > reliable source for randomness. > > > 3. Device generates five random numbers in each pass but old driver was > > > returning only one thus its performance was reduced. > > > > > > Compatibility with DeviceTree bindings is preserved. > > > > > > New driver does not use runtime power management but manually enables > > > and disables the clock when needed. This is preferred approach because > > > using runtime PM just to toggle clock is huge overhead. Another > > > > I'm not entirely convinced that the new approach is better. > > > > With the old approach exynos_rng_generate() can be called more > > than once before PM autosuspend kicks in and thus clk_prepare_enable()/ > > clk_disable()_unprepare() operations will be done only once. This > > would give better performance on the "burst" operations. > > > > [ The above assumes that clock operations are more costly than > > going through PM core to check the current device state. ] > > I agree that we loose the "burst" mode but: > 1. At least on Exynso4 SSS is part of TOP power domain so it will not > help to reduce any more power consumption (on Exynos5422 it is > mentioned in G2D... which seems incorrect). > 2. I think the overhead of clk operations is much smaller. These are only > two locks (prepare mutex + enable spin), simple tree traversal and > play with few SFRs. > > The power domain code in comparison to that is huge and complicated > with inter-device links and dependencies. Also the actual runtime PM > suspend would anyway fall back at then end to clk prepare/enable > locks and paths. > > We've been talking about this a lot with Marek Szyprowski (cc'ed) and > he was always (AFAIR) against attempts of runtime power > management of a single clock... OK, thanks for explanation. > > > +static int exynos_rng_probe(struct platform_device *pdev) > > > +{ > > > + struct exynos_rng_dev *rng; > > > + struct resource *res; > > > + int ret; > > > + > > > + if (exynos_rng_dev) > > > + return -EEXIST; > > > > How this condition could ever happen? > > > > The probe function will never be called twice. > > I really do not like global or file-scope variables. I do not like > drivers using them. Actually I hate them. > > From time to time I encounter a driver which was designed with that > approach - static fields and hidden assumption that there will be only > one instance. Usually that assumption is really hidden... > > ... and then it happens that I want to use two instances which of course > fails. > > This code serves as a clear documentation for this assumption - only one > instance is allowed. You can look at it as a self-documenting > requirement. For me it looks as needless case of defensive programming and when I see the code like this it always raises questions about the real intentions of the code. I find it puzzling and not helpful. > And I think the probe might be called twice, for example in case of > mistake in DTB. Even if this is possible resource allocation code in the driver will take take care of handling it just fine, Best regards, -- Bartlomiej Zolnierkiewicz Samsung R&D Institute Poland Samsung Electronics