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

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <27811a97-6368-dab2-5163-cbd0169b8666@linux.intel.com>
Date:   Mon, 8 Nov 2021 21:26:37 -0800
From:   "Martinez, Ricardo" <ricardo.martinez@...ux.intel.com>
To:     Sergey Ryazanov <ryazanov.s.a@...il.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



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