Re: [PATCH V3 6/6] crypto/nx: Add P9 NX support for 842 compression engine

[Michael Ellerman]

Hi Haren,

Some comments inline ...

Haren Myneni  writes:

> diff --git a/drivers/crypto/nx/nx-842-powernv.c 
> b/drivers/crypto/nx/nx-842-powernv.c
> index c0dd4c7e17d3..13089a0b9dfa 100644
> --- a/drivers/crypto/nx/nx-842-powernv.c
> +++ b/drivers/crypto/nx/nx-842-powernv.c
> @@ -32,6 +33,9 @@ MODULE_ALIAS_CRYPTO("842-nx");
>  
>  #define WORKMEM_ALIGN(CRB_ALIGN)
>  #define CSB_WAIT_MAX (5000) /* ms */
> +#define VAS_RETRIES  (10)

Where does that number come from?

Do we have any idea what the trade off is between retrying vs just
falling back to doing the request in software?

> +/* # of requests allowed per RxFIFO at a time. 0 for unlimited */
> +#define MAX_CREDITS_PER_RXFIFO   (1024)
>  
>  struct nx842_workmem {
>   /* Below fields must be properly aligned */
> @@ -42,16 +46,27 @@ struct nx842_workmem {
>  
>   ktime_t start;
>  
> + struct vas_window *txwin;   /* Used with VAS function */

I don't understand how it makes sense to put txwin and start between the
fields above, and the padding.

If the workmem pointer you receive is not aligned, then PTR_ALIGN() will
advance it and mean you end up writing over start and txwin.

That's probably not your bug, the code is already like that.

>   char padding[WORKMEM_ALIGN]; /* unused, to allow alignment */
>  } __packed __aligned(WORKMEM_ALIGN);

> @@ -576,6 +690,198 @@ static inline void nx842_add_coprocs_list(struct 
> nx842_coproc *coproc,
>   list_add(>list, _coprocs);
>  }
>  
> +/*
> + * Identify chip ID for each CPU and save coprocesor adddress for the
> + * corresponding NX engine in percpu coproc_inst.
> + * coproc_inst is used in crypto_init to open send window on the NX instance
> + * for the corresponding CPU / chip where the open request is executed.
> + */
> +static void nx842_set_per_cpu_coproc(struct nx842_coproc *coproc)
> +{
> + unsigned int i, chip_id;
> +
> + for_each_possible_cpu(i) {
> + chip_id = cpu_to_chip_id(i);
> +
> + if (coproc->chip_id == chip_id)
> + per_cpu(coproc_inst, i) = coproc;
> + }
> +}
> +
> +
> +static struct vas_window *nx842_alloc_txwin(struct nx842_coproc *coproc)
> +{
> + struct vas_window *txwin = NULL;
> + struct vas_tx_win_attr txattr;
> +
> + /*
> +  * Kernel requests will be high priority. So open send
> +  * windows only for high priority RxFIFO entries.
> +  */
> + vas_init_tx_win_attr(, coproc->ct);
> + txattr.lpid = 0;/* lpid is 0 for kernel requests */
> + txattr.pid = mfspr(SPRN_PID);

Should we be using SPRN_PID here? That makes it appear as if it comes
from the current user process, which seems fishy.

> + /*
> +  * Open a VAS send window which is used to send request to NX.
> +  */
> + txwin = vas_tx_win_open(coproc->vas.id, coproc->ct, );
> + if (IS_ERR(txwin)) {
> + pr_err("ibm,nx-842: Can not open TX window: %ld\n",
> + PTR_ERR(txwin));
> + return NULL;
> + }
> +
> + return txwin;
> +}
> +
> +static int __init vas_cfg_coproc_info(struct device_node *dn, int chip_id,
> + int vasid)
> +{
> + struct vas_window *rxwin = NULL;
> + struct vas_rx_win_attr rxattr;
> + struct nx842_coproc *coproc;
> + u32 lpid, pid, tid, fifo_size;
> + u64 rx_fifo;
> + const char *priority;
> + int ret;
> +
> + ret = of_property_read_u64(dn, "rx-fifo-address", (void *)_fifo);
  
  Unnecessary cast.

> + if (ret) {
> + pr_err("Missing rx-fifo-address property\n");
> + return ret;
> + }
> +
> + ret = of_property_read_u32(dn, "rx-fifo-size", _size);
> + if (ret) {
> + pr_err("Missing rx-fifo-size property\n");
> + return ret;
> + }
> +
> + ret = of_property_read_u32(dn, "lpid", );
> + if (ret) {
> + pr_err("Missing lpid property\n");
> + return ret;
> + }
> +
> + ret = of_property_read_u32(dn, "pid", );
> + if (ret) {
> + pr_err("Missing pid property\n");
> + return ret;
> + }
> +
> + ret = of_property_read_u32(dn, "tid", );
> + if (ret) {
> + pr_err("Missing tid property\n");
> + return ret;
> + }
> +
> + ret = of_property_read_string(dn, "priority", );
> + if (ret) {
> + pr_err("Missing priority property\n");
> + return ret;
> + }
> +
> + coproc = kzalloc(sizeof(*coproc), GFP_KERNEL);
> + if (!coproc)
> + return -ENOMEM;
> +
> + if (!strcmp(priority, "High"))
> + coproc->ct = VAS_COP_TYPE_842_HIPRI;
> + else if (!strcmp(priority, "Normal"))
> + coproc->ct = VAS_COP_TYPE_842;
> + else {
> + pr_err("Invalid RxFIFO priority 

Re: [PATCH v2 1/4] crypto: jz4780-rng: Add JZ4780 PRNG devicetree binding documentation

[Rob Herring]

On Fri, Aug 25, 2017 at 10:20 PM, PrasannaKumar Muralidharan
 wrote:
> Hi Rob,
>
> On 26 August 2017 at 03:27, Rob Herring  wrote:
>> On Wed, Aug 23, 2017 at 08:27:04AM +0530, PrasannaKumar Muralidharan wrote:
>>> Add devicetree bindings for hardware pseudo random number generator
>>> present in Ingenic JZ4780 SoC.
>>>
>>> Signed-off-by: PrasannaKumar Muralidharan 
>>> ---
>>> Changes in v2:
>>> * Add "syscon" in CGU node's compatible section
>>> * Make RNG child node of CGU.
>>>
>>>  .../bindings/crypto/ingenic,jz4780-rng.txt   | 20 
>>> 
>>
>> bindings/rng/ for RNG h/w.
>
> There are two subsystem for dealing with RNG hw. Hw_random subsystem
> for true RNG (driver/char/hw_random) and crypto framework for pseudo
> RNG (crypto/ and drviers/crypto). This HW is pseudo RNG so I have
> placed the dt bindings in bindings/crypto as the driver itself is in
> drivers/crypto folder. I am wondering if there is any relation between
> driver folder and bindings folder. Can you please explain the folder
> relation? Should this be put in bindings/rng or bindings/crypto?

There's not a 1-1 relationship though obviously there's a lot of
overlap. I'd still say this should go in bindings/rng.

>>>  1 file changed, 20 insertions(+)
>>>  create mode 100644 
>>> Documentation/devicetree/bindings/crypto/ingenic,jz4780-rng.txt
>>>
>>> diff --git 
>>> a/Documentation/devicetree/bindings/crypto/ingenic,jz4780-rng.txt 
>>> b/Documentation/devicetree/bindings/crypto/ingenic,jz4780-rng.txt
>>> new file mode 100644
>>> index 000..a0c18e5
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/crypto/ingenic,jz4780-rng.txt
>>> @@ -0,0 +1,20 @@
>>> +Ingenic jz4780 RNG driver
>>> +
>>> +Required properties:
>>> +- compatible : Should be "ingenic,jz4780-rng"
>>> +
>>> +Example:
>>> +
>>> +cgu: jz4780-cgu@1000 {
>>> + compatible = "ingenic,jz4780-cgu", "syscon";
>>> + reg = <0x1000 0x100>;
>>> +
>>> + clocks = <>, <>;
>>> + clock-names = "ext", "rtc";
>>> +
>>> + #clock-cells = <1>;
>>> +
>>> + rng: rng@d8 {
>>
>> unit-address requires reg property.
>
> The driver uses regmap to access the registers. In this case reg
> property is not useful. Is reg property still needed? If not, how
> should the node be declared?

What the driver (currently) does is irrelevant to the binding. Your
choice is either add the reg property or name the node just "rng".
Either is fine, but better to have more information than less IMO.

Rob

[PATCH] crypto: drop unnecessary return statements

[Geliang Tang]

Fix checkpatch.pl warnings:

WARNING: void function return statements are not generally useful
FILE: crypto/rmd128.c:218:
FILE: crypto/rmd160.c:261:
FILE: crypto/rmd256.c:233:
FILE: crypto/rmd320.c:280:
FILE: crypto/tcrypt.c:385:
FILE: drivers/crypto/ixp4xx_crypto.c:538:
FILE: drivers/crypto/marvell/cesa.c:81:
FILE: drivers/crypto/ux500/cryp/cryp_core.c:1755:

Signed-off-by: Geliang Tang 
---
 crypto/rmd128.c   | 2 --
 crypto/rmd160.c   | 2 --
 crypto/rmd256.c   | 2 --
 crypto/rmd320.c   | 2 --
 crypto/tcrypt.c   | 1 -
 drivers/crypto/ixp4xx_crypto.c| 1 -
 drivers/crypto/marvell/cesa.c | 2 --
 drivers/crypto/ux500/cryp/cryp_core.c | 1 -
 8 files changed, 13 deletions(-)

diff --git a/crypto/rmd128.c b/crypto/rmd128.c
index 049486e..40e053b 100644
--- a/crypto/rmd128.c
+++ b/crypto/rmd128.c
@@ -213,8 +213,6 @@ static void rmd128_transform(u32 *state, const __le32 *in)
state[2] = state[3] + aa + bbb;
state[3] = state[0] + bb + ccc;
state[0] = ddd;
-
-   return;
 }
 
 static int rmd128_init(struct shash_desc *desc)
diff --git a/crypto/rmd160.c b/crypto/rmd160.c
index de585e5..5f3e6ea 100644
--- a/crypto/rmd160.c
+++ b/crypto/rmd160.c
@@ -256,8 +256,6 @@ static void rmd160_transform(u32 *state, const __le32 *in)
state[3] = state[4] + aa + bbb;
state[4] = state[0] + bb + ccc;
state[0] = ddd;
-
-   return;
 }
 
 static int rmd160_init(struct shash_desc *desc)
diff --git a/crypto/rmd256.c b/crypto/rmd256.c
index 4ec02a7..f50c025 100644
--- a/crypto/rmd256.c
+++ b/crypto/rmd256.c
@@ -228,8 +228,6 @@ static void rmd256_transform(u32 *state, const __le32 *in)
state[5] += bbb;
state[6] += ccc;
state[7] += ddd;
-
-   return;
 }
 
 static int rmd256_init(struct shash_desc *desc)
diff --git a/crypto/rmd320.c b/crypto/rmd320.c
index 770f2cb..e1315e4 100644
--- a/crypto/rmd320.c
+++ b/crypto/rmd320.c
@@ -275,8 +275,6 @@ static void rmd320_transform(u32 *state, const __le32 *in)
state[7] += ccc;
state[8] += ddd;
state[9] += eee;
-
-   return;
 }
 
 static int rmd320_init(struct shash_desc *desc)
diff --git a/crypto/tcrypt.c b/crypto/tcrypt.c
index 0022a18..4b20d99 100644
--- a/crypto/tcrypt.c
+++ b/crypto/tcrypt.c
@@ -381,7 +381,6 @@ static void test_aead_speed(const char *algo, int enc, 
unsigned int secs,
testmgr_free_buf(xbuf);
 out_noxbuf:
kfree(iv);
-   return;
 }
 
 static void test_hash_sg_init(struct scatterlist *sg)
diff --git a/drivers/crypto/ixp4xx_crypto.c b/drivers/crypto/ixp4xx_crypto.c
index dadc4a8..8705b28 100644
--- a/drivers/crypto/ixp4xx_crypto.c
+++ b/drivers/crypto/ixp4xx_crypto.c
@@ -534,7 +534,6 @@ static void release_ixp_crypto(struct device *dev)
NPE_QLEN_TOTAL * sizeof( struct crypt_ctl),
crypt_virt, crypt_phys);
}
-   return;
 }
 
 static void reset_sa_dir(struct ix_sa_dir *dir)
diff --git a/drivers/crypto/marvell/cesa.c b/drivers/crypto/marvell/cesa.c
index 6e7a5c7..b657e7c 100644
--- a/drivers/crypto/marvell/cesa.c
+++ b/drivers/crypto/marvell/cesa.c
@@ -76,8 +76,6 @@ static void mv_cesa_rearm_engine(struct mv_cesa_engine 
*engine)
 
ctx = crypto_tfm_ctx(req->tfm);
ctx->ops->step(req);
-
-   return;
 }
 
 static int mv_cesa_std_process(struct mv_cesa_engine *engine, u32 status)
diff --git a/drivers/crypto/ux500/cryp/cryp_core.c 
b/drivers/crypto/ux500/cryp/cryp_core.c
index 790f7ca..765f53e 100644
--- a/drivers/crypto/ux500/cryp/cryp_core.c
+++ b/drivers/crypto/ux500/cryp/cryp_core.c
@@ -1751,7 +1751,6 @@ static void __exit ux500_cryp_mod_fini(void)
 {
pr_debug("[%s] is called!", __func__);
platform_driver_unregister(_driver);
-   return;
 }
 
 module_init(ux500_cryp_mod_init);
-- 
2.9.3

Re: [PATCH 7/8] crypto: ecdh - constify key

[Tudor Ambarus]

On 04/19/2017 02:07 AM, Stephan Müller wrote:

Signed-off-by: Stephan Mueller 
---
  include/crypto/ecdh.h | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/crypto/ecdh.h b/include/crypto/ecdh.h
index 03a64f6..b5bb149 100644
--- a/include/crypto/ecdh.h
+++ b/include/crypto/ecdh.h
@@ -40,7 +40,7 @@
   */
  struct ecdh {
unsigned short curve_id;
-   char *key;
+   const char *key;
unsigned short key_size;
  };
  



I just came across this and remembered that Stephan already
made a patch, so:

Acked-by: Tudor Ambarus