LKML Archive on lore.kernel.org
 help / color / Atom feed
From: Arnd Bergmann <arnd@arndb.de>
To: Alex Elder <elder@linaro.org>
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, 7 Nov 2018 15:08:46 +0100
Message-ID: <CAK8P3a0RH4wbigKR0zsbxPzqWDjrLEkYfQk4GNxhGuxs=HJHHw@mail.gmail.com> (raw)
In-Reply-To: <20181107003250.5832-10-elder@linaro.org>

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);
> +
> +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.

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

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.

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

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

        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 [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 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='CAK8P3a0RH4wbigKR0zsbxPzqWDjrLEkYfQk4GNxhGuxs=HJHHw@mail.gmail.com' \
    --to=arnd@arndb.de \
    --cc=bjorn.andersson@linaro.org \
    --cc=davem@davemloft.net \
    --cc=devicetree@vger.kernel.org \
    --cc=elder@linaro.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
	public-inbox-index lkml

Example config snippet for mirrors

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