Re: [PATCH 1/9] crypto: qce: Add core driver implementation

[Stanimir Varbanov]

Hi Courtney,

On 04/09/2014 01:00 AM, Courtney Cavin wrote:
> On Tue, Apr 08, 2014 at 06:26:44PM +0200, Stanimir Varbanov wrote:
>> On 04/04/2014 02:38 AM, Courtney Cavin wrote:
>>> On Thu, Apr 03, 2014 at 06:17:58PM +0200, Stanimir Varbanov wrote:
>>>> This adds core driver files. The core part is implementing a
>>>> platform driver probe and remove callbaks, the probe enables
>>>> clocks, checks crypto version, initialize and request dma
>>>> channels, create done tasklet and work queue and finally
>>>> register the algorithms into crypto subsystem.
>>>>
>>>> Signed-off-by: Stanimir Varbanov <svarbanov@xxxxxxxxxx>
>>>> ---
>>>>  drivers/crypto/qce/core.c | 333 ++++++++++++++++++++++++++++++++++++++++++++++
>>>>  drivers/crypto/qce/core.h |  69 ++++++++++
>>>>  2 files changed, 402 insertions(+)
>>>>  create mode 100644 drivers/crypto/qce/core.c
>>>>  create mode 100644 drivers/crypto/qce/core.h
>>>>
>>>> diff --git a/drivers/crypto/qce/core.c b/drivers/crypto/qce/core.c
>>> [...]
>>>> +static struct qce_algo_ops qce_ops[] = {
>>>> +	{
>>>> +		.type = CRYPTO_ALG_TYPE_ABLKCIPHER,
>>>> +		.register_alg = qce_ablkcipher_register,
>>>> +	},
>>>> +	{
>>>> +		.type = CRYPTO_ALG_TYPE_AHASH,
>>>> +		.register_alg = qce_ahash_register,
>>>> +	},
>>>> +};
>>>> +
>>>> +static void qce_unregister_algs(struct qce_device *qce)
>>>> +{
>>>> +	struct qce_alg_template *tmpl, *n;
>>>> +
>>>> +	list_for_each_entry_safe(tmpl, n, &qce->alg_list, entry) {
>>>> +		if (tmpl->crypto_alg_type == CRYPTO_ALG_TYPE_AHASH)
>>>> +			crypto_unregister_ahash(&tmpl->alg.ahash);
>>>> +		else
>>>> +			crypto_unregister_alg(&tmpl->alg.crypto);
>>>> +
>>>> +		list_del(&tmpl->entry);
>>>> +		kfree(tmpl);
>>>
>>> I find this whole memory/list management to be very disorganised.
>>> ops->register_alg() is supposed to allocate this item--more precisely,
>>> multiple items--using something that must be able to be kfree'd
>>> directly, register it with the crypto core, and put it on this list
>>> manually.  Here we unregister/remove/free this in the core.  Josh's
>>> recommendation of a unregister_alg callback might help, but it all
>>> remains a bit unclear with register_alg/unregister_alg managing X
>>> algorithms per call. 
>>>
>>> Additionally, above you have qce_ops, which clearly defines the
>>> operations for specific algorithms types/groups, which in later patches
>>> are shown to be seperated out into independent implementations.
>>>
>>> From what I can tell, this seems to be a framework with built-in yet
>>> independent crypto implementations which call the crypto API directly.
>>>
>>> It would be more logical to me if this was seperated out into a
>>> "library/core" API, with the individual implementations as platform
>>> drivers of their own.  Then they can register with the core, managing
>>> memory how they please.
>>>
>>> What am I missing?
>>>
>>
>> No, you have not miss nothing.
>>
>> OK I see your point. I made few changes in the core, killed the alg_list
>> and its manipulation function and added a .unregister_algs operation.
>> Now every type of algorithm will handle all core crypto api functions
>> itself. Also I'm using devm_kzalloc() in .register_algs when allocating
>> memory for qce_alg_template structures to avoid kfree(). The callbacks
>> async_req_queue/done are now embedded in qce_device structure and they
>> are invoked directly from algorithm implementations. Thus I have
>> separated the interfaces: functions implemented in core part of the
>> driver and struct qce_algo_ops having the function pointers implemented
>> by every type of algorithm.
>>
>> If you don't have some objections I can send out a version 2.
> 
> 
> Well, I'd have to see the code to understand clearly what you are
> describing here, but the mention of devm_kzalloc() concerns me.  The
> only device which I currently see to which this allocation could be
> associated is the single platform_device in the core.  Associating the
> memory with the core gets rid of the explicit call to kfree() by the
> core, but doesn't rearrange the way the memory is actually managed.

OK, no worries. I have no strong opinion and will use kzalloc() then.

> 
> If you have changed it so that each algorithm "block" has its own
> device, then this would seem more reasonable, but I don't see that in
> the explanation you provided.

No, that is not possible. The platform driver must be one because the
register space is common. The hardware accesses must be serialised from
core part of the driver.

-- 
regards,
Stan
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/