Re: [PATCH v11 3/6] net: wwan: t9xx: Add control DMA interface

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

Thanks for the review.

> Can this branch leave a started TX queue stopped while it still has
> descriptors pending?
>
> Suppose the instance reset comes from another queue's
> mtk_cldma_txq_free() or mtk_cldma_rxq_free() -ETIMEDOUT path, or from
> mtk_cldma_hw_recovery(). Suppose also that this queue's slot at free_idx
> is completed but not yet reaped (HWO=0, req->skb set), and later slots
> still have HWO=1.
>
> In that case this branch sets tx_started to false and only queues
> tx_done_work. It programs no start address and issues no UL START.
>
> mtk_cldma_tx_done_work() then reaps the completed slot and stops at the
> first HWO=1 slot:
>
> 	if (!req->skb || (req->gpd->tx_gpd.gpd_flags & CLDMA_GPD_FLAG_HWO)) {
> 		spin_unlock(&txq->ring_lock);
> 		break;
> 	}
>
> Nothing restarts the engine after that. The remaining descriptors and
> their TRBs seem to stay stuck until another TX is submitted on this
> queue. Later in the series, a blocking writer in mtk_port_send_data()
> waits for these TRBs with no timeout.
>
> When the next TX does arrive, mtk_cldma_start_xfer() reads a start
> address of 0, which this branch left unprogrammed on purpose. It treats
> that as lost device state:
>
> 	if (unlikely(!val)) {
> 		ret = mtk_cldma_hw_recovery(drv_info, txq, qno);
>
> That logs "lost its queue state", runs mtk_cldma_drv_init() again and
> sets need_restart on every RX queue of the instance.
>
> mtk_cldma_tx_start_idx() already finds the first slot the device still
> owns. Could it be used here, under the ring_lock already held, to
> program that slot and keep the queue running?
>
> This branch is unchanged at the end of the series. The "Add FSM thread"
> patch only adds an is_stopping check.

No functional change; v12 adds a paragraph to the commit message that
describes the reset and re-arm behaviour, including this case.

This state needs another queue on the same instance to have failed its
stop, or the device to have lost its queue state.  The stranded
descriptors keep HWO and their mappings, so nothing is lost or unsafe:
the next write to the queue goes through mtk_cldma_tx_start_idx() and
starts from the first descriptor the device still owns, after the extra
re-init you describe.  Starting the engine from the re-arm instead would
add a recovery action straight after an IP reset that we cannot
validate, so we keep the re-arm limited to what it does now.  The
blocking writer waits interruptibly and is woken when the port is
closed.

> If one queue fails to stop while its channel is closing, this pulses the
> infra reset for the whole CLDMA instance. Is that safe while other
> channels on the same instance may have DMA in flight?
>
> The other queues are not stopped before mtk_cldma_drv_reset(). At the
> end of the series CLDMA1 carries three channels (TXQ 0, 2 and 5 in
> mtk_queue_info[]).
>
> A TX GPD that another queue was sending keeps HWO=1, and
> mtk_cldma_rearm_queues() restarts it from free_idx. Could the modem then
> receive a partial copy followed by a full resend? An RX GPD that was
> being filled would be left half-written.
>
> mtk_cldma_rxq_free() reaches the same reset.

No functional change; the same commit message paragraph says that the
reset also cuts the instance's other queues and what happens to them.

An RX descriptor being filled still has HWO set, so the host never reads
it; the restart hands it back to the device, which refills it from the
start.  A TX descriptor cut by the reset stays mapped and device-owned
and is sent again in full, which is the only option that does not drop
it.  Whether the modem can already have passed on part of a transfer cut
by the reset is part of what I am confirming with the modem team (see
the next point). The reset is the escalation for a queue that already
failed to stop within the poll, added in v8 so that a ring the device
may still walk is not freed.  Stopping every other queue first would not
make it cleaner: a stuck queue makes that stop time out as well.

> Also, mtk_cldma_drv_reset() returns right after releasing the reset:
>
> 	mtk_pci_write32(mdev, REG_DEV_INFRA_BASE + hw_regs->reg_infra_rst0_clr, val);
> 	mtk_pci_read32(mdev, REG_DEV_INFRA_BASE + hw_regs->reg_infra_rst0_clr);
>
> The next accesses are the mtk_cldma_stop_queue() poll and the
> read-modify-write of UL_CFG and SO_CFG in mtk_cldma_drv_init(), which
> selects 64-bit GPD mode.
>
> Does the IP need a settle delay or a readiness check after RST0_CLR
> before those accesses?

The read-back after RST0_CLR makes sure the release has reached the
device before we continue, but how soon the block may be accessed after
release is not something the code can show.  I am confirming this with
the modem team, and will add the delay or ready poll they require.  This
path is only taken after a queue has already failed to stop.

> mtk_cldma_start_xfer() calls this when one TX queue's start address
> reads back 0. It then calls mtk_cldma_rearm_queues() without resetting
> or stopping the instance first. Is the re-arm safe on an IP that may
> still be running?
>
> mtk_cldma_rearm_queues() looks written for the state right after
> mtk_cldma_drv_reset(). On this path it read-modify-writes UL_CFG and
> SO_CFG and rewrites the interrupt and busy masks on a live IP.
>
> For every other published TX queue with tx_started set and HWO=1 at
> free_idx, it also does:
>
> 	mtk_cldma_setup_start_addr(drv_info, DIR_TX, i,
> 				   req->gpd_dma_addr);
> 	...
> 	if (READ_ONCE(txq->tx_started))
> 		mtk_cldma_start_queue(drv_info, DIR_TX, i);
>
> Nothing shows those engines are idle. Could a running queue be
> reprogrammed and restarted mid-transfer, so a descriptor is processed
> twice or a transfer is cut short?
>
> The RX side has the same pattern in this patch. need_restart makes
> mtk_cldma_rxq_restart() program SO_START_ADDR and issue START without
> stopping the queue.
>
> The later "Add FSM thread" patch adds mtk_cldma_stop_queue() to
> mtk_cldma_rxq_restart(), but the TX side is unchanged at the end of the
> series.

No functional change; v12 corrects the comment above
mtk_cldma_hw_recovery(), which only named the lost-state case.

hw_recovery() is meant for an IP that has lost its queue state; then no
engine is running and drv_init() rewrites the same configuration.  The
live-IP case only comes from the zero start address the re-arm leaves on
a queue it stopped after a host-side reset, and costs one extra re-init.
The RX half is stopped first by the later patch, as you note.  We keep
the TX half as it is because changing what this recovery does to the
hardware cannot be validated without the failure it handles.

> This may not be a bug, but could the reason for disabling the illegal
> memory check be documented? Neither the code nor the commit message
> explains it.
>
> The write runs on every instance init and on every
> mtk_cldma_rearm_queues() recovery.
>
> Some paths can leave a stale buffer address in a GPD. One is the re-arm
> after a reset. Another is an RX slot in mtk_cldma_rx_done_work() whose
> skb was freed after a mapping failure while data_buff_ptr still holds
> the old address.
>
> With the check turned off, the device would not catch such an address.
> Without an IOMMU, it could turn into a stray DMA to host memory.
>
> The t7xx CLDMA driver does not appear to do this. Does the hardware
> require this write?

I am confirming with the hardware team what the check covers and why it
has to be off, and will state the reason in the comment, or drop the
writes if it is not needed.

On the stale addresses: neither case can reach the device.  An RX slot
whose buffer is gone keeps HWO clear and is never armed, and the device
stops at the unarmed slot before it; the retry reloads and maps a new
buffer before arming.  A TX slot is unmapped only after the device has
cleared HWO, and gets HWO again only after its new pointer is written
behind dma_wmb().

> Can this restart the RX engine on a descriptor the device has already
> completed?
>
> mtk_cldma_rx_done_work() leaves the descriptor loop when the slot at
> rxq->free_idx still has CLDMA_GPD_FLAG_HWO set. In that case
> mtk_cldma_check_rx_req() returns -EAGAIN because SO_CUR_ADDR points at
> that slot.
>
> Between that check and mutex_lock(&drv_info->rearm_lock), the device
> can finish the packet and clear HWO. Another context can then take
> rearm_lock to reset the instance. Possible callers are the -ETIMEDOUT
> paths in mtk_cldma_txq_free() and mtk_cldma_rxq_free() for another
> queue on the same instance, or mtk_cldma_hw_recovery(). The worker may
> also just be blocked on rearm_lock while the reset runs. Either way,
> mtk_cldma_rearm_queues() sets need_restart and queues rx_done_work
> again.
>
> When the worker gets the lock, it consumes need_restart. Then
> mtk_cldma_rxq_restart() programs SO_START_ADDR with
> req_pool[free_idx].gpd_dma_addr and issues SO START. Nothing checks
> HWO on that slot again, so the engine stops at once on an HWO=0
> descriptor.
>
> The re-queued pass then reaps that slot. Its arm_slot step sets HWO on
> the previous slot and leaves the reaped one as the new HWO=0 tail. That
> is where the device cursor is parked.
>
> The next slot never completes, so mtk_cldma_check_rx_req() times out
> with "Failed to check HWO=0" and returns -EAGAIN. need_restart is
> already 0, so only mtk_cldma_resume_queue() is issued. As the comment
> in mtk_cldma_rearm_queues() notes, RESUME never moves the cursor past
> an HWO=0 descriptor. No further XFER_DONE comes to run the worker.
>
> That seems to leave the RX queue wedged until the channel is closed.
> On the MD and sAP control channels, that would stop all handshake
> messages from the device.
>
> Should the worker re-check the slot at free_idx after taking
> rearm_lock? If the device has completed it, the worker could set
> need_restart again and loop to reap it before restarting. After a
> reset, the RX engine should stay idle until mtk_cldma_rxq_restart()
> issues START, so that check would be stable under the lock.

Will fix in v12, as you suggest: mtk_cldma_rxq_restart() checks HWO on
the descriptor at free_idx before programming it, and if the device has
completed it, the worker keeps need_restart and loops to reap it first.
The reset (and, later in the series, the stop in rxq_restart()) keeps
the engine idle until START, so the check is stable under rearm_lock.

--
Jack