Linux-Crypto Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH 00/12] CAAM bugfixes, small improvements
@ 2019-09-04  2:35 Andrey Smirnov
  2019-09-04  2:35 ` [PATCH 01/12] crypto: caam - make sure clocks are enabled first Andrey Smirnov
                   ` (12 more replies)
  0 siblings, 13 replies; 43+ messages in thread
From: Andrey Smirnov @ 2019-09-04  2:35 UTC (permalink / raw)
  To: linux-crypto
  Cc: Andrey Smirnov, Chris Healy, Lucas Stach, Horia Geantă,
	Herbert Xu, Iuliana Prodan, linux-kernel

Everyone:

This series bugfixes and small improvement I made while doing more
testing of CAAM code:

 - "crypto: caam - make sure clocks are enabled first"

   fixes a recent regression (and, conincidentally a leak cause by one
   of my i.MX8MQ patches)

 - "crypto: caam - use devres to unmap JR's registers"
   "crypto: caam - check irq_of_parse_and_map for errors"

   are small improvements

 - "crypto: caam - dispose of IRQ mapping only after IRQ is freed"

   fixes a bug introduced by my i.MX8MQ series

 - "crypto: caam - use devres to unmap memory"
   "crypto: caam - use devres to remove debugfs"
   "crypto: caam - use devres to de-initialize the RNG"
   "crypto: caam - use devres to de-initialize QI"
   "crypto: caam - user devres to populate platform devices"
   "crypto: caam - populate platform devices last"

   are devres conversions/small improvments

 - "crypto: caam - convert caamrng to platform device"
   "crypto: caam - change JR device ownership scheme"

   are more of an RFC than proper fixes. I don't have a very high
   confidence in those fixes, but I think they are a good conversation
   stater about the best approach to fix those issues

Thanks,
Andrey Smirnov

Andrey Smirnov (12):
  crypto: caam - make sure clocks are enabled first
  crypto: caam - use devres to unmap JR's registers
  crypto: caam - check irq_of_parse_and_map for errors
  crypto: caam - dispose of IRQ mapping only after IRQ is freed
  crypto: caam - use devres to unmap memory
  crypto: caam - use devres to remove debugfs
  crypto: caam - use devres to de-initialize the RNG
  crypto: caam - use devres to de-initialize QI
  crypto: caam - user devres to populate platform devices
  crypto: caam - populate platform devices last
  crypto: caam - convert caamrng to platform device
  crypto: caam - change JR device ownership scheme

 drivers/crypto/caam/caamrng.c | 102 +++++-------
 drivers/crypto/caam/ctrl.c    | 294 ++++++++++++++++++----------------
 drivers/crypto/caam/intern.h  |   4 -
 drivers/crypto/caam/jr.c      |  90 ++++++++---
 drivers/crypto/caam/qi.c      |   8 +-
 drivers/crypto/caam/qi.h      |   1 -
 6 files changed, 267 insertions(+), 232 deletions(-)

-- 
2.21.0


^ permalink raw reply	[flat|nested] 43+ messages in thread

* [PATCH 01/12] crypto: caam - make sure clocks are enabled first
  2019-09-04  2:35 [PATCH 00/12] CAAM bugfixes, small improvements Andrey Smirnov
@ 2019-09-04  2:35 ` Andrey Smirnov
  2019-09-06 11:18   ` Horia Geanta
  2019-09-04  2:35 ` [PATCH 02/12] crypto: caam - use devres to unmap JR's registers Andrey Smirnov
                   ` (11 subsequent siblings)
  12 siblings, 1 reply; 43+ messages in thread
From: Andrey Smirnov @ 2019-09-04  2:35 UTC (permalink / raw)
  To: linux-crypto
  Cc: Andrey Smirnov, Chris Healy, Lucas Stach, Horia Geantă,
	Herbert Xu, Iuliana Prodan, linux-kernel

In order to access IP block's registers we need to enable appropriate
clocks first, otherwise we are risking hanging the CPU.

The problem becomes very apparent when trying to use CAAM driver built
as a kernel module. In that case caam_probe() gets called after
clk_disable_unused() which means all of the necessary clocks are
guaranteed to be disabled.

Coincidentally, this change also fixes iomap leak introduced by early
return (instead of "goto iounmap_ctrl") in commit
41fc54afae70 ("crypto: caam - simplfy clock initialization")

Tested on ZII i.MX6Q+ RDU2

Fixes: 176435ad2ac7 ("crypto: caam - defer probing until QMan is available")
Fixes: 41fc54afae70 ("crypto: caam - simplfy clock initialization")
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
---
 drivers/crypto/caam/ctrl.c | 30 +++++++++++++++---------------
 1 file changed, 15 insertions(+), 15 deletions(-)

diff --git a/drivers/crypto/caam/ctrl.c b/drivers/crypto/caam/ctrl.c
index 3c059d0e4207..db22777d59b4 100644
--- a/drivers/crypto/caam/ctrl.c
+++ b/drivers/crypto/caam/ctrl.c
@@ -594,6 +594,21 @@ static int caam_probe(struct platform_device *pdev)
 	dev_set_drvdata(dev, ctrlpriv);
 	nprop = pdev->dev.of_node;
 
+	imx_soc_match = soc_device_match(caam_imx_soc_table);
+	caam_imx = (bool)imx_soc_match;
+
+	if (imx_soc_match) {
+		if (!imx_soc_match->data) {
+			dev_err(dev, "No clock data provided for i.MX SoC");
+			return -EINVAL;
+		}
+
+		ret = init_clocks(dev, imx_soc_match->data);
+		if (ret)
+			return ret;
+	}
+
+
 	/* Get configuration properties from device tree */
 	/* First, get register page */
 	ctrl = of_iomap(nprop, 0);
@@ -604,9 +619,6 @@ static int caam_probe(struct platform_device *pdev)
 
 	caam_little_end = !(bool)(rd_reg32(&ctrl->perfmon.status) &
 				  (CSTA_PLEND | CSTA_ALT_PLEND));
-	imx_soc_match = soc_device_match(caam_imx_soc_table);
-	caam_imx = (bool)imx_soc_match;
-
 	comp_params = rd_reg32(&ctrl->perfmon.comp_parms_ms);
 	if (comp_params & CTPR_MS_PS && rd_reg32(&ctrl->mcr) & MCFGR_LONG_PTR)
 		caam_ptr_sz = sizeof(u64);
@@ -640,18 +652,6 @@ static int caam_probe(struct platform_device *pdev)
 	}
 #endif
 
-	if (imx_soc_match) {
-		if (!imx_soc_match->data) {
-			dev_err(dev, "No clock data provided for i.MX SoC");
-			return -EINVAL;
-		}
-
-		ret = init_clocks(dev, imx_soc_match->data);
-		if (ret)
-			return ret;
-	}
-
-
 	/* Allocating the BLOCK_OFFSET based on the supported page size on
 	 * the platform
 	 */
-- 
2.21.0


^ permalink raw reply	[flat|nested] 43+ messages in thread

* [PATCH 02/12] crypto: caam - use devres to unmap JR's registers
  2019-09-04  2:35 [PATCH 00/12] CAAM bugfixes, small improvements Andrey Smirnov
  2019-09-04  2:35 ` [PATCH 01/12] crypto: caam - make sure clocks are enabled first Andrey Smirnov
@ 2019-09-04  2:35 ` Andrey Smirnov
  2019-09-04  2:43   ` Fabio Estevam
  2019-09-09 13:01   ` Horia Geanta
  2019-09-04  2:35 ` [PATCH 03/12] crypto: caam - check irq_of_parse_and_map for errors Andrey Smirnov
                   ` (10 subsequent siblings)
  12 siblings, 2 replies; 43+ messages in thread
From: Andrey Smirnov @ 2019-09-04  2:35 UTC (permalink / raw)
  To: linux-crypto
  Cc: Andrey Smirnov, Chris Healy, Lucas Stach, Horia Geantă,
	Herbert Xu, Iuliana Prodan, linux-kernel

Use devres to unmap memory and drop explicit de-initialization
code.

NOTE: There's no corresponding unmapping code in caam_jr_remove which
seems like a resource leak.

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
---
 drivers/crypto/caam/jr.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/crypto/caam/jr.c b/drivers/crypto/caam/jr.c
index 417ad52615c6..7947d61a25cf 100644
--- a/drivers/crypto/caam/jr.c
+++ b/drivers/crypto/caam/jr.c
@@ -498,6 +498,7 @@ static int caam_jr_probe(struct platform_device *pdev)
 	struct caam_job_ring __iomem *ctrl;
 	struct caam_drv_private_jr *jrpriv;
 	static int total_jobrs;
+	struct resource *r;
 	int error;
 
 	jrdev = &pdev->dev;
@@ -513,9 +514,15 @@ static int caam_jr_probe(struct platform_device *pdev)
 	nprop = pdev->dev.of_node;
 	/* Get configuration properties from device tree */
 	/* First, get register page */
-	ctrl = of_iomap(nprop, 0);
+	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!r) {
+		dev_err(jrdev, "platform_get_resource() failed\n");
+		return -ENOMEM;
+	}
+
+	ctrl = devm_ioremap(jrdev, r->start, resource_size(r));
 	if (!ctrl) {
-		dev_err(jrdev, "of_iomap() failed\n");
+		dev_err(jrdev, "devm_ioremap() failed\n");
 		return -ENOMEM;
 	}
 
@@ -525,7 +532,6 @@ static int caam_jr_probe(struct platform_device *pdev)
 	if (error) {
 		dev_err(jrdev, "dma_set_mask_and_coherent failed (%d)\n",
 			error);
-		iounmap(ctrl);
 		return error;
 	}
 
@@ -536,7 +542,6 @@ static int caam_jr_probe(struct platform_device *pdev)
 	error = caam_jr_init(jrdev); /* now turn on hardware */
 	if (error) {
 		irq_dispose_mapping(jrpriv->irq);
-		iounmap(ctrl);
 		return error;
 	}
 
-- 
2.21.0


^ permalink raw reply	[flat|nested] 43+ messages in thread

* [PATCH 03/12] crypto: caam - check irq_of_parse_and_map for errors
  2019-09-04  2:35 [PATCH 00/12] CAAM bugfixes, small improvements Andrey Smirnov
  2019-09-04  2:35 ` [PATCH 01/12] crypto: caam - make sure clocks are enabled first Andrey Smirnov
  2019-09-04  2:35 ` [PATCH 02/12] crypto: caam - use devres to unmap JR's registers Andrey Smirnov
@ 2019-09-04  2:35 ` Andrey Smirnov
  2019-09-06 12:29   ` Horia Geanta
  2019-09-04  2:35 ` [PATCH 04/12] crypto: caam - dispose of IRQ mapping only after IRQ is freed Andrey Smirnov
                   ` (9 subsequent siblings)
  12 siblings, 1 reply; 43+ messages in thread
From: Andrey Smirnov @ 2019-09-04  2:35 UTC (permalink / raw)
  To: linux-crypto
  Cc: Andrey Smirnov, Chris Healy, Lucas Stach, Horia Geantă,
	Herbert Xu, Iuliana Prodan, linux-kernel

Irq_of_parse_and_map will return zero in case of error, so add a error
check for that.

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
---
 drivers/crypto/caam/jr.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/crypto/caam/jr.c b/drivers/crypto/caam/jr.c
index 7947d61a25cf..2732f3a0725a 100644
--- a/drivers/crypto/caam/jr.c
+++ b/drivers/crypto/caam/jr.c
@@ -537,6 +537,10 @@ static int caam_jr_probe(struct platform_device *pdev)
 
 	/* Identify the interrupt */
 	jrpriv->irq = irq_of_parse_and_map(nprop, 0);
+	if (!jrpriv->irq) {
+		dev_err(jrdev, "irq_of_parse_and_map failed\n");
+		return -EINVAL;
+	}
 
 	/* Now do the platform independent part */
 	error = caam_jr_init(jrdev); /* now turn on hardware */
-- 
2.21.0


^ permalink raw reply	[flat|nested] 43+ messages in thread

* [PATCH 04/12] crypto: caam - dispose of IRQ mapping only after IRQ is freed
  2019-09-04  2:35 [PATCH 00/12] CAAM bugfixes, small improvements Andrey Smirnov
                   ` (2 preceding siblings ...)
  2019-09-04  2:35 ` [PATCH 03/12] crypto: caam - check irq_of_parse_and_map for errors Andrey Smirnov
@ 2019-09-04  2:35 ` Andrey Smirnov
  2019-09-06 12:26   ` Horia Geanta
  2019-09-09  7:46   ` crypto: caam - Cast to long first before pointer conversion Herbert Xu
  2019-09-04  2:35 ` [PATCH 05/12] crypto: caam - use devres to unmap memory Andrey Smirnov
                   ` (8 subsequent siblings)
  12 siblings, 2 replies; 43+ messages in thread
From: Andrey Smirnov @ 2019-09-04  2:35 UTC (permalink / raw)
  To: linux-crypto
  Cc: Andrey Smirnov, Chris Healy, Lucas Stach, Horia Geantă,
	Herbert Xu, Iuliana Prodan, linux-kernel

With IRQ requesting being managed by devres we need to make sure that
we dispose of IRQ mapping after and not before it is free'd (otherwise
we'll end up with a warning from the kernel). To achieve that simply
convert IRQ mapping to rely on devres as well.

Fixes: f314f12db65c ("crypto: caam - convert caam_jr_init() to use devres")
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
---
 drivers/crypto/caam/jr.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/crypto/caam/jr.c b/drivers/crypto/caam/jr.c
index 2732f3a0725a..d11956bc358f 100644
--- a/drivers/crypto/caam/jr.c
+++ b/drivers/crypto/caam/jr.c
@@ -146,7 +146,6 @@ static int caam_jr_remove(struct platform_device *pdev)
 	ret = caam_jr_shutdown(jrdev);
 	if (ret)
 		dev_err(jrdev, "Failed to shut down job ring\n");
-	irq_dispose_mapping(jrpriv->irq);
 
 	return ret;
 }
@@ -487,6 +486,10 @@ static int caam_jr_init(struct device *dev)
 	return error;
 }
 
+static void caam_jr_irq_dispose_mapping(void *data)
+{
+	irq_dispose_mapping((int)data);
+}
 
 /*
  * Probe routine for each detected JobR subsystem.
@@ -542,12 +545,15 @@ static int caam_jr_probe(struct platform_device *pdev)
 		return -EINVAL;
 	}
 
+	error = devm_add_action_or_reset(jrdev, caam_jr_irq_dispose_mapping,
+					 (void *)jrpriv->irq);
+	if (error)
+		return error;
+
 	/* Now do the platform independent part */
 	error = caam_jr_init(jrdev); /* now turn on hardware */
-	if (error) {
-		irq_dispose_mapping(jrpriv->irq);
+	if (error)
 		return error;
-	}
 
 	jrpriv->dev = jrdev;
 	spin_lock(&driver_data.jr_alloc_lock);
-- 
2.21.0


^ permalink raw reply	[flat|nested] 43+ messages in thread

* [PATCH 05/12] crypto: caam - use devres to unmap memory
  2019-09-04  2:35 [PATCH 00/12] CAAM bugfixes, small improvements Andrey Smirnov
                   ` (3 preceding siblings ...)
  2019-09-04  2:35 ` [PATCH 04/12] crypto: caam - dispose of IRQ mapping only after IRQ is freed Andrey Smirnov
@ 2019-09-04  2:35 ` Andrey Smirnov
  2019-09-09 13:20   ` Horia Geanta
  2019-09-04  2:35 ` [PATCH 06/12] crypto: caam - use devres to remove debugfs Andrey Smirnov
                   ` (7 subsequent siblings)
  12 siblings, 1 reply; 43+ messages in thread
From: Andrey Smirnov @ 2019-09-04  2:35 UTC (permalink / raw)
  To: linux-crypto
  Cc: Andrey Smirnov, Chris Healy, Lucas Stach, Horia Geantă,
	Herbert Xu, Iuliana Prodan, linux-kernel

Use devres to unmap memory and drop corresponding iounmap() call.

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
---
 drivers/crypto/caam/ctrl.c | 28 +++++++++-------------------
 1 file changed, 9 insertions(+), 19 deletions(-)

diff --git a/drivers/crypto/caam/ctrl.c b/drivers/crypto/caam/ctrl.c
index db22777d59b4..35bf82d1bedc 100644
--- a/drivers/crypto/caam/ctrl.c
+++ b/drivers/crypto/caam/ctrl.c
@@ -308,11 +308,9 @@ static int caam_remove(struct platform_device *pdev)
 {
 	struct device *ctrldev;
 	struct caam_drv_private *ctrlpriv;
-	struct caam_ctrl __iomem *ctrl;
 
 	ctrldev = &pdev->dev;
 	ctrlpriv = dev_get_drvdata(ctrldev);
-	ctrl = (struct caam_ctrl __iomem *)ctrlpriv->ctrl;
 
 	/* Remove platform devices under the crypto node */
 	of_platform_depopulate(ctrldev);
@@ -334,9 +332,6 @@ static int caam_remove(struct platform_device *pdev)
 	debugfs_remove_recursive(ctrlpriv->dfs_root);
 #endif
 
-	/* Unmap controller region */
-	iounmap(ctrl);
-
 	return 0;
 }
 
@@ -611,10 +606,11 @@ static int caam_probe(struct platform_device *pdev)
 
 	/* Get configuration properties from device tree */
 	/* First, get register page */
-	ctrl = of_iomap(nprop, 0);
-	if (!ctrl) {
+	ctrl = devm_of_iomap(dev, nprop, 0, NULL);
+	ret = PTR_ERR_OR_ZERO(ctrl);
+	if (ret) {
 		dev_err(dev, "caam: of_iomap() failed\n");
-		return -ENOMEM;
+		return ret;
 	}
 
 	caam_little_end = !(bool)(rd_reg32(&ctrl->perfmon.status) &
@@ -632,22 +628,18 @@ static int caam_probe(struct platform_device *pdev)
 	if (ctrlpriv->qi_present && !caam_dpaa2) {
 		ret = qman_is_probed();
 		if (!ret) {
-			ret = -EPROBE_DEFER;
-			goto iounmap_ctrl;
+			return -EPROBE_DEFER;
 		} else if (ret < 0) {
 			dev_err(dev, "failing probe due to qman probe error\n");
-			ret = -ENODEV;
-			goto iounmap_ctrl;
+			return -ENODEV;
 		}
 
 		ret = qman_portals_probed();
 		if (!ret) {
-			ret = -EPROBE_DEFER;
-			goto iounmap_ctrl;
+			return -EPROBE_DEFER;
 		} else if (ret < 0) {
 			dev_err(dev, "failing probe due to qman portals probe error\n");
-			ret = -ENODEV;
-			goto iounmap_ctrl;
+			return -ENODEV;
 		}
 	}
 #endif
@@ -722,7 +714,7 @@ static int caam_probe(struct platform_device *pdev)
 	ret = dma_set_mask_and_coherent(dev, caam_get_dma_mask(dev));
 	if (ret) {
 		dev_err(dev, "dma_set_mask_and_coherent failed (%d)\n", ret);
-		goto iounmap_ctrl;
+		return ret;
 	}
 
 	ctrlpriv->era = caam_get_era(ctrl);
@@ -927,8 +919,6 @@ static int caam_probe(struct platform_device *pdev)
 	if (ctrlpriv->qi_init)
 		caam_qi_shutdown(dev);
 #endif
-iounmap_ctrl:
-	iounmap(ctrl);
 	return ret;
 }
 
-- 
2.21.0


^ permalink raw reply	[flat|nested] 43+ messages in thread

* [PATCH 06/12] crypto: caam - use devres to remove debugfs
  2019-09-04  2:35 [PATCH 00/12] CAAM bugfixes, small improvements Andrey Smirnov
                   ` (4 preceding siblings ...)
  2019-09-04  2:35 ` [PATCH 05/12] crypto: caam - use devres to unmap memory Andrey Smirnov
@ 2019-09-04  2:35 ` Andrey Smirnov
  2019-09-09 13:25   ` Horia Geanta
  2019-09-04  2:35 ` [PATCH 07/12] crypto: caam - use devres to de-initialize the RNG Andrey Smirnov
                   ` (6 subsequent siblings)
  12 siblings, 1 reply; 43+ messages in thread
From: Andrey Smirnov @ 2019-09-04  2:35 UTC (permalink / raw)
  To: linux-crypto
  Cc: Andrey Smirnov, Chris Healy, Lucas Stach, Horia Geantă,
	Herbert Xu, Iuliana Prodan, linux-kernel

Use devres to remove debugfs and drop corresponding
debugfs_remove_recursive() call.

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
---
 drivers/crypto/caam/ctrl.c   | 21 ++++++++++++++-------
 drivers/crypto/caam/intern.h |  1 -
 2 files changed, 14 insertions(+), 8 deletions(-)

diff --git a/drivers/crypto/caam/ctrl.c b/drivers/crypto/caam/ctrl.c
index 35bf82d1bedc..254963498abc 100644
--- a/drivers/crypto/caam/ctrl.c
+++ b/drivers/crypto/caam/ctrl.c
@@ -327,11 +327,6 @@ static int caam_remove(struct platform_device *pdev)
 	if (!ctrlpriv->mc_en && ctrlpriv->rng4_sh_init)
 		deinstantiate_rng(ctrldev, ctrlpriv->rng4_sh_init);
 
-	/* Shut down debug views */
-#ifdef CONFIG_DEBUG_FS
-	debugfs_remove_recursive(ctrlpriv->dfs_root);
-#endif
-
 	return 0;
 }
 
@@ -563,6 +558,13 @@ static int init_clocks(struct device *dev, const struct caam_imx_data *data)
 	return devm_add_action_or_reset(dev, disable_clocks, ctrlpriv);
 }
 
+#ifdef CONFIG_DEBUG_FS
+static void caam_remove_debugfs(void *root)
+{
+	debugfs_remove_recursive(root);
+}
+#endif
+
 /* Probe routine for CAAM top (controller) level */
 static int caam_probe(struct platform_device *pdev)
 {
@@ -575,6 +577,7 @@ static int caam_probe(struct platform_device *pdev)
 	struct caam_drv_private *ctrlpriv;
 #ifdef CONFIG_DEBUG_FS
 	struct caam_perfmon *perfmon;
+	struct dentry *dfs_root;
 #endif
 	u32 scfgr, comp_params;
 	u8 rng_vid;
@@ -728,8 +731,12 @@ static int caam_probe(struct platform_device *pdev)
 	 */
 	perfmon = (struct caam_perfmon __force *)&ctrl->perfmon;
 
-	ctrlpriv->dfs_root = debugfs_create_dir(dev_name(dev), NULL);
-	ctrlpriv->ctl = debugfs_create_dir("ctl", ctrlpriv->dfs_root);
+	dfs_root = debugfs_create_dir(dev_name(dev), NULL);
+	ret = devm_add_action_or_reset(dev, caam_remove_debugfs, dfs_root);
+	if (ret)
+		return ret;
+
+	ctrlpriv->ctl = debugfs_create_dir("ctl", dfs_root);
 #endif
 
 	/* Check to see if (DPAA 1.x) QI present. If so, enable */
diff --git a/drivers/crypto/caam/intern.h b/drivers/crypto/caam/intern.h
index 731b06becd9c..359eb76d1259 100644
--- a/drivers/crypto/caam/intern.h
+++ b/drivers/crypto/caam/intern.h
@@ -102,7 +102,6 @@ struct caam_drv_private {
 	 * variables at runtime.
 	 */
 #ifdef CONFIG_DEBUG_FS
-	struct dentry *dfs_root;
 	struct dentry *ctl; /* controller dir */
 	struct debugfs_blob_wrapper ctl_kek_wrap, ctl_tkek_wrap, ctl_tdsk_wrap;
 #endif
-- 
2.21.0


^ permalink raw reply	[flat|nested] 43+ messages in thread

* [PATCH 07/12] crypto: caam - use devres to de-initialize the RNG
  2019-09-04  2:35 [PATCH 00/12] CAAM bugfixes, small improvements Andrey Smirnov
                   ` (5 preceding siblings ...)
  2019-09-04  2:35 ` [PATCH 06/12] crypto: caam - use devres to remove debugfs Andrey Smirnov
@ 2019-09-04  2:35 ` Andrey Smirnov
  2019-09-09 15:39   ` Horia Geanta
  2019-09-04  2:35 ` [PATCH 08/12] crypto: caam - use devres to de-initialize QI Andrey Smirnov
                   ` (5 subsequent siblings)
  12 siblings, 1 reply; 43+ messages in thread
From: Andrey Smirnov @ 2019-09-04  2:35 UTC (permalink / raw)
  To: linux-crypto
  Cc: Andrey Smirnov, Chris Healy, Lucas Stach, Horia Geantă,
	Herbert Xu, Iuliana Prodan, linux-kernel

Use devres to de-initialize the RNG and drop explicit de-initialization
code in caam_remove().

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
---
 drivers/crypto/caam/ctrl.c | 129 ++++++++++++++++++++-----------------
 1 file changed, 70 insertions(+), 59 deletions(-)

diff --git a/drivers/crypto/caam/ctrl.c b/drivers/crypto/caam/ctrl.c
index 254963498abc..25f8f76551a5 100644
--- a/drivers/crypto/caam/ctrl.c
+++ b/drivers/crypto/caam/ctrl.c
@@ -175,6 +175,73 @@ static inline int run_descriptor_deco0(struct device *ctrldev, u32 *desc,
 	return 0;
 }
 
+/*
+ * deinstantiate_rng - builds and executes a descriptor on DECO0,
+ *		       which deinitializes the RNG block.
+ * @ctrldev - pointer to device
+ * @state_handle_mask - bitmask containing the instantiation status
+ *			for the RNG4 state handles which exist in
+ *			the RNG4 block: 1 if it's been instantiated
+ *
+ * Return: - 0 if no error occurred
+ *	   - -ENOMEM if there isn't enough memory to allocate the descriptor
+ *	   - -ENODEV if DECO0 couldn't be acquired
+ *	   - -EAGAIN if an error occurred when executing the descriptor
+ */
+static int deinstantiate_rng(struct device *ctrldev, int state_handle_mask)
+{
+	u32 *desc, status;
+	int sh_idx, ret = 0;
+
+	desc = kmalloc(CAAM_CMD_SZ * 3, GFP_KERNEL);
+	if (!desc)
+		return -ENOMEM;
+
+	for (sh_idx = 0; sh_idx < RNG4_MAX_HANDLES; sh_idx++) {
+		/*
+		 * If the corresponding bit is set, then it means the state
+		 * handle was initialized by us, and thus it needs to be
+		 * deinitialized as well
+		 */
+		if ((1 << sh_idx) & state_handle_mask) {
+			/*
+			 * Create the descriptor for deinstantating this state
+			 * handle
+			 */
+			build_deinstantiation_desc(desc, sh_idx);
+
+			/* Try to run it through DECO0 */
+			ret = run_descriptor_deco0(ctrldev, desc, &status);
+
+			if (ret ||
+			    (status && status != JRSTA_SSRC_JUMP_HALT_CC)) {
+				dev_err(ctrldev,
+					"Failed to deinstantiate RNG4 SH%d\n",
+					sh_idx);
+				break;
+			}
+			dev_info(ctrldev, "Deinstantiated RNG4 SH%d\n", sh_idx);
+		}
+	}
+
+	kfree(desc);
+
+	return ret;
+}
+
+static void devm_deinstantiate_rng(void *data)
+{
+	struct device *ctrldev = data;
+	struct caam_drv_private *ctrlpriv = dev_get_drvdata(ctrldev);
+
+	/*
+	 * De-initialize RNG state handles initialized by this driver.
+	 * In case of SoCs with Management Complex, RNG is managed by MC f/w.
+	 */
+	if (ctrlpriv->rng4_sh_init)
+		deinstantiate_rng(ctrldev, ctrlpriv->rng4_sh_init);
+}
+
 /*
  * instantiate_rng - builds and executes a descriptor on DECO0,
  *		     which initializes the RNG block.
@@ -247,60 +314,11 @@ static int instantiate_rng(struct device *ctrldev, int state_handle_mask,
 
 	kfree(desc);
 
-	return ret;
-}
-
-/*
- * deinstantiate_rng - builds and executes a descriptor on DECO0,
- *		       which deinitializes the RNG block.
- * @ctrldev - pointer to device
- * @state_handle_mask - bitmask containing the instantiation status
- *			for the RNG4 state handles which exist in
- *			the RNG4 block: 1 if it's been instantiated
- *
- * Return: - 0 if no error occurred
- *	   - -ENOMEM if there isn't enough memory to allocate the descriptor
- *	   - -ENODEV if DECO0 couldn't be acquired
- *	   - -EAGAIN if an error occurred when executing the descriptor
- */
-static int deinstantiate_rng(struct device *ctrldev, int state_handle_mask)
-{
-	u32 *desc, status;
-	int sh_idx, ret = 0;
-
-	desc = kmalloc(CAAM_CMD_SZ * 3, GFP_KERNEL);
-	if (!desc)
-		return -ENOMEM;
-
-	for (sh_idx = 0; sh_idx < RNG4_MAX_HANDLES; sh_idx++) {
-		/*
-		 * If the corresponding bit is set, then it means the state
-		 * handle was initialized by us, and thus it needs to be
-		 * deinitialized as well
-		 */
-		if ((1 << sh_idx) & state_handle_mask) {
-			/*
-			 * Create the descriptor for deinstantating this state
-			 * handle
-			 */
-			build_deinstantiation_desc(desc, sh_idx);
-
-			/* Try to run it through DECO0 */
-			ret = run_descriptor_deco0(ctrldev, desc, &status);
-
-			if (ret ||
-			    (status && status != JRSTA_SSRC_JUMP_HALT_CC)) {
-				dev_err(ctrldev,
-					"Failed to deinstantiate RNG4 SH%d\n",
-					sh_idx);
-				break;
-			}
-			dev_info(ctrldev, "Deinstantiated RNG4 SH%d\n", sh_idx);
-		}
+	if (!ret) {
+		ret = devm_add_action_or_reset(ctrldev, devm_deinstantiate_rng,
+					       ctrldev);
 	}
 
-	kfree(desc);
-
 	return ret;
 }
 
@@ -320,13 +338,6 @@ static int caam_remove(struct platform_device *pdev)
 		caam_qi_shutdown(ctrldev);
 #endif
 
-	/*
-	 * De-initialize RNG state handles initialized by this driver.
-	 * In case of SoCs with Management Complex, RNG is managed by MC f/w.
-	 */
-	if (!ctrlpriv->mc_en && ctrlpriv->rng4_sh_init)
-		deinstantiate_rng(ctrldev, ctrlpriv->rng4_sh_init);
-
 	return 0;
 }
 
-- 
2.21.0


^ permalink raw reply	[flat|nested] 43+ messages in thread

* [PATCH 08/12] crypto: caam - use devres to de-initialize QI
  2019-09-04  2:35 [PATCH 00/12] CAAM bugfixes, small improvements Andrey Smirnov
                   ` (6 preceding siblings ...)
  2019-09-04  2:35 ` [PATCH 07/12] crypto: caam - use devres to de-initialize the RNG Andrey Smirnov
@ 2019-09-04  2:35 ` Andrey Smirnov
  2019-09-20 15:10   ` Horia Geanta
  2019-09-04  2:35 ` [PATCH 09/12] crypto: caam - user devres to populate platform devices Andrey Smirnov
                   ` (4 subsequent siblings)
  12 siblings, 1 reply; 43+ messages in thread
From: Andrey Smirnov @ 2019-09-04  2:35 UTC (permalink / raw)
  To: linux-crypto
  Cc: Andrey Smirnov, Chris Healy, Lucas Stach, Horia Geantă,
	Herbert Xu, Iuliana Prodan, linux-kernel

Use devres to de-initialize the QI and drop explicit de-initialization
code in caam_remove().

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
---
 drivers/crypto/caam/ctrl.c   | 14 +-------------
 drivers/crypto/caam/intern.h |  3 ---
 drivers/crypto/caam/qi.c     |  8 ++++++--
 drivers/crypto/caam/qi.h     |  1 -
 4 files changed, 7 insertions(+), 19 deletions(-)

diff --git a/drivers/crypto/caam/ctrl.c b/drivers/crypto/caam/ctrl.c
index 25f8f76551a5..37cd04f8ddb1 100644
--- a/drivers/crypto/caam/ctrl.c
+++ b/drivers/crypto/caam/ctrl.c
@@ -333,11 +333,6 @@ static int caam_remove(struct platform_device *pdev)
 	/* Remove platform devices under the crypto node */
 	of_platform_depopulate(ctrldev);
 
-#ifdef CONFIG_CAAM_QI
-	if (ctrlpriv->qi_init)
-		caam_qi_shutdown(ctrldev);
-#endif
-
 	return 0;
 }
 
@@ -770,7 +765,7 @@ static int caam_probe(struct platform_device *pdev)
 	ret = of_platform_populate(nprop, caam_match, NULL, dev);
 	if (ret) {
 		dev_err(dev, "JR platform devices creation error\n");
-		goto shutdown_qi;
+		return ret;
 	}
 
 	ring = 0;
@@ -931,13 +926,6 @@ static int caam_probe(struct platform_device *pdev)
 caam_remove:
 	caam_remove(pdev);
 	return ret;
-
-shutdown_qi:
-#ifdef CONFIG_CAAM_QI
-	if (ctrlpriv->qi_init)
-		caam_qi_shutdown(dev);
-#endif
-	return ret;
 }
 
 static struct platform_driver caam_driver = {
diff --git a/drivers/crypto/caam/intern.h b/drivers/crypto/caam/intern.h
index 359eb76d1259..c7c10c90464b 100644
--- a/drivers/crypto/caam/intern.h
+++ b/drivers/crypto/caam/intern.h
@@ -81,9 +81,6 @@ struct caam_drv_private {
 	 */
 	u8 total_jobrs;		/* Total Job Rings in device */
 	u8 qi_present;		/* Nonzero if QI present in device */
-#ifdef CONFIG_CAAM_QI
-	u8 qi_init;		/* Nonzero if QI has been initialized */
-#endif
 	u8 mc_en;		/* Nonzero if MC f/w is active */
 	int secvio_irq;		/* Security violation interrupt number */
 	int virt_en;		/* Virtualization enabled in CAAM */
diff --git a/drivers/crypto/caam/qi.c b/drivers/crypto/caam/qi.c
index 378f627e1d64..dacf2fa4aa8e 100644
--- a/drivers/crypto/caam/qi.c
+++ b/drivers/crypto/caam/qi.c
@@ -500,9 +500,10 @@ void caam_drv_ctx_rel(struct caam_drv_ctx *drv_ctx)
 }
 EXPORT_SYMBOL(caam_drv_ctx_rel);
 
-void caam_qi_shutdown(struct device *qidev)
+static void caam_qi_shutdown(void *data)
 {
 	int i;
+	struct device *qidev = data;
 	struct caam_qi_priv *priv = &qipriv;
 	const cpumask_t *cpus = qman_affine_cpus();
 
@@ -761,7 +762,10 @@ int caam_qi_init(struct platform_device *caam_pdev)
 			    &times_congested, &caam_fops_u64_ro);
 #endif
 
-	ctrlpriv->qi_init = 1;
+	err = devm_add_action_or_reset(qidev, caam_qi_shutdown, ctrlpriv);
+	if (err)
+		return err;
+
 	dev_info(qidev, "Linux CAAM Queue I/F driver initialised\n");
 	return 0;
 }
diff --git a/drivers/crypto/caam/qi.h b/drivers/crypto/caam/qi.h
index db0549549e3b..848958951f68 100644
--- a/drivers/crypto/caam/qi.h
+++ b/drivers/crypto/caam/qi.h
@@ -147,7 +147,6 @@ int caam_drv_ctx_update(struct caam_drv_ctx *drv_ctx, u32 *sh_desc);
 void caam_drv_ctx_rel(struct caam_drv_ctx *drv_ctx);
 
 int caam_qi_init(struct platform_device *pdev);
-void caam_qi_shutdown(struct device *dev);
 
 /**
  * qi_cache_alloc - Allocate buffers from CAAM-QI cache
-- 
2.21.0


^ permalink raw reply	[flat|nested] 43+ messages in thread

* [PATCH 09/12] crypto: caam - user devres to populate platform devices
  2019-09-04  2:35 [PATCH 00/12] CAAM bugfixes, small improvements Andrey Smirnov
                   ` (7 preceding siblings ...)
  2019-09-04  2:35 ` [PATCH 08/12] crypto: caam - use devres to de-initialize QI Andrey Smirnov
@ 2019-09-04  2:35 ` Andrey Smirnov
  2019-09-20 15:29   ` Horia Geanta
  2019-09-04  2:35 ` [PATCH 10/12] crypto: caam - populate platform devices last Andrey Smirnov
                   ` (3 subsequent siblings)
  12 siblings, 1 reply; 43+ messages in thread
From: Andrey Smirnov @ 2019-09-04  2:35 UTC (permalink / raw)
  To: linux-crypto
  Cc: Andrey Smirnov, Chris Healy, Lucas Stach, Horia Geantă,
	Herbert Xu, Iuliana Prodan, linux-kernel

Use devres to de-initialize the RNG and drop explicit de-initialization
code in caam_remove().

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
---
 drivers/crypto/caam/ctrl.c | 26 +++-----------------------
 1 file changed, 3 insertions(+), 23 deletions(-)

diff --git a/drivers/crypto/caam/ctrl.c b/drivers/crypto/caam/ctrl.c
index 37cd04f8ddb1..4a84c2701311 100644
--- a/drivers/crypto/caam/ctrl.c
+++ b/drivers/crypto/caam/ctrl.c
@@ -322,20 +322,6 @@ static int instantiate_rng(struct device *ctrldev, int state_handle_mask,
 	return ret;
 }
 
-static int caam_remove(struct platform_device *pdev)
-{
-	struct device *ctrldev;
-	struct caam_drv_private *ctrlpriv;
-
-	ctrldev = &pdev->dev;
-	ctrlpriv = dev_get_drvdata(ctrldev);
-
-	/* Remove platform devices under the crypto node */
-	of_platform_depopulate(ctrldev);
-
-	return 0;
-}
-
 /*
  * kick_trng - sets the various parameters for enabling the initialization
  *	       of the RNG4 block in CAAM
@@ -762,7 +748,7 @@ static int caam_probe(struct platform_device *pdev)
 #endif
 	}
 
-	ret = of_platform_populate(nprop, caam_match, NULL, dev);
+	ret = devm_of_platform_populate(dev);
 	if (ret) {
 		dev_err(dev, "JR platform devices creation error\n");
 		return ret;
@@ -784,8 +770,7 @@ static int caam_probe(struct platform_device *pdev)
 	/* If no QI and no rings specified, quit and go home */
 	if ((!ctrlpriv->qi_present) && (!ctrlpriv->total_jobrs)) {
 		dev_err(dev, "no queues configured, terminating\n");
-		ret = -ENOMEM;
-		goto caam_remove;
+		return -ENOMEM;
 	}
 
 	if (ctrlpriv->era < 10)
@@ -848,7 +833,7 @@ static int caam_probe(struct platform_device *pdev)
 		} while ((ret == -EAGAIN) && (ent_delay < RTSDCTL_ENT_DLY_MAX));
 		if (ret) {
 			dev_err(dev, "failed to instantiate RNG");
-			goto caam_remove;
+			return ret;
 		}
 		/*
 		 * Set handles init'ed by this module as the complement of the
@@ -922,10 +907,6 @@ static int caam_probe(struct platform_device *pdev)
 			    &ctrlpriv->ctl_tdsk_wrap);
 #endif
 	return 0;
-
-caam_remove:
-	caam_remove(pdev);
-	return ret;
 }
 
 static struct platform_driver caam_driver = {
@@ -934,7 +915,6 @@ static struct platform_driver caam_driver = {
 		.of_match_table = caam_match,
 	},
 	.probe       = caam_probe,
-	.remove      = caam_remove,
 };
 
 module_platform_driver(caam_driver);
-- 
2.21.0


^ permalink raw reply	[flat|nested] 43+ messages in thread

* [PATCH 10/12] crypto: caam - populate platform devices last
  2019-09-04  2:35 [PATCH 00/12] CAAM bugfixes, small improvements Andrey Smirnov
                   ` (8 preceding siblings ...)
  2019-09-04  2:35 ` [PATCH 09/12] crypto: caam - user devres to populate platform devices Andrey Smirnov
@ 2019-09-04  2:35 ` Andrey Smirnov
  2019-09-20 15:35   ` Horia Geanta
  2019-09-04  2:35 ` [PATCH 11/12] crypto: caam - convert caamrng to platform device Andrey Smirnov
                   ` (2 subsequent siblings)
  12 siblings, 1 reply; 43+ messages in thread
From: Andrey Smirnov @ 2019-09-04  2:35 UTC (permalink / raw)
  To: linux-crypto
  Cc: Andrey Smirnov, Chris Healy, Lucas Stach, Horia Geantă,
	Herbert Xu, Iuliana Prodan, linux-kernel

Move the call to devm_of_platform_populate() at the end of
caam_probe(), so we won't try to add any child devices until all of
the initialization is finished successfully.

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
---
 drivers/crypto/caam/ctrl.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/crypto/caam/ctrl.c b/drivers/crypto/caam/ctrl.c
index 4a84c2701311..d101c28d3d1f 100644
--- a/drivers/crypto/caam/ctrl.c
+++ b/drivers/crypto/caam/ctrl.c
@@ -748,12 +748,6 @@ static int caam_probe(struct platform_device *pdev)
 #endif
 	}
 
-	ret = devm_of_platform_populate(dev);
-	if (ret) {
-		dev_err(dev, "JR platform devices creation error\n");
-		return ret;
-	}
-
 	ring = 0;
 	for_each_available_child_of_node(nprop, np)
 		if (of_device_is_compatible(np, "fsl,sec-v4.0-job-ring") ||
@@ -906,6 +900,13 @@ static int caam_probe(struct platform_device *pdev)
 	debugfs_create_blob("tdsk", S_IRUSR | S_IRGRP | S_IROTH, ctrlpriv->ctl,
 			    &ctrlpriv->ctl_tdsk_wrap);
 #endif
+
+	ret = devm_of_platform_populate(dev);
+	if (ret) {
+		dev_err(dev, "JR platform devices creation error\n");
+		return ret;
+	}
+
 	return 0;
 }
 
-- 
2.21.0


^ permalink raw reply	[flat|nested] 43+ messages in thread

* [PATCH 11/12] crypto: caam - convert caamrng to platform device
  2019-09-04  2:35 [PATCH 00/12] CAAM bugfixes, small improvements Andrey Smirnov
                   ` (9 preceding siblings ...)
  2019-09-04  2:35 ` [PATCH 10/12] crypto: caam - populate platform devices last Andrey Smirnov
@ 2019-09-04  2:35 ` Andrey Smirnov
  2019-09-11  9:35   ` [PATCH] crypto: caam - use the same jr for rng init/exit Horia Geanta
  2019-09-04  2:35 ` [PATCH 12/12] crypto: caam - change JR device ownership scheme Andrey Smirnov
  2019-09-09  7:53 ` [PATCH 00/12] CAAM bugfixes, small improvements Herbert Xu
  12 siblings, 1 reply; 43+ messages in thread
From: Andrey Smirnov @ 2019-09-04  2:35 UTC (permalink / raw)
  To: linux-crypto
  Cc: Andrey Smirnov, Chris Healy, Lucas Stach, Horia Geantă,
	Herbert Xu, Iuliana Prodan, linux-kernel

In order to allow caam_jr_enqueue() to lock underlying JR's
device (via device_lock(), see commit that follows) we need to make
sure that no code calls caam_jr_enqueue() as a part of caam_jr_probe()
to avoid a deadlock. Unfortunately, current implementation of caamrng
code does exactly that in caam_init_buf().

Another big problem with original caamrng initialization is a
circular reference in the form of:

 1. caam_rng_init() aquires JR via caam_jr_alloc(). Freed only by
    caam_rng_exit()

 2. caam_rng_exit() is only called by unregister_algs() once last JR
    is shut down

 3. caam_jr_remove() won't call unregister_algs() for last JR
    until tfm_count reaches zero, which can only happen via
    unregister_algs() -> caam_rng_exit() call chain.

To avoid all of that, convert caamrng code to a platform device driver
and extend caam_probe() to create corresponding platform device.

Additionally, this change also allows us to remove any access to
struct caam_drv_private in caamrng.c as well as simplify resource
ownership/deallocation via devres.

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
---
 drivers/crypto/caam/caamrng.c | 102 +++++++++++++---------------------
 drivers/crypto/caam/ctrl.c    |  39 +++++++++++++
 drivers/crypto/caam/jr.c      |  23 ++++++--
 3 files changed, 97 insertions(+), 67 deletions(-)

diff --git a/drivers/crypto/caam/caamrng.c b/drivers/crypto/caam/caamrng.c
index e8baacaabe07..f83ad34ac009 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 hwrng;
 	struct device *jrdev;
 	dma_addr_t sh_desc_dma;
 	u32 sh_desc[DESC_RNG_LEN];
@@ -78,14 +79,6 @@ 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 inline void rng_unmap_buf(struct device *jrdev, struct buf_data *bd)
 {
 	if (bd->addr)
@@ -143,7 +136,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 = (void *)rng->priv;
 	struct buf_data *bd = &ctx->bufs[ctx->current_buf];
 	int next_buf_idx, copied_idx;
 	int err;
@@ -247,15 +240,16 @@ static inline int rng_create_job_desc(struct caam_rng_ctx *ctx, int buf_id)
 static void caam_cleanup(struct hwrng *rng)
 {
 	int i;
+	struct caam_rng_ctx *ctx = (void *)rng->priv;
 	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);
+	rng_unmap_ctx(ctx);
 }
 
 static int caam_init_buf(struct caam_rng_ctx *ctx, int buf_id)
@@ -274,12 +268,10 @@ static int caam_init_buf(struct caam_rng_ctx *ctx, int buf_id)
 	return 0;
 }
 
-static int caam_init_rng(struct caam_rng_ctx *ctx, struct device *jrdev)
+static int caam_init_rng(struct caam_rng_ctx *ctx)
 {
 	int err;
 
-	ctx->jrdev = jrdev;
-
 	err = rng_create_sh_desc(ctx);
 	if (err)
 		return err;
@@ -294,65 +286,49 @@ static int caam_init_rng(struct caam_rng_ctx *ctx, struct device *jrdev)
 	return caam_init_buf(ctx, 1);
 }
 
-static struct hwrng caam_rng = {
-	.name		= "rng-caam",
-	.cleanup	= caam_cleanup,
-	.read		= caam_read,
-};
-
-void caam_rng_exit(void)
+static void caamrng_jr_free(void *data)
 {
-	if (!init_done)
-		return;
-
-	caam_jr_free(rng_ctx->jrdev);
-	hwrng_unregister(&caam_rng);
-	kfree(rng_ctx);
+	caam_jr_free(data);
 }
 
-int caam_rng_init(struct device *ctrldev)
+static int caamrng_probe(struct platform_device *pdev)
 {
-	struct device *dev;
-	u32 rng_inst;
-	struct caam_drv_private *priv = dev_get_drvdata(ctrldev);
+	struct device *dev = &pdev->dev;
+	struct caam_rng_ctx *ctx;
 	int err;
-	init_done = false;
-
-	/* Check for an instantiated RNG before registration */
-	if (priv->era < 10)
-		rng_inst = (rd_reg32(&priv->ctrl->perfmon.cha_num_ls) &
-			    CHA_ID_LS_RNG_MASK) >> CHA_ID_LS_RNG_SHIFT;
-	else
-		rng_inst = rd_reg32(&priv->ctrl->vreg.rng) & CHA_VER_NUM_MASK;
 
-	if (!rng_inst)
-		return 0;
+	ctx = devm_kmalloc(dev, sizeof(*ctx), GFP_DMA | GFP_KERNEL);
+	if (!ctx)
+		return -ENOMEM;
 
-	dev = caam_jr_alloc();
-	if (IS_ERR(dev)) {
-		pr_err("Job Ring Device allocation for transform failed\n");
-		return PTR_ERR(dev);
-	}
-	rng_ctx = kmalloc(sizeof(*rng_ctx), GFP_DMA | GFP_KERNEL);
-	if (!rng_ctx) {
-		err = -ENOMEM;
-		goto free_caam_alloc;
+	ctx->jrdev = caam_jr_alloc();
+	err = PTR_ERR_OR_ZERO(ctx->jrdev);
+	if (err) {
+		dev_err(dev, "Job Ring Device allocation for transform failed\n");
+		return err;
 	}
-	err = caam_init_rng(rng_ctx, dev);
-	if (err)
-		goto free_rng_ctx;
 
-	dev_info(dev, "registering rng-caam\n");
+	err = devm_add_action_or_reset(dev, caamrng_jr_free, ctx->jrdev);
+	if (err)
+		return err;
 
-	err = hwrng_register(&caam_rng);
-	if (!err) {
-		init_done = true;
+	err = caam_init_rng(ctx);
+	if (err)
 		return err;
-	}
 
-free_rng_ctx:
-	kfree(rng_ctx);
-free_caam_alloc:
-	caam_jr_free(dev);
-	return err;
+	ctx->hwrng.cleanup = caam_cleanup;
+	ctx->hwrng.read    = caam_read;
+	ctx->hwrng.name    = "rng-caam";
+	ctx->hwrng.priv    = (unsigned long)ctx;
+
+	dev_info(dev, "registering rng-caam\n");
+
+	return devm_hwrng_register(dev, &ctx->hwrng);
 }
+
+struct platform_driver caamrng_driver = {
+	.probe = caamrng_probe,
+	.driver = {
+		.name = "caamrng",
+	},
+};
diff --git a/drivers/crypto/caam/ctrl.c b/drivers/crypto/caam/ctrl.c
index d101c28d3d1f..ce3d5817c443 100644
--- a/drivers/crypto/caam/ctrl.c
+++ b/drivers/crypto/caam/ctrl.c
@@ -557,6 +557,26 @@ static void caam_remove_debugfs(void *root)
 }
 #endif
 
+static void caam_platform_device_unregister(void *data)
+{
+	platform_device_unregister(data);
+}
+
+static int caam_platform_device_register(struct device *dev, const char *name)
+{
+	struct platform_device *pdev;
+	int ret;
+
+	pdev = platform_device_register_simple(name,  -1, NULL, 0);
+	ret = PTR_ERR_OR_ZERO(pdev);
+	if (ret)
+		return ret;
+
+	return devm_add_action_or_reset(dev,
+					caam_platform_device_unregister,
+					pdev);
+}
+
 /* Probe routine for CAAM top (controller) level */
 static int caam_probe(struct platform_device *pdev)
 {
@@ -907,6 +927,25 @@ static int caam_probe(struct platform_device *pdev)
 		return ret;
 	}
 
+	if (IS_ENABLED(CONFIG_CRYPTO_DEV_FSL_CAAM_RNG_API)) {
+		u32 rng_inst;
+
+		/* Check for an instantiated RNG before registration */
+		if (ctrlpriv->era < 10)
+			rng_inst = (rd_reg32(&ctrl->perfmon.cha_num_ls) &
+				    CHA_ID_LS_RNG_MASK) >> CHA_ID_LS_RNG_SHIFT;
+		else
+			rng_inst = rd_reg32(&ctrl->vreg.rng) &
+				CHA_VER_NUM_MASK;
+
+
+		if (rng_inst) {
+			ret = caam_platform_device_register(dev, "caamrng");
+			if (ret)
+				return ret;
+		}
+	}
+
 	return 0;
 }
 
diff --git a/drivers/crypto/caam/jr.c b/drivers/crypto/caam/jr.c
index d11956bc358f..47b389cb1c62 100644
--- a/drivers/crypto/caam/jr.c
+++ b/drivers/crypto/caam/jr.c
@@ -37,7 +37,6 @@ static void register_algs(struct device *dev)
 	caam_algapi_init(dev);
 	caam_algapi_hash_init(dev);
 	caam_pkc_init(dev);
-	caam_rng_init(dev);
 	caam_qi_algapi_init(dev);
 
 algs_unlock:
@@ -53,7 +52,6 @@ static void unregister_algs(void)
 
 	caam_qi_algapi_exit();
 
-	caam_rng_exit();
 	caam_pkc_exit();
 	caam_algapi_hash_exit();
 	caam_algapi_exit();
@@ -287,7 +285,7 @@ struct device *caam_jr_alloc(void)
 
 	if (list_empty(&driver_data.jr_list)) {
 		spin_unlock(&driver_data.jr_alloc_lock);
-		return ERR_PTR(-ENODEV);
+		return ERR_PTR(-EPROBE_DEFER);
 	}
 
 	list_for_each_entry(jrpriv, &driver_data.jr_list, list_node) {
@@ -587,15 +585,32 @@ static struct platform_driver caam_jr_driver = {
 	.remove      = caam_jr_remove,
 };
 
+extern struct platform_driver caamrng_driver;
+
 static int __init jr_driver_init(void)
 {
+	int ret;
+
 	spin_lock_init(&driver_data.jr_alloc_lock);
 	INIT_LIST_HEAD(&driver_data.jr_list);
-	return platform_driver_register(&caam_jr_driver);
+	ret = platform_driver_register(&caam_jr_driver);
+	if (ret)
+		return ret;
+
+	if (IS_ENABLED(CONFIG_CRYPTO_DEV_FSL_CAAM_RNG_API)) {
+		ret = platform_driver_register(&caamrng_driver);
+		if (ret) {
+			platform_driver_unregister(&caam_jr_driver);
+			return ret;
+		}
+	}
+
+	return 0;
 }
 
 static void __exit jr_driver_exit(void)
 {
+	platform_driver_unregister(&caamrng_driver);
 	platform_driver_unregister(&caam_jr_driver);
 }
 
-- 
2.21.0


^ permalink raw reply	[flat|nested] 43+ messages in thread

* [PATCH 12/12] crypto: caam - change JR device ownership scheme
  2019-09-04  2:35 [PATCH 00/12] CAAM bugfixes, small improvements Andrey Smirnov
                   ` (10 preceding siblings ...)
  2019-09-04  2:35 ` [PATCH 11/12] crypto: caam - convert caamrng to platform device Andrey Smirnov
@ 2019-09-04  2:35 ` Andrey Smirnov
  2019-09-13 19:16   ` Leonard Crestez
  2019-09-09  7:53 ` [PATCH 00/12] CAAM bugfixes, small improvements Herbert Xu
  12 siblings, 1 reply; 43+ messages in thread
From: Andrey Smirnov @ 2019-09-04  2:35 UTC (permalink / raw)
  To: linux-crypto
  Cc: Andrey Smirnov, Chris Healy, Lucas Stach, Horia Geantă,
	Herbert Xu, Iuliana Prodan, linux-kernel

Returning -EBUSY from platform device's .remove() callback won't stop
the removal process, so the code in caam_jr_remove() is not going to
have the desired effect of preventing JR from being removed.

In order to be able to deal with removal of the JR device, change the
code as follows:

  1. To make sure that underlying struct device remains valid for as
     long as we have a reference to it, add appropriate device
     refcount management to caam_jr_alloc() and caam_jr_free()

  2. To make sure that device removal doesn't happen in parallel to
      use using the device in caam_jr_enqueue() augment the latter to
      acquire/release device lock for the duration of the subroutine

  3. In order to handle the case when caam_jr_enqueue() is executed
     right after corresponding caam_jr_remove(), add code to check
     that driver data has not been set to NULL.

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
---
 drivers/crypto/caam/jr.c | 36 +++++++++++++++++++++++++-----------
 1 file changed, 25 insertions(+), 11 deletions(-)

diff --git a/drivers/crypto/caam/jr.c b/drivers/crypto/caam/jr.c
index 47b389cb1c62..8a30bbd7f2aa 100644
--- a/drivers/crypto/caam/jr.c
+++ b/drivers/crypto/caam/jr.c
@@ -124,14 +124,6 @@ static int caam_jr_remove(struct platform_device *pdev)
 	jrdev = &pdev->dev;
 	jrpriv = dev_get_drvdata(jrdev);
 
-	/*
-	 * Return EBUSY if job ring already allocated.
-	 */
-	if (atomic_read(&jrpriv->tfm_count)) {
-		dev_err(jrdev, "Device is busy\n");
-		return -EBUSY;
-	}
-
 	/* Unregister JR-based RNG & crypto algorithms */
 	unregister_algs();
 
@@ -300,7 +292,7 @@ struct device *caam_jr_alloc(void)
 
 	if (min_jrpriv) {
 		atomic_inc(&min_jrpriv->tfm_count);
-		dev = min_jrpriv->dev;
+		dev = get_device(min_jrpriv->dev);
 	}
 	spin_unlock(&driver_data.jr_alloc_lock);
 
@@ -318,13 +310,16 @@ void caam_jr_free(struct device *rdev)
 	struct caam_drv_private_jr *jrpriv = dev_get_drvdata(rdev);
 
 	atomic_dec(&jrpriv->tfm_count);
+	put_device(rdev);
 }
 EXPORT_SYMBOL(caam_jr_free);
 
 /**
  * caam_jr_enqueue() - Enqueue a job descriptor head. Returns 0 if OK,
  * -EBUSY if the queue is full, -EIO if it cannot map the caller's
- * descriptor.
+ * descriptor, -ENODEV if given device was removed and is no longer
+ * valid
+ *
  * @dev:  device of the job ring to be used. This device should have
  *        been assigned prior by caam_jr_register().
  * @desc: points to a job descriptor that execute our request. All
@@ -354,15 +349,32 @@ int caam_jr_enqueue(struct device *dev, u32 *desc,
 				u32 status, void *areq),
 		    void *areq)
 {
-	struct caam_drv_private_jr *jrp = dev_get_drvdata(dev);
+	struct caam_drv_private_jr *jrp;
 	struct caam_jrentry_info *head_entry;
 	int head, tail, desc_size;
 	dma_addr_t desc_dma;
 
+	/*
+	 * Lock the device to prevent it from being removed while we
+	 * are using it
+	 */
+	device_lock(dev);
+
+	/*
+	 * If driver data is NULL, it is very likely that this device
+	 * was removed already. Nothing we can do here but bail out.
+	 */
+	jrp = dev_get_drvdata(dev);
+	if (!jrp) {
+		device_unlock(dev);
+		return -ENODEV;
+	}
+
 	desc_size = (caam32_to_cpu(*desc) & HDR_JD_LENGTH_MASK) * sizeof(u32);
 	desc_dma = dma_map_single(dev, desc, desc_size, DMA_TO_DEVICE);
 	if (dma_mapping_error(dev, desc_dma)) {
 		dev_err(dev, "caam_jr_enqueue(): can't map jobdesc\n");
+		device_unlock(dev);
 		return -EIO;
 	}
 
@@ -375,6 +387,7 @@ int caam_jr_enqueue(struct device *dev, u32 *desc,
 	    CIRC_SPACE(head, tail, JOBR_DEPTH) <= 0) {
 		spin_unlock_bh(&jrp->inplock);
 		dma_unmap_single(dev, desc_dma, desc_size, DMA_TO_DEVICE);
+		device_unlock(dev);
 		return -EBUSY;
 	}
 
@@ -411,6 +424,7 @@ int caam_jr_enqueue(struct device *dev, u32 *desc,
 		jrp->inpring_avail = rd_reg32(&jrp->rregs->inpring_avail);
 
 	spin_unlock_bh(&jrp->inplock);
+	device_unlock(dev);
 
 	return 0;
 }
-- 
2.21.0


^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH 02/12] crypto: caam - use devres to unmap JR's registers
  2019-09-04  2:35 ` [PATCH 02/12] crypto: caam - use devres to unmap JR's registers Andrey Smirnov
@ 2019-09-04  2:43   ` Fabio Estevam
  2019-09-04  2:55     ` Andrey Smirnov
  2019-09-09 13:01   ` Horia Geanta
  1 sibling, 1 reply; 43+ messages in thread
From: Fabio Estevam @ 2019-09-04  2:43 UTC (permalink / raw)
  To: Andrey Smirnov
  Cc: open list:HARDWARE RANDOM NUMBER GENERATOR CORE, Chris Healy,
	Lucas Stach, Horia Geantă,
	Herbert Xu, Iuliana Prodan, linux-kernel

Hi Andrey,

On Tue, Sep 3, 2019 at 11:37 PM Andrey Smirnov <andrew.smirnov@gmail.com> wrote:
>
> Use devres to unmap memory and drop explicit de-initialization
> code.
>
> NOTE: There's no corresponding unmapping code in caam_jr_remove which
> seems like a resource leak.
>
> 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
> ---
>  drivers/crypto/caam/jr.c | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/crypto/caam/jr.c b/drivers/crypto/caam/jr.c
> index 417ad52615c6..7947d61a25cf 100644
> --- a/drivers/crypto/caam/jr.c
> +++ b/drivers/crypto/caam/jr.c
> @@ -498,6 +498,7 @@ static int caam_jr_probe(struct platform_device *pdev)
>         struct caam_job_ring __iomem *ctrl;
>         struct caam_drv_private_jr *jrpriv;
>         static int total_jobrs;
> +       struct resource *r;
>         int error;
>
>         jrdev = &pdev->dev;
> @@ -513,9 +514,15 @@ static int caam_jr_probe(struct platform_device *pdev)
>         nprop = pdev->dev.of_node;
>         /* Get configuration properties from device tree */
>         /* First, get register page */
> -       ctrl = of_iomap(nprop, 0);
> +       r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +       if (!r) {
> +               dev_err(jrdev, "platform_get_resource() failed\n");
> +               return -ENOMEM;
> +       }
> +
> +       ctrl = devm_ioremap(jrdev, r->start, resource_size(r));

It seems that using devm_platform_ioremap_resource() could make the
code even smaller.

Regards,

Fabio Estevam

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH 02/12] crypto: caam - use devres to unmap JR's registers
  2019-09-04  2:43   ` Fabio Estevam
@ 2019-09-04  2:55     ` Andrey Smirnov
  0 siblings, 0 replies; 43+ messages in thread
From: Andrey Smirnov @ 2019-09-04  2:55 UTC (permalink / raw)
  To: Fabio Estevam
  Cc: open list:HARDWARE RANDOM NUMBER GENERATOR CORE, Chris Healy,
	Lucas Stach, Horia Geantă,
	Herbert Xu, Iuliana Prodan, linux-kernel

On Tue, Sep 3, 2019 at 7:43 PM Fabio Estevam <festevam@gmail.com> wrote:
>
> Hi Andrey,
>
> On Tue, Sep 3, 2019 at 11:37 PM Andrey Smirnov <andrew.smirnov@gmail.com> wrote:
> >
> > Use devres to unmap memory and drop explicit de-initialization
> > code.
> >
> > NOTE: There's no corresponding unmapping code in caam_jr_remove which
> > seems like a resource leak.
> >
> > 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
> > ---
> >  drivers/crypto/caam/jr.c | 13 +++++++++----
> >  1 file changed, 9 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/crypto/caam/jr.c b/drivers/crypto/caam/jr.c
> > index 417ad52615c6..7947d61a25cf 100644
> > --- a/drivers/crypto/caam/jr.c
> > +++ b/drivers/crypto/caam/jr.c
> > @@ -498,6 +498,7 @@ static int caam_jr_probe(struct platform_device *pdev)
> >         struct caam_job_ring __iomem *ctrl;
> >         struct caam_drv_private_jr *jrpriv;
> >         static int total_jobrs;
> > +       struct resource *r;
> >         int error;
> >
> >         jrdev = &pdev->dev;
> > @@ -513,9 +514,15 @@ static int caam_jr_probe(struct platform_device *pdev)
> >         nprop = pdev->dev.of_node;
> >         /* Get configuration properties from device tree */
> >         /* First, get register page */
> > -       ctrl = of_iomap(nprop, 0);
> > +       r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +       if (!r) {
> > +               dev_err(jrdev, "platform_get_resource() failed\n");
> > +               return -ENOMEM;
> > +       }
> > +
> > +       ctrl = devm_ioremap(jrdev, r->start, resource_size(r));
>
> It seems that using devm_platform_ioremap_resource() could make the
> code even smaller.

Unfortunately that function would do devm_ioremap_resource() under the
hood and claim the ownership of the corresponding memory region.
That's going to create a conflict with devm_of_iomap() used in
"crypto: caam - use devres to unmap memory".

Thanks,
Andrey Smirnov

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH 01/12] crypto: caam - make sure clocks are enabled first
  2019-09-04  2:35 ` [PATCH 01/12] crypto: caam - make sure clocks are enabled first Andrey Smirnov
@ 2019-09-06 11:18   ` Horia Geanta
  2019-09-09  7:21     ` Herbert Xu
  0 siblings, 1 reply; 43+ messages in thread
From: Horia Geanta @ 2019-09-06 11:18 UTC (permalink / raw)
  To: Andrey Smirnov, linux-crypto, Herbert Xu
  Cc: Chris Healy, Lucas Stach, Iuliana Prodan, linux-kernel

On 9/4/2019 5:35 AM, Andrey Smirnov wrote:
> In order to access IP block's registers we need to enable appropriate
> clocks first, otherwise we are risking hanging the CPU.
> 
> The problem becomes very apparent when trying to use CAAM driver built
> as a kernel module. In that case caam_probe() gets called after
> clk_disable_unused() which means all of the necessary clocks are
> guaranteed to be disabled.
> 
> Coincidentally, this change also fixes iomap leak introduced by early
> return (instead of "goto iounmap_ctrl") in commit
> 41fc54afae70 ("crypto: caam - simplfy clock initialization")
> 
> Tested on ZII i.MX6Q+ RDU2
> 
> Fixes: 176435ad2ac7 ("crypto: caam - defer probing until QMan is available")
> Fixes: 41fc54afae70 ("crypto: caam - simplfy clock initialization")
> 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
Tested-by: Horia Geantã <horia.geanta@nxp.com>

Considering this is a boot hang, in case this does not make into v5.4
I would appreciate appending:
Cc: <stable@vger.kernel.org>

Thanks,
Horia


^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH 04/12] crypto: caam - dispose of IRQ mapping only after IRQ is freed
  2019-09-04  2:35 ` [PATCH 04/12] crypto: caam - dispose of IRQ mapping only after IRQ is freed Andrey Smirnov
@ 2019-09-06 12:26   ` Horia Geanta
  2019-09-09  7:46   ` crypto: caam - Cast to long first before pointer conversion Herbert Xu
  1 sibling, 0 replies; 43+ messages in thread
From: Horia Geanta @ 2019-09-06 12:26 UTC (permalink / raw)
  To: Andrey Smirnov, linux-crypto, Herbert Xu
  Cc: Chris Healy, Lucas Stach, Iuliana Prodan, linux-kernel

On 9/4/2019 5:35 AM, Andrey Smirnov wrote:
> With IRQ requesting being managed by devres we need to make sure that
> we dispose of IRQ mapping after and not before it is free'd (otherwise
> we'll end up with a warning from the kernel). To achieve that simply
> convert IRQ mapping to rely on devres as well.
> 
> Fixes: f314f12db65c ("crypto: caam - convert caam_jr_init() to use devres")
> 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: <stable@vger.kernel.org>
Reviewed-by: Horia Geantã <horia.geanta@nxp.com>

I'd prefer pushing patches 1 and 4 first, considering they are fixes
while the rest are optimizations.

Thanks,
Horia


^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH 03/12] crypto: caam - check irq_of_parse_and_map for errors
  2019-09-04  2:35 ` [PATCH 03/12] crypto: caam - check irq_of_parse_and_map for errors Andrey Smirnov
@ 2019-09-06 12:29   ` Horia Geanta
  0 siblings, 0 replies; 43+ messages in thread
From: Horia Geanta @ 2019-09-06 12:29 UTC (permalink / raw)
  To: Andrey Smirnov, linux-crypto
  Cc: Chris Healy, Lucas Stach, Herbert Xu, Iuliana Prodan, linux-kernel

On 9/4/2019 5:35 AM, Andrey Smirnov wrote:
> Irq_of_parse_and_map will return zero in case of error, so add a error
> check for that.
> 
> 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
Reviewed-by: Horia Geantã <horia.geanta@nxp.com>

Thanks,
Horia

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH 01/12] crypto: caam - make sure clocks are enabled first
  2019-09-06 11:18   ` Horia Geanta
@ 2019-09-09  7:21     ` Herbert Xu
  2019-09-09  7:22       ` Herbert Xu
  0 siblings, 1 reply; 43+ messages in thread
From: Herbert Xu @ 2019-09-09  7:21 UTC (permalink / raw)
  To: Horia Geanta
  Cc: Andrey Smirnov, linux-crypto, Chris Healy, Lucas Stach,
	Iuliana Prodan, linux-kernel

On Fri, Sep 06, 2019 at 11:18:19AM +0000, Horia Geanta wrote:
> On 9/4/2019 5:35 AM, Andrey Smirnov wrote:
> > In order to access IP block's registers we need to enable appropriate
> > clocks first, otherwise we are risking hanging the CPU.
> > 
> > The problem becomes very apparent when trying to use CAAM driver built
> > as a kernel module. In that case caam_probe() gets called after
> > clk_disable_unused() which means all of the necessary clocks are
> > guaranteed to be disabled.
> > 
> > Coincidentally, this change also fixes iomap leak introduced by early
> > return (instead of "goto iounmap_ctrl") in commit
> > 41fc54afae70 ("crypto: caam - simplfy clock initialization")
> > 
> > Tested on ZII i.MX6Q+ RDU2
> > 
> > Fixes: 176435ad2ac7 ("crypto: caam - defer probing until QMan is available")
> > Fixes: 41fc54afae70 ("crypto: caam - simplfy clock initialization")
> > 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
> Tested-by: Horia Geantă <horia.geanta@nxp.com>
> 
> Considering this is a boot hang, in case this does not make into v5.4
> I would appreciate appending:
> Cc: <stable@vger.kernel.org>

This patch does not apply against cryptodev or crypto.

Please rebase.

Thanks,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH 01/12] crypto: caam - make sure clocks are enabled first
  2019-09-09  7:21     ` Herbert Xu
@ 2019-09-09  7:22       ` Herbert Xu
  0 siblings, 0 replies; 43+ messages in thread
From: Herbert Xu @ 2019-09-09  7:22 UTC (permalink / raw)
  To: Horia Geanta
  Cc: Andrey Smirnov, linux-crypto, Chris Healy, Lucas Stach,
	Iuliana Prodan, linux-kernel

On Mon, Sep 09, 2019 at 05:21:55PM +1000, Herbert Xu wrote:
> On Fri, Sep 06, 2019 at 11:18:19AM +0000, Horia Geanta wrote:
> > On 9/4/2019 5:35 AM, Andrey Smirnov wrote:
> > > In order to access IP block's registers we need to enable appropriate
> > > clocks first, otherwise we are risking hanging the CPU.
> > > 
> > > The problem becomes very apparent when trying to use CAAM driver built
> > > as a kernel module. In that case caam_probe() gets called after
> > > clk_disable_unused() which means all of the necessary clocks are
> > > guaranteed to be disabled.
> > > 
> > > Coincidentally, this change also fixes iomap leak introduced by early
> > > return (instead of "goto iounmap_ctrl") in commit
> > > 41fc54afae70 ("crypto: caam - simplfy clock initialization")
> > > 
> > > Tested on ZII i.MX6Q+ RDU2
> > > 
> > > Fixes: 176435ad2ac7 ("crypto: caam - defer probing until QMan is available")
> > > Fixes: 41fc54afae70 ("crypto: caam - simplfy clock initialization")
> > > 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
> > Tested-by: Horia Geantă <horia.geanta@nxp.com>
> > 
> > Considering this is a boot hang, in case this does not make into v5.4
> > I would appreciate appending:
> > Cc: <stable@vger.kernel.org>
> 
> This patch does not apply against cryptodev or crypto.

Nevermind, I was trying to apply patch 4 on top of patch 1 which
is why it didn't work.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

^ permalink raw reply	[flat|nested] 43+ messages in thread

* crypto: caam - Cast to long first before pointer conversion
  2019-09-04  2:35 ` [PATCH 04/12] crypto: caam - dispose of IRQ mapping only after IRQ is freed Andrey Smirnov
  2019-09-06 12:26   ` Horia Geanta
@ 2019-09-09  7:46   ` Herbert Xu
  2019-09-09 11:06     ` Horia Geanta
  2019-09-09 13:55     ` [v2 PATCH] " Herbert Xu
  1 sibling, 2 replies; 43+ messages in thread
From: Herbert Xu @ 2019-09-09  7:46 UTC (permalink / raw)
  To: Andrey Smirnov
  Cc: linux-crypto, Chris Healy, Lucas Stach, Horia Geantă,
	Iuliana Prodan, linux-kernel

On Tue, Sep 03, 2019 at 07:35:07PM -0700, Andrey Smirnov wrote:
> With IRQ requesting being managed by devres we need to make sure that
> we dispose of IRQ mapping after and not before it is free'd (otherwise
> we'll end up with a warning from the kernel). To achieve that simply
> convert IRQ mapping to rely on devres as well.
> 
> Fixes: f314f12db65c ("crypto: caam - convert caam_jr_init() to use devres")
> 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
> ---
>  drivers/crypto/caam/jr.c | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)

I needed to apply this on top of it to shut up the compiler:

---8<---
While storing an int in a pointer is safe the compiler is not
happy about it.  So we need some extra casting in order to make
this warning free.

Fixes: 1d3f75bce123 ("crypto: caam - dispose of IRQ mapping only...")
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

diff --git a/drivers/crypto/caam/jr.c b/drivers/crypto/caam/jr.c
index 8a30bbd7f2aa..c5acd9e24e14 100644
--- a/drivers/crypto/caam/jr.c
+++ b/drivers/crypto/caam/jr.c
@@ -500,7 +500,7 @@ static int caam_jr_init(struct device *dev)
 
 static void caam_jr_irq_dispose_mapping(void *data)
 {
-	irq_dispose_mapping((int)data);
+	irq_dispose_mapping((unsigned long)data);
 }
 
 /*
@@ -558,7 +558,7 @@ static int caam_jr_probe(struct platform_device *pdev)
 	}
 
 	error = devm_add_action_or_reset(jrdev, caam_jr_irq_dispose_mapping,
-					 (void *)jrpriv->irq);
+					 (void *)(unsigned long)jrpriv->irq);
 	if (error)
 		return error;
 
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH 00/12] CAAM bugfixes, small improvements
  2019-09-04  2:35 [PATCH 00/12] CAAM bugfixes, small improvements Andrey Smirnov
                   ` (11 preceding siblings ...)
  2019-09-04  2:35 ` [PATCH 12/12] crypto: caam - change JR device ownership scheme Andrey Smirnov
@ 2019-09-09  7:53 ` Herbert Xu
  2019-09-09 12:07   ` Horia Geanta
  12 siblings, 1 reply; 43+ messages in thread
From: Herbert Xu @ 2019-09-09  7:53 UTC (permalink / raw)
  To: Andrey Smirnov
  Cc: linux-crypto, Chris Healy, Lucas Stach, Horia Geantă,
	Iuliana Prodan, linux-kernel

On Tue, Sep 03, 2019 at 07:35:03PM -0700, Andrey Smirnov wrote:
> Everyone:
> 
> This series bugfixes and small improvement I made while doing more
> testing of CAAM code:
> 
>  - "crypto: caam - make sure clocks are enabled first"
> 
>    fixes a recent regression (and, conincidentally a leak cause by one
>    of my i.MX8MQ patches)
> 
>  - "crypto: caam - use devres to unmap JR's registers"
>    "crypto: caam - check irq_of_parse_and_map for errors"
> 
>    are small improvements
> 
>  - "crypto: caam - dispose of IRQ mapping only after IRQ is freed"
> 
>    fixes a bug introduced by my i.MX8MQ series
> 
>  - "crypto: caam - use devres to unmap memory"
>    "crypto: caam - use devres to remove debugfs"
>    "crypto: caam - use devres to de-initialize the RNG"
>    "crypto: caam - use devres to de-initialize QI"
>    "crypto: caam - user devres to populate platform devices"
>    "crypto: caam - populate platform devices last"
> 
>    are devres conversions/small improvments
> 
>  - "crypto: caam - convert caamrng to platform device"
>    "crypto: caam - change JR device ownership scheme"
> 
>    are more of an RFC than proper fixes. I don't have a very high
>    confidence in those fixes, but I think they are a good conversation
>    stater about the best approach to fix those issues
> 
> Thanks,
> Andrey Smirnov
> 
> Andrey Smirnov (12):
>   crypto: caam - make sure clocks are enabled first
>   crypto: caam - use devres to unmap JR's registers
>   crypto: caam - check irq_of_parse_and_map for errors
>   crypto: caam - dispose of IRQ mapping only after IRQ is freed
>   crypto: caam - use devres to unmap memory
>   crypto: caam - use devres to remove debugfs
>   crypto: caam - use devres to de-initialize the RNG
>   crypto: caam - use devres to de-initialize QI
>   crypto: caam - user devres to populate platform devices
>   crypto: caam - populate platform devices last
>   crypto: caam - convert caamrng to platform device
>   crypto: caam - change JR device ownership scheme
> 
>  drivers/crypto/caam/caamrng.c | 102 +++++-------
>  drivers/crypto/caam/ctrl.c    | 294 ++++++++++++++++++----------------
>  drivers/crypto/caam/intern.h  |   4 -
>  drivers/crypto/caam/jr.c      |  90 ++++++++---
>  drivers/crypto/caam/qi.c      |   8 +-
>  drivers/crypto/caam/qi.h      |   1 -
>  6 files changed, 267 insertions(+), 232 deletions(-)

All applied.  Thanks.
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: crypto: caam - Cast to long first before pointer conversion
  2019-09-09  7:46   ` crypto: caam - Cast to long first before pointer conversion Herbert Xu
@ 2019-09-09 11:06     ` Horia Geanta
  2019-09-09 13:55     ` [v2 PATCH] " Herbert Xu
  1 sibling, 0 replies; 43+ messages in thread
From: Horia Geanta @ 2019-09-09 11:06 UTC (permalink / raw)
  To: Herbert Xu, Andrey Smirnov
  Cc: linux-crypto, Chris Healy, Lucas Stach, Iuliana Prodan, linux-kernel

On 9/9/2019 10:46 AM, Herbert Xu wrote:
> On Tue, Sep 03, 2019 at 07:35:07PM -0700, Andrey Smirnov wrote:
>> With IRQ requesting being managed by devres we need to make sure that
>> we dispose of IRQ mapping after and not before it is free'd (otherwise
>> we'll end up with a warning from the kernel). To achieve that simply
>> convert IRQ mapping to rely on devres as well.
>>
>> Fixes: f314f12db65c ("crypto: caam - convert caam_jr_init() to use devres")
>> 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
>> ---
>>  drivers/crypto/caam/jr.c | 14 ++++++++++----
>>  1 file changed, 10 insertions(+), 4 deletions(-)
> 
> I needed to apply this on top of it to shut up the compiler:
> 
> ---8<---
> While storing an int in a pointer is safe the compiler is not
> happy about it.  So we need some extra casting in order to make
> this warning free.
> 
> Fixes: 1d3f75bce123 ("crypto: caam - dispose of IRQ mapping only...")
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
> 
Thanks Herbert.

Indeed, this is needed for silencing compilation on ARM64
(while compiling for ARM works fine).

Reviewed-by: Horia Geantã <horia.geanta@nxp.com>
for the squashed patches.

Horia

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH 00/12] CAAM bugfixes, small improvements
  2019-09-09  7:53 ` [PATCH 00/12] CAAM bugfixes, small improvements Herbert Xu
@ 2019-09-09 12:07   ` Horia Geanta
  2019-09-09 12:52     ` Herbert Xu
  0 siblings, 1 reply; 43+ messages in thread
From: Horia Geanta @ 2019-09-09 12:07 UTC (permalink / raw)
  To: Herbert Xu, Andrey Smirnov
  Cc: linux-crypto, Chris Healy, Lucas Stach, Iuliana Prodan, linux-kernel

On 9/9/2019 10:53 AM, Herbert Xu wrote:
> On Tue, Sep 03, 2019 at 07:35:03PM -0700, Andrey Smirnov wrote:
>> Everyone:
>>
>> This series bugfixes and small improvement I made while doing more
>> testing of CAAM code:
>>
>>  - "crypto: caam - make sure clocks are enabled first"
>>
>>    fixes a recent regression (and, conincidentally a leak cause by one
>>    of my i.MX8MQ patches)
>>
>>  - "crypto: caam - use devres to unmap JR's registers"
>>    "crypto: caam - check irq_of_parse_and_map for errors"
>>
>>    are small improvements
>>
>>  - "crypto: caam - dispose of IRQ mapping only after IRQ is freed"
>>
>>    fixes a bug introduced by my i.MX8MQ series
>>
>>  - "crypto: caam - use devres to unmap memory"
>>    "crypto: caam - use devres to remove debugfs"
>>    "crypto: caam - use devres to de-initialize the RNG"
>>    "crypto: caam - use devres to de-initialize QI"
>>    "crypto: caam - user devres to populate platform devices"
>>    "crypto: caam - populate platform devices last"
>>
>>    are devres conversions/small improvments
>>
>>  - "crypto: caam - convert caamrng to platform device"
>>    "crypto: caam - change JR device ownership scheme"
>>
>>    are more of an RFC than proper fixes. I don't have a very high
>>    confidence in those fixes, but I think they are a good conversation
>>    stater about the best approach to fix those issues
>>
>> Thanks,
>> Andrey Smirnov
>>
>> Andrey Smirnov (12):
>>   crypto: caam - make sure clocks are enabled first
>>   crypto: caam - use devres to unmap JR's registers
>>   crypto: caam - check irq_of_parse_and_map for errors
>>   crypto: caam - dispose of IRQ mapping only after IRQ is freed
>>   crypto: caam - use devres to unmap memory
>>   crypto: caam - use devres to remove debugfs
>>   crypto: caam - use devres to de-initialize the RNG
>>   crypto: caam - use devres to de-initialize QI
>>   crypto: caam - user devres to populate platform devices
>>   crypto: caam - populate platform devices last
>>   crypto: caam - convert caamrng to platform device
>>   crypto: caam - change JR device ownership scheme
>>
>>  drivers/crypto/caam/caamrng.c | 102 +++++-------
>>  drivers/crypto/caam/ctrl.c    | 294 ++++++++++++++++++----------------
>>  drivers/crypto/caam/intern.h  |   4 -
>>  drivers/crypto/caam/jr.c      |  90 ++++++++---
>>  drivers/crypto/caam/qi.c      |   8 +-
>>  drivers/crypto/caam/qi.h      |   1 -
>>  6 files changed, 267 insertions(+), 232 deletions(-)
> 
> All applied.  Thanks.
> 
Why all?
I've ack-ed only 1 and 4.

Besides this, patches 11 and/or 12 break the functionality,
i.e. driver gets stuck during crypto self-tests.

Horia

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH 00/12] CAAM bugfixes, small improvements
  2019-09-09 12:07   ` Horia Geanta
@ 2019-09-09 12:52     ` Herbert Xu
  2019-09-09 13:26       ` Horia Geanta
  0 siblings, 1 reply; 43+ messages in thread
From: Herbert Xu @ 2019-09-09 12:52 UTC (permalink / raw)
  To: Horia Geanta
  Cc: Andrey Smirnov, linux-crypto, Chris Healy, Lucas Stach,
	Iuliana Prodan, linux-kernel

On Mon, Sep 09, 2019 at 12:07:17PM +0000, Horia Geanta wrote:
>
> Why all?
> I've ack-ed only 1 and 4.
> 
> Besides this, patches 11 and/or 12 break the functionality,
> i.e. driver gets stuck during crypto self-tests.

Should I back out 5-12 or everything but patch 1?

Patch 4 can't be applied without 2/3.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH 02/12] crypto: caam - use devres to unmap JR's registers
  2019-09-04  2:35 ` [PATCH 02/12] crypto: caam - use devres to unmap JR's registers Andrey Smirnov
  2019-09-04  2:43   ` Fabio Estevam
@ 2019-09-09 13:01   ` Horia Geanta
  1 sibling, 0 replies; 43+ messages in thread
From: Horia Geanta @ 2019-09-09 13:01 UTC (permalink / raw)
  To: Andrey Smirnov, linux-crypto
  Cc: Chris Healy, Lucas Stach, Herbert Xu, Iuliana Prodan, linux-kernel

On 9/4/2019 5:35 AM, Andrey Smirnov wrote:
> Use devres to unmap memory and drop explicit de-initialization
> code.
> 
> NOTE: There's no corresponding unmapping code in caam_jr_remove which
> seems like a resource leak.
> 
> 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
Reviewed-by: Horia Geantã <horia.geanta@nxp.com>

Thanks,
Horia

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH 05/12] crypto: caam - use devres to unmap memory
  2019-09-04  2:35 ` [PATCH 05/12] crypto: caam - use devres to unmap memory Andrey Smirnov
@ 2019-09-09 13:20   ` Horia Geanta
  0 siblings, 0 replies; 43+ messages in thread
From: Horia Geanta @ 2019-09-09 13:20 UTC (permalink / raw)
  To: Andrey Smirnov, linux-crypto
  Cc: Chris Healy, Lucas Stach, Herbert Xu, Iuliana Prodan, linux-kernel

On 9/4/2019 5:35 AM, Andrey Smirnov wrote:
> Use devres to unmap memory and drop corresponding iounmap() call.
> 
> 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
Reviewed-by: Horia Geantã <horia.geanta@nxp.com>

Thanks,
Horia

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH 06/12] crypto: caam - use devres to remove debugfs
  2019-09-04  2:35 ` [PATCH 06/12] crypto: caam - use devres to remove debugfs Andrey Smirnov
@ 2019-09-09 13:25   ` Horia Geanta
  0 siblings, 0 replies; 43+ messages in thread
From: Horia Geanta @ 2019-09-09 13:25 UTC (permalink / raw)
  To: Andrey Smirnov, linux-crypto
  Cc: Chris Healy, Lucas Stach, Herbert Xu, Iuliana Prodan, linux-kernel

On 9/4/2019 5:35 AM, Andrey Smirnov wrote:
> Use devres to remove debugfs and drop corresponding
> debugfs_remove_recursive() call.
> 
> 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
Reviewed-by: Horia Geantã <horia.geanta@nxp.com>

Thanks,
Horia

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH 00/12] CAAM bugfixes, small improvements
  2019-09-09 12:52     ` Herbert Xu
@ 2019-09-09 13:26       ` Horia Geanta
  2019-09-09 13:52         ` Herbert Xu
  0 siblings, 1 reply; 43+ messages in thread
From: Horia Geanta @ 2019-09-09 13:26 UTC (permalink / raw)
  To: Herbert Xu
  Cc: Andrey Smirnov, linux-crypto, Chris Healy, Lucas Stach,
	Iuliana Prodan, linux-kernel

On 9/9/2019 3:52 PM, Herbert Xu wrote:
> On Mon, Sep 09, 2019 at 12:07:17PM +0000, Horia Geanta wrote:
>>
>> Why all?
>> I've ack-ed only 1 and 4.
>>
>> Besides this, patches 11 and/or 12 break the functionality,
>> i.e. driver gets stuck during crypto self-tests.
> 
> Should I back out 5-12 or everything but patch 1?
> 
> Patch 4 can't be applied without 2/3.
> 
Let's go with patches 1-4, and revert 5-12.

Thanks,
Horia

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH 00/12] CAAM bugfixes, small improvements
  2019-09-09 13:26       ` Horia Geanta
@ 2019-09-09 13:52         ` Herbert Xu
  0 siblings, 0 replies; 43+ messages in thread
From: Herbert Xu @ 2019-09-09 13:52 UTC (permalink / raw)
  To: Horia Geanta
  Cc: Andrey Smirnov, linux-crypto, Chris Healy, Lucas Stach,
	Iuliana Prodan, linux-kernel

On Mon, Sep 09, 2019 at 01:26:31PM +0000, Horia Geanta wrote:
>
> Let's go with patches 1-4, and revert 5-12.

OK, done and pushed out.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

^ permalink raw reply	[flat|nested] 43+ messages in thread

* [v2 PATCH] crypto: caam - Cast to long first before pointer conversion
  2019-09-09  7:46   ` crypto: caam - Cast to long first before pointer conversion Herbert Xu
  2019-09-09 11:06     ` Horia Geanta
@ 2019-09-09 13:55     ` " Herbert Xu
  1 sibling, 0 replies; 43+ messages in thread
From: Herbert Xu @ 2019-09-09 13:55 UTC (permalink / raw)
  To: Andrey Smirnov
  Cc: linux-crypto, Chris Healy, Lucas Stach, Horia Geantă,
	Iuliana Prodan, linux-kernel

While storing an int in a pointer is safe the compiler is not
happy about it.  So we need some extra casting in order to make
this warning free.

Fixes: 1d3f75bce123 ("crypto: caam - dispose of IRQ mapping only...")
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
Reviewed-by: Horia Geantă <horia.geanta@nxp.com>

diff --git a/drivers/crypto/caam/jr.c b/drivers/crypto/caam/jr.c
index 8a30bbd7f2aa..c5acd9e24e14 100644
--- a/drivers/crypto/caam/jr.c
+++ b/drivers/crypto/caam/jr.c
@@ -500,7 +500,7 @@ static int caam_jr_init(struct device *dev)
 
 static void caam_jr_irq_dispose_mapping(void *data)
 {
-	irq_dispose_mapping((int)data);
+	irq_dispose_mapping((unsigned long)data);
 }
 
 /*
@@ -558,7 +558,7 @@ static int caam_jr_probe(struct platform_device *pdev)
 	}
 
 	error = devm_add_action_or_reset(jrdev, caam_jr_irq_dispose_mapping,
-					 (void *)jrpriv->irq);
+					 (void *)(unsigned long)jrpriv->irq);
 	if (error)
 		return error;
 
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH 07/12] crypto: caam - use devres to de-initialize the RNG
  2019-09-04  2:35 ` [PATCH 07/12] crypto: caam - use devres to de-initialize the RNG Andrey Smirnov
@ 2019-09-09 15:39   ` Horia Geanta
  2019-09-18  6:06     ` Andrey Smirnov
  0 siblings, 1 reply; 43+ messages in thread
From: Horia Geanta @ 2019-09-09 15:39 UTC (permalink / raw)
  To: Andrey Smirnov, linux-crypto
  Cc: Chris Healy, Lucas Stach, Herbert Xu, Iuliana Prodan, linux-kernel

On 9/4/2019 5:35 AM, Andrey Smirnov wrote:
> Use devres to de-initialize the RNG and drop explicit de-initialization
> code in caam_remove().
> 
> 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
> ---
>  drivers/crypto/caam/ctrl.c | 129 ++++++++++++++++++++-----------------
>  1 file changed, 70 insertions(+), 59 deletions(-)
> 
> diff --git a/drivers/crypto/caam/ctrl.c b/drivers/crypto/caam/ctrl.c
> index 254963498abc..25f8f76551a5 100644
> --- a/drivers/crypto/caam/ctrl.c
> +++ b/drivers/crypto/caam/ctrl.c
> @@ -175,6 +175,73 @@ static inline int run_descriptor_deco0(struct device *ctrldev, u32 *desc,
>  	return 0;
>  }
>  
> +/*
> + * deinstantiate_rng - builds and executes a descriptor on DECO0,
> + *		       which deinitializes the RNG block.
> + * @ctrldev - pointer to device
> + * @state_handle_mask - bitmask containing the instantiation status
> + *			for the RNG4 state handles which exist in
> + *			the RNG4 block: 1 if it's been instantiated
> + *
> + * Return: - 0 if no error occurred
> + *	   - -ENOMEM if there isn't enough memory to allocate the descriptor
> + *	   - -ENODEV if DECO0 couldn't be acquired
> + *	   - -EAGAIN if an error occurred when executing the descriptor
> + */
> +static int deinstantiate_rng(struct device *ctrldev, int state_handle_mask)
I assume this function is not modified, only moved further up
to avoid forward declaration.

> +	if (!ret) {
> +		ret = devm_add_action_or_reset(ctrldev, devm_deinstantiate_rng,
> +					       ctrldev);
>  	}
Braces not needed.

Is there any guidance wrt. when *not* to use devres?

Horia

^ permalink raw reply	[flat|nested] 43+ messages in thread

* [PATCH] crypto: caam - use the same jr for rng init/exit
  2019-09-04  2:35 ` [PATCH 11/12] crypto: caam - convert caamrng to platform device Andrey Smirnov
@ 2019-09-11  9:35   ` Horia Geanta
  2019-09-18  6:01     ` Andrey Smirnov
  0 siblings, 1 reply; 43+ messages in thread
From: Horia Geanta @ 2019-09-11  9:35 UTC (permalink / raw)
  To: Andrey Smirnov, linux-crypto
  Cc: Chris Healy, Lucas Stach, Herbert Xu, Iuliana Prodan, linux-kernel

On 9/4/2019 5:35 AM, Andrey Smirnov wrote:
> In order to allow caam_jr_enqueue() to lock underlying JR's
> device (via device_lock(), see commit that follows) we need to make
> sure that no code calls caam_jr_enqueue() as a part of caam_jr_probe()
> to avoid a deadlock. Unfortunately, current implementation of caamrng
> code does exactly that in caam_init_buf().
> 
> Another big problem with original caamrng initialization is a
> circular reference in the form of:
> 
>  1. caam_rng_init() aquires JR via caam_jr_alloc(). Freed only by
>     caam_rng_exit()
> 
>  2. caam_rng_exit() is only called by unregister_algs() once last JR
>     is shut down
> 
>  3. caam_jr_remove() won't call unregister_algs() for last JR
>     until tfm_count reaches zero, which can only happen via
>     unregister_algs() -> caam_rng_exit() call chain.
> 
> To avoid all of that, convert caamrng code to a platform device driver
> and extend caam_probe() to create corresponding platform device.
> 
> Additionally, this change also allows us to remove any access to
> struct caam_drv_private in caamrng.c as well as simplify resource
> ownership/deallocation via devres.
> 
I would avoid adding platform devices that don't have
corresponding DT nodes.

There's some generic advice here:
https://www.kernel.org/doc/html/latest/driver-api/driver-model/platform.html#legacy-drivers-device-probing

and there's also previous experience in caam driver:
6b175685b4a1 crypto: caam/qi - don't allocate an extra platform device

The issue in caamrng is actually that caam/jr driver (jr.c) tries to call
caam_rng_exit() on the last available jr device.
Instead, caam_rng_exit() must be called on the same jr device that
was used during caam_rng_init().

Otherwise, by the time:

void caam_rng_exit(void)
{
        if (!init_done)
                return;

        caam_jr_free(rng_ctx->jrdev);
	hwrng_unregister(&caam_rng);
[...]

is executed, rng_ctx->jrdev might have been removed.

This will cause an oops in caam_jr_free().
caam_cleanup() - .cleanup hwrng callback that is called when doing
hwrng_unregister() - also needs to be executed on that jr device.

The problem can be easily reproduced as follows.

If caamrng was initialized on jr0:
[...]
caam_jr 2101000.jr0: registering rng-caam
[...]

then by manually unbinding jr0 first, then jr1 (using i.MX6SX):
# echo -n "2101000.jr0" > /sys/bus/platform/drivers/caam_jr/
# echo -n "2102000.jr1" > /sys/bus/platform/drivers/caam_jr/

Unable to handle kernel NULL pointer dereference at virtual address 00000040
pgd = 572e14e7
[00000040] *pgd=be2e8831
Internal error: Oops: 17 [#1] SMP ARM
Modules linked in:
CPU: 0 PID: 629 Comm: sh Not tainted 5.3.0-rc1-00299-g8e2a2738e5d3-dirty #8
Hardware name: Freescale i.MX6 SoloX (Device Tree)
PC is at caam_jr_free+0xc/0x24
LR is at caam_rng_exit+0x20/0x3c
pc : [<c08aef20>]    lr : [<c08bab1c>]    psr: 200f0013
sp : e9669e68  ip : 00001488  fp : 00000000
r10: 00000000  r9 : e9669f80  r8 : e9284010
r7 : e872d410  r6 : e872d410  r5 : e872d400  r4 : c1aa7cd8
r3 : 00000000  r2 : 00000040  r1 : 00000000  r0 : e872d010
Flags: nzCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment none
Control: 10c5387d  Table: a969004a  DAC: 00000051
Process sh (pid: 629, stack limit = 0xfc1b6e94)
Stack: (0xe9669e68 to 0xe966a000)
9e60:                   e865c940 c08af7dc e872d410 e872d410 c13a9cec c06b223c
9e80: c06b2218 e872d410 e81a9410 c06b08dc c13806f0 0000000b c13a9cec c06aeaf8
9ea0: 0000000b 00000000 0000000b e9284000 e91189c0 c0318c3c 00000000 00000000
9ec0: e95ddbd0 e8843500 c1308b08 c6614c9f e8843500 00438340 e9668000 00000004
9ee0: 00000000 c0285e00 00000001 00000000 00000000 c0288a44 00000000 00000000
9f00: c1308b28 00000000 00000001 c130911c 00000001 c13e81d1 c0288a44 00000000
9f20: e8ed9800 c019df00 e8ed9a7c c028ac08 00000001 00000000 c0288a44 c6614c9f
9f40: c1308b08 0000000b 00438340 e9669f80 e8843500 00438340 e9668000 c028899c
9f60: e95ddbc0 c6614c9f e8843500 e8843500 c1308b08 0000000b 00438340 c0288bfc
9f80: 00000000 00000000 00000000 c6614c9f 0000000b 00438340 b6ef1da0 00000004
9fa0: c01011c4 c0101000 0000000b 00438340 00000001 00438340 0000000b 00000000
9fc0: 0000000b 00438340 b6ef1da0 00000004 00438340 0000000b 00000000 00000000
9fe0: 0000006c bea7f908 b6e19e58 b6e7325c 600f0010 00000001 00000000 00000000
[<c08aef20>] (caam_jr_free) from [<c08bab1c>] (caam_rng_exit+0x20/0x3c)
[<c08bab1c>] (caam_rng_exit) from [<c08af7dc>] (caam_jr_remove+0x38/0xc0)
[<c08af7dc>] (caam_jr_remove) from [<c06b223c>] (platform_drv_remove+0x24/0x3c)
[<c06b223c>] (platform_drv_remove) from [<c06b08dc>] (device_release_driver_internal+0xdc/0x1a0)
[<c06b08dc>] (device_release_driver_internal) from [<c06aeaf8>] (unbind_store+0x5c/0xcc)
[<c06aeaf8>] (unbind_store) from [<c0318c3c>] (kernfs_fop_write+0xfc/0x1e0)
[<c0318c3c>] (kernfs_fop_write) from [<c0285e00>] (__vfs_write+0x2c/0x1d0)
[<c0285e00>] (__vfs_write) from [<c028899c>] (vfs_write+0xa0/0x180)
[<c028899c>] (vfs_write) from [<c0288bfc>] (ksys_write+0x5c/0xdc)
[<c0288bfc>] (ksys_write) from [<c0101000>] (ret_fast_syscall+0x0/0x28)
Exception stack(0xe9669fa8 to 0xe9669ff0)
9fa0:                   0000000b 00438340 00000001 00438340 0000000b 00000000
9fc0: 0000000b 00438340 b6ef1da0 00000004 00438340 0000000b 00000000 00000000
9fe0: 0000006c bea7f908 b6e19e58 b6e7325c
Code: eaffff49 e5903040 e2832040 f5d2f000 (e1921f9f)

Thus, I'd say the following fix is needed:

-- >8 --

When caam_rng_exit() executes, the jr device that was used
during caam_rng_init() must be available.

This means that current scheme - where caam_rng_exit() is called
when last jr device is removed - is incorrect.
Instead, caam_rng_exit() has to run when the jr acquired
during caam_rng_init() is removed.

Fixes: 1b46c90c8e00 ("crypto: caam - convert top level drivers to libraries")
Signed-off-by: Horia Geantã <horia.geanta@nxp.com>

diff --git a/drivers/crypto/caam/caamrng.c b/drivers/crypto/caam/caamrng.c
index e8baacaabe07..ec40178fa688 100644
--- a/drivers/crypto/caam/caamrng.c
+++ b/drivers/crypto/caam/caamrng.c
@@ -300,9 +300,9 @@ static struct hwrng caam_rng = {
        .read           = caam_read,
 };

-void caam_rng_exit(void)
+void caam_rng_exit(struct device *jrdev)
 {
-       if (!init_done)
+       if (!init_done || jrdev != rng_ctx->jrdev)
                return;

        caam_jr_free(rng_ctx->jrdev);
diff --git a/drivers/crypto/caam/intern.h b/drivers/crypto/caam/intern.h
index 731b06becd9c..4795530203ad 100644
--- a/drivers/crypto/caam/intern.h
+++ b/drivers/crypto/caam/intern.h
@@ -165,7 +165,7 @@ 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);
+void caam_rng_exit(struct device *jrdev);

 #else

@@ -174,7 +174,7 @@ static inline int caam_rng_init(struct device *dev)
        return 0;
 }

-static inline void caam_rng_exit(void)
+static inline void caam_rng_exit(struct device *jrdev)
 {
 }

diff --git a/drivers/crypto/caam/jr.c b/drivers/crypto/caam/jr.c
index d11956bc358f..61aea11773a6 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();
@@ -126,6 +125,8 @@ static int caam_jr_remove(struct platform_device *pdev)
        jrdev = &pdev->dev;
        jrpriv = dev_get_drvdata(jrdev);

+       caam_rng_exit(jrdev);
+
        /*
         * Return EBUSY if job ring already allocated.
         */

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH 12/12] crypto: caam - change JR device ownership scheme
  2019-09-04  2:35 ` [PATCH 12/12] crypto: caam - change JR device ownership scheme Andrey Smirnov
@ 2019-09-13 19:16   ` Leonard Crestez
  2019-09-18  3:13     ` Andrey Smirnov
  0 siblings, 1 reply; 43+ messages in thread
From: Leonard Crestez @ 2019-09-13 19:16 UTC (permalink / raw)
  To: Andrey Smirnov, Horia Geanta
  Cc: linux-crypto, Chris Healy, Lucas Stach, Herbert Xu,
	Iuliana Prodan, linux-kernel, Abel Vesa

On 04.09.2019 05:35, Andrey Smirnov wrote:
> Returning -EBUSY from platform device's .remove() callback won't stop
> the removal process, so the code in caam_jr_remove() is not going to
> have the desired effect of preventing JR from being removed.
> 
> In order to be able to deal with removal of the JR device, change the
> code as follows:
> 
>    1. To make sure that underlying struct device remains valid for as
>       long as we have a reference to it, add appropriate device
>       refcount management to caam_jr_alloc() and caam_jr_free()
> 
>    2. To make sure that device removal doesn't happen in parallel to
>        use using the device in caam_jr_enqueue() augment the latter to
>        acquire/release device lock for the duration of the subroutine
> 
>    3. In order to handle the case when caam_jr_enqueue() is executed
>       right after corresponding caam_jr_remove(), add code to check
>       that driver data has not been set to NULL.

What happens if a driver is unbound while a transform is executing 
asynchronously?

Doing get_device and put_device in caam_jr_alloc and caam_jr_free 
doesn't protect you against an explicit unbind of caam_jr, that path 
doesn't care about device reference counts. For example on imx6qp:

# echo 2102000.jr1 > /sys/bus/platform/drivers/caam_jr/unbind

CPU: 2 PID: 561 Comm: bash Not tainted 5.3.0-rc7-next-20190904
Hardware name: Freescale i.MX6 Quad/DualLite (Device Tree)
Backtrace:
[<c010f8a4>] (dump_backtrace) from [<c010fc5c>] (show_stack+0x20/0x24)
[<c010fc3c>] (show_stack) from [<c0d21600>] (dump_stack+0xdc/0x114)
[<c0d21524>] (dump_stack) from [<c0955774>] (caam_jr_remove+0x24/0xf8)
[<c0955750>] (caam_jr_remove) from [<c06e2d98>] 
(platform_drv_remove+0x34/0x4c)
[<c06e2d64>] (platform_drv_remove) from [<c06e0b74>] 
(device_release_driver_internal+0xf8/0x1b0)
[<c06e0a7c>] (device_release_driver_internal) from [<c06e0c70>] 
(device_driver_detach+0x20/0x24)
[<c06e0c50>] (device_driver_detach) from [<c06de5a4>] 
(unbind_store+0x6c/0xe0)
[<c06de538>] (unbind_store) from [<c06dd950>] (drv_attr_store+0x30/0x3c)
[<c06dd920>] (drv_attr_store) from [<c0364c18>] (sysfs_kf_write+0x50/0x5c)
[<c0364bc8>] (sysfs_kf_write) from [<c0363e0c>] 
(kernfs_fop_write+0x10c/0x1f8)
[<c0363d00>] (kernfs_fop_write) from [<c02c3a30>] (__vfs_write+0x44/0x1d0)
[<c02c39ec>] (__vfs_write) from [<c02c68ec>] (vfs_write+0xb0/0x194)
[<c02c683c>] (vfs_write) from [<c02c6b7c>] (ksys_write+0x64/0xd8)
[<c02c6b18>] (ksys_write) from [<c02c6c08>] (sys_write+0x18/0x1c)
[<c02c6bf0>] (sys_write) from [<c0101000>] (ret_fast_syscall+0x0/0x28)

I think you need to do some form of slow wait loop in jrpriv until 
jrpriv->tfm_count reaches zero. Preventing new operations from starting 
would help but isn't strictly necessary for correctness.

> diff --git a/drivers/crypto/caam/jr.c b/drivers/crypto/caam/jr.c
> index 47b389cb1c62..8a30bbd7f2aa 100644
> --- a/drivers/crypto/caam/jr.c
> +++ b/drivers/crypto/caam/jr.c
> @@ -124,14 +124,6 @@ static int caam_jr_remove(struct platform_device *pdev)
>   	jrdev = &pdev->dev;
>   	jrpriv = dev_get_drvdata(jrdev);
>   
> -	/*
> -	 * Return EBUSY if job ring already allocated.
> -	 */
> -	if (atomic_read(&jrpriv->tfm_count)) {
> -		dev_err(jrdev, "Device is busy\n");
> -		return -EBUSY;
> -	}
> -
>   	/* Unregister JR-based RNG & crypto algorithms */
>   	unregister_algs();
>   
> @@ -300,7 +292,7 @@ struct device *caam_jr_alloc(void)
>   
>   	if (min_jrpriv) {
>   		atomic_inc(&min_jrpriv->tfm_count);
> -		dev = min_jrpriv->dev;
> +		dev = get_device(min_jrpriv->dev);
>   	}
>   	spin_unlock(&driver_data.jr_alloc_lock);
>   
> @@ -318,13 +310,16 @@ void caam_jr_free(struct device *rdev)
>   	struct caam_drv_private_jr *jrpriv = dev_get_drvdata(rdev);
>   
>   	atomic_dec(&jrpriv->tfm_count);
> +	put_device(rdev);
>   }
>   EXPORT_SYMBOL(caam_jr_free);
>   
>   /**
>    * caam_jr_enqueue() - Enqueue a job descriptor head. Returns 0 if OK,
>    * -EBUSY if the queue is full, -EIO if it cannot map the caller's
> - * descriptor.
> + * descriptor, -ENODEV if given device was removed and is no longer
> + * valid
> + *
>    * @dev:  device of the job ring to be used. This device should have
>    *        been assigned prior by caam_jr_register().
>    * @desc: points to a job descriptor that execute our request. All
> @@ -354,15 +349,32 @@ int caam_jr_enqueue(struct device *dev, u32 *desc,
>   				u32 status, void *areq),
>   		    void *areq)
>   {
> -	struct caam_drv_private_jr *jrp = dev_get_drvdata(dev);
> +	struct caam_drv_private_jr *jrp;
>   	struct caam_jrentry_info *head_entry;
>   	int head, tail, desc_size;
>   	dma_addr_t desc_dma;
>   
> +	/*
> +	 * Lock the device to prevent it from being removed while we
> +	 * are using it
> +	 */
> +	device_lock(dev);
> +
> +	/*
> +	 * If driver data is NULL, it is very likely that this device
> +	 * was removed already. Nothing we can do here but bail out.
> +	 */
> +	jrp = dev_get_drvdata(dev);
> +	if (!jrp) {
> +		device_unlock(dev);
> +		return -ENODEV;
> +	}
> +
>   	desc_size = (caam32_to_cpu(*desc) & HDR_JD_LENGTH_MASK) * sizeof(u32);
>   	desc_dma = dma_map_single(dev, desc, desc_size, DMA_TO_DEVICE);
>   	if (dma_mapping_error(dev, desc_dma)) {
>   		dev_err(dev, "caam_jr_enqueue(): can't map jobdesc\n");
> +		device_unlock(dev);
>   		return -EIO;
>   	}
>   
> @@ -375,6 +387,7 @@ int caam_jr_enqueue(struct device *dev, u32 *desc,
>   	    CIRC_SPACE(head, tail, JOBR_DEPTH) <= 0) {
>   		spin_unlock_bh(&jrp->inplock);
>   		dma_unmap_single(dev, desc_dma, desc_size, DMA_TO_DEVICE);
> +		device_unlock(dev);
>   		return -EBUSY;
>   	}
>   
> @@ -411,6 +424,7 @@ int caam_jr_enqueue(struct device *dev, u32 *desc,
>   		jrp->inpring_avail = rd_reg32(&jrp->rregs->inpring_avail);
>   
>   	spin_unlock_bh(&jrp->inplock);
> +	device_unlock(dev);
>   
>   	return 0;
>   }

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH 12/12] crypto: caam - change JR device ownership scheme
  2019-09-13 19:16   ` Leonard Crestez
@ 2019-09-18  3:13     ` Andrey Smirnov
  2019-09-19 11:19       ` Horia Geanta
  0 siblings, 1 reply; 43+ messages in thread
From: Andrey Smirnov @ 2019-09-18  3:13 UTC (permalink / raw)
  To: Leonard Crestez
  Cc: Horia Geanta, linux-crypto, Chris Healy, Lucas Stach, Herbert Xu,
	Iuliana Prodan, linux-kernel, Abel Vesa

On Fri, Sep 13, 2019 at 12:16 PM Leonard Crestez
<leonard.crestez@nxp.com> wrote:
>
> On 04.09.2019 05:35, Andrey Smirnov wrote:
> > Returning -EBUSY from platform device's .remove() callback won't stop
> > the removal process, so the code in caam_jr_remove() is not going to
> > have the desired effect of preventing JR from being removed.
> >
> > In order to be able to deal with removal of the JR device, change the
> > code as follows:
> >
> >    1. To make sure that underlying struct device remains valid for as
> >       long as we have a reference to it, add appropriate device
> >       refcount management to caam_jr_alloc() and caam_jr_free()
> >
> >    2. To make sure that device removal doesn't happen in parallel to
> >        use using the device in caam_jr_enqueue() augment the latter to
> >        acquire/release device lock for the duration of the subroutine
> >
> >    3. In order to handle the case when caam_jr_enqueue() is executed
> >       right after corresponding caam_jr_remove(), add code to check
> >       that driver data has not been set to NULL.
>
> What happens if a driver is unbound while a transform is executing
> asynchronously?
>

AFAICT caam_jr_shutdown() is going to disable JR's interrupt and kill
corresponding tasklet, so I think that transform will never finish. We
probably could handle that better by walking the list of unfinished
jobs and calling their callbacks with an appropriate error code.

> Doing get_device and put_device in caam_jr_alloc and caam_jr_free
> doesn't protect you against an explicit unbind of caam_jr, that path
> doesn't care about device reference counts. For example on imx6qp:
>

My thinking with get_device() and put_device() was to prevent struct
device * from being freed while we are using it. The intent with
explicit unbind was to protect against it by
device_lock()/device_unlock() as a well as check that device data is
not NULL. I think I did miss code to do that in caam_jr_free() before
accessing tfm_count.


> # echo 2102000.jr1 > /sys/bus/platform/drivers/caam_jr/unbind
>

A bit of a silly question, but is this with my patches applied or
against the vanilla driver?

> CPU: 2 PID: 561 Comm: bash Not tainted 5.3.0-rc7-next-20190904
> Hardware name: Freescale i.MX6 Quad/DualLite (Device Tree)
> Backtrace:
> [<c010f8a4>] (dump_backtrace) from [<c010fc5c>] (show_stack+0x20/0x24)
> [<c010fc3c>] (show_stack) from [<c0d21600>] (dump_stack+0xdc/0x114)
> [<c0d21524>] (dump_stack) from [<c0955774>] (caam_jr_remove+0x24/0xf8)
> [<c0955750>] (caam_jr_remove) from [<c06e2d98>]
> (platform_drv_remove+0x34/0x4c)
> [<c06e2d64>] (platform_drv_remove) from [<c06e0b74>]
> (device_release_driver_internal+0xf8/0x1b0)
> [<c06e0a7c>] (device_release_driver_internal) from [<c06e0c70>]
> (device_driver_detach+0x20/0x24)
> [<c06e0c50>] (device_driver_detach) from [<c06de5a4>]
> (unbind_store+0x6c/0xe0)
> [<c06de538>] (unbind_store) from [<c06dd950>] (drv_attr_store+0x30/0x3c)
> [<c06dd920>] (drv_attr_store) from [<c0364c18>] (sysfs_kf_write+0x50/0x5c)
> [<c0364bc8>] (sysfs_kf_write) from [<c0363e0c>]
> (kernfs_fop_write+0x10c/0x1f8)
> [<c0363d00>] (kernfs_fop_write) from [<c02c3a30>] (__vfs_write+0x44/0x1d0)
> [<c02c39ec>] (__vfs_write) from [<c02c68ec>] (vfs_write+0xb0/0x194)
> [<c02c683c>] (vfs_write) from [<c02c6b7c>] (ksys_write+0x64/0xd8)
> [<c02c6b18>] (ksys_write) from [<c02c6c08>] (sys_write+0x18/0x1c)
> [<c02c6bf0>] (sys_write) from [<c0101000>] (ret_fast_syscall+0x0/0x28)
>
> I think you need to do some form of slow wait loop in jrpriv until
> jrpriv->tfm_count reaches zero.

Hmm, what do we do if it never does? Why do you think it would be
better than cancelling all outstanding jobs and resetting the HW?

> Preventing new operations from starting
> would help but isn't strictly necessary for correctness.

Wouldn't it be strictly necessary if you want to wait for tfm_count
reaching zero?

Thanks,
Andrey Smirnov

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH] crypto: caam - use the same jr for rng init/exit
  2019-09-11  9:35   ` [PATCH] crypto: caam - use the same jr for rng init/exit Horia Geanta
@ 2019-09-18  6:01     ` Andrey Smirnov
  2019-09-20 15:50       ` Horia Geanta
  0 siblings, 1 reply; 43+ messages in thread
From: Andrey Smirnov @ 2019-09-18  6:01 UTC (permalink / raw)
  To: Horia Geanta
  Cc: linux-crypto, Chris Healy, Lucas Stach, Herbert Xu,
	Iuliana Prodan, linux-kernel

On Wed, Sep 11, 2019 at 2:35 AM Horia Geanta <horia.geanta@nxp.com> wrote:
>
> On 9/4/2019 5:35 AM, Andrey Smirnov wrote:
> > In order to allow caam_jr_enqueue() to lock underlying JR's
> > device (via device_lock(), see commit that follows) we need to make
> > sure that no code calls caam_jr_enqueue() as a part of caam_jr_probe()
> > to avoid a deadlock. Unfortunately, current implementation of caamrng
> > code does exactly that in caam_init_buf().
> >

I don't think your patch addresses the above, so it can probably be
cut out of the message.

> > Another big problem with original caamrng initialization is a
> > circular reference in the form of:
> >
> >  1. caam_rng_init() aquires JR via caam_jr_alloc(). Freed only by
> >     caam_rng_exit()
> >
> >  2. caam_rng_exit() is only called by unregister_algs() once last JR
> >     is shut down
> >
> >  3. caam_jr_remove() won't call unregister_algs() for last JR
> >     until tfm_count reaches zero, which can only happen via
> >     unregister_algs() -> caam_rng_exit() call chain.
> >
> > To avoid all of that, convert caamrng code to a platform device driver
> > and extend caam_probe() to create corresponding platform device.
> >
> > Additionally, this change also allows us to remove any access to
> > struct caam_drv_private in caamrng.c as well as simplify resource
> > ownership/deallocation via devres.
> >
> I would avoid adding platform devices that don't have
> corresponding DT nodes.
>
> There's some generic advice here:
> https://www.kernel.org/doc/html/latest/driver-api/driver-model/platform.html#legacy-drivers-device-probing
>
> and there's also previous experience in caam driver:
> 6b175685b4a1 crypto: caam/qi - don't allocate an extra platform device
>

Hmm, I am not sure how that experience relates to the case we have
with hwrng, but OK, I'm going to assume that platform driver approach
is a no-go.

> The issue in caamrng is actually that caam/jr driver (jr.c) tries to call
> caam_rng_exit() on the last available jr device.
> Instead, caam_rng_exit() must be called on the same jr device that
> was used during caam_rng_init().
>
> Otherwise, by the time:
>
> void caam_rng_exit(void)
> {
>         if (!init_done)
>                 return;
>
>         caam_jr_free(rng_ctx->jrdev);
>         hwrng_unregister(&caam_rng);
> [...]
>
> is executed, rng_ctx->jrdev might have been removed.
>
> This will cause an oops in caam_jr_free().
> caam_cleanup() - .cleanup hwrng callback that is called when doing
> hwrng_unregister() - also needs to be executed on that jr device.
>
> The problem can be easily reproduced as follows.
>
> If caamrng was initialized on jr0:
> [...]
> caam_jr 2101000.jr0: registering rng-caam
> [...]
>
> then by manually unbinding jr0 first, then jr1 (using i.MX6SX):
> # echo -n "2101000.jr0" > /sys/bus/platform/drivers/caam_jr/
> # echo -n "2102000.jr1" > /sys/bus/platform/drivers/caam_jr/
>
> Unable to handle kernel NULL pointer dereference at virtual address 00000040
> pgd = 572e14e7
> [00000040] *pgd=be2e8831
> Internal error: Oops: 17 [#1] SMP ARM
> Modules linked in:
> CPU: 0 PID: 629 Comm: sh Not tainted 5.3.0-rc1-00299-g8e2a2738e5d3-dirty #8
> Hardware name: Freescale i.MX6 SoloX (Device Tree)
> PC is at caam_jr_free+0xc/0x24
> LR is at caam_rng_exit+0x20/0x3c
> pc : [<c08aef20>]    lr : [<c08bab1c>]    psr: 200f0013
> sp : e9669e68  ip : 00001488  fp : 00000000
> r10: 00000000  r9 : e9669f80  r8 : e9284010
> r7 : e872d410  r6 : e872d410  r5 : e872d400  r4 : c1aa7cd8
> r3 : 00000000  r2 : 00000040  r1 : 00000000  r0 : e872d010
> Flags: nzCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment none
> Control: 10c5387d  Table: a969004a  DAC: 00000051
> Process sh (pid: 629, stack limit = 0xfc1b6e94)
> Stack: (0xe9669e68 to 0xe966a000)
> 9e60:                   e865c940 c08af7dc e872d410 e872d410 c13a9cec c06b223c
> 9e80: c06b2218 e872d410 e81a9410 c06b08dc c13806f0 0000000b c13a9cec c06aeaf8
> 9ea0: 0000000b 00000000 0000000b e9284000 e91189c0 c0318c3c 00000000 00000000
> 9ec0: e95ddbd0 e8843500 c1308b08 c6614c9f e8843500 00438340 e9668000 00000004
> 9ee0: 00000000 c0285e00 00000001 00000000 00000000 c0288a44 00000000 00000000
> 9f00: c1308b28 00000000 00000001 c130911c 00000001 c13e81d1 c0288a44 00000000
> 9f20: e8ed9800 c019df00 e8ed9a7c c028ac08 00000001 00000000 c0288a44 c6614c9f
> 9f40: c1308b08 0000000b 00438340 e9669f80 e8843500 00438340 e9668000 c028899c
> 9f60: e95ddbc0 c6614c9f e8843500 e8843500 c1308b08 0000000b 00438340 c0288bfc
> 9f80: 00000000 00000000 00000000 c6614c9f 0000000b 00438340 b6ef1da0 00000004
> 9fa0: c01011c4 c0101000 0000000b 00438340 00000001 00438340 0000000b 00000000
> 9fc0: 0000000b 00438340 b6ef1da0 00000004 00438340 0000000b 00000000 00000000
> 9fe0: 0000006c bea7f908 b6e19e58 b6e7325c 600f0010 00000001 00000000 00000000
> [<c08aef20>] (caam_jr_free) from [<c08bab1c>] (caam_rng_exit+0x20/0x3c)
> [<c08bab1c>] (caam_rng_exit) from [<c08af7dc>] (caam_jr_remove+0x38/0xc0)
> [<c08af7dc>] (caam_jr_remove) from [<c06b223c>] (platform_drv_remove+0x24/0x3c)
> [<c06b223c>] (platform_drv_remove) from [<c06b08dc>] (device_release_driver_internal+0xdc/0x1a0)
> [<c06b08dc>] (device_release_driver_internal) from [<c06aeaf8>] (unbind_store+0x5c/0xcc)
> [<c06aeaf8>] (unbind_store) from [<c0318c3c>] (kernfs_fop_write+0xfc/0x1e0)
> [<c0318c3c>] (kernfs_fop_write) from [<c0285e00>] (__vfs_write+0x2c/0x1d0)
> [<c0285e00>] (__vfs_write) from [<c028899c>] (vfs_write+0xa0/0x180)
> [<c028899c>] (vfs_write) from [<c0288bfc>] (ksys_write+0x5c/0xdc)
> [<c0288bfc>] (ksys_write) from [<c0101000>] (ret_fast_syscall+0x0/0x28)
> Exception stack(0xe9669fa8 to 0xe9669ff0)
> 9fa0:                   0000000b 00438340 00000001 00438340 0000000b 00000000
> 9fc0: 0000000b 00438340 b6ef1da0 00000004 00438340 0000000b 00000000 00000000
> 9fe0: 0000006c bea7f908 b6e19e58 b6e7325c
> Code: eaffff49 e5903040 e2832040 f5d2f000 (e1921f9f)
>
> Thus, I'd say the following fix is needed:
>
> -- >8 --
>
> When caam_rng_exit() executes, the jr device that was used
> during caam_rng_init() must be available.
>
> This means that current scheme - where caam_rng_exit() is called
> when last jr device is removed - is incorrect.
> Instead, caam_rng_exit() has to run when the jr acquired
> during caam_rng_init() is removed.
>
> Fixes: 1b46c90c8e00 ("crypto: caam - convert top level drivers to libraries")
> Signed-off-by: Horia Geantă <horia.geanta@nxp.com>
>
> diff --git a/drivers/crypto/caam/caamrng.c b/drivers/crypto/caam/caamrng.c
> index e8baacaabe07..ec40178fa688 100644
> --- a/drivers/crypto/caam/caamrng.c
> +++ b/drivers/crypto/caam/caamrng.c
> @@ -300,9 +300,9 @@ static struct hwrng caam_rng = {
>         .read           = caam_read,
>  };
>
> -void caam_rng_exit(void)
> +void caam_rng_exit(struct device *jrdev)
>  {
> -       if (!init_done)
> +       if (!init_done || jrdev != rng_ctx->jrdev)
>                 return;
>
>         caam_jr_free(rng_ctx->jrdev);
> diff --git a/drivers/crypto/caam/intern.h b/drivers/crypto/caam/intern.h
> index 731b06becd9c..4795530203ad 100644
> --- a/drivers/crypto/caam/intern.h
> +++ b/drivers/crypto/caam/intern.h
> @@ -165,7 +165,7 @@ 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);
> +void caam_rng_exit(struct device *jrdev);
>
>  #else
>
> @@ -174,7 +174,7 @@ static inline int caam_rng_init(struct device *dev)
>         return 0;
>  }
>
> -static inline void caam_rng_exit(void)
> +static inline void caam_rng_exit(struct device *jrdev)
>  {
>  }
>
> diff --git a/drivers/crypto/caam/jr.c b/drivers/crypto/caam/jr.c
> index d11956bc358f..61aea11773a6 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();
> @@ -126,6 +125,8 @@ static int caam_jr_remove(struct platform_device *pdev)
>         jrdev = &pdev->dev;
>         jrpriv = dev_get_drvdata(jrdev);
>
> +       caam_rng_exit(jrdev);
> +
>         /*
>          * Return EBUSY if job ring already allocated.
>          */

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH 07/12] crypto: caam - use devres to de-initialize the RNG
  2019-09-09 15:39   ` Horia Geanta
@ 2019-09-18  6:06     ` Andrey Smirnov
  0 siblings, 0 replies; 43+ messages in thread
From: Andrey Smirnov @ 2019-09-18  6:06 UTC (permalink / raw)
  To: Horia Geanta
  Cc: linux-crypto, Chris Healy, Lucas Stach, Herbert Xu,
	Iuliana Prodan, linux-kernel

On Mon, Sep 9, 2019 at 8:39 AM Horia Geanta <horia.geanta@nxp.com> wrote:
>
> On 9/4/2019 5:35 AM, Andrey Smirnov wrote:
> > Use devres to de-initialize the RNG and drop explicit de-initialization
> > code in caam_remove().
> >
> > 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
> > ---
> >  drivers/crypto/caam/ctrl.c | 129 ++++++++++++++++++++-----------------
> >  1 file changed, 70 insertions(+), 59 deletions(-)
> >
> > diff --git a/drivers/crypto/caam/ctrl.c b/drivers/crypto/caam/ctrl.c
> > index 254963498abc..25f8f76551a5 100644
> > --- a/drivers/crypto/caam/ctrl.c
> > +++ b/drivers/crypto/caam/ctrl.c
> > @@ -175,6 +175,73 @@ static inline int run_descriptor_deco0(struct device *ctrldev, u32 *desc,
> >       return 0;
> >  }
> >
> > +/*
> > + * deinstantiate_rng - builds and executes a descriptor on DECO0,
> > + *                  which deinitializes the RNG block.
> > + * @ctrldev - pointer to device
> > + * @state_handle_mask - bitmask containing the instantiation status
> > + *                   for the RNG4 state handles which exist in
> > + *                   the RNG4 block: 1 if it's been instantiated
> > + *
> > + * Return: - 0 if no error occurred
> > + *      - -ENOMEM if there isn't enough memory to allocate the descriptor
> > + *      - -ENODEV if DECO0 couldn't be acquired
> > + *      - -EAGAIN if an error occurred when executing the descriptor
> > + */
> > +static int deinstantiate_rng(struct device *ctrldev, int state_handle_mask)
> I assume this function is not modified, only moved further up
> to avoid forward declaration.
>

Correct.

> > +     if (!ret) {
> > +             ret = devm_add_action_or_reset(ctrldev, devm_deinstantiate_rng,
> > +                                            ctrldev);
> >       }
> Braces not needed.
>

OK, will remove in next version.

> Is there any guidance wrt. when *not* to use devres?
>

Not that I now of.

Thanks,
Andrey Smirnov

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH 12/12] crypto: caam - change JR device ownership scheme
  2019-09-18  3:13     ` Andrey Smirnov
@ 2019-09-19 11:19       ` Horia Geanta
  2019-09-19 13:45         ` Herbert Xu
  0 siblings, 1 reply; 43+ messages in thread
From: Horia Geanta @ 2019-09-19 11:19 UTC (permalink / raw)
  To: Andrey Smirnov, Leonard Crestez, Herbert Xu
  Cc: linux-crypto, Chris Healy, Lucas Stach, Iuliana Prodan,
	linux-kernel, Abel Vesa

On 9/18/2019 6:13 AM, Andrey Smirnov wrote:
>> I think you need to do some form of slow wait loop in jrpriv until
>> jrpriv->tfm_count reaches zero.
> Hmm, what do we do if it never does? Why do you think it would be
> better than cancelling all outstanding jobs and resetting the HW?
> 
Herbert,

What should a driver do when:
-user tries to unbind it AND
-there are tfms referencing algorithms registered by this driver

1. If driver tries to unregister the algorithms during its .remove()
callback, then this BUG_ON is hit:

int crypto_unregister_alg(struct crypto_alg *alg)
{
[...]
        BUG_ON(refcount_read(&alg->cra_refcnt) != 1);

2. If driver exits without unregistering the algorithms,
next time one of the tfms referencing those algorithms will be used
bad things will happen.

3. There is no mechanism in crypto core for notifying users
to stop using a tfm.

Thanks,
Horia

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH 12/12] crypto: caam - change JR device ownership scheme
  2019-09-19 11:19       ` Horia Geanta
@ 2019-09-19 13:45         ` Herbert Xu
  0 siblings, 0 replies; 43+ messages in thread
From: Herbert Xu @ 2019-09-19 13:45 UTC (permalink / raw)
  To: Horia Geanta
  Cc: Andrey Smirnov, Leonard Crestez, linux-crypto, Chris Healy,
	Lucas Stach, Iuliana Prodan, linux-kernel, Abel Vesa

On Thu, Sep 19, 2019 at 11:19:22AM +0000, Horia Geanta wrote:
>
> What should a driver do when:
> -user tries to unbind it AND
> -there are tfms referencing algorithms registered by this driver
> 
> 1. If driver tries to unregister the algorithms during its .remove()
> callback, then this BUG_ON is hit:
> 
> int crypto_unregister_alg(struct crypto_alg *alg)
> {
> [...]
>         BUG_ON(refcount_read(&alg->cra_refcnt) != 1);

You must not unregister the algorithm until it's no longer in use.
Normally we enforce this using module reference counts.

For hardware devices that need to be unregistered without unloading
the module at the same time, you will need your own reference
counting to determine when unregistration can be carried out.  But
it must be carefully done so that it is not racy.

> 2. If driver exits without unregistering the algorithms,
> next time one of the tfms referencing those algorithms will be used
> bad things will happen.

Well remember hardware devices can always go away (e.g., dead
or unplugged) so the driver must do something sensible when that
happens.  If this happened on a live tfm then further operations
should ideally fail and any outstanding requests should also fail
in a timely manner.

> 3. There is no mechanism in crypto core for notifying users
> to stop using a tfm.

For software implementations we use the module reference count
as the mechanism to signal that an algorithm is going away.  In
particular try_module_get (and consequently crypto_mod_get) will
fail when the module is being unregistered.

We should extend this to cover hardware devices.  Perhaps we can
introduce a callback in crypto_alg that crypto_mod_get can invoke
which would then fail if the device is going away (or has gone away).

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH 08/12] crypto: caam - use devres to de-initialize QI
  2019-09-04  2:35 ` [PATCH 08/12] crypto: caam - use devres to de-initialize QI Andrey Smirnov
@ 2019-09-20 15:10   ` Horia Geanta
  0 siblings, 0 replies; 43+ messages in thread
From: Horia Geanta @ 2019-09-20 15:10 UTC (permalink / raw)
  To: Andrey Smirnov, linux-crypto
  Cc: Chris Healy, Lucas Stach, Herbert Xu, Iuliana Prodan, linux-kernel

On 9/4/2019 5:35 AM, Andrey Smirnov wrote:
> Use devres to de-initialize the QI and drop explicit de-initialization
> code in caam_remove().
> 
> 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
Reviewed-by: Horia Geantã <horia.geanta@nxp.com>

Thanks,
Horia

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH 09/12] crypto: caam - user devres to populate platform devices
  2019-09-04  2:35 ` [PATCH 09/12] crypto: caam - user devres to populate platform devices Andrey Smirnov
@ 2019-09-20 15:29   ` Horia Geanta
  0 siblings, 0 replies; 43+ messages in thread
From: Horia Geanta @ 2019-09-20 15:29 UTC (permalink / raw)
  To: Andrey Smirnov, linux-crypto
  Cc: Chris Healy, Lucas Stach, Herbert Xu, Iuliana Prodan, linux-kernel

On 9/4/2019 5:35 AM, Andrey Smirnov wrote:
> Use devres to de-initialize the RNG and drop explicit de-initialization
> code in caam_remove().
> 
> 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
Reviewed-by: Horia Geantã <horia.geanta@nxp.com>

Thanks,
Horia

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH 10/12] crypto: caam - populate platform devices last
  2019-09-04  2:35 ` [PATCH 10/12] crypto: caam - populate platform devices last Andrey Smirnov
@ 2019-09-20 15:35   ` Horia Geanta
  0 siblings, 0 replies; 43+ messages in thread
From: Horia Geanta @ 2019-09-20 15:35 UTC (permalink / raw)
  To: Andrey Smirnov, linux-crypto
  Cc: Chris Healy, Lucas Stach, Herbert Xu, Iuliana Prodan, linux-kernel

On 9/4/2019 5:35 AM, Andrey Smirnov wrote:
> @@ -906,6 +900,13 @@ static int caam_probe(struct platform_device *pdev)
>  	debugfs_create_blob("tdsk", S_IRUSR | S_IRGRP | S_IROTH, ctrlpriv->ctl,
>  			    &ctrlpriv->ctl_tdsk_wrap);
>  #endif
> +
> +	ret = devm_of_platform_populate(dev);
> +	if (ret) {
> +		dev_err(dev, "JR platform devices creation error\n");
> +		return ret;
> +	}
> +
>  	return 0;
>  }
This is a bit better:

	if (ret)
		dev_err(dev, "JR platform devices creation error\n");

	return ret;

Horia

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH] crypto: caam - use the same jr for rng init/exit
  2019-09-18  6:01     ` Andrey Smirnov
@ 2019-09-20 15:50       ` Horia Geanta
  0 siblings, 0 replies; 43+ messages in thread
From: Horia Geanta @ 2019-09-20 15:50 UTC (permalink / raw)
  To: Andrey Smirnov
  Cc: linux-crypto, Chris Healy, Lucas Stach, Herbert Xu,
	Iuliana Prodan, linux-kernel

On 9/18/2019 9:01 AM, Andrey Smirnov wrote:
> On Wed, Sep 11, 2019 at 2:35 AM Horia Geanta <horia.geanta@nxp.com> wrote:
>>
>> On 9/4/2019 5:35 AM, Andrey Smirnov wrote:
>>> In order to allow caam_jr_enqueue() to lock underlying JR's
>>> device (via device_lock(), see commit that follows) we need to make
>>> sure that no code calls caam_jr_enqueue() as a part of caam_jr_probe()
>>> to avoid a deadlock. Unfortunately, current implementation of caamrng
>>> code does exactly that in caam_init_buf().
>>>
> 
> I don't think your patch addresses the above, so it can probably be
> cut out of the message.
> 
No, it does not, it addresses the issue right below.

Not sure what you mean by "cut out of the message". If you look carefully
in the original message, at some pointer there is a scissors line.

>>> Another big problem with original caamrng initialization is a
>>> circular reference in the form of:
>>>
>>>  1. caam_rng_init() aquires JR via caam_jr_alloc(). Freed only by
>>>     caam_rng_exit()
>>>
>>>  2. caam_rng_exit() is only called by unregister_algs() once last JR
>>>     is shut down
>>>
>>>  3. caam_jr_remove() won't call unregister_algs() for last JR
>>>     until tfm_count reaches zero, which can only happen via
>>>     unregister_algs() -> caam_rng_exit() call chain.
>>>
>>> To avoid all of that, convert caamrng code to a platform device driver
>>> and extend caam_probe() to create corresponding platform device.
>>>
>>> Additionally, this change also allows us to remove any access to
>>> struct caam_drv_private in caamrng.c as well as simplify resource
>>> ownership/deallocation via devres.
>>>
>> I would avoid adding platform devices that don't have
>> corresponding DT nodes.
>>
>> There's some generic advice here:
>> https://www.kernel.org/doc/html/latest/driver-api/driver-model/platform.html#legacy-drivers-device-probing
>>
>> and there's also previous experience in caam driver:
>> 6b175685b4a1 crypto: caam/qi - don't allocate an extra platform device
>>
> 
> Hmm, I am not sure how that experience relates to the case we have
> with hwrng, but OK, I'm going to assume that platform driver approach
> is a no-go.
> 
Not specific to hwrng, but platform drivers in general:
    [...]
    SMMU. A platform device allocated using platform_device_register_full()
    is not completely set up - most importantly .dma_configure()
    is not called.

Horia

^ permalink raw reply	[flat|nested] 43+ messages in thread

end of thread, back to index

Thread overview: 43+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-09-04  2:35 [PATCH 00/12] CAAM bugfixes, small improvements Andrey Smirnov
2019-09-04  2:35 ` [PATCH 01/12] crypto: caam - make sure clocks are enabled first Andrey Smirnov
2019-09-06 11:18   ` Horia Geanta
2019-09-09  7:21     ` Herbert Xu
2019-09-09  7:22       ` Herbert Xu
2019-09-04  2:35 ` [PATCH 02/12] crypto: caam - use devres to unmap JR's registers Andrey Smirnov
2019-09-04  2:43   ` Fabio Estevam
2019-09-04  2:55     ` Andrey Smirnov
2019-09-09 13:01   ` Horia Geanta
2019-09-04  2:35 ` [PATCH 03/12] crypto: caam - check irq_of_parse_and_map for errors Andrey Smirnov
2019-09-06 12:29   ` Horia Geanta
2019-09-04  2:35 ` [PATCH 04/12] crypto: caam - dispose of IRQ mapping only after IRQ is freed Andrey Smirnov
2019-09-06 12:26   ` Horia Geanta
2019-09-09  7:46   ` crypto: caam - Cast to long first before pointer conversion Herbert Xu
2019-09-09 11:06     ` Horia Geanta
2019-09-09 13:55     ` [v2 PATCH] " Herbert Xu
2019-09-04  2:35 ` [PATCH 05/12] crypto: caam - use devres to unmap memory Andrey Smirnov
2019-09-09 13:20   ` Horia Geanta
2019-09-04  2:35 ` [PATCH 06/12] crypto: caam - use devres to remove debugfs Andrey Smirnov
2019-09-09 13:25   ` Horia Geanta
2019-09-04  2:35 ` [PATCH 07/12] crypto: caam - use devres to de-initialize the RNG Andrey Smirnov
2019-09-09 15:39   ` Horia Geanta
2019-09-18  6:06     ` Andrey Smirnov
2019-09-04  2:35 ` [PATCH 08/12] crypto: caam - use devres to de-initialize QI Andrey Smirnov
2019-09-20 15:10   ` Horia Geanta
2019-09-04  2:35 ` [PATCH 09/12] crypto: caam - user devres to populate platform devices Andrey Smirnov
2019-09-20 15:29   ` Horia Geanta
2019-09-04  2:35 ` [PATCH 10/12] crypto: caam - populate platform devices last Andrey Smirnov
2019-09-20 15:35   ` Horia Geanta
2019-09-04  2:35 ` [PATCH 11/12] crypto: caam - convert caamrng to platform device Andrey Smirnov
2019-09-11  9:35   ` [PATCH] crypto: caam - use the same jr for rng init/exit Horia Geanta
2019-09-18  6:01     ` Andrey Smirnov
2019-09-20 15:50       ` Horia Geanta
2019-09-04  2:35 ` [PATCH 12/12] crypto: caam - change JR device ownership scheme Andrey Smirnov
2019-09-13 19:16   ` Leonard Crestez
2019-09-18  3:13     ` Andrey Smirnov
2019-09-19 11:19       ` Horia Geanta
2019-09-19 13:45         ` Herbert Xu
2019-09-09  7:53 ` [PATCH 00/12] CAAM bugfixes, small improvements Herbert Xu
2019-09-09 12:07   ` Horia Geanta
2019-09-09 12:52     ` Herbert Xu
2019-09-09 13:26       ` Horia Geanta
2019-09-09 13:52         ` Herbert Xu

Linux-Crypto Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-crypto/0 linux-crypto/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-crypto linux-crypto/ https://lore.kernel.org/linux-crypto \
		linux-crypto@vger.kernel.org linux-crypto@archiver.kernel.org
	public-inbox-index linux-crypto


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-crypto


AGPL code for this site: git clone https://public-inbox.org/ public-inbox