Message ID | 20240930093054.215809-1-pavitrakumarm@vayavyalabs.com |
---|---|
Headers | show |
Series | Add SPAcc Crypto Driver | expand |
On 30/09/2024 11:30, Pavitrakumar M wrote: > Add SPAcc Skcipher support to Synopsys Protocol Accelerator(SPAcc) IP, > which is a crypto accelerator engine. > SPAcc supports ciphers, hashes and AEAD algorithms such as > AES in different modes, SHA variants, AES-GCM, Chacha-poly1305 etc. > ... > + > + /* reset registers */ > + writel(0, spacc->regmap + SPACC_REG_IRQ_CTRL); > + writel(0, spacc->regmap + SPACC_REG_IRQ_EN); > + writel(0xFFFFFFFF, spacc->regmap + SPACC_REG_IRQ_STAT); > + > + writel(0, spacc->regmap + SPACC_REG_SRC_PTR); > + writel(0, spacc->regmap + SPACC_REG_DST_PTR); > + writel(0, spacc->regmap + SPACC_REG_PROC_LEN); > + writel(0, spacc->regmap + SPACC_REG_ICV_LEN); > + writel(0, spacc->regmap + SPACC_REG_PRE_AAD_LEN); > + > + pdu_ddt_free(&ddt); > + dma_free_coherent(get_ddt_device(), 256, virt, dma); > + > + return CRYPTO_OK; > +} > + > +/* free up the memory */ > +void spacc_fini(struct spacc_device *spacc) > +{ > + vfree(spacc->ctx); > + vfree(spacc->job); > +} > + > +int spacc_init(void __iomem *baseaddr, struct spacc_device *spacc, > + struct pdu_info *info) > +{ > + unsigned long id; > + char version_string[3][16] = { "SPACC", "SPACC-PDU" }; > + char idx_string[2][16] = { "(Normal Port)", "(Secure Port)" }; > + char dma_type_string[4][16] = { "Unknown", "Scattergather", "Linear", > + "Unknown" }; > + > + if (!baseaddr) { > + pr_debug("ERR: baseaddr is NULL\n"); You must use dev_ instead. > + return -1; What is -1? No, use errno numbers. > + } > + if (!spacc) { > + pr_debug("ERR: spacc is NULL\n"); > + return -1; Same problems. How is this even possible? Don't add dead code. > + } > + > + memset(spacc, 0, sizeof(*spacc)); > + spin_lock_init(&spacc->lock); > + spin_lock_init(&spacc->ctx_lock); > + > + /* assign the baseaddr*/ The missing space before */ is just sloppy. Please write good, readable code so review will be able to focus on important things not such nitpicks. > + spacc->regmap = baseaddr; > + > + /* version info*/ > + spacc->config.version = info->spacc_version.version; > + spacc->config.pdu_version = (info->pdu_config.major << 4) | > + info->pdu_config.minor; > + spacc->config.project = info->spacc_version.project; > + spacc->config.is_pdu = info->spacc_version.is_pdu; > + spacc->config.is_qos = info->spacc_version.qos; > + > + /* misc*/ > + spacc->config.is_partial = info->spacc_version.partial; > + spacc->config.num_ctx = info->spacc_config.num_ctx; > + spacc->config.ciph_page_size = 1U << > + info->spacc_config.ciph_ctx_page_size; > + > + spacc->config.hash_page_size = 1U << > + info->spacc_config.hash_ctx_page_size; > + > + spacc->config.dma_type = info->spacc_config.dma_type; > + spacc->config.idx = info->spacc_version.vspacc_id; > + spacc->config.cmd0_fifo_depth = info->spacc_config.cmd0_fifo_depth; > + spacc->config.cmd1_fifo_depth = info->spacc_config.cmd1_fifo_depth; > + spacc->config.cmd2_fifo_depth = info->spacc_config.cmd2_fifo_depth; > + spacc->config.stat_fifo_depth = info->spacc_config.stat_fifo_depth; > + spacc->config.fifo_cnt = 1; > + spacc->config.is_ivimport = info->spacc_version.ivimport; > + > + /* ctrl register map*/ > + if (spacc->config.version <= 0x4E) > + spacc->config.ctrl_map = spacc_ctrl_map[SPACC_CTRL_VER_0]; > + else if (spacc->config.version <= 0x60) > + spacc->config.ctrl_map = spacc_ctrl_map[SPACC_CTRL_VER_1]; > + else > + spacc->config.ctrl_map = spacc_ctrl_map[SPACC_CTRL_VER_2]; > + > + spacc->job_next_swid = 0; > + spacc->wdcnt = 0; > + spacc->config.wd_timer = SPACC_WD_TIMER_INIT; > + > + /* version 4.10 uses IRQ, Please use Linux coding style comments for given subsystem. See Coding style. > + * above uses WD and we don't support below 4.00 > + */ > + if (spacc->config.version < 0x40) { > + pr_debug("ERR: Unsupported SPAcc version\n"); > + return -EIO; > + } else if (spacc->config.version < 0x4B) { > + spacc->op_mode = SPACC_OP_MODE_IRQ; > + } else { > + spacc->op_mode = SPACC_OP_MODE_WD; > + } > + > + /* set threshold and enable irq > + * on 4.11 and newer cores we can derive this > + * from the HW reported depths. > + */ > + if (spacc->config.stat_fifo_depth == 1) > + spacc->config.ideal_stat_level = 1; > + else if (spacc->config.stat_fifo_depth <= 4) > + spacc->config.ideal_stat_level = > + spacc->config.stat_fifo_depth - 1; > + else if (spacc->config.stat_fifo_depth <= 8) > + spacc->config.ideal_stat_level = > + spacc->config.stat_fifo_depth - 2; > + else > + spacc->config.ideal_stat_level = > + spacc->config.stat_fifo_depth - 4; > + > + /* determine max PROClen value */ > + writel(0xFFFFFFFF, spacc->regmap + SPACC_REG_PROC_LEN); > + spacc->config.max_msg_size = readl(spacc->regmap + SPACC_REG_PROC_LEN); > + > + /* read config info*/ > + if (spacc->config.is_pdu) { > + pr_debug("PDU:\n"); > + pr_debug(" MAJOR : %u\n", info->pdu_config.major); > + pr_debug(" MINOR : %u\n", info->pdu_config.minor); Why? > + } > + > + id = readl(spacc->regmap + SPACC_REG_ID); > + pr_debug("SPACC ID: (%08lx)\n", (unsigned long)id); > + pr_debug(" MAJOR : %x\n", info->spacc_version.major); > + pr_debug(" MINOR : %x\n", info->spacc_version.minor); > + pr_debug(" QOS : %x\n", info->spacc_version.qos); > + pr_debug(" IVIMPORT : %x\n", spacc->config.is_ivimport); What are you going to debug here? > + > + if (spacc->config.version >= 0x48) > + pr_debug(" TYPE : %lx (%s)\n", SPACC_ID_TYPE(id), > + version_string[SPACC_ID_TYPE(id) & 3]); > + > + pr_debug(" AUX : %x\n", info->spacc_version.qos); > + pr_debug(" IDX : %lx %s\n", SPACC_ID_VIDX(id), > + spacc->config.is_secure ? > + (idx_string[spacc->config.is_secure_port & 1]) : ""); > + pr_debug(" PARTIAL : %x\n", info->spacc_version.partial); > + pr_debug(" PROJECT : %x\n", info->spacc_version.project); > + > + if (spacc->config.version >= 0x48) > + id = readl(spacc->regmap + SPACC_REG_CONFIG); > + else > + id = 0xFFFFFFFF; > + > + pr_debug("SPACC CFG: (%08lx)\n", id); > + pr_debug(" CTX CNT : %u\n", info->spacc_config.num_ctx); > + pr_debug(" VSPACC CNT : %u\n", info->spacc_config.num_vspacc); > + pr_debug(" CIPH SZ : %-3lu bytes\n", 1UL << > + info->spacc_config.ciph_ctx_page_size); > + pr_debug(" HASH SZ : %-3lu bytes\n", 1UL << > + info->spacc_config.hash_ctx_page_size); > + pr_debug(" DMA TYPE : %u (%s)\n", info->spacc_config.dma_type, > + dma_type_string[info->spacc_config.dma_type & 3]); > + pr_debug(" MAX PROCLEN: %lu bytes\n", (unsigned long) > + spacc->config.max_msg_size); > + pr_debug(" FIFO CONFIG :\n"); > + pr_debug(" CMD0 DEPTH: %d\n", spacc->config.cmd0_fifo_depth); You could not flood dmesg with more debugging, could you? > + > + if (spacc->config.is_qos) { > + pr_debug(" CMD1 DEPTH: %d\n", > + spacc->config.cmd1_fifo_depth); > + pr_debug(" CMD2 DEPTH: %d\n", > + spacc->config.cmd2_fifo_depth); > + } > + pr_debug(" STAT DEPTH: %d\n", spacc->config.stat_fifo_depth); > + > + if (spacc->config.dma_type == SPACC_DMA_DDT) { > + writel(0x1234567F, baseaddr + SPACC_REG_DST_PTR); > + writel(0xDEADBEEF, baseaddr + SPACC_REG_SRC_PTR); > + > + if (((readl(baseaddr + SPACC_REG_DST_PTR)) != > + (0x1234567F & SPACC_DST_PTR_PTR)) || > + ((readl(baseaddr + SPACC_REG_SRC_PTR)) != > + (0xDEADBEEF & SPACC_SRC_PTR_PTR))) { > + pr_debug("ERR: Failed to set pointers\n"); > + goto ERR; > + } > + } > + > + /* zero the IRQ CTRL/EN register > + * (to make sure we're in a sane state) > + */ > + writel(0, spacc->regmap + SPACC_REG_IRQ_CTRL); > + writel(0, spacc->regmap + SPACC_REG_IRQ_EN); > + writel(0xFFFFFFFF, spacc->regmap + SPACC_REG_IRQ_STAT); > + > + /* init cache*/ > + memset(&spacc->cache, 0, sizeof(spacc->cache)); > + writel(0, spacc->regmap + SPACC_REG_SRC_PTR); > + writel(0, spacc->regmap + SPACC_REG_DST_PTR); > + writel(0, spacc->regmap + SPACC_REG_PROC_LEN); > + writel(0, spacc->regmap + SPACC_REG_ICV_LEN); > + writel(0, spacc->regmap + SPACC_REG_ICV_OFFSET); > + writel(0, spacc->regmap + SPACC_REG_PRE_AAD_LEN); > + writel(0, spacc->regmap + SPACC_REG_POST_AAD_LEN); > + writel(0, spacc->regmap + SPACC_REG_IV_OFFSET); > + writel(0, spacc->regmap + SPACC_REG_OFFSET); > + writel(0, spacc->regmap + SPACC_REG_AUX_INFO); > + > + spacc->ctx = vmalloc(sizeof(struct spacc_ctx) * spacc->config.num_ctx); > + if (!spacc->ctx) > + goto ERR; > + > + spacc->job = vmalloc(sizeof(struct spacc_job) * SPACC_MAX_JOBS); > + if (!spacc->job) > + goto ERR; > + > + /* initialize job_idx and lookup table */ > + spacc_job_init_all(spacc); > + > + /* initialize contexts */ > + spacc_ctx_init_all(spacc); > + > + /* autodetect and set string size setting*/ > + if (spacc->config.version == 0x61 || spacc->config.version >= 0x65) > + spacc_xof_stringsize_autodetect(spacc); > + > + return CRYPTO_OK; ??? Please do not redefine numbers. Missing blank line > +ERR: > + spacc_fini(spacc); > + pr_debug("ERR: Crypto Failed\n"); Drop > + > + return -EIO; > +} > + > +/* callback function to initialize tasklet running */ > +void spacc_pop_jobs(unsigned long data) > +{ > + int num = 0; > + struct spacc_priv *priv = (struct spacc_priv *)data; > + struct spacc_device *spacc = &priv->spacc; > + > + /* decrement the WD CNT here since > + * now we're actually going to respond > + * to the IRQ completely > + */ > + if (spacc->wdcnt) > + --(spacc->wdcnt); > + > + spacc_pop_packets(spacc, &num); > +} > + > +int spacc_remove(struct platform_device *pdev) > +{ > + struct spacc_device *spacc; > + struct spacc_priv *priv = platform_get_drvdata(pdev); > + > + /* free test vector memory*/ > + spacc = &priv->spacc; > + spacc_fini(spacc); > + > + tasklet_kill(&priv->pop_jobs); > + > + /* devm functions do proper cleanup */ > + pdu_mem_deinit(&pdev->dev); > + dev_dbg(&pdev->dev, "removed!\n"); > + > + return 0; > +} > + > +int spacc_set_key_exp(struct spacc_device *spacc, int job_idx) > +{ > + struct spacc_ctx *ctx = NULL; > + struct spacc_job *job = NULL; > + > + if (job_idx < 0 || job_idx >= SPACC_MAX_JOBS) { > + pr_debug("ERR: Invalid Job id specified (out of range)\n"); > + return -ENXIO; > + } > + > + job = &spacc->job[job_idx]; > + ctx = context_lookup_by_job(spacc, job_idx); > + > + if (!ctx) { > + pr_debug("ERR: Failed to find ctx id\n"); > + return -EIO; > + } > + > + job->ctrl |= SPACC_CTRL_MASK(SPACC_CTRL_KEY_EXP); > + > + return CRYPTO_OK; This code has terrible quality and does not fit basic Linux coding style. Why v9 has trivial issues like this? I don't understand. Please cleanup your code thoroughly from all such weird all-platform-Windows-downstream junk. This code is just not ready for submission. > +} > + > +int spacc_compute_xcbc_key(struct spacc_device *spacc, int mode_id, > + int job_idx, const unsigned char *key, > + int keylen, unsigned char *xcbc_out) > +{ > + unsigned char *buf; > + dma_addr_t bufphys; > + struct pdu_ddt ddt; > + unsigned char iv[16]; > + int err, i, handle, usecbc, ctx_idx; > + > + if (job_idx >= 0 && job_idx < SPACC_MAX_JOBS) > + ctx_idx = spacc->job[job_idx].ctx_idx; > + else > + ctx_idx = -1; > + > + if (mode_id == CRYPTO_MODE_MAC_XCBC) { > + /* figure out if we can schedule the key */ > + if (spacc_isenabled(spacc, CRYPTO_MODE_AES_ECB, 16)) > + usecbc = 0; > + else if (spacc_isenabled(spacc, CRYPTO_MODE_AES_CBC, 16)) > + usecbc = 1; > + else > + return -1; > + } else if (mode_id == CRYPTO_MODE_MAC_SM4_XCBC) { > + /* figure out if we can schedule the key */ > + if (spacc_isenabled(spacc, CRYPTO_MODE_SM4_ECB, 16)) > + usecbc = 0; > + else if (spacc_isenabled(spacc, CRYPTO_MODE_SM4_CBC, 16)) > + usecbc = 1; > + else > + return -1; > + } else { > + return -1; What is -1? > + } > + > + memset(iv, 0, sizeof(iv)); > + memset(&ddt, 0, sizeof(ddt)); > + > + buf = dma_alloc_coherent(get_ddt_device(), 64, &bufphys, GFP_KERNEL); > + if (!buf) > + return -EINVAL; and here -EINVAL? So what was the meaning of -1? This is terrible code. > --- /dev/null > +++ b/drivers/crypto/dwc-spacc/spacc_device.c > @@ -0,0 +1,296 @@ > +// SPDX-License-Identifier: GPL-2.0 > + > +#include <linux/module.h> > +#include <linux/interrupt.h> > +#include <linux/dma-mapping.h> > +#include <linux/platform_device.h> > +#include "spacc_device.h" > + > +static struct platform_device *spacc_pdev; This is too much. NAK. Code has terrible quality but what is worse: you ignored several comments, which means code does not improve. You send the same code over and over. Sending same code with same junk is not acceptable. You waste reviewer's time. Best regards, Krzysztof
On 01/10/2024 04:57, Pavitrakumar Managutte wrote: > Hi Krzysztof, > Thanks for the quick review and I do really appreciate everybody's time here. > If something got missed, it's just because of the exhaustive > hardware and the SystemC Model testing. > We make minimal/incremental changes and run things in debug mode > which takes a lot of time, > since this is a large code base. Never ignored anything till date. > Every single comment has been and will be addressed. We will work > on code quality as per your inputs. No, it was not addressed. Do you want proofs? Look: 1. Drop contains. The list of compatible strings and order must be defined. Not addressed at all. 2. crypto@40000000 Ignored completely 3. Generally drivers aren't limited to some number of instances (except... Also ignored completely and more.. > > Warm regards, > PK > > > On Mon, Sep 30, 2024 at 6:50 PM Krzysztof Kozlowski <krzk@kernel.org> wrote: >> >> On 30/09/2024 11:30, Pavitrakumar M wrote: >>> Add DT bindings related to the SPAcc driver for Documentation. >>> DWC Synopsys Security Protocol Accelerator(SPAcc) Hardware Crypto >>> Engine is a crypto IP designed by Synopsys. >>> >>> Co-developed-by: Bhoomika Kadabi <bhoomikak@vayavyalabs.com> >>> Signed-off-by: Bhoomika Kadabi <bhoomikak@vayavyalabs.com> >>> Co-developed-by: Pavitrakumar Managutte <pavitrakumarm@vayavyalabs.com> >>> Signed-off-by: Pavitrakumar Managutte <pavitrakumarm@vayavyalabs.com> >>> Acked-by: Ruud Derwig <Ruud.Derwig@synopsys.com> >>> --- >>> .../bindings/crypto/snps,dwc-spacc.yaml | 71 +++++++++++++++++++ >>> 1 file changed, 71 insertions(+) >>> create mode 100644 Documentation/devicetree/bindings/crypto/snps,dwc-spacc.yaml >> >> Bindings come before users, so please re-order your patches. > > PK: Will re-order >> >> >>> >>> diff --git a/Documentation/devicetree/bindings/crypto/snps,dwc-spacc.yaml b/Documentation/devicetree/bindings/crypto/snps,dwc-spacc.yaml >>> new file mode 100644 >>> index 000000000000..6b94d0aa7280 >>> --- /dev/null >>> +++ b/Documentation/devicetree/bindings/crypto/snps,dwc-spacc.yaml >>> @@ -0,0 +1,71 @@ >>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) >>> +%YAML 1.2 >>> +--- >>> +$id: http://devicetree.org/schemas/crypto/snps,dwc-spacc.yaml# >>> +$schema: http://devicetree.org/meta-schemas/core.yaml# >>> + >>> +title: Synopsys DesignWare Security Protocol Accelerator(SPAcc) Crypto Engine >>> + >>> +maintainers: >>> + - Ruud Derwig <Ruud.Derwig@synopsys.com> >>> + >>> +description: >>> + DWC Synopsys Security Protocol Accelerator(SPAcc) Hardware Crypto Engine is >>> + a crypto IP designed by Synopsys, that can accelerate cryptographic >>> + operations. >>> + >>> +properties: >>> + compatible: >>> + contains: >> >> Nope, you cannot have contains. From where did you get it? Please use >> existing, recent bindings as starting point or just use exampl-eschema. > > PK: Will fix that. >> >> >> Eh, you already got this comment and just ignored it. > > PK: It got missed, never ignored. Too valuable to ignore comments from demigods. >> >> >> You ignored all other comments as well. This is quite disappointing to >> ask us to do the same review over and over. > > PK: That never was the intent nor the impression I wanted to make. > Appreciate everybody's time here. Then why do you ignore review? Best regards, Krzysztof
Hi Krzysztof, I fully agree, some things got missed out; they were supposed to be addressed in the v9 patch. Not contesting that. My apologies. We are doing a code cleanup based on your inputs and previous comments. I will address everything. Warm regards, PK On Tue, Oct 1, 2024 at 12:03 PM Krzysztof Kozlowski <krzk@kernel.org> wrote: > > On Mon, Sep 30, 2024 at 03:00:54PM +0530, Pavitrakumar M wrote: > > Add DT bindings related to the SPAcc driver for Documentation. > > DWC Synopsys Security Protocol Accelerator(SPAcc) Hardware Crypto > > Engine is a crypto IP designed by Synopsys. > > > > Co-developed-by: Bhoomika Kadabi <bhoomikak@vayavyalabs.com> > > Signed-off-by: Bhoomika Kadabi <bhoomikak@vayavyalabs.com> > > Co-developed-by: Pavitrakumar Managutte <pavitrakumarm@vayavyalabs.com> > > Signed-off-by: Pavitrakumar Managutte <pavitrakumarm@vayavyalabs.com> > > Acked-by: Ruud Derwig <Ruud.Derwig@synopsys.com> > > Please run scripts/checkpatch.pl and fix reported warnings. Then please > run and (probably) fix more warnings. > Some warnings can be ignored, especially from --strict run, but the code > here looks like it needs a fix. Feel free to get in touch if the warning > is not clear. > > Best regards, > Krzysztof >