Date:   Sat, 6 Nov 2021 21:10:57 +0300
From:   Sergey Ryazanov <ryazanov.s.a@...il.com>
To:     Ricardo Martinez <ricardo.martinez@...ux.intel.com>
Cc:     netdev@...r.kernel.org, linux-wireless@...r.kernel.org,
        Jakub Kicinski <kuba@...nel.org>,
        David Miller <davem@...emloft.net>,
        Johannes Berg <johannes@...solutions.net>,
        Loic Poulain <loic.poulain@...aro.org>,
        M Chetan Kumar <m.chetan.kumar@...el.com>,
        chandrashekar.devegowda@...el.com,
        Intel Corporation <linuxwwan@...el.com>,
        chiranjeevi.rapolu@...ux.intel.com, haijun.liu@...iatek.com,
        amir.hanania@...el.com,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        dinesh.sharma@...el.com, eliot.lee@...el.com,
        mika.westerberg@...ux.intel.com, moises.veleta@...el.com,
        pierre-louis.bossart@...el.com, muralidharan.sethuraman@...el.com,
        Soumya.Prakash.Mishra@...el.com, sreehari.kancharla@...el.com,
        suresh.nagaraj@...el.com
Subject: Re: [PATCH v2 00/14] net: wwan: t7xx: PCIe driver for MediaTek M.2 modem

Hello Ricardo,

On Mon, Nov 1, 2021 at 6:57 AM Ricardo Martinez
<ricardo.martinez@...ux.intel.com> wrote:
> t7xx is the PCIe host device driver for Intel 5G 5000 M.2 solution which
> is based on MediaTek's T700 modem to provide WWAN connectivity.
> The driver uses the WWAN framework infrastructure to create the following
> control ports and network interfaces:
> * /dev/wwan0mbim0 - Interface conforming to the MBIM protocol.
>   Applications like libmbim [1] or Modem Manager [2] from v1.16 onwards
>   with [3][4] can use it to enable data communication towards WWAN.
> * /dev/wwan0at0 - Interface that supports AT commands.
> * wwan0 - Primary network interface for IP traffic.
>
> The main blocks in t7xx driver are:
> * PCIe layer - Implements probe, removal, and power management callbacks.
> * Port-proxy - Provides a common interface to interact with different types
>   of ports such as WWAN ports.
> * Modem control & status monitor - Implements the entry point for modem
>   initialization, reset and exit, as well as exception handling.
> * CLDMA (Control Layer DMA) - Manages the HW used by the port layer to send
>   control messages to the modem using MediaTek's CCCI (Cross-Core
>   Communication Interface) protocol.
> * DPMAIF (Data Plane Modem AP Interface) - Controls the HW that provides
>   uplink and downlink queues for the data path. The data exchange takes
>   place using circular buffers to share data buffer addresses and metadata
>   to describe the packets.
> * MHCCIF (Modem Host Cross-Core Interface) - Provides interrupt channels
>   for bidirectional event notification such as handshake, exception, PM and
>   port enumeration.
>
> The compilation of the t7xx driver is enabled by the CONFIG_MTK_T7XX config
> option which depends on CONFIG_WWAN.
> This driver was originally developed by MediaTek. Intel adapted t7xx to
> the WWAN framework, optimized and refactored the driver source in close
> collaboration with MediaTek. This will enable getting the t7xx driver on
> Approved Vendor List for interested OEM's and ODM's productization plans
> with Intel 5G 5000 M.2 solution.

Nice work! The driver generally looks good for me. But at the same
time the driver looks a bit raw, needs some style and functionality
improvements, and a lot of cleanup. Please find general thoughts below
and per-patch comments.

A one nitpick that is common for the entire series. Please consider
using a common prefix for all driver function names (e.g. t7xx_) to
make them more specific. This should improve the code readability.
Thus, any reader will know for sure that the called functions belong
to the driver, and not to a generic kernel API. E.g. use the
t7xx_cldma_hw_init() name for the  CLDMA initialization function
instead of the too generic cldma_hw_init() name, etc.

Interestingly, that you are using the common 't7xx_' prefix for all
driver file names. This is Ok, but it does not add to the specifics as
all driver files are already located in a common directory with the
specific name. But function names at the same time lack a common
prefix.

Another common drawback is that the driver should break as soon as two
modems are connected simultaneously. This should happen due to the use
of multiple _global_ variables that keeps pointers to a modem runtime
state. Out of curiosity, did you test the driver with two or more
modems connected simultaneously?

Next, the driver entirely lacks the multibyte field endians handling.
Looks like it will be unable to run on a big-endians CPU. To fix this,
it is needed to find all the structures that are passed to the modem
and replace the multibyte fields of types u16/u32 with __le16/__le32.
Then examine all the field accesses and use
cpu_to_le{16,32}()/le{16,32}_to_cpu() to update/read field contents.
As soon as you change the types to __le16/__le32, sparse (a static
analyzing utility) will warn you about every unsafe field access. Just
build your kernel with make C=1.

Ricardo, please consider submitting at the next iteration a patch
series with the driver that will be cleaned from debug stuff and
questionable optimizations. Just a bare minimum functional: AT/MBIM
control ports and network interface for data communications. This will
cut the code in half. What will greatly facilitate the reviewing
process. And then extend the driver functionality with follow up
patches.

--
Sergey

Powered by blists - more mailing lists