Re: [PATCH 12/18] soc: qcom: ipa: immediate commands

[Alex Elder <elder@linaro.org>]

On 5/15/19 3:16 AM, Arnd Bergmann wrote:
> On Sun, May 12, 2019 at 3:25 AM Alex Elder <elder@linaro.org> wrote:
> 
>> +/* Initialize header space in IPA local memory */
>> +int ipa_cmd_hdr_init_local(struct ipa *ipa, u32 offset, u32 size)
>> +{
>> +       struct ipa_imm_cmd_hw_hdr_init_local *payload;
>> +       struct device *dev = &ipa->pdev->dev;
>> +       dma_addr_t addr;
>> +       void *virt;
>> +       u32 flags;
>> +       u32 max;
>> +       int ret;
>> +
>> +       /* Note: size *can* be zero in this case */
>> +       if (size > field_max(IPA_CMD_HDR_INIT_FLAGS_TABLE_SIZE_FMASK))
>> +               return -EINVAL;
>> +
>> +       max = field_max(IPA_CMD_HDR_INIT_FLAGS_HDR_ADDR_FMASK);
>> +       if (offset > max || ipa->shared_offset > max - offset)
>> +               return -EINVAL;
>> +       offset += ipa->shared_offset;
>> +
>> +       /* A zero-filled buffer of the right size is all that's required */
>> +       virt = dma_alloc_coherent(dev, size, &addr, GFP_KERNEL);
>> +       if (!virt)
>> +               return -ENOMEM;
>> +
>> +       payload = kzalloc(sizeof(*payload), GFP_KERNEL);
>> +       if (!payload) {
>> +               ret = -ENOMEM;
>> +               goto out_dma_free;
>> +       }
>> +
>> +       payload->hdr_table_addr = addr;
>> +       flags = u32_encode_bits(size, IPA_CMD_HDR_INIT_FLAGS_TABLE_SIZE_FMASK);
>> +       flags |= u32_encode_bits(offset, IPA_CMD_HDR_INIT_FLAGS_HDR_ADDR_FMASK);
>> +       payload->flags = flags;
>> +
>> +       ret = ipa_cmd(ipa, IPA_CMD_HDR_INIT_LOCAL, payload, sizeof(*payload));
>> +
>> +       kfree(payload);
>> +out_dma_free:
>> +       dma_free_coherent(dev, size, virt, addr);
>> +
>> +       return ret;
>> +}
> 
> This looks rather strange. I think I looked at it before and you explained
> it, but I have since forgotten what you do it for, so I assume everyone else
> that tries to understand this will have problems too.

This is a bug.  I think I misunderstood why you were
puzzled before.  Now I get it.  I need to save that
DMA address and not free it at the end of the function
(except on error).

Here's what I think happened.  There are two parts of
initializing these tables.  One part tells the hardware
where the table is located.  Another part zeroes the
contents of those tables.  (The zeroing part could be
accomplished when the table is allocated, but there
are cases where they have to be zeroed again without
needing to tell the hardware so we need to at least
be able to do that independently.)

I think I was assuming this was the function that did
the zeroing, and I thought that adding the comment about
"all we need is a zero-filled buffer" addressed what
you thought should be made clearer.

I will definitely fix this, and I'm glad you repeated
it so I was forced to take another look.

If I again misunderstand your point, please let me know.

					-Alex

> The issue I see is that you do an expensive dma_alloc_coherent()
> but then never actually use the pointer returned by it, only the
> dma address that cannot be turned back into a virtual address
> in order to access the data in it.
> 
> If you can't actually use payload->hdr_table_addr, why even allocate
> it here?
> 
>      Arnd
>