| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <09719f6e-a886-68b8-3fb9-cc0a92db41af@ieee.org>
Date: Wed, 13 Oct 2021 17:27:25 -0500
From: Alex Elder <elder@...e.org>
To: Sireesh Kodali <sireeshkodali1@...il.com>,
phone-devel@...r.kernel.org, ~postmarketos/upstreaming@...ts.sr.ht,
netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-arm-msm@...r.kernel.org, elder@...nel.org
Subject: Re: [RFC PATCH 00/17] net: ipa: Add support for IPA v2.x
On 9/19/21 10:07 PM, Sireesh Kodali wrote:
> Hi,
>
> This RFC patch series adds support for IPA v2, v2.5 and v2.6L
> (collectively referred to as IPA v2.x).
I'm sorry for the delay on this. I want to give this a
reasonable review, but it's been hard to prioritize doing
so. So for now I aim to give you some "easy" feedback,
knowing that this doesn't cover all issues. This is an
RFC, after all...
So this isn't a "real review" but I'll try to be helpful.
Overall, I appreciate how well you adhered to the patterns and
conventions used elsewhere in the driver. There are many levels
to that, but I think consistency is a huge factor in keeping
code maintainable. I didn't see all that many places where
I felt like whining about naming you used, or oddities in
indentation, and so on.
Abstracting the GSI layer seemed to be done more easily than
I expected. I didn't dive deep into the BAM code, and would
want to pay much closer attention to that in the future.
The BAM/GSI difference is the biggest one dividing IPA v3.0+
from its predecessors. But as you see, the 32- versus 64-bit
address and field size differences lead to some ugliness
that's hard to avoid.
Anyway, nice work; I hope my feedback is helpful.
-Alex
> Basic description:
> IPA v2.x is the older version of the IPA hardware found on Qualcomm
> SoCs. The biggest differences between v2.x and later versions are:
> - 32 bit hardware (the IPA microcontroler is 32 bit)
> - BAM (as opposed to GSI as a DMA transport)
> - Changes to the QMI init sequence (described in the commit message)
>
> The fact that IPA v2.x are 32 bit only affects us directly in the table
> init code. However, its impact is felt in other parts of the code, as it
> changes the size of fields of various structs (e.g. in the commands that
> can be sent).
>
> BAM support is already present in the mainline kernel, however it lacks
> two things:
> - Support for DMA metadata, to pass the size of the transaction from the
> hardware to the dma client
> - Support for immediate commands, which are needed to pass commands from
> the driver to the microcontroller
>
> Separate patch series have been created to deal with these (linked in
> the end)
>
> This patch series adds support for BAM as a transport by refactoring the
> current GSI code to create an abstract uniform API on top. This API
> allows the rest of the driver to handle DMA without worrying about the
> IPA version.
>
> The final thing that hasn't been touched by this patch series is the IPA
> resource manager. On the downstream CAF kernel, the driver seems to
> share the resource code between IPA v2.x and IPA v3.x, which should mean
> all it would take to add support for resources on IPA v2.x would be to
> add the definitions in the ipa_data.
>
> Testing:
> This patch series was tested on kernel version 5.13 on a phone with
> SDM625 (IPA v2.6L), and a phone with MSM8996 (IPA v2.5). The phone with
> IPA v2.5 was able to get an IP address using modem-manager, although
> sending/receiving packets was not tested. The phone with IPA v2.6L was
> able to get an IP, but was unable to send/receive packets. Its modem
> also relies on IPA v2.6l's compression/decompression support, and
> without this patch series, the modem simply crashes and restarts,
> waiting for the IPA block to come up.
>
> This patch series is based on code from the downstream CAF kernel v4.9
>
> There are some things in this patch series that would obviously not get
> accepted in their current form:
> - All IPA 2.x data is in a single file
> - Some stray printks might still be around
> - Some values have been hardcoded (e.g. the filter_map)
> Please excuse these
>
> Lastly, this patch series depends upon the following patches for BAM:
> [0]: https://lkml.org/lkml/2021/9/19/126
> [1]: https://lkml.org/lkml/2021/9/19/135
>
> Regards,
> Sireesh Kodali
>
> Sireesh Kodali (10):
> net: ipa: Add IPA v2.x register definitions
> net: ipa: Add support for using BAM as a DMA transport
> net: ipa: Add support for IPA v2.x commands and table init
> net: ipa: Add support for IPA v2.x endpoints
> net: ipa: Add support for IPA v2.x memory map
> net: ipa: Add support for IPA v2.x in the driver's QMI interface
> net: ipa: Add support for IPA v2 microcontroller
> net: ipa: Add IPA v2.6L initialization sequence support
> net: ipa: Add hw config describing IPA v2.x hardware
> dt-bindings: net: qcom,ipa: Add support for MSM8953 and MSM8996 IPA
>
> Vladimir Lypak (7):
> net: ipa: Correct ipa_status_opcode enumeration
> net: ipa: revert to IPA_TABLE_ENTRY_SIZE for 32-bit IPA support
> net: ipa: Refactor GSI code
> net: ipa: Establish ipa_dma interface
> net: ipa: Check interrupts for availability
> net: ipa: Add timeout for ipa_cmd_pipeline_clear_wait
> net: ipa: Add support for IPA v2.x interrupts
>
> .../devicetree/bindings/net/qcom,ipa.yaml | 2 +
> drivers/net/ipa/Makefile | 11 +-
> drivers/net/ipa/bam.c | 525 ++++++++++++++++++
> drivers/net/ipa/gsi.c | 322 ++++++-----
> drivers/net/ipa/ipa.h | 8 +-
> drivers/net/ipa/ipa_cmd.c | 244 +++++---
> drivers/net/ipa/ipa_cmd.h | 20 +-
> drivers/net/ipa/ipa_data-v2.c | 369 ++++++++++++
> drivers/net/ipa/ipa_data-v3.1.c | 2 +-
> drivers/net/ipa/ipa_data-v3.5.1.c | 2 +-
> drivers/net/ipa/ipa_data-v4.11.c | 2 +-
> drivers/net/ipa/ipa_data-v4.2.c | 2 +-
> drivers/net/ipa/ipa_data-v4.5.c | 2 +-
> drivers/net/ipa/ipa_data-v4.9.c | 2 +-
> drivers/net/ipa/ipa_data.h | 4 +
> drivers/net/ipa/{gsi.h => ipa_dma.h} | 179 +++---
> .../ipa/{gsi_private.h => ipa_dma_private.h} | 46 +-
> drivers/net/ipa/ipa_endpoint.c | 188 ++++---
> drivers/net/ipa/ipa_endpoint.h | 6 +-
> drivers/net/ipa/ipa_gsi.c | 18 +-
> drivers/net/ipa/ipa_gsi.h | 12 +-
> drivers/net/ipa/ipa_interrupt.c | 36 +-
> drivers/net/ipa/ipa_main.c | 82 ++-
> drivers/net/ipa/ipa_mem.c | 55 +-
> drivers/net/ipa/ipa_mem.h | 5 +-
> drivers/net/ipa/ipa_power.c | 4 +-
> drivers/net/ipa/ipa_qmi.c | 37 +-
> drivers/net/ipa/ipa_qmi.h | 10 +
> drivers/net/ipa/ipa_reg.h | 184 +++++-
> drivers/net/ipa/ipa_resource.c | 3 +
> drivers/net/ipa/ipa_smp2p.c | 11 +-
> drivers/net/ipa/ipa_sysfs.c | 6 +
> drivers/net/ipa/ipa_table.c | 86 +--
> drivers/net/ipa/ipa_table.h | 6 +-
> drivers/net/ipa/{gsi_trans.c => ipa_trans.c} | 182 +++---
> drivers/net/ipa/{gsi_trans.h => ipa_trans.h} | 78 +--
> drivers/net/ipa/ipa_uc.c | 96 ++--
> drivers/net/ipa/ipa_version.h | 12 +
> 38 files changed, 2133 insertions(+), 726 deletions(-)
> create mode 100644 drivers/net/ipa/bam.c
> create mode 100644 drivers/net/ipa/ipa_data-v2.c
> rename drivers/net/ipa/{gsi.h => ipa_dma.h} (57%)
> rename drivers/net/ipa/{gsi_private.h => ipa_dma_private.h} (66%)
> rename drivers/net/ipa/{gsi_trans.c => ipa_trans.c} (80%)
> rename drivers/net/ipa/{gsi_trans.h => ipa_trans.h} (71%)
>
Powered by blists - more mailing lists