Message-id: <9265537.P6AeF50kg8@amdc3058>
Date:   Fri, 24 Mar 2017 17:11:25 +0100
From:   Bartlomiej Zolnierkiewicz <b.zolnierkie@...sung.com>
To:     Krzysztof Kozlowski <krzk@...nel.org>
Cc:     linux-arm-kernel@...ts.infradead.org,
        Kukjin Kim <kgene@...nel.org>,
        Javier Martinez Canillas <javier@....samsung.com>,
        Matt Mackall <mpm@...enic.com>,
        Herbert Xu <herbert@...dor.apana.org.au>,
        "David S. Miller" <davem@...emloft.net>,
        linux-kernel@...r.kernel.org, linux-samsung-soc@...r.kernel.org,
        linux-crypto@...r.kernel.org, Olof Johansson <olof@...om.net>,
        Arnd Bergmann <arnd@...db.de>,
        Marek Szyprowski <m.szyprowski@...sung.com>
Subject: Re: [PATCH 1/3] crypto: hw_random - Add new Exynos RNG driver

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

Powered by blists - more mailing lists