[HIFN 01/n]: Endianess fixes

[HIFN 01/n]: Endianess fixes

[Patrick McHardy]

[-- Attachment #1: Type: text/plain, Size: 236 bytes --]

Attached is the first of my fixes for the HIFN driver. I've
got it to a working state with tcrypt, IPsec and dm-crypt, a
few of the patches still need a bit work though, so I'll send
the ones that I already consider ready one by one.



[-- Attachment #2: 01.diff --]
[-- Type: text/x-diff, Size: 4427 bytes --]

commit aa543b1fcddd097b5129a9a3056f888737d88102
Author: Patrick McHardy <kaber@trash.net>
Date:   Wed May 7 12:27:09 2008 +0200

    [HIFN]: Endianess fixes
    
    HIFN uses little-endian by default, move cpu_to_le32 conversion to hifn_write_0/
    hifn_write_1, add sparse annotations and fix an invalid endian conversion in
    hifn_setup_src_desc.
    
    Signed-off-by: Patrick McHardy <kaber@trash.net>

diff --git a/drivers/crypto/hifn_795x.c b/drivers/crypto/hifn_795x.c
index 81f3f95..d7a51ee 100644
--- a/drivers/crypto/hifn_795x.c
+++ b/drivers/crypto/hifn_795x.c
@@ -535,10 +535,10 @@ struct hifn_crypt_command
  */
 struct hifn_mac_command
 {
-	volatile u16 		masks;
-	volatile u16 		header_skip;
-	volatile u16 		source_count;
-	volatile u16 		reserved;
+	volatile __le16 	masks;
+	volatile __le16 	header_skip;
+	volatile __le16 	source_count;
+	volatile __le16 	reserved;
 };
 
 #define	HIFN_MAC_CMD_ALG_MASK		0x0001
@@ -564,10 +564,10 @@ struct hifn_mac_command
 
 struct hifn_comp_command
 {
-	volatile u16 		masks;
-	volatile u16 		header_skip;
-	volatile u16 		source_count;
-	volatile u16 		reserved;
+	volatile __le16 	masks;
+	volatile __le16 	header_skip;
+	volatile __le16 	source_count;
+	volatile __le16 	reserved;
 };
 
 #define	HIFN_COMP_CMD_SRCLEN_M		0xc000
@@ -583,10 +583,10 @@ struct hifn_comp_command
 
 struct hifn_base_result
 {
-	volatile u16 		flags;
-	volatile u16 		session;
-	volatile u16 		src_cnt;		/* 15:0 of source count */
-	volatile u16 		dst_cnt;		/* 15:0 of dest count */
+	volatile __le16 	flags;
+	volatile __le16 	session;
+	volatile __le16 	src_cnt;		/* 15:0 of source count */
+	volatile __le16 	dst_cnt;		/* 15:0 of dest count */
 };
 
 #define	HIFN_BASE_RES_DSTOVERRUN	0x0200	/* destination overrun */
@@ -597,8 +597,8 @@ struct hifn_base_result
 
 struct hifn_comp_result
 {
-	volatile u16 		flags;
-	volatile u16 		crc;
+	volatile __le16		flags;
+	volatile __le16		crc;
 };
 
 #define	HIFN_COMP_RES_LCB_M		0xff00	/* longitudinal check byte */
@@ -609,8 +609,8 @@ struct hifn_comp_result
 
 struct hifn_mac_result
 {
-	volatile u16 		flags;
-	volatile u16 		reserved;
+	volatile __le16 	flags;
+	volatile __le16 	reserved;
 	/* followed by 0, 6, 8, or 10 u16's of the MAC, then crypt */
 };
 
@@ -619,8 +619,8 @@ struct hifn_mac_result
 
 struct hifn_crypt_result
 {
-	volatile u16 		flags;
-	volatile u16 		reserved;
+	volatile __le16		flags;
+	volatile __le16		reserved;
 };
 
 #define	HIFN_CRYPT_RES_SRC_NOTZERO	0x0001	/* source expired */
@@ -686,12 +686,12 @@ static inline u32 hifn_read_1(struct hifn_device *dev, u32 reg)
 
 static inline void hifn_write_0(struct hifn_device *dev, u32 reg, u32 val)
 {
-	writel(val, dev->bar[0] + reg);
+	writel((__force u32)cpu_to_le32(val), dev->bar[0] + reg);
 }
 
 static inline void hifn_write_1(struct hifn_device *dev, u32 reg, u32 val)
 {
-	writel(val, dev->bar[1] + reg);
+	writel((__force u32)cpu_to_le32(val), dev->bar[1] + reg);
 }
 
 static void hifn_wait_puc(struct hifn_device *dev)
@@ -1037,14 +1037,14 @@ static void hifn_init_registers(struct hifn_device *dev)
 	hifn_write_0(dev, HIFN_0_PUIER, HIFN_PUIER_DSTOVER);
 
 	/* write all 4 ring address registers */
-	hifn_write_1(dev, HIFN_1_DMA_CRAR, __cpu_to_le32(dptr +
-				offsetof(struct hifn_dma, cmdr[0])));
-	hifn_write_1(dev, HIFN_1_DMA_SRAR, __cpu_to_le32(dptr +
-				offsetof(struct hifn_dma, srcr[0])));
-	hifn_write_1(dev, HIFN_1_DMA_DRAR, __cpu_to_le32(dptr +
-				offsetof(struct hifn_dma, dstr[0])));
-	hifn_write_1(dev, HIFN_1_DMA_RRAR, __cpu_to_le32(dptr +
-				offsetof(struct hifn_dma, resr[0])));
+	hifn_write_1(dev, HIFN_1_DMA_CRAR, dptr +
+				offsetof(struct hifn_dma, cmdr[0]));
+	hifn_write_1(dev, HIFN_1_DMA_SRAR, dptr +
+				offsetof(struct hifn_dma, srcr[0]));
+	hifn_write_1(dev, HIFN_1_DMA_DRAR, dptr +
+				offsetof(struct hifn_dma, dstr[0]));
+	hifn_write_1(dev, HIFN_1_DMA_RRAR, dptr +
+				offsetof(struct hifn_dma, resr[0]));
 
 	mdelay(2);
 #if 0
@@ -1178,8 +1178,8 @@ static int hifn_setup_src_desc(struct hifn_device *dev, struct page *page,
 	idx = dma->srci;
 
 	dma->srcr[idx].p = __cpu_to_le32(addr);
-	dma->srcr[idx].l = __cpu_to_le32(size) | HIFN_D_VALID |
-			HIFN_D_MASKDONEIRQ | HIFN_D_NOINVALID | HIFN_D_LAST;
+	dma->srcr[idx].l = __cpu_to_le32(size | HIFN_D_VALID |
+			HIFN_D_MASKDONEIRQ | HIFN_D_NOINVALID | HIFN_D_LAST);
 
 	if (++idx == HIFN_D_SRC_RSIZE) {
 		dma->srcr[idx].l = __cpu_to_le32(HIFN_D_VALID |

[HIFN 02/n]: Remove printk_ratelimit() for debugging printk

[Patrick McHardy]

[-- Attachment #1: Type: text/plain, Size: 1 bytes --]



[-- Attachment #2: 02.diff --]
[-- Type: text/x-diff, Size: 926 bytes --]

commit 9dbbaf326c4033cd8d973cb6bf10298faf50180c
Author: Patrick McHardy <kaber@trash.net>
Date:   Wed May 7 12:38:01 2008 +0200

    [HIFN]: Remove printk_ratelimit() for debugging printk
    
    Without debugging this spams the log with "printk: N messages surpressed"
    without any actual messages on error. With debugging its more useful to
    always see the message.
    
    Signed-off-by: Patrick McHardy <kaber@trash.net>

diff --git a/drivers/crypto/hifn_795x.c b/drivers/crypto/hifn_795x.c
index d7a51ee..e5c3bc4 100644
--- a/drivers/crypto/hifn_795x.c
+++ b/drivers/crypto/hifn_795x.c
@@ -1651,7 +1651,7 @@ static int hifn_setup_session(struct ablkcipher_request *req)
 err_out:
 	spin_unlock_irqrestore(&dev->lock, flags);
 err_out_exit:
-	if (err && printk_ratelimit())
+	if (err)
 		dprintk("%s: iv: %p [%d], key: %p [%d], mode: %u, op: %u, "
 				"type: %u, err: %d.\n",
 			dev->name, ctx->iv, ctx->ivsize,

Re: [HIFN 02/n]: Remove printk_ratelimit() for debugging printk

[Evgeniy Polyakov]

On Wed, May 07, 2008 at 02:15:38PM +0200, Patrick McHardy (kaber@trash.net) wrote:
>     [HIFN]: Remove printk_ratelimit() for debugging printk
>     
>     Without debugging this spams the log with "printk: N messages surpressed"
>     without any actual messages on error. With debugging its more useful to
>     always see the message.
>     
>     Signed-off-by: Patrick McHardy <kaber@trash.net>

Acked :)

-- 
	Evgeniy Polyakov

[HIFN 03/n]: Indicate asynchronous processing to crypto API

[Patrick McHardy]

[-- Attachment #1: Type: text/plain, Size: 1 bytes --]



[-- Attachment #2: 03.diff --]
[-- Type: text/x-diff, Size: 912 bytes --]

commit 835b2aece80670cd439ff74e51a647836f73d0ca
Author: Patrick McHardy <kaber@trash.net>
Date:   Wed May 7 12:42:06 2008 +0200

    [HIFN]: Indicate asynchronous processing to crypto API
    
    hifn_setup_crypto() needs to return -EINPROGRESS on success to indicate
    asynchronous processing to the crypto API. This also means it must not
    return the errno code returned by hifn_process_queue(), if any.
    
    Signed-off-by: Patrick McHardy <kaber@trash.net>

diff --git a/drivers/crypto/hifn_795x.c b/drivers/crypto/hifn_795x.c
index e5c3bc4..cce6e6f 100644
--- a/drivers/crypto/hifn_795x.c
+++ b/drivers/crypto/hifn_795x.c
@@ -2202,9 +2202,9 @@ static int hifn_setup_crypto(struct ablkcipher_request *req, u8 op,
 		return err;
 
 	if (dev->started < HIFN_QUEUE_LENGTH &&	dev->queue.qlen)
-		err = hifn_process_queue(dev);
+		hifn_process_queue(dev);
 
-	return err;
+	return -EINPROGRESS;
 }
 
 /*

Re: [HIFN 03/n]: Indicate asynchronous processing to crypto API

[Evgeniy Polyakov]

Hi.

On Wed, May 07, 2008 at 02:19:36PM +0200, Patrick McHardy (kaber@trash.net) wrote:
>     hifn_setup_crypto() needs to return -EINPROGRESS on success to indicate
>     asynchronous processing to the crypto API. This also means it must not
>     return the errno code returned by hifn_process_queue(), if any.

Then how to indicate that error occured?
What if we will return -EINPROGRESS in case of success and negative
errno otherwise?

> @@ -2202,9 +2202,9 @@ static int hifn_setup_crypto(struct ablkcipher_request *req, u8 op,
>  		return err;
>  
>  	if (dev->started < HIFN_QUEUE_LENGTH &&	dev->queue.qlen)
> -		err = hifn_process_queue(dev);
> +		hifn_process_queue(dev);
>  
> -	return err;
> +	return -EINPROGRESS;

Thus we are not able to return any error at all...

-- 
	Evgeniy Polyakov

Re: [HIFN 03/n]: Indicate asynchronous processing to crypto API

[Patrick McHardy]

Evgeniy Polyakov wrote:
> Hi.
>
> On Wed, May 07, 2008 at 02:19:36PM +0200, Patrick McHardy (kaber@trash.net) wrote:
>   
>>     hifn_setup_crypto() needs to return -EINPROGRESS on success to indicate
>>     asynchronous processing to the crypto API. This also means it must not
>>     return the errno code returned by hifn_process_queue(), if any.
>>     
>
> Then how to indicate that error occured?
> What if we will return -EINPROGRESS in case of success and negative
> errno otherwise?
>   

The completion handler should be called with the error code,
a later patch of mine will change that. Returning it while
enqueing a new request is wrong since the error affects a
different request. Queue processing should happen on completion
of requests anyway to make sure the queue is kept running
while requests are queued (and avoid reordering).

>   
>> @@ -2202,9 +2202,9 @@ static int hifn_setup_crypto(struct ablkcipher_request *req, u8 op,
>>  		return err;
>>  
>>  	if (dev->started < HIFN_QUEUE_LENGTH &&	dev->queue.qlen)
>> -		err = hifn_process_queue(dev);
>> +		hifn_process_queue(dev);
>>  
>> -	return err;
>> +	return -EINPROGRESS;
>>     
>
> Thus we are not able to return any error at all...
>   

We are, errors affecting *this request* are returned as they should be.

[HIFN 04/n]: Handle ablkcipher_walk errors

[Patrick McHardy]

[-- Attachment #1: Type: text/plain, Size: 1 bytes --]



[-- Attachment #2: 04.diff --]
[-- Type: text/x-diff, Size: 690 bytes --]

commit 7e24c849df30a5d2b0bb89165b933ea2faa747bd
Author: Patrick McHardy <kaber@trash.net>
Date:   Wed May 7 12:43:20 2008 +0200

    [HIFN]: Handle ablkcipher_walk errors
    
    ablkcipher_walk may return a negative error value, handle this properly
    instead of treating it as a huge number of scatter-gather elements.
    
    Signed-off-by: Patrick McHardy <kaber@trash.net>

diff --git a/drivers/crypto/hifn_795x.c b/drivers/crypto/hifn_795x.c
index cce6e6f..4e89cd8 100644
--- a/drivers/crypto/hifn_795x.c
+++ b/drivers/crypto/hifn_795x.c
@@ -1602,7 +1602,10 @@ static int hifn_setup_session(struct ablkcipher_request *req)
 	idx = 0;
 
 	sg_num = ablkcipher_walk(req, &ctx->walk);

Re: [HIFN 04/n]: Handle ablkcipher_walk errors

[Evgeniy Polyakov]

On Wed, May 07, 2008 at 02:20:34PM +0200, Patrick McHardy (kaber@trash.net) wrote:
> commit 7e24c849df30a5d2b0bb89165b933ea2faa747bd
> Author: Patrick McHardy <kaber@trash.net>
> Date:   Wed May 7 12:43:20 2008 +0200
> 
>     [HIFN]: Handle ablkcipher_walk errors
>     
>     ablkcipher_walk may return a negative error value, handle this properly
>     instead of treating it as a huge number of scatter-gather elements.
>     
>     Signed-off-by: Patrick McHardy <kaber@trash.net>

Ooops, ACK :)


-- 
	Evgeniy Polyakov

[HIFN 05/n]: Fix data alignment checks

[Patrick McHardy]

[-- Attachment #1: Type: text/plain, Size: 230 bytes --]

I'm not entirely sure about the alignmask change at the end of
this patch, is an alignmask of 1 correct if no source buffer
alignment is required, but the destination buffer should be
(doesn't have to be though) 4 byte aligned?



[-- Attachment #2: 05.diff --]
[-- Type: text/x-diff, Size: 4573 bytes --]

commit f76618d53e82c8905214e889a3f79f1816c680fb
Author: Patrick McHardy <kaber@trash.net>
Date:   Wed May 7 12:44:15 2008 +0200

    [HIFN]: Fix data alignment checks
    
    The check for misalignment of the scatterlist data has two bugs:
    
    - the source buffer doesn't need to be aligned at all
    - the destination buffer and its size needs to be aligned to a multiple
      of 4, not to the crypto alg blocksize
    
    Introduce symbolic constant for destination buffer alignment requirements,
    use it instead of the crypto alg blocksize and remove the unnecessary
    checks for source buffer alignment and change cra_alignmask to 1.
    
    Signed-off-by: Patrick McHardy <kaber@trash.net>

diff --git a/drivers/crypto/hifn_795x.c b/drivers/crypto/hifn_795x.c
index 4e89cd8..7706461 100644
--- a/drivers/crypto/hifn_795x.c
+++ b/drivers/crypto/hifn_795x.c
@@ -369,6 +369,8 @@ static atomic_t hifn_dev_number;
 #define	HIFN_D_DST_RSIZE		80*4
 #define	HIFN_D_RES_RSIZE		24*4
 
+#define HIFN_D_DST_DALIGN		4
+
 #define HIFN_QUEUE_LENGTH		HIFN_D_CMD_RSIZE-5
 
 #define AES_MIN_KEY_SIZE		16
@@ -1458,10 +1460,6 @@ static int ablkcipher_add(void *daddr, unsigned int *drestp, struct scatterlist
 static int ablkcipher_walk(struct ablkcipher_request *req,
 		struct ablkcipher_walk *w)
 {
-	unsigned blocksize =
-		crypto_ablkcipher_blocksize(crypto_ablkcipher_reqtfm(req));
-	unsigned alignmask =
-		crypto_ablkcipher_alignmask(crypto_ablkcipher_reqtfm(req));
 	struct scatterlist *src, *dst, *t;
 	void *daddr;
 	unsigned int nbytes = req->nbytes, offset, copy, diff;
@@ -1477,15 +1475,13 @@ static int ablkcipher_walk(struct ablkcipher_request *req,
 		dst = &req->dst[idx];
 
 		dprintk("\n%s: slen: %u, dlen: %u, soff: %u, doff: %u, offset: %u, "
-				"blocksize: %u, nbytes: %u.\n",
+				"nbytes: %u.\n",
 				__func__, src->length, dst->length, src->offset,
-				dst->offset, offset, blocksize, nbytes);
+				dst->offset, offset, nbytes);
 
-		if (src->length & (blocksize - 1) ||
-				src->offset & (alignmask - 1) ||
-				dst->length & (blocksize - 1) ||
-				dst->offset & (alignmask - 1) ||
-				offset) {
+		if (!IS_ALIGNED(dst->offset, HIFN_D_DST_DALIGN) ||
+		    !IS_ALIGNED(dst->length, HIFN_D_DST_DALIGN) ||
+		    offset) {
 			unsigned slen = src->length - offset;
 			unsigned dlen = PAGE_SIZE;
 
@@ -1498,8 +1494,8 @@ static int ablkcipher_walk(struct ablkcipher_request *req,
 
 			idx += err;
 
-			copy = slen & ~(blocksize - 1);
-			diff = slen & (blocksize - 1);
+			copy = slen & ~(HIFN_D_DST_DALIGN - 1);
+			diff = slen & (HIFN_D_DST_DALIGN - 1);
 
 			if (dlen < nbytes) {
 				/*
@@ -1507,7 +1503,7 @@ static int ablkcipher_walk(struct ablkcipher_request *req,
 				 * to put there additional blocksized chunk,
 				 * so we mark that page as containing only
 				 * blocksize aligned chunks:
-				 * 	t->length = (slen & ~(blocksize - 1));
+				 * 	t->length = (slen & ~(HIFN_D_DST_DALIGN - 1));
 				 * and increase number of bytes to be processed
 				 * in next chunk:
 				 * 	nbytes += diff;
@@ -1567,10 +1563,6 @@ static int hifn_setup_session(struct ablkcipher_request *req)
 	unsigned int nbytes = req->nbytes, idx = 0, len;
 	int err = -EINVAL, sg_num;
 	struct scatterlist *src, *dst, *t;
-	unsigned blocksize =
-		crypto_ablkcipher_blocksize(crypto_ablkcipher_reqtfm(req));
-	unsigned alignmask =
-		crypto_ablkcipher_alignmask(crypto_ablkcipher_reqtfm(req));
 
 	if (ctx->iv && !ctx->ivsize && ctx->mode != ACRYPTO_MODE_ECB)
 		goto err_out_exit;
@@ -1578,17 +1570,13 @@ static int hifn_setup_session(struct ablkcipher_request *req)
 	ctx->walk.flags = 0;
 
 	while (nbytes) {
-		src = &req->src[idx];
 		dst = &req->dst[idx];
 
-		if (src->length & (blocksize - 1) ||
-				src->offset & (alignmask - 1) ||
-				dst->length & (blocksize - 1) ||
-				dst->offset & (alignmask - 1)) {
+		if (!IS_ALIGNED(dst->offset, HIFN_D_DST_DALIGN) ||
+		    !IS_ALIGNED(dst->length, HIFN_D_DST_DALIGN))
 			ctx->walk.flags |= ASYNC_FLAGS_MISALIGNED;
-		}
 
-		nbytes -= src->length;
+		nbytes -= dst->length;
 		idx++;
 	}
 
@@ -2523,9 +2511,7 @@ static int hifn_alg_alloc(struct hifn_device *dev, struct hifn_alg_template *t)
 	alg->alg.cra_flags = CRYPTO_ALG_TYPE_ABLKCIPHER | CRYPTO_ALG_ASYNC;
 	alg->alg.cra_blocksize = t->bsize;
 	alg->alg.cra_ctxsize = sizeof(struct hifn_context);
-	alg->alg.cra_alignmask = 15;
-	if (t->bsize == 8)
-		alg->alg.cra_alignmask = 3;
+	alg->alg.cra_alignmask = 1;
 	alg->alg.cra_type = &crypto_ablkcipher_type;
 	alg->alg.cra_module = THIS_MODULE;
 	alg->alg.cra_u.ablkcipher = t->ablkcipher;

Re: [HIFN 05/n]: Fix data alignment checks

[Evgeniy Polyakov]

On Wed, May 07, 2008 at 02:26:30PM +0200, Patrick McHardy (kaber@trash.net) wrote:
> I'm not entirely sure about the alignmask change at the end of
> this patch, is an alignmask of 1 correct if no source buffer
> alignment is required, but the destination buffer should be
> (doesn't have to be though) 4 byte aligned?

> commit f76618d53e82c8905214e889a3f79f1816c680fb
> Author: Patrick McHardy <kaber@trash.net>
> Date:   Wed May 7 12:44:15 2008 +0200
> 
>     [HIFN]: Fix data alignment checks
>     
>     The check for misalignment of the scatterlist data has two bugs:
>     
>     - the source buffer doesn't need to be aligned at all
>     - the destination buffer and its size needs to be aligned to a multiple
>       of 4, not to the crypto alg blocksize
>     

If memory serves me right, both src and dst addresses have to be 4 bytes
aligned. Protocol alignment is not needed.

If it is not the issue, then I have no objections.

-- 
	Evgeniy Polyakov

Re: [HIFN 05/n]: Fix data alignment checks

[Patrick McHardy]

Evgeniy Polyakov wrote:
> On Wed, May 07, 2008 at 02:26:30PM +0200, Patrick McHardy (kaber@trash.net) wrote:
>   
>> I'm not entirely sure about the alignmask change at the end of
>> this patch, is an alignmask of 1 correct if no source buffer
>> alignment is required, but the destination buffer should be
>> (doesn't have to be though) 4 byte aligned?
>>     
>
>   
>> commit f76618d53e82c8905214e889a3f79f1816c680fb
>> Author: Patrick McHardy <kaber@trash.net>
>> Date:   Wed May 7 12:44:15 2008 +0200
>>
>>     [HIFN]: Fix data alignment checks
>>     
>>     The check for misalignment of the scatterlist data has two bugs:
>>     
>>     - the source buffer doesn't need to be aligned at all
>>     - the destination buffer and its size needs to be aligned to a multiple
>>       of 4, not to the crypto alg blocksize
>>     
>>     
>
> If memory serves me right, both src and dst addresses have to be 4 bytes
> aligned. Protocol alignment is not needed.
>
> If it is not the issue, then I have no objections.
>   

Of the data buffers only the destination buffer needs to be aligned,
see the Source Pointer description in "2.2 Source Descriptors" in
the HIFN documentation.

Re: [HIFN 05/n]: Fix data alignment checks

[Evgeniy Polyakov]

On Wed, May 07, 2008 at 02:45:15PM +0200, Patrick McHardy (kaber@trash.net) wrote:
> Of the data buffers only the destination buffer needs to be aligned,
> see the Source Pointer description in "2.2 Source Descriptors" in
> the HIFN documentation.

Ok, thanks for the reference.

-- 
	Evgeniy Polyakov

Re: [HIFN 05/n]: Fix data alignment checks

[Patrick McHardy]

Evgeniy Polyakov wrote:
> On Wed, May 07, 2008 at 02:45:15PM +0200, Patrick McHardy (kaber@trash.net) wrote:
>> Of the data buffers only the destination buffer needs to be aligned,
>> see the Source Pointer description in "2.2 Source Descriptors" in
>> the HIFN documentation.
> 
> Ok, thanks for the reference.


As a side-note: its also not just an overseight in the documentation,
not aligning the destination buffer to a multiple of 4 results in the
HW freezing, the source buffer alignment changes work fine though.

Re: [HIFN 05/n]: Fix data alignment checks

[Evgeniy Polyakov]

On Wed, May 07, 2008 at 03:05:48PM +0200, Patrick McHardy (kaber@trash.net) wrote:
> As a side-note: its also not just an overseight in the documentation,
> not aligning the destination buffer to a multiple of 4 results in the
> HW freezing, the source buffer alignment changes work fine though.

Probably I just mirrored dst to src alignment and thus forced mask to be
at least 3.

-- 
	Evgeniy Polyakov

Re: [HIFN 05/n]: Fix data alignment checks

[Herbert Xu]

On Wed, May 07, 2008 at 02:26:30PM +0200, Patrick McHardy wrote:
> I'm not entirely sure about the alignmask change at the end of
> this patch, is an alignmask of 1 correct if no source buffer

If no alignment is required you want 0, 1 means 2-byte aligned.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [HIFN 05/n]: Fix data alignment checks

[Patrick McHardy]

[-- Attachment #1: Type: text/plain, Size: 338 bytes --]

Herbert Xu wrote:
> On Wed, May 07, 2008 at 02:26:30PM +0200, Patrick McHardy wrote:
>   
>> I'm not entirely sure about the alignmask change at the end of
>> this patch, is an alignmask of 1 correct if no source buffer
>>     
>
> If no alignment is required you want 0, 1 means 2-byte aligned.
>   

Thanks, fixed in the patch below.



[-- Attachment #2: 05.diff --]
[-- Type: text/x-diff, Size: 4576 bytes --]

commit de0a3b3bdf5a4d91ff77547dfad96dcbfd5a989f
Author: Patrick McHardy <kaber@trash.net>
Date:   Wed May 7 13:14:50 2008 +0200

    [HIFN]: Fix data alignment checks
    
    The check for misalignment of the scatterlist data has two bugs:
    
    - the source buffer doesn't need to be aligned at all
    - the destination buffer and its size needs to be aligned to a multiple
      of 4, not to the crypto alg blocksize
    
    Introduce symbolic constant for destination buffer alignment requirements,
    use it instead of the crypto alg blocksize and remove the unnecessary
    checks for source buffer alignment and change cra_alignmask to zero.
    
    Signed-off-by: Patrick McHardy <kaber@trash.net>

diff --git a/drivers/crypto/hifn_795x.c b/drivers/crypto/hifn_795x.c
index 4e89cd8..4428e8e 100644
--- a/drivers/crypto/hifn_795x.c
+++ b/drivers/crypto/hifn_795x.c
@@ -369,6 +369,8 @@ static atomic_t hifn_dev_number;
 #define	HIFN_D_DST_RSIZE		80*4
 #define	HIFN_D_RES_RSIZE		24*4
 
+#define HIFN_D_DST_DALIGN		4
+
 #define HIFN_QUEUE_LENGTH		HIFN_D_CMD_RSIZE-5
 
 #define AES_MIN_KEY_SIZE		16
@@ -1458,10 +1460,6 @@ static int ablkcipher_add(void *daddr, unsigned int *drestp, struct scatterlist
 static int ablkcipher_walk(struct ablkcipher_request *req,
 		struct ablkcipher_walk *w)
 {
-	unsigned blocksize =
-		crypto_ablkcipher_blocksize(crypto_ablkcipher_reqtfm(req));
-	unsigned alignmask =
-		crypto_ablkcipher_alignmask(crypto_ablkcipher_reqtfm(req));
 	struct scatterlist *src, *dst, *t;
 	void *daddr;
 	unsigned int nbytes = req->nbytes, offset, copy, diff;
@@ -1477,15 +1475,13 @@ static int ablkcipher_walk(struct ablkcipher_request *req,
 		dst = &req->dst[idx];
 
 		dprintk("\n%s: slen: %u, dlen: %u, soff: %u, doff: %u, offset: %u, "
-				"blocksize: %u, nbytes: %u.\n",
+				"nbytes: %u.\n",
 				__func__, src->length, dst->length, src->offset,
-				dst->offset, offset, blocksize, nbytes);
+				dst->offset, offset, nbytes);
 
-		if (src->length & (blocksize - 1) ||
-				src->offset & (alignmask - 1) ||
-				dst->length & (blocksize - 1) ||
-				dst->offset & (alignmask - 1) ||
-				offset) {
+		if (!IS_ALIGNED(dst->offset, HIFN_D_DST_DALIGN) ||
+		    !IS_ALIGNED(dst->length, HIFN_D_DST_DALIGN) ||
+		    offset) {
 			unsigned slen = src->length - offset;
 			unsigned dlen = PAGE_SIZE;
 
@@ -1498,8 +1494,8 @@ static int ablkcipher_walk(struct ablkcipher_request *req,
 
 			idx += err;
 
-			copy = slen & ~(blocksize - 1);
-			diff = slen & (blocksize - 1);
+			copy = slen & ~(HIFN_D_DST_DALIGN - 1);
+			diff = slen & (HIFN_D_DST_DALIGN - 1);
 
 			if (dlen < nbytes) {
 				/*
@@ -1507,7 +1503,7 @@ static int ablkcipher_walk(struct ablkcipher_request *req,
 				 * to put there additional blocksized chunk,
 				 * so we mark that page as containing only
 				 * blocksize aligned chunks:
-				 * 	t->length = (slen & ~(blocksize - 1));
+				 * 	t->length = (slen & ~(HIFN_D_DST_DALIGN - 1));
 				 * and increase number of bytes to be processed
 				 * in next chunk:
 				 * 	nbytes += diff;
@@ -1567,10 +1563,6 @@ static int hifn_setup_session(struct ablkcipher_request *req)
 	unsigned int nbytes = req->nbytes, idx = 0, len;
 	int err = -EINVAL, sg_num;
 	struct scatterlist *src, *dst, *t;
-	unsigned blocksize =
-		crypto_ablkcipher_blocksize(crypto_ablkcipher_reqtfm(req));
-	unsigned alignmask =
-		crypto_ablkcipher_alignmask(crypto_ablkcipher_reqtfm(req));
 
 	if (ctx->iv && !ctx->ivsize && ctx->mode != ACRYPTO_MODE_ECB)
 		goto err_out_exit;
@@ -1578,17 +1570,13 @@ static int hifn_setup_session(struct ablkcipher_request *req)
 	ctx->walk.flags = 0;
 
 	while (nbytes) {
-		src = &req->src[idx];
 		dst = &req->dst[idx];
 
-		if (src->length & (blocksize - 1) ||
-				src->offset & (alignmask - 1) ||
-				dst->length & (blocksize - 1) ||
-				dst->offset & (alignmask - 1)) {
+		if (!IS_ALIGNED(dst->offset, HIFN_D_DST_DALIGN) ||
+		    !IS_ALIGNED(dst->length, HIFN_D_DST_DALIGN))
 			ctx->walk.flags |= ASYNC_FLAGS_MISALIGNED;
-		}
 
-		nbytes -= src->length;
+		nbytes -= dst->length;
 		idx++;
 	}
 
@@ -2523,9 +2511,7 @@ static int hifn_alg_alloc(struct hifn_device *dev, struct hifn_alg_template *t)
 	alg->alg.cra_flags = CRYPTO_ALG_TYPE_ABLKCIPHER | CRYPTO_ALG_ASYNC;
 	alg->alg.cra_blocksize = t->bsize;
 	alg->alg.cra_ctxsize = sizeof(struct hifn_context);
-	alg->alg.cra_alignmask = 15;
-	if (t->bsize == 8)
-		alg->alg.cra_alignmask = 3;
+	alg->alg.cra_alignmask = 0;
 	alg->alg.cra_type = &crypto_ablkcipher_type;
 	alg->alg.cra_module = THIS_MODULE;
 	alg->alg.cra_u.ablkcipher = t->ablkcipher;

[HIFN 06/n]: Properly handle requests for less than the full scatterlist

[Patrick McHardy]

[-- Attachment #1: Type: text/plain, Size: 1 bytes --]



[-- Attachment #2: 06.diff --]
[-- Type: text/x-diff, Size: 2797 bytes --]

commit 4f3353b225b7123cf9c04cb9ae3b987f0671ee26
Author: Patrick McHardy <kaber@trash.net>
Date:   Wed May 7 12:52:53 2008 +0200

    [HIFN]: Properly handle requests for less than the full scatterlist
    
    The scatterlist may contain more data than the crypto request, causing
    an underflow of the remaining byte count while walking the list.
    
    Use the minimum of the scatterlist element size and the remaining byte
    count specified in the crypto request to avoid this.
    
    Signed-off-by: Patrick McHardy <kaber@trash.net>

diff --git a/drivers/crypto/hifn_795x.c b/drivers/crypto/hifn_795x.c
index 7706461..650523c 100644
--- a/drivers/crypto/hifn_795x.c
+++ b/drivers/crypto/hifn_795x.c
@@ -1433,7 +1433,7 @@ static int ablkcipher_add(void *daddr, unsigned int *drestp, struct scatterlist
 		return -EINVAL;
 
 	while (size) {
-		copy = min(drest, src->length);
+		copy = min(drest, min(size, src->length));
 
 		saddr = kmap_atomic(sg_page(src), KM_SOFTIRQ1);
 		memcpy(daddr, saddr + src->offset, copy);
@@ -1482,7 +1482,7 @@ static int ablkcipher_walk(struct ablkcipher_request *req,
 		if (!IS_ALIGNED(dst->offset, HIFN_D_DST_DALIGN) ||
 		    !IS_ALIGNED(dst->length, HIFN_D_DST_DALIGN) ||
 		    offset) {
-			unsigned slen = src->length - offset;
+			unsigned slen = min(src->length - offset, nbytes);
 			unsigned dlen = PAGE_SIZE;
 
 			t = &w->cache[idx];
@@ -1540,7 +1540,7 @@ static int ablkcipher_walk(struct ablkcipher_request *req,
 
 			kunmap_atomic(daddr, KM_SOFTIRQ0);
 		} else {
-			nbytes -= src->length;
+			nbytes -= min(src->length, nbytes);
 			idx++;
 		}
 
@@ -1559,7 +1559,7 @@ static int hifn_setup_session(struct ablkcipher_request *req)
 	struct hifn_context *ctx = crypto_tfm_ctx(req->base.tfm);
 	struct hifn_device *dev = ctx->dev;
 	struct page *spage, *dpage;
-	unsigned long soff, doff, flags;
+	unsigned long soff, doff, dlen, flags;
 	unsigned int nbytes = req->nbytes, idx = 0, len;
 	int err = -EINVAL, sg_num;
 	struct scatterlist *src, *dst, *t;
@@ -1571,12 +1571,13 @@ static int hifn_setup_session(struct ablkcipher_request *req)
 
 	while (nbytes) {
 		dst = &req->dst[idx];
+		dlen = min(dst->length, nbytes);
 
 		if (!IS_ALIGNED(dst->offset, HIFN_D_DST_DALIGN) ||
-		    !IS_ALIGNED(dst->length, HIFN_D_DST_DALIGN))
+		    !IS_ALIGNED(dlen, HIFN_D_DST_DALIGN))
 			ctx->walk.flags |= ASYNC_FLAGS_MISALIGNED;
 
-		nbytes -= dst->length;
+		nbytes -= dlen;
 		idx++;
 	}
 
@@ -1631,7 +1632,7 @@ static int hifn_setup_session(struct ablkcipher_request *req)
 		if (err)
 			goto err_out;
 
-		nbytes -= len;
+		nbytes -= min(len, nbytes);
 	}
 
 	dev->active = HIFN_DEFAULT_ACTIVE_NUM;
@@ -1736,8 +1737,7 @@ static int ablkcipher_get(void *saddr, unsigned int *srestp, unsigned int offset
 		return -EINVAL;
 
 	while (size) {

Re: [HIFN 06/n]: Properly handle requests for less than the full scatterlist

[Evgeniy Polyakov]

On Wed, May 07, 2008 at 02:30:28PM +0200, Patrick McHardy (kaber@trash.net) wrote:
>     [HIFN]: Properly handle requests for less than the full scatterlist
>     
>     The scatterlist may contain more data than the crypto request, causing
>     an underflow of the remaining byte count while walking the list.
>     
>     Use the minimum of the scatterlist element size and the remaining byte
>     count specified in the crypto request to avoid this.
>     
>     Signed-off-by: Patrick McHardy <kaber@trash.net>

That one is the nastiest! Thanks a lot Patrick for fixing that.
Ack of course.

-- 
	Evgeniy Polyakov

[HIFN 07/n]: Use unique driver names for different algos

[Patrick McHardy]

[-- Attachment #1: Type: text/plain, Size: 1 bytes --]



[-- Attachment #2: 07.diff --]
[-- Type: text/x-diff, Size: 5078 bytes --]

commit 02cabb7369102c475dfa33dbb6787db1855a7062
Author: Patrick McHardy <kaber@trash.net>
Date:   Wed May 7 12:54:30 2008 +0200

    [HIFN]: Use unique driver names for different algos
    
    When the CryptoAPI instantiates a new algorithm, it performs a lookup
    by driver name. Since hifn uses the same name for all modes of one
    algorithm, the lookup may return an incorrect algorithm.
    
    Change the name to use <mode>-<algo>-<devicename> to provide unique
    names for the different combinations and devices.
    
    Signed-off-by: Patrick McHardy <kaber@trash.net>

diff --git a/drivers/crypto/hifn_795x.c b/drivers/crypto/hifn_795x.c
index 650523c..4027fa1 100644
--- a/drivers/crypto/hifn_795x.c
+++ b/drivers/crypto/hifn_795x.c
@@ -2355,7 +2355,7 @@ static struct hifn_alg_template hifn_alg_templates[] = {
 	 * 3DES ECB, CBC, CFB and OFB modes.
 	 */
 	{
-		.name = "cfb(des3_ede)", .drv_name = "hifn-3des", .bsize = 8,
+		.name = "cfb(des3_ede)", .drv_name = "cfb-3des", .bsize = 8,
 		.ablkcipher = {
 			.min_keysize	=	HIFN_3DES_KEY_LENGTH,
 			.max_keysize	=	HIFN_3DES_KEY_LENGTH,
@@ -2365,7 +2365,7 @@ static struct hifn_alg_template hifn_alg_templates[] = {
 		},
 	},
 	{
-		.name = "ofb(des3_ede)", .drv_name = "hifn-3des", .bsize = 8,
+		.name = "ofb(des3_ede)", .drv_name = "ofb-3des", .bsize = 8,
 		.ablkcipher = {
 			.min_keysize	=	HIFN_3DES_KEY_LENGTH,
 			.max_keysize	=	HIFN_3DES_KEY_LENGTH,
@@ -2375,7 +2375,7 @@ static struct hifn_alg_template hifn_alg_templates[] = {
 		},
 	},
 	{
-		.name = "cbc(des3_ede)", .drv_name = "hifn-3des", .bsize = 8,
+		.name = "cbc(des3_ede)", .drv_name = "cbc-3des", .bsize = 8,
 		.ablkcipher = {
 			.min_keysize	=	HIFN_3DES_KEY_LENGTH,
 			.max_keysize	=	HIFN_3DES_KEY_LENGTH,
@@ -2385,7 +2385,7 @@ static struct hifn_alg_template hifn_alg_templates[] = {
 		},
 	},
 	{
-		.name = "ecb(des3_ede)", .drv_name = "hifn-3des", .bsize = 8,
+		.name = "ecb(des3_ede)", .drv_name = "ecb-3des", .bsize = 8,
 		.ablkcipher = {
 			.min_keysize	=	HIFN_3DES_KEY_LENGTH,
 			.max_keysize	=	HIFN_3DES_KEY_LENGTH,
@@ -2399,7 +2399,7 @@ static struct hifn_alg_template hifn_alg_templates[] = {
 	 * DES ECB, CBC, CFB and OFB modes.
 	 */
 	{
-		.name = "cfb(des)", .drv_name = "hifn-des", .bsize = 8,
+		.name = "cfb(des)", .drv_name = "cfb-des", .bsize = 8,
 		.ablkcipher = {
 			.min_keysize	=	HIFN_DES_KEY_LENGTH,
 			.max_keysize	=	HIFN_DES_KEY_LENGTH,
@@ -2409,7 +2409,7 @@ static struct hifn_alg_template hifn_alg_templates[] = {
 		},
 	},
 	{
-		.name = "ofb(des)", .drv_name = "hifn-des", .bsize = 8,
+		.name = "ofb(des)", .drv_name = "ofb-des", .bsize = 8,
 		.ablkcipher = {
 			.min_keysize	=	HIFN_DES_KEY_LENGTH,
 			.max_keysize	=	HIFN_DES_KEY_LENGTH,
@@ -2419,7 +2419,7 @@ static struct hifn_alg_template hifn_alg_templates[] = {
 		},
 	},
 	{
-		.name = "cbc(des)", .drv_name = "hifn-des", .bsize = 8,
+		.name = "cbc(des)", .drv_name = "cbc-des", .bsize = 8,
 		.ablkcipher = {
 			.min_keysize	=	HIFN_DES_KEY_LENGTH,
 			.max_keysize	=	HIFN_DES_KEY_LENGTH,
@@ -2429,7 +2429,7 @@ static struct hifn_alg_template hifn_alg_templates[] = {
 		},
 	},
 	{
-		.name = "ecb(des)", .drv_name = "hifn-des", .bsize = 8,
+		.name = "ecb(des)", .drv_name = "ecb-des", .bsize = 8,
 		.ablkcipher = {
 			.min_keysize	=	HIFN_DES_KEY_LENGTH,
 			.max_keysize	=	HIFN_DES_KEY_LENGTH,
@@ -2443,7 +2443,7 @@ static struct hifn_alg_template hifn_alg_templates[] = {
 	 * AES ECB, CBC, CFB and OFB modes.
 	 */
 	{
-		.name = "ecb(aes)", .drv_name = "hifn-aes", .bsize = 16,
+		.name = "ecb(aes)", .drv_name = "ecb-aes", .bsize = 16,
 		.ablkcipher = {
 			.min_keysize	=	AES_MIN_KEY_SIZE,
 			.max_keysize	=	AES_MAX_KEY_SIZE,
@@ -2453,7 +2453,7 @@ static struct hifn_alg_template hifn_alg_templates[] = {
 		},
 	},
 	{
-		.name = "cbc(aes)", .drv_name = "hifn-aes", .bsize = 16,
+		.name = "cbc(aes)", .drv_name = "cbc-aes", .bsize = 16,
 		.ablkcipher = {
 			.min_keysize	=	AES_MIN_KEY_SIZE,
 			.max_keysize	=	AES_MAX_KEY_SIZE,
@@ -2463,7 +2463,7 @@ static struct hifn_alg_template hifn_alg_templates[] = {
 		},
 	},
 	{
-		.name = "cfb(aes)", .drv_name = "hifn-aes", .bsize = 16,
+		.name = "cfb(aes)", .drv_name = "cfb-aes", .bsize = 16,
 		.ablkcipher = {
 			.min_keysize	=	AES_MIN_KEY_SIZE,
 			.max_keysize	=	AES_MAX_KEY_SIZE,
@@ -2473,7 +2473,7 @@ static struct hifn_alg_template hifn_alg_templates[] = {
 		},
 	},
 	{
-		.name = "ofb(aes)", .drv_name = "hifn-aes", .bsize = 16,
+		.name = "ofb(aes)", .drv_name = "ofb-aes", .bsize = 16,
 		.ablkcipher = {
 			.min_keysize	=	AES_MIN_KEY_SIZE,
 			.max_keysize	=	AES_MAX_KEY_SIZE,
@@ -2505,7 +2505,8 @@ static int hifn_alg_alloc(struct hifn_device *dev, struct hifn_alg_template *t)
 		return -ENOMEM;
 
 	snprintf(alg->alg.cra_name, CRYPTO_MAX_ALG_NAME, "%s", t->name);
-	snprintf(alg->alg.cra_driver_name, CRYPTO_MAX_ALG_NAME, "%s", t->drv_name);
+	snprintf(alg->alg.cra_driver_name, CRYPTO_MAX_ALG_NAME, "%s-%s",
+		 t->drv_name, dev->name);
 
 	alg->alg.cra_priority = 300;
 	alg->alg.cra_flags = CRYPTO_ALG_TYPE_ABLKCIPHER | CRYPTO_ALG_ASYNC;

Re: [HIFN 07/n]: Use unique driver names for different algos

[Evgeniy Polyakov]

On Wed, May 07, 2008 at 02:32:45PM +0200, Patrick McHardy (kaber@trash.net) wrote:
>     [HIFN]: Use unique driver names for different algos
>     
>     When the CryptoAPI instantiates a new algorithm, it performs a lookup
>     by driver name. Since hifn uses the same name for all modes of one
>     algorithm, the lookup may return an incorrect algorithm.
>     
>     Change the name to use <mode>-<algo>-<devicename> to provide unique
>     names for the different combinations and devices.
>     
>     Signed-off-by: Patrick McHardy <kaber@trash.net>

Ack, thanks Patrick.

-- 
	Evgeniy Polyakov

[HIFN 08/n]: Properly initialize ivsize for CBC modes

[Patrick McHardy]

[-- Attachment #1: Type: text/plain, Size: 202 bytes --]

Two questions regarding this patch:

- do OFB and CFB also need ivsize initialization?

- the HIFN documentation doesn't mention OFB and CFB (the
  values are marked reserved), do they actually work?



[-- Attachment #2: 08.diff --]
[-- Type: text/x-diff, Size: 1407 bytes --]

commit d4ef6dd3f4e37a37c0823fd09c53657af63c8d88
Author: Patrick McHardy <kaber@trash.net>
Date:   Wed May 7 12:56:18 2008 +0200

    [HIFN]: Properly initialize ivsize for CBC modes
    
    For combined modes like cbc(aes) the driver is responsible for
    initializing ivsize.
    
    Signed-off-by: Patrick McHardy <kaber@trash.net>

diff --git a/drivers/crypto/hifn_795x.c b/drivers/crypto/hifn_795x.c
index 4027fa1..b28e8b1 100644
--- a/drivers/crypto/hifn_795x.c
+++ b/drivers/crypto/hifn_795x.c
@@ -2377,6 +2377,7 @@ static struct hifn_alg_template hifn_alg_templates[] = {
 	{
 		.name = "cbc(des3_ede)", .drv_name = "cbc-3des", .bsize = 8,
 		.ablkcipher = {
+			.ivsize		=	HIFN_IV_LENGTH,
 			.min_keysize	=	HIFN_3DES_KEY_LENGTH,
 			.max_keysize	=	HIFN_3DES_KEY_LENGTH,
 			.setkey		=	hifn_setkey,
@@ -2421,6 +2422,7 @@ static struct hifn_alg_template hifn_alg_templates[] = {
 	{
 		.name = "cbc(des)", .drv_name = "cbc-des", .bsize = 8,
 		.ablkcipher = {
+			.ivsize		=	HIFN_IV_LENGTH,
 			.min_keysize	=	HIFN_DES_KEY_LENGTH,
 			.max_keysize	=	HIFN_DES_KEY_LENGTH,
 			.setkey		=	hifn_setkey,
@@ -2455,6 +2457,7 @@ static struct hifn_alg_template hifn_alg_templates[] = {
 	{
 		.name = "cbc(aes)", .drv_name = "cbc-aes", .bsize = 16,
 		.ablkcipher = {
+			.ivsize		=	HIFN_AES_IV_LENGTH,
 			.min_keysize	=	AES_MIN_KEY_SIZE,
 			.max_keysize	=	AES_MAX_KEY_SIZE,
 			.setkey		=	hifn_setkey,

[HIFN 09/n]: Fix max queue length value

[Patrick McHardy]

[-- Attachment #1: Type: text/plain, Size: 1 bytes --]



[-- Attachment #2: 09.diff --]
[-- Type: text/x-diff, Size: 772 bytes --]

commit 21b18f9b22c7aa0a458c3f93fc1771a5eb5e70c8
Author: Patrick McHardy <kaber@trash.net>
Date:   Wed May 7 13:00:10 2008 +0200

    [HIFN]: Fix max queue length value
    
    All but the last element of the command and result descriptor rings can be
    used for crypto requests, fix HIFN_QUEUE_LENGTH.
    
    Signed-off-by: Patrick McHardy <kaber@trash.net>

diff --git a/drivers/crypto/hifn_795x.c b/drivers/crypto/hifn_795x.c
index b28e8b1..7d19b15 100644
--- a/drivers/crypto/hifn_795x.c
+++ b/drivers/crypto/hifn_795x.c
@@ -371,7 +371,7 @@ static atomic_t hifn_dev_number;
 
 #define HIFN_D_DST_DALIGN		4
 
-#define HIFN_QUEUE_LENGTH		HIFN_D_CMD_RSIZE-5
+#define HIFN_QUEUE_LENGTH		HIFN_D_CMD_RSIZE-1
 
 #define AES_MIN_KEY_SIZE		16
 #define AES_MAX_KEY_SIZE		32

Re: [HIFN 09/n]: Fix max queue length value

[Evgeniy Polyakov]

On Wed, May 07, 2008 at 02:38:31PM +0200, Patrick McHardy (kaber@trash.net) wrote:
> 

> commit 21b18f9b22c7aa0a458c3f93fc1771a5eb5e70c8
> Author: Patrick McHardy <kaber@trash.net>
> Date:   Wed May 7 13:00:10 2008 +0200
> 
>     [HIFN]: Fix max queue length value
>     
>     All but the last element of the command and result descriptor rings can be
>     used for crypto requests, fix HIFN_QUEUE_LENGTH.
>     
>     Signed-off-by: Patrick McHardy <kaber@trash.net>

Yaah, that's right, we can also wrap it into (), although current usage
is ok.

Thanks Patric.

-- 
	Evgeniy Polyakov

[HIFN 10/n]: Move command descriptor setup to seperate function

[Patrick McHardy]

[-- Attachment #1: Type: text/plain, Size: 180 bytes --]

Note: the DMA setup fixes mentioned in the changelog still
need a bit of work, so I'm not sure if I'll manage to send
them today. This one is fine for review or applying anyway.



[-- Attachment #2: 10.diff --]
[-- Type: text/x-diff, Size: 7414 bytes --]

commit 163c74fc4f18d086bbb7f114d55bc7dac939d6b7
Author: Patrick McHardy <kaber@trash.net>
Date:   Wed May 7 13:11:59 2008 +0200

    [HIFN]: Move command descriptor setup to seperate function
    
    Move command descriptor setup to seperate function as preparation
    for the following DMA setup fixes.
    
    Note 1: also fix a harmless typo while moving it: sa_idx is initialized
    	to dma->resi instead of dma->cmdi.
    
    Note 2: errors from command descriptor setup are not propagated back,
    	anymore, they can't be handled anyway and all conditions leading
    	to errors should be checked earlier.
    
    Signed-off-by: Patrick McHardy <kaber@trash.net>

diff --git a/drivers/crypto/hifn_795x.c b/drivers/crypto/hifn_795x.c
index 7d19b15..d1660b5 100644
--- a/drivers/crypto/hifn_795x.c
+++ b/drivers/crypto/hifn_795x.c
@@ -1168,109 +1168,15 @@ static int hifn_setup_crypto_command(struct hifn_device *dev,
 	return cmd_len;
 }
 
-static int hifn_setup_src_desc(struct hifn_device *dev, struct page *page,
-		unsigned int offset, unsigned int size)
-{
-	struct hifn_dma *dma = (struct hifn_dma *)dev->desc_virt;
-	int idx;
-	dma_addr_t addr;
-
-	addr = pci_map_page(dev->pdev, page, offset, size, PCI_DMA_TODEVICE);
-
-	idx = dma->srci;
-
-	dma->srcr[idx].p = __cpu_to_le32(addr);
-	dma->srcr[idx].l = __cpu_to_le32(size | HIFN_D_VALID |
-			HIFN_D_MASKDONEIRQ | HIFN_D_NOINVALID | HIFN_D_LAST);
-
-	if (++idx == HIFN_D_SRC_RSIZE) {
-		dma->srcr[idx].l = __cpu_to_le32(HIFN_D_VALID |
-				HIFN_D_JUMP |
-				HIFN_D_MASKDONEIRQ | HIFN_D_LAST);
-		idx = 0;
-	}
-
-	dma->srci = idx;
-	dma->srcu++;
-
-	if (!(dev->flags & HIFN_FLAG_SRC_BUSY)) {
-		hifn_write_1(dev, HIFN_1_DMA_CSR, HIFN_DMACSR_S_CTRL_ENA);
-		dev->flags |= HIFN_FLAG_SRC_BUSY;
-	}
-
-	return size;
-}
-
-static void hifn_setup_res_desc(struct hifn_device *dev)
-{
-	struct hifn_dma *dma = (struct hifn_dma *)dev->desc_virt;
-
-	dma->resr[dma->resi].l = __cpu_to_le32(HIFN_USED_RESULT |
-			HIFN_D_VALID | HIFN_D_LAST);
-	/*
-	 * dma->resr[dma->resi].l = __cpu_to_le32(HIFN_MAX_RESULT | HIFN_D_VALID |
-	 *					HIFN_D_LAST | HIFN_D_NOINVALID);
-	 */
-
-	if (++dma->resi == HIFN_D_RES_RSIZE) {
-		dma->resr[HIFN_D_RES_RSIZE].l = __cpu_to_le32(HIFN_D_VALID |
-				HIFN_D_JUMP | HIFN_D_MASKDONEIRQ | HIFN_D_LAST);
-		dma->resi = 0;
-	}
-
-	dma->resu++;
-
-	if (!(dev->flags & HIFN_FLAG_RES_BUSY)) {
-		hifn_write_1(dev, HIFN_1_DMA_CSR, HIFN_DMACSR_R_CTRL_ENA);
-		dev->flags |= HIFN_FLAG_RES_BUSY;
-	}
-}
-
-static void hifn_setup_dst_desc(struct hifn_device *dev, struct page *page,
-		unsigned offset, unsigned size)
-{
-	struct hifn_dma *dma = (struct hifn_dma *)dev->desc_virt;
-	int idx;
-	dma_addr_t addr;
-
-	addr = pci_map_page(dev->pdev, page, offset, size, PCI_DMA_FROMDEVICE);
-
-	idx = dma->dsti;
-	dma->dstr[idx].p = __cpu_to_le32(addr);
-	dma->dstr[idx].l = __cpu_to_le32(size |	HIFN_D_VALID |
-			HIFN_D_MASKDONEIRQ | HIFN_D_NOINVALID | HIFN_D_LAST);
-
-	if (++idx == HIFN_D_DST_RSIZE) {
-		dma->dstr[idx].l = __cpu_to_le32(HIFN_D_VALID |
-				HIFN_D_JUMP | HIFN_D_MASKDONEIRQ |
-				HIFN_D_LAST | HIFN_D_NOINVALID);
-		idx = 0;
-	}
-	dma->dsti = idx;
-	dma->dstu++;
-
-	if (!(dev->flags & HIFN_FLAG_DST_BUSY)) {
-		hifn_write_1(dev, HIFN_1_DMA_CSR, HIFN_DMACSR_D_CTRL_ENA);
-		dev->flags |= HIFN_FLAG_DST_BUSY;
-	}
-}
-
-static int hifn_setup_dma(struct hifn_device *dev, struct page *spage, unsigned int soff,
-		struct page *dpage, unsigned int doff, unsigned int nbytes, void *priv,
-		struct hifn_context *ctx)
+static int hifn_setup_cmd_desc(struct hifn_device *dev,
+		struct hifn_context *ctx, void *priv, unsigned int nbytes)
 {
 	struct hifn_dma *dma = (struct hifn_dma *)dev->desc_virt;
 	int cmd_len, sa_idx;
 	u8 *buf, *buf_pos;
 	u16 mask;
 
-	dprintk("%s: spage: %p, soffset: %u, dpage: %p, doffset: %u, nbytes: %u, priv: %p, ctx: %p.\n",
-			dev->name, spage, soff, dpage, doff, nbytes, priv, ctx);
-
-	sa_idx = dma->resi;
-
-	hifn_setup_src_desc(dev, spage, soff, nbytes);
-
+	sa_idx = dma->cmdi;
 	buf_pos = buf = dma->command_bufs[dma->cmdi];
 
 	mask = 0;
@@ -1372,16 +1278,113 @@ static int hifn_setup_dma(struct hifn_device *dev, struct page *spage, unsigned
 		hifn_write_1(dev, HIFN_1_DMA_CSR, HIFN_DMACSR_C_CTRL_ENA);
 		dev->flags |= HIFN_FLAG_CMD_BUSY;
 	}
-
-	hifn_setup_dst_desc(dev, dpage, doff, nbytes);
-	hifn_setup_res_desc(dev);
-
 	return 0;
 
 err_out:
 	return -EINVAL;
 }
 
+static int hifn_setup_src_desc(struct hifn_device *dev, struct page *page,
+		unsigned int offset, unsigned int size)
+{
+	struct hifn_dma *dma = (struct hifn_dma *)dev->desc_virt;
+	int idx;
+	dma_addr_t addr;
+
+	addr = pci_map_page(dev->pdev, page, offset, size, PCI_DMA_TODEVICE);
+
+	idx = dma->srci;
+
+	dma->srcr[idx].p = __cpu_to_le32(addr);
+	dma->srcr[idx].l = __cpu_to_le32(size | HIFN_D_VALID |
+			HIFN_D_MASKDONEIRQ | HIFN_D_NOINVALID | HIFN_D_LAST);
+
+	if (++idx == HIFN_D_SRC_RSIZE) {
+		dma->srcr[idx].l = __cpu_to_le32(HIFN_D_VALID |
+				HIFN_D_JUMP |
+				HIFN_D_MASKDONEIRQ | HIFN_D_LAST);
+		idx = 0;
+	}
+
+	dma->srci = idx;
+	dma->srcu++;
+
+	if (!(dev->flags & HIFN_FLAG_SRC_BUSY)) {
+		hifn_write_1(dev, HIFN_1_DMA_CSR, HIFN_DMACSR_S_CTRL_ENA);
+		dev->flags |= HIFN_FLAG_SRC_BUSY;
+	}
+
+	return size;
+}
+
+static void hifn_setup_res_desc(struct hifn_device *dev)
+{
+	struct hifn_dma *dma = (struct hifn_dma *)dev->desc_virt;
+
+	dma->resr[dma->resi].l = __cpu_to_le32(HIFN_USED_RESULT |
+			HIFN_D_VALID | HIFN_D_LAST);
+	/*
+	 * dma->resr[dma->resi].l = __cpu_to_le32(HIFN_MAX_RESULT | HIFN_D_VALID |
+	 *					HIFN_D_LAST | HIFN_D_NOINVALID);
+	 */
+
+	if (++dma->resi == HIFN_D_RES_RSIZE) {
+		dma->resr[HIFN_D_RES_RSIZE].l = __cpu_to_le32(HIFN_D_VALID |
+				HIFN_D_JUMP | HIFN_D_MASKDONEIRQ | HIFN_D_LAST);
+		dma->resi = 0;
+	}
+
+	dma->resu++;
+
+	if (!(dev->flags & HIFN_FLAG_RES_BUSY)) {
+		hifn_write_1(dev, HIFN_1_DMA_CSR, HIFN_DMACSR_R_CTRL_ENA);
+		dev->flags |= HIFN_FLAG_RES_BUSY;
+	}
+}
+
+static void hifn_setup_dst_desc(struct hifn_device *dev, struct page *page,
+		unsigned offset, unsigned size)
+{
+	struct hifn_dma *dma = (struct hifn_dma *)dev->desc_virt;
+	int idx;
+	dma_addr_t addr;
+
+	addr = pci_map_page(dev->pdev, page, offset, size, PCI_DMA_FROMDEVICE);
+
+	idx = dma->dsti;
+	dma->dstr[idx].p = __cpu_to_le32(addr);
+	dma->dstr[idx].l = __cpu_to_le32(size |	HIFN_D_VALID |
+			HIFN_D_MASKDONEIRQ | HIFN_D_NOINVALID | HIFN_D_LAST);
+
+	if (++idx == HIFN_D_DST_RSIZE) {
+		dma->dstr[idx].l = __cpu_to_le32(HIFN_D_VALID |
+				HIFN_D_JUMP | HIFN_D_MASKDONEIRQ |
+				HIFN_D_LAST | HIFN_D_NOINVALID);
+		idx = 0;
+	}
+	dma->dsti = idx;
+	dma->dstu++;
+
+	if (!(dev->flags & HIFN_FLAG_DST_BUSY)) {
+		hifn_write_1(dev, HIFN_1_DMA_CSR, HIFN_DMACSR_D_CTRL_ENA);
+		dev->flags |= HIFN_FLAG_DST_BUSY;
+	}
+}
+
+static int hifn_setup_dma(struct hifn_device *dev, struct page *spage, unsigned int soff,
+		struct page *dpage, unsigned int doff, unsigned int nbytes, void *priv,
+		struct hifn_context *ctx)
+{
+	dprintk("%s: spage: %p, soffset: %u, dpage: %p, doffset: %u, nbytes: %u, priv: %p, ctx: %p.\n",
+			dev->name, spage, soff, dpage, doff, nbytes, priv, ctx);
+
+	hifn_setup_src_desc(dev, spage, soff, nbytes);
+	hifn_setup_cmd_desc(dev, ctx, priv, nbytes);
+	hifn_setup_dst_desc(dev, dpage, doff, nbytes);
+	hifn_setup_res_desc(dev);
+	return 0;
+}
+
 static int ablkcipher_walk_init(struct ablkcipher_walk *w,
 		int num, gfp_t gfp_flags)
 {

Re: [HIFN 10/n]: Move command descriptor setup to seperate function

[Evgeniy Polyakov]

On Wed, May 07, 2008 at 02:50:29PM +0200, Patrick McHardy (kaber@trash.net) wrote:
> commit 163c74fc4f18d086bbb7f114d55bc7dac939d6b7
> Author: Patrick McHardy <kaber@trash.net>
> Date:   Wed May 7 13:11:59 2008 +0200
> 
>     [HIFN]: Move command descriptor setup to seperate function
>     
>     Move command descriptor setup to seperate function as preparation
>     for the following DMA setup fixes.
>     
>     Note 1: also fix a harmless typo while moving it: sa_idx is initialized
>     	to dma->resi instead of dma->cmdi.

Yep, resource and command indexes should be always equal.

>     Note 2: errors from command descriptor setup are not propagated back,
>     	anymore, they can't be handled anyway and all conditions leading
>     	to errors should be checked earlier.
>     
>     Signed-off-by: Patrick McHardy <kaber@trash.net>

I have no objections, thanks Patrick.
Ack.


-- 
	Evgeniy Polyakov

[HIFN 11/n]: Have HW invalidate src and dest descriptors after processing

[Patrick McHardy]

[-- Attachment #1: Type: text/plain, Size: 1 bytes --]



[-- Attachment #2: 11.diff --]
[-- Type: text/x-diff, Size: 1894 bytes --]

commit 0b4add2721b85f3ea873b87b02c057f2b5cff7aa
Author: Patrick McHardy <kaber@trash.net>
Date:   Wed May 7 13:15:44 2008 +0200

    [HIFN]: Have HW invalidate src and dest descriptors after processing
    
    The descriptors need to be invalidated after processing for ring
    cleanup to work properly and to avoid using an old destination
    descriptor when the src and cmd descriptors are already set up
    and the dst descriptor isn't.
    
    Signed-off-by: Patrick McHardy <kaber@trash.net>

diff --git a/drivers/crypto/hifn_795x.c b/drivers/crypto/hifn_795x.c
index c9fe18d..459d283 100644
--- a/drivers/crypto/hifn_795x.c
+++ b/drivers/crypto/hifn_795x.c
@@ -1297,7 +1297,7 @@ static int hifn_setup_src_desc(struct hifn_device *dev, struct page *page,
 
 	dma->srcr[idx].p = __cpu_to_le32(addr);
 	dma->srcr[idx].l = __cpu_to_le32(size | HIFN_D_VALID |
-			HIFN_D_MASKDONEIRQ | HIFN_D_NOINVALID | HIFN_D_LAST);
+			HIFN_D_MASKDONEIRQ | HIFN_D_LAST);
 
 	if (++idx == HIFN_D_SRC_RSIZE) {
 		dma->srcr[idx].l = __cpu_to_le32(HIFN_D_VALID |
@@ -1325,7 +1325,7 @@ static void hifn_setup_res_desc(struct hifn_device *dev)
 			HIFN_D_VALID | HIFN_D_LAST);
 	/*
 	 * dma->resr[dma->resi].l = __cpu_to_le32(HIFN_MAX_RESULT | HIFN_D_VALID |
-	 *					HIFN_D_LAST | HIFN_D_NOINVALID);
+	 *					HIFN_D_LAST);
 	 */
 
 	if (++dma->resi == HIFN_D_RES_RSIZE) {
@@ -1354,12 +1354,12 @@ static void hifn_setup_dst_desc(struct hifn_device *dev, struct page *page,
 	idx = dma->dsti;
 	dma->dstr[idx].p = __cpu_to_le32(addr);
 	dma->dstr[idx].l = __cpu_to_le32(size |	HIFN_D_VALID |
-			HIFN_D_MASKDONEIRQ | HIFN_D_NOINVALID | HIFN_D_LAST);
+			HIFN_D_MASKDONEIRQ | HIFN_D_LAST);
 
 	if (++idx == HIFN_D_DST_RSIZE) {
 		dma->dstr[idx].l = __cpu_to_le32(HIFN_D_VALID |
 				HIFN_D_JUMP | HIFN_D_MASKDONEIRQ |
-				HIFN_D_LAST | HIFN_D_NOINVALID);
+				HIFN_D_LAST);
 		idx = 0;
 	}
 	dma->dsti = idx;

Re: [HIFN 11/n]: Have HW invalidate src and dest descriptors after processing

[Evgeniy Polyakov]

On Wed, May 07, 2008 at 02:53:17PM +0200, Patrick McHardy (kaber@trash.net) wrote:
>     The descriptors need to be invalidated after processing for ring
>     cleanup to work properly and to avoid using an old destination
>     descriptor when the src and cmd descriptors are already set up
>     and the dst descriptor isn't.
>     
>     Signed-off-by: Patrick McHardy <kaber@trash.net>

Well, we only care about cmd register and do not process valid bit in
others, instead of cmd is not valid we do not use others, but we can
also check other descriptors too.

Thanks Patrick, I have no objections.
Ack.

-- 
	Evgeniy Polyakov

Re: [HIFN 11/n]: Have HW invalidate src and dest descriptors after processing

[Patrick McHardy]

Evgeniy Polyakov wrote:
> On Wed, May 07, 2008 at 02:53:17PM +0200, Patrick McHardy (kaber@trash.net) wrote:
>>     The descriptors need to be invalidated after processing for ring
>>     cleanup to work properly and to avoid using an old destination
>>     descriptor when the src and cmd descriptors are already set up
>>     and the dst descriptor isn't.
>>     
>>     Signed-off-by: Patrick McHardy <kaber@trash.net>
> 
> Well, we only care about cmd register and do not process valid bit in
> others, instead of cmd is not valid we do not use others, but we can
> also check other descriptors too.

I noticed the ring cleanup debugging printing ever increasing
use counts for src and dst descriptors, which went away with
this patch, but the more important point is avoiding that the
HW accidentally uses an old descriptors of course.

> Thanks Patrick, I have no objections.

Thanks Evgeniy. Thats it for now, next round will take until
tonight or maybe tommorrow.

Re: [HIFN 01/n]: Endianess fixes

[Evgeniy Polyakov]

On Wed, May 07, 2008 at 02:14:28PM +0200, Patrick McHardy (kaber@trash.net) wrote:
>     [HIFN]: Endianess fixes
>     
>     HIFN uses little-endian by default, move cpu_to_le32 conversion to hifn_write_0/
>     hifn_write_1, add sparse annotations and fix an invalid endian conversion in
>     hifn_setup_src_desc.

We can also be safe without volatile annotations.
Ack, thanks Patrick.


-- 
	Evgeniy Polyakov

Re: [HIFN 01/n]: Endianess fixes

[Patrick McHardy]

Evgeniy Polyakov wrote:
> On Wed, May 07, 2008 at 02:14:28PM +0200, Patrick McHardy (kaber@trash.net) wrote:
>   
>>     [HIFN]: Endianess fixes
>>     
>>     HIFN uses little-endian by default, move cpu_to_le32 conversion to hifn_write_0/
>>     hifn_write_1, add sparse annotations and fix an invalid endian conversion in
>>     hifn_setup_src_desc.
>>     
>
> We can also be safe without volatile annotations.
>   

Yes, will also be taken care of by a later patch :)

Re: [HIFN 01/n]: Endianess fixes

[Evgeniy Polyakov]

On Wed, May 07, 2008 at 03:01:55PM +0200, Patrick McHardy (kaber@trash.net) wrote:
> >We can also be safe without volatile annotations.
> >  
> 
> Yes, will also be taken care of by a later patch :)

I just receive them in a strange order and do not know when others will
arrive :)

-- 
	Evgeniy Polyakov

Re: [HIFN 01/n]: Endianess fixes

[Patrick McHardy]

Evgeniy Polyakov wrote:
> On Wed, May 07, 2008 at 03:01:55PM +0200, Patrick McHardy (kaber@trash.net) wrote:
>>> We can also be safe without volatile annotations.
>>>  
>> Yes, will also be taken care of by a later patch :)
> 
> I just receive them in a strange order and do not know when others will
> arrive :)


Appologies for imperfect ordering :) The patchset grew while fixing
issues as I ran into them and attempts to reorder caused too many
conflicts.

The removal of volatile doesn't belong in this patch though.

Re: [HIFN 01/n]: Endianess fixes

[Evgeniy Polyakov]

On Wed, May 07, 2008 at 03:46:12PM +0200, Patrick McHardy (kaber@trash.net) wrote:
> Appologies for imperfect ordering :) The patchset grew while fixing
> issues as I ran into them and attempts to reorder caused too many
> conflicts.

I believe it is more to mail system, which delivers 5 before 1 and so
on, not how patches are organized in the patchset :)

> The removal of volatile doesn't belong in this patch though.

Ok, no problem.

-- 
	Evgeniy Polyakov

Re: [HIFN 01/n]: Endianess fixes

[Herbert Xu]

On Wed, May 07, 2008 at 02:14:28PM +0200, Patrick McHardy wrote:
> Attached is the first of my fixes for the HIFN driver. I've
> got it to a working state with tcrypt, IPsec and dm-crypt, a
> few of the patches still need a bit work though, so I'll send
> the ones that I already consider ready one by one.

All 11 patches applied.  Thanks a lot Patrick!
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt