Re: [PATCH] crypto: caam - use the same jr for rng init/exit

From: Horia Geanta <horia.geanta@nxp.com>
To: Andrey Smirnov <andrew.smirnov@gmail.com>
Cc: "linux-crypto@vger.kernel.org" <linux-crypto@vger.kernel.org>,
	Chris Healy <cphealy@gmail.com>,
	Lucas Stach <l.stach@pengutronix.de>,
	Herbert Xu <herbert@gondor.apana.org.au>,
	Iuliana Prodan <iuliana.prodan@nxp.com>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] crypto: caam - use the same jr for rng init/exit
Date: Fri, 20 Sep 2019 15:50:24 +0000	[thread overview]
Message-ID: <VI1PR0402MB34853076C01D2973E9401AEC98880@VI1PR0402MB3485.eurprd04.prod.outlook.com> (raw)
In-Reply-To: CAHQ1cqFyPs1vONwV3Ujv6MwqoP=672pCEXFTuM+j20zNPD86tw@mail.gmail.com

On 9/18/2019 9:01 AM, Andrey Smirnov wrote:
> On Wed, Sep 11, 2019 at 2:35 AM Horia Geanta <horia.geanta@nxp.com> wrote:
>>
>> On 9/4/2019 5:35 AM, Andrey Smirnov wrote:
>>> In order to allow caam_jr_enqueue() to lock underlying JR's
>>> device (via device_lock(), see commit that follows) we need to make
>>> sure that no code calls caam_jr_enqueue() as a part of caam_jr_probe()
>>> to avoid a deadlock. Unfortunately, current implementation of caamrng
>>> code does exactly that in caam_init_buf().
>>>
> 
> I don't think your patch addresses the above, so it can probably be
> cut out of the message.
> 
No, it does not, it addresses the issue right below.

Not sure what you mean by "cut out of the message". If you look carefully
in the original message, at some pointer there is a scissors line.

>>> Another big problem with original caamrng initialization is a
>>> circular reference in the form of:
>>>
>>>  1. caam_rng_init() aquires JR via caam_jr_alloc(). Freed only by
>>>     caam_rng_exit()
>>>
>>>  2. caam_rng_exit() is only called by unregister_algs() once last JR
>>>     is shut down
>>>
>>>  3. caam_jr_remove() won't call unregister_algs() for last JR
>>>     until tfm_count reaches zero, which can only happen via
>>>     unregister_algs() -> caam_rng_exit() call chain.
>>>
>>> To avoid all of that, convert caamrng code to a platform device driver
>>> and extend caam_probe() to create corresponding platform device.
>>>
>>> Additionally, this change also allows us to remove any access to
>>> struct caam_drv_private in caamrng.c as well as simplify resource
>>> ownership/deallocation via devres.
>>>
>> I would avoid adding platform devices that don't have
>> corresponding DT nodes.
>>
>> There's some generic advice here:
>> https://www.kernel.org/doc/html/latest/driver-api/driver-model/platform.html#legacy-drivers-device-probing
>>
>> and there's also previous experience in caam driver:
>> 6b175685b4a1 crypto: caam/qi - don't allocate an extra platform device
>>
> 
> Hmm, I am not sure how that experience relates to the case we have
> with hwrng, but OK, I'm going to assume that platform driver approach
> is a no-go.
> 
Not specific to hwrng, but platform drivers in general:
    [...]
    SMMU. A platform device allocated using platform_device_register_full()
    is not completely set up - most importantly .dma_configure()
    is not called.

Horia