From: Corentin Labbe <clabbe.montjoie@gmail.com> To: Maxime Ripard <mripard@kernel.org> Cc: mark.rutland@arm.com, devicetree@vger.kernel.org, herbert@gondor.apana.org.au, linux-sunxi@googlegroups.com, linux@armlinux.org.uk, linux-kernel@vger.kernel.org, wens@csie.org, robh+dt@kernel.org, linux-crypto@vger.kernel.org, davem@davemloft.net, linux-arm-kernel@lists.infradead.org Subject: Re: [PATCH 2/9] crypto: Add Allwinner sun8i-ce Crypto Engine Date: Sat, 7 Sep 2019 21:04:08 +0200 Message-ID: <20190907190408.GE2628@Red> (raw) In-Reply-To: <20190907081951.v2huvhm44jfprfop@flea> On Sat, Sep 07, 2019 at 11:19:51AM +0300, Maxime Ripard wrote: > Hi, > > I can't really comment on the crypto side, so my review is going to be > pretty boring. > > On Fri, Sep 06, 2019 at 08:45:44PM +0200, Corentin Labbe wrote: > > +static const struct ce_variant ce_h3_variant = { > > + .alg_cipher = { CE_ID_NOTSUPP, CE_ALG_AES, CE_ALG_DES, CE_ALG_3DES, > > + CE_ID_NOTSUPP, > > + }, > > As far as I can see, it's always the same value, so I'm not sure why > it's a parameter. > No it is not the same value. If by same value you mean "the list is the same accross variant", it will be different when I will add CTS/CTR/XTS. Note that .alg_cipher was already different on h6, since I forgot to remove the RAES. So it will be the same on PATChv2, but again il will be different after. > > + .op_mode = { CE_ID_NOTSUPP, CE_OP_ECB, CE_OP_CBC > > + }, > > Ditto > > > + .intreg = CE_ISR, > > Ditto > > > + .maxflow = 4, > > Ditto > Both .intreg and .maxflow are remains of sun8i-ss support. I will remove them. > > + .ce_clks = { > > + { "ahb", 200000000 }, > > This is the default IIRC, and the clock driver will ignore any clock > rate change on it anyway, so the clock rate is pretty much useless > there. > > > + { "mod", 48000000 }, > > 48MHz seems pretty slow, especially compared to the other rates you > have listed there. Is that on purpose? On H3, the value used on others SoC bring to random fail. I will add a comment. > > Also, I'm not sure what is the point of having the clocks names be > parameters there as well. It's constant across all the compatibles, > the only thing that isn't is the number of clocks and the module clock > rate. It's what you should have in there. > Since the datasheet give some max frequency, I think I will add a max_freq and add a check to verify if the clock is in the right range > > + } > > +}; > > + > > +static const struct ce_variant ce_h5_variant = { > > + .alg_cipher = { CE_ID_NOTSUPP, CE_ALG_AES, CE_ALG_DES, CE_ALG_3DES, > > + CE_ID_NOTSUPP, > > + }, > > + .op_mode = { CE_ID_NOTSUPP, CE_OP_ECB, CE_OP_CBC > > + }, > > + .intreg = CE_ISR, > > + .maxflow = 4, > > + .ce_clks = { > > + { "ahb", 200000000 }, > > + { "mod", 300000000 }, > > + } > > +}; > > + > > +static const struct ce_variant ce_h6_variant = { > > + .alg_cipher = { CE_ID_NOTSUPP, CE_ALG_AES, CE_ALG_DES, CE_ALG_3DES, > > + CE_ALG_RAES, > > + }, > > + .op_mode = { CE_ID_NOTSUPP, CE_OP_ECB, CE_OP_CBC > > + }, > > + .model = CE_v2, > > Can't that be derived from the version register and / or the > compatible? This seems to be redundant with each. > I could use the compatible, but I want to avoid a string comparison on each request. > > + .intreg = CE_ISR, > > + .maxflow = 4, > > + .ce_clks = { > > + { "ahb", 200000000 }, > > + { "mod", 300000000 }, > > + { "mbus", 400000000 }, > > That rate is going to be ignored as well. > > > + } > > +}; > > + > > +static const struct ce_variant ce_a64_variant = { > > + .alg_cipher = { CE_ID_NOTSUPP, CE_ALG_AES, CE_ALG_DES, CE_ALG_3DES, > > + CE_ID_NOTSUPP, > > + }, > > + .op_mode = { CE_ID_NOTSUPP, CE_OP_ECB, CE_OP_CBC > > + }, > > + .intreg = CE_ISR, > > + .maxflow = 4, > > + .ce_clks = { > > + { "ahb", 200000000 }, > > + { "mod", 300000000 }, > > + } > > +}; > > You should order your variants by alphabetical order. > Will do. > > +static const struct ce_variant ce_r40_variant = { > > + .alg_cipher = { CE_ID_NOTSUPP, CE_ALG_AES, CE_ALG_DES, CE_ALG_3DES, > > + CE_ID_NOTSUPP, > > + }, > > + .op_mode = { CE_ID_NOTSUPP, CE_OP_ECB, CE_OP_CBC > > + }, > > + .intreg = CE_ISR, > > + .maxflow = 4, > > + .ce_clks = { > > + { "ahb", 200000000 }, > > + { "mod", 300000000 }, > > + } > > +}; > > + > > ... > > > +int sun8i_ce_get_engine_number(struct sun8i_ce_dev *ce) > > +{ > > + return atomic_inc_return(&ce->flow) % ce->variant->maxflow; > > +} > > I'm not sure what this is supposed to be doing, but that mod there > seems pretty dangerous. > > ... > This mod do a round robin on each channel. I dont see why it is dangerous. > > +static int sun8i_ce_probe(struct platform_device *pdev) > > +{ > > + struct resource *res; > > + u32 v; > > + int err, i, ce_method, id, irq; > > + unsigned long cr; > > + struct sun8i_ce_dev *ce; > > + > > + if (!pdev->dev.of_node) > > + return -ENODEV; > > This is pretty much guaranteed already > Ok, removed > > + ce = devm_kzalloc(&pdev->dev, sizeof(*ce), GFP_KERNEL); > > + if (!ce) > > + return -ENOMEM; > > + > > + ce->variant = of_device_get_match_data(&pdev->dev); > > + if (!ce->variant) { > > + dev_err(&pdev->dev, "Missing Crypto Engine variant\n"); > > + return -EINVAL; > > + } > > + > > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > > + ce->base = devm_ioremap_resource(&pdev->dev, res); > > + if (IS_ERR(ce->base)) { > > + err = PTR_ERR(ce->base); > > + dev_err(&pdev->dev, "Cannot request MMIO err=%d\n", err); > > + return err; > > ioremap_resource already prints an error message on failure, so this > is redundant. > Will remove. > > + } > > + > > + for (i = 0; i < CE_MAX_CLOCKS; i++) { > > + if (!ce->variant->ce_clks[i].name) > > + continue; > > + dev_info(&pdev->dev, "Get %s clock\n", ce->variant->ce_clks[i].name); > > There's no reason to print this at the info level > Will remove. > > + ce->ceclks[i] = devm_clk_get(&pdev->dev, ce->variant->ce_clks[i].name); > > + if (IS_ERR(ce->ceclks[i])) { > > + err = PTR_ERR(ce->ceclks[i]); > > + dev_err(&pdev->dev, "Cannot get %s CE clock err=%d\n", > > + ce->variant->ce_clks[i].name, err); > > + } > > + cr = clk_get_rate(ce->ceclks[i]); > > So on error you'd call clk_get_rate on the clock still? That seems > pretty fragile, you should return there, it's a hard error. > I will add the missing "return err" > > + if (ce->variant->ce_clks[i].freq) { > > + dev_info(&pdev->dev, "Set %s clock to %lu (%lu Mhz) from %lu (%lu Mhz)\n", > > + ce->variant->ce_clks[i].name, > > + ce->variant->ce_clks[i].freq, > > + ce->variant->ce_clks[i].freq / 1000000, > > + cr, > > + cr / 1000000); > > Same remark about that message than the previous one. > > > + err = clk_set_rate(ce->ceclks[i], ce->variant->ce_clks[i].freq); > > + if (err) > > + dev_err(&pdev->dev, "Fail to set %s clk speed to %lu\n", > > + ce->variant->ce_clks[i].name, > > + ce->variant->ce_clks[i].freq); > > + } else { > > + dev_info(&pdev->dev, "%s run at %lu\n", > > + ce->variant->ce_clks[i].name, cr); > > Ditto. > > > + } > > + err = clk_prepare_enable(ce->ceclks[i]); > > Do you really need this right now though? > Not sure to understand, why I shouldnt do it now ? Does it is related to your pm_runtime remark below ? My feeling was to submit the driver without PM and convert it after. > > + if (err) { > > + dev_err(&pdev->dev, "Cannot prepare_enable %s\n", > > + ce->variant->ce_clks[i].name); > > + return err; > > + } > > + } > > + > > + /* Get Non Secure IRQ */ > > + irq = platform_get_irq(pdev, 0); > > + if (irq < 0) { > > + dev_err(ce->dev, "Cannot get NS IRQ\n"); > > + return irq; > > + } > > + > > + err = devm_request_irq(&pdev->dev, irq, ce_irq_handler, 0, > > + "sun8i-ce-ns", ce); > > + if (err < 0) { > > + dev_err(ce->dev, "Cannot request NS IRQ\n"); > > + return err; > > + } > > + > > + ce->reset = devm_reset_control_get_optional(&pdev->dev, "ahb"); > > + if (IS_ERR(ce->reset)) { > > + if (PTR_ERR(ce->reset) == -EPROBE_DEFER) > > + return PTR_ERR(ce->reset); > > + dev_info(&pdev->dev, "No reset control found\n"); > > It's not optional though. > I dont understand why. > > + ce->reset = NULL; > > + } > > + > > + err = reset_control_deassert(ce->reset); > > + if (err) { > > + dev_err(&pdev->dev, "Cannot deassert reset control\n"); > > + goto error_clk; > > + } > > Again, you don't really need this at this moment. Using runtime_pm > would make more sense. > > > + v = readl(ce->base + CE_CTR); > > + v >>= 16; > > + v &= 0x07; > > This should be in a define > Will fix. > > + dev_info(&pdev->dev, "CE_NS Die ID %x\n", v); > > And if that really makes sense to print it, the error message should > be made less cryptic. > Will fix. > > + > > + ce->dev = &pdev->dev; > > + platform_set_drvdata(pdev, ce); > > + > > + mutex_init(&ce->mlock); > > + > > + ce->chanlist = devm_kcalloc(ce->dev, ce->variant->maxflow, > > + sizeof(struct sun8i_ce_flow), GFP_KERNEL); > > + if (!ce->chanlist) { > > + err = -ENOMEM; > > + goto error_flow; > > + } > > + > > + for (i = 0; i < ce->variant->maxflow; i++) { > > + init_completion(&ce->chanlist[i].complete); > > + mutex_init(&ce->chanlist[i].lock); > > + > > + ce->chanlist[i].engine = crypto_engine_alloc_init(ce->dev, true); > > + if (!ce->chanlist[i].engine) { > > + dev_err(ce->dev, "Cannot allocate engine\n"); > > + i--; > > + goto error_engine; > > + } > > + err = crypto_engine_start(ce->chanlist[i].engine); > > + if (err) { > > + dev_err(ce->dev, "Cannot start engine\n"); > > + goto error_engine; > > + } > > + ce->chanlist[i].tl = dma_alloc_coherent(ce->dev, > > + sizeof(struct ce_task), > > + &ce->chanlist[i].t_phy, > > + GFP_KERNEL); > > + if (!ce->chanlist[i].tl) { > > + dev_err(ce->dev, "Cannot get DMA memory for task %d\n", > > + i); > > + err = -ENOMEM; > > + goto error_engine; > > + } > > + } > > All this initialization should be done before calling > request_irq. You're using some of those fields in your handler. > No interrupt could fire, since algorithms are still not registred. > > + > > +#ifdef CONFIG_CRYPTO_DEV_SUN8I_CE_DEBUG > > + ce->dbgfs_dir = debugfs_create_dir("sun8i-ce", NULL); > > + if (IS_ERR_OR_NULL(ce->dbgfs_dir)) { > > + dev_err(ce->dev, "Fail to create debugfs dir"); > > + err = -ENOMEM; > > + goto error_engine; > > + } > > + ce->dbgfs_stats = debugfs_create_file("stats", 0444, > > + ce->dbgfs_dir, ce, > > + &sun8i_ce_debugfs_fops); > > + if (IS_ERR_OR_NULL(ce->dbgfs_stats)) { > > + dev_err(ce->dev, "Fail to create debugfs stat"); > > + err = -ENOMEM; > > + goto error_debugfs; > > + } > > +#endif > > + for (i = 0; i < ARRAY_SIZE(ce_algs); i++) { > > + ce_algs[i].ce = ce; > > + switch (ce_algs[i].type) { > > + case CRYPTO_ALG_TYPE_SKCIPHER: > > + id = ce_algs[i].ce_algo_id; > > + ce_method = ce->variant->alg_cipher[id]; > > + if (ce_method == CE_ID_NOTSUPP) { > > + dev_info(ce->dev, > > + "DEBUG: Algo of %s not supported\n", > > + ce_algs[i].alg.skcipher.base.cra_name); > > + ce_algs[i].ce = NULL; > > + break; > > + } > > + id = ce_algs[i].ce_blockmode; > > + ce_method = ce->variant->op_mode[id]; > > + if (ce_method == CE_ID_NOTSUPP) { > > + dev_info(ce->dev, "DEBUG: Blockmode of %s not supported\n", > > + ce_algs[i].alg.skcipher.base.cra_name); > > + ce_algs[i].ce = NULL; > > + break; > > + } > > + dev_info(ce->dev, "DEBUG: Register %s\n", > > + ce_algs[i].alg.skcipher.base.cra_name); > > + err = crypto_register_skcipher(&ce_algs[i].alg.skcipher); > > + if (err) { > > + dev_err(ce->dev, "Fail to register %s\n", > > + ce_algs[i].alg.skcipher.base.cra_name); > > + ce_algs[i].ce = NULL; > > + goto error_alg; > > + } > > + break; > > + default: > > + dev_err(ce->dev, "ERROR: tryed to register an unknown algo\n"); > > + } > > + } > > + > > + return 0; > > +error_alg: > > + i--; > > + for (; i >= 0; i--) { > > + switch (ce_algs[i].type) { > > + case CRYPTO_ALG_TYPE_SKCIPHER: > > + if (ce_algs[i].ce) > > + crypto_unregister_skcipher(&ce_algs[i].alg.skcipher); > > + break; > > + } > > + } > > +#ifdef CONFIG_CRYPTO_DEV_SUN8I_CE_DEBUG > > +error_debugfs: > > + debugfs_remove_recursive(ce->dbgfs_dir); > > +#endif > > + i = ce->variant->maxflow; > > +error_engine: > > + while (i >= 0) { > > + crypto_engine_exit(ce->chanlist[i].engine); > > + if (ce->chanlist[i].tl) > > + dma_free_coherent(ce->dev, sizeof(struct ce_task), > > + ce->chanlist[i].tl, > > + ce->chanlist[i].t_phy); > > + i--; > > + } > > +error_flow: > > + reset_control_assert(ce->reset); > > +error_clk: > > + for (i = 0; i < CE_MAX_CLOCKS; i++) > > + clk_disable_unprepare(ce->ceclks[i]); > > + return err; > > +} > > So that function takes around 200-250 LoC, this is definitely too > much and should be split into multiple functions. > Will do. Thanks for your review. Regards _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply index Thread overview: 32+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-09-06 18:45 [PATCH 0/9] crypto: add sun8i-ce driver for Allwinner crypto engine Corentin Labbe 2019-09-06 18:45 ` [PATCH 1/9] crypto: Add allwinner subdirectory Corentin Labbe 2019-09-07 3:54 ` Maxime Ripard 2019-09-07 17:53 ` Corentin Labbe 2019-09-06 18:45 ` [PATCH 2/9] crypto: Add Allwinner sun8i-ce Crypto Engine Corentin Labbe 2019-09-07 8:19 ` Maxime Ripard 2019-09-07 19:04 ` Corentin Labbe [this message] 2019-09-09 11:38 ` Maxime Ripard 2019-09-09 13:19 ` Corentin Labbe 2019-09-09 13:59 ` Maxime Ripard 2019-09-06 18:45 ` [PATCH 3/9] dt-bindings: crypto: Add DT bindings documentation for " Corentin Labbe 2019-09-07 4:01 ` Maxime Ripard 2019-09-11 18:31 ` Corentin Labbe 2019-09-12 9:37 ` Maxime Ripard 2019-09-12 20:26 ` Chen-Yu Tsai 2019-09-12 20:33 ` Maxime Ripard 2019-09-12 20:37 ` Chen-Yu Tsai 2019-09-13 12:11 ` Maxime Ripard 2019-09-06 18:45 ` [PATCH 4/9] ARM: dts: sun8i: r40: add crypto engine node Corentin Labbe 2019-09-07 4:02 ` Maxime Ripard 2019-09-07 17:54 ` Corentin Labbe 2019-09-06 18:45 ` [PATCH 5/9] ARM: dts: sun8i: h3: Add Crypto Engine node Corentin Labbe 2019-09-06 18:45 ` [PATCH 6/9] ARM64: dts: allwinner: sun50i: Add Crypto Engine node on A64 Corentin Labbe 2019-09-07 4:02 ` Maxime Ripard 2019-09-07 17:54 ` Corentin Labbe 2019-09-06 18:45 ` [PATCH 7/9] ARM64: dts: allwinner: sun50i: Add crypto engine node on H5 Corentin Labbe 2019-09-06 18:45 ` [PATCH 8/9] ARM64: dts: allwinner: sun50i: Add Crypto Engine node on H6 Corentin Labbe 2019-09-06 18:45 ` [PATCH 9/9] sunxi_defconfig: add new crypto options Corentin Labbe 2019-09-07 4:03 ` Maxime Ripard 2019-09-07 17:55 ` Corentin Labbe 2019-09-13 8:15 ` Corentin Labbe 2019-09-13 12:10 ` Maxime Ripard
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20190907190408.GE2628@Red \ --to=clabbe.montjoie@gmail.com \ --cc=davem@davemloft.net \ --cc=devicetree@vger.kernel.org \ --cc=herbert@gondor.apana.org.au \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-crypto@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-sunxi@googlegroups.com \ --cc=linux@armlinux.org.uk \ --cc=mark.rutland@arm.com \ --cc=mripard@kernel.org \ --cc=robh+dt@kernel.org \ --cc=wens@csie.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
Linux-ARM-Kernel Archive on lore.kernel.org Archives are clonable: git clone --mirror https://lore.kernel.org/linux-arm-kernel/0 linux-arm-kernel/git/0.git git clone --mirror https://lore.kernel.org/linux-arm-kernel/1 linux-arm-kernel/git/1.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-arm-kernel linux-arm-kernel/ https://lore.kernel.org/linux-arm-kernel \ linux-arm-kernel@lists.infradead.org public-inbox-index linux-arm-kernel Example config snippet for mirrors Newsgroup available over NNTP: nntp://nntp.lore.kernel.org/org.infradead.lists.linux-arm-kernel AGPL code for this site: git clone https://public-inbox.org/public-inbox.git