LKML Archive on lore.kernel.org
 help / color / 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>,
	Networking <netdev@vger.kernel.org>,
	DTML <devicetree@vger.kernel.org>,
	linux-arm-msm@vger.kernel.org, linux-soc@vger.kernel.org,
	Linux ARM <linux-arm-kernel@lists.infradead.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	syadagir@codeaurora.org, mjavid@codeaurora.org,
	Rob Herring <robh+dt@kernel.org>,
	Mark Rutland <mark.rutland@arm.com>
Subject: Re: [RFC PATCH 09/12] soc: qcom: ipa: main IPA source file
Date: Wed, 14 Nov 2018 21:11:38 -0600
Message-ID: <4cc029b5-590d-fcba-6c29-b8dac6d03133@linaro.org> (raw)
In-Reply-To: <CAK8P3a0RH4wbigKR0zsbxPzqWDjrLEkYfQk4GNxhGuxs=HJHHw@mail.gmail.com>

On 11/7/18 8:08 AM, Arnd Bergmann wrote:
> On Wed, Nov 7, 2018 at 1:33 AM Alex Elder <elder@linaro.org> wrote:
> 
>> +static void ipa_client_remove_deferred(struct work_struct *work);
> 
> Try to avoid forward declarations by reordering the code in call order,
> it will also make it easier to read.
> 
>> +static DECLARE_WORK(ipa_client_remove_work, ipa_client_remove_deferred);

Done.  I've actually reworked this a lot, and pulled all the
clock (and interconnect) related code into a separate source file.
No more forward declarations (there anyway), and the work structure
is now embedded in the top-level IPA structure so I can derive
it again in the work function (rather than using the global).

>> +static struct ipa_context ipa_ctx_struct;
>> +struct ipa_context *ipa_ctx = &ipa_ctx_struct;
> 
> Global state variables should generally be removed as well, and
> passed around as function arguments.

Working on this.

>> +static int hdr_init_local_cmd(u32 offset, u32 size)
>> +{
>> +       struct ipa_desc desc = { };
>> +       struct ipa_dma_mem mem;
>> +       void *payload;
>> +       int ret;
>> +
>> +       if (ipa_dma_alloc(&mem, size, GFP_KERNEL))
>> +               return -ENOMEM;
>> +
>> +       offset += ipa_ctx->smem_offset;
>> +
>> +       payload = ipahal_hdr_init_local_pyld(&mem, offset);
>> +       if (!payload) {
>> +               ret = -ENOMEM;
>> +               goto err_dma_free;
>> +       }
>> +
>> +       desc.type = IPA_IMM_CMD_DESC;
>> +       desc.len_opcode = IPA_IMM_CMD_HDR_INIT_LOCAL;
>> +       desc.payload = payload;
>> +
>> +       ret = ipa_send_cmd(&desc);
> 
> You have a bunch of dynamic allocations in here, which you
> then immediately tear down again after the command is complete.
> I can't see at all what you do with the DMA address, since you
> seem to not use the virtual address at all but only store
> the physical address in some kind of descriptor without ever
> writing to it.

I should probably have added at least a comment here.  The
DMA memory was zeroed at the time of allocation.  That zero
buffer is then referred to in the payload to the HDR_INIT_LOCAL
immediate command.  So that command, when executing in the IPA
hardware, uses the contents of the buffer whose physical address
it's supplied, which in this case is full of zeroes.  We don't
use the virtual address because the buffer came pre-zeroed.

Based on your comment elsewhere I will be putting the command
payload in a structure on the stack rather than allocating it
dynamically.

> Am I missing something here?
> 
>> +/* Remoteproc callbacks for SSR events: prepare, start, stop, unprepare */
>> +int ipa_ssr_prepare(struct rproc_subdev *subdev)
>> +{
>> +       printk("======== SSR prepare received ========\n");
> 
> I think you mean dev_dbg() here. A plain printk() without a level
> is not correct and we probably don't want those messages to arrive
> on the console for normal users.

Yes, this was obviously a debug message in some code I should
have removed before sending...

>> +static int ipa_firmware_load(struct de
>> +
>> +err_clear_dev:
>> +       ipa_ctx->lan_cons_ep_id = 0;
>> +       ipa_ctx->cmd_prod_ep_id = 0;
>> +       ipahal_exit();
>> +err_dma_exit:
>> +       ipa_dma_exit();
>> +err_clear_gsi:
>> +       ipa_ctx->gsi = NULL;
>> +       ipa_ctx->ipa_phys = 0;
>> +       ipa_reg_exit();
>> +err_clear_ipa_irq:
>> +       ipa_ctx->ipa_irq = 0;
>> +err_clear_filter_bitmap:
>> +       ipa_ctx->filter_bitmap = 0;
>> +err_interconnect_exit:
>> +       ipa_interconnect_exit();
>> +err_clock_exit:
>> +       ipa_clock_exit();
>> +       ipa_ctx->dev = NULL;
>> +out_smp2p_exit:
>> +       ipa_smp2p_exit(dev);
>> +
> 
> No need to initialize members to zero when you are about
> to free the structure.

The IPA context is in fact a global, static structure at
the moment.  All of this bookkeeping (zeroing out things)
is a habitual practice, basically.  Regardless your point
is good and I'll remove these kinds of things as part of
converting to not using globals.

>> +static struct platform_driver ipa_plat_drv = {
>> +       .probe = ipa_plat_drv_probe,
>> +       .remove = ipa_plat_drv_remove,
>> +       .driver = {
>> +               .name = "ipa",
>> +               .owner = THIS_MODULE,
>> +               .pm = &ipa_pm_ops,
>> +               .of_match_table = ipa_plat_drv_match,
>> +       },
>> +};
>> +
>> +builtin_platform_driver(ipa_plat_drv);
> 
> This should be module_platform_driver(), and allow unloading
> the driver.

Yes.  I've done this for my own use, but the code is not
currently able to shut down cleanly.  I've been fixing a
*lot* of the things that don't clean up after themselves
today but there's more work before I can say this can be
safely built as a module.  But it's a requirement.

					-Alex



> 
>         Arnd
> 


  reply index

Thread overview: 37+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-11-07  0:32 [RFC PATCH 00/12] net: introduce Qualcomm IPA driver Alex Elder
2018-11-07  0:32 ` [RFC PATCH 01/12] dt-bindings: soc: qcom: add IPA bindings Alex Elder
2018-11-07 11:50   ` Arnd Bergmann
2018-11-09 22:38     ` Alex Elder
2018-11-07 14:59   ` Rob Herring
2018-11-09 22:38     ` Alex Elder
2018-11-11  1:40       ` Rob Herring
2018-11-13 16:28     ` Alex Elder
2018-11-07  0:32 ` [RFC PATCH 02/12] soc: qcom: ipa: DMA helpers Alex Elder
2018-11-07 12:17   ` Arnd Bergmann
2018-11-13 16:33     ` Alex Elder
2018-11-07  0:32 ` [RFC PATCH 03/12] soc: qcom: ipa: generic software interface Alex Elder
2018-11-07  0:32 ` [RFC PATCH 04/12] soc: qcom: ipa: immediate commands Alex Elder
2018-11-07 14:36   ` Arnd Bergmann
2018-11-13 16:58     ` Alex Elder
2018-11-07  0:32 ` [RFC PATCH 05/12] soc: qcom: ipa: IPA interrupts and the microcontroller Alex Elder
2018-11-07  0:32 ` [RFC PATCH 06/12] soc: qcom: ipa: QMI modem communication Alex Elder
2018-11-07  0:32 ` [RFC PATCH 07/12] soc: qcom: ipa: IPA register abstraction Alex Elder
2018-11-07 15:00   ` Arnd Bergmann
2018-11-15  2:48     ` Alex Elder
2018-11-15 14:42       ` Arnd Bergmann
2018-11-07  0:32 ` [RFC PATCH 08/12] soc: qcom: ipa: utility functions Alex Elder
2018-11-07  0:32 ` [RFC PATCH 09/12] soc: qcom: ipa: main IPA source file Alex Elder
2018-11-07 14:08   ` Arnd Bergmann
2018-11-15  3:11     ` Alex Elder [this message]
2018-11-07  0:32 ` [RFC PATCH 10/12] soc: qcom: ipa: data path Alex Elder
2018-11-07 14:55   ` Arnd Bergmann
2018-11-15  3:31     ` Alex Elder
2018-11-15 14:48       ` Arnd Bergmann
2018-11-07  0:32 ` [RFC PATCH 11/12] soc: qcom: ipa: IPA rmnet interface Alex Elder
2018-11-07 13:30   ` Arnd Bergmann
2018-11-07 15:26   ` Dan Williams
2018-11-07  0:32 ` [RFC PATCH 12/12] soc: qcom: ipa: build and "ipa_i.h" Alex Elder
2018-11-07  0:40   ` Randy Dunlap
2018-11-08 16:22     ` Alex Elder
2018-11-07 12:34   ` Arnd Bergmann
2018-11-07 15:46 ` [RFC PATCH 00/12] net: introduce Qualcomm IPA driver Arnd Bergmann

Reply instructions:

You may reply publically 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=4cc029b5-590d-fcba-6c29-b8dac6d03133@linaro.org \
    --to=elder@linaro.org \
    --cc=arnd@arndb.de \
    --cc=bjorn.andersson@linaro.org \
    --cc=davem@davemloft.net \
    --cc=devicetree@vger.kernel.org \
    --cc=ilias.apalodimas@linaro.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-arm-msm@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-soc@vger.kernel.org \
    --cc=mark.rutland@arm.com \
    --cc=mjavid@codeaurora.org \
    --cc=netdev@vger.kernel.org \
    --cc=robh+dt@kernel.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

LKML Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/lkml/0 lkml/git/0.git
	git clone --mirror https://lore.kernel.org/lkml/1 lkml/git/1.git
	git clone --mirror https://lore.kernel.org/lkml/2 lkml/git/2.git
	git clone --mirror https://lore.kernel.org/lkml/3 lkml/git/3.git
	git clone --mirror https://lore.kernel.org/lkml/4 lkml/git/4.git
	git clone --mirror https://lore.kernel.org/lkml/5 lkml/git/5.git
	git clone --mirror https://lore.kernel.org/lkml/6 lkml/git/6.git
	git clone --mirror https://lore.kernel.org/lkml/7 lkml/git/7.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 lkml lkml/ https://lore.kernel.org/lkml \
		linux-kernel@vger.kernel.org linux-kernel@archiver.kernel.org
	public-inbox-index lkml


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kernel


AGPL code for this site: git clone https://public-inbox.org/ public-inbox