On Thu, Aug 31, 2017 at 3:44 AM, Haren Myneni <haren@xxxxxxxxxxxxxxxxxx> wrote:
> Thanks MIchael and Dan for your review comments.
>
>
> On 08/29/2017 06:32 AM, Dan Streetman wrote:
>> On Mon, Aug 28, 2017 at 7:25 PM, Michael Ellerman <mpe@xxxxxxxxxxxxxx> wrote:
>>> Hi Haren,
>>>
>>> Some comments inline ...
>>>
>>> Haren Myneni <haren@xxxxxxxxxxxxxxxxxx> 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?
>
> 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.
>
>>>
>>> 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.
>
>>>
>>>> +/* # 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.
>>
>>>
>>> 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.
>
>>>
>>> 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).
>>
>>>
>>>> char padding[WORKMEM_ALIGN]; /* unused, to allow alignment */
>>>> } __packed __aligned(WORKMEM_ALIGN);
>>>
>
> Thanks
> Haren
>
