[PATCH 0/3] Clean up crypto documentation

[PATCH 0/3] Clean up crypto documentation

[Hook, Gary]

Tidy up the crypto documentation by filling in some variable
descriptions, make some grammatical corrections, and enhance
formatting.

---

Gary R Hook (3):
      crypto: doc - Add parameter documentation
      crypto: doc - Describe the crypto engine
      crypto: doc - Fix formatting of new crypto engine content


 Documentation/crypto/api-skcipher.rst  |    2 -
 Documentation/crypto/crypto_engine.rst |  120 ++++++++++++++++++++++----------
 Documentation/crypto/index.rst         |    1 
 include/linux/crypto.h                 |   11 +++
 4 files changed, 95 insertions(+), 39 deletions(-)

--

[PATCH 1/3] crypto: doc - Add parameter documentation

[Hook, Gary]

Fill in missing parameter descriptions for the ompression algorithm,
then pick them up to document for the compression_alg structure.

Signed-off-by: Gary R Hook <gary.hook@amd.com>
---
 Documentation/crypto/api-skcipher.rst |    2 +-
 include/linux/crypto.h                |   11 +++++++++++
 2 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/Documentation/crypto/api-skcipher.rst b/Documentation/crypto/api-skcipher.rst
index 4eec4a93f7e3..20ba08dddf2e 100644
--- a/Documentation/crypto/api-skcipher.rst
+++ b/Documentation/crypto/api-skcipher.rst
@@ -5,7 +5,7 @@ Block Cipher Algorithm Definitions
    :doc: Block Cipher Algorithm Definitions
 
 .. kernel-doc:: include/linux/crypto.h
-   :functions: crypto_alg ablkcipher_alg blkcipher_alg cipher_alg
+   :functions: crypto_alg ablkcipher_alg blkcipher_alg cipher_alg compress_alg
 
 Symmetric Key Cipher API
 ------------------------
diff --git a/include/linux/crypto.h b/include/linux/crypto.h
index 311237b1dab0..4b4e2ffbee74 100644
--- a/include/linux/crypto.h
+++ b/include/linux/crypto.h
@@ -327,6 +327,17 @@ struct cipher_alg {
 	void (*cia_decrypt)(struct crypto_tfm *tfm, u8 *dst, const u8 *src);
 };
 
+/**
+ * struct compress_alg - compression/decompression algorithm
+ * @coa_compress: Compress a buffer of specified length, storing the resulting
+ *		  data in the specified buffer. Return the length of the
+ *		  compressed data in dlen.
+ * @coa_decompress: Decompress the source buffer, storing the uncompressed
+ *		    data in the specified buffer. The length of the data is
+ *		    returned in dlen.
+ *
+ * All fields are mandatory.
+ */
 struct compress_alg {
 	int (*coa_compress)(struct crypto_tfm *tfm, const u8 *src,
 			    unsigned int slen, u8 *dst, unsigned int *dlen);

[PATCH 2/3] crypto: doc - Describe the crypto engine

[Hook, Gary]

Add a reference to the crypto engine documentation to
the index.

Signed-off-by: Gary R Hook <gary.hook@amd.com>
---
 Documentation/crypto/index.rst |    1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/crypto/index.rst b/Documentation/crypto/index.rst
index c4ff5d791233..37cd7fb0ea82 100644
--- a/Documentation/crypto/index.rst
+++ b/Documentation/crypto/index.rst
@@ -19,6 +19,7 @@ for cryptographic use cases, as well as programming examples.
    intro
    architecture
    devel-algos
+   crypto_engine
    userspace-if
    crypto_engine
    api

Re: [PATCH 2/3] crypto: doc - Describe the crypto engine

[Eric Biggers]

On Mon, Jun 24, 2019 at 07:07:49PM +0000, Hook, Gary wrote:
> Add a reference to the crypto engine documentation to
> the index.
> 
> Signed-off-by: Gary R Hook <gary.hook@amd.com>
> ---
>  Documentation/crypto/index.rst |    1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/Documentation/crypto/index.rst b/Documentation/crypto/index.rst
> index c4ff5d791233..37cd7fb0ea82 100644
> --- a/Documentation/crypto/index.rst
> +++ b/Documentation/crypto/index.rst
> @@ -19,6 +19,7 @@ for cryptographic use cases, as well as programming examples.
>     intro
>     architecture
>     devel-algos
> +   crypto_engine
>     userspace-if
>     crypto_engine
>     api
> 

It's already in the list.

- Eric

Re: [PATCH 2/3] crypto: doc - Describe the crypto engine

[Gary R Hook]

On 6/24/19 5:03 PM, Eric Biggers wrote:
> On Mon, Jun 24, 2019 at 07:07:49PM +0000, Hook, Gary wrote:
>> Add a reference to the crypto engine documentation to
>> the index.
>>
>> Signed-off-by: Gary R Hook <gary.hook@amd.com>
>> ---
>>   Documentation/crypto/index.rst |    1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/Documentation/crypto/index.rst b/Documentation/crypto/index.rst
>> index c4ff5d791233..37cd7fb0ea82 100644
>> --- a/Documentation/crypto/index.rst
>> +++ b/Documentation/crypto/index.rst
>> @@ -19,6 +19,7 @@ for cryptographic use cases, as well as programming examples.
>>      intro
>>      architecture
>>      devel-algos
>> +   crypto_engine
>>      userspace-if
>>      crypto_engine
>>      api
>>
> 
> It's already in the list.

Gah! And at the moment I can't remember why that even got put there.

Apologies.

grh

[PATCH 3/3] crypto: doc - Fix formatting of new crypto engine content

[Hook, Gary]

Tidy up the formatting/grammar in crypto_engine.rst. Use lists where
appropriate.

Signed-off-by: Gary R Hook <gary.hook@amd.com>
---
 Documentation/crypto/crypto_engine.rst |  120 ++++++++++++++++++++++----------
 1 file changed, 82 insertions(+), 38 deletions(-)

diff --git a/Documentation/crypto/crypto_engine.rst b/Documentation/crypto/crypto_engine.rst
index 1d56221dfe35..18e3a9afa444 100644
--- a/Documentation/crypto/crypto_engine.rst
+++ b/Documentation/crypto/crypto_engine.rst
@@ -1,50 +1,94 @@
-=============
-CRYPTO ENGINE
+.. SPDX-License-Identifier: GPL-2.0
+
+Crypto Engine
 =============
 
 Overview
 --------
-The crypto engine API (CE), is a crypto queue manager.
+
+The crypto engine (CE) API is a crypto queue manager.
 
 Requirement
 -----------
-You have to put at start of your tfm_ctx the struct crypto_engine_ctx::
 
-  struct your_tfm_ctx {
-        struct crypto_engine_ctx enginectx;
-        ...
-  };
+You must put, at the start of your transform context your_tfm_ctx, the structure
+crypto_engine:
+
+::
 
-Why: Since CE manage only crypto_async_request, it cannot know the underlying
-request_type and so have access only on the TFM.
-So using container_of for accessing __ctx is impossible.
-Furthermore, the crypto engine cannot know the "struct your_tfm_ctx",
-so it must assume that crypto_engine_ctx is at start of it.
+
+	struct your_tfm_ctx {
+		struct crypto_engine engine;
+		...
+	};
+
+
+The crypto engine only manages asynchronous requests in the form of
+crypto_async_request. It cannot know the underlying request type and thus only
+has access to the transform structure. It is not possible to access the context
+using container_of. In addition, the engine knows nothing about your
+structure "``struct your_tfm_ctx``". The engine assumes (requires) the placement
+of the known member ``struct crypto_engine`` at the beginning.
 
 Order of operations
 -------------------
-You have to obtain a struct crypto_engine via crypto_engine_alloc_init().
-And start it via crypto_engine_start().
-
-Before transferring any request, you have to fill the enginectx.
-- prepare_request: (taking a function pointer) If you need to do some processing before doing the request
-- unprepare_request: (taking a function pointer) Undoing what's done in prepare_request
-- do_one_request: (taking a function pointer) Do encryption for current request
-
-Note: that those three functions get the crypto_async_request associated with the received request.
-So your need to get the original request via container_of(areq, struct yourrequesttype_request, base);
-
-When your driver receive a crypto_request, you have to transfer it to
-the cryptoengine via one of:
-- crypto_transfer_ablkcipher_request_to_engine()
-- crypto_transfer_aead_request_to_engine()
-- crypto_transfer_akcipher_request_to_engine()
-- crypto_transfer_hash_request_to_engine()
-- crypto_transfer_skcipher_request_to_engine()
-
-At the end of the request process, a call to one of the following function is needed:
-- crypto_finalize_ablkcipher_request
-- crypto_finalize_aead_request
-- crypto_finalize_akcipher_request
-- crypto_finalize_hash_request
-- crypto_finalize_skcipher_request
+
+You are required to obtain a struct crypto_engine via ``crypto_engine_alloc_init()``.
+Start it via ``crypto_engine_start()``. When finished with your work, shut down the
+engine using ``crypto_engine_stop()`` and destroy the engine with
+``crypto_engine_exit()``.
+
+Before transferring any request, you have to fill the context enginectx by
+providing functions for the following:
+
+* ``prepare_crypt_hardware``: Called once before any prepare functions are
+  called.
+
+* ``unprepare_crypt_hardware``: Called once after all unprepare functions have
+  been called.
+
+* ``prepare_cipher_request``/``prepare_hash_request``: Called before each
+  corresponding request is performed. If some processing or other preparatory
+  work is required, do it here.
+
+* ``unprepare_cipher_request``/``unprepare_hash_request``: Called after each
+  request is handled. Clean up / undo what was done in the prepare function.
+
+* ``cipher_one_request``/``hash_one_request``: Handle the current request by
+  performing the operation.
+
+Note that these functions access the crypto_async_request structure
+associated with the received request. You are able to retrieve the original
+request by using:
+
+::
+
+
+	container_of(areq, struct yourrequesttype_request, base);
+
+
+
+When your driver receives a crypto_request, you must to transfer it to
+the crypto engine via one of:
+
+* ``crypto_transfer_ablkcipher_request_to_engine()``
+
+* ``crypto_transfer_aead_request_to_engine()``
+
+* ``crypto_transfer_akcipher_request_to_engine()``
+
+* ``crypto_transfer_hash_request_to_engine()``
+
+* ``crypto_transfer_skcipher_request_to_engine()``
+
+At the end of the request process, a call to one of the following functions is needed:
+
+* ``crypto_finalize_ablkcipher_request()``
+
+* ``crypto_finalize_aead_request()``
+
+* ``crypto_finalize_akcipher_request()``
+
+* ``crypto_finalize_hash_request()``
+
+* ``crypto_finalize_skcipher_request()``

Re: [PATCH 0/3] Clean up crypto documentation

[Joe Perches]

On Mon, 2019-06-24 at 19:07 +0000, Hook, Gary wrote:
> Tidy up the crypto documentation by filling in some variable
> descriptions, make some grammatical corrections, and enhance
> formatting.

While this seems generally OK, please try not to make the
readability of the source _text_ less intelligible just
to enhance the output formatting of the html.

e.g.:

Unnecessary blank lines separating function descriptions
Removing space alignment from bullet point descriptions

etc..,

Re: [PATCH 0/3] Clean up crypto documentation

[Gary R Hook]

On 6/24/19 2:30 PM, Joe Perches wrote:
> On Mon, 2019-06-24 at 19:07 +0000, Hook, Gary wrote:
>> Tidy up the crypto documentation by filling in some variable
>> descriptions, make some grammatical corrections, and enhance
>> formatting.
> 
> While this seems generally OK, please try not to make the
> readability of the source _text_ less intelligible just
> to enhance the output formatting of the html.
> 
> e.g.:
> 
> Unnecessary blank lines separating function descriptions
> Removing space alignment from bullet point descriptions

Apologies. I generally consider white space a Good Thing,
but will take your note and not do that. The blank lines I
added do not affect the output, so I should not have done
that.

Also, I turned sentences into bulleted lists here, so I'm not
clear on whether that was a Bad Thing or not. Seems more legible
to me all the way around, but I clearly could be incorrect. I
agree that mucking with alignment is a bad thing, and would not
intentionally do so. That said, if you would please elaborate on
any mistakes I've made?

Finally, would you prefer a v2 of the patch set? Happy to do
whatever is preferred, of course.

grh

Re: [PATCH 0/3] Clean up crypto documentation

[Joe Perches]

On Mon, 2019-06-24 at 20:06 +0000, Gary R Hook wrote:

Hi Gary.

> On 6/24/19 2:30 PM, Joe Perches wrote:
> > On Mon, 2019-06-24 at 19:07 +0000, Hook, Gary wrote:
> > > Tidy up the crypto documentation by filling in some variable
> > > descriptions, make some grammatical corrections, and enhance
> > > formatting.
> > 
> > While this seems generally OK, please try not to make the
> > readability of the source _text_ less intelligible just
> > to enhance the output formatting of the html.
> > 
> > e.g.:
> > 
> > Unnecessary blank lines separating function descriptions
> > Removing space alignment from bullet point descriptions
> 
> Apologies. I generally consider white space a Good Thing,
> but will take your note and not do that. The blank lines I
> added do not affect the output, so I should not have done
> that.
> 
> Also, I turned sentences into bulleted lists here, so I'm not
> clear on whether that was a Bad Thing or not.

To me, using bulleted lists are not a bad thing at all
but are quite the opposite for humans to read.

> Seems more legible
> to me all the way around, but I clearly could be incorrect.

Not at all.

> I agree that mucking with alignment is a bad thing, and would not
> intentionally do so. That said, if you would please elaborate on
> any mistakes I've made?
> 
> Finally, would you prefer a v2 of the patch set? Happy to do
> whatever is preferred, of course.

Whatever Jonathan decides is fine with me.
Mine was just a plea to avoid unnecessarily
making the source text harder to read as
that's what I mostly use.

I don't know if this extension is valid yet, but
I believe just using <function_name>() is more
readable as text than ``<function_name>`` or
:c:func:`<function_name>`

https://lore.kernel.org/lkml/20190425200125.12302-1-corbet@lwn.net/T/

I prefer the automatic approach over the manual
marking of functions as ideally sphinx formatting
should not overly impact the source text.

Re: [PATCH 0/3] Clean up crypto documentation

[Jonathan Corbet]

On Mon, 24 Jun 2019 13:29:42 -0700
Joe Perches <joe@perches.com> wrote:

> > Finally, would you prefer a v2 of the patch set? Happy to do
> > whatever is preferred, of course.  
> 
> Whatever Jonathan decides is fine with me.
> Mine was just a plea to avoid unnecessarily
> making the source text harder to read as
> that's what I mostly use.

Usually Herbert seems to take crypto docs, so it's not necessarily up to
me :)

I don't see much that's objectionable here.  But...

> I don't know if this extension is valid yet, but
> I believe just using <function_name>() is more
> readable as text than ``<function_name>`` or
> :c:func:`<function_name>`

It's been "valid" since I wrote it...it's just not upstream yet :)  I
expect it to be in 5.3, though.  So the best way to refer to a kernel
function, going forward, is just function() with no markup needed.

Thanks,

jon

Re: [PATCH 0/3] Clean up crypto documentation

[Joe Perches]

On Mon, 2019-06-24 at 14:37 -0600, Jonathan Corbet wrote:
> On Mon, 24 Jun 2019 13:29:42 -0700
> Joe Perches <joe@perches.com> wrote:
> 
> > > Finally, would you prefer a v2 of the patch set? Happy to do
> > > whatever is preferred, of course.  
> > 
> > Whatever Jonathan decides is fine with me.
> > Mine was just a plea to avoid unnecessarily
> > making the source text harder to read as
> > that's what I mostly use.
> 
> Usually Herbert seems to take crypto docs, so it's not necessarily up to
> me :)
> 
> I don't see much that's objectionable here.  But...
> 
> > I don't know if this extension is valid yet, but
> > I believe just using <function_name>() is more
> > readable as text than ``<function_name>`` or
> > :c:func:`<function_name>`
> 
> It's been "valid" since I wrote it...it's just not upstream yet :)  I
> expect it to be in 5.3, though.  So the best way to refer to a kernel
> function, going forward, is just function() with no markup needed.

When that's actually "valid" I suggest doing:

$ git ls-files -- 'Documentation/*.rst' | \
 xargs perl -pi -e 's/:c:func:`(\w+)(?:\(\))?`/\1()/g; s/``(\w+)\(\)``/\1()/g'

so function designations in Documentation are simpler to read.

Right now that's:

$ git diff --shortstat Documentation/
 125 files changed, 1680 insertions(+), 1680 deletions(-)

Re: [PATCH 0/3] Clean up crypto documentation

[Gary R Hook]

On 6/24/19 3:37 PM, Jonathan Corbet wrote:
> On Mon, 24 Jun 2019 13:29:42 -0700
> Joe Perches <joe@perches.com> wrote:
> 
>>> Finally, would you prefer a v2 of the patch set? Happy to do
>>> whatever is preferred, of course.
>>
>> Whatever Jonathan decides is fine with me.
>> Mine was just a plea to avoid unnecessarily
>> making the source text harder to read as
>> that's what I mostly use.
> 
> Usually Herbert seems to take crypto docs, so it's not necessarily up to
> me :)
> 
> I don't see much that's objectionable here.  But...
> 
>> I don't know if this extension is valid yet, but
>> I believe just using <function_name>() is more
>> readable as text than ``<function_name>`` or
>> :c:func:`<function_name>`
> 
> It's been "valid" since I wrote it...it's just not upstream yet :)  I
> expect it to be in 5.3, though.  So the best way to refer to a kernel
> function, going forward, is just function() with no markup needed.

So I'm unclear:

1) would you prefer I wait on your 5.3 change being fully committed,
2) add your change to my local tree and use it, then submit an update 
patchset that depends upon it, or
3) re-submit now (using the current method) with suggested changes?

I'm thinking that this will go in after the referenced patch, so (2) is 
the preferred choice?

grh

Re: [PATCH 0/3] Clean up crypto documentation

[Jonathan Corbet]

On Tue, 25 Jun 2019 13:33:27 +0000
Gary R Hook <ghook@amd.com> wrote:

> > It's been "valid" since I wrote it...it's just not upstream yet :)  I
> > expect it to be in 5.3, though.  So the best way to refer to a kernel
> > function, going forward, is just function() with no markup needed.  
> 
> So I'm unclear:
> 
> 1) would you prefer I wait on your 5.3 change being fully committed,
> 2) add your change to my local tree and use it, then submit an update 
> patchset that depends upon it, or
> 3) re-submit now (using the current method) with suggested changes?

I would just not mark up function() at all, and the right thing will
happen to it in the very near future.

Thanks,

jon

Re: [PATCH 0/3] Clean up crypto documentation

[Gary R Hook]

On 6/25/19 8:52 AM, Jonathan Corbet wrote:
> On Tue, 25 Jun 2019 13:33:27 +0000
> Gary R Hook <ghook@amd.com> wrote:
> 
>>> It's been "valid" since I wrote it...it's just not upstream yet :)  I
>>> expect it to be in 5.3, though.  So the best way to refer to a kernel
>>> function, going forward, is just function() with no markup needed.
>>
>> So I'm unclear:
>>
>> 1) would you prefer I wait on your 5.3 change being fully committed,
>> 2) add your change to my local tree and use it, then submit an update
>> patchset that depends upon it, or
>> 3) re-submit now (using the current method) with suggested changes?
> 
> I would just not mark up function() at all, and the right thing will
> happen to it in the very near future.

Done.

I applied your two patches (locally) to verify the result, and it looks 
good to me. In the interim, I think it's NBD.

Thanks much.

grh