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 04/12] soc: qcom: ipa: immediate commands
Date: Tue, 13 Nov 2018 10:58:02 -0600
Message-ID: <dbaf1c86-ab9d-fa90-6718-998f1f1b6dd3@linaro.org> (raw)
In-Reply-To: <CAK8P3a3iKshE3yCG477AeZbKbLtpDwMhn5mfLySFRLOLmUv5AQ@mail.gmail.com>

On 11/7/18 8:36 AM, Arnd Bergmann wrote:
> 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

Not done yet, but I will do this.  I've been working on eliminating
the top-level "ipa_ctx" global (which is *very* pervasive) and in
the process I'm eliminating all the others as well.  I'll get to
this soon.

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

This is something I held off fixing because I have seen other use
of bit fields in the kernel.  I wasn't sure whether my instinct about
it (which matches what you say) was wrong, and didn't want to do the
work to change things over to masks without knowing.  Based on your
suggestion, I will proceed with this conversion.

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

Yes, this is a good suggestion, and I will implement it.

You're right that the actual size of this structure includes the
extra 4 byte pad.  But I'm not actually sure whether the hardware
touches it because the size of immediate commands is implied by
the opcode.  To be safe, I'll make the pad explicit; but if I
learn it's not needed I'll define it to be packed.

>> +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 was done because it allows the definition of the data structure
to be hidden within this file.

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

Yes, I see what you're saying.  In fact, now that I look, all of
these payload allocating functions except for one are used just
the way you describe (freed in the same function that uses it).
And the one is saved with the intention of avoiding an allocation
failure...  But I'll mention that this code was structured very
differently originally.

So I agree, putting them on the stack (given they're relatively
small--most 16 bytes one 24 bytes) is better.  And it seems I
can reduce some complexity by getting rid of that preallocated
command, which is a great outcome.

If I run into trouble implementing any of the above suggestions
I will circle back and explain.

Thanks a lot.

					-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 [this message]
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
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=dbaf1c86-ab9d-fa90-6718-998f1f1b6dd3@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