[PATCH 0/8] crypto: inside-secure - stabilization and fixes

[PATCH 0/8] crypto: inside-secure - stabilization and fixes

[Antoine Tenart]

Hi Herbert,

We spent a lot of time with Ofer to test various use cases of the Inside
Secure driver. We performed many tests in addition to the crypto
subsystem ones (IPsec, openssl speed, tcrypt...). As a result the driver
is a lot more stable in various configurations, with this series
applied.

The fixes of this series aren't all related, but were all found while
performing these stabilization process and tests.

Also as the company I'm working at changed its name from Free Electrons
to Bootlin, the first patch updates my email address in the MAINTAINERS
file.

The series is based on v4.16-rc1.

Thanks!
Antoine

Antoine Tenart (8):
  MAINTAINERS: update the Inside Secure maintainer email
  crypto: inside-secure - do not overwrite the threshold value
  crypto: inside-secure - fix the extra cache computation
  crypto: inside-secure - fix the cache_len computation
  crypto: inside-secure - do not process request if no command was
    issued
  crypto: inside-secure - fix the invalidation step during cra_exit
  crypto: inside-secure - keep the requests push/pop synced
  crypto: inside-secure - unmap the result in the hash send error path

 MAINTAINERS                                    |  2 +-
 drivers/crypto/inside-secure/safexcel.c        | 40 +++++++++++++++-----------
 drivers/crypto/inside-secure/safexcel.h        |  6 ++--
 drivers/crypto/inside-secure/safexcel_cipher.c |  2 +-
 drivers/crypto/inside-secure/safexcel_hash.c   | 10 ++++---
 5 files changed, 33 insertions(+), 27 deletions(-)

-- 
2.14.3

[PATCH 1/8] MAINTAINERS: update the Inside Secure maintainer email

[Antoine Tenart]

Free Electrons became Bootlin. Update my email accordingly.

Signed-off-by: Antoine Tenart <antoine.tenart@bootlin.com>
---
 MAINTAINERS | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index 3bdc260e36b7..d0f6811b9eed 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -6990,7 +6990,7 @@ F:	drivers/input/input-mt.c
 K:	\b(ABS|SYN)_MT_
 
 INSIDE SECURE CRYPTO DRIVER
-M:	Antoine Tenart <antoine.tenart@free-electrons.com>
+M:	Antoine Tenart <antoine.tenart@bootlin.com>
 F:	drivers/crypto/inside-secure/
 S:	Maintained
 L:	linux-crypto@vger.kernel.org
-- 
2.14.3

[PATCH 2/8] crypto: inside-secure - do not overwrite the threshold value

[Antoine Tenart]

This patch fixes the Inside Secure SafeXcel driver not to overwrite the
interrupt threshold value. In certain cases the value of this register,
which controls when to fire an interrupt, was overwritten. This lead to
packet not being processed or acked as the driver never was aware of
their completion.

This patch fixes this behaviour by not setting the threshold when
requests are being processed by the engine.

Fixes: dc7e28a3286e ("crypto: inside-secure - dequeue all requests at once")
Suggested-by: Ofer Heifetz <oferh@marvell.com>
Signed-off-by: Antoine Tenart <antoine.tenart@bootlin.com>
---
 drivers/crypto/inside-secure/safexcel.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/crypto/inside-secure/safexcel.c b/drivers/crypto/inside-secure/safexcel.c
index 225e74a7f724..5cb90bcd3f18 100644
--- a/drivers/crypto/inside-secure/safexcel.c
+++ b/drivers/crypto/inside-secure/safexcel.c
@@ -514,8 +514,7 @@ void safexcel_dequeue(struct safexcel_crypto_priv *priv, int ring)
 
 	if (!priv->ring[ring].busy) {
 		nreq -= safexcel_try_push_requests(priv, ring, nreq);
-		if (nreq)
-			priv->ring[ring].busy = true;
+		priv->ring[ring].busy = true;
 	}
 
 	priv->ring[ring].requests_left += nreq;
-- 
2.14.3

[PATCH 3/8] crypto: inside-secure - fix the extra cache computation

[Antoine Tenart]

This patch fixes the extra cache computation when the queued data is a
multiple of a block size. This fixes the hash support in some cases.

Fixes: 809778e02cd4 ("crypto: inside-secure - fix hash when length is a multiple of a block")
Signed-off-by: Antoine Tenart <antoine.tenart@bootlin.com>
---
 drivers/crypto/inside-secure/safexcel_hash.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/crypto/inside-secure/safexcel_hash.c b/drivers/crypto/inside-secure/safexcel_hash.c
index 122a2a58e98f..8fac23b78da5 100644
--- a/drivers/crypto/inside-secure/safexcel_hash.c
+++ b/drivers/crypto/inside-secure/safexcel_hash.c
@@ -198,7 +198,7 @@ static int safexcel_ahash_send_req(struct crypto_async_request *async, int ring,
 			/* If this is not the last request and the queued data
 			 * is a multiple of a block, cache the last one for now.
 			 */
-			extra = queued - crypto_ahash_blocksize(ahash);
+			extra = crypto_ahash_blocksize(ahash);
 
 		if (extra) {
 			sg_pcopy_to_buffer(areq->src, sg_nents(areq->src),
-- 
2.14.3

[PATCH 4/8] crypto: inside-secure - fix the cache_len computation

[Antoine Tenart]

This patch fixes the cache length computation as cache_len could end up
being a negative value. The check between the queued size and the
block size is updated to reflect the caching mechanism which can cache
up to a full block size (included!).

Fixes: 809778e02cd4 ("crypto: inside-secure - fix hash when length is a multiple of a block")
Signed-off-by: Antoine Tenart <antoine.tenart@bootlin.com>
---
 drivers/crypto/inside-secure/safexcel_hash.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/crypto/inside-secure/safexcel_hash.c b/drivers/crypto/inside-secure/safexcel_hash.c
index 8fac23b78da5..2951101e1831 100644
--- a/drivers/crypto/inside-secure/safexcel_hash.c
+++ b/drivers/crypto/inside-secure/safexcel_hash.c
@@ -184,7 +184,7 @@ static int safexcel_ahash_send_req(struct crypto_async_request *async, int ring,
 	int i, queued, len, cache_len, extra, n_cdesc = 0, ret = 0;
 
 	queued = len = req->len - req->processed;
-	if (queued < crypto_ahash_blocksize(ahash))
+	if (queued <= crypto_ahash_blocksize(ahash))
 		cache_len = queued;
 	else
 		cache_len = queued - areq->nbytes;
-- 
2.14.3

[PATCH 5/8] crypto: inside-secure - do not process request if no command was issued

[Antoine Tenart]

This patch adds a check in the SafeXcel dequeue function, to avoid
processing request further if no hardware command was issued. This can
happen in certain cases where the ->send() function caches all the data
that would have been send.

Fixes: 809778e02cd4 ("crypto: inside-secure - fix hash when length is a multiple of a block")
Signed-off-by: Antoine Tenart <antoine.tenart@bootlin.com>
---
 drivers/crypto/inside-secure/safexcel.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/crypto/inside-secure/safexcel.c b/drivers/crypto/inside-secure/safexcel.c
index 5cb90bcd3f18..0642d7181c9e 100644
--- a/drivers/crypto/inside-secure/safexcel.c
+++ b/drivers/crypto/inside-secure/safexcel.c
@@ -490,6 +490,15 @@ void safexcel_dequeue(struct safexcel_crypto_priv *priv, int ring)
 		if (backlog)
 			backlog->complete(backlog, -EINPROGRESS);
 
+		/* In case the send() helper did not issue any command to push
+		 * to the engine because the input data was cached, continue to
+		 * dequeue other requests as this is valid and not an error.
+		 */
+		if (!commands && !results) {
+			kfree(request);
+			continue;
+		}
+
 		spin_lock_bh(&priv->ring[ring].egress_lock);
 		list_add_tail(&request->list, &priv->ring[ring].list);
 		spin_unlock_bh(&priv->ring[ring].egress_lock);
-- 
2.14.3

[PATCH 6/8] crypto: inside-secure - fix the invalidation step during cra_exit

[Antoine Tenart]

When exiting a transformation, the cra_exit() helper is called in each
driver providing one. The Inside Secure SafeXcel driver has one, which
is responsible of freeing some areas and of sending one invalidation
request to the crypto engine, to invalidate the context that was used
during the transformation.

We could see in some setups (when lots of transformations were being
used with a short lifetime, and hence lots of cra_exit() calls) NULL
pointer dereferences and other weird issues. All these issues were
coming from accessing the tfm context.

The issue is the invalidation request completion is checked using a
wait_for_completion_interruptible() call in both the cipher and hash
cra_exit() helpers. In some cases this was interrupted while the
invalidation request wasn't processed yet. And then cra_exit() returned,
and its caller was freeing the tfm instance. Only then the request was
being handled by the SafeXcel driver, which lead to the said issues.

This patch fixes this by using wait_for_completion() calls in these
specific cases.

Fixes: 1b44c5a60c13 ("crypto: inside-secure - add SafeXcel EIP197 crypto engine driver")
Signed-off-by: Antoine Tenart <antoine.tenart@bootlin.com>
---
 drivers/crypto/inside-secure/safexcel_cipher.c | 2 +-
 drivers/crypto/inside-secure/safexcel_hash.c   | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/crypto/inside-secure/safexcel_cipher.c b/drivers/crypto/inside-secure/safexcel_cipher.c
index 63a8768ed2ae..17a7725a6f6d 100644
--- a/drivers/crypto/inside-secure/safexcel_cipher.c
+++ b/drivers/crypto/inside-secure/safexcel_cipher.c
@@ -456,7 +456,7 @@ static int safexcel_cipher_exit_inv(struct crypto_tfm *tfm)
 	queue_work(priv->ring[ring].workqueue,
 		   &priv->ring[ring].work_data.work);
 
-	wait_for_completion_interruptible(&result.completion);
+	wait_for_completion(&result.completion);
 
 	if (result.error) {
 		dev_warn(priv->dev,
diff --git a/drivers/crypto/inside-secure/safexcel_hash.c b/drivers/crypto/inside-secure/safexcel_hash.c
index 2951101e1831..43e94cd59c86 100644
--- a/drivers/crypto/inside-secure/safexcel_hash.c
+++ b/drivers/crypto/inside-secure/safexcel_hash.c
@@ -493,7 +493,7 @@ static int safexcel_ahash_exit_inv(struct crypto_tfm *tfm)
 	queue_work(priv->ring[ring].workqueue,
 		   &priv->ring[ring].work_data.work);
 
-	wait_for_completion_interruptible(&result.completion);
+	wait_for_completion(&result.completion);
 
 	if (result.error) {
 		dev_warn(priv->dev, "hash: completion error (%d)\n",
-- 
2.14.3

[PATCH 7/8] crypto: inside-secure - keep the requests push/pop synced

[Antoine Tenart]

This patch updates the Inside Secure SafeXcel driver to avoid being
out-of-sync between the number of requests sent and the one being
completed.

The number of requests acknowledged by the driver can be different than
the threshold that was configured if new requests were being pushed to
the h/w in the meantime. The driver wasn't taking those into account,
and the number of remaining requests to handled (to reconfigure the
interrupt threshold) could be out-of sync.

This patch fixes it by not taking in account the number of requests
left, but by taking in account the total number of requests being sent
to the hardware, so that new requests are being taken into account.

Fixes: dc7e28a3286e ("crypto: inside-secure - dequeue all requests at once")
Suggested-by: Ofer Heifetz <oferh@marvell.com>
Signed-off-by: Antoine Tenart <antoine.tenart@bootlin.com>
---
 drivers/crypto/inside-secure/safexcel.c | 28 +++++++++++++---------------
 drivers/crypto/inside-secure/safexcel.h |  6 ++----
 2 files changed, 15 insertions(+), 19 deletions(-)

diff --git a/drivers/crypto/inside-secure/safexcel.c b/drivers/crypto/inside-secure/safexcel.c
index 0642d7181c9e..956a37692e42 100644
--- a/drivers/crypto/inside-secure/safexcel.c
+++ b/drivers/crypto/inside-secure/safexcel.c
@@ -432,20 +432,18 @@ static int safexcel_hw_init(struct safexcel_crypto_priv *priv)
 }
 
 /* Called with ring's lock taken */
-static int safexcel_try_push_requests(struct safexcel_crypto_priv *priv,
-				      int ring, int reqs)
+static void safexcel_try_push_requests(struct safexcel_crypto_priv *priv,
+				       int ring)
 {
-	int coal = min_t(int, reqs, EIP197_MAX_BATCH_SZ);
+	int coal = min_t(int, priv->ring[ring].requests, EIP197_MAX_BATCH_SZ);
 
 	if (!coal)
-		return 0;
+		return;
 
 	/* Configure when we want an interrupt */
 	writel(EIP197_HIA_RDR_THRESH_PKT_MODE |
 	       EIP197_HIA_RDR_THRESH_PROC_PKT(coal),
 	       EIP197_HIA_RDR(priv, ring) + EIP197_HIA_xDR_THRESH);
-
-	return coal;
 }
 
 void safexcel_dequeue(struct safexcel_crypto_priv *priv, int ring)
@@ -521,13 +519,13 @@ void safexcel_dequeue(struct safexcel_crypto_priv *priv, int ring)
 
 	spin_lock_bh(&priv->ring[ring].egress_lock);
 
+	priv->ring[ring].requests += nreq;
+
 	if (!priv->ring[ring].busy) {
-		nreq -= safexcel_try_push_requests(priv, ring, nreq);
+		safexcel_try_push_requests(priv, ring);
 		priv->ring[ring].busy = true;
 	}
 
-	priv->ring[ring].requests_left += nreq;
-
 	spin_unlock_bh(&priv->ring[ring].egress_lock);
 
 	/* let the RDR know we have pending descriptors */
@@ -631,7 +629,7 @@ static inline void safexcel_handle_result_descriptor(struct safexcel_crypto_priv
 {
 	struct safexcel_request *sreq;
 	struct safexcel_context *ctx;
-	int ret, i, nreq, ndesc, tot_descs, done;
+	int ret, i, nreq, ndesc, tot_descs, handled = 0;
 	bool should_complete;
 
 handle_results:
@@ -667,6 +665,7 @@ static inline void safexcel_handle_result_descriptor(struct safexcel_crypto_priv
 
 		kfree(sreq);
 		tot_descs += ndesc;
+		handled++;
 	}
 
 acknowledge:
@@ -685,11 +684,10 @@ static inline void safexcel_handle_result_descriptor(struct safexcel_crypto_priv
 requests_left:
 	spin_lock_bh(&priv->ring[ring].egress_lock);
 
-	done = safexcel_try_push_requests(priv, ring,
-					  priv->ring[ring].requests_left);
+	priv->ring[ring].requests -= handled;
+	safexcel_try_push_requests(priv, ring);
 
-	priv->ring[ring].requests_left -= done;
-	if (!done && !priv->ring[ring].requests_left)
+	if (!priv->ring[ring].requests)
 		priv->ring[ring].busy = false;
 
 	spin_unlock_bh(&priv->ring[ring].egress_lock);
@@ -970,7 +968,7 @@ static int safexcel_probe(struct platform_device *pdev)
 			goto err_clk;
 		}
 
-		priv->ring[i].requests_left = 0;
+		priv->ring[i].requests = 0;
 		priv->ring[i].busy = false;
 
 		crypto_init_queue(&priv->ring[i].queue,
diff --git a/drivers/crypto/inside-secure/safexcel.h b/drivers/crypto/inside-secure/safexcel.h
index 4e219c21608b..caaf6a81b162 100644
--- a/drivers/crypto/inside-secure/safexcel.h
+++ b/drivers/crypto/inside-secure/safexcel.h
@@ -551,10 +551,8 @@ struct safexcel_crypto_priv {
 		struct crypto_queue queue;
 		spinlock_t queue_lock;
 
-		/* Number of requests in the engine that needs the threshold
-		 * interrupt to be set up.
-		 */
-		int requests_left;
+		/* Number of requests in the engine. */
+		int requests;
 
 		/* The ring is currently handling at least one request */
 		bool busy;
-- 
2.14.3

[PATCH 8/8] crypto: inside-secure - unmap the result in the hash send error path

[Antoine Tenart]

This patch adds a label to unmap the result buffer in the hash send
function error path.

Fixes: 1b44c5a60c13 ("crypto: inside-secure - add SafeXcel EIP197 crypto engine driver")
Suggested-by: Ofer Heifetz <oferh@marvell.com>
Signed-off-by: Antoine Tenart <antoine.tenart@bootlin.com>
---
 drivers/crypto/inside-secure/safexcel_hash.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/crypto/inside-secure/safexcel_hash.c b/drivers/crypto/inside-secure/safexcel_hash.c
index 43e94cd59c86..a4960a934eef 100644
--- a/drivers/crypto/inside-secure/safexcel_hash.c
+++ b/drivers/crypto/inside-secure/safexcel_hash.c
@@ -303,7 +303,7 @@ static int safexcel_ahash_send_req(struct crypto_async_request *async, int ring,
 				   req->state_sz);
 	if (IS_ERR(rdesc)) {
 		ret = PTR_ERR(rdesc);
-		goto cdesc_rollback;
+		goto unmap_result;
 	}
 
 	spin_unlock_bh(&priv->ring[ring].egress_lock);
@@ -315,6 +315,8 @@ static int safexcel_ahash_send_req(struct crypto_async_request *async, int ring,
 	*results = 1;
 	return 0;
 
+unmap_result:
+	dma_unmap_sg(priv->dev, areq->src, req->nents, DMA_TO_DEVICE);
 cdesc_rollback:
 	for (i = 0; i < n_cdesc; i++)
 		safexcel_ring_rollback_wptr(priv, &priv->ring[ring].cdr);
-- 
2.14.3