On 2/20/2015 6:21 PM, Martin Hicks wrote:
> I was running into situations where the hardware FIFO was filling up, and
> the code was returning EAGAIN to dm-crypt and just dropping the submitted
> crypto request.
> 
> This adds support in talitos for a software backlog queue.  When requests
> can't be queued to the hardware immediately EBUSY is returned.  The queued
> requests are dispatched to the hardware in received order as hardware FIFO
> slots become available.
> 
> Signed-off-by: Martin Hicks <m...@bork.org>

Hi Martin,

Thanks for the effort!
Indeed we noticed that talitos (and caam) don't play nicely with
dm-crypt, lacking a backlog mechanism.

Please run checkpatch --strict and fix the errors, warnings.

> ---
>  drivers/crypto/talitos.c |   92 
> +++++++++++++++++++++++++++++++++++-----------
>  drivers/crypto/talitos.h |    3 ++
>  2 files changed, 74 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/crypto/talitos.c b/drivers/crypto/talitos.c
> index d3472be..226654c 100644
> --- a/drivers/crypto/talitos.c
> +++ b/drivers/crypto/talitos.c
> @@ -183,43 +183,72 @@ static int init_device(struct device *dev)
>  }
>  
>  /**
> - * talitos_submit - submits a descriptor to the device for processing
> + * talitos_handle_queue - performs submissions either of new descriptors
> + *                        or ones waiting in the queue backlog.
>   * @dev:     the SEC device to be used
>   * @ch:              the SEC device channel to be used
> - * @edesc:   the descriptor to be processed by the device
> - * @context: a handle for use by caller (optional)

The "context" kernel-doc should have been removed in patch 4/5.

> + * @edesc:   the descriptor to be processed by the device (optional)
>   *
>   * desc must contain valid dma-mapped (bus physical) address pointers.
>   * callback must check err and feedback in descriptor header
> - * for device processing status.
> + * for device processing status upon completion.
>   */
> -int talitos_submit(struct device *dev, int ch, struct talitos_edesc *edesc)
> +int talitos_handle_queue(struct device *dev, int ch, struct talitos_edesc 
> *edesc)
>  {
>       struct talitos_private *priv = dev_get_drvdata(dev);
> -     struct talitos_request *request = &edesc->req;
> +     struct talitos_request *request, *orig_request = NULL;
> +     struct crypto_async_request *async_req;
>       unsigned long flags;
>       int head;
> +     int ret = -EINPROGRESS; 
>  
>       spin_lock_irqsave(&priv->chan[ch].head_lock, flags);
>  
> +     if (edesc) {
> +             orig_request = &edesc->req;
> +             crypto_enqueue_request(&priv->chan[ch].queue, 
> &orig_request->base);
> +     }

The request goes through the SW queue even if there are empty slots in
the HW queue, doing unnecessary crypto_queue_encrypt() and
crypto_queue_decrypt(). Trying to use the HW queue first would be better.

> +
> +flush_another:
> +     if (priv->chan[ch].queue.qlen == 0) {
> +             spin_unlock_irqrestore(&priv->chan[ch].head_lock, flags);
> +             return 0;
> +     }
> +
>       if (!atomic_inc_not_zero(&priv->chan[ch].submit_count)) {
>               /* h/w fifo is full */
>               spin_unlock_irqrestore(&priv->chan[ch].head_lock, flags);
> -             return -EAGAIN;
> +             return -EBUSY;
>       }
>  
> -     head = priv->chan[ch].head;
> +     /* Dequeue the oldest request */
> +     async_req = crypto_dequeue_request(&priv->chan[ch].queue);
> +
> +     request = container_of(async_req, struct talitos_request, base);
>       request->dma_desc = dma_map_single(dev, request->desc,
>                                          sizeof(*request->desc),
>                                          DMA_BIDIRECTIONAL);
>  
>       /* increment fifo head */
> +     head = priv->chan[ch].head;
>       priv->chan[ch].head = (priv->chan[ch].head + 1) & (priv->fifo_len - 1);
>  
> -     smp_wmb();
> -     priv->chan[ch].fifo[head] = request;
> +     spin_unlock_irqrestore(&priv->chan[ch].head_lock, flags);
> +
> +     /*
> +      * Mark a backlogged request as in-progress, return EBUSY because
> +      * the original request that was submitted is backlogged.

s/is backlogged/is backlogged or dropped
Original request will not be enqueued by crypto_queue_enqueue() if the
CRYPTO_TFM_REQ_MAY_BACKLOG flag is not set (since SW queue is for
backlog only) - that's the case for IPsec requests.

> +      */
> +     if (request != orig_request) {
> +             struct crypto_async_request *areq = request->context;
> +             areq->complete(areq, -EINPROGRESS);
> +             ret = -EBUSY;
> +     }
> +
> +     spin_lock_irqsave(&priv->chan[ch].head_lock, flags);
>  
>       /* GO! */
> +     priv->chan[ch].fifo[head] = request;
>       wmb();
>       out_be32(priv->chan[ch].reg + TALITOS_FF,
>                upper_32_bits(request->dma_desc));
> @@ -228,9 +257,18 @@ int talitos_submit(struct device *dev, int ch, struct 
> talitos_edesc *edesc)
>  
>       spin_unlock_irqrestore(&priv->chan[ch].head_lock, flags);
>  
> -     return -EINPROGRESS;
> +     /*
> +      * When handling the queue via the completion path, queue more
> +      * requests if the hardware has room.
> +      */
> +     if (!edesc) {
> +             spin_lock_irqsave(&priv->chan[ch].head_lock, flags);
> +             goto flush_another;
> +     }

This is better - avoids releasing and reacquiring the lock:

if (!edesc) {
        goto flush_another;
}

spin_unlock_irqrestore(&priv->chan[ch].head_lock, flags);


> +
> +     return ret;
>  }
> -EXPORT_SYMBOL(talitos_submit);
> +EXPORT_SYMBOL(talitos_handle_queue);
>  
>  /*
>   * process what was done, notify callback of error if not
> @@ -284,6 +322,8 @@ static void flush_channel(struct device *dev, int ch, int 
> error, int reset_ch)
>       }
>  
>       spin_unlock_irqrestore(&priv->chan[ch].tail_lock, flags);
> +
> +     talitos_handle_queue(dev, ch, NULL);
>  }
>  
>  /*
> @@ -1038,8 +1078,8 @@ static int ipsec_esp(struct talitos_edesc *edesc, 
> struct aead_request *areq,
>       edesc->req.callback = callback;
>       edesc->req.context = areq;
>  
> -     ret = talitos_submit(dev, ctx->ch, edesc);
> -     if (ret != -EINPROGRESS) {
> +     ret = talitos_handle_queue(dev, ctx->ch, edesc);
> +     if (ret != -EINPROGRESS && ret != -EBUSY) {

Again, factor in CRYPTO_TFM_REQ_MAY_BACKLOG.
Only when talitos_handle_queue() returns -EBUSY *and*
CRYPTO_TFM_REQ_MAY_BACKLOG is set, the request is backlogged.

Thus the 2nd condition should be:
!(ret == -EBUSY && areq->base.flags & CRYPTO_TFM_REQ_MAY_BACKLOG)

Other places need similar fix.


>               ipsec_esp_unmap(dev, edesc, areq);
>               kfree(edesc);
>       }
> @@ -1080,6 +1120,7 @@ static struct talitos_edesc *talitos_edesc_alloc(struct 
> device *dev,
>                                                unsigned int ivsize,
>                                                int icv_stashing,
>                                                u32 cryptoflags,
> +                                              struct crypto_async_request 
> *areq,
>                                                bool encrypt)
>  {
>       struct talitos_edesc *edesc;
> @@ -1170,6 +1211,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));

Why not have a
struct crypto_async_request *base;
instead of
struct crypto_async_request base;
in talitos_request structure?

This way:
1. memcopy above is avoided
2. talitos_request structure gets smaller - remember that
talitos_request is now embedded in talitos_edesc structure, which gets
kmalloc-ed for every request

That would be similar to previous driver behaviour.

Caller is expected not to destroy the request if the return code from
the Crypto API / backend driver is -EINPROGRESS or -EBUSY (when
MAY_BACKLOG flag is set).

>  
>       return edesc;
>  }
> @@ -1184,7 +1227,7 @@ static struct talitos_edesc *aead_edesc_alloc(struct 
> aead_request *areq, u8 *iv,
>       return talitos_edesc_alloc(ctx->dev, areq->assoc, areq->src, areq->dst,
>                                  iv, areq->assoclen, areq->cryptlen,
>                                  ctx->authsize, ivsize, icv_stashing,
> -                                areq->base.flags, encrypt);
> +                                areq->base.flags, &areq->base, encrypt);
>  }
>  
>  static int aead_encrypt(struct aead_request *req)
> @@ -1413,8 +1456,8 @@ static int common_nonsnoop(struct talitos_edesc *edesc,
>       edesc->req.callback = callback;
>       edesc->req.context = areq;
>  
> -     ret = talitos_submit(dev, ctx->ch, edesc);
> -     if (ret != -EINPROGRESS) {
> +     ret = talitos_handle_queue(dev, ctx->ch, edesc);
> +     if (ret != -EINPROGRESS && ret != -EBUSY) {
>               common_nonsnoop_unmap(dev, edesc, areq);
>               kfree(edesc);
>       }
> @@ -1430,7 +1473,7 @@ static struct talitos_edesc 
> *ablkcipher_edesc_alloc(struct ablkcipher_request *
>  
>       return talitos_edesc_alloc(ctx->dev, NULL, areq->src, areq->dst,
>                                  areq->info, 0, areq->nbytes, 0, ivsize, 0,
> -                                areq->base.flags, encrypt);
> +                                areq->base.flags, &areq->base, encrypt);
>  }
>  
>  static int ablkcipher_encrypt(struct ablkcipher_request *areq)
> @@ -1596,8 +1639,8 @@ static int common_nonsnoop_hash(struct talitos_edesc 
> *edesc,
>       edesc->req.callback = callback;
>       edesc->req.context = areq;
>  
> -     ret = talitos_submit(dev, ctx->ch, edesc);
> -     if (ret != -EINPROGRESS) {
> +     ret = talitos_handle_queue(dev, ctx->ch, edesc);
> +     if (ret != -EINPROGRESS && ret != -EBUSY) {
>               common_nonsnoop_hash_unmap(dev, edesc, areq);
>               kfree(edesc);
>       }
> @@ -1612,7 +1655,7 @@ static struct talitos_edesc *ahash_edesc_alloc(struct 
> ahash_request *areq,
>       struct talitos_ahash_req_ctx *req_ctx = ahash_request_ctx(areq);
>  
>       return talitos_edesc_alloc(ctx->dev, NULL, req_ctx->psrc, NULL, NULL, 0,
> -                                nbytes, 0, 0, 0, areq->base.flags, false);
> +                                nbytes, 0, 0, 0, areq->base.flags, 
> &areq->base, false);
>  }
>  
>  static int ahash_init(struct ahash_request *areq)
> @@ -2690,6 +2733,13 @@ static int talitos_probe(struct platform_device *ofdev)
>               }
>  
>               atomic_set(&priv->chan[i].submit_count, -priv->chfifo_len);
> +
> +             /*
> +              * The crypto_queue is used to manage the backlog only.  While
> +              * the hardware FIFO has space requests are dispatched
> +              * immediately.
> +              */
> +             crypto_init_queue(&priv->chan[i].queue, 0);
>       }
>  
>       dma_set_mask(dev, DMA_BIT_MASK(36));
> diff --git a/drivers/crypto/talitos.h b/drivers/crypto/talitos.h
> index 91faa76..a6f73e2 100644
> --- a/drivers/crypto/talitos.h
> +++ b/drivers/crypto/talitos.h
> @@ -65,6 +65,7 @@ struct talitos_desc {
>   * @context: caller context (optional)
>   */
>  struct talitos_request {
> +     struct crypto_async_request base;
>       struct talitos_desc *desc;
>       dma_addr_t dma_desc;
>       void (*callback) (struct device *dev, struct talitos_desc *desc,
> @@ -91,6 +92,8 @@ struct talitos_channel {
>       spinlock_t tail_lock ____cacheline_aligned;
>       /* index to next in-progress/done descriptor request */
>       int tail;
> +
> +     struct crypto_queue queue;
>  };
>  
>  struct talitos_private {
> 



--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to