From aa2be9b3d6d2d699e9ca7cbfc00867c80e5da213 Mon Sep 17 00:00:00 2001 From: Daniel Axtens Date: Fri, 3 Mar 2017 17:56:55 +1100 Subject: [PATCH 1/6] crypto: powerpc - Fix initialisation of crc32c context Turning on crypto self-tests on a POWER8 shows: alg: hash: Test 1 failed for crc32c-vpmsum 00000000: ff ff ff ff Comparing the code with the Intel CRC32c implementation on which ours is based shows that we are doing an init with 0, not ~0 as CRC32c requires. This probably wasn't caught because btrfs does its own weird open-coded initialisation. Initialise our internal context to ~0 on init. This makes the self-tests pass, and btrfs continues to work. Fixes: 6dd7a82cc54e ("crypto: powerpc - Add POWER8 optimised crc32c") Cc: Anton Blanchard Cc: stable@vger.kernel.org Signed-off-by: Daniel Axtens Acked-by: Anton Blanchard Signed-off-by: Herbert Xu --- arch/powerpc/crypto/crc32c-vpmsum_glue.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/arch/powerpc/crypto/crc32c-vpmsum_glue.c b/arch/powerpc/crypto/crc32c-vpmsum_glue.c index 9fa046d56eba..411994551afc 100644 --- a/arch/powerpc/crypto/crc32c-vpmsum_glue.c +++ b/arch/powerpc/crypto/crc32c-vpmsum_glue.c @@ -52,7 +52,7 @@ static int crc32c_vpmsum_cra_init(struct crypto_tfm *tfm) { u32 *key = crypto_tfm_ctx(tfm); - *key = 0; + *key = ~0; return 0; } From 07de4bc88ce6a4d898cad9aa4c99c1df7e87702d Mon Sep 17 00:00:00 2001 From: Krzysztof Kozlowski Date: Sun, 5 Mar 2017 19:14:07 +0200 Subject: [PATCH 2/6] crypto: s5p-sss - Fix completing crypto request in IRQ handler In a regular interrupt handler driver was finishing the crypt/decrypt request by calling complete on crypto request. This is disallowed since converting to skcipher in commit b286d8b1a690 ("crypto: skcipher - Add skcipher walk interface") and causes a warning: WARNING: CPU: 0 PID: 0 at crypto/skcipher.c:430 skcipher_walk_first+0x13c/0x14c The interrupt is marked shared but in fact there are no other users sharing it. Thus the simplest solution seems to be to just use a threaded interrupt handler, after converting it to oneshot. Signed-off-by: Krzysztof Kozlowski Signed-off-by: Herbert Xu --- drivers/crypto/s5p-sss.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/drivers/crypto/s5p-sss.c b/drivers/crypto/s5p-sss.c index dce1af0ce85c..a668286d62cb 100644 --- a/drivers/crypto/s5p-sss.c +++ b/drivers/crypto/s5p-sss.c @@ -805,8 +805,9 @@ static int s5p_aes_probe(struct platform_device *pdev) dev_warn(dev, "feed control interrupt is not available.\n"); goto err_irq; } - err = devm_request_irq(dev, pdata->irq_fc, s5p_aes_interrupt, - IRQF_SHARED, pdev->name, pdev); + err = devm_request_threaded_irq(dev, pdata->irq_fc, NULL, + s5p_aes_interrupt, IRQF_ONESHOT, + pdev->name, pdev); if (err < 0) { dev_warn(dev, "feed control interrupt is not available.\n"); goto err_irq; From 45c2fdde01299b02a6e3225e848598a3c1e55539 Mon Sep 17 00:00:00 2001 From: Thomas Petazzoni Date: Tue, 7 Mar 2017 15:14:46 +0100 Subject: [PATCH 3/6] hwrng: omap - write registers after enabling the clock Commit 383212425c926 ("hwrng: omap - Add device variant for SafeXcel IP-76 found in Armada 8K") added support for the SafeXcel IP-76 variant of the IP. This modification included getting a reference and enabling a clock. Unfortunately, this was done *after* writing to the RNG_INTMASK_REG register. This generally works fine when the driver is built-in because the clock might have been left enabled by the bootloader, but fails short when the driver is built as a module: it causes a system hang because a register is being accessed while the clock is not enabled. This commit fixes that by making the register access *after* enabling the clock. This issue was found by the kernelci.org testing effort. Fixes: 383212425c926 ("hwrng: omap - Add device variant for SafeXcel IP-76 found in Armada 8K") Cc: Signed-off-by: Thomas Petazzoni Signed-off-by: Herbert Xu --- drivers/char/hw_random/omap-rng.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/char/hw_random/omap-rng.c b/drivers/char/hw_random/omap-rng.c index 3ad86fdf954e..efa3747c1750 100644 --- a/drivers/char/hw_random/omap-rng.c +++ b/drivers/char/hw_random/omap-rng.c @@ -397,7 +397,6 @@ static int of_get_omap_rng_device_details(struct omap_rng_dev *priv, irq, err); return err; } - omap_rng_write(priv, RNG_INTMASK_REG, RNG_SHUTDOWN_OFLO_MASK); priv->clk = of_clk_get(pdev->dev.of_node, 0); if (IS_ERR(priv->clk) && PTR_ERR(priv->clk) == -EPROBE_DEFER) @@ -408,6 +407,8 @@ static int of_get_omap_rng_device_details(struct omap_rng_dev *priv, dev_err(&pdev->dev, "unable to enable the clk, " "err = %d\n", err); } + + omap_rng_write(priv, RNG_INTMASK_REG, RNG_SHUTDOWN_OFLO_MASK); } return 0; } From 761c2510283066324cab7859930777ee34cbca78 Mon Sep 17 00:00:00 2001 From: Thomas Petazzoni Date: Tue, 7 Mar 2017 15:14:47 +0100 Subject: [PATCH 4/6] hwrng: omap - use devm_clk_get() instead of of_clk_get() The omap-rng driver currently uses of_clk_get() to get a reference to the clock, but never releases that reference. This commit fixes that by using devm_clk_get() instead. Fixes: 383212425c926 ("hwrng: omap - Add device variant for SafeXcel IP-76 found in Armada 8K") Cc: Signed-off-by: Thomas Petazzoni Signed-off-by: Herbert Xu --- drivers/char/hw_random/omap-rng.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/char/hw_random/omap-rng.c b/drivers/char/hw_random/omap-rng.c index efa3747c1750..d2866280f130 100644 --- a/drivers/char/hw_random/omap-rng.c +++ b/drivers/char/hw_random/omap-rng.c @@ -398,7 +398,7 @@ static int of_get_omap_rng_device_details(struct omap_rng_dev *priv, return err; } - priv->clk = of_clk_get(pdev->dev.of_node, 0); + priv->clk = devm_clk_get(&pdev->dev, NULL); if (IS_ERR(priv->clk) && PTR_ERR(priv->clk) == -EPROBE_DEFER) return -EPROBE_DEFER; if (!IS_ERR(priv->clk)) { From b985735be7afea3a5e0570ce2ea0b662c0e12e19 Mon Sep 17 00:00:00 2001 From: Thomas Petazzoni Date: Tue, 7 Mar 2017 15:14:48 +0100 Subject: [PATCH 5/6] hwrng: omap - Do not access INTMASK_REG on EIP76 The INTMASK_REG register does not exist on EIP76. Due to this, the call: omap_rng_write(priv, RNG_INTMASK_REG, RNG_SHUTDOWN_OFLO_MASK); ends up, through the reg_map_eip76[] array, in accessing the register at offset 0, which is the RNG_OUTPUT_0_REG. This by itself doesn't cause any problem, but clearly doesn't enable the interrupt as it was expected. On EIP76, the register that allows to enable the interrupt is RNG_CONTROL_REG. And just like RNG_INTMASK_REG, it's bit 1 of this register that allows to enable the shutdown_oflo interrupt. Fixes: 383212425c926 ("hwrng: omap - Add device variant for SafeXcel IP-76 found in Armada 8K") Cc: Signed-off-by: Thomas Petazzoni Signed-off-by: Herbert Xu --- drivers/char/hw_random/omap-rng.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/drivers/char/hw_random/omap-rng.c b/drivers/char/hw_random/omap-rng.c index d2866280f130..b1ad12552b56 100644 --- a/drivers/char/hw_random/omap-rng.c +++ b/drivers/char/hw_random/omap-rng.c @@ -408,7 +408,18 @@ static int of_get_omap_rng_device_details(struct omap_rng_dev *priv, "err = %d\n", err); } - omap_rng_write(priv, RNG_INTMASK_REG, RNG_SHUTDOWN_OFLO_MASK); + /* + * On OMAP4, enabling the shutdown_oflo interrupt is + * done in the interrupt mask register. There is no + * such register on EIP76, and it's enabled by the + * same bit in the control register + */ + if (priv->pdata->regs[RNG_INTMASK_REG]) + omap_rng_write(priv, RNG_INTMASK_REG, + RNG_SHUTDOWN_OFLO_MASK); + else + omap_rng_write(priv, RNG_CONTROL_REG, + RNG_SHUTDOWN_OFLO_MASK); } return 0; } From 28b62b1458685d8f68f67d9b2d511bf8fa32b746 Mon Sep 17 00:00:00 2001 From: Krzysztof Kozlowski Date: Wed, 8 Mar 2017 23:14:20 +0200 Subject: [PATCH 6/6] crypto: s5p-sss - Fix spinlock recursion on LRW(AES) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Running TCRYPT with LRW compiled causes spinlock recursion: testing speed of async lrw(aes) (lrw(ecb-aes-s5p)) encryption tcrypt: test 0 (256 bit key, 16 byte blocks): 19007 operations in 1 seconds (304112 bytes) tcrypt: test 1 (256 bit key, 64 byte blocks): 15753 operations in 1 seconds (1008192 bytes) tcrypt: test 2 (256 bit key, 256 byte blocks): 14293 operations in 1 seconds (3659008 bytes) tcrypt: test 3 (256 bit key, 1024 byte blocks): 11906 operations in 1 seconds (12191744 bytes) tcrypt: test 4 (256 bit key, 8192 byte blocks): BUG: spinlock recursion on CPU#1, irq/84-10830000/89  lock: 0xeea99a68, .magic: dead4ead, .owner: irq/84-10830000/89, .owner_cpu: 1 CPU: 1 PID: 89 Comm: irq/84-10830000 Not tainted 4.11.0-rc1-00001-g897ca6d0800d #559 Hardware name: SAMSUNG EXYNOS (Flattened Device Tree) [] (unwind_backtrace) from [] (show_stack+0x10/0x14) [] (show_stack) from [] (dump_stack+0x78/0x8c) [] (dump_stack) from [] (do_raw_spin_lock+0x11c/0x120) [] (do_raw_spin_lock) from [] (_raw_spin_lock_irqsave+0x20/0x28) [] (_raw_spin_lock_irqsave) from [] (s5p_aes_crypt+0x2c/0xb4) [] (s5p_aes_crypt) from [] (do_encrypt+0x78/0xb0 [lrw]) [] (do_encrypt [lrw]) from [] (encrypt_done+0x24/0x54 [lrw]) [] (encrypt_done [lrw]) from [] (s5p_aes_complete+0x60/0xcc) [] (s5p_aes_complete) from [] (s5p_aes_interrupt+0x134/0x1a0) [] (s5p_aes_interrupt) from [] (irq_thread_fn+0x1c/0x54) [] (irq_thread_fn) from [] (irq_thread+0x12c/0x1e0) [] (irq_thread) from [] (kthread+0x108/0x138) [] (kthread) from [] (ret_from_fork+0x14/0x3c) Interrupt handling routine was calling req->base.complete() under spinlock. In most cases this wasn't fatal but when combined with some of the cipher modes (like LRW) this caused recursion - starting the new encryption (s5p_aes_crypt()) while still holding the spinlock from previous round (s5p_aes_complete()). Beside that, the s5p_aes_interrupt() error handling path could execute two completions in case of error for RX and TX blocks. Rewrite the interrupt handling routine and the completion by: 1. Splitting the operations on scatterlist copies from s5p_aes_complete() into separate s5p_sg_done(). This still should be done under lock. The s5p_aes_complete() now only calls req->base.complete() and it has to be called outside of lock. 2. Moving the s5p_aes_complete() out of spinlock critical sections. In interrupt service routine s5p_aes_interrupts(), it appeared in few places, including error paths inside other functions called from ISR. This code was not so obvious to read so simplify it by putting the s5p_aes_complete() only within ISR level. Reported-by: Nathan Royce Cc: # v4.10.x: 07de4bc88c crypto: s5p-sss - Fix completing Cc: # v4.10.x Signed-off-by: Krzysztof Kozlowski Signed-off-by: Herbert Xu --- drivers/crypto/s5p-sss.c | 127 +++++++++++++++++++++++++-------------- 1 file changed, 82 insertions(+), 45 deletions(-) diff --git a/drivers/crypto/s5p-sss.c b/drivers/crypto/s5p-sss.c index a668286d62cb..1b9da3dc799b 100644 --- a/drivers/crypto/s5p-sss.c +++ b/drivers/crypto/s5p-sss.c @@ -270,7 +270,7 @@ static void s5p_sg_copy_buf(void *buf, struct scatterlist *sg, scatterwalk_done(&walk, out, 0); } -static void s5p_aes_complete(struct s5p_aes_dev *dev, int err) +static void s5p_sg_done(struct s5p_aes_dev *dev) { if (dev->sg_dst_cpy) { dev_dbg(dev->dev, @@ -281,8 +281,11 @@ static void s5p_aes_complete(struct s5p_aes_dev *dev, int err) } s5p_free_sg_cpy(dev, &dev->sg_src_cpy); s5p_free_sg_cpy(dev, &dev->sg_dst_cpy); +} - /* holding a lock outside */ +/* Calls the completion. Cannot be called with dev->lock hold. */ +static void s5p_aes_complete(struct s5p_aes_dev *dev, int err) +{ dev->req->base.complete(&dev->req->base, err); dev->busy = false; } @@ -368,51 +371,44 @@ exit: } /* - * Returns true if new transmitting (output) data is ready and its - * address+length have to be written to device (by calling - * s5p_set_dma_outdata()). False otherwise. + * Returns -ERRNO on error (mapping of new data failed). + * On success returns: + * - 0 if there is no more data, + * - 1 if new transmitting (output) data is ready and its address+length + * have to be written to device (by calling s5p_set_dma_outdata()). */ -static bool s5p_aes_tx(struct s5p_aes_dev *dev) +static int s5p_aes_tx(struct s5p_aes_dev *dev) { - int err = 0; - bool ret = false; + int ret = 0; s5p_unset_outdata(dev); if (!sg_is_last(dev->sg_dst)) { - err = s5p_set_outdata(dev, sg_next(dev->sg_dst)); - if (err) - s5p_aes_complete(dev, err); - else - ret = true; - } else { - s5p_aes_complete(dev, err); - - dev->busy = true; - tasklet_schedule(&dev->tasklet); + ret = s5p_set_outdata(dev, sg_next(dev->sg_dst)); + if (!ret) + ret = 1; } return ret; } /* - * Returns true if new receiving (input) data is ready and its - * address+length have to be written to device (by calling - * s5p_set_dma_indata()). False otherwise. + * Returns -ERRNO on error (mapping of new data failed). + * On success returns: + * - 0 if there is no more data, + * - 1 if new receiving (input) data is ready and its address+length + * have to be written to device (by calling s5p_set_dma_indata()). */ -static bool s5p_aes_rx(struct s5p_aes_dev *dev) +static int s5p_aes_rx(struct s5p_aes_dev *dev/*, bool *set_dma*/) { - int err; - bool ret = false; + int ret = 0; s5p_unset_indata(dev); if (!sg_is_last(dev->sg_src)) { - err = s5p_set_indata(dev, sg_next(dev->sg_src)); - if (err) - s5p_aes_complete(dev, err); - else - ret = true; + ret = s5p_set_indata(dev, sg_next(dev->sg_src)); + if (!ret) + ret = 1; } return ret; @@ -422,33 +418,73 @@ static irqreturn_t s5p_aes_interrupt(int irq, void *dev_id) { struct platform_device *pdev = dev_id; struct s5p_aes_dev *dev = platform_get_drvdata(pdev); - bool set_dma_tx = false; - bool set_dma_rx = false; + int err_dma_tx = 0; + int err_dma_rx = 0; + bool tx_end = false; unsigned long flags; uint32_t status; + int err; spin_lock_irqsave(&dev->lock, flags); + /* + * Handle rx or tx interrupt. If there is still data (scatterlist did not + * reach end), then map next scatterlist entry. + * In case of such mapping error, s5p_aes_complete() should be called. + * + * If there is no more data in tx scatter list, call s5p_aes_complete() + * and schedule new tasklet. + */ status = SSS_READ(dev, FCINTSTAT); if (status & SSS_FCINTSTAT_BRDMAINT) - set_dma_rx = s5p_aes_rx(dev); - if (status & SSS_FCINTSTAT_BTDMAINT) - set_dma_tx = s5p_aes_tx(dev); + err_dma_rx = s5p_aes_rx(dev); + + if (status & SSS_FCINTSTAT_BTDMAINT) { + if (sg_is_last(dev->sg_dst)) + tx_end = true; + err_dma_tx = s5p_aes_tx(dev); + } SSS_WRITE(dev, FCINTPEND, status); - /* - * Writing length of DMA block (either receiving or transmitting) - * will start the operation immediately, so this should be done - * at the end (even after clearing pending interrupts to not miss the - * interrupt). - */ - if (set_dma_tx) - s5p_set_dma_outdata(dev, dev->sg_dst); - if (set_dma_rx) - s5p_set_dma_indata(dev, dev->sg_src); + if (err_dma_rx < 0) { + err = err_dma_rx; + goto error; + } + if (err_dma_tx < 0) { + err = err_dma_tx; + goto error; + } + if (tx_end) { + s5p_sg_done(dev); + + spin_unlock_irqrestore(&dev->lock, flags); + + s5p_aes_complete(dev, 0); + dev->busy = true; + tasklet_schedule(&dev->tasklet); + } else { + /* + * Writing length of DMA block (either receiving or + * transmitting) will start the operation immediately, so this + * should be done at the end (even after clearing pending + * interrupts to not miss the interrupt). + */ + if (err_dma_tx == 1) + s5p_set_dma_outdata(dev, dev->sg_dst); + if (err_dma_rx == 1) + s5p_set_dma_indata(dev, dev->sg_src); + + spin_unlock_irqrestore(&dev->lock, flags); + } + + return IRQ_HANDLED; + +error: + s5p_sg_done(dev); spin_unlock_irqrestore(&dev->lock, flags); + s5p_aes_complete(dev, err); return IRQ_HANDLED; } @@ -597,8 +633,9 @@ outdata_error: s5p_unset_indata(dev); indata_error: - s5p_aes_complete(dev, err); + s5p_sg_done(dev); spin_unlock_irqrestore(&dev->lock, flags); + s5p_aes_complete(dev, err); } static void s5p_tasklet_cb(unsigned long data)