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 04/12] soc: qcom: ipa: immediate commands Date: Wed, 7 Nov 2018 15:36:50 +0100 [thread overview] Message-ID: <CAK8P3a3iKshE3yCG477AeZbKbLtpDwMhn5mfLySFRLOLmUv5AQ@mail.gmail.com> (raw) In-Reply-To: <20181107003250.5832-5-elder@linaro.org> On Wed, Nov 7, 2018 at 1:33 AM Alex Elder <elder@linaro.org> wrote: > > +/** > + * struct ipahal_context - HAL global context data > + * @empty_fltrt_tbl: Empty table to be used for table initialization > + */ > +static struct ipahal_context { > + struct ipa_dma_mem empty_fltrt_tbl; > +} ipahal_ctx_struct; > +static struct ipahal_context *ipahal_ctx = &ipahal_ctx_struct; Remove the global variables here > +/* Immediate commands H/W structures */ > + > +/* struct ipa_imm_cmd_hw_ip_fltrt_init - IP_V*_FILTER_INIT/IP_V*_ROUTING_INIT > + * command payload in H/W format. > + * Inits IPv4/v6 routing or filter block. > + * @hash_rules_addr: Addr in system mem where hashable flt/rt rules starts > + * @hash_rules_size: Size in bytes of the hashable tbl to cpy to local mem > + * @hash_local_addr: Addr in shared mem where hashable flt/rt tbl should > + * be copied to > + * @nhash_rules_size: Size in bytes of the non-hashable tbl to cpy to local mem > + * @nhash_local_addr: Addr in shared mem where non-hashable flt/rt tbl should > + * be copied to > + * @rsvd: reserved > + * @nhash_rules_addr: Addr in sys mem where non-hashable flt/rt tbl starts > + */ > +struct ipa_imm_cmd_hw_ip_fltrt_init { > + u64 hash_rules_addr; > + u64 hash_rules_size : 12, > + hash_local_addr : 16, > + nhash_rules_size : 12, > + nhash_local_addr : 16, > + rsvd : 8; > + u64 nhash_rules_addr; > +}; In hardware structures, you should not use bit fields, as the ordering of the bits is not well-defined in C. The only portable way to do this is to use shifts and masks unfortunately. > +struct ipa_imm_cmd_hw_hdr_init_local { > + u64 hdr_table_addr; > + u32 size_hdr_table : 12, > + hdr_addr : 16, > + rsvd : 4; > +}; I would also add a 'u32 pad' member at the end to make the padding explicit here, or mark the first member as '__aligned(4) __packed' if you want to avoid the padding. > +void *ipahal_dma_shared_mem_write_pyld(struct ipa_dma_mem *mem, u32 offset) > +{ > + struct ipa_imm_cmd_hw_dma_shared_mem *data; > + > + ipa_assert(mem->size < 1 << 16); /* size is 16 bits wide */ > + ipa_assert(offset < 1 << 16); /* local_addr is 16 bits wide */ > + > + data = kzalloc(sizeof(*data), GFP_KERNEL); > + if (!data) > + return NULL; > + > + data->size = mem->size; > + data->local_addr = offset; > + data->direction = 0; /* 0 = write to IPA; 1 = read from IPA */ > + data->skip_pipeline_clear = 0; > + data->pipeline_clear_options = IPAHAL_HPS_CLEAR; > + data->system_addr = mem->phys; > + > + return data; > +} The 'void *' return looks odd here, and also the dynamic allocation. It looks to me like all these functions could be better done the other way round, basically putting the ipa_imm_cmd_hw_dma_shared_mem etc structures on the stack of the caller. At least for this one, the dynamic allocation doesn't help at all because the caller is the same that frees it again after the command. I suspect the same is true for a lot of those commands. Arnd
WARNING: multiple messages have this Message-ID (diff)
From: arnd@arndb.de (Arnd Bergmann) To: linux-arm-kernel@lists.infradead.org Subject: [RFC PATCH 04/12] soc: qcom: ipa: immediate commands Date: Wed, 7 Nov 2018 15:36:50 +0100 [thread overview] Message-ID: <CAK8P3a3iKshE3yCG477AeZbKbLtpDwMhn5mfLySFRLOLmUv5AQ@mail.gmail.com> (raw) In-Reply-To: <20181107003250.5832-5-elder@linaro.org> On Wed, Nov 7, 2018 at 1:33 AM Alex Elder <elder@linaro.org> wrote: > > +/** > + * struct ipahal_context - HAL global context data > + * @empty_fltrt_tbl: Empty table to be used for table initialization > + */ > +static struct ipahal_context { > + struct ipa_dma_mem empty_fltrt_tbl; > +} ipahal_ctx_struct; > +static struct ipahal_context *ipahal_ctx = &ipahal_ctx_struct; Remove the global variables here > +/* Immediate commands H/W structures */ > + > +/* struct ipa_imm_cmd_hw_ip_fltrt_init - IP_V*_FILTER_INIT/IP_V*_ROUTING_INIT > + * command payload in H/W format. > + * Inits IPv4/v6 routing or filter block. > + * @hash_rules_addr: Addr in system mem where hashable flt/rt rules starts > + * @hash_rules_size: Size in bytes of the hashable tbl to cpy to local mem > + * @hash_local_addr: Addr in shared mem where hashable flt/rt tbl should > + * be copied to > + * @nhash_rules_size: Size in bytes of the non-hashable tbl to cpy to local mem > + * @nhash_local_addr: Addr in shared mem where non-hashable flt/rt tbl should > + * be copied to > + * @rsvd: reserved > + * @nhash_rules_addr: Addr in sys mem where non-hashable flt/rt tbl starts > + */ > +struct ipa_imm_cmd_hw_ip_fltrt_init { > + u64 hash_rules_addr; > + u64 hash_rules_size : 12, > + hash_local_addr : 16, > + nhash_rules_size : 12, > + nhash_local_addr : 16, > + rsvd : 8; > + u64 nhash_rules_addr; > +}; In hardware structures, you should not use bit fields, as the ordering of the bits is not well-defined in C. The only portable way to do this is to use shifts and masks unfortunately. > +struct ipa_imm_cmd_hw_hdr_init_local { > + u64 hdr_table_addr; > + u32 size_hdr_table : 12, > + hdr_addr : 16, > + rsvd : 4; > +}; I would also add a 'u32 pad' member at the end to make the padding explicit here, or mark the first member as '__aligned(4) __packed' if you want to avoid the padding. > +void *ipahal_dma_shared_mem_write_pyld(struct ipa_dma_mem *mem, u32 offset) > +{ > + struct ipa_imm_cmd_hw_dma_shared_mem *data; > + > + ipa_assert(mem->size < 1 << 16); /* size is 16 bits wide */ > + ipa_assert(offset < 1 << 16); /* local_addr is 16 bits wide */ > + > + data = kzalloc(sizeof(*data), GFP_KERNEL); > + if (!data) > + return NULL; > + > + data->size = mem->size; > + data->local_addr = offset; > + data->direction = 0; /* 0 = write to IPA; 1 = read from IPA */ > + data->skip_pipeline_clear = 0; > + data->pipeline_clear_options = IPAHAL_HPS_CLEAR; > + data->system_addr = mem->phys; > + > + return data; > +} The 'void *' return looks odd here, and also the dynamic allocation. It looks to me like all these functions could be better done the other way round, basically putting the ipa_imm_cmd_hw_dma_shared_mem etc structures on the stack of the caller. At least for this one, the dynamic allocation doesn't help at all because the caller is the same that frees it again after the command. I suspect the same is true for a lot of those commands. Arnd
next prev parent reply other threads:[~2018-11-07 14:36 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 [this message] 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 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=CAK8P3a3iKshE3yCG477AeZbKbLtpDwMhn5mfLySFRLOLmUv5AQ@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: 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.