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 [thread overview] 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 >
WARNING: multiple messages have this Message-ID (diff)
From: elder@linaro.org (Alex Elder) To: linux-arm-kernel@lists.infradead.org Subject: [RFC PATCH 09/12] soc: qcom: ipa: main IPA source file Date: Wed, 14 Nov 2018 21:11:38 -0600 [thread overview] 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 >
next prev parent reply other threads:[~2018-11-15 3:11 UTC|newest] Thread overview: 76+ 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 ` Alex Elder 2018-11-07 0:32 ` [RFC PATCH 01/12] dt-bindings: soc: qcom: add IPA bindings Alex Elder 2018-11-07 0:32 ` Alex Elder 2018-11-07 11:50 ` Arnd Bergmann 2018-11-07 11:50 ` Arnd Bergmann 2018-11-09 22:38 ` Alex Elder 2018-11-09 22:38 ` Alex Elder 2018-11-07 14:59 ` Rob Herring 2018-11-07 14:59 ` Rob Herring 2018-11-09 22:38 ` Alex Elder 2018-11-09 22:38 ` Alex Elder 2018-11-11 1:40 ` Rob Herring 2018-11-11 1:40 ` Rob Herring 2018-11-11 1:40 ` Rob Herring 2018-11-13 16:28 ` Alex Elder 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 0:32 ` Alex Elder 2018-11-07 12:17 ` Arnd Bergmann 2018-11-07 12:17 ` Arnd Bergmann 2018-11-13 16:33 ` Alex Elder 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 ` Alex Elder 2018-11-07 0:32 ` [RFC PATCH 04/12] soc: qcom: ipa: immediate commands Alex Elder 2018-11-07 0:32 ` Alex Elder 2018-11-07 14:36 ` Arnd Bergmann 2018-11-07 14:36 ` Arnd Bergmann 2018-11-13 16:58 ` Alex Elder 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 ` Alex Elder 2018-11-07 0:32 ` [RFC PATCH 06/12] soc: qcom: ipa: QMI modem communication Alex Elder 2018-11-07 0:32 ` Alex Elder 2018-11-07 0:32 ` [RFC PATCH 07/12] soc: qcom: ipa: IPA register abstraction Alex Elder 2018-11-07 0:32 ` Alex Elder 2018-11-07 15:00 ` Arnd Bergmann 2018-11-07 15:00 ` Arnd Bergmann 2018-11-15 2:48 ` Alex Elder 2018-11-15 2:48 ` Alex Elder 2018-11-15 14:42 ` Arnd Bergmann 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 ` Alex Elder 2018-11-07 0:32 ` [RFC PATCH 09/12] soc: qcom: ipa: main IPA source file Alex Elder 2018-11-07 0:32 ` Alex Elder 2018-11-07 14:08 ` Arnd Bergmann 2018-11-07 14:08 ` Arnd Bergmann 2018-11-15 3:11 ` Alex Elder [this message] 2018-11-15 3:11 ` Alex Elder 2018-11-07 0:32 ` [RFC PATCH 10/12] soc: qcom: ipa: data path Alex Elder 2018-11-07 0:32 ` Alex Elder 2018-11-07 14:55 ` Arnd Bergmann 2018-11-07 14:55 ` Arnd Bergmann 2018-11-15 3:31 ` Alex Elder 2018-11-15 3:31 ` Alex Elder 2018-11-15 14:48 ` Arnd Bergmann 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 0:32 ` Alex Elder 2018-11-07 13:30 ` Arnd Bergmann 2018-11-07 13:30 ` Arnd Bergmann 2018-11-07 15:26 ` Dan Williams 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:32 ` Alex Elder 2018-11-07 0:40 ` Randy Dunlap 2018-11-07 0:40 ` Randy Dunlap 2018-11-07 0:40 ` Randy Dunlap 2018-11-08 16:22 ` Alex Elder 2018-11-08 16:22 ` Alex Elder 2018-11-07 12:34 ` Arnd Bergmann 2018-11-07 12:34 ` Arnd Bergmann 2018-11-07 15:46 ` [RFC PATCH 00/12] net: introduce Qualcomm IPA driver Arnd Bergmann 2018-11-07 15:46 ` Arnd Bergmann
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=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: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.