On 10/31/2017 2:01 PM, Vinod Koul wrote:
> On Mon, Oct 30, 2017 at 03:46:54PM +0200, Horia Geantă wrote:
>
>> @@ -600,6 +600,23 @@ config ZX_DMA
>> help
>> Support the DMA engine for ZTE ZX family platform devices.
>>
>> +config CRYPTO_DEV_FSL_CAAM_DMA
>
> File is sorted alphabetically
>
I didn't know. I will change the position in file.
>> + tristate "CAAM DMA engine support"
>> + depends on CRYPTO_DEV_FSL_CAAM_JR
>> + default y
>
> why should it be default?
>
My mistake. It remained to 'y' from testing. I will change it to 'n'.
>> --- /dev/null
>> +++ b/drivers/dma/caam_dma.c
>> @@ -0,0 +1,444 @@
>> +/*
>> + * caam support for SG DMA
>> + *
>> + * Copyright 2016 Freescale Semiconductor, Inc
>> + * Copyright 2017 NXP
>> + */
>
> that is interesting, no license text?
>
Thanks for the catch. The next patch will contain the full license text.
>> +
>> +#include <linux/module.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/dma-mapping.h>
>> +#include <linux/interrupt.h>
>
> why do you need this
>
I don't. I thought I removed any not-needed header files. I will remove it.
>> +#include <linux/slab.h>
>> +#include <linux/debugfs.h>
>
> i didn't see any debugfs code, why do you need this
>
I don't. I will remove it.
> alphabetical sort pls
It will be done.
>> +
>> +#include <linux/dmaengine.h>
>> +#include "dmaengine.h"
>> +
>> +#include "../crypto/caam/regs.h"
>> +#include "../crypto/caam/jr.h"
>> +#include "../crypto/caam/error.h"
>> +#include "../crypto/caam/intern.h"
>> +#include "../crypto/caam/desc_constr.h"
>
> ah that puts very hard assumptions on locations of the subsystems. Please do
> not do that and move the required stuff into common header location in
> include/
>
Unfortunately this is not possible. The functionality used by CAAM DMA
from the CAAM subsystem should not be exported to be used by other
modules. It is because this driver is so tightly coupled with the CAAM
driver(s) that it needs access to it's 'internal' functionality (that
should not be otherwise shared with anyone).
>> +/* This is max chunk size of a DMA transfer. If a buffer is larger than this
>> + * value it is internally broken into chunks of max CAAM_DMA_CHUNK_SIZE bytes
>> + * and for each chunk a DMA transfer request is issued.
>> + * This value is the largest number on 16 bits that is a multiple of 256 bytes
>> + * (the largest configurable CAAM DMA burst size).
>> + */
>
> Kernel comments style we follow is:
>
> /*
> * this is for
> * multi line comment
> */
>
> Pls fix this in the file
>
It will be done.
>> +#define CAAM_DMA_CHUNK_SIZE 65280
>> +
>> +struct caam_dma_sh_desc {
>
> sh?
>
It means "shared". It is obvious to anyone who is familiar with the CAAM
system.
>> +struct caam_dma_ctx {
>> + struct dma_chan chan;
>> + struct list_head node;
>> + struct device *jrdev;
>> + struct list_head submit_q;
>
> call it pending
>
It will be done.
>> +static struct dma_device *dma_dev;
>> +static struct caam_dma_sh_desc *dma_sh_desc;
>> +static LIST_HEAD(dma_ctx_list);
>
> why do you need so many globals?
>
How many globals are too many? I can group them in a common structure.
But I'm not sure how would that help.
>> +static dma_cookie_t caam_dma_tx_submit(struct dma_async_tx_descriptor *tx)
>> +{
>> + struct caam_dma_edesc *edesc = NULL;
>> + struct caam_dma_ctx *ctx = NULL;
>> + dma_cookie_t cookie;
>> +
>> + edesc = container_of(tx, struct caam_dma_edesc, async_tx);
>> + ctx = container_of(tx->chan, struct caam_dma_ctx, chan);
>> +
>> + spin_lock_bh(&ctx->edesc_lock);
>
> why _bh, i didnt see any irqs or tasklets here which is actually odd, so
> what is going on
>
The tasklet is hidden inside the JR driver from the CAAM subsystem (see
drivers/crypto/caam/jr.c). The function caam_dma_done(), which is called
from the JR tasklet handler, also uses the spin_lock. I need to disable
bottom half to make sure that I don't run into a deadlock when I'm
preempted.
>> +
>> + cookie = dma_cookie_assign(tx);
>> + list_add_tail(&edesc->node, &ctx->submit_q);
>> +
>> + spin_unlock_bh(&ctx->edesc_lock);
>> +
>> + return cookie;
>> +}
>
> we have a virtual channel wrapper where we do the same stuff as above, so
> consider reusing that
>
Some of the functionality that the virtual channel might provide cannot
be extracted because it is embedded into the JR driver (e.g. the
tasklet). Therefore the use of the virtual channel is inefficient at
best if not even impossible.
>> +static void caam_dma_memcpy_init_job_desc(struct caam_dma_edesc *edesc)
>> +{
>> + u32 *jd = edesc->jd;
>> + u32 *sh_desc = dma_sh_desc->desc;
>> + dma_addr_t desc_dma = dma_sh_desc->desc_dma;
>> +
>> + /* init the job descriptor */
>> + init_job_desc_shared(jd, desc_dma, desc_len(sh_desc), HDR_REVERSE);
>> +
>> + /* set SEQIN PTR */
>> + append_seq_in_ptr(jd, edesc->src_dma, edesc->src_len, 0);
>> +
>> + /* set SEQOUT PTR */
>> + append_seq_out_ptr(jd, edesc->dst_dma, edesc->dst_len, 0);
>> +
>> +#ifdef DEBUG
>> + print_hex_dump(KERN_ERR, "caam dma desc@" __stringify(__LINE__) ": ",
>> + DUMP_PREFIX_ADDRESS, 16, 4, jd, desc_bytes(jd), 1);
>
> ah this make you compile kernel. Consider the dynamic printk helpers for
> printing
>
It will be done.
>> +/* This function can be called in an interrupt context */
>> +static void caam_dma_issue_pending(struct dma_chan *chan)
>> +{
>> + struct caam_dma_ctx *ctx = container_of(chan, struct caam_dma_ctx,
>> + chan);
>> + struct caam_dma_edesc *edesc, *_edesc;
>> +
>> + spin_lock_bh(&ctx->edesc_lock);
>> + list_for_each_entry_safe(edesc, _edesc, &ctx->submit_q, node) {
>> + if (caam_jr_enqueue(ctx->jrdev, edesc->jd,
>> + caam_dma_done, edesc) < 0)
>
> what does the caam_jr_enqueue() do?
>
Enqueues a job descriptor (jd) into a job ring (jr). Additionally the
"caam_dma_done()" function is registered to be used as a callback when
the job finishes. For more details see drivers/crypto/caam/jr.c.
>> +static int caam_dma_jr_chan_bind(void)
>> +{
>> + struct device *jrdev;
>> + struct caam_dma_ctx *ctx;
>> + int bonds = 0;
>> + int i;
>> +
>> + for (i = 0; i < caam_jr_driver_probed(); i++) {
>> + jrdev = caam_jridx_alloc(i);
>> + if (IS_ERR(jrdev)) {
>> + pr_err("job ring device %d allocation failed\n", i);
>> + continue;
>> + }
>> +
>> + ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
>> + if (!ctx) {
>> + caam_jr_free(jrdev);
>> + continue;
>
> you want to continue?
>
Yes. If on the next loop memory becomes available I would like to
allocate and use another job ring(jr). If there is no memory still then
it will do nothing.
>> + dma_dev->dev = dev;
>> + dma_dev->residue_granularity = DMA_RESIDUE_GRANULARITY_DESCRIPTOR;
>> + dma_cap_set(DMA_MEMCPY, dma_dev->cap_mask);
>> + dma_cap_set(DMA_PRIVATE, dma_dev->cap_mask);
>> + dma_dev->device_tx_status = dma_cookie_status;
>> + dma_dev->device_issue_pending = caam_dma_issue_pending;
>> + dma_dev->device_prep_dma_memcpy = caam_dma_prep_memcpy;
>> + dma_dev->device_free_chan_resources = caam_dma_free_chan_resources;
>> +
>> + err = dma_async_device_register(dma_dev);
>> + if (err) {
>> + dev_err(dev, "Failed to register CAAM DMA engine\n");
>> + goto jr_bind_err;
>> + }
>> +
>> + dev_info(dev, "caam dma support with %d job rings\n", bonds);
>
> that is very noisy
>
I want to print a line that informs that the caam_dma driver has been
successfully probed. Do you have any other suggestion?
>> +MODULE_LICENSE("Dual BSD/GPL");
>> +MODULE_DESCRIPTION("NXP CAAM support for SG DMA");
>> +MODULE_AUTHOR("NXP Semiconductors");
>
> No MODULE_ALIAS, how did you load the driver
>
It was built in as default. I will add a MODULE_ALIAS().
Thanks for the remarks.
Radu
