Re: [PATCH v2 03/14] net: wwan: t7xx: Add core components
flat view
From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Date: 2021-11-02 16:40:26
Also in:
linux-wireless
On Sun, Oct 31, 2021 at 08:56:24PM -0700, Ricardo Martinez wrote:
From: Haijun Lio <haijun.liu@mediatek.com> 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.
I will assume that the comments given against previous patch will be applied to this and the rest of the patches where it makes sense or appropriate. Below only new comments. ...
+config MTK_T7XX + tristate "MediaTek PCIe 5G WWAN modem T7XX device"
T77xx is easier to read by human beings.
+ depends on PCI
...
+struct ccci_header {
+ /* do not assume data[1] is data length in rx */To understand this comment you need to elaborate the content of the header.
quoted hunk ↗ jump to hunk
+ u32 data[2]; + u32 status; + u32 reserved; +};
...
+#define CCCI_HEADER_NO_DATA 0xffffffff
Is this internal value to Linux or something which is given by hardware? ...
quoted hunk ↗ jump to hunk
+/* Modem exception check identification number */ +#define MD_EX_CHK_ID 0x45584350 +/* Modem exception check acknowledge identification number */ +#define MD_EX_CHK_ACK_ID 0x45524543
To me both looks like fourcc. Can you add their ASCII values into comments? ...
+ /* Use 1*4 bits to avoid low power bits*/
What does "1*4 bits" mean?
+ iowrite32(L1_1_DISABLE_BIT(1) | L1_2_DISABLE_BIT(1), + IREG_BASE(mtk_dev) + DIS_ASPM_LOWPWR_SET_0);
...
quoted hunk ↗ jump to hunk
+int mtk_pci_mhccif_isr(struct mtk_pci_dev *mtk_dev) +{ + struct md_sys_info *md_info; + struct ccci_fsm_ctl *ctl; + struct mtk_modem *md; + unsigned int int_sta; + unsigned long flags; + u32 mask; + + md = mtk_dev->md; + ctl = fsm_get_entry(); + if (!ctl) {
+ dev_err(&mtk_dev->pdev->dev, + "process MHCCIF interrupt before modem monitor was initialized\n");
Can this potentially flood the logs? If so, needs to be rate limited.
+ return -EINVAL; + }
+ md_info = md->md_info;
+ spin_lock_irqsave(&md_info->exp_spinlock, flags);
Can it be called outside of IRQ context?
quoted hunk ↗ jump to hunk
+ int_sta = get_interrupt_status(mtk_dev); + md_info->exp_id |= int_sta; + + if (md_info->exp_id & D2H_INT_PORT_ENUM) { + md_info->exp_id &= ~D2H_INT_PORT_ENUM; + if (ctl->curr_state == CCCI_FSM_INIT || + ctl->curr_state == CCCI_FSM_PRE_START || + ctl->curr_state == CCCI_FSM_STOPPED) + ccci_fsm_recv_md_interrupt(MD_IRQ_PORT_ENUM); + } + + if (md_info->exp_id & D2H_INT_EXCEPTION_INIT) { + if (ctl->md_state == MD_STATE_INVALID || + ctl->md_state == MD_STATE_WAITING_FOR_HS1 || + ctl->md_state == MD_STATE_WAITING_FOR_HS2 || + ctl->md_state == MD_STATE_READY) { + md_info->exp_id &= ~D2H_INT_EXCEPTION_INIT; + ccci_fsm_recv_md_interrupt(MD_IRQ_CCIF_EX); + } + } else if (ctl->md_state == MD_STATE_WAITING_FOR_HS1) { + /* start handshake if MD not assert */ + mask = mhccif_mask_get(mtk_dev); + if ((md_info->exp_id & D2H_INT_ASYNC_MD_HK) && !(mask & D2H_INT_ASYNC_MD_HK)) { + md_info->exp_id &= ~D2H_INT_ASYNC_MD_HK; + queue_work(md->handshake_wq, &md->handshake_work); + } + } + + spin_unlock_irqrestore(&md_info->exp_spinlock, flags); + + return 0; +}
...
quoted hunk ↗ jump to hunk
+static int mtk_acpi_reset(struct mtk_pci_dev *mtk_dev, char *fn_name) +{ +#ifdef CONFIG_ACPI + struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL }; + acpi_status acpi_ret; + struct device *dev; + acpi_handle handle; + + dev = &mtk_dev->pdev->dev;
quoted hunk ↗ jump to hunk
+ if (acpi_disabled) { + dev_err(dev, "acpi function isn't enabled\n"); + return -EFAULT; + }
Why this check?
quoted hunk ↗ jump to hunk
+ handle = ACPI_HANDLE(dev); + if (!handle) { + dev_err(dev, "acpi handle isn't found\n");
acpi --> ACPI
quoted hunk ↗ jump to hunk
+ return -EFAULT; + } + + if (!acpi_has_method(handle, fn_name)) { + dev_err(dev, "%s method isn't found\n", fn_name); + return -EFAULT; + } + + acpi_ret = acpi_evaluate_object(handle, fn_name, NULL, &buffer); + if (ACPI_FAILURE(acpi_ret)) { + dev_err(dev, "%s method fail: %s\n", fn_name, acpi_format_exception(acpi_ret)); + return -EFAULT; + } +#endif + return 0; +}
...
+ msleep(RGU_RESET_DELAY_US);
DELAY in microseconds while msleep() takes milliseconds. Something is wrong here. Also, delays such as 10ms+ should be explained. Esp. when they are in the threaded IRQ handler. ...
quoted hunk ↗ jump to hunk
+void mtk_md_exception_handshake(struct mtk_modem *md) +{ + struct mtk_pci_dev *mtk_dev;
struct device *dev = &mtk_dev->pdev->dev; will help a lot to make below code cleaner.
+ int ret;
quoted hunk ↗ jump to hunk
+ mtk_dev = md->mtk_dev; + md_exception(md, HIF_EX_INIT); + ret = wait_hif_ex_hk_event(md, D2H_INT_EXCEPTION_INIT_DONE); + + if (ret) + dev_err(&mtk_dev->pdev->dev, "EX CCIF HS timeout, RCH 0x%lx\n", + D2H_INT_EXCEPTION_INIT_DONE); + + md_exception(md, HIF_EX_INIT_DONE); + ret = wait_hif_ex_hk_event(md, D2H_INT_EXCEPTION_CLEARQ_DONE); + if (ret) + dev_err(&mtk_dev->pdev->dev, "EX CCIF HS timeout, RCH 0x%lx\n", + D2H_INT_EXCEPTION_CLEARQ_DONE); + + md_exception(md, HIF_EX_CLEARQ_DONE); + ret = wait_hif_ex_hk_event(md, D2H_INT_EXCEPTION_ALLQ_RESET); + if (ret) + dev_err(&mtk_dev->pdev->dev, "EX CCIF HS timeout, RCH 0x%lx\n", + D2H_INT_EXCEPTION_ALLQ_RESET); + + md_exception(md, HIF_EX_ALLQ_RESET); +}
...
quoted hunk ↗ jump to hunk
+err_fsm_init: + ccci_fsm_uninit(); +err_alloc: + destroy_workqueue(md->handshake_wq);
Labels should explain what will be done when goto, and not what was done. ...
+/* Modem feature query identification number */ +#define MD_FEATURE_QUERY_ID 0x49434343
All fourcc:s should be represented as ASCII in the comments. ...
+#ifndef __T7XX_MONITOR_H__ +#define __T7XX_MONITOR_H__
+#include <linux/sched.h>
Who is the user of this? ...
quoted hunk ↗ jump to hunk
+static int mtk_request_irq(struct pci_dev *pdev) +{ + struct mtk_pci_dev *mtk_dev; + int ret, i; + + mtk_dev = pci_get_drvdata(pdev); + + for (i = 0; i < EXT_INT_NUM; i++) { + const char *irq_descr; + int irq_vec; + + if (!mtk_dev->intr_handler[i]) + continue; + + irq_descr = devm_kasprintf(&pdev->dev, GFP_KERNEL, "%s_%d", + dev_driver_string(&pdev->dev), i); + if (!irq_descr)
Resource leak is here.
quoted hunk ↗ jump to hunk
+ return -ENOMEM; + + irq_vec = pci_irq_vector(pdev, i); + ret = request_threaded_irq(irq_vec, mtk_dev->intr_handler[i], + mtk_dev->intr_thread[i], 0, irq_descr, + mtk_dev->callback_param[i]); + if (ret) { + dev_err(&pdev->dev, "Failed to request_irq: %d, int: %d, ret: %d\n", + irq_vec, i, ret); + while (i--) { + if (!mtk_dev->intr_handler[i]) + continue; + + free_irq(pci_irq_vector(pdev, i), mtk_dev->callback_param[i]); + } + + return ret; + } + } + + return 0; +}
...
quoted hunk ↗ jump to hunk
+ ret = pci_alloc_irq_vectors(mtk_dev->pdev, EXT_INT_NUM, EXT_INT_NUM, PCI_IRQ_MSIX); + if (ret < 0) { + dev_err(&mtk_dev->pdev->dev, "Failed to allocate MSI-X entry, errno: %d\n", ret);
', errno' is redundant.
quoted hunk ↗ jump to hunk
+ return ret; + } + + ret = mtk_request_irq(mtk_dev->pdev); + if (ret) { + pci_free_irq_vectors(mtk_dev->pdev); + return ret; + } + + /* Set MSIX merge config */ + mtk_pcie_mac_msix_cfg(mtk_dev, EXT_INT_NUM); + return 0; +}
...
+ ret = pci_set_dma_mask(pdev, DMA_BIT_MASK(64));
This API is absoleted, use corresponding DMA API directly. On top of that, 64-bit setting never fail...
+ if (ret) {quoted hunk ↗ jump to hunk
+ ret = pci_set_dma_mask(pdev, DMA_BIT_MASK(32)); + if (ret) { + dev_err(&pdev->dev, "Could not set PCI DMA mask, err: %d\n", ret); + return ret; + }
...so this attempt is almost a dead code.
+ }
...
quoted hunk ↗ jump to hunk
+ ret = pci_set_consistent_dma_mask(pdev, DMA_BIT_MASK(64)); + if (ret) { + ret = pci_set_consistent_dma_mask(pdev, DMA_BIT_MASK(32)); + if (ret) { + dev_err(&pdev->dev, "Could not set consistent PCI DMA mask, err: %d\n", + ret); + return ret; + } + }
Ditto. ...
+ mtk_pcie_mac_set_int(mtk_dev, MHCCIF_INT); + mtk_pcie_mac_interrupts_en(mtk_dev);
+ pci_set_master(pdev);
It's too late for this call. Are you sure it's needed here? Why?
+ + return 0;
...
+err:
Meaningless label name. Try your best to make it better.
+ ccci_skb_pool_free(&mtk_dev->pools);
Does it free IRQ handlers? If so, the function naming is not good enough.
+ return ret;
...
quoted hunk ↗ jump to hunk
+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);
NIH module_pci_driver(). ...
+ * @pdev: pci device
pci --> PCI ...
+#include <linux/io-64-nonatomic-lo-hi.h>
+#include <linux/msi.h>
Wondering what the APIs you are using from there. ...
+ for (i = 0; i < ATR_TABLE_NUM_PER_ATR; i++) {
+ offset = (ATR_PORT_OFFSET * port) + (ATR_TABLE_OFFSET * i);Too many parentheses.
quoted hunk ↗ jump to hunk
+ /* Disable table by SRC_ADDR */ + reg = pbase + ATR_PCIE_WIN0_T0_ATR_PARAM_SRC_ADDR + offset; + iowrite64(0, reg); + }
...
quoted hunk ↗ jump to hunk
+ pos = ffs(lower_32_bits(cfg->size)); + if (!pos) + pos = ffs(upper_32_bits(cfg->size)) + 32;
NIH __ffs64() ? ...
quoted hunk ↗ jump to hunk
+static void mtk_pcie_mac_enable_disable_int(struct mtk_pci_dev *mtk_dev, bool enable) +{ + u32 value; + + value = ioread32(IREG_BASE(mtk_dev) + ISTAT_HST_CTRL);
Either add...
quoted hunk ↗ jump to hunk
+ if (enable) + value &= ~ISTAT_HST_CTRL_DIS; + else + value |= ISTAT_HST_CTRL_DIS;
+
...or remove blank line for the sake of consistency.
+ iowrite32(value, IREG_BASE(mtk_dev) + ISTAT_HST_CTRL); +}
...
+#include <linux/bitops.h>
Who is the user of this? ...
quoted hunk ↗ jump to hunk
+#ifndef __T7XX_REG_H__ +#define __T7XX_REG_H__ + +#include <linux/bits.h>
...
quoted hunk ↗ jump to hunk
+#define EXP_BAR0 0x0c +#define EXP_BAR2 0x04 +#define EXP_BAR4 0x0c
BAR0 and BAR4 have the same value. Either explain or fix accordingly. ...
+#define MSIX_MSK_SET_ALL GENMASK(31, 24)
Missed blank line ?
quoted hunk ↗ jump to hunk
+enum pcie_int { + DPMAIF_INT = 0, + CLDMA0_INT, + CLDMA1_INT, + CLDMA2_INT, + MHCCIF_INT, + DPMAIF2_INT, + SAP_RGU_INT, + CLDMA3_INT, +};
...
quoted hunk ↗ jump to hunk
+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);
Redundant 'else'. Recommend to read again our internal Wiki about typical issues with the code.
+ return NULL; +}
...
quoted hunk ↗ jump to hunk
+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);
Ditto.
+ return NULL; +}
...
quoted hunk ↗ jump to hunk
+ for (i = 0; i < queue->max_len; i++) { + struct sk_buff *skb; + + skb = alloc_skb_from_kernel(skb_size, GFP_KERNEL);
+
Redundant.
quoted hunk ↗ jump to hunk
+ if (!skb) { + while ((skb = skb_dequeue(&queue->skb_list))) + dev_kfree_skb_any(skb); + return -ENOMEM; + } + + skb_queue_tail(&queue->skb_list, skb); + }
...
quoted hunk ↗ jump to hunk
+/** + * ccci_alloc_skb_from_pool() - allocate memory for skb from pre-allocated pools + * @pools: skb pools + * @size: allocation size + * @blocking : true for blocking operation
Extra white space. Again, revisit _all_ comments in your series and make them consistent in _all_ possible aspects (style, grammar, ...).
quoted hunk ↗ jump to hunk
+ * + * Returns pointer to skb on success, NULL on failure. + */
...
+ if (blocking) {+ might_sleep();
might_sleep_if() at the top of the function?
quoted hunk ↗ jump to hunk
+ skb = alloc_skb_from_kernel(size, GFP_KERNEL); + } else { + for (count = 0; count < ALLOC_SKB_RETRY; count++) { + skb = alloc_skb_from_kernel(size, GFP_ATOMIC); + if (skb) + return skb; + } + }
...
quoted hunk ↗ jump to hunk
+ while (queue->skb_list.qlen < SKB_64K_POOL_SIZE) { + skb = alloc_skb_from_kernel(MTK_SKB_64K, GFP_KERNEL); + if (skb) + skb_queue_tail(&queue->skb_list, skb); + }
May it become an infinite loop? ...
quoted hunk ↗ jump to hunk
+ while (queue->skb_list.qlen < SKB_4K_POOL_SIZE) { + skb = alloc_skb_from_kernel(MTK_SKB_4K, GFP_KERNEL); + if (skb) + skb_queue_tail(&queue->skb_list, skb); + }
Ditto. ...
quoted hunk ↗ jump to hunk
+ while (queue->skb_list.qlen < SKB_16_POOL_SIZE) { + skb = alloc_skb_from_kernel(MTK_SKB_16, GFP_KERNEL); + if (skb) + skb_queue_tail(&queue->skb_list, skb); + }
Ditto. ...
quoted hunk ↗ jump to hunk
+ pools->reload_work_queue = alloc_workqueue("pool_reload_work", + WQ_UNBOUND | WQ_MEM_RECLAIM | WQ_HIGHPRI, + 1);
... wqflags = WQ_UNBOUND | WQ_MEM_RECLAIM | WQ_HIGHPRI;
pools->reload_work_queue = alloc_workqueue("pool_reload_work", wqflags, 1);
quoted hunk ↗ jump to hunk
+ if (!pools->reload_work_queue) { + ret = -ENOMEM; + goto err_wq; + }
...
+ list_for_each_entry_safe(notifier_cur, notifier_next,
+ &ctl->notifier_list, entry) {Out of a sudden this is two lines...
quoted hunk ↗ jump to hunk
+ if (notifier_cur == notifier) + list_del(¬ifier->entry); + }
...
quoted hunk ↗ jump to hunk
+ if (!list_empty(&ctl->event_queue)) { + event = list_first_entry(&ctl->event_queue, + struct ccci_fsm_event, entry);
event = list_first_entry_or_null();
if (event) {
quoted hunk ↗ jump to hunk
+ if (event->event_id == CCCI_EVENT_MD_EX) { + fsm_finish_event(ctl, event); + } else if (event->event_id == CCCI_EVENT_MD_EX_REC_OK) { + rec_ok = true; + fsm_finish_event(ctl, event); + } + }
...
quoted hunk ↗ jump to hunk
+ if (!list_empty(&ctl->event_queue)) { + event = list_first_entry(&ctl->event_queue, + struct ccci_fsm_event, entry); + if (event->event_id == CCCI_EVENT_MD_EX_PASS) + fsm_finish_event(ctl, event); + }
Ditto ...
+ if (!atomic_read(&ctl->md->rgu_irq_asserted)) {It may be set exactly here, what's the point in atomicity of the above check?
quoted hunk ↗ jump to hunk
+ /* disable DRM before FLDR */ + mhccif_h2d_swint_trigger(mtk_dev, H2D_CH_DRM_DISABLE_AP); + msleep(FSM_DRM_DISABLE_DELAY_MS); + /* try FLDR first */ + err = mtk_acpi_fldr_func(mtk_dev); + if (err) + mhccif_h2d_swint_trigger(mtk_dev, H2D_CH_DEVICE_RESET); + }
...
quoted hunk ↗ jump to hunk
+ wait_event_interruptible_timeout(ctl->async_hk_wq, + atomic_read(&md->core_md.ready) || + atomic_read(&ctl->exp_flg), HZ * 60);
Are you sure you understand what you are doing with the atomics?
quoted hunk ↗ jump to hunk
+ if (atomic_read(&ctl->exp_flg)) + dev_err(dev, "MD exception is captured during handshake\n"); + + if (!atomic_read(&md->core_md.ready)) { + dev_err(dev, "MD handshake timeout\n"); + fsm_routine_exception(ctl, NULL, EXCEPTION_HS_TIMEOUT); + } else { + fsm_routine_ready(ctl); + }
...
+ read_poll_timeout(ioread32, dev_status, (dev_status & MISC_STAGE_MASK) == LINUX_STAGE, + 20000, 2000000, false, IREG_BASE(md->mtk_dev) + PCIE_MISC_DEV_STATUS);
Why ignoring an error is fine here? ...
+ cmd = kmalloc(sizeof(*cmd), + (in_irq() || in_softirq() || irqs_disabled()) ? GFP_ATOMIC : GFP_KERNEL);
Hmm...
+ if (!cmd) + return -ENOMEM;
+ if (in_irq() || irqs_disabled()) + flag &= ~FSM_CMD_FLAG_WAITING_TO_COMPLETE;
Even more hmm...
+ if (flag & FSM_CMD_FLAG_WAITING_TO_COMPLETE) {
+ wait_event(cmd->complete_wq, cmd->result != FSM_CMD_RESULT_PENDING);Is it okay in IRQ context?
+ if (cmd->result != FSM_CMD_RESULT_OK) + result = -EINVAL;
quoted hunk ↗ jump to hunk
+ spin_lock_irqsave(&ctl->cmd_complete_lock, flags); + kfree(cmd); + spin_unlock_irqrestore(&ctl->cmd_complete_lock, flags);
While this is under spin lock?
+ }
...
quoted hunk ↗ jump to hunk
+enum md_state ccci_fsm_get_md_state(void) +{ + struct ccci_fsm_ctl *ctl; + + ctl = ccci_fsm_entry; + if (ctl) + return ctl->md_state; + else + return MD_STATE_INVALID; +} + +unsigned int ccci_fsm_get_current_state(void) +{ + struct ccci_fsm_ctl *ctl; + + ctl = ccci_fsm_entry; + if (ctl) + return ctl->curr_state; + else + return CCCI_FSM_STOPPED; +}
Redundant 'else' everywhere. ...
quoted hunk ↗ jump to hunk
+int ccci_fsm_init(struct mtk_modem *md) +{ + struct ccci_fsm_ctl *ctl;
struct device *dev = &md->mtk_dev->pdev->dev; ...
quoted hunk ↗ jump to hunk
+ ctl->fsm_thread = kthread_run(fsm_main_thread, ctl, "ccci_fsm"); + if (IS_ERR(ctl->fsm_thread)) { + dev_err(&md->mtk_dev->pdev->dev, "failed to start monitor thread\n");
quoted hunk ↗ jump to hunk
+ return PTR_ERR(ctl->fsm_thread); + } + + return 0;
return PTR_ERR_OR_ZERO(...);
+}
-- With Best Regards, Andy Shevchenko