linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Alex Elder <elder@linaro.org>
To: Arnd Bergmann <arnd@arndb.de>
Cc: David Miller <davem@davemloft.net>,
	Bjorn Andersson <bjorn.andersson@linaro.org>,
	Ilias Apalodimas <ilias.apalodimas@linaro.org>,
	syadagir@codeaurora.org, mjavid@codeaurora.org,
	evgreen@chromium.org, Ben Chan <benchan@google.com>,
	Eric Caruso <ejcaruso@google.com>,
	abhishek.esse@gmail.com,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 12/18] soc: qcom: ipa: immediate commands
Date: Fri, 17 May 2019 19:34:15 -0500	[thread overview]
Message-ID: <5d948d74-f739-0cfa-8fae-b15c20fbe7ec@linaro.org> (raw)
In-Reply-To: <f92bfb59-07bb-e8c0-c307-cd69da7ccd8a@linaro.org>

On 5/15/19 7:35 AM, Alex Elder wrote:
> 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).

OK, now I'm going to correct myself.  I hope I don't make
any mistakes here because things are confused enough...

Part of what I described previously is still true, namely
there are tables that need to be initialized (i.e., the
IPA needs to be told where they reside), and there is a
separate step is available to zero the content of the tables.

But there really is no need for the AP to hang onto this
DMA memory after this immediate command has been issued.
I will add comments in the code to make it less surprising.

But here's a summary of why.

I think there are two things at play that make it confusing.

The first thing is that these "header tables" are actually
located in a region of shared memory ("smem") that is local
to the IPA (not the AP).  The the IPA_CMD_HDR_INIT_LOCAL
immediate command is meant to:
1) define the header table location in IPA local memory
2) define the header table size
3) provide a buffer used to fill the table with its initial
   contents

The location and size are encoded in the flags field
of the payload (offset and size).

The initial contents are filled via DMA from a buffer
in main memory, whose DMA address is supplied in the
hdr_table_addr parameter in the payload.  The initial
contents we supply are all zero.  So this is why we
need to allocate DMA memory.

The second thing is that this is an instance where the
AP is responsible for performing some initialization
of resources it may not "own" thereafter.  The IPA
hardware owns this table, even though the AP needs to
tell it where it sits in IPA local memory.  The AP is
able to copy (using DMA) content into that table, but
doing so involves a DMA transfer.

More advanced features of the IPA would make more use
of this header table, but those features not yet
supported so this initialization (and a subsequent,
seemingly redundant zeroing) is all we do.

Does that make sense?

					-Alex


> 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
>>
> 


  reply	other threads:[~2019-05-18  0:34 UTC|newest]

Thread overview: 66+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-05-12  1:24 [PATCH 00/18] net: introduce Qualcomm IPA driver Alex Elder
2019-05-12  1:24 ` [PATCH 01/18] bitfield.h: add FIELD_MAX() and field_max() Alex Elder
2019-05-12  6:33   ` Kalle Valo
2019-05-12 12:18     ` Alex Elder
2019-05-12 19:30       ` Johannes Berg
2019-05-12  1:24 ` [PATCH 02/18] soc: qcom: create "include/soc/qcom/rmnet.h" Alex Elder
2019-05-12  2:34   ` Joe Perches
2019-05-12 12:15     ` Alex Elder
2019-05-15  6:59   ` Arnd Bergmann
2019-05-15 12:03     ` Alex Elder
2019-05-16  1:09       ` Subash Abhinov Kasiviswanathan
2019-05-17 17:27         ` Alex Elder
2019-05-17 18:08           ` Subash Abhinov Kasiviswanathan
2019-05-19 17:37             ` Alex Elder
2019-05-12  1:24 ` [PATCH 03/18] dt-bindings: soc: qcom: add IPA bindings Alex Elder
2019-05-15  7:03   ` Arnd Bergmann
2019-05-15 12:04     ` Alex Elder
2019-05-15 16:50       ` Rob Herring
2019-05-15 17:05         ` Alex Elder
2019-05-12  1:24 ` [PATCH 04/18] soc: qcom: ipa: main code Alex Elder
2019-05-12  1:24 ` [PATCH 05/18] soc: qcom: ipa: configuration data Alex Elder
2019-05-12  1:24 ` [PATCH 06/18] soc: qcom: ipa: clocking, interrupts, and memory Alex Elder
2019-05-12  1:24 ` [PATCH 07/18] soc: qcom: ipa: GSI headers Alex Elder
2019-05-12  1:24 ` [PATCH 08/18] soc: qcom: ipa: the generic software interface Alex Elder
2019-05-15  7:21   ` Arnd Bergmann
2019-05-15 12:13     ` Alex Elder
2019-05-15 12:40       ` Arnd Bergmann
2019-05-15 10:47   ` Arnd Bergmann
2019-05-15 13:32     ` Alex Elder
2019-05-15 19:37   ` Arnd Bergmann
2019-05-12  1:24 ` [PATCH 09/18] soc: qcom: ipa: GSI transactions Alex Elder
2019-05-15  7:34   ` Arnd Bergmann
2019-05-15 12:25     ` Alex Elder
2019-05-15 20:50       ` Arnd Bergmann
2019-05-17 18:08     ` Alex Elder
2019-05-17 18:33       ` Arnd Bergmann
2019-05-17 18:44         ` Alex Elder
2019-05-19 17:11           ` Alex Elder
2019-05-20  9:25             ` Arnd Bergmann
2019-05-20 12:50               ` Alex Elder
2019-05-20 14:43                 ` Arnd Bergmann
2019-05-20 14:44                   ` Alex Elder
2019-05-20 16:34                     ` Evan Green
2019-05-20 16:50                       ` Alex Elder
2019-05-20 17:36                         ` Evan Green
2019-05-12  1:25 ` [PATCH 10/18] soc: qcom: ipa: IPA interface to GSI Alex Elder
2019-05-12  1:25 ` [PATCH 11/18] soc: qcom: ipa: IPA endpoints Alex Elder
2019-05-12  1:25 ` [PATCH 12/18] soc: qcom: ipa: immediate commands Alex Elder
2019-05-15  8:16   ` Arnd Bergmann
2019-05-15 12:35     ` Alex Elder
2019-05-18  0:34       ` Alex Elder [this message]
2019-05-20 14:50         ` Arnd Bergmann
2019-05-20 14:55           ` Alex Elder
2019-05-20 17:35             ` Christoph Hellwig
2019-05-12  1:25 ` [PATCH 13/18] soc: qcom: ipa: IPA network device and microcontroller Alex Elder
2019-05-15  8:21   ` Arnd Bergmann
2019-05-15 12:46     ` Alex Elder
2019-05-12  1:25 ` [PATCH 14/18] soc: qcom: ipa: AP/modem communications Alex Elder
2019-05-12  1:25 ` [PATCH 15/18] soc: qcom: ipa: support build of IPA code Alex Elder
2019-05-12  1:25 ` [PATCH 16/18] MAINTAINERS: add entry for the Qualcomm IPA driver Alex Elder
2019-05-12  1:25 ` [PATCH 17/18] arm64: dts: sdm845: add IPA information Alex Elder
2019-05-12  1:25 ` [PATCH 18/18] arm64: defconfig: enable build of IPA code Alex Elder
2019-05-15  8:23   ` Arnd Bergmann
2019-05-15 12:49     ` Alex Elder
2019-05-15 12:37 ` [PATCH 00/18] net: introduce Qualcomm IPA driver Arnd Bergmann
2019-05-15 12:52   ` Alex Elder

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=5d948d74-f739-0cfa-8fae-b15c20fbe7ec@linaro.org \
    --to=elder@linaro.org \
    --cc=abhishek.esse@gmail.com \
    --cc=arnd@arndb.de \
    --cc=benchan@google.com \
    --cc=bjorn.andersson@linaro.org \
    --cc=davem@davemloft.net \
    --cc=ejcaruso@google.com \
    --cc=evgreen@chromium.org \
    --cc=ilias.apalodimas@linaro.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mjavid@codeaurora.org \
    --cc=syadagir@codeaurora.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).