[PATCH v2 5/5] crypto: talitos: Add software backlog queue handling

[Kim Phillips]

On Thu, 5 Mar 2015 11:35:23 +0200
Horia Geantă <horia.geanta at freescale.com> wrote:

> On 3/4/2015 2:23 AM, Kim Phillips wrote:
> > On Tue,  3 Mar 2015 08:21:37 -0500
> > Martin Hicks <mort at bork.org> wrote:
> > 
> >> @@ -1170,6 +1237,8 @@ static struct talitos_edesc *talitos_edesc_alloc(struct device *dev,
> >>  						     edesc->dma_len,
> >>  						     DMA_BIDIRECTIONAL);
> >>  	edesc->req.desc = &edesc->desc;
> >> +	/* A copy of the crypto_async_request to use the crypto_queue backlog */
> >> +	memcpy(&edesc->req.base, areq, sizeof(struct crypto_async_request));
> > 
> > this seems backward, or, at least can be done more efficiently IMO:
> > talitos_cra_init should set the tfm's reqsize so the rest of
> > the driver can wholly embed its talitos_edesc (which should also
> > wholly encapsulate its talitos_request (i.e., not via a pointer))
> > into the crypto API's request handle allocation.  This
> > would absorb and eliminate the talitos_edesc kmalloc and frees, the
> > above memcpy, and replace the container_of after the
> > crypto_dequeue_request with an offset_of, right?
> > 
> > When scatter-gather buffers are needed, we can assume a slower-path
> > and make them do their own allocations, since their sizes vary
> > depending on each request.  Of course, a pointer to those
> > allocations would need to be retained somewhere in the request
> > handle.
> 
> Unfortunately talitos_edesc structure size is most of the times
> variable. Its exact size can only be established at "request time", and
> not at "tfm init time".

yes, I was suggesting a common minimum should be set in cra_init.

> Fixed size would be sizeof(talitos_edesc).
> Below are factors that influence the variable part, i.e. link_tbl in
> talitos_edesc:
> - whether any assoc / src / dst data is scattered
> - icv_stashing (case when ICV checking is done in SW)

both being slow(er) paths, IMO.

> Still we'd be better with:
> -crypto API allocates request + request context (i.e.
> sizeof(talitos_edesc) + any alignment required)
> -talitos driver allocates variable part of talitos_edesc (if needed)
> 
> instead of:
> -crypto API allocates request
> -talitos driver allocates talitos_edesc (fixed + variable)
> -memcopy of the req.base (crypto_async_request) into talitos_edesc
> 
> both in terms of performance and readability.

indeed.

> At first look, the driver wouldn't change that much:
> -talitos_cra_init() callback would have to set tfm.reqsize to
> sizeof(talitos_edesc) + padding and also add the CRYPTO_TFM_REQ_DMA
> indication in tfm.crt_flags
> -talitos_edesc_alloc() logic would be pretty much the same, but would
> allocate memory only for the link_tbl
> 
> I'm willing to do these changes if needed.

Please coordinate with Martin.

fwiw, caam could use this, too.

Kim