Re: [PATCH v2 2/5] mtd: nand: Qualcomm NAND controller driver

[Stephen Boyd <sboyd@codeaurora.org>]

On 07/27/2015 09:34 PM, Archit Taneja wrote:
> Hi,
>
> On 07/25/2015 06:21 AM, Stephen Boyd wrote:
>> On 07/21/2015 03:34 AM, Archit Taneja wrote:
>>
>>> +              int size)
>>> +{
>>> +    struct desc_info *desc;
>>> +    struct dma_async_tx_descriptor *dma_desc;
>>> +    struct scatterlist *sgl;
>>> +    int r;
>>> +
>>> +    desc = kzalloc(sizeof(*desc), GFP_KERNEL);
>>> +    if (!desc)
>>> +        return -ENOMEM;
>>> +
>>> +    list_add_tail(&desc->list, &this->list);
>>> +
>>> +    sgl = &desc->sgl;
>>> +
>>> +    sg_init_one(sgl, vaddr, size);
>>> +
>>> +    desc->dir = DMA_MEM_TO_DEV;
>>> +
>>> +    r = dma_map_sg(this->dev, sgl, 1, desc->dir);
>>> +    if (r == 0)
>>> +        goto err;
>>
>> Should we return an error in this case? Looks like return 0.
>
> dma_map_sg returns the number of sg entries successfully mapped. In
> this case, it should be 1.

Right, but this function returns 0 (success?) if we failed to map anything.

>
>>
>>> +
>>> +    this->slave_conf.device_fc = 0;
>>> +    this->slave_conf.dst_addr = this->res->start + reg_off;
>>> +    this->slave_conf.dst_maxburst = 16;
>>
>> Is there any reason why slave_conf can't be on the stack? Otherwise it's
>> odd that it's overwritten a few times before we submit the descriptors,
>> so it must be copied by the dmaengine provider, but that isn't clear at
>> all from the code. If it isn't copied, perhaps it should be part of the
>> desc_info structure. If it is copied I wonder why it isn't const in the
>> function signature.
>
> The dmaengine drivers either memcpy slave_config in their
> device_config() dmaengine op, or populate their local members reading
> params in the passed slave_config.
>
> I'll move slave_conf to stack. As you said, the config argument
> in dmaengine_slave_config should ideally use const.

Cool, someone should send a patch.

>
>>
>>> +
>>> +    r = dmaengine_slave_config(this->chan, &this->slave_conf);
>>> +    if (r) {
>>> +        dev_err(this->dev, "failed to configure dma channel\n");
>>> +        goto err;
>>> +    }
>>> +
>>> +    dma_desc = dmaengine_prep_slave_sg(this->chan, sgl, 1, 
>>> desc->dir, 0);
>>> +    if (!dma_desc) {
>>> +        dev_err(this->dev, "failed to prepare data write desc\n");
>>> +        r = PTR_ERR(dma_desc);
>>> +        goto err;
>>> +    }
>>> +
>>> +    desc->dma_desc = dma_desc;
>>> +
>>> +    return 0;
>>> +err:
>>> +    kfree(desc);
>>> +
>>> +    return r;
>>> +}
>>> +
>>> +/*
>>> + * helper to prepare dma descriptors to configure registers needed 
>>> for reading a
>>> + * codeword/step in a page
>>> + */
>>> +static void config_cw_read(struct qcom_nandc_data *this)
>>> +{
>>> +    struct nandc_regs *regs = this->regs;
>>> +
>>> +    write_reg_dma(this, NAND_FLASH_CMD, &regs->cmd, 3, true);
>>
>> Maybe it would be better to have a case statement inside
>> {write,read}_reg_dma() that looked at the second argument and matched it
>> up with an offset in regs. Then this could be
>>
>>      write_reg_dma(this, NAND_FLASH_CMD, 3, true);
>
> That's a good idea. However, we have at least one programming seqeunce 
> (in nandc_param) where we need to write two different values to the 
> same register. In such a case, we need two different locations to 
> store the two values.
>
> I can split up the programming sequence into two parts such that we 
> won't write to the same register twice. But doing this for the sake of 
> reducing an argument to write_reg_dma seems a bit unnecessary.
>

Or you could have two #defines that indicate the different usage? Like 
NAND_CMD_FOO and NAND_CMD_BAR that both map to the same register but 
uses different locations as storage.

>>
>> Are we guaranteed that this is called within the same context as where
>> the buffer is passed to this function? Otherwise this stack check isn't
>> going to work because object_is_on_stack() will silently fail.
>
> These are funcs are finally tied to mtd ops. I think in normal operation
> it'll be the same context. I'll still cross check. The aim of the check
> is to prevent a memcpy of the buffer to/from the layer above. A false
> negative will result in a slower read/write operation.

Right. It would be nice if the mtd layer was DMA aware and could 
indicate to drivers that DMA on the buffer is allowable or not. Trying 
to figure it out if the buffer is in lowmem after the buffer is mapped 
is error prone, which is why not a lot of usage of object_is_on_stack() 
exists. Honestly I'm confused, I thought the DMA APIs would "do the 
right thing" when highmem was passed into the mapping APIs, but maybe 
I'm wrong. I'll have to look.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project