Re: [PATCH v2 00/14] net: wwan: t7xx: PCIe driver for MediaTek M.2 modem

["Martinez, Ricardo" <ricardo.martinez@linux.intel.com>]

On 11/6/2021 11:10 AM, Sergey Ryazanov wrote:
> Hello Ricardo,
> 
> On Mon, Nov 1, 2021 at 6:57 AM Ricardo Martinez
> <ricardo.martinez@linux.intel.com> wrote:
...
> 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.
Thanks for the feedback Sergey, we will work on it.

> 
> 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.
Does this apply to static functions as well?

> 
> 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?
We haven't tested such configurations, we are focusing on platforms with one single modem.

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