Re: [PATCH v2 03/14] net: wwan: t7xx: Add core components

[Sergey Ryazanov <ryazanov.s.a@gmail.com>]

On Mon, Nov 1, 2021 at 6:57 AM Ricardo Martinez
<ricardo.martinez@linux.intel.com> wrote:
> Registers the t7xx device driver with the kernel. Setup all the core
> components: PCIe layer, Modem Host Cross Core Interface (MHCCIF),
> modem control operations, modem state machine, and build
> infrastructure.
>
> * PCIe layer code implements driver probe and removal.
> * MHCCIF provides interrupt channels to communicate events
>   such as handshake, PM and port enumeration.
> * Modem control implements the entry point for modem init,
>   reset and exit.
> * The modem status monitor is a state machine used by modem control
>   to complete initialization and stop. It is used also to propagate
>   exception events reported by other components.

[skipped]

>  drivers/net/wwan/t7xx/t7xx_monitor.h       | 144 +++++
> ...
>  drivers/net/wwan/t7xx/t7xx_state_monitor.c | 598 +++++++++++++++++++++

Out of curiosity, why is this file called t7xx_state_monitor.c, while
the corresponding header file is called simply t7xx_monitor.h? Are any
other monitors planed?

[skipped]

> diff --git a/drivers/net/wwan/Kconfig b/drivers/net/wwan/Kconfig
> ...
> +config MTK_T7XX
> +       tristate "MediaTek PCIe 5G WWAN modem T7XX device"

As already suggested by Andy, using "T7xx" (lowercase x) in the title
to make it more readable.

> +       depends on PCI
> +       help
> +         Enables MediaTek PCIe based 5G WWAN modem (T700 series) device.

Maybe use the term "T7xx series" here too? Otherwise, it sounds like a
driver for the smartphone chipset only.

> +         Adapts WWAN framework and provides network interface like wwan0
> +         and tty interfaces like wwan0at0 (AT protocol), wwan0mbim0
> +         (MBIM protocol), etc.

[skipped]

> diff --git a/drivers/net/wwan/t7xx/t7xx_common.h b/drivers/net/wwan/t7xx/t7xx_common.h
> ...
> +struct ccci_header {
> +       /* do not assume data[1] is data length in rx */
> +       u32 data[2];

If these fields have different meaning on Tx and Rx you could define
them using a union. E.g.

        union {
                struct {
                       __le32 idx;
                       __le32 type;
                } rx;
                struct {
                       __le32 idx;
                      __le32 len;
                } tx;
        };
        __le32 status;

or even like this:

        __le32 idx;
        union {
                __le32 rx_type;
                __le32 tx_len;
        };
        __le32 status;

Such definition better documents code then the comment above the field.

> +       u32 status;
> +       u32 reserved;
> +};

All these fields should have a type of __le32 since the structure
passed to the modem as is.

[skipped]

> diff --git a/drivers/net/wwan/t7xx/t7xx_pci.c b/drivers/net/wwan/t7xx/t7xx_pci.c
> ...
> +static int __init mtk_pci_init(void)
> +{
> +       return pci_register_driver(&mtk_pci_driver);
> +}
> +module_init(mtk_pci_init);
> +
> +static void __exit mtk_pci_cleanup(void)
> +{
> +       pci_unregister_driver(&mtk_pci_driver);
> +}
> +module_exit(mtk_pci_cleanup);

Since the module does not do anything specific on (de-)initialization
use the module_pci_driver() macro instead of this boilerplate code.

[skipped]

> diff --git a/drivers/net/wwan/t7xx/t7xx_skb_util.c b/drivers/net/wwan/t7xx/t7xx_skb_util.c
> ...
> +static struct sk_buff *alloc_skb_from_pool(struct skb_pools *pools, size_t size)
> +{
> +       if (size > MTK_SKB_4K)
> +               return ccci_skb_dequeue(pools->reload_work_queue, &pools->skb_pool_64k);
> +       else if (size > MTK_SKB_16)
> +               return ccci_skb_dequeue(pools->reload_work_queue, &pools->skb_pool_4k);
> +       else if (size > 0)
> +               return ccci_skb_dequeue(pools->reload_work_queue, &pools->skb_pool_16);
> +
> +       return NULL;
> +}
> +
> +static struct sk_buff *alloc_skb_from_kernel(size_t size, gfp_t gfp_mask)
> +{
> +       if (size > MTK_SKB_4K)
> +               return __dev_alloc_skb(MTK_SKB_64K, gfp_mask);
> +       else if (size > MTK_SKB_1_5K)
> +               return __dev_alloc_skb(MTK_SKB_4K, gfp_mask);
> +       else if (size > MTK_SKB_16)
> +               return __dev_alloc_skb(MTK_SKB_1_5K, gfp_mask);
> +       else if (size > 0)
> +               return __dev_alloc_skb(MTK_SKB_16, gfp_mask);
> +
> +       return NULL;
> +}

I am wondering what performance gains have you achieved with these skb
pools? Can we see any numbers?

I do not think the control path performance is worth the complexity of
the multilayer skb allocation. In the data packet Rx path, you need to
allocate skb anyway as soon as the driver passes them to the stack. So
what is the gain?

[skipped]

> diff --git a/drivers/net/wwan/t7xx/t7xx_state_monitor.c b/drivers/net/wwan/t7xx/t7xx_state_monitor.c
> ...
> +static struct ccci_fsm_ctl *ccci_fsm_entry;

Place this pointer at least to the mtk_modem structure. Otherwise,
with this global pointer, the driver will break as soon as a second
modem will be connected to the host.

Also all functions in this file also should be reworked to be able to
accept modem state container pointer, e.g. mtk_modem or some other
related structure.

--
Sergey