RE: [External Mail] [PATCH v2 6/7] net: wwan: t9xx: Add AT & MBIM WWAN ports

[Wu. JackBB (GSM)]

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@xxxxxxxxxx/#27006088

Q1: Is this mutex ever acquired? It is initialized during port setup, but
it does not appear to be used to serialize operations in
mtk_port_common_write() or when sending data.

  Valid. Fixed in v3 by removing the unused write_lock mutex from
  struct mtk_port and its mutex_init call.

Q2: Will concurrent writes safely execute here without holding write_lock?
Could this lack of serialization lead to sequence number corruption?

  The WWAN core framework holds port->ops_lock (a mutex) around the
  tx/tx_blocking callback invocations, serializing write operations
  on the same WWAN port. For internal ports, writes are exclusively
  performed by the FSM kthread, which is single-threaded. Concurrent
  writes to the same port do not occur, and port->tx_seq is always
  modified by a single thread.

Q3: What happens if a blocking write is interrupted by a port teardown?
If PORT_S_WR is cleared, trb->status remains at MTK_DFLT_TRB_STATUS (1).
ret = (!trb->status) ? len : trb->status evaluates to 1, causing an
incorrect byte count to be returned.

  This only occurs during port teardown (PORT_S_WR cleared by
  mtk_port_common_close or disable). At this point the port is being
  shut down and the return value is largely irrelevant — the caller
  cannot meaningfully use the port afterward. The submitted data may
  or may not have been transmitted by DMA, depending on timing. This
  is a teardown-only scenario with no practical impact on data
  integrity.

Q4: Is it safe to mutate the flags directly here? This is a non-atomic
read-modify-write on the shared port structure.

  The WWAN core holds port->ops_lock (a mutex) around tx/tx_blocking
  callbacks, serializing flag modifications. mtk_port_wwan_write()
  and mtk_port_wwan_write_blocking() are never called concurrently
  for the same port.

Q5: Does this silently drop data on partial writes? If
mtk_port_common_write() returns a positive value (partial success),
consume_skb is called unconditionally and 0 (success) is returned.

  This is a design limitation of the WWAN port API: wwan_port_op_tx
  returns 0 for success or negative for error — there is no mechanism
  to report partial writes back to the WWAN core. The already-submitted
  fragments cannot be recalled from the DMA engine. For the AT/MBIM
  control ports in this driver, messages are small (typically under
  1KB, within a single MTU). The multi-fragment path is rarely
  exercised for control plane traffic.

Q6: As with mtk_port_wwan_write(), mutating the shared blocking flag
without atomics could race, and ignoring positive return values could
lead to silent data loss.

  Same as Q4 (flags serialized by WWAN core ops_lock) and Q5
  (partial write is a WWAN API limitation, rare for control messages).

Q7: Is there a race condition here if wwan_create_port() fails? The return
value is directly assigned to w_port without checking IS_ERR() first.
Could concurrent RX pass the error pointer to wwan_port_rx()?

  No race. mtk_port_wwan_enable() is called from the FSM thread
  during the handshake sequence, before the port starts receiving
  data. The CLDMA RX queue for this port has not been opened at this
  point — RX data only arrives after the modem completes its
  handshake. The RX path cannot observe the error pointer because no
  data arrives until after the port is fully enabled.

Q8: Is the WWAN port exposed to userspace before its state is fully
initialized? wwan_create_port() registers the character device and
triggers a uevent. If userspace opens immediately, PORT_S_ENABLE is
not set yet so open returns -ENODEV.

  The window between wwan_create_port() returning and
  set_bit(PORT_S_ENABLE) is a few instructions (nanoseconds). If
  userspace opens in that window, the open returns -ENODEV and the
  application retries. In practice, user space WWAN managers (e.g.,
  ModemManager) wait for udev events to settle before opening ports.
  Reordering to set PORT_S_ENABLE before wwan_create_port is not
  correct either — the port should not be marked enabled before the
  WWAN port object exists.

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.
================================================================================================================================================================