[PATCH 3/3] crypto: Add Allwinner Security System crypto accelerator
Corentin LABBE
clabbe.montjoie at gmail.com
Sun May 25 04:58:39 PDT 2014
Le 24/05/2014 14:00, Marek Vasut a écrit :
> On Thursday, May 22, 2014 at 05:09:56 PM, LABBE Corentin wrote:
>
> Do I have to repeat myself ? :)
No, sorry for the commit message, I begin to use git
Generally I agree all your remarks with some comments below.
>
>> Signed-off-by: LABBE Corentin <clabbe.montjoie at gmail.com>
>> ---
>> drivers/crypto/Kconfig | 49 ++
>> drivers/crypto/Makefile | 1 +
>> drivers/crypto/sunxi-ss.c | 1476
>> +++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 1526
>> insertions(+)
>> create mode 100644 drivers/crypto/sunxi-ss.c
>>
>> diff --git a/drivers/crypto/Kconfig b/drivers/crypto/Kconfig
>> index 03ccdb0..5ea0922 100644
>> --- a/drivers/crypto/Kconfig
>> +++ b/drivers/crypto/Kconfig
>> @@ -418,4 +418,53 @@ config CRYPTO_DEV_MXS_DCP
>> To compile this driver as a module, choose M here: the module
>> will be called mxs-dcp.
>>
>> +config CRYPTO_DEV_SUNXI_SS
>> + tristate "Support for Allwinner Security System cryptographic
>> accelerator" + depends on ARCH_SUNXI
>> + help
>> + Some Allwinner processors have a crypto accelerator named
>> + Security System. Select this if you want to use it.
>> +
>> + To compile this driver as a module, choose M here: the module
>> + will be called sunxi-ss.
>> +
>> +if CRYPTO_DEV_SUNXI_SS
>> +config CRYPTO_DEV_SUNXI_SS_PRNG
>> + bool "Security System PRNG"
>> + select CRYPTO_RNG2
>> + help
>> + If you enable this option, the SS will provide a pseudo random
>> + number generator.
>> +config CRYPTO_DEV_SUNXI_SS_MD5
>> + bool "Security System MD5"
>> + select CRYPTO_MD5
>> + help
>> + If you enable this option, the SS will provide MD5 hardware
>> + acceleration.
>
> This is one IP block and it provides 5 algorithms. Put it under one config
> option please.
I want to let the user choose what it want to be used. Someone could want only to accelerate md5 and to not use the PRNG.
Lots of other hw crypto driver do the same.
>
> Also, just shorted this to CONFIG_CRYPTO_SUNXI_SS , the _DEV stuff in the name
> is useless.
I think not, most of cryptographic hardware driver begin with CRYPTO_DEV (CRYPTO_DEV_PADLOCK, CRYPTO_DEV_GEODE, CRYPTO_DEV_TALITOS etc...), only S390 does not have a _DEV.
,
>
> [...]
>
>> diff --git a/drivers/crypto/sunxi-ss.c b/drivers/crypto/sunxi-ss.c
>> new file mode 100644
>> index 0000000..bbf57bc
>> --- /dev/null
>> +++ b/drivers/crypto/sunxi-ss.c
>> @@ -0,0 +1,1476 @@
>> +/*
>> + * sunxi-ss.c - hardware cryptographic accelerator for Allwinner A20 SoC
>
> Why can this not be generic Allwinner-all-chips driver ? Does the IP block
> change that dramatically between the chips?
As I said in my introduction email, lots of allwinner chips seems to have the same crypto device.
But since I do not own any of those hardware, and in most case does not have a datasheet, I only assume support for A20.
I will add this comment in the header of the driver.
>
> [...]
>
>> +#ifdef CONFIG_CRYPTO_DEV_SUNXI_SS_MD5
>> +#include <crypto/md5.h>
>> +#define SUNXI_SS_HASH_COMMON
>> +#endif
>> +#ifdef CONFIG_CRYPTO_DEV_SUNXI_SS_SHA1
>> +#include <crypto/sha.h>
>> +#define SUNXI_SS_HASH_COMMON
>> +#endif
>> +#ifdef SUNXI_SS_HASH_COMMON
>> +#include <crypto/hash.h>
>> +#include <crypto/internal/hash.h>
>> +#endif
>> +#ifdef CONFIG_CRYPTO_DEV_SUNXI_SS_AES
>> +#include <crypto/aes.h>
>> +#endif
>> +
>> +#ifdef CONFIG_CRYPTO_DEV_SUNXI_SS_3DES
>> +#define SUNXI_SS_DES
>> +#endif
>> +#ifdef CONFIG_CRYPTO_DEV_SUNXI_SS_DES
>> +#define SUNXI_SS_DES
>> +#endif
>> +#ifdef SUNXI_SS_DES
>> +#include <crypto/des.h>
>> +#endif
>
> Please discard this mayhem when getting rid of all the configuration option.
>
>> +#ifdef CONFIG_CRYPTO_DEV_SUNXI_SS_PRNG
>> +#include <crypto/internal/rng.h>
>> +
>> +struct prng_context {
>> + u8 seed[192/8];
>> + unsigned int slen;
>> +};
>> +#endif
>> +
>> +#define SUNXI_SS_CTL 0x00
>> +#define SUNXI_SS_KEY0 0x04
>> +#define SUNXI_SS_KEY1 0x08
>> +#define SUNXI_SS_KEY2 0x0C
>> +#define SUNXI_SS_KEY3 0x10
>> +#define SUNXI_SS_KEY4 0x14
>> +#define SUNXI_SS_KEY5 0x18
>> +#define SUNXI_SS_KEY6 0x1C
>> +#define SUNXI_SS_KEY7 0x20
>> +
>> +#define SUNXI_SS_IV0 0x24
>> +#define SUNXI_SS_IV1 0x28
>> +#define SUNXI_SS_IV2 0x2C
>> +#define SUNXI_SS_IV3 0x30
>> +
>> +#define SUNXI_SS_CNT0 0x34
>> +#define SUNXI_SS_CNT1 0x38
>> +#define SUNXI_SS_CNT2 0x3C
>> +#define SUNXI_SS_CNT3 0x40
>> +
>> +#define SUNXI_SS_FCSR 0x44
>> +#define SUNXI_SS_ICSR 0x48
>> +
>> +#define SUNXI_SS_MD0 0x4C
>> +#define SUNXI_SS_MD1 0x50
>> +#define SUNXI_SS_MD2 0x54
>> +#define SUNXI_SS_MD3 0x58
>> +#define SUNXI_SS_MD4 0x5C
>> +
>> +#define SS_RXFIFO 0x200
>> +#define SS_TXFIFO 0x204
>
> You don't have much consistency in the register naming scheme, please fix this
> somehow. I suppose renaming the SS_[RT]XFIFO registers to SUNXI_SS_[RT]XFIFO is
> a good idea.
>
>> +/* SUNXI_SS_CTL configuration values */
>> +
>> +/* AES/DES/3DES key select - bits 24-27 */
>> +#define SUNXI_SS_KEYSELECT_KEYN (0 << 24)
>
> Uh oh , you ORR some values with this flag, which is always zero. This seems
> broken.
>
> [...]
>
>> +/* SS Method - bits 4-6 */
>> +#define SUNXI_OP_AES (0 << 4)
>> +#define SUNXI_OP_DES (1 << 4)
>> +#define SUNXI_OP_3DES (2 << 4)
>> +#define SUNXI_OP_SHA1 (3 << 4)
>> +#define SUNXI_OP_MD5 (4 << 4)
>> +#define SUNXI_OP_PRNG (5 << 4)
>> +
>> +/* Data end bit - bit 2 */
>> +#define SUNXI_SS_DATA_END BIT(2)
>
> Please use the BIT() macros in consistent fashion. Either use it or don't use it
> (I'd much rather see it not used), but don't mix the stuff :-(
>
> [...]
>
>> +/* General notes:
>> + * I cannot use a key/IV cache because each time one of these change ALL
>> stuff + * need to be re-writed.
>> + * And for example, with dm-crypt IV changes on each request.
>> + *
>> + * After each request the device must be disabled.
>
> This really isn't quite clear, can you please expand the comment so it's more
> understandtable ?
>
>> + * For performance reason, we use writel_relaxed/read_relaxed for all
>> + * operations on RX and TX FIFO.
>> + * For all other registers, we use writel.
>> + * See http://permalink.gmane.org/gmane.linux.ports.arm.kernel/117644
>> + * and http://permalink.gmane.org/gmane.linux.ports.arm.kernel/117640
>> + * */
>
> I will not review the algos, they need to be AHASH/ABLKCIPHER algos. See the
> reasoning further down below.
>
> [...]
>
>> +/*========================================================================
>> ====*/
>> +/*=======================================================================
>> =====*/ +static struct crypto_alg sunxi_des3_alg = {
>> + .cra_name = "cbc(des3_ede)",
>> + .cra_driver_name = "cbc-des3-sunxi-ss",
>> + .cra_priority = 300,
>> + .cra_blocksize = DES3_EDE_BLOCK_SIZE,
>> + .cra_flags = CRYPTO_ALG_TYPE_BLKCIPHER,
>
> This must be rewritten to be ABLKCIPHER.
>
>> + .cra_ctxsize = sizeof(struct sunxi_req_ctx),
>> + .cra_module = THIS_MODULE,
>> + .cra_type = &crypto_blkcipher_type,
>> + .cra_init = sunxi_des_init,
>> + .cra_exit = sunxi_des_exit,
>> + .cra_alignmask = 3,
>> + .cra_u.blkcipher = {
>> + .min_keysize = DES3_EDE_KEY_SIZE,
>> + .max_keysize = DES3_EDE_KEY_SIZE,
>> + .ivsize = 8,
>> + .setkey = sunxi_des3_setkey,
>> + .encrypt = sunxi_des3_cbc_encrypt,
>> + .decrypt = sunxi_des3_cbc_decrypt,
>> + }
>> +};
>> +
>> +#endif /* CONFIG_CRYPTO_DEV_SUNXI_SS_3DES */
>> +
>> +#ifdef CONFIG_CRYPTO_DEV_SUNXI_SS_PRNG
>> +/*========================================================================
>> ====*/
>> +/*=======================================================================
>> =====*/ +static int sunxi_ss_rng_get_random(struct crypto_rng *tfm, u8
>> *rdata, + unsigned int dlen)
>> +{
>> + struct prng_context *ctx = crypto_tfm_ctx((struct crypto_tfm *)tfm);
>
> Use crypto_rng_ctx() here . The cast is plain wrong. I know it works due to how
> the structures are defined, but this is not right.
>
>> + unsigned int i;
>> + u32 mode = 0;
>> + u32 v;
>> +
>> + dev_dbg(ss_ctx->dev, "DEBUG %s dlen=%u\n", __func__, dlen);
>> +
>> + if (dlen == 0 || rdata == NULL)
>> + return 0;
>
> Return -EINVAL or somesuch here, no?
>
>> + mode |= SUNXI_OP_PRNG;
>> + mode |= SUNXI_PRNG_ONESHOT;
>> + mode |= SUNXI_SS_ENABLED;
>> +
>> + mutex_lock(&lock);
>> + writel(mode, ss_ctx->base + SUNXI_SS_CTL);
>> +
>> + for (i = 0; i < ctx->slen; i += 4) {
>> + v = *(u32 *)(ctx->seed + i);
>
> Define the .seed field as u32 if you actually want to access it as u32. You are
> very lucky you didn't trigger alignment faults with this yet.
>
>> + dev_dbg(ss_ctx->dev, "DEBUG Seed %d %x\n", i, v);
>> + }
>
> But this debug instrumentation looks quite useless anyway.
As I said in my introduction mail, I cannot get PRNG to work, so it is the reason of lots of dev_dbg()
Anyway, I will remove them.
>
>> + for (i = 0; i < ctx->slen && i < 192/8 && i < 16; i += 4) {
>> + v = *(u32 *)(ctx->seed + i);
>> + dev_dbg(ss_ctx->dev, "DEBUG Seed %d %x\n", i, v);
>> + writel(v, ss_ctx->base + SUNXI_SS_KEY0 + i);
>> + }
>> +
>> + mode |= SUNXI_PRNG_START;
>> + writel(mode, ss_ctx->base + SUNXI_SS_CTL);
>> + for (i = 0; i < 4; i++) {
>> + v = readl(ss_ctx->base + SUNXI_SS_CTL);
>> + dev_dbg(ss_ctx->dev, "DEBUG CTL %x %x\n", mode, v);
>> + }
>> + for (i = 0; i < dlen && i < 160 / 8; i += 4) {
>> + v = readl(ss_ctx->base + SUNXI_SS_MD0 + i);
>> + *(u32 *)(rdata + i) = v;
>> + dev_dbg(ss_ctx->dev, "DEBUG MD%d %x\n", i, v);
>> + }
>
> Drop the debug instrumentation and define the seed buffer as u32.
>
>> + writel(0, ss_ctx->base + SUNXI_SS_CTL);
>> + mutex_unlock(&lock);
>> + return dlen;
>> +}
>> +
>> +/*========================================================================
>> ====*/
>> +/*=======================================================================
>> =====*/ +static int sunxi_ss_rng_reset(struct crypto_rng *tfm, u8 *seed,
>> + unsigned int slen)
>> +{
>> + struct prng_context *ctx = crypto_tfm_ctx((struct crypto_tfm *)tfm);
>
> Use crypto_rng_ctx() here . The cast is plain wrong.
>
>> + dev_dbg(ss_ctx->dev, "DEBUG %s slen=%u\n", __func__, slen);
>> + memcpy(ctx->seed, seed, slen);
>> + ctx->slen = slen;
>> + return 0;
>> +}
>
> [...]
>
>> +static int sunxi_ss_probe(struct platform_device *pdev)
>> +{
>> + struct resource *res;
>> + u32 v;
>> + int err;
>> + unsigned long cr;
>> +
>> + if (!pdev->dev.of_node)
>> + return -ENODEV;
>> +
>> + memset(ss_ctx, 0, sizeof(struct sunxi_ss_ctx));
>
> Static data are always zeroed out by default, no ?
>
> If you just so accidentally happen to probe() this driver twice, you will get
> some seriously ugly surprise here, since you zero the data out without checking
> if the device might have already been probed. A much better idea would be to
> define a static struct sunxi_ss_ctx *ss_ctx; and then
>
> if (ss_ctx) {
> dev_err(&pdev->dev, "Only one instance allowed");
> return -ENODEV;
> }
> ss_ctx = devm_kzalloc(&pdev->dev, sizeof(*ss_ctx), GFP_KERNEL);
>
> Also note the use of sizeof(*ss_ctx), this will allow for this line to not be
> changed in case the type of *ss_ctx ever changes in the future. So use it.
>
>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> + if (res == NULL) {
>> + dev_err(&pdev->dev, "Cannot get the MMIO ressource\n");
>> + /* TODO PTR_ERR ? */
>
> Fix the TODOs .
>
>> + return -ENXIO;
>> + }
>> + ss_ctx->base = devm_ioremap_resource(&pdev->dev, res);
>> + if (IS_ERR(ss_ctx->base)) {
>> + dev_err(&pdev->dev, "Cannot request MMIO\n");
>> + return PTR_ERR(ss_ctx->base);
>> + }
>
> Actually, this is the pattern. You don't need to check for IS_ERR(res) before
> calling devm_ioremap_resource().
>
> res = platform_get_resource();
> base = devm_ioremap_resource(&pdev->dev, res)
> if (IS_ERR(base))
> return PTR_ERR(base);
>
>> + /* TODO Does this information could be usefull ? */
>> + writel(SUNXI_SS_ENABLED, ss_ctx->base + SUNXI_SS_CTL);
>> + v = readl(ss_ctx->base + SUNXI_SS_CTL);
>> + v >>= 16;
>> + v &= 0x07;
>> + dev_info(&pdev->dev, "Die ID %d\n", v);
>> + writel(0, ss_ctx->base + SUNXI_SS_CTL);
>
> You are configuring hardware before enabling clock for that hardware. That does
> not seem right.
>
>> + ss_ctx->ssclk = devm_clk_get(&pdev->dev, "mod");
>> + if (IS_ERR(ss_ctx->ssclk)) {
>> + err = PTR_ERR(ss_ctx->ssclk);
>> + dev_err(&pdev->dev, "Cannot get SS clock err=%d\n", err);
>> + return err;
>> + }
>> + dev_dbg(&pdev->dev, "clock ss acquired\n");
>> +
>> + ss_ctx->busclk = devm_clk_get(&pdev->dev, "ahb");
>> + if (IS_ERR(ss_ctx->busclk)) {
>> + err = PTR_ERR(ss_ctx->busclk);
>> + dev_err(&pdev->dev, "Cannot get AHB SS clock err=%d\n", err);
>> + return err;
>> + }
>> + dev_dbg(&pdev->dev, "clock ahb_ss acquired\n");
>
> These dev_dbg() aren't really useful.
>
>> + /* Enable the clocks */
>> + err = clk_prepare_enable(ss_ctx->busclk);
>> + if (err != 0) {
>> + dev_err(&pdev->dev, "Cannot prepare_enable busclk\n");
>> + return err;
>> + }
>> + err = clk_prepare_enable(ss_ctx->ssclk);
>> + if (err != 0) {
>> + dev_err(&pdev->dev, "Cannot prepare_enable ssclk\n");
>> + clk_disable_unprepare(ss_ctx->busclk);
>> + return err;
>> + }
>> +
>> +#define SUNXI_SS_CLOCK_RATE_BUS (24 * 1000 * 1000)
>> +#define SUNXI_SS_CLOCK_RATE_SS (150 * 1000 * 1000)
>
> Make this const unsigned long or somesuch, the define is horrid and not
> typesafe.
>
>> + /* Check that clock have the correct rates gived in the datasheet */
>> + cr = clk_get_rate(ss_ctx->busclk);
>> + if (cr >= SUNXI_SS_CLOCK_RATE_BUS)
>> + dev_dbg(&pdev->dev, "Clock bus %lu (%lu MHz) (must be >= %u)\n",
>> + cr, cr / 1000000, SUNXI_SS_CLOCK_RATE_BUS);
>> + else
>> + dev_warn(&pdev->dev, "Clock bus %lu (%lu MHz) (must be >=
> %u)\n",
>> + cr, cr / 1000000, SUNXI_SS_CLOCK_RATE_BUS);
>> + cr = clk_get_rate(ss_ctx->ssclk);
>> + if (cr == SUNXI_SS_CLOCK_RATE_SS)
>> + dev_dbg(&pdev->dev, "Clock ss %lu (%lu MHz) (must be <= %u)\n",
>> + cr, cr / 1000000, SUNXI_SS_CLOCK_RATE_SS);
>> + else {
>> + dev_warn(&pdev->dev, "Clock ss is at %lu (%lu MHz) (must be <=
> %u)\n",
>> + cr, cr / 1000000, SUNXI_SS_CLOCK_RATE_SS);
>> + /* Try to set the clock to the maximum allowed */
>> + err = clk_set_rate(ss_ctx->ssclk, SUNXI_SS_CLOCK_RATE_SS);
>> + if (err != 0) {
>> + dev_err(&pdev->dev, "Cannot set clock rate to ssclk\n");
>> + goto label_error_clock;
>> + }
>> + cr = clk_get_rate(ss_ctx->ssclk);
>> + dev_info(&pdev->dev, "Clock ss set to %lu (%lu MHz) (must be >=
> %u)\n",
>> + cr, cr / 1000000, SUNXI_SS_CLOCK_RATE_BUS);
>> + }
>
> Just run clk_set_rate() for both and be done with it.
>
>> + ss_ctx->buf_in = NULL;
>> + ss_ctx->buf_in_size = 0;
>> + ss_ctx->buf_out = NULL;
>> + ss_ctx->buf_out_size = 0;
>
> This is useless, you already did that with devm_kzalloc() above.
>
>> + ss_ctx->dev = &pdev->dev;
>> +
>> + mutex_init(&lock);
>> +
>> +#ifdef CONFIG_CRYPTO_DEV_SUNXI_SS_PRNG
>> + err = crypto_register_alg(&sunxi_ss_prng);
>
> Use crypto_register_algs() when you get rid of those useless #defines for each
> algo.
>
>> + if (err) {
>> + dev_err(&pdev->dev, "crypto_register_alg error\n");
>> + goto label_error_prng;
>> + } else {
>> + dev_info(&pdev->dev, "Registred PRNG\n");
>
> This is useless, you can always check that the thing was probed via /proc/crypto
> . Please remove this dev_into().
>
>> + }
>> +#endif
>> +
>> +#ifdef CONFIG_CRYPTO_DEV_SUNXI_SS_MD5
>> + err = crypto_register_shash(&sunxi_md5_alg);
>
> Do not use shash for such device. This is clearly and ahash (and async in
> general) device. The rule of a thumb here is that you use sync algos only for
> devices which have dedicated instructions for computing the transformation. For
> devices which are attached to some kind of bus, you use async algos (ahash etc).
>
>> + if (err) {
>> + dev_err(&pdev->dev, "Fail to register MD5\n");
>> + goto label_error_md5;
>> + } else {
>> + dev_info(&pdev->dev, "Registred MD5\n");
>> + }
>> +#endif
>> +#ifdef CONFIG_CRYPTO_DEV_SUNXI_SS_SHA1
>> + err = crypto_register_shash(&sunxi_sha1_alg);
>
> DTTO.
>
> I am preparing a document for the crypto API, drop me a PM if you want the
> preliminary version. I don't want to release it until it's more complete as it
> will only produce more confusion throughout the crypto API than there already
> is.
>
> [...]
>
>> +static const struct of_device_id a20ss_crypto_of_match_table[] = {
>> + { .compatible = "allwinner,sun7i-a20-crypto" },
>> + {}
>> +};
>> +MODULE_DEVICE_TABLE(of, a20ss_crypto_of_match_table);
>
> If possible, scrub the a20 nonsense.
>
>> +static struct platform_driver sunxi_ss_driver = {
>> + .probe = sunxi_ss_probe,
>> + .remove = sunxi_ss_remove,
>> + .driver = {
>> + .owner = THIS_MODULE,
>> + .name = "sunxi-ss",
>> + .of_match_table = a20ss_crypto_of_match_table,
>> + },
>> +};
>> +
>> +module_platform_driver(sunxi_ss_driver);
>> +
>> +MODULE_DESCRIPTION("Allwinner Security System crypto accelerator");
>> +MODULE_LICENSE("GPL");
>> +MODULE_AUTHOR("Corentin LABBE <clabbe.montjoie at gmail.com>");
>
> MODULE_ALIAS is missing I think.
>
Thanks for your review.
More information about the linux-arm-kernel
mailing list