LinuxLists.cc - [PATCH 00/12] CAAM bugfixes, small improvements

2019-09-04 02:36:02

Subject: [PATCH 00/12] CAAM bugfixes, small improvements

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

2019-09-04 02:36:02

by Andrey Smirnov

[permalink] [raw]

Subject: [PATCH 05/12] crypto: caam - use devres to unmap memory

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

Signed-off-by: Andrey Smirnov <[email protected]>
Cc: Chris Healy <[email protected]>
Cc: Lucas Stach <[email protected]>
Cc: Horia Geantă <[email protected]>
Cc: Herbert Xu <[email protected]>
Cc: Iuliana Prodan <[email protected]>
Cc: [email protected]
Cc: [email protected]
---
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

2019-09-04 02:36:11

by Andrey Smirnov

[permalink] [raw]

Subject: [PATCH 09/12] crypto: caam - user devres to populate platform devices

2019-09-04 02:36:13

by Andrey Smirnov

[permalink] [raw]

Subject: [PATCH 10/12] crypto: caam - populate platform devices last

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 <[email protected]>
Cc: Chris Healy <[email protected]>
Cc: Lucas Stach <[email protected]>
Cc: Horia Geantă <[email protected]>
Cc: Herbert Xu <[email protected]>
Cc: Iuliana Prodan <[email protected]>
Cc: [email protected]
Cc: [email protected]
---
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

2019-09-04 02:36:31

by Andrey Smirnov

[permalink] [raw]

Subject: [PATCH 12/12] crypto: caam - change JR device ownership scheme

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 <[email protected]>
Cc: Chris Healy <[email protected]>
Cc: Lucas Stach <[email protected]>
Cc: Horia Geantă <[email protected]>
Cc: Herbert Xu <[email protected]>
Cc: Iuliana Prodan <[email protected]>
Cc: [email protected]
Cc: [email protected]
---
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

2019-09-04 02:36:33

by Andrey Smirnov

[permalink] [raw]

Subject: [PATCH 11/12] crypto: caam - convert caamrng to platform device

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 <[email protected]>
Cc: Chris Healy <[email protected]>
Cc: Lucas Stach <[email protected]>
Cc: Horia Geantă <[email protected]>
Cc: Herbert Xu <[email protected]>
Cc: Iuliana Prodan <[email protected]>
Cc: [email protected]
Cc: [email protected]
---
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

2019-09-04 02:36:59

by Andrey Smirnov

[permalink] [raw]

Subject: [PATCH 07/12] crypto: caam - use devres to de-initialize the RNG

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

Signed-off-by: Andrey Smirnov <[email protected]>
Cc: Chris Healy <[email protected]>
Cc: Lucas Stach <[email protected]>
Cc: Horia Geantă <[email protected]>
Cc: Herbert Xu <[email protected]>
Cc: Iuliana Prodan <[email protected]>
Cc: [email protected]
Cc: [email protected]
---
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

2019-09-04 02:37:24

by Andrey Smirnov

[permalink] [raw]

Subject: [PATCH 03/12] crypto: caam - check irq_of_parse_and_map for errors

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

Signed-off-by: Andrey Smirnov <[email protected]>
Cc: Chris Healy <[email protected]>
Cc: Lucas Stach <[email protected]>
Cc: Horia Geantă <[email protected]>
Cc: Herbert Xu <[email protected]>
Cc: Iuliana Prodan <[email protected]>
Cc: [email protected]
Cc: [email protected]
---
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

2019-09-04 02:38:05

by Andrey Smirnov

[permalink] [raw]

Subject: [PATCH 08/12] crypto: caam - use devres to de-initialize QI

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

Signed-off-by: Andrey Smirnov <[email protected]>
Cc: Chris Healy <[email protected]>
Cc: Lucas Stach <[email protected]>
Cc: Horia Geantă <[email protected]>
Cc: Herbert Xu <[email protected]>
Cc: Iuliana Prodan <[email protected]>
Cc: [email protected]
Cc: [email protected]
---
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

2019-09-04 02:38:19

by Andrey Smirnov

[permalink] [raw]

Subject: [PATCH 06/12] crypto: caam - use devres to remove debugfs

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

Signed-off-by: Andrey Smirnov <[email protected]>
Cc: Chris Healy <[email protected]>
Cc: Lucas Stach <[email protected]>
Cc: Horia Geantă <[email protected]>
Cc: Herbert Xu <[email protected]>
Cc: Iuliana Prodan <[email protected]>
Cc: [email protected]
Cc: [email protected]
---
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

2019-09-06 15:26:55

by Horia Geanta

[permalink] [raw]

Subject: Re: [PATCH 03/12] crypto: caam - check irq_of_parse_and_map for errors

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 <[email protected]>
> Cc: Chris Healy <[email protected]>
> Cc: Lucas Stach <[email protected]>
> Cc: Horia Geant? <[email protected]>
> Cc: Herbert Xu <[email protected]>
> Cc: Iuliana Prodan <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
Reviewed-by: Horia Geant? <[email protected]>

Thanks,
Horia

2019-09-09 14:48:43

by Herbert Xu

[permalink] [raw]

Subject: Re: [PATCH 00/12] CAAM bugfixes, small improvements

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 <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

2019-09-10 01:11:06

by Horia Geanta

[permalink] [raw]

Subject: Re: [PATCH 00/12] CAAM bugfixes, small improvements

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

2019-09-10 07:26:13

by Herbert Xu

[permalink] [raw]

Subject: Re: [PATCH 00/12] CAAM bugfixes, small improvements

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 <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

2019-09-10 07:46:36

by Horia Geanta

[permalink] [raw]

Subject: Re: [PATCH 05/12] crypto: caam - use devres to unmap memory

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 <[email protected]>
> Cc: Chris Healy <[email protected]>
> Cc: Lucas Stach <[email protected]>
> Cc: Horia Geant? <[email protected]>
> Cc: Herbert Xu <[email protected]>
> Cc: Iuliana Prodan <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
Reviewed-by: Horia Geant? <[email protected]>

Thanks,
Horia

2019-09-10 07:48:17

by Horia Geanta

[permalink] [raw]

Subject: Re: [PATCH 06/12] crypto: caam - use devres to remove debugfs

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 <[email protected]>
> Cc: Chris Healy <[email protected]>
> Cc: Lucas Stach <[email protected]>
> Cc: Horia Geant? <[email protected]>
> Cc: Herbert Xu <[email protected]>
> Cc: Iuliana Prodan <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
Reviewed-by: Horia Geant? <[email protected]>

Thanks,
Horia

2019-09-10 07:48:43

by Horia Geanta

[permalink] [raw]

Subject: Re: [PATCH 00/12] CAAM bugfixes, small improvements

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

2019-09-10 08:47:57

by Horia Geanta

[permalink] [raw]

Subject: Re: [PATCH 07/12] crypto: caam - use devres to de-initialize the RNG

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 <[email protected]>
> Cc: Chris Healy <[email protected]>
> Cc: Lucas Stach <[email protected]>
> Cc: Horia Geant? <[email protected]>
> Cc: Herbert Xu <[email protected]>
> Cc: Iuliana Prodan <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> ---
> 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

2019-09-13 20:13:18

by Leonard Crestez

[permalink] [raw]

Subject: Re: [PATCH 12/12] crypto: caam - change JR device ownership scheme

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;
> }

2019-09-18 05:59:47

by Andrey Smirnov

[permalink] [raw]

Subject: Re: [PATCH 12/12] crypto: caam - change JR device ownership scheme

On Fri, Sep 13, 2019 at 12:16 PM Leonard Crestez
<[email protected]> 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

2019-09-18 06:21:46

by Andrey Smirnov

[permalink] [raw]

Subject: Re: [PATCH 07/12] crypto: caam - use devres to de-initialize the RNG

On Mon, Sep 9, 2019 at 8:39 AM Horia Geanta <[email protected]> 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 <[email protected]>
> > Cc: Chris Healy <[email protected]>
> > Cc: Lucas Stach <[email protected]>
> > Cc: Horia Geantă <[email protected]>
> > Cc: Herbert Xu <[email protected]>
> > Cc: Iuliana Prodan <[email protected]>
> > Cc: [email protected]
> > Cc: [email protected]
> > ---
> > 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

2019-09-19 13:02:43

by Horia Geanta

[permalink] [raw]

Subject: Re: [PATCH 12/12] crypto: caam - change JR device ownership scheme

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

2019-09-19 13:55:44

by Herbert Xu

[permalink] [raw]

Subject: Re: [PATCH 12/12] crypto: caam - change JR device ownership scheme

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 <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

2019-09-22 18:47:19

by Horia Geanta

[permalink] [raw]

Subject: Re: [PATCH 08/12] crypto: caam - use devres to de-initialize QI

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 <[email protected]>
> Cc: Chris Healy <[email protected]>
> Cc: Lucas Stach <[email protected]>
> Cc: Horia Geant? <[email protected]>
> Cc: Herbert Xu <[email protected]>
> Cc: Iuliana Prodan <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
Reviewed-by: Horia Geant? <[email protected]>

Thanks,
Horia

2019-09-22 18:47:53

by Horia Geanta

[permalink] [raw]

Subject: Re: [PATCH 09/12] crypto: caam - user devres to populate platform devices

2019-09-22 18:48:14

by Horia Geanta

[permalink] [raw]

Subject: Re: [PATCH 10/12] crypto: caam - populate platform devices last

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