Re: [PATCH v2 03/14] net: wwan: t7xx: Add core components
From: Sergey Ryazanov <ryazanov.s.a@gmail.com>
Date: 2021-11-06 18:04:30
Also in:
netdev
On Mon, Nov 1, 2021 at 6:57 AM Ricardo Martinez [off-list ref] 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]
quoted hunk ↗ jump to hunk
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]
quoted hunk ↗ jump to hunk
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]
quoted hunk ↗ jump to hunk
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]
quoted hunk ↗ jump to hunk
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]
quoted hunk ↗ jump to hunk
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