On Thu, Aug 31, 2017 at 3:44 AM, Haren Myneni [off-list ref] wrote:

Thanks MIchael and Dan for your review comments.


On 08/29/2017 06:32 AM, Dan Streetman wrote:

quoted

On Mon, Aug 28, 2017 at 7:25 PM, Michael Ellerman [off-list ref] wrote:

quoted

Hi Haren,

Some comments inline ...

Haren Myneni [off-list ref] writes:

quoted

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?

Sometimes HW returns copy/paste failures.

why?  what is causing the failure?

So we should retry the request again. With 10 retries, Test running
12 hours was successful for repeated compression/decompression
requests with 1024 threads.

quoted

quoted

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

Not checked the overhead with falling back to SW compression.

SW is very, very, very slow, due to 842 being an unaligned compression format.

quoted

quoted

quoted

+/* # 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.

workmem is a scratch buffer and shouldn't be used for something
persistent like this.

quoted

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.

We always access workmem with PTR_ALIGN even when assigning txwin (nx842_powernv_crypto_init/exit_vas).
So we should not overwrite start and txwin,

We can add txwin in nx842_crypto_ctx instead of workmem. But nx842_crypto_ctx is used for both powernv and pseries. Hence used workmem. But if nx842_crypto_ctx is preferred, I will send new patch soon.

quoted

quoted

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

no, it's a bug in this patch, because workmem is scratch whose
contents are only valid for the duration of each operation (compress
or decompress).

quoted

quoted

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

Thanks
Haren