Re: [PATCH v11 4/6] net: wwan: t9xx: Add control port

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

Thanks for the review.

> Should this also check that the channel belongs to the queue the frame
> arrived on?
>
> The destination port is taken from the channel field of the CCCI header,
> and the device writes that field. The only check is that its peer ID
> matches the peer of the port that armed this RX queue.
>
> Later in the series the MD peer has three ports, each on its own queue:
> MDCTRL on CLDMA1 RXQ0, MBIM on CLDMA1 RXQ2 and AT on CLDMA1 RXQ5. If a
> frame arrives on the MBIM or AT queue carrying channel CCCI_CONTROL_RX,
> it goes to the MDCTRL internal port's ->recv().
>
> Each queue has its own CLDMA RX work item on a WQ_UNBOUND workqueue. Two
> queues' workers can therefore run mtk_port_check_rx_seq(), write
> port->rx_seq and call ->recv() for the same port at the same time. That
> breaks the rule in struct mtk_port that rx_seq is written "only from the
> rx_done callback of the one queue serving this channel". That rule is
> the reason rx_seq has no lock.
>
> Could the frame be dropped unless MTK_CH_ID(channel) matches the arming
> port's channel, or unless the channel's queue matches the receiving
> queue? t7xx does this by matching rxq_index.

No change.  t7xx routes CCCI frames the same way:
t7xx_port_proxy_find_port() matches only the path (hif_id) and the
channel, and its rxq_index check covers only the dedicated header-less
queue.  A check against the port that armed the queue would also drop
frames on queues shared by several channels, which later users of this
driver have.  A frame on the wrong queue of the same CLDMA is a firmware
bug.  Even then, both ->recv() paths tolerate concurrent calls, and
rx_seq only feeds the rate-limited warning.

> Is the channel always closed when these counters are reset? The comment
> in struct mtk_port in mtk_port.h depends on it:
>
> 	/* tx_seq is stamped and advanced only at hardware hand-off, by the one
> 	 * thread serving this port's TX queue; rx_seq only from the rx_done
> 	 * callback of the one queue serving this channel.  Both are reset
> 	 * while the channel is disabled, so neither needs a lock.
> 	 */
>
> These are plain stores, made before the ENABLE is submitted. Nothing
> checks that the CLDMA queue is closed. I can see two ways the queue can
> still be live at this point.
>
> The first is a retry after a timeout. On -ETIMEDOUT,
> mtk_port_internal_enable() does this:
>
> 	ret = mtk_port_ch_enable(port);
> 	if (ret && ret != -EBUSY) {
> 		...
> 		mtk_port_ch_disable(port);
> 		clear_bit(PORT_S_ENABLE, &port->status);
> 		return;
> 	}
>
> The ENABLE trb is still queued, with the DISABLE behind it. With "Add
> FSM thread" later in the series, the handshake then fails in
> mtk_fsm_ctrl_ch_start()->mtk_port_internal_open() with -ENODEV. The
> device sends HS1 again, and mtk_port_mngr_fsm_state_handler_late() calls
> ->enable() a second time.
>
> Suppose the trb thread has already run the first ENABLE. The RX queue is
> then armed with rxq->arg = port, and the DISABLE has not closed it yet.
> mtk_port_rx_dispatch() can then write port->rx_seq while this function
> resets it.
>
> The second is -EBUSY, which mtk_port_ch_enable() treats as success.
> Suppose the queue was left open, for example because an earlier DISABLE
> submit failed with -ENOMEM. Both counters are then reset while the queue
> is live. The trb thread may still be running mtk_port_tx_seq_stamp() and
> mtk_port_tx_seq_advance() on queued TX packets.
>
> rx_seq only feeds the rate-limited warning in mtk_port_check_rx_seq(),
> so the visible effect is a spurious or missed out-of-order warning.
> KCSAN would still report the race.
>
> Could the guarantee in the comment be enforced, or could the comment be
> reworded to match what the code actually ensures?

No change.  The reset only runs when PORT_S_ENABLE was clear, and there
are two ways to get there.  One is after OFF, where mtk_pcie_hif_exit()
has closed every queue and the next hif init starts the queues from
scratch, so the -EBUSY case cannot carry an open queue across it.  The
other is an enable that failed before the port could be opened: then no
TX TRB of this port exists, and the modem sends nothing on the control
channel, because it only answers the HS1 that was never sent.  A
repeated HS1 during a live handshake finds PORT_S_ENABLE set and does
not reach the reset.  The comment is accurate as written.

--
Jack