lkml.org 
[lkml]   [2019]   [May]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [PATCH 12/18] soc: qcom: ipa: immediate commands
    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.

    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

    \
     
     \ /
      Last update: 2019-05-15 10:17    [W:4.173 / U:0.220 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site