[PATCH] crypto: clear htmldocs build warnings for crypto/hash

[Tobin C. Harding]

SPHINX build emits multiple warnings of kind:

warning: duplicate section name 'Note'

(when building kernel via make target 'htmldocs')

This is caused by repeated use of comments of form:

* Note: soau soaeusoa uoe

We can change the format without loss of clarity and clear the build
warnings.

Add '**[mandatory]**' or '**[optional]**' as kernel-doc field element
description prefix

This renders in HTML as (prefixes in bold)

final
[mandatory] Retrieve result from the driver. This function finalizes the
transformation and retrieves the resulting hash from the driver and
pushes it back to upper layers. No data processing happens at this
point unless hardware requires it to finish the transformation (then
the data buffered by the device driver is processed).

Signed-off-by: Tobin C. Harding <m...@tobin.cc>
---

This patch begs the question why the other members of struct ahash_alg
are not marked? Some are marked 'optional' some 'mandatory'. It would
seem that if the marking were necessary for some members it is necessary
for all to eliminate ambiguity?

thanks

 include/crypto/hash.h | 12 
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/include/crypto/hash.h b/include/crypto/hash.h
index 0ed31fd80242..d1cd75faff40 100644
--- a/include/crypto/hash.h
+++ b/include/crypto/hash.h
@@ -71,12 +71,11 @@ struct ahash_request {
 
 /**
  * struct ahash_alg - asynchronous message digest definition
- * @init: Initialize the transformation context. Intended only to initialize 
the
+ * @init: **[mandatory]** Initialize the transformation context. Intended only 
to initialize the
  *   state of the HASH transformation at the beginning. This shall fill in
  *   the internal structures used during the entire duration of the whole
  *   transformation. No data processing happens at this point.
- *   Note: mandatory.
- * @update: Push a chunk of data into the driver for transformation. This
+ * @update: **[mandatory]** Push a chunk of data into the driver for 
transformation. This
  *function actually pushes blocks of data from upper layers into the
  *driver, which then passes those to the hardware as seen fit. This
  *function must not finalize the HASH transformation by calculating the
@@ -85,20 +84,17 @@ struct ahash_request {
  *context, as this function may be called in parallel with the same
  *transformation object. Data processing can happen synchronously
  *[SHASH] or asynchronously [AHASH] at this point.
- *Note: mandatory.
- * @final: Retrieve result from the driver. This function finalizes the
+ * @final: **[mandatory]** Retrieve result from the driver. This function 
finalizes the
  *transformation and retrieves the resulting hash from the driver and
  *pushes it back to upper layers. No data processing happens at this
  *point unless hardware requires it to finish the transformation
  *(then the data buffered by the device driver is processed).
- *Note: mandatory.
- * @finup: Combination of @update and @final. This function is effectively a
+ * @finup: **[optional]** Combination of @update and @final. This function is 
effectively a
  *combination of @update and @final calls issued in sequence. As some
  *hardware cannot do @update and @final separately, this callback was
  *added to allow such hardware to be used at least by IPsec. Data
  *processing can happen synchronously [SHASH] or asynchronously [AHASH]
  *at this point.
- *Note: optional.
  * @digest: Combination of @init and @update and @final. This function
  * effectively behaves as the entire chain of operations, @init,
  * @update and @final issued in sequence. Just like @finup, this was
-- 
2.7.4

Re: [PATCH 3/3] staging: ccree: simplify ioread/iowrite

[Tobin C. Harding]

On Mon, Nov 06, 2017 at 04:46:54PM +0100, Greg Kroah-Hartman wrote:
> On Mon, Nov 06, 2017 at 10:59:47AM +0200, Gilad Ben-Yossef wrote:
> > On Mon, Nov 6, 2017 at 10:37 AM, Tobin C. Harding <m...@tobin.cc> wrote:
> > > On Mon, Nov 06, 2017 at 06:55:52AM +, Gilad Ben-Yossef wrote:
> > >> Registers ioread/iowrite operations were done via macros,
> > >> sometime using a "magical" implicit parameter.
> > >>
> > >> Replace all register access with simple inline macros.
> > >>
> > >> Signed-off-by: Gilad Ben-Yossef <gi...@benyossef.com>
> > >
> > > Hi,
> > >
> > > Nice work. I had a little trouble following this one. Perhaps you are
> > > doing more than one thing per patch, feel free to ignore me if I am
> > > wrong but it seems you are moving the macro definition of CC_REG to a
> > > different header, adding the new inline functions and doing some other
> > > change that I can't grok (commented on below).
> > >
> > > Perhaps this patch could be broken up.
> > 
> > Thank you Tobin.
> > 
> > The original macro that I am replacing had an assumption of a variable void 
> > *
> > cc_base being defined in the context of the macro being called, even though
> > it was not listed in the explicit parameter list of the macro.
> > 
> > The inline function that replace it instead takes an explicit
> > parameter a pointer to
> > struct ssi_drive data * , who has said cc_base as one of the fields.
> > 
> > As a result several function that took a void * cc_base parameter
> > (which is than only
> > used implicitly via the macro without ever being visibly referenced), now 
> > take
> > struct ssi_drive data * parameter instead which is passed explicitly
> > to the inline
> > function.
> > 
> > These seems to be the places you are referring to. They are cascading 
> > changes
> > resulting from the change in API between the macro and the inline
> > function that replaces it.
> > 
> > I imagine I can try to break that change to two patches but at least
> > in my mind this is artificial
> > and it is a single logical change.
> > 
> > Having said that, if you think otherwise and consider this
> > none-reviewable even after this
> > explanation let me know and  I'd be happy to break it down.
> 
> Nah, this is fine, I'll take it as-is.  Tobin, thanks for the review.

No worries. Greg make sure you yell at me if I start causing you more
work than I'm saving. It's a fine line reviewing patches when you are
not super experienced.

thanks,
Tobin.

Re: [PATCH 3/3] staging: ccree: simplify ioread/iowrite

[Tobin C. Harding]

On Mon, Nov 06, 2017 at 06:55:52AM +, Gilad Ben-Yossef wrote:
> Registers ioread/iowrite operations were done via macros,
> sometime using a "magical" implicit parameter.
> 
> Replace all register access with simple inline macros.
> 
> Signed-off-by: Gilad Ben-Yossef 

Hi,

Nice work. I had a little trouble following this one. Perhaps you are
doing more than one thing per patch, feel free to ignore me if I am
wrong but it seems you are moving the macro definition of CC_REG to a
different header, adding the new inline functions and doing some other
change that I can't grok (commented on below).

Perhaps this patch could be broken up.

> ---
>  drivers/staging/ccree/cc_hal.h   | 33 --
>  drivers/staging/ccree/cc_regs.h  | 35 
>  drivers/staging/ccree/dx_reg_base_host.h | 25 --
>  drivers/staging/ccree/ssi_driver.c   | 47 --
>  drivers/staging/ccree/ssi_driver.h   | 20 +--
>  drivers/staging/ccree/ssi_fips.c | 12 +++
>  drivers/staging/ccree/ssi_pm.c   |  4 +--
>  drivers/staging/ccree/ssi_request_mgr.c  | 57 
> 
>  drivers/staging/ccree/ssi_sysfs.c| 11 +++---
>  9 files changed, 78 insertions(+), 166 deletions(-)
>  delete mode 100644 drivers/staging/ccree/cc_hal.h
>  delete mode 100644 drivers/staging/ccree/cc_regs.h
>  delete mode 100644 drivers/staging/ccree/dx_reg_base_host.h
> 
> diff --git a/drivers/staging/ccree/cc_hal.h b/drivers/staging/ccree/cc_hal.h
> deleted file mode 100644
> index eecc866..000
> --- a/drivers/staging/ccree/cc_hal.h
> +++ /dev/null
> @@ -1,33 +0,0 @@
> -/*
> - * Copyright (C) 2012-2017 ARM Limited or its affiliates.
> - *
> - * This program is free software; you can redistribute it and/or modify
> - * it under the terms of the GNU General Public License version 2 as
> - * published by the Free Software Foundation.
> - *
> - * This program is distributed in the hope that it will be useful,
> - * but WITHOUT ANY WARRANTY; without even the implied warranty of
> - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> - * GNU General Public License for more details.
> - *
> - * You should have received a copy of the GNU General Public License
> - * along with this program; if not, see .
> - */
> -
> -/* pseudo cc_hal.h for cc7x_perf_test_driver (to be able to include code from
> - * CC drivers).
> - */
> -
> -#ifndef __CC_HAL_H__
> -#define __CC_HAL_H__
> -
> -#include 
> -
> -#define READ_REGISTER(_addr) ioread32((_addr))
> -#define WRITE_REGISTER(_addr, _data)  iowrite32((_data), (_addr))
> -
> -#define CC_HAL_WRITE_REGISTER(offset, val) \
> - WRITE_REGISTER(cc_base + (offset), val)
> -#define CC_HAL_READ_REGISTER(offset) READ_REGISTER(cc_base + (offset))
> -
> -#endif
> diff --git a/drivers/staging/ccree/cc_regs.h b/drivers/staging/ccree/cc_regs.h
> deleted file mode 100644
> index 2a8fc73..000
> --- a/drivers/staging/ccree/cc_regs.h
> +++ /dev/null
> @@ -1,35 +0,0 @@
> -/*
> - * Copyright (C) 2012-2017 ARM Limited or its affiliates.
> - *
> - * This program is free software; you can redistribute it and/or modify
> - * it under the terms of the GNU General Public License version 2 as
> - * published by the Free Software Foundation.
> - *
> - * This program is distributed in the hope that it will be useful,
> - * but WITHOUT ANY WARRANTY; without even the implied warranty of
> - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> - * GNU General Public License for more details.
> - *
> - * You should have received a copy of the GNU General Public License
> - * along with this program; if not, see .
> - */
> -
> -/*!
> - * @file
> - * @brief This file contains macro definitions for accessing ARM TrustZone
> - *CryptoCell register space.
> - */
> -
> -#ifndef _CC_REGS_H_
> -#define _CC_REGS_H_
> -
> -#include 
> -
> -#define AXIM_MON_COMP_VALUE GENMASK(DX_AXIM_MON_COMP_VALUE_BIT_SIZE + \
> - DX_AXIM_MON_COMP_VALUE_BIT_SHIFT, \
> - DX_AXIM_MON_COMP_VALUE_BIT_SHIFT)
> -
> -/* Register name mangling macro */
> -#define CC_REG(reg_name) DX_ ## reg_name ## _REG_OFFSET
> -
> -#endif /*_CC_REGS_H_*/
> diff --git a/drivers/staging/ccree/dx_reg_base_host.h 
> b/drivers/staging/ccree/dx_reg_base_host.h
> deleted file mode 100644
> index 47bbadb..000
> --- a/drivers/staging/ccree/dx_reg_base_host.h
> +++ /dev/null
> @@ -1,25 +0,0 @@
> -/*
> - * Copyright (C) 2012-2017 ARM Limited or its affiliates.
> - *
> - * This program is free software; you can redistribute it and/or modify
> - * it under the terms of the GNU General Public License version 2 as
> - * published by the Free Software Foundation.
> - *
> - * This program is distributed in the hope that it will be useful,
> - * but WITHOUT ANY WARRANTY; without even the implied warranty 

Re: [PATCH] staging: ccree: Fix lines longer than 80 characters

[Tobin C. Harding]

On Mon, Oct 23, 2017 at 07:53:18AM -0700, Stephen Brennan wrote:
> Simply break down some long lines and tab-indent them.

Hi Stephen,

Welcome to the Linux kernel. Great that you have put in a patch, you are, 
however, unlikely to see
success fixing 'line over 80' warnings. There are a bunch of arguments for and 
against the line over
80 limit, a web search will surely show them to you. The TL;DR is that it these 
changes do not
_really_ improve the readability of the code, they are just changes to quiet 
the static analysis
tool.

There are a bunch of other white space fixes that are more beneficial, perhaps 
you could wet your
feet with these (incorrect placement of parenthesis for example).

Good luck,
Tobin.

Re: [PATCH v3] staging: ccree: fix boolreturn.cocci warning

[Tobin C. Harding]

Hi Suniel,

Well done with you continued versions. I am being particularly nit picky here 
but since we are
striving for perfection I'm sure will humour me. If English is not your first 
language please
forgive me for picking you up on language subtleties.

On Wed, Oct 18, 2017 at 12:11:55PM +0530, suni...@techveda.org wrote:
> From: Suniel Mahesh <suni...@techveda.org>
> 
> This fixes the following coccinelle warning:
> WARNING: return of 0/1 in function 'ssi_is_hw_key' with return type bool.

This should be a description of the problem, so saying _why_ there is a problem 
or _what_ is wrong
with the code currently that warrants a patch. Sometimes while describing the 
problem you may
include descriptions of the solution especially it is not immediately obvious 
why your proposed
solution fixes the issue being explained. As an extra we shouldn't ever say 
'This patch ...' or
'This does xyz'.

> return "false" instead of 0.

Perfect, this is in imperative mood. Spot on!

> Signed-off-by: Suniel Mahesh <suni...@techveda.org>
> ---
> Changes for v3:
> - Changed the commit log even more to give an accurate
>   description of the changeset as suggested by Toby C.Harding.

My name is Tobin :)

> ---
> Changes for v2:
> - Changed the commit log to give a more accurate description
>   of the changeset as suggested by Toby C.Harding.
> ---
> Note:
> - Patch was built(ARCH=arm) on latest linux-next.
> - No build issues reported, however it was not
>   tested on real hardware.
> - Please discard this changeset, if this is not
>   helping the code look better.
> ---
>  drivers/staging/ccree/ssi_cipher.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/ccree/ssi_cipher.h 
> b/drivers/staging/ccree/ssi_cipher.h
> index c9a83df..f499962 100644
> --- a/drivers/staging/ccree/ssi_cipher.h
> +++ b/drivers/staging/ccree/ssi_cipher.h
> @@ -75,7 +75,7 @@ struct arm_hw_key_info {
>  
>  static inline bool ssi_is_hw_key(struct crypto_tfm *tfm)
>  {
> - return 0;
> + return false;
>  }
>  
>  #endif /* CRYPTO_TFM_REQ_HW_KEY */
> -- 
> 1.9.1
> 

For what it's worth, Reviewed-by: Tobin C. Harding <m...@tobin.cc>

As stated I am being particularly 'nit picky', the commit log is _probably_ 
good enough to be
merged, I am not a maintainer though so it's not really anything to do with me. 
I do know however
that sometimes patches go to the bottom of Greg's list if they have 
comments/suggestions. I mention
this only so you learn more about the process and to help you with successfully 
getting you patches
merged. Keep up the work!

Good luck,
Tobin.

Re: [PATCH v2] staging: ccree: fix boolreturn.cocci warning

[Tobin C. Harding]

On Wed, Oct 18, 2017 at 07:42:53AM +0530, suni...@techveda.org wrote:
> From: Suniel Mahesh 
>
> Return "false" instead of 0.
> 
> This fixes the following coccinelle warning:
> WARNING: return of 0/1 in function 'ssi_is_hw_key' with return type bool.

So close! The order of problem description and fix is inverted. What about

```
coccinelle emits: WARNING: return of 0/1 in function 'ssi_is_hw_key' with
return type bool.

Return "false" instead of 0.
```


Good luck,
Tobin.

Re: [PATCH v2] staging: ccree: Fix bool comparison

[Tobin C. Harding]

On Wed, Oct 18, 2017 at 07:40:14AM +0530, suni...@techveda.org wrote:
> From: Suniel Mahesh 
> 
> Comparision operator "equal to" not required on a variable
> "foo" of type "bool". Bool has only two values, can be used
> directly or with logical not.
> 
> This fixes the following coccinelle warning:
> WARNING: Comparison of bool to 0/1
> 
> Signed-off-by: Suniel Mahesh 

Nice.

Re: [PATCH] staging: ccree: fix boolreturn.cocci warning

[Tobin C. Harding]

On Mon, Oct 16, 2017 at 03:39:57PM +0530, suni...@techveda.org wrote:
> From: Suniel Mahesh 
> 
> This fixes the following coccinelle warning:
> WARNING: return of 0/1 in function 'ssi_is_hw_key' with return type bool.

Perhaps

Coccinelle emits WARNING: return of 0/1 in function 'ssi_is_hw_key' with return 
type bool.

Return 'false' instead of 0.

> Signed-off-by: Suniel Mahesh 
> ---
> Note:
> - Patch was tested and built(ARCH=arm) on latest
>   linux-next.
> - No build issues reported, however it was not
>   tested on real hardware.
> - Please discard this changeset, if this is not
>   helping the code look better.
> ---
>  drivers/staging/ccree/ssi_cipher.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/ccree/ssi_cipher.h 
> b/drivers/staging/ccree/ssi_cipher.h
> index c9a83df..f499962 100644
> --- a/drivers/staging/ccree/ssi_cipher.h
> +++ b/drivers/staging/ccree/ssi_cipher.h
> @@ -75,7 +75,7 @@ struct arm_hw_key_info {
>  
>  static inline bool ssi_is_hw_key(struct crypto_tfm *tfm)
>  {
> - return 0;
> + return false;
>  }

Hope this helps,
Tobin.

Re: [PATCH] staging: ccree: Fix bool comparison

[Tobin C. Harding]

On Mon, Oct 16, 2017 at 03:38:11PM +0530, suni...@techveda.org wrote:
> From: Suniel Mahesh 
> 
> Bool tests don't need comparisons.

This commit log could be a bit longer. You may like to read
Documentation/process/submitting-patches.rst (section 2).

> This fixes the following coccinelle warning:
> WARNING: Comparison of bool to 0/1
> 
> Signed-off-by: Suniel Mahesh 
> ---
> Note:
> - Patch was tested and built(ARCH=arm) on latest
>   linux-next.

Building is _not_ testing.

> - No build issues reported, however it was not
>   tested on real hardware.

This is implicit if you state 'builds on ARM'

> - Please discard this changeset, if this is not
>   helping the code look better.

This is implicit also ;)

> ---
>  drivers/staging/ccree/ssi_request_mgr.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/ccree/ssi_request_mgr.c 
> b/drivers/staging/ccree/ssi_request_mgr.c
> index 2e0df57..942afe2 100644
> --- a/drivers/staging/ccree/ssi_request_mgr.c
> +++ b/drivers/staging/ccree/ssi_request_mgr.c
> @@ -272,7 +272,7 @@ int send_request(
>   unsigned int max_required_seq_len = (total_seq_len +
>   ((ssi_req->ivgen_dma_addr_len == 0) ? 0 
> :
>   SSI_IVPOOL_SEQ_LEN) +
> - ((is_dout == 0) ? 1 : 0));
> + (!is_dout ? 1 : 0));

Perhaps

-   ((is_dout == 0) ? 1 : 0));
+   (is_dout ? 0 : 1)

Good luck,
Tobin.

Re: [PATCH 01/27] Drivers: ccree: ssi_sysfs.h - align block comments

[Tobin C. Harding]

On Wed, May 24, 2017 at 04:40:32PM +1200, Derek Robson wrote:
> Fixed block comment alignment, Style fix only
> Found using checkpatch

It's 'one thing per patch', this whole set does one thing. You may
like to submit it as a single patch. You won't need the file name in
the commit summary then also :)

Good luck,
Tobin.

> 
> Signed-off-by: Derek Robson 
> ---
>  drivers/staging/ccree/ssi_sysfs.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/ccree/ssi_sysfs.h 
> b/drivers/staging/ccree/ssi_sysfs.h
> index cd456c5dccc4..4893e014adf7 100644
> --- a/drivers/staging/ccree/ssi_sysfs.h
> +++ b/drivers/staging/ccree/ssi_sysfs.h
> @@ -15,7 +15,7 @@
>   */
>  
>  /* \file ssi_sysfs.h
> -   ARM CryptoCell sysfs APIs
> + * ARM CryptoCell sysfs APIs
>   */
>  
>  #ifndef __SSI_SYSFS_H__
> -- 
> 2.12.2
> 
> ___
> devel mailing list
> de...@linuxdriverproject.org
> http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel