Re: [PATCH] drivers/crypto/nx: Add CRC and validation support for nx842

[Dan Streetman]

On Mon, Sep 21, 2015 at 10:26 AM, Herbert Xu
<herbert@xxxxxxxxxxxxxxxxxxx> wrote:
> On Sat, Sep 19, 2015 at 05:02:42PM -0700, Haren Myneni wrote:
>> Hi,
>>
>> This patch allows nx842 coprocessor to add CRC for compression and
>> check the computed CRC value for uncompression. Please let me know
>> if you have any comments.
>>
>> Thanks
>> Haren
>>
>> commit d0b34d2e3ed41e7ec2afdbd654f0dd7716e4d4c0
>> Author: Haren Myneni <haren@xxxxxxxxxx>
>> Date:   Sat Sep 12 01:20:51 2015 -0700
>>
>>     crypto/nx842: Add CRC and validation support
>>
>>     This patch adds CRC generation and validation support for nx-842.
>>     Add CRC flag so that nx842 coprocessor includes CRC during
>>     compression and validates during uncompression.
>>
>>     Signed-off-by: Haren Myneni <haren@xxxxxxxxxx>
>
> In future please post the patch without all the metadata.  You
> can use the helper git format-patch to help you prepare the patch
> for submission.
>
> As to the CRC itself what is the purpose of this and wouldn't it
> fail our test vectors if we had any? Remember that we also have a
> software implementation now and the hardware should produce exactly
> the same output as the software version.

As far as the hw and sw drivers producing the exact same output, I
don't think that's possible with the current hw and sw drivers,
because the hw driver may have to add a header to the actual byte
stream that the hw creates, depending on buffer alignment and size
(the hw has specific restrictions).  Currently, the sw driver doesn't
understand that header that the 842 hw driver creates, although that
could be added to the sw driver.  And, the hw driver skips adding the
header if the buffers are correctly aligned/sized, which would result
in a test vector failure if it doesn't align the buffer the same way
each time.

However, you're right that if the hw driver is doing crc checking, the
sw driver needs to be updated to make sure to add the crc to anything
it encodes, otherwise the hw driver will fail decompressing it; Haren,
you'll need to update the lib/842 code to also include crc
creation/checking.

Also, it might be a good time to add what we talked about a while ago,
to push the alignment/size restrictions into the crypto compression
layer, by adding cra_alignmask and cra_blocksize support to
crypto/compress.c.  Since the 842 hw has requirements not only for
specific alignment and min/max sizes, but also a requirement for
specific length multiple (i.e. must be !(len % 8)) it might be
worthwhile to also add a cra_sizemodulo or something like that.
However, if the common crypto alignment/size handling code allows any
alignment/size buffers (instead of just returning error for mis-sized
buffers), I think a common crypto header would need to be added in
cases of mis-sizing, which may not be appropriate for common code.
Alternately, the common crypto code could just return error for
mis-sized buffers; users of the crypto comp api would just have to
check crypto_tfm_alg_blocksize() before calling.

In case I haven't said it before, I really hate how the 842 hw
requires specific alignment and sizing.  How hard is it to add support
for any alignment/size in the hw?!?


>
> Thanks,
> --
> Email: Herbert Xu <herbert@xxxxxxxxxxxxxxxxxxx>
> Home Page: http://gondor.apana.org.au/~herbert/
> PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
> --
> To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/