Re: [PATCH 4/7] crypto: marvell: Moving the tdma chain out of mv_cesa_tdma_req

On Thu, 16 Jun 2016 14:02:42 +0200
Romain Perier [off-list ref] wrote:

quoted

Now that the dma specific fields are part of the base request there's no
reason to keep this union.

You can just put struct mv_cesa_req base; directly under struct
mv_cesa_ablkcipher_req, and move mv_cesa_ablkcipher_std_req fields in
mv_cesa_ablkcipher_req.  

Well, I think that I might keep the changes related to mv_cesa_tdma_req 
in this commit (+ put struct mv_cesa_req base; direct under struct 
mv_cesa_ablkcipher_req) and move the changes related to 
mv_cesa_ablkcipher_std_req into another commit. What do you think ?

Sounds good.

quoted

quoted

  	struct mv_cesa_ahash_dma_iter iter;
  	struct mv_cesa_op_ctx *op = NULL;
  	unsigned int frag_len;

@@ -662,11 +661,6 @@ static int mv_cesa_ahash_req_init(struct ahash_request *req, bool *cached)
  	struct mv_cesa_ahash_req *creq = ahash_request_ctx(req);
  	int ret;

-	if (cesa_dev->caps->has_tdma)
-		creq->req.base.type = CESA_DMA_REQ;
-	else
-		creq->req.base.type = CESA_STD_REQ;
-  

Hm, where is it decided now? I mean, I don't see this test anywhere
else in your patch, which means you're now always using standard mode.  

It has been replaced by mv_cesa_req_get_type() + initializing 
chain.first to NULL in std_init. So, that's the same thing, no ?

And that's exactly my point :-). When these fields are NULL the request
is a STD request...

quoted

quoted

  	creq->src_nents = sg_nents_for_len(req->src, req->nbytes);
  	if (creq->src_nents < 0) {
  		dev_err(cesa_dev->dev, "Invalid number of src SG");

@@ -680,7 +674,7 @@ static int mv_cesa_ahash_req_init(struct ahash_request *req, bool *cached)
  	if (*cached)
  		return 0;

-	if (creq->req.base.type == CESA_DMA_REQ)
+	if (mv_cesa_req_get_type(&creq->req.base) == CESA_DMA_REQ)  

... and here you're testing if it's a DMA request, which will always be
false, since mv_cesa_ahash_dma_req_init() is the function supposed to
fill the ->first and ->last fields.

quoted

Should be

	if (cesa_dev->caps->has_tdma)
 

quoted

  		ret = mv_cesa_ahash_dma_req_init(req);  

Why ? mv_cesa_req_get_type() tests mv_cesa_req->chain and returns a code 
depending on its value. This value is initialized according to what is 
set un "has_tdma"...

As explained above, it's not ;).


-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com