diff mbox series

[v6,2/7] crypto: caam - drop global context pointer and init_done

Message ID 20200108154047.12526-3-andrew.smirnov@gmail.com (mailing list archive)
State Changes Requested
Delegated to: Herbert Xu
Headers show
Series enable CAAM's HWRNG as default | expand

Commit Message

Andrey Smirnov Jan. 8, 2020, 3:40 p.m. UTC 
Leverage devres to get rid of code storing global context as well as
init_done flag.

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
Cc: Chris Healy <cphealy@gmail.com>
Cc: Lucas Stach <l.stach@pengutronix.de>
Cc: Horia Geantă <horia.geanta@nxp.com>
Cc: Herbert Xu <herbert@gondor.apana.org.au>
Cc: Iuliana Prodan <iuliana.prodan@nxp.com>
Cc: linux-crypto@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: linux-imx@nxp.com
---
 drivers/crypto/caam/caamrng.c | 60 ++++++++++++-----------------------
 drivers/crypto/caam/intern.h  |  5 ---
 drivers/crypto/caam/jr.c      |  1 -
 3 files changed, 20 insertions(+), 46 deletions(-)

Comments

Horia Geanta Jan. 13, 2020, 9:41 a.m. UTC | #1 
On 1/8/2020 5:42 PM, Andrey Smirnov wrote:
> @@ -342,18 +324,16 @@ int caam_rng_init(struct device *ctrldev)
>  	if (!rng_inst)
>  		return 0;
>  
> -	rng_ctx = kmalloc(sizeof(*rng_ctx), GFP_DMA | GFP_KERNEL);
> -	if (!rng_ctx)
> +	ctx = devm_kzalloc(ctrldev, sizeof(*ctx), GFP_DMA | GFP_KERNEL);
> +	if (!ctx)
>  		return -ENOMEM;
>  
> -	dev_info(ctrldev, "registering rng-caam\n");
> +	ctx->rng.name    = "rng-caam";
> +	ctx->rng.init    = caam_init;
> +	ctx->rng.cleanup = caam_cleanup;
> +	ctx->rng.read    = caam_read;
>  
> -	err = hwrng_register(&caam_rng);
> -	if (!err) {
> -		init_done = true;
> -		return err;
> -	}
> +	dev_info(ctrldev, "registering rng-caam\n");
>  
> -	kfree(rng_ctx);
> -	return err;
> +	return devm_hwrng_register(ctrldev, &ctx->rng);
This means hwrng_unregister() is called only when ctrldev is removed.

OTOH caam_rng_init() could be called multiple times, e.g. if there's only one
jrdev left in the system and it's removed then added back.
This will lead to caam_rng_init() -> hwrng_register() called twice
with the same "rng-caam" name, without a hwrng_unregister() called in-between.

Horia
Andrey Smirnov Jan. 27, 2020, 1:44 p.m. UTC | #2 
On Mon, Jan 13, 2020 at 1:41 AM Horia Geanta <horia.geanta@nxp.com> wrote:
>
> On 1/8/2020 5:42 PM, Andrey Smirnov wrote:
> > @@ -342,18 +324,16 @@ int caam_rng_init(struct device *ctrldev)
> >       if (!rng_inst)
> >               return 0;
> >
> > -     rng_ctx = kmalloc(sizeof(*rng_ctx), GFP_DMA | GFP_KERNEL);
> > -     if (!rng_ctx)
> > +     ctx = devm_kzalloc(ctrldev, sizeof(*ctx), GFP_DMA | GFP_KERNEL);
> > +     if (!ctx)
> >               return -ENOMEM;
> >
> > -     dev_info(ctrldev, "registering rng-caam\n");
> > +     ctx->rng.name    = "rng-caam";
> > +     ctx->rng.init    = caam_init;
> > +     ctx->rng.cleanup = caam_cleanup;
> > +     ctx->rng.read    = caam_read;
> >
> > -     err = hwrng_register(&caam_rng);
> > -     if (!err) {
> > -             init_done = true;
> > -             return err;
> > -     }
> > +     dev_info(ctrldev, "registering rng-caam\n");
> >
> > -     kfree(rng_ctx);
> > -     return err;
> > +     return devm_hwrng_register(ctrldev, &ctx->rng);
> This means hwrng_unregister() is called only when ctrldev is removed.
>
> OTOH caam_rng_init() could be called multiple times, e.g. if there's only one
> jrdev left in the system and it's removed then added back.
> This will lead to caam_rng_init() -> hwrng_register() called twice
> with the same "rng-caam" name, without a hwrng_unregister() called in-between.
>

True, but the logic you describe is broken in reality due to circular
reference from HWRNG, which we never fixed. I'll fix both in v7.

Thanks,
Andrey Smirnov
diff mbox series

Patch

diff --git a/drivers/crypto/caam/caamrng.c b/drivers/crypto/caam/caamrng.c
index 1ce7fbd29e85..fe187db91233 100644
--- a/drivers/crypto/caam/caamrng.c
+++ b/drivers/crypto/caam/caamrng.c
@@ -70,6 +70,7 @@  struct buf_data {
 
 /* rng per-device context */
 struct caam_rng_ctx {
+	struct hwrng rng;
 	struct device *jrdev;
 	dma_addr_t sh_desc_dma;
 	u32 sh_desc[DESC_RNG_LEN];
@@ -78,13 +79,10 @@  struct caam_rng_ctx {
 	struct buf_data bufs[2];
 };
 
-static struct caam_rng_ctx *rng_ctx;
-
-/*
- * Variable used to avoid double free of resources in case
- * algorithm registration was unsuccessful
- */
-static bool init_done;
+static struct caam_rng_ctx *to_caam_rng_ctx(struct hwrng *r)
+{
+	return container_of(r, struct caam_rng_ctx, rng);
+}
 
 static inline void rng_unmap_buf(struct device *jrdev, struct buf_data *bd)
 {
@@ -143,7 +141,7 @@  static inline int submit_job(struct caam_rng_ctx *ctx, int to_current)
 
 static int caam_read(struct hwrng *rng, void *data, size_t max, bool wait)
 {
-	struct caam_rng_ctx *ctx = rng_ctx;
+	struct caam_rng_ctx *ctx = to_caam_rng_ctx(rng);
 	struct buf_data *bd = &ctx->bufs[ctx->current_buf];
 	int next_buf_idx, copied_idx;
 	int err;
@@ -246,17 +244,18 @@  static inline int rng_create_job_desc(struct caam_rng_ctx *ctx, int buf_id)
 
 static void caam_cleanup(struct hwrng *rng)
 {
+	struct caam_rng_ctx *ctx = to_caam_rng_ctx(rng);
 	int i;
 	struct buf_data *bd;
 
 	for (i = 0; i < 2; i++) {
-		bd = &rng_ctx->bufs[i];
+		bd = &ctx->bufs[i];
 		if (atomic_read(&bd->empty) == BUF_PENDING)
 			wait_for_completion(&bd->filled);
 	}
 
-	rng_unmap_ctx(rng_ctx);
-	caam_jr_free(rng_ctx->jrdev);
+	rng_unmap_ctx(ctx);
+	caam_jr_free(ctx->jrdev);
 }
 
 static int caam_init_buf(struct caam_rng_ctx *ctx, int buf_id)
@@ -277,7 +276,7 @@  static int caam_init_buf(struct caam_rng_ctx *ctx, int buf_id)
 
 static int caam_init(struct hwrng *rng)
 {
-	struct caam_rng_ctx *ctx = rng_ctx;
+	struct caam_rng_ctx *ctx = to_caam_rng_ctx(rng);
 	int err;
 
 	ctx->jrdev = caam_jr_alloc();
@@ -309,28 +308,11 @@  static int caam_init(struct hwrng *rng)
 	return err;
 }
 
-static struct hwrng caam_rng = {
-	.name		= "rng-caam",
-	.init           = caam_init,
-	.cleanup	= caam_cleanup,
-	.read		= caam_read,
-};
-
-void caam_rng_exit(void)
-{
-	if (!init_done)
-		return;
-
-	hwrng_unregister(&caam_rng);
-	kfree(rng_ctx);
-}
-
 int caam_rng_init(struct device *ctrldev)
 {
+	struct caam_rng_ctx *ctx;
 	u32 rng_inst;
 	struct caam_drv_private *priv = dev_get_drvdata(ctrldev);
-	int err;
-	init_done = false;
 
 	/* Check for an instantiated RNG before registration */
 	if (priv->era < 10)
@@ -342,18 +324,16 @@  int caam_rng_init(struct device *ctrldev)
 	if (!rng_inst)
 		return 0;
 
-	rng_ctx = kmalloc(sizeof(*rng_ctx), GFP_DMA | GFP_KERNEL);
-	if (!rng_ctx)
+	ctx = devm_kzalloc(ctrldev, sizeof(*ctx), GFP_DMA | GFP_KERNEL);
+	if (!ctx)
 		return -ENOMEM;
 
-	dev_info(ctrldev, "registering rng-caam\n");
+	ctx->rng.name    = "rng-caam";
+	ctx->rng.init    = caam_init;
+	ctx->rng.cleanup = caam_cleanup;
+	ctx->rng.read    = caam_read;
 
-	err = hwrng_register(&caam_rng);
-	if (!err) {
-		init_done = true;
-		return err;
-	}
+	dev_info(ctrldev, "registering rng-caam\n");
 
-	kfree(rng_ctx);
-	return err;
+	return devm_hwrng_register(ctrldev, &ctx->rng);
 }
diff --git a/drivers/crypto/caam/intern.h b/drivers/crypto/caam/intern.h
index c7c10c90464b..6d64931409eb 100644
--- a/drivers/crypto/caam/intern.h
+++ b/drivers/crypto/caam/intern.h
@@ -161,7 +161,6 @@  static inline void caam_pkc_exit(void)
 #ifdef CONFIG_CRYPTO_DEV_FSL_CAAM_RNG_API
 
 int caam_rng_init(struct device *dev);
-void caam_rng_exit(void);
 
 #else
 
@@ -170,10 +169,6 @@  static inline int caam_rng_init(struct device *dev)
 	return 0;
 }
 
-static inline void caam_rng_exit(void)
-{
-}
-
 #endif /* CONFIG_CRYPTO_DEV_FSL_CAAM_RNG_API */
 
 #ifdef CONFIG_CAAM_QI
diff --git a/drivers/crypto/caam/jr.c b/drivers/crypto/caam/jr.c
index fc97cde27059..f15d0d92c031 100644
--- a/drivers/crypto/caam/jr.c
+++ b/drivers/crypto/caam/jr.c
@@ -53,7 +53,6 @@  static void unregister_algs(void)
 
 	caam_qi_algapi_exit();
 
-	caam_rng_exit();
 	caam_pkc_exit();
 	caam_algapi_hash_exit();
 	caam_algapi_exit();