RE: [External Mail] [PATCH v2 5/7] net: wwan: t9xx: Add FSM thread

["Wu. JackBB (GSM)" <JackBB_Wu@compal.com>]

Hi Jakub,

Addressing sashiko AI code review comments for this patch, as
requested by you in the patch 3/7 review:
https://patchwork.kernel.org/project/netdevbpf/patch/20260610-t9xx_driver_v1-v2-3-c65addf23b3f@compal.com/#27006088

Q1: Will this compile on standard compilers? The container_of macro relies
on offsetof, which requires an integer constant expression. Evaluating
hs_info->id dynamically at runtime will cause a compilation error.

  This compiles correctly. The Linux kernel's container_of uses
  __builtin_offsetof (GCC extension), which supports variable array
  indices. GCC and Clang both generate runtime offset calculation
  code for container_of(ptr, type, member[variable_index]). This
  pattern is used in multiple places in the kernel.

Q2: Could this result in a NULL pointer dereference? If the device sends
multiple CTRL_MSG_HS2 messages, the second message overwrites the skb
pointer. When the first event is processed, it frees the skb and sets
hs_info->rt_data to NULL, causing a deref on the second event.

  This cannot happen in practice. The handshake follows a strict
  HS1 -> HS2 -> HS3 sequence. The MHCCIF channel interrupt is masked
  in mtk_fsm_hs1_handler() before the FSM event is submitted,
  preventing duplicate HS2 notifications. Only one HS2 message is
  expected per handshake cycle.

Q3: Can this read past the end of the packet? We pass rtft_entry->data to
the action callback before checking if rtft_entry->data_len fits within
the remaining packet length.

  The bounds check at the loop start ensures the rtft_entry struct
  header is within bounds. The action functions only read fixed-size
  fields (e.g., a single __le32 for packet padding mode). The modem
  firmware is a trusted source — the feature query protocol uses
  head_pattern/tail_pattern integrity checks, and the message format
  is guaranteed by the modem firmware.

Q4: Could this result in unaligned memory accesses on the next loop
iteration? Since data_len is not verified to be a multiple of 4, the
next rtft_entry could be misaligned.

  On x86 (the target platform for this PCIe WWAN modem), unaligned
  memory access is handled natively without traps. The feature query
  entries from the modem firmware are always naturally aligned in
  practice (data_len is always a multiple of 4 for the defined
  feature types).

Q5: Does this code ensure the packet is long enough before parsing the
feature query? It casts and reads ft_query fields without verifying if
hs_info->rt_data_len is large enough to hold a feature_query structure.

  The SKB is received from the CLDMA RX path with validated length.
  After skb_pull, the remaining data is the feature query payload.
  The head/tail pattern check immediately following this cast
  validates the message integrity. A truncated message would fail the
  pattern check and take the error path.

Q6: Is there a risk of accessing uninitialized memory here? If the skb is
fragmented, checking skb->len is not sufficient. We should use
pskb_may_pull() before casting and accessing skb->data.

  The RX SKBs are allocated internally by the CLDMA driver using
  __dev_alloc_skb with linear buffers. The CCCI header and control
  message data always reside in the contiguous linear data region.
  These SKBs are not received from the network stack and are always
  linear, so pskb_may_pull is not needed.

Q7: Does this introduce a leak if the device sends multiple messages
rapidly? Overwriting hs_info->rt_data discards the previous skb pointer
without freeing it.

  Same as Q2. The handshake is a strict HS1 -> HS2 -> HS3 sequence
  with the MHCCIF channel masked after HS1. Only one HS2 message
  arrives per handshake cycle. No overwrite can occur.

Q8: Does this permanently hang the device initialization? The interrupt is
masked and cleared at the beginning. If the device stage is not
DEV_STAGE_IDLE, we return without unmasking.

  The early_bootup_handler is registered for the BOOT_FLOW_SYNC
  MHCCIF event. The MTK modem firmware sends this event once, only
  when the modem reaches IDLE stage. The non-IDLE code path handles
  unexpected states defensively — masking prevents spurious interrupts
  from an error state. The normal boot flow always reaches IDLE
  before sending BOOT_FLOW_SYNC.

Q9: Can concurrent modifications corrupt the list traversal? External
modules can call mtk_fsm_notifier_register() to modify these lists
concurrently with FSM thread iteration.

  The notifier registration and unregistration are only called during
  driver probe and remove paths, which are serialized by the driver
  model. The FSM kthread does not start processing events until probe
  completes, so there is no concurrent access to the notifier lists
  between registration and traversal.

Q10: These lists are modified without any locking mechanism, meaning
concurrent registrations or iterations could crash the kernel.

  Same as Q9. mtk_fsm_notifier_register/unregister are only called
  from probe/remove paths, serialized by the driver model.

Q11: Is it safe to guess the caller's context like this? The checks do not
catch contexts where preemption is disabled by a standard spin_lock().

  The heuristic is sufficient for all current callers.
  mtk_fsm_evt_submit is called from FSM kthread, MHCCIF workqueue
  callback, probe/remove paths, and CLDMA workqueue callbacks — all
  process context, none holding spinlocks. The in_hardirq/in_softirq/
  irqs_disabled checks cover the actual calling contexts.

Q12: Could this lead to a NULL pointer dereference? wake_up_process() is
performed outside the lock. If mtk_fsm_exit() stops the thread and sets
fsm->fsm_handler to NULL concurrently.

  This race cannot occur. mtk_fsm_exit() is called during the device
  remove sequence, which first submits a FSM_EVT_DEV_RM event. The
  FSM kthread processes this event via mtk_fsm_dev_rm_act(), which
  sets EVT_TF_GATECLOSED under evtq_lock. Only after the kthread
  finishes does mtk_fsm_exit() proceed to call kthread_stop() and
  set fsm_handler to NULL. The EVT_TF_GATECLOSED check inside the
  spinlock provides ordering guarantee.

Q13: Can this loop endlessly if a signal is continuously delivered? Since
this resets the timer to the full MTK_DFLT_TRB_TIMEOUT every time, a
stream of signals might trap the task in an infinite loop.

  This is intentional. Channel enable/disable are control plane
  operations that must complete or timeout. Each iteration sleeps in
  wait_event_interruptible_timeout — not a busy loop. The timeout
  guarantees bounded per-iteration execution. The total wait time may
  exceed a single timeout with continuous signals, but the operation
  eventually completes or times out.

Q14: Does this code silently ignore interrupt registration failures? The
return value of mtk_pci_register_irq() is not checked, and the cleanup
operations below the return statement are unreachable dead code.

  Valid. Fixed in v3 by checking the return value and adding an
  err_destroy_wq error label. Also propagated specific error codes
  from all error paths.

Q15: Does this leak the head SKB when operating in scatter-gather mode? If
rxq->nr_bds > 0, the frag_list is detached but the head SKB itself is
never freed because dev_kfree_skb_any(req->skb) is inside the else block.

  Valid. Fixed in v3 by adding dev_kfree_skb_any(req->skb) after
  detaching frag_list in SG mode.

Thanks.


================================================================================================================================================================
This message may contain information which is private, privileged or confidential of Compal Electronics, Inc. If you are not the intended recipient of this message, please notify the sender and destroy/delete the message. Any review, retransmission, dissemination or other use of, or taking of any action in reliance upon this information, by persons or entities other than the intended recipient is prohibited.
================================================================================================================================================================