Re: [PATCH v11 5/6] net: wwan: t9xx: Add FSM thread

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

Thanks for the review.

> Should this case also check the host's configured type?
>
> Every host query entry is version 0. The entries the host never configured
> are zero, because the fsm comes from devm_kzalloc(). So the version test is
> always true. Any feature the device marks BACKWARD_COMPAT gets promoted to
> RTFT_TYPE_MUST_SUPPORT, even when the host set it to NOT_EXIST or
> NOT_SUPPORT. The MUST_SUPPORT case returns -EPROTO for that host config,
> and the OPTIONAL case downgrades it.
>
> In mtk_fsm_hs_info_init_by_hsid(), the MD handshake leaves
> query_ft_set[QUERY_RTFT_ID_SAP_PORT_ENUM] as NOT_EXIST. The SAP handshake
> leaves query_ft_set[QUERY_RTFT_ID_MD_PORT_ENUM] as NOT_EXIST. Both slots
> have a handler in query_rtft_action[]. So mtk_fsm_parse_hs2_msg() would
> run mtk_port_status_update() on port data the host never asked for:
>
>     if (cur_ft_spt == RTFT_TYPE_MUST_SUPPORT && query_rtft_action[ft_id]) {
>         if (!data_len) {
>             ...
>             ret = -EPROTO;
>             break;
>         }
>
> If that entry has data_len == 0, the whole HS2 fails with -EPROTO. No HS3
> is sent and the FSM never reaches READY.
>
> The comment says the host's type is ignored because it "can be the older
> of the two". That is about versions. Does it really cover a host that
> never queried the feature, or one that explicitly refused it?

No functional change; v12 rewrites the comment to state what
BACKWARD_COMPAT means rather than talking about versions.

This was discussed in the v10 thread: BACKWARD_COMPAT is the device's
promise that a host which does not know the feature, or knows an older
version of it, can still use it.  The host's NOT_EXIST slot means
exactly "this host does not know the feature", so the device's
declaration decides by design.  The action it enables validates its own
input.  A device that declares a feature it insists on and then sends no
data for it is breaking the protocol, and failing that handshake with
-EPROTO is the intended result; the same happens for PORT_ENUM that the
host did ask for.

> This skips the feature_query header without checking head_pattern or
> tail_pattern. It then runs query_rtft_action[] on the entries, and
> mtk_port_status_update() writes port->enable. The FEATURE_QUERY_PATTERN
> check on the same skb only runs later:
>
> mtk_fsm_startup_act()
>   mtk_fsm_parse_hs2_msg()          <- actions applied
>   mtk_fsm_send_hs3_msg()
>     mtk_fsm_append_rtft_entries()  <- -EPROTO on bad patterns
>
> So an HS2 with a bad header but valid entries updates the port table
> first, and only then is rejected. mtk_fsm_startup_act() goes to hs_err
> and re-arms the channel. The port enable state from the rejected message
> stays in place, and any port the retried HS2 does not list keeps that
> value.
>
> Could the pattern check move into mtk_fsm_parse_hs2_msg(), before any
> action runs?

No change.  A rejected HS2 never gets an HS3 and the FSM does not reach
READY, and port->enable is only consumed at READY. A port left enabled
by a rejected HS2 is also not specific to the pattern check: a later
entry, or the HS3 write, can fail after the action ran in the same way.
That residue is answered under patch 6, where port->enable is consumed.

> Once READY is refused here, how does the FSM recover?
>
> This returns directly and does not go through hs_err, so no handshake
> channel is re-armed. Both channels were masked by their HS1 handlers. No
> further STARTUP can arrive, and the FSM stays in BOOTUP until the driver
> is rebound.
>
> One way to get here is a DEV_CFG_MD_ONLY device. mtk_fsm_idle_evt_handler()
> sets hs_done_flag to the MD bits only, but it still unmasks both channels:
>
>     for (hs_id = 0; hs_id < HS_ID_MAX; hs_id++)
>         mtk_fsm_unmask_ext_evt(fsm, fsm->hs_info[hs_id].mhccif_ch);
>
> If a SAP HS1 arrives and mtk_cldma_dev_init(CLDMA0) fails,
> hif_err[MTK_FSM_HIF_USER_CLDMA0] stays set. When the MD handshake
> completes, READY is refused for good, even though SAP is not required in
> that configuration. Can firmware send a SAP HS1 in MD-only mode?

No functional change; the commit message will describe how a refused
READY recovers.

A CLDMA init failure leaves that side's control port unable to open, so
that side's own HS1 fails through hs_err, which re-arms its channel; the
next HS1 re-runs the init and overwrites the slot.  In MD-only mode both
channels stay armed on purpose: masking SAP on the host side would leave
firmware that does send a SAP HS1 in that mode without a reply.

> Separately, the comment above mtk_fsm_hif_err_record() says:
>
>     so a retried init that succeeds clears the failure it recorded before.
>
> The MTK_FSM_HIF_USER_CTRL slot is only written by
> mtk_ctrl_trans_fsm_state_handler(), on the transition to FSM_STATE_ON.
> mtk_fsm_dev_add_act() allows that transition only once, from
> FSM_STATE_INVALID. Is there any retry path for that slot?

No functional change; v12 rewords that sentence to say that a retried
CLDMA init clears its slot, while a CTRL failure is final for the bind.
There is no retry for the CTRL slot by design: if the transport plane
could not be created, no handshake can complete anyway, and refusing
READY is the intended outcome.

> The commit message says "the FSM advances to READY only after both
> complete". It does not mention the MD-only case or the hif_err gate.

Will fix the commit message in v12: an MD-only device needs only the MD
handshake for READY, and READY is refused when a transport init that the
handshake triggered has failed.

> This isn't a bug, but this thread is not parked. It was only created by
> kthread_create() and never woken, and nothing in mtk_fsm.c calls
> kthread_park().
>
> The comment in mtk_pci_dev_exit() has the same problem:
>
>     /* Close the event gate and park the FSM thread before the transport
>      * plane it drives is torn down.
>      */
>     mtk_fsm_stop(mdev);
>
> mtk_fsm_stop() clears fsm->fsm_handler and calls kthread_stop() and
> put_task_struct(). The thread is terminated, not parked.

Will fix in v12: both comments will say the thread is stopped.

> The is_stopping test only runs after the start address has been read.
> If that address was zero, mtk_cldma_hw_recovery() has also already
> re-armed the whole instance by this point. Shouldn't a queue that
> err_work owns be turned away before any of that?
>
> Take a queue whose stop timed out in mtk_cldma_err_tx_one(). It keeps
> is_stopping set and tx_started true. Later, a stop timeout in another
> queue's mtk_cldma_txq_free() or mtk_cldma_rxq_free() resets the
> instance. mtk_cldma_rearm_queues() then skips this queue on purpose and
> leaves its start address at 0. The next mtk_cldma_tx() on it goes:
>
> mtk_cldma_tx()
>   mtk_cldma_start_xfer()
>     mtk_cldma_get_tx_start_addr()  <- reads 0
>     mtk_cldma_hw_recovery()
>       mtk_cldma_rearm_queues()     <- instance re-armed again
>     returns -EIO                   <- tx_started is still true
>   mtk_cldma_stop_queue()
>   mtk_cldma_txq_flush(..., -EIO)
>
> A U64_MAX read on a dead link takes the same early -EIO exit. The same
> thing can also happen while mtk_cldma_err_tx_one() is still polling its
> stop, if the device lost its queue state along with the error.
>
> The error is -EIO, not -EPIPE, so the new -EPIPE check in
> mtk_cldma_tx() does not catch it. mtk_cldma_tx() then stops the queue,
> clears tx_started and flushes the ring itself. Meanwhile
> mtk_cldma_err_tx_one() is either doing the same thing or has decided
> to keep those requests. The flushed TRBs complete with -EIO, not the
> -EPIPE that recovery promises. Nothing clears is_stopping afterwards,
> so the queue keeps refusing new submissions.

Will fix in v12: mtk_cldma_start_xfer() tests is_stopping under
ring_lock before reading the start address, so a queue that err_work
owns returns -EPIPE without triggering recovery.  The check before the
kick stays, since that is the one ordered against err_work.

> There is also a gap in the other direction. If the stop inside
> mtk_cldma_tx() times out, it keeps the requests just as
> mtk_cldma_err_tx_one() does, but it never sets is_stopping. So
> mtk_cldma_submit_tx() keeps publishing HWO descriptors into a ring the
> device may still be walking. That is exactly what is_stopping was
> added to prevent.
>
> Would it work to test is_stopping under ring_lock before the start
> address read and the recovery? The stop-timeout path in mtk_cldma_tx()
> could then set is_stopping the same way err_work does.

No functional change; v12 rewords the comment on that path to say the
queue is left running with its requests.

is_stopping guards the window between a stop and the flush that unmaps
the ring.  When the stop in mtk_cldma_tx() times out it neither flushes
nor unmaps, and tx_started stays set, so the queue simply stays in
service; descriptors published afterwards are ordinary producer activity
on mapped buffers.  Setting is_stopping there would turn a possibly
transient start failure into a channel that refuses everything until it
is closed, since nothing would clear the flag.

> What clears is_stopping after this return? The only place that clears it
> is the end of a later mtk_cldma_err_tx_one() pass whose stop succeeds, and
> that pass needs a new QUEUE_ERROR.
>
> Another queue's stop can later time out in mtk_cldma_txq_free() or
> mtk_cldma_rxq_free(). The instance is then reset with
> mtk_cldma_drv_reset(), so the engine is idle. But mtk_cldma_rearm_queues()
> skips this queue:
>
>     if (txq->is_stopping) {
>         /* Being stopped: restarting it would let the
>          * device walk buffers the stopper is unmapping.
>          */
>     } else if (...
>
> It programs no start address and issues no START. It does not flush the
> kept requests, and it does not clear the flag. QUEUE_ERROR is unmasked,
> but a reset queue that is not started will not raise one, so err_work
> never runs for it again.
>
> After that, mtk_cldma_submit_tx() keeps returning -EPIPE. The kept TRBs
> stay incomplete until the channel is closed via mtk_cldma_txq_free(),
> even though the reset made it safe to flush them. Is that intended? The
> comment's "until an instance re-arm" suggests the re-arm should recover
> this queue.

Will fix the comment in v12 so that "until an instance re-arm" applies
only to the QUEUE_ERROR mask, which the re-arm does lift; the behaviour
stays.  The re-arm cannot tell a stop that failed from an err_work pass
that is still stopping and flushing the same queue, and clearing the
flag there would let start_xfer() kick the queue while that pass unmaps
it.  The kept requests are completed or leaked by txq_free() at close,
as the commit message states.

--
Jack