Linux cryptographic layer development
 help / color / mirror / Atom feed
* Re: [PATCH v2 2/5] async_tx: Handle DMA devices having support for fewer PQ coefficients
From: Anup Patel @ 2017-02-09  9:29 UTC (permalink / raw)
  To: Dan Williams
  Cc: Vinod Koul, Rob Herring, Mark Rutland, Herbert Xu,
	David S . Miller, Jassi Brar, Ray Jui, Scott Branden, Jon Mason,
	Rob Rice, BCM Kernel Feedback,
	dmaengine-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Device Tree,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	linux-crypto-u79uwXL29TY76Z2rM5mHXA, linux-raid
In-Reply-To: <CAPcyv4iFJXxvJFrUs2jtwP9GX5NcJ8LiEDHeZ5b1fwjCAToe5w-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>

On Wed, Feb 8, 2017 at 9:54 PM, Dan Williams <dan.j.williams-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> wrote:
> On Wed, Feb 8, 2017 at 12:57 AM, Anup Patel <anup.patel-dY08KVG/lbpWk0Htik3J/w@public.gmane.org> wrote:
>> On Tue, Feb 7, 2017 at 11:46 PM, Dan Williams <dan.j.williams-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> wrote:
>>> On Tue, Feb 7, 2017 at 1:02 AM, Anup Patel <anup.patel-dY08KVG/lbpWk0Htik3J/w@public.gmane.org> wrote:
>>>> On Tue, Feb 7, 2017 at 1:57 PM, Dan Williams <dan.j.williams-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> wrote:
>>>>> On Tue, Feb 7, 2017 at 12:16 AM, Anup Patel <anup.patel-dY08KVG/lbpWk0Htik3J/w@public.gmane.org> wrote:
>>>>>> The DMAENGINE framework assumes that if PQ offload is supported by a
>>>>>> DMA device then all 256 PQ coefficients are supported. This assumption
>>>>>> does not hold anymore because we now have BCM-SBA-RAID offload engine
>>>>>> which supports PQ offload with limited number of PQ coefficients.
>>>>>>
>>>>>> This patch extends async_tx APIs to handle DMA devices with support
>>>>>> for fewer PQ coefficients.
>>>>>>
>>>>>> Signed-off-by: Anup Patel <anup.patel-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
>>>>>> Reviewed-by: Scott Branden <scott.branden-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
>>>>>
>>>>> I don't like this approach. Define an interface for md to query the
>>>>> offload engine once at the beginning of time. We should not be adding
>>>>> any new extensions to async_tx.
>>>>
>>>> Even if we do capability checks in Linux MD, we still need a way
>>>> for DMAENGINE drivers to advertise number of PQ coefficients
>>>> handled by the HW.
>>>>
>>>> I agree capability checks should be done once in Linux MD but I don't
>>>> see why this has to be part of BCM-SBA-RAID driver patches. We need
>>>> separate patchsets to address limitations of async_tx framework.
>>>
>>> Right, separate enabling before we pile on new hardware support to a
>>> known broken framework.
>>
>> Linux Async Tx not broken framework. The issue is:
>> 1. Its not complete enough
>> 2. Its not optimized for very high through-put offload engines
>
> I'm not understanding your point. I'm nak'ing this change to add yet
> more per-transaction capability checking to async_tx. I don't like the
> DMA_HAS_FEWER_PQ_COEF flag, especially since it is equal to
> DMA_HAS_PQ_CONTINUE. I'm not asking for all of async_tx's problems to
> be fixed before this new hardware support, I'm simply saying we should
> start the process of moving offload-engine capability checking to the
> raid code.

The DMA_HAS_FEWER_PQ_COEF is not equal to
DMA_HAS_PQ_CONTINUE.

I will try to drop this patch and take care of unsupported PQ
coefficients in BCM-SBA-RAID driver itself even if this means
doing some computations in BCM-SBA-RAID driver itself.

Regards,
Anup
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* Re: [PATCH] Revert "hwrng: core - zeroize buffers with random data"
From: Stephan Müller @ 2017-02-09  9:32 UTC (permalink / raw)
  To: Linus Torvalds
  Cc: David Daney, Linux Crypto Mailing List, Matt Mackall, Herbert Xu,
	Linux Kernel Mailing List
In-Reply-To: <CA+55aFwBdG42kk8J0t2tufHE=OUk4qWXEkFyySNgU3Ru2TA-tQ@mail.gmail.com>

Am Mittwoch, 8. Februar 2017, 17:57:23 CET schrieb Linus Torvalds:

Hi Linus,

> Stephan, Herbert? The zeroes in /dev/hwrng output are obviously
> complete crap, so there's something badly wrong somewhere.
> 
> The locking, for example, is completely buggered. There's even a
> comment about it, but that comment makes the correct observation of
> "but y'know: randomness". But the memset() also being outside the lock
> makes a complete joke of the whole thing.

That is correct, the patch is broken and should be reverted.

May I ask, however, why the add_device_randomness is invoked outside the lock 
as well. Shouldn't it be moved into the lock?

Besides, I still would think that a memset(0) is needed because we have long-
living memory locations (rng_buffer and rng_fillbuf) which may be overwritten 
sporadically. As these memory locations are expected to hold entropy, they 
should be overwritten as soon as the data is processed. Obviously, such memset 
must be done within the lock.

Ciao
Stephan

^ permalink raw reply

* Re: [PATCH v7 0/5] Update LZ4 compressor module
From: Sven Schmidt @ 2017-02-09 10:56 UTC (permalink / raw)
  To: Minchan Kim
  Cc: akpm, bongkyu.kim, rsalvaterra, sergey.senozhatsky, gregkh,
	linux-kernel, herbert, davem, linux-crypto, anton, ccross,
	keescook, tony.luck
In-Reply-To: <20170208233121.GA16728@bbox>

Hey Minchan,

On Thu, Feb 09, 2017 at 08:31:21AM +0900, Minchan Kim wrote:
> Hello Sven,
> 
> On Sun, Feb 05, 2017 at 08:09:03PM +0100, Sven Schmidt wrote:
> > 
> > This patchset is for updating the LZ4 compression module to a version based
> > on LZ4 v1.7.3 allowing to use the fast compression algorithm aka LZ4 fast
> > which provides an "acceleration" parameter as a tradeoff between
> > high compression ratio and high compression speed.
> > 
> > We want to use LZ4 fast in order to support compression in lustre
> > and (mostly, based on that) investigate data reduction techniques in behalf of
> > storage systems.
> > 
> > Also, it will be useful for other users of LZ4 compression, as with LZ4 fast
> > it is possible to enable applications to use fast and/or high compression
> > depending on the usecase.
> > For instance, ZRAM is offering a LZ4 backend and could benefit from an updated
> > LZ4 in the kernel.
> > 
> > LZ4 homepage: http://www.lz4.org/
> > LZ4 source repository: https://github.com/lz4/lz4
> > Source version: 1.7.3
> > 
> > Benchmark (taken from [1], Core i5-4300U @1.9GHz):
> > ----------------|--------------|----------------|----------
> > Compressor      | Compression  | Decompression  | Ratio
> > ----------------|--------------|----------------|----------
> > memcpy          |  4200 MB/s   |  4200 MB/s     | 1.000
> > LZ4 fast 50     |  1080 MB/s   |  2650 MB/s     | 1.375
> > LZ4 fast 17     |   680 MB/s   |  2220 MB/s     | 1.607
> > LZ4 fast 5      |   475 MB/s   |  1920 MB/s     | 1.886
> > LZ4 default     |   385 MB/s   |  1850 MB/s     | 2.101
> > 
> > [1] http://fastcompression.blogspot.de/2015/04/sampling-or-faster-lz4.html
> > 
> > [PATCH 1/5] lib: Update LZ4 compressor module
> > [PATCH 2/5] lib/decompress_unlz4: Change module to work with new LZ4 module version
> > [PATCH 3/5] crypto: Change LZ4 modules to work with new LZ4 module version
> > [PATCH 4/5] fs/pstore: fs/squashfs: Change usage of LZ4 to work with new LZ4 version
> > [PATCH 5/5] lib/lz4: Remove back-compat wrappers
> 
> Today, I did zram-lz4 performance test with fio in current mmotm and
> found it makes regression about 20%.
> 
> "lz4-update" means current mmots(git://git.cmpxchg.org/linux-mmots.git) so
> applied your 5 patches. (But now sure current mmots has recent uptodate
> patches)
> "revert" means I reverted your 5 patches in current mmots.
> 
>                      revert    lz4-update
> 
>       seq-write       1547       1339      86.55%
>      rand-write      22775      19381      85.10%
>        seq-read       7035       5589      79.45%
>       rand-read      78556      68479      87.17%
>    mixed-seq(R)       1305       1066      81.69%
>    mixed-seq(W)       1205        984      81.66%
>   mixed-rand(R)      17421      14993      86.06%
>   mixed-rand(W)      17391      14968      86.07%

which parts of the output (as well as units) are these values exactly?
I did not work with fio until now, so I think I might ask before misinterpreting my results.
 
> My fio description file
> 
> [global]
> bs=4k
> ioengine=sync
> size=100m
> numjobs=1
> group_reporting
> buffer_compress_percentage=30
> scramble_buffers=0
> filename=/dev/zram0
> loops=10
> fsync_on_close=1
> 
> [seq-write]
> bs=64k
> rw=write
> stonewall
> 
> [rand-write]
> rw=randwrite
> stonewall
> 
> [seq-read]
> bs=64k
> rw=read
> stonewall
> 
> [rand-read]
> rw=randread
> stonewall
> 
> [mixed-seq]
> bs=64k
> rw=rw
> stonewall
> 
> [mixed-rand]
> rw=randrw
> stonewall
> 

Great, this makes it easy for me to reproduce your test.

Thanks,

Sven

^ permalink raw reply

* Re: [PATCH v7 0/5] Update LZ4 compressor module
From: Sven Schmidt @ 2017-02-09 11:02 UTC (permalink / raw)
  To: Eric Biggers
  Cc: Minchan Kim, akpm, bongkyu.kim, rsalvaterra, sergey.senozhatsky,
	gregkh, linux-kernel, herbert, davem, linux-crypto, anton, ccross,
	keescook, tony.luck
In-Reply-To: <20170209002436.GA103792@gmail.com>

Hey Eric,

On Wed, Feb 08, 2017 at 04:24:36PM -0800, Eric Biggers wrote:
> On Thu, Feb 09, 2017 at 08:31:21AM +0900, Minchan Kim wrote:
> > 
> > Today, I did zram-lz4 performance test with fio in current mmotm and
> > found it makes regression about 20%.
> > 
> 
> This may or may not be the cause of the specific regression you're observing,
> but I just noticed that the proposed patch drops a lot of FORCEINLINE
> annotations from upstream LZ4.  The FORCEINLINE's are there for a reason,
> especially for the main decompression and compression functions which are
> basically "templates" that take in different sets of constant parameters, and
> should be left in.  We should #define FORCEINLINE to __always_inline somewhere,
> or just do a s/FORCEINLINE/__always_inline/g.
>

I generally just replaced "FORCE_INLINE" by "static inline". At least I thought so.
I rechecked and realised, I missed at least two of them (why did I not just use "search+replace"?).
So I think it's maybe safer and easier to eventually just use "FORCE_INLINE"
with the definition you suggested. Will try that.
 
> Note that the upstream LZ4 code is very carefully optimized, so we should not,
> in general, be changing things like when functions are force-inlined, what the
> hash table size is, etc.
> 
> [Also, for some reason linux-crypto is apparently still not receiving patch 1/5
> in the series.  It's missing from the linux-crypto archive at
> http://www.spinics.net/lists/linux-crypto/, so it's not just me.]
> 

I don't really know what to do about this. I think the matter is the size of the E-Mail.
Are there filters or something like that? Since in linux-kernel the patch seems to get delivered.
I could otherwise CC you if you wish.

Thanks,

Sven

^ permalink raw reply

* Re: [PATCH v7 0/5] Update LZ4 compressor module
From: Sven Schmidt @ 2017-02-09 11:05 UTC (permalink / raw)
  To: Eric Biggers
  Cc: Minchan Kim, akpm, bongkyu.kim, rsalvaterra, sergey.senozhatsky,
	gregkh, linux-kernel, herbert, davem, linux-crypto, anton, ccross,
	keescook, tony.luck
In-Reply-To: <20170209052425.GA4678@zzz>

Hey Eric,

On Wed, Feb 08, 2017 at 09:24:25PM -0800, Eric Biggers wrote:
> Also I noticed another bug, this time in LZ4_count():
> 
> > #if defined(CONFIG_64BIT)
> > #define LZ4_ARCH64 1
> > #else
> > #define LZ4_ARCH64 0
> > #endif
> ...
> > #ifdef LZ4_ARCH64
> >        if ((pIn < (pInLimit-3))
> >                && (LZ4_read32(pMatch) == LZ4_read32(pIn))) {
> >                pIn += 4; pMatch += 4;
> >        }
> > #endif
> 
> Because of how LZ4_ARCH64 is defined, it needs to be '#if LZ4_ARCH64'.
> 
> But I also think the way upstream LZ4 does 64-bit detection could have just been
> left as-is; it has a function which gets inlined:
> 
> 	static unsigned LZ4_64bits(void) { return sizeof(void*)==8; }
> 
> Eric

does this apply for LZ4_isLittleEndian() as well? As a reminder:
	
	static unsigned LZ4_isLittleEndian(void)
	{
		/* don't use static : performance detrimental */
		const union { U32 u; BYTE c[4]; } one = { 1 };

		return one.c[0];
	}

It is surely easier to read and understand using these functions in favor of the macros.

Thanks,

Sven

^ permalink raw reply

* [PATCH -next] crypto: asymmetric_keys - Fix error return code on failure
From: Wei Yongjun @ 2017-02-09 15:57 UTC (permalink / raw)
  To: David Howells, Herbert Xu; +Cc: Wei Yongjun, keyrings, linux-crypto

From: Wei Yongjun <weiyongjun1@huawei.com>

Fix to return error code -ENOMEM from the akcipher_request_alloc()
error handling case instead of 0.

Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com>
---
 crypto/asymmetric_keys/public_key.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/crypto/asymmetric_keys/public_key.c b/crypto/asymmetric_keys/public_key.c
index 3a23274..3131bba 100644
--- a/crypto/asymmetric_keys/public_key.c
+++ b/crypto/asymmetric_keys/public_key.c
@@ -184,8 +184,10 @@ static int software_key_eds_op(struct kernel_pkey_params *params,
 		return PTR_ERR(tfm);
 
 	req = akcipher_request_alloc(tfm, GFP_KERNEL);
-	if (!req)
+	if (!req) {
+		ret = -ENOMEM;
 		goto error_free_tfm;
+	}
 
 	if (pkey->key_is_private)
 		ret = crypto_akcipher_set_priv_key(tfm,
@@ -268,8 +270,10 @@ int public_key_verify_signature(const struct public_key *pkey,
 		return PTR_ERR(tfm);
 
 	req = akcipher_request_alloc(tfm, GFP_KERNEL);
-	if (!req)
+	if (!req) {
+		ret = -ENOMEM;
 		goto error_free_tfm;
+	}
 
 	if (pkey->key_is_private)
 		ret = crypto_akcipher_set_priv_key(tfm,

^ permalink raw reply related

* Re: [PATCH v2 2/5] async_tx: Handle DMA devices having support for fewer PQ coefficients
From: Dan Williams @ 2017-02-09 16:44 UTC (permalink / raw)
  To: Anup Patel
  Cc: Vinod Koul, Rob Herring, Mark Rutland, Herbert Xu,
	David S . Miller, Jassi Brar, Ray Jui, Scott Branden, Jon Mason,
	Rob Rice, BCM Kernel Feedback, dmaengine@vger.kernel.org,
	Device Tree, linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org, linux-crypto, linux-raid
In-Reply-To: <CAALAos-ua6hVpUqjnJSQ=ysSOKrN67toiT3J808uHC4A_ZV3mg@mail.gmail.com>

On Thu, Feb 9, 2017 at 1:29 AM, Anup Patel <anup.patel@broadcom.com> wrote:
> On Wed, Feb 8, 2017 at 9:54 PM, Dan Williams <dan.j.williams@intel.com> wrote:
>> On Wed, Feb 8, 2017 at 12:57 AM, Anup Patel <anup.patel@broadcom.com> wrote:
>>> On Tue, Feb 7, 2017 at 11:46 PM, Dan Williams <dan.j.williams@intel.com> wrote:
>>>> On Tue, Feb 7, 2017 at 1:02 AM, Anup Patel <anup.patel@broadcom.com> wrote:
>>>>> On Tue, Feb 7, 2017 at 1:57 PM, Dan Williams <dan.j.williams@intel.com> wrote:
>>>>>> On Tue, Feb 7, 2017 at 12:16 AM, Anup Patel <anup.patel@broadcom.com> wrote:
>>>>>>> The DMAENGINE framework assumes that if PQ offload is supported by a
>>>>>>> DMA device then all 256 PQ coefficients are supported. This assumption
>>>>>>> does not hold anymore because we now have BCM-SBA-RAID offload engine
>>>>>>> which supports PQ offload with limited number of PQ coefficients.
>>>>>>>
>>>>>>> This patch extends async_tx APIs to handle DMA devices with support
>>>>>>> for fewer PQ coefficients.
>>>>>>>
>>>>>>> Signed-off-by: Anup Patel <anup.patel@broadcom.com>
>>>>>>> Reviewed-by: Scott Branden <scott.branden@broadcom.com>
>>>>>>
>>>>>> I don't like this approach. Define an interface for md to query the
>>>>>> offload engine once at the beginning of time. We should not be adding
>>>>>> any new extensions to async_tx.
>>>>>
>>>>> Even if we do capability checks in Linux MD, we still need a way
>>>>> for DMAENGINE drivers to advertise number of PQ coefficients
>>>>> handled by the HW.
>>>>>
>>>>> I agree capability checks should be done once in Linux MD but I don't
>>>>> see why this has to be part of BCM-SBA-RAID driver patches. We need
>>>>> separate patchsets to address limitations of async_tx framework.
>>>>
>>>> Right, separate enabling before we pile on new hardware support to a
>>>> known broken framework.
>>>
>>> Linux Async Tx not broken framework. The issue is:
>>> 1. Its not complete enough
>>> 2. Its not optimized for very high through-put offload engines
>>
>> I'm not understanding your point. I'm nak'ing this change to add yet
>> more per-transaction capability checking to async_tx. I don't like the
>> DMA_HAS_FEWER_PQ_COEF flag, especially since it is equal to
>> DMA_HAS_PQ_CONTINUE. I'm not asking for all of async_tx's problems to
>> be fixed before this new hardware support, I'm simply saying we should
>> start the process of moving offload-engine capability checking to the
>> raid code.
>
> The DMA_HAS_FEWER_PQ_COEF is not equal to
> DMA_HAS_PQ_CONTINUE.

#define DMA_HAS_PQ_CONTINUE (1 << 15
#define DMA_HAS_FEWER_PQ_COEF (1 << 15)

> I will try to drop this patch and take care of unsupported PQ
> coefficients in BCM-SBA-RAID driver itself even if this means
> doing some computations in BCM-SBA-RAID driver itself.

That should be nak'd as well, please do capability detection in a
routine that is common to all raid engines.

^ permalink raw reply

* [PATCH next 0/2] crypto: atmel-sha: fix error management
From: Cyrille Pitchen @ 2017-02-09 16:51 UTC (permalink / raw)
  To: herbert, davem, nicolas.ferre
  Cc: Cyrille Pitchen, dan.carpenter, linux-crypto, linux-arm-kernel,
	linux-kernel

Hi all,

this series is based on next-20170209.

The first patch fixes a bug reported by Dan Carpenter. I didn't put a
Fixes tag since the buggy patch is only in linux-next for now so its
commit ID is likely to change when entering Linus' tree.
It fixes a wrong 'sed' command: many "return -EINVAL;" lines should have
been replaced by "return atmel_sha_complete(dd, -EINVAL);" but instead
were replaced by direct calls of "atmel_sha_complete(dd, -EINVAL);".
My bad, sorry for that!

The second patch fixes the way error cases are handled from
atmel_sha_start(). For instance, when atmel_sha_update_req() returned an
error, atmel_sha_final_req() may have been called after anyway. This issue
was present even before my rework of the request queue management, which
introduced atmel_sha_start(), so I guess this is a long time issue.

Finally, for driver maintainance purpose, I'm preparing other patches to
fix the very same and very unlikely issue in both atmel-aes.c and
atmel-sha.c:

atmel_{aes|sha}_hw_init() may fail, for instance if clk_enable() fails.
If so, atmel_{aes|sha}_complete() is called to release the hardware and
report the error. Indeed this _complete() function should be called to
report and handle any error. However it also incondionnally calls
clk_disable(). Hence the following sequence may be buggy:

err = atmel_{aes|sha}_hw_init(dd); /* clk_enable() may have failed. */
if (err)
	return atmel_{aes|sha}_hw_init(dd, err);
	/* clk_disable() is called anyway. */

I didn't finalize my fixes yet for this unlikely bug. Besides the bug was
already present in v4.9 and before, so before introducing
atmel_sha_complete().

from atmel_sha_handle_queue(), the older sequence was:

	err = atmel_sha_hw_init(dd);

	if (err)
		goto err1;

	[...]

err1:
	if (err != -EINPROGRESS)
		/* done_task will not finish it, so do it here */
		atmel_sha_finish_req(req, err);

Best regards,

Cyrille

Cyrille Pitchen (2):
  crypto: atmel-sha: fix missing "return" instructions
  crypto: atmel-sha: fix error management in atmel_sha_start()

 drivers/crypto/atmel-sha.c | 37 +++++++++++++++++++++++++++----------
 1 file changed, 27 insertions(+), 10 deletions(-)

-- 
2.7.4

^ permalink raw reply

* [PATCH next 1/2] crypto: atmel-sha: fix missing "return" instructions
From: Cyrille Pitchen @ 2017-02-09 16:51 UTC (permalink / raw)
  To: herbert, davem, nicolas.ferre
  Cc: Cyrille Pitchen, dan.carpenter, linux-crypto, linux-arm-kernel,
	linux-kernel
In-Reply-To: <cover.1486655562.git.cyrille.pitchen@atmel.com>

This patch fixes a previous patch: "crypto: atmel-sha - update request
queue management to make it more generic".

Indeed the patch above should have replaced the "return -EINVAL;" lines by
"return atmel_sha_complete(dd, -EINVAL);" but instead replaced them by a
simple call of "atmel_sha_complete(dd, -EINVAL);".
Hence all "return" instructions were missing.

Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Cyrille Pitchen <cyrille.pitchen@atmel.com>
---
 drivers/crypto/atmel-sha.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/crypto/atmel-sha.c b/drivers/crypto/atmel-sha.c
index 22d0c0c118da..d6c3d9529d36 100644
--- a/drivers/crypto/atmel-sha.c
+++ b/drivers/crypto/atmel-sha.c
@@ -668,7 +668,7 @@ static int atmel_sha_xmit_dma(struct atmel_sha_dev *dd, dma_addr_t dma_addr1,
 			DMA_MEM_TO_DEV, DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
 	}
 	if (!in_desc)
-		atmel_sha_complete(dd, -EINVAL);
+		return atmel_sha_complete(dd, -EINVAL);
 
 	in_desc->callback = atmel_sha_dma_callback;
 	in_desc->callback_param = dd;
@@ -725,7 +725,7 @@ static int atmel_sha_xmit_dma_map(struct atmel_sha_dev *dd,
 	if (dma_mapping_error(dd->dev, ctx->dma_addr)) {
 		dev_err(dd->dev, "dma %u bytes error\n", ctx->buflen +
 				ctx->block_size);
-		atmel_sha_complete(dd, -EINVAL);
+		return atmel_sha_complete(dd, -EINVAL);
 	}
 
 	ctx->flags &= ~SHA_FLAGS_SG;
@@ -816,7 +816,7 @@ static int atmel_sha_update_dma_start(struct atmel_sha_dev *dd)
 		if (dma_mapping_error(dd->dev, ctx->dma_addr)) {
 			dev_err(dd->dev, "dma %u bytes error\n",
 				ctx->buflen + ctx->block_size);
-			atmel_sha_complete(dd, -EINVAL);
+			return atmel_sha_complete(dd, -EINVAL);
 		}
 
 		if (length == 0) {
@@ -830,7 +830,7 @@ static int atmel_sha_update_dma_start(struct atmel_sha_dev *dd)
 			if (!dma_map_sg(dd->dev, ctx->sg, 1,
 				DMA_TO_DEVICE)) {
 					dev_err(dd->dev, "dma_map_sg  error\n");
-					atmel_sha_complete(dd, -EINVAL);
+					return atmel_sha_complete(dd, -EINVAL);
 			}
 
 			ctx->flags |= SHA_FLAGS_SG;
@@ -844,7 +844,7 @@ static int atmel_sha_update_dma_start(struct atmel_sha_dev *dd)
 
 	if (!dma_map_sg(dd->dev, ctx->sg, 1, DMA_TO_DEVICE)) {
 		dev_err(dd->dev, "dma_map_sg  error\n");
-		atmel_sha_complete(dd, -EINVAL);
+		return atmel_sha_complete(dd, -EINVAL);
 	}
 
 	ctx->flags |= SHA_FLAGS_SG;
-- 
2.7.4

^ permalink raw reply related

* [PATCH next 2/2] crypto: atmel-sha: fix error management in atmel_sha_start()
From: Cyrille Pitchen @ 2017-02-09 16:51 UTC (permalink / raw)
  To: herbert, davem, nicolas.ferre
  Cc: Cyrille Pitchen, dan.carpenter, linux-crypto, linux-arm-kernel,
	linux-kernel
In-Reply-To: <cover.1486655562.git.cyrille.pitchen@atmel.com>

This patch clarifies and fixes how errors should be handled by
atmel_sha_start().

For update operations, the previous code wrongly assumed that
(err != -EINPROGRESS) implies (err == 0). It's wrong because that doesn't
take the error cases (err < 0) into account.

This patch also adds many comments to detail all the possible returned
values and what should be done in each case.

Especially, when an error occurs, since atmel_sha_complete() has already
been called, hence releasing the hardware, atmel_sha_start() must not call
atmel_sha_finish_req() later otherwise atmel_sha_complete() would be
called a second time.

Signed-off-by: Cyrille Pitchen <cyrille.pitchen@atmel.com>
---
 drivers/crypto/atmel-sha.c | 27 ++++++++++++++++++++++-----
 1 file changed, 22 insertions(+), 5 deletions(-)

diff --git a/drivers/crypto/atmel-sha.c b/drivers/crypto/atmel-sha.c
index d6c3d9529d36..0d207dac9aa2 100644
--- a/drivers/crypto/atmel-sha.c
+++ b/drivers/crypto/atmel-sha.c
@@ -1106,22 +1106,39 @@ static int atmel_sha_start(struct atmel_sha_dev *dd)
 						ctx->op, req->nbytes);
 
 	err = atmel_sha_hw_init(dd);
-
 	if (err)
-		goto err1;
+		return atmel_sha_complete(dd, err);
+
+	/*
+	 * atmel_sha_update_req() and atmel_sha_final_req() can return either:
+	 *  -EINPROGRESS: the hardware is busy and the SHA driver will resume
+	 *                its job later in the done_task.
+	 *                This is the main path.
+	 *
+	 * 0: the SHA driver can continue its job then release the hardware
+	 *    later, if needed, with atmel_sha_finish_req().
+	 *    This is the alternate path.
+	 *
+	 * < 0: an error has occurred so atmel_sha_complete(dd, err) has already
+	 *      been called, hence the hardware has been released.
+	 *      The SHA driver must stop its job without calling
+	 *      atmel_sha_finish_req(), otherwise atmel_sha_complete() would be
+	 *      called a second time.
+	 *
+	 * Please note that currently, atmel_sha_final_req() never returns 0.
+	 */
 
 	dd->resume = atmel_sha_done;
 	if (ctx->op == SHA_OP_UPDATE) {
 		err = atmel_sha_update_req(dd);
-		if (err != -EINPROGRESS && (ctx->flags & SHA_FLAGS_FINUP))
+		if (!err && (ctx->flags & SHA_FLAGS_FINUP))
 			/* no final() after finup() */
 			err = atmel_sha_final_req(dd);
 	} else if (ctx->op == SHA_OP_FINAL) {
 		err = atmel_sha_final_req(dd);
 	}
 
-err1:
-	if (err != -EINPROGRESS)
+	if (!err)
 		/* done_task will not finish it, so do it here */
 		atmel_sha_finish_req(req, err);
 
-- 
2.7.4

^ permalink raw reply related

* Re: [PATCH -next] crypto: asymmetric_keys - Fix error return code on failure
From: David Howells @ 2017-02-09 16:57 UTC (permalink / raw)
  To: Wei Yongjun; +Cc: dhowells, Herbert Xu, Wei Yongjun, keyrings, linux-crypto
In-Reply-To: <20170209155724.19737-1-weiyj.lk@gmail.com>

Wei Yongjun <weiyj.lk@gmail.com> wrote:

> --- a/crypto/asymmetric_keys/public_key.c
> +++ b/crypto/asymmetric_keys/public_key.c
> @@ -184,8 +184,10 @@ static int software_key_eds_op(struct kernel_pkey_params *params,
>  		return PTR_ERR(tfm);
>  
>  	req = akcipher_request_alloc(tfm, GFP_KERNEL);
> -	if (!req)
> +	if (!req) {
> +		ret = -ENOMEM;

Ummm...  What should I apply your patch to?

>  		goto error_free_tfm;
> +	}
>  
>  	if (pkey->key_is_private)
>  		ret = crypto_akcipher_set_priv_key(tfm,
> @@ -268,8 +270,10 @@ int public_key_verify_signature(const struct public_key *pkey,
>  		return PTR_ERR(tfm);
>  
>  	req = akcipher_request_alloc(tfm, GFP_KERNEL);
> -	if (!req)
> +	if (!req) {
> +		ret = -ENOMEM;
>  		goto error_free_tfm;

This shouldn't be necessary.  ret should already be -ENOMEM from
initialisation of the variable at the top of the function.

David

^ permalink raw reply

* Re: [PATCH v7 0/5] Update LZ4 compressor module
From: Eric Biggers @ 2017-02-09 18:20 UTC (permalink / raw)
  To: Sven Schmidt
  Cc: Minchan Kim, akpm, bongkyu.kim, rsalvaterra, sergey.senozhatsky,
	gregkh, linux-kernel, herbert, davem, linux-crypto, anton, ccross,
	keescook, tony.luck
In-Reply-To: <20170209110540.GC3575@bierbaron.springfield.local>

On Thu, Feb 09, 2017 at 12:05:40PM +0100, Sven Schmidt wrote:
> > Because of how LZ4_ARCH64 is defined, it needs to be '#if LZ4_ARCH64'.
> > 
> > But I also think the way upstream LZ4 does 64-bit detection could have just been
> > left as-is; it has a function which gets inlined:
> > 
> > 	static unsigned LZ4_64bits(void) { return sizeof(void*)==8; }
> > 
> > Eric
> 
> does this apply for LZ4_isLittleEndian() as well? As a reminder:
> 	
> 	static unsigned LZ4_isLittleEndian(void)
> 	{
> 		/* don't use static : performance detrimental */
> 		const union { U32 u; BYTE c[4]; } one = { 1 };
> 
> 		return one.c[0];
> 	}
> 
> It is surely easier to read and understand using these functions in favor of the macros.

Yes, it makes sense to retain LZ4_isLittleEndian() too, mainly so that the call
sites don't need to be changed and we have less chance of introducing bugs.

I also believe the *implementation* of LZ4_isLittleEndian() using the union
"hack" to detecting endianness works fine and will get optimized correctly,
though we could replace it with an #ifdef __LITTLE_ENDIAN__ if we wanted to.  (I
am sure that upstream LZ4 would do that if it was guaranteed to work, but
upstream LZ4 needs to detect endianness reliably across many more different
compilers, compiler versions, and platforms than the Linux kernel does, and
there is no standard preprocessor macro for doing so.)

Eric

^ permalink raw reply

* Re: [PATCH v7 0/5] Update LZ4 compressor module
From: Eric Biggers @ 2017-02-09 18:29 UTC (permalink / raw)
  To: Sven Schmidt
  Cc: Minchan Kim, akpm, bongkyu.kim, rsalvaterra, sergey.senozhatsky,
	gregkh, linux-kernel, herbert, davem, linux-crypto, anton, ccross,
	keescook, tony.luck
In-Reply-To: <20170209110211.GB3575@bierbaron.springfield.local>

On Thu, Feb 09, 2017 at 12:02:11PM +0100, Sven Schmidt wrote:
> > 
> > [Also, for some reason linux-crypto is apparently still not receiving patch 1/5
> > in the series.  It's missing from the linux-crypto archive at
> > http://www.spinics.net/lists/linux-crypto/, so it's not just me.]
> > 
> 
> I don't really know what to do about this. I think the matter is the size of the E-Mail.
> Are there filters or something like that? Since in linux-kernel the patch seems to get delivered.
> I could otherwise CC you if you wish.
> 

If I'm not mistaken, David Miller is the admin of the mail server on
vger.kernel.org, and he already happens to be Cc'ed on this thread, so maybe he
can answer as to why linux-crypto may be configured differently?

Anyway, since the patch did make it to linux-kernel anyone can still download it
from patchwork if they know where to look:
https://patchwork.kernel.org/patch/9556271/

Eric

^ permalink raw reply

* Re: [PATCH] random: Don't overwrite CRNG state in crng_initialize()
From: Theodore Ts'o @ 2017-02-09 18:32 UTC (permalink / raw)
  To: Alden Tondettar, Arnd Bergmann, Greg Kroah-Hartman, linux-crypto,
	linux-kernel
In-Reply-To: <20170209175600.pnyn7e3iypmd5sis@thunk.org>

OK, I figured out what is going on with your test results.

If you use qemu-system-x86_64 **without** --enable-kvm, then on both
the Debian Jessie version of qemu as well as the Debian Stretch
version of qemu, crng_fast_load() will be called _twice_ before
crng_initialize has a chance to be called.  At least for my kernel
configuration and my CPU.

If you're using a different kernel configuration and a slower CPU,
such that when qemu is doing instruction by instruction emulation,
which slows down the boot sequence **massively**, then that probably
explains your results.

I'm not sure if there are any real life use cases where someone would
be insane enough to use virtualization without enabling KVM, but at
least we know what is happening now.

This makes me feel better, because I've looked at kernel boot messags
from a variety of systems, from big data center servers to laptops to
mobile handsets, and I had **never** seen the sequence of crng
initialization messages that you had been reporting.

						- Ted

^ permalink raw reply

* Re: [PATCH] random: Don't overwrite CRNG state in crng_initialize()
From: Theodore Ts'o @ 2017-02-09 17:56 UTC (permalink / raw)
  To: Alden Tondettar
  Cc: Arnd Bergmann, Greg Kroah-Hartman, linux-crypto, linux-kernel
In-Reply-To: <20170209081322.GA17535@rincewind>

On Thu, Feb 09, 2017 at 01:13:22AM -0700, Alden Tondettar wrote:
> And using:
> 
> $ qemu-system-x86_64 --version
> QEMU emulator version 2.1.2 (Debian 1:2.1+dfsg-12+deb8u6), Copyright (c) 2003-2008 Fabrice Bellard
> $ qemu-system-x86_64 -nographic -enable-kvm -m 1024M -kernel bzImage -append "root=/dev/sda1 loglevel=3 console=ttyS0" hd3

Hmm, I'm not seeing this at *all*.  I assume you must be using Debian
stable?  I'm using Debain Testing, which has much newer version of qemu:

% /usr/bin/kvm --version
QEMU emulator version 2.8.0(Debian 1:2.8+dfsg-2)
Copyright (c) 2003-2016 Fabrice Bellard and the QEMU Project developers

And I'm using:

/usr/bin/kvm -drive file=/usr/projects/xfstests-bld/build-32/kvm-xfstests/test-appliance/root_fs.img,if=virtio,snapshot=on -vga none -nographic -m 1024 --kernel /build/random/arch/x86/boot/bzImage --append "root=/dev/vda console=ttyS0,115200"

See below for an excerpt of the log, but basically we don't get the
first call to crng_fast_load until a good 2 seconds into the boot,
when we're doing device probing.

The only thing I think of is that your version of qemu is spewing a
*huge* number of interrupts to the guest kernel, as soon as interrupts
are enabled, and *before* the kernel even starts trying to talk to the
devices.

That's bad, because it's going to be destroying CPU efficiency of the
VM, and even if we add a safety mechanism to prohibit calling
crng_fast_load until after crng_initialize() has been called, it's
likely that you're not getting much entropy from the interrupts,
because qemu must be spewing interrupts as fast as possible, and there
may not be a lot of unpredictability in that circumstance.  So we can
put in some changes to try to mitigate this, but even with your patch,
there might not be a lot of entropy because qemu is clearly spewing
interrupts at line rate.

Hence, I'd call this a qemu BUG, and I'd strongly suggest you look at
fixing it by upgrading qemu.

       	  				- Ted


[    0.029226] mce: CPU supports 10 MCE banks
[    0.030077] Last level iTLB entries: 4KB 0, 2MB 0, 4MB 0
[    0.033339] Last level dTLB entries: 4KB 0, 2MB 0, 4MB 0, 1GB 0
[    0.041436] Freeing SMP alternatives memory: 20K
[    0.043621] ftrace: allocating 34091 entries in 67 pages
[    0.053659] smpboot: Max logical packages: 1
[    0.056696] Enabling APIC mode:  Flat.  Using 1 I/O APICs
[    0.061854] ..TIMER: vector=0x30 apic1=0 pin1=2 apic2=-1 pin2=-1
[    0.063333] smpboot: CPU0: Intel QEMU Virtual CPU version 2.5+ (family: 0x6, model: 0x6, stepping: 0x3)
[    0.063588] Performance Events: PMU not available due to virtualization, using software events only.
[    0.067555] crng_initialize called
[    0.070107] smp: Bringing up secondary CPUs ...
[    0.072108] smp: Brought up 1 node, 1 CPU
[    0.073351] smpboot: Total of 1 processors activated (4801.01 BogoMIPS)
[    0.077456] devtmpfs: initialized
[    0.079945] clocksource: jiffies: mask: 0xffffffff max_cycles: 0xffffffff, max_idle_ns: 6370867519511994 ns
    ....
[    2.063906] crng: dumping entropy
[    2.065382] crng_fast_load called
[    2.066747] crng_fast_load: 16/64
[    2.066747] crng_fast_load: 16
[    2.073526] tsc: Refined TSC clocksource calibration: 2399.998 MHz
[    2.076219] clocksource: tsc: mask: 0xffffffffffffffff max_cycles: 0x229835b7123, max_idle_ns: 440795242976 ns
[    2.134486] ata2.00: ATAPI: QEMU DVD-ROM, 2.5+, max UDMA/100
[    2.144405] ata2.00: configured for MWDMA2
[    2.153349] scsi 1:0:0:0: CD-ROM            QEMU     QEMU DVD-ROM     2.5+ PQ: 0 ANSI: 5
[    2.187210] sr 1:0:0:0: [sr0] scsi3-mmc drive: 4x/4x cd/rw xa/form2 tray
[    2.194289] cdrom: Uniform CD-ROM driver Revision: 3.20
[    2.205026] sr 1:0:0:0: Attached scsi generic sg0 type 5
[    2.277461] crng: dumping entropy
[    2.279017] crng_fast_load called
[    2.279017] crng_fast_load: 32/64
[    2.279017] crng_fast_load: 16
[    2.720393] crng: dumping entropy
[    2.723448] crng_fast_load called
[    2.723448] crng_fast_load: 48/64
[    2.723448] crng_fast_load: 16
[    2.744182] input: ImExPS/2 Generic Explorer Mouse as /devices/platform/i8042/serio1/input/input3
[    2.760954] EXT4-fs (vda): couldn't mount as ext3 due to feature incompatibilities
[    2.774648] EXT4-fs (vda): mounted filesystem with ordered data mode. Opts: (null)
[    2.779939] VFS: Mounted root (ext4 filesystem) readonly on device 254:0.
[    2.785356] devtmpfs: mounted
[    2.788127] Freeing unused kernel memory: 2404K
[    2.789833] Write protecting the kernel text: 7512k
[    2.791856] Write protecting the kernel read-only data: 3568k
[    2.793918] NX-protecting the kernel data: 8872k
[    2.822964] x86/mm: Checked W+X mappings: FAILED, 96 W+X pages found.
[    2.845398] crng: dumping entropy
[    2.846536] crng_fast_load called
[    2.847460] crng_fast_load: 64/64
[    2.848137] random: fast init done
[    2.848137] crng_fast_load: 16
[    2.914998] systemd[1]: systemd 215 running in system mode. (+PAM +AUDIT +SELINUX +IMA +SYSVINIT +LIBCRYPTSETUP +GCRYPT +ACL +XZ -SECCOMP -APPARMOR)
[    2.920775] systemd[1]: Detected virtualization 'kvm'.
[    2.923019] systemd[1]: Detected architecture 'x86'.

Welcome to ^[[1mDebian GNU/Linux 8 (jessie)^[[0m!

^ permalink raw reply

* [PATCH v2 3/3] crypto: ccp - Simplify some buffer management routines
From: Gary R Hook @ 2017-02-09 21:50 UTC (permalink / raw)
  To: linux-crypto; +Cc: thomas.lendacky, herbert, davem
In-Reply-To: <20170209214704.11781.28640.stgit@taos>

The reverse-get/set functions can be simplified by
eliminating unused code.


Signed-off-by: Gary R Hook <gary.hook@amd.com>
---
 drivers/crypto/ccp/ccp-ops.c |  142 +++++++++++++++++-------------------------
 1 file changed, 56 insertions(+), 86 deletions(-)

diff --git a/drivers/crypto/ccp/ccp-ops.c b/drivers/crypto/ccp/ccp-ops.c
index 50fae44..efac3d5 100644
--- a/drivers/crypto/ccp/ccp-ops.c
+++ b/drivers/crypto/ccp/ccp-ops.c
@@ -184,62 +184,46 @@ static void ccp_get_dm_area(struct ccp_dm_workarea *wa, unsigned int wa_offset,
 }
 
 static int ccp_reverse_set_dm_area(struct ccp_dm_workarea *wa,
+				   unsigned int wa_offset,
 				   struct scatterlist *sg,
-				   unsigned int len, unsigned int se_len,
-				   bool sign_extend)
+				   unsigned int sg_offset,
+				   unsigned int len)
 {
-	unsigned int nbytes, sg_offset, dm_offset, sb_len, i;
-	u8 buffer[CCP_REVERSE_BUF_SIZE];
-
-	if (WARN_ON(se_len > sizeof(buffer)))
-		return -EINVAL;
-
-	sg_offset = len;
-	dm_offset = 0;
-	nbytes = len;
-	while (nbytes) {
-		sb_len = min_t(unsigned int, nbytes, se_len);
-		sg_offset -= sb_len;
-
-		scatterwalk_map_and_copy(buffer, sg, sg_offset, sb_len, 0);
-		for (i = 0; i < sb_len; i++)
-			wa->address[dm_offset + i] = buffer[sb_len - i - 1];
-
-		dm_offset += sb_len;
-		nbytes -= sb_len;
-
-		if ((sb_len != se_len) && sign_extend) {
-			/* Must sign-extend to nearest sign-extend length */
-			if (wa->address[dm_offset - 1] & 0x80)
-				memset(wa->address + dm_offset, 0xff,
-				       se_len - sb_len);
-		}
+	u8 *p, *q;
+
+	ccp_set_dm_area(wa, wa_offset, sg, sg_offset, len);
+
+	p = wa->address + wa_offset;
+	q = p + len - 1;
+	while (p < q) {
+		*p = *p ^ *q;
+		*q = *p ^ *q;
+		*p = *p ^ *q;
+		p++;
+		q--;
 	}
-
 	return 0;
 }
 
 static void ccp_reverse_get_dm_area(struct ccp_dm_workarea *wa,
+				    unsigned int wa_offset,
 				    struct scatterlist *sg,
+				    unsigned int sg_offset,
 				    unsigned int len)
 {
-	unsigned int nbytes, sg_offset, dm_offset, sb_len, i;
-	u8 buffer[CCP_REVERSE_BUF_SIZE];
-
-	sg_offset = 0;
-	dm_offset = len;
-	nbytes = len;
-	while (nbytes) {
-		sb_len = min_t(unsigned int, nbytes, sizeof(buffer));
-		dm_offset -= sb_len;
-
-		for (i = 0; i < sb_len; i++)
-			buffer[sb_len - i - 1] = wa->address[dm_offset + i];
-		scatterwalk_map_and_copy(buffer, sg, sg_offset, sb_len, 1);
-
-		sg_offset += sb_len;
-		nbytes -= sb_len;
+	u8 *p, *q;
+
+	p = wa->address + wa_offset;
+	q = p + len - 1;
+	while (p < q) {
+		*p = *p ^ *q;
+		*q = *p ^ *q;
+		*p = *p ^ *q;
+		p++;
+		q--;
 	}
+
+	ccp_get_dm_area(wa, wa_offset, sg, sg_offset, len);
 }
 
 static void ccp_free_data(struct ccp_data *data, struct ccp_cmd_queue *cmd_q)
@@ -1261,8 +1245,7 @@ static int ccp_run_rsa_cmd(struct ccp_cmd_queue *cmd_q, struct ccp_cmd *cmd)
 	if (ret)
 		goto e_sb;
 
-	ret = ccp_reverse_set_dm_area(&exp, rsa->exp, rsa->exp_len,
-				      CCP_SB_BYTES, false);
+	ret = ccp_reverse_set_dm_area(&exp, 0, rsa->exp, 0, rsa->exp_len);
 	if (ret)
 		goto e_exp;
 	ret = ccp_copy_to_sb(cmd_q, &exp, op.jobid, op.sb_key,
@@ -1280,16 +1263,12 @@ static int ccp_run_rsa_cmd(struct ccp_cmd_queue *cmd_q, struct ccp_cmd *cmd)
 	if (ret)
 		goto e_exp;
 
-	ret = ccp_reverse_set_dm_area(&src, rsa->mod, rsa->mod_len,
-				      CCP_SB_BYTES, false);
+	ret = ccp_reverse_set_dm_area(&src, 0, rsa->mod, 0, rsa->mod_len);
 	if (ret)
 		goto e_src;
-	src.address += o_len;	/* Adjust the address for the copy operation */
-	ret = ccp_reverse_set_dm_area(&src, rsa->src, rsa->src_len,
-				      CCP_SB_BYTES, false);
+	ret = ccp_reverse_set_dm_area(&src, o_len, rsa->src, 0, rsa->src_len);
 	if (ret)
 		goto e_src;
-	src.address -= o_len;	/* Reset the address to original value */
 
 	/* Prepare the output area for the operation */
 	ret = ccp_init_data(&dst, cmd_q, rsa->dst, rsa->mod_len,
@@ -1314,7 +1293,7 @@ static int ccp_run_rsa_cmd(struct ccp_cmd_queue *cmd_q, struct ccp_cmd *cmd)
 		goto e_dst;
 	}
 
-	ccp_reverse_get_dm_area(&dst.dm_wa, rsa->dst, rsa->mod_len);
+	ccp_reverse_get_dm_area(&dst.dm_wa, 0, rsa->dst, 0, rsa->mod_len);
 
 e_dst:
 	ccp_free_data(&dst, cmd_q);
@@ -1566,25 +1545,22 @@ static int ccp_run_ecc_mm_cmd(struct ccp_cmd_queue *cmd_q, struct ccp_cmd *cmd)
 	save = src.address;
 
 	/* Copy the ECC modulus */
-	ret = ccp_reverse_set_dm_area(&src, ecc->mod, ecc->mod_len,
-				      CCP_ECC_OPERAND_SIZE, false);
+	ret = ccp_reverse_set_dm_area(&src, 0, ecc->mod, 0, ecc->mod_len);
 	if (ret)
 		goto e_src;
 	src.address += CCP_ECC_OPERAND_SIZE;
 
 	/* Copy the first operand */
-	ret = ccp_reverse_set_dm_area(&src, ecc->u.mm.operand_1,
-				      ecc->u.mm.operand_1_len,
-				      CCP_ECC_OPERAND_SIZE, false);
+	ret = ccp_reverse_set_dm_area(&src, 0, ecc->u.mm.operand_1, 0,
+				      ecc->u.mm.operand_1_len);
 	if (ret)
 		goto e_src;
 	src.address += CCP_ECC_OPERAND_SIZE;
 
 	if (ecc->function != CCP_ECC_FUNCTION_MINV_384BIT) {
 		/* Copy the second operand */
-		ret = ccp_reverse_set_dm_area(&src, ecc->u.mm.operand_2,
-					      ecc->u.mm.operand_2_len,
-					      CCP_ECC_OPERAND_SIZE, false);
+		ret = ccp_reverse_set_dm_area(&src, 0, ecc->u.mm.operand_2, 0,
+					      ecc->u.mm.operand_2_len);
 		if (ret)
 			goto e_src;
 		src.address += CCP_ECC_OPERAND_SIZE;
@@ -1623,7 +1599,8 @@ static int ccp_run_ecc_mm_cmd(struct ccp_cmd_queue *cmd_q, struct ccp_cmd *cmd)
 	}
 
 	/* Save the ECC result */
-	ccp_reverse_get_dm_area(&dst, ecc->u.mm.result, CCP_ECC_MODULUS_BYTES);
+	ccp_reverse_get_dm_area(&dst, 0, ecc->u.mm.result, 0,
+				CCP_ECC_MODULUS_BYTES);
 
 e_dst:
 	ccp_dm_free(&dst);
@@ -1691,22 +1668,19 @@ static int ccp_run_ecc_pm_cmd(struct ccp_cmd_queue *cmd_q, struct ccp_cmd *cmd)
 	save = src.address;
 
 	/* Copy the ECC modulus */
-	ret = ccp_reverse_set_dm_area(&src, ecc->mod, ecc->mod_len,
-				      CCP_ECC_OPERAND_SIZE, false);
+	ret = ccp_reverse_set_dm_area(&src, 0, ecc->mod, 0, ecc->mod_len);
 	if (ret)
 		goto e_src;
 	src.address += CCP_ECC_OPERAND_SIZE;
 
 	/* Copy the first point X and Y coordinate */
-	ret = ccp_reverse_set_dm_area(&src, ecc->u.pm.point_1.x,
-				      ecc->u.pm.point_1.x_len,
-				      CCP_ECC_OPERAND_SIZE, false);
+	ret = ccp_reverse_set_dm_area(&src, 0, ecc->u.pm.point_1.x, 0,
+				      ecc->u.pm.point_1.x_len);
 	if (ret)
 		goto e_src;
 	src.address += CCP_ECC_OPERAND_SIZE;
-	ret = ccp_reverse_set_dm_area(&src, ecc->u.pm.point_1.y,
-				      ecc->u.pm.point_1.y_len,
-				      CCP_ECC_OPERAND_SIZE, false);
+	ret = ccp_reverse_set_dm_area(&src, 0, ecc->u.pm.point_1.y, 0,
+				      ecc->u.pm.point_1.y_len);
 	if (ret)
 		goto e_src;
 	src.address += CCP_ECC_OPERAND_SIZE;
@@ -1717,15 +1691,13 @@ static int ccp_run_ecc_pm_cmd(struct ccp_cmd_queue *cmd_q, struct ccp_cmd *cmd)
 
 	if (ecc->function == CCP_ECC_FUNCTION_PADD_384BIT) {
 		/* Copy the second point X and Y coordinate */
-		ret = ccp_reverse_set_dm_area(&src, ecc->u.pm.point_2.x,
-					      ecc->u.pm.point_2.x_len,
-					      CCP_ECC_OPERAND_SIZE, false);
+		ret = ccp_reverse_set_dm_area(&src, 0, ecc->u.pm.point_2.x, 0,
+					      ecc->u.pm.point_2.x_len);
 		if (ret)
 			goto e_src;
 		src.address += CCP_ECC_OPERAND_SIZE;
-		ret = ccp_reverse_set_dm_area(&src, ecc->u.pm.point_2.y,
-					      ecc->u.pm.point_2.y_len,
-					      CCP_ECC_OPERAND_SIZE, false);
+		ret = ccp_reverse_set_dm_area(&src, 0, ecc->u.pm.point_2.y, 0,
+					      ecc->u.pm.point_2.y_len);
 		if (ret)
 			goto e_src;
 		src.address += CCP_ECC_OPERAND_SIZE;
@@ -1735,19 +1707,17 @@ static int ccp_run_ecc_pm_cmd(struct ccp_cmd_queue *cmd_q, struct ccp_cmd *cmd)
 		src.address += CCP_ECC_OPERAND_SIZE;
 	} else {
 		/* Copy the Domain "a" parameter */
-		ret = ccp_reverse_set_dm_area(&src, ecc->u.pm.domain_a,
-					      ecc->u.pm.domain_a_len,
-					      CCP_ECC_OPERAND_SIZE, false);
+		ret = ccp_reverse_set_dm_area(&src, 0, ecc->u.pm.domain_a, 0,
+					      ecc->u.pm.domain_a_len);
 		if (ret)
 			goto e_src;
 		src.address += CCP_ECC_OPERAND_SIZE;
 
 		if (ecc->function == CCP_ECC_FUNCTION_PMUL_384BIT) {
 			/* Copy the scalar value */
-			ret = ccp_reverse_set_dm_area(&src, ecc->u.pm.scalar,
-						      ecc->u.pm.scalar_len,
-						      CCP_ECC_OPERAND_SIZE,
-						      false);
+			ret = ccp_reverse_set_dm_area(&src, 0,
+						      ecc->u.pm.scalar, 0,
+						      ecc->u.pm.scalar_len);
 			if (ret)
 				goto e_src;
 			src.address += CCP_ECC_OPERAND_SIZE;
@@ -1792,10 +1762,10 @@ static int ccp_run_ecc_pm_cmd(struct ccp_cmd_queue *cmd_q, struct ccp_cmd *cmd)
 	save = dst.address;
 
 	/* Save the ECC result X and Y coordinates */
-	ccp_reverse_get_dm_area(&dst, ecc->u.pm.result.x,
+	ccp_reverse_get_dm_area(&dst, 0, ecc->u.pm.result.x, 0,
 				CCP_ECC_MODULUS_BYTES);
 	dst.address += CCP_ECC_OUTPUT_SIZE;
-	ccp_reverse_get_dm_area(&dst, ecc->u.pm.result.y,
+	ccp_reverse_get_dm_area(&dst, 0, ecc->u.pm.result.y, 0,
 				CCP_ECC_MODULUS_BYTES);
 	dst.address += CCP_ECC_OUTPUT_SIZE;
 

^ permalink raw reply related

* [PATCH v2 2/3] crypto: ccp - Update the command queue on errors
From: Gary R Hook @ 2017-02-09 21:49 UTC (permalink / raw)
  To: linux-crypto; +Cc: thomas.lendacky, herbert, davem
In-Reply-To: <20170209214704.11781.28640.stgit@taos>

Move the command queue tail pointer when an error is
detected. Always return the error.

Signed-off-by: Gary R Hook <gary.hook@amd.com>
---
 drivers/crypto/ccp/ccp-dev-v5.c |    7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/crypto/ccp/ccp-dev-v5.c b/drivers/crypto/ccp/ccp-dev-v5.c
index 5fb6c8c..d9e1876 100644
--- a/drivers/crypto/ccp/ccp-dev-v5.c
+++ b/drivers/crypto/ccp/ccp-dev-v5.c
@@ -250,17 +250,20 @@ static int ccp5_do_cmd(struct ccp5_desc *desc,
 		ret = wait_event_interruptible(cmd_q->int_queue,
 					       cmd_q->int_rcvd);
 		if (ret || cmd_q->cmd_error) {
+			/* Log the error and flush the queue by
+			 * moving the head pointer
+			 */
 			if (cmd_q->cmd_error)
 				ccp_log_error(cmd_q->ccp,
 					      cmd_q->cmd_error);
-			/* A version 5 device doesn't use Job IDs... */
+			iowrite32(tail, cmd_q->reg_head_lo);
 			if (!ret)
 				ret = -EIO;
 		}
 		cmd_q->int_rcvd = 0;
 	}
 
-	return 0;
+	return ret;
 }
 
 static int ccp5_perform_aes(struct ccp_op *op)

^ permalink raw reply related

* [PATCH v2 1/3] crypto: ccp - Change mode for detailed CCP init messages
From: Gary R Hook @ 2017-02-09 21:49 UTC (permalink / raw)
  To: linux-crypto; +Cc: thomas.lendacky, herbert, davem
In-Reply-To: <20170209214704.11781.28640.stgit@taos>

The CCP initialization messages only need to be sent to
syslog in debug mode.


Signed-off-by: Gary R Hook <gary.hook@amd.com>
---
 drivers/crypto/ccp/ccp-dev-v5.c |    5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/crypto/ccp/ccp-dev-v5.c b/drivers/crypto/ccp/ccp-dev-v5.c
index e2ce819..5fb6c8c 100644
--- a/drivers/crypto/ccp/ccp-dev-v5.c
+++ b/drivers/crypto/ccp/ccp-dev-v5.c
@@ -532,7 +532,7 @@ static int ccp_find_lsb_regions(struct ccp_cmd_queue *cmd_q, u64 status)
 		status >>= LSB_REGION_WIDTH;
 	}
 	queues = bitmap_weight(cmd_q->lsbmask, MAX_LSB_CNT);
-	dev_info(cmd_q->ccp->dev, "Queue %d can access %d LSB regions\n",
+	dev_dbg(cmd_q->ccp->dev, "Queue %d can access %d LSB regions\n",
 		 cmd_q->id, queues);
 
 	return queues ? 0 : -EINVAL;
@@ -574,7 +574,7 @@ static int ccp_find_and_assign_lsb_to_q(struct ccp_device *ccp,
 					 */
 					cmd_q->lsb = bitno;
 					bitmap_clear(lsb_pub, bitno, 1);
-					dev_info(ccp->dev,
+					dev_dbg(ccp->dev,
 						 "Queue %d gets LSB %d\n",
 						 i, bitno);
 					break;
@@ -732,7 +732,6 @@ static int ccp5_init(struct ccp_device *ccp)
 		ret = -EIO;
 		goto e_pool;
 	}
-	dev_notice(dev, "%u command queues available\n", ccp->cmd_q_count);
 
 	/* Turn off the queues and disable interrupts until ready */
 	for (i = 0; i < ccp->cmd_q_count; i++) {

^ permalink raw reply related

* [PATCH v2 0/3] Minor CCP improvements and clean-up
From: Gary R Hook @ 2017-02-09 21:49 UTC (permalink / raw)
  To: linux-crypto; +Cc: thomas.lendacky, herbert, davem

The following series implements...
 - Move verbose init messages to debug mode
 - Update the queue pointers in the event of an error
 - Simply buffer management and eliminate an unused option


---

Gary R Hook (3):
      crypto: ccp - Change mode for detailed CCP init messages
      crypto: ccp - Update the command queue on errors
      crypto: ccp - Simplify some buffer management routines


 drivers/crypto/ccp/ccp-dev-v5.c |   12 ++-
 drivers/crypto/ccp/ccp-ops.c    |  142 +++++++++++++++------------------------
 2 files changed, 63 insertions(+), 91 deletions(-)

^ permalink raw reply

* Re: [PATCH v7 0/5] Update LZ4 compressor module
From: Minchan Kim @ 2017-02-10  0:14 UTC (permalink / raw)
  To: Eric Biggers
  Cc: Sven Schmidt, akpm, bongkyu.kim, rsalvaterra, sergey.senozhatsky,
	gregkh, linux-kernel, herbert, davem, linux-crypto, anton, ccross,
	keescook, tony.luck
In-Reply-To: <20170209052425.GA4678@zzz>

Hello Eric,

On Wed, Feb 08, 2017 at 09:24:25PM -0800, Eric Biggers wrote:
> Also I noticed another bug, this time in LZ4_count():
> 
> > #if defined(CONFIG_64BIT)
> > #define LZ4_ARCH64 1
> > #else
> > #define LZ4_ARCH64 0
> > #endif
> ...
> > #ifdef LZ4_ARCH64
> >        if ((pIn < (pInLimit-3))
> >                && (LZ4_read32(pMatch) == LZ4_read32(pIn))) {
> >                pIn += 4; pMatch += 4;
> >        }
> > #endif
> 
> Because of how LZ4_ARCH64 is defined, it needs to be '#if LZ4_ARCH64'.
> 
> But I also think the way upstream LZ4 does 64-bit detection could have just been
> left as-is; it has a function which gets inlined:
> 
> 	static unsigned LZ4_64bits(void) { return sizeof(void*)==8; }
> 

Thanks for looking this.

If you have a fix, could you send a patch? I'm happy to test it.

^ permalink raw reply

* Re: [PATCH v7 0/5] Update LZ4 compressor module
From: Minchan Kim @ 2017-02-10  0:13 UTC (permalink / raw)
  To: Sven Schmidt
  Cc: akpm, bongkyu.kim, rsalvaterra, sergey.senozhatsky, gregkh,
	linux-kernel, herbert, davem, linux-crypto, anton, ccross,
	keescook, tony.luck
In-Reply-To: <20170209105617.GA3575@bierbaron.springfield.local>

Hello Sven,

On Thu, Feb 09, 2017 at 11:56:17AM +0100, Sven Schmidt wrote:
> Hey Minchan,
> 
> On Thu, Feb 09, 2017 at 08:31:21AM +0900, Minchan Kim wrote:
> > Hello Sven,
> > 
> > On Sun, Feb 05, 2017 at 08:09:03PM +0100, Sven Schmidt wrote:
> > > 
> > > This patchset is for updating the LZ4 compression module to a version based
> > > on LZ4 v1.7.3 allowing to use the fast compression algorithm aka LZ4 fast
> > > which provides an "acceleration" parameter as a tradeoff between
> > > high compression ratio and high compression speed.
> > > 
> > > We want to use LZ4 fast in order to support compression in lustre
> > > and (mostly, based on that) investigate data reduction techniques in behalf of
> > > storage systems.
> > > 
> > > Also, it will be useful for other users of LZ4 compression, as with LZ4 fast
> > > it is possible to enable applications to use fast and/or high compression
> > > depending on the usecase.
> > > For instance, ZRAM is offering a LZ4 backend and could benefit from an updated
> > > LZ4 in the kernel.
> > > 
> > > LZ4 homepage: http://www.lz4.org/
> > > LZ4 source repository: https://github.com/lz4/lz4
> > > Source version: 1.7.3
> > > 
> > > Benchmark (taken from [1], Core i5-4300U @1.9GHz):
> > > ----------------|--------------|----------------|----------
> > > Compressor      | Compression  | Decompression  | Ratio
> > > ----------------|--------------|----------------|----------
> > > memcpy          |  4200 MB/s   |  4200 MB/s     | 1.000
> > > LZ4 fast 50     |  1080 MB/s   |  2650 MB/s     | 1.375
> > > LZ4 fast 17     |   680 MB/s   |  2220 MB/s     | 1.607
> > > LZ4 fast 5      |   475 MB/s   |  1920 MB/s     | 1.886
> > > LZ4 default     |   385 MB/s   |  1850 MB/s     | 2.101
> > > 
> > > [1] http://fastcompression.blogspot.de/2015/04/sampling-or-faster-lz4.html
> > > 
> > > [PATCH 1/5] lib: Update LZ4 compressor module
> > > [PATCH 2/5] lib/decompress_unlz4: Change module to work with new LZ4 module version
> > > [PATCH 3/5] crypto: Change LZ4 modules to work with new LZ4 module version
> > > [PATCH 4/5] fs/pstore: fs/squashfs: Change usage of LZ4 to work with new LZ4 version
> > > [PATCH 5/5] lib/lz4: Remove back-compat wrappers
> > 
> > Today, I did zram-lz4 performance test with fio in current mmotm and
> > found it makes regression about 20%.
> > 
> > "lz4-update" means current mmots(git://git.cmpxchg.org/linux-mmots.git) so
> > applied your 5 patches. (But now sure current mmots has recent uptodate
> > patches)
> > "revert" means I reverted your 5 patches in current mmots.
> > 
> >                      revert    lz4-update
> > 
> >       seq-write       1547       1339      86.55%
> >      rand-write      22775      19381      85.10%
> >        seq-read       7035       5589      79.45%
> >       rand-read      78556      68479      87.17%
> >    mixed-seq(R)       1305       1066      81.69%
> >    mixed-seq(W)       1205        984      81.66%
> >   mixed-rand(R)      17421      14993      86.06%
> >   mixed-rand(W)      17391      14968      86.07%
> 
> which parts of the output (as well as units) are these values exactly?
> I did not work with fio until now, so I think I might ask before misinterpreting my results.

It is IOPS.

>  
> > My fio description file
> > 
> > [global]
> > bs=4k
> > ioengine=sync
> > size=100m
> > numjobs=1
> > group_reporting
> > buffer_compress_percentage=30
> > scramble_buffers=0
> > filename=/dev/zram0
> > loops=10
> > fsync_on_close=1
> > 
> > [seq-write]
> > bs=64k
> > rw=write
> > stonewall
> > 
> > [rand-write]
> > rw=randwrite
> > stonewall
> > 
> > [seq-read]
> > bs=64k
> > rw=read
> > stonewall
> > 
> > [rand-read]
> > rw=randread
> > stonewall
> > 
> > [mixed-seq]
> > bs=64k
> > rw=rw
> > stonewall
> > 
> > [mixed-rand]
> > rw=randrw
> > stonewall
> > 
> 
> Great, this makes it easy for me to reproduce your test.

If you have trouble to reproduce, feel free to ask me. I'm happy to test it. :)

Thanks!

^ permalink raw reply

* RE: [PATCH -next] crypto: asymmetric_keys - Fix error return code on failure
From: weiyongjun (A) @ 2017-02-10  2:11 UTC (permalink / raw)
  To: David Howells, Wei Yongjun
  Cc: Herbert Xu, keyrings@vger.kernel.org,
	linux-crypto@vger.kernel.org
In-Reply-To: <27512.1486659476@warthog.procyon.org.uk>

Hi David,

> > --- a/crypto/asymmetric_keys/public_key.c
> > +++ b/crypto/asymmetric_keys/public_key.c
> > @@ -184,8 +184,10 @@ static int software_key_eds_op(struct
> kernel_pkey_params *params,
> >  		return PTR_ERR(tfm);
> >
> >  	req = akcipher_request_alloc(tfm, GFP_KERNEL);
> > -	if (!req)
> > +	if (!req) {
> > +		ret = -ENOMEM;
> 
> Ummm...  What should I apply your patch to?

This one introduced by patch " KEYS: Implement encrypt, decrypt and sign for software asymmetric key".

> 
> >  		goto error_free_tfm;
> > +	}
> >
> >  	if (pkey->key_is_private)
> >  		ret = crypto_akcipher_set_priv_key(tfm,
> > @@ -268,8 +270,10 @@ int public_key_verify_signature(const struct
> public_key *pkey,
> >  		return PTR_ERR(tfm);
> >
> >  	req = akcipher_request_alloc(tfm, GFP_KERNEL);
> > -	if (!req)
> > +	if (!req) {
> > +		ret = -ENOMEM;
> >  		goto error_free_tfm;
> 
> This shouldn't be necessary.  ret should already be -ENOMEM from
> initialisation of the variable at the top of the function.

Introduced by patch " KEYS: Provide software public key query function", ret have
been overwritten to 0 after ret = software_key_determine_akcipher(...).

Regards,
Wei Yongjun

^ permalink raw reply

* Re: linux-next: build warnings after merge of the crypto tree
From: Stephen Rothwell @ 2017-02-10  3:12 UTC (permalink / raw)
  To: Herbert Xu
  Cc: linux-next, linux-kernel, Cyrille Pitchen,
	Linux Crypto Mailing List
In-Reply-To: <20170206090340.GA9531@gondor.apana.org.au>

Hi Herbert,

On Mon, 6 Feb 2017 17:03:40 +0800 Herbert Xu <herbert@gondor.apana.org.au> wrote:
>
> On Mon, Feb 06, 2017 at 12:28:37PM +1100, Stephen Rothwell wrote:
> > 
> > After merging the crypto tree, today's linux-next build (x86_64
> > allmodconfig) produced these warnings:
> > 
> > warning: (CRYPTO_DEV_ATMEL_AUTHENC) selects CRYPTO_DEV_ATMEL_SHA which has unmet direct dependencies (CRYPTO && CRYPTO_HW && ARCH_AT91)
> > warning: (CRYPTO_DEV_ATMEL_AUTHENC) selects CRYPTO_DEV_ATMEL_SHA which has unmet direct dependencies (CRYPTO && CRYPTO_HW && ARCH_AT91)
> > 
> > Introduced by commit
> > 
> >   89a82ef87e01 ("crypto: atmel-authenc - add support to authenc(hmac(shaX), Y(aes)) modes")
> > 
> > In file included from include/linux/printk.h:329:0,
> >                  from include/linux/kernel.h:13,
> >                  from drivers/crypto/atmel-sha.c:17:  
> 
> This patch should fix both issues.  Thanks,

I am still getting these warnings ... I have seen no updates to the
crypot tree since Feb 2.

-- 
Cheers,
Stephen Rothwell

^ permalink raw reply

* Re: [PATCH v2 2/5] async_tx: Handle DMA devices having support for fewer PQ coefficients
From: Anup Patel @ 2017-02-10  3:24 UTC (permalink / raw)
  To: Dan Williams
  Cc: Mark Rutland, Device Tree, Herbert Xu, Scott Branden, Vinod Koul,
	Ray Jui, Jassi Brar, linux-kernel@vger.kernel.org, linux-raid,
	Jon Mason, Rob Herring, BCM Kernel Feedback, linux-crypto,
	Rob Rice, dmaengine@vger.kernel.org, David S . Miller,
	linux-arm-kernel@lists.infradead.org
In-Reply-To: <CAPcyv4gDq+our1+sAPShBru-qvebo+Zvk0crgSUNXucHGQwZ1Q@mail.gmail.com>

On Thu, Feb 9, 2017 at 10:14 PM, Dan Williams <dan.j.williams@intel.com> wrote:
> On Thu, Feb 9, 2017 at 1:29 AM, Anup Patel <anup.patel@broadcom.com> wrote:
>> On Wed, Feb 8, 2017 at 9:54 PM, Dan Williams <dan.j.williams@intel.com> wrote:
>>> On Wed, Feb 8, 2017 at 12:57 AM, Anup Patel <anup.patel@broadcom.com> wrote:
>>>> On Tue, Feb 7, 2017 at 11:46 PM, Dan Williams <dan.j.williams@intel.com> wrote:
>>>>> On Tue, Feb 7, 2017 at 1:02 AM, Anup Patel <anup.patel@broadcom.com> wrote:
>>>>>> On Tue, Feb 7, 2017 at 1:57 PM, Dan Williams <dan.j.williams@intel.com> wrote:
>>>>>>> On Tue, Feb 7, 2017 at 12:16 AM, Anup Patel <anup.patel@broadcom.com> wrote:
>>>>>>>> The DMAENGINE framework assumes that if PQ offload is supported by a
>>>>>>>> DMA device then all 256 PQ coefficients are supported. This assumption
>>>>>>>> does not hold anymore because we now have BCM-SBA-RAID offload engine
>>>>>>>> which supports PQ offload with limited number of PQ coefficients.
>>>>>>>>
>>>>>>>> This patch extends async_tx APIs to handle DMA devices with support
>>>>>>>> for fewer PQ coefficients.
>>>>>>>>
>>>>>>>> Signed-off-by: Anup Patel <anup.patel@broadcom.com>
>>>>>>>> Reviewed-by: Scott Branden <scott.branden@broadcom.com>
>>>>>>>
>>>>>>> I don't like this approach. Define an interface for md to query the
>>>>>>> offload engine once at the beginning of time. We should not be adding
>>>>>>> any new extensions to async_tx.
>>>>>>
>>>>>> Even if we do capability checks in Linux MD, we still need a way
>>>>>> for DMAENGINE drivers to advertise number of PQ coefficients
>>>>>> handled by the HW.
>>>>>>
>>>>>> I agree capability checks should be done once in Linux MD but I don't
>>>>>> see why this has to be part of BCM-SBA-RAID driver patches. We need
>>>>>> separate patchsets to address limitations of async_tx framework.
>>>>>
>>>>> Right, separate enabling before we pile on new hardware support to a
>>>>> known broken framework.
>>>>
>>>> Linux Async Tx not broken framework. The issue is:
>>>> 1. Its not complete enough
>>>> 2. Its not optimized for very high through-put offload engines
>>>
>>> I'm not understanding your point. I'm nak'ing this change to add yet
>>> more per-transaction capability checking to async_tx. I don't like the
>>> DMA_HAS_FEWER_PQ_COEF flag, especially since it is equal to
>>> DMA_HAS_PQ_CONTINUE. I'm not asking for all of async_tx's problems to
>>> be fixed before this new hardware support, I'm simply saying we should
>>> start the process of moving offload-engine capability checking to the
>>> raid code.
>>
>> The DMA_HAS_FEWER_PQ_COEF is not equal to
>> DMA_HAS_PQ_CONTINUE.
>
> #define DMA_HAS_PQ_CONTINUE (1 << 15
> #define DMA_HAS_FEWER_PQ_COEF (1 << 15)

You are only looking at the values of these flags.

The semantics of both these flags are different and both
flags are set in different members of "struct dma_device"
The DMA_HAS_PQ_CONTINUE is set in "max_pq" whereas
DMA_HAS_FEWER_PQ_COEF is set in "max_pqcoef".

When DMA_HAS_PQ_CONTINUE is set in "max_pq", it
means that PQ HW is capable of taking P & Q computed
previous txn as input. If DMA_HAS_PQ_CONTINUE is
not supported the async_pq() will pass P & Q computed
by previous txn as sources with coef as g^0.

When DMA_HAS_FEWER_PQ_COEF is set in "max_pqcoef",
it means the PQ HW is not capable of handling all 256 coefs.

>
>> I will try to drop this patch and take care of unsupported PQ
>> coefficients in BCM-SBA-RAID driver itself even if this means
>> doing some computations in BCM-SBA-RAID driver itself.
>
> That should be nak'd as well, please do capability detection in a
> routine that is common to all raid engines.

Thanks for NAKing this patch.

This motivated me to find clean work-around for handling
unsupported PQ coefs in BCM-SBA-RAID driver.

Let's assume max number of PQ coefs supported by PQ HW
is m coefs. Now for any coef n > m, we can use RAID6 math
to get g^n = (g^m)*(g^m)*....*(g^k) where k <= m.

Using the above fact, we can create chained txn for each
source of PQ request in BCM-SBA-RAID driver where each
txn will only compute PQ from one source using above
described RAID6 math. Also, each txn in chained txn will
depend on output of previous txn because we are computing
PQ from one source at a time.

I will drop this patch and send updated BCM-SBA-RAID
driver with above described work-around for handling
unsupported PQ coefs.

Regards,
Anup

^ permalink raw reply

* Re: [PATCH v7 0/5] Update LZ4 compressor module
From: David Miller @ 2017-02-10  3:57 UTC (permalink / raw)
  To: ebiggers3
  Cc: 4sschmid, minchan, akpm, bongkyu.kim, rsalvaterra,
	sergey.senozhatsky, gregkh, linux-kernel, herbert, linux-crypto,
	anton, ccross, keescook, tony.luck
In-Reply-To: <20170209182914.GB92711@gmail.com>

From: Eric Biggers <ebiggers3@gmail.com>
Date: Thu, 9 Feb 2017 10:29:14 -0800

> On Thu, Feb 09, 2017 at 12:02:11PM +0100, Sven Schmidt wrote:
>> > 
>> > [Also, for some reason linux-crypto is apparently still not receiving patch 1/5
>> > in the series.  It's missing from the linux-crypto archive at
>> > http://www.spinics.net/lists/linux-crypto/, so it's not just me.]
>> > 
>> 
>> I don't really know what to do about this. I think the matter is the size of the E-Mail.
>> Are there filters or something like that? Since in linux-kernel the patch seems to get delivered.
>> I could otherwise CC you if you wish.
>> 
> 
> If I'm not mistaken, David Miller is the admin of the mail server on
> vger.kernel.org, and he already happens to be Cc'ed on this thread, so maybe he
> can answer as to why linux-crypto may be configured differently?
> 
> Anyway, since the patch did make it to linux-kernel anyone can still download it
> from patchwork if they know where to look:
> https://patchwork.kernel.org/patch/9556271/

I've increased the maxlength parameter for linux-cryto so this doesn't
happen again.

^ permalink raw reply


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox