: Endianess fixes

HIFN 01/n Patrick McHardy 3 participants 34 messages May 7, 2008 Crypto API

Share & tools

Patrick McHardy

author

2008-05-07 12:15 UTC #0 lore raw

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.

commit aa543b1fcddd097b5129a9a3056f888737d88102

Author: Patrick McHardy <[email protected]>
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 <[email protected]>

drivers/crypto/hifn_795x.c +30 ~~-30~~

index 81f3f95..d7a51ee 100644
@@ -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 |

Patrick McHardy

author

2008-05-07 12:16 UTC #1 lore raw

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

commit 9dbbaf326c4033cd8d973cb6bf10298faf50180c

Author: Patrick McHardy <[email protected]>
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 <[email protected]>

drivers/crypto/hifn_795x.c +1 -1

index d7a51ee..e5c3bc4 100644
@@ -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,

Evgeniy Polyakov

2008-05-07 12:23 UTC #2 lore raw

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

Quoted 7 lines · Patrick McHardy

[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 <[email protected]>

Acked :)

Signature · 1 line

	Evgeniy Polyakov

Patrick McHardy

author

2008-05-07 12:20 UTC #3 lore raw

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

In reply to Patrick McHardy

commit 835b2aece80670cd439ff74e51a647836f73d0ca

Author: Patrick McHardy <[email protected]>
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 <[email protected]>

drivers/crypto/hifn_795x.c +2 -2

index e5c3bc4..cce6e6f 100644
@@ -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;
 }
 
 /*

Evgeniy Polyakov

2008-05-07 12:23 UTC #4 lore raw

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

Hi.

Quoted 3 lines · Patrick McHardy

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?

drivers/crypto/hifn_795x.c static int hifn_setup_crypto(struct ablkcipher_request *req, u8 op, · line 2207

Quoted 9 lines

drivers/crypto/hifn_795x.c +2 -2

@@ -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...

Signature · 1 line

	Evgeniy Polyakov

Patrick McHardy

author

2008-05-07 12:29 UTC #5 lore raw

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

Quoted 4 lines · Evgeniy Polyakov

Hi.
On Wed, May 07, 2008 at 02:19:36PM +0200, Patrick McHardy ([email protected]) wrote:

Quoted 9 lines

Quoted 4 lines
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).

Quoted 8 lines

drivers/crypto/hifn_795x.c +1 -1

@@ -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);

Quoted 6 lines

drivers/crypto/hifn_795x.c static int hifn_setup_crypto(struct ablkcipher_request *req, u8 op, · line 2209
Quoted 3 lines
drivers/crypto/hifn_795x.c +1 -1
-	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.

Patrick McHardy

author

2008-05-07 12:21 UTC #6 lore raw

[HIFN 04/n]: Handle ablkcipher_walk errors

In reply to Patrick McHardy

commit 7e24c849df30a5d2b0bb89165b933ea2faa747bd

Author: Patrick McHardy <[email protected]>
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 <[email protected]>

drivers/crypto/hifn_795x.c +0 -0

index cce6e6f..4e89cd8 100644
@@ -1602,7 +1602,10 @@ static int hifn_setup_session(struct ablkcipher_request *req)
 	idx = 0;
 
 	sg_num = ablkcipher_walk(req, &ctx->walk);

Evgeniy Polyakov

2008-05-07 12:44 UTC #7 lore raw

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

Quoted 10 lines · Patrick McHardy

commit 7e24c849df30a5d2b0bb89165b933ea2faa747bd
Author: Patrick McHardy <[email protected]>
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 <[email protected]>

Ooops, ACK :)

Signature · 1 line

	Evgeniy Polyakov

Patrick McHardy

author

2008-05-07 12:27 UTC #8 lore raw

[HIFN 05/n]: Fix data alignment checks

In reply to Patrick McHardy

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 <[email protected]>
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 <[email protected]>

drivers/crypto/hifn_795x.c +14 ~~-28~~

index 4e89cd8..7706461 100644
@@ -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;

Evgeniy Polyakov

2008-05-07 12:42 UTC #9 lore raw

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

Quoted 9 lines · Patrick McHardy

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 <[email protected]>
Date:   Wed May 7 12:44:15 2008 +0200

Quoted 8 lines

[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.

Signature · 1 line

	Evgeniy Polyakov

Patrick McHardy

author

2008-05-07 12:45 UTC #10 lore raw

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

Quoted 23 lines · Evgeniy Polyakov

Quoted 11 lines · Patrick McHardy
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 <[email protected]>
Date:   Wed May 7 12:44:15 2008 +0200
Quoted 9 lines
[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

Quoted 5 lines

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.

Evgeniy Polyakov

2008-05-07 13:04 UTC #11 lore raw

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

Quoted 3 lines · Patrick McHardy

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.

Signature · 1 line

	Evgeniy Polyakov

Patrick McHardy

author

2008-05-07 13:06 UTC #12 lore raw

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

Quoted 6 lines · Evgeniy Polyakov

Quoted 3 lines · Patrick McHardy
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.

Evgeniy Polyakov

2008-05-07 13:23 UTC #13 lore raw

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

Quoted 3 lines · Patrick McHardy

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.

Signature · 1 line

	Evgeniy Polyakov

Herbert Xu

maintainer

2008-05-07 12:48 UTC #14 lore raw

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

In reply to Patrick McHardy

Quoted 2 lines · Patrick McHardy

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,

Signature · 4 lines

Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Patrick McHardy

author

2008-05-07 12:52 UTC #15 lore raw

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

Quoted 8 lines · Herbert Xu

Quoted 3 lines · Patrick McHardy
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.

commit de0a3b3bdf5a4d91ff77547dfad96dcbfd5a989f

Author: Patrick McHardy <[email protected]>
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

Signed-off-by Patrick McHardy <[email protected]>

drivers/crypto/hifn_795x.c +14 ~~-28~~

index 4e89cd8..4428e8e 100644
@@ -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;

Patrick McHardy

author

2008-05-07 12:31 UTC #16 lore raw

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

In reply to Patrick McHardy

commit 4f3353b225b7123cf9c04cb9ae3b987f0671ee26

Author: Patrick McHardy <[email protected]>
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 <[email protected]>

drivers/crypto/hifn_795x.c +8 -7

index 7706461..650523c 100644
@@ -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) {

Evgeniy Polyakov

2008-05-07 13:02 UTC #17 lore raw

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

Quoted 9 lines · Patrick McHardy

[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 <[email protected]>

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

Signature · 1 line

	Evgeniy Polyakov

Patrick McHardy

author

2008-05-07 12:33 UTC #18 lore raw

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

In reply to Patrick McHardy

commit 02cabb7369102c475dfa33dbb6787db1855a7062

Author: Patrick McHardy <[email protected]>
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 <[email protected]>

drivers/crypto/hifn_795x.c +14 ~~-13~~

index 650523c..4027fa1 100644
@@ -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;

Evgeniy Polyakov

2008-05-07 13:07 UTC #19 lore raw

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

Quoted 10 lines · Patrick McHardy

[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 <[email protected]>

Ack, thanks Patrick.

Signature · 1 line

	Evgeniy Polyakov

Patrick McHardy

author

2008-05-07 12:37 UTC #20 lore raw

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

In reply to Patrick McHardy

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?

commit d4ef6dd3f4e37a37c0823fd09c53657af63c8d88

Author: Patrick McHardy <[email protected]>
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 <[email protected]>

drivers/crypto/hifn_795x.c +3 -0

index 4027fa1..b28e8b1 100644
@@ -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,

Patrick McHardy

author

2008-05-07 12:39 UTC #21 lore raw

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

In reply to Patrick McHardy

commit 21b18f9b22c7aa0a458c3f93fc1771a5eb5e70c8

Author: Patrick McHardy <[email protected]>
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 <[email protected]>

drivers/crypto/hifn_795x.c +1 -1

index b28e8b1..7d19b15 100644
@@ -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

Evgeniy Polyakov

2008-05-07 12:47 UTC #22 lore raw

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

Quoted 6 lines · Patrick McHardy

commit 21b18f9b22c7aa0a458c3f93fc1771a5eb5e70c8
Author: Patrick McHardy <[email protected]>
Date:   Wed May 7 13:00:10 2008 +0200

Quoted 6 lines

[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 <[email protected]>

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

Thanks Patric.

Signature · 1 line

	Evgeniy Polyakov

Patrick McHardy

author

2008-05-07 12:51 UTC #23 lore raw

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

In reply to Patrick McHardy

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.

commit 163c74fc4f18d086bbb7f114d55bc7dac939d6b7

Author: Patrick McHardy <[email protected]>
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 <[email protected]>

drivers/crypto/hifn_795x.c +104 ~~-101~~

index 7d19b15..d1660b5 100644
@@ -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)
 {

Evgeniy Polyakov

2008-05-07 13:11 UTC #24 lore raw

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

Quoted 11 lines · Patrick McHardy

commit 163c74fc4f18d086bbb7f114d55bc7dac939d6b7
Author: Patrick McHardy <[email protected]>
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.

Quoted 5 lines

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 <[email protected]>

I have no objections, thanks Patrick.
Ack.

Signature · 1 line

	Evgeniy Polyakov

Patrick McHardy

author

2008-05-07 12:53 UTC #25 lore raw

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

In reply to Patrick McHardy

commit 0b4add2721b85f3ea873b87b02c057f2b5cff7aa

Author: Patrick McHardy <[email protected]>
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 <[email protected]>

drivers/crypto/hifn_795x.c +4 -4

index c9fe18d..459d283 100644
@@ -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;

Evgeniy Polyakov

2008-05-07 13:14 UTC #26 lore raw

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

Quoted 6 lines · Patrick McHardy

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 <[email protected]>

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.

Signature · 1 line

	Evgeniy Polyakov

Patrick McHardy

author

2008-05-07 13:18 UTC #27 lore raw

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

Quoted 11 lines · Evgeniy Polyakov

Quoted 6 lines · Patrick McHardy
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 <[email protected]>
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.

Quoted 1 line

Thanks Patrick, I have no objections.

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

Evgeniy Polyakov

2008-05-07 13:00 UTC #28 lore raw

In reply to Patrick McHardy

Quoted 5 lines · Patrick McHardy

[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.

Signature · 1 line

	Evgeniy Polyakov

Patrick McHardy

author

2008-05-07 13:02 UTC #29 lore raw

Quoted 11 lines · Evgeniy Polyakov

Quoted 6 lines · Patrick McHardy
[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 :)

Evgeniy Polyakov

2008-05-07 13:23 UTC #30 lore raw

Quoted 4 lines · Patrick McHardy

Quoted 2 lines
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 :)

Signature · 1 line

	Evgeniy Polyakov

Patrick McHardy

author

2008-05-07 13:46 UTC #31 lore raw

Quoted 7 lines · Evgeniy Polyakov

Quoted 3 lines · Patrick McHardy
Quoted 2 lines
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.

Evgeniy Polyakov

2008-05-07 14:04 UTC #32 lore raw

Quoted 3 lines · Patrick McHardy

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 :)

Quoted 1 line

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

Ok, no problem.

Signature · 1 line

	Evgeniy Polyakov

Herbert Xu

maintainer

2008-05-07 14:38 UTC #33 lore raw

In reply to Patrick McHardy

Quoted 4 lines · Patrick McHardy

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!

Signature · 4 lines

Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt