Re: [PATCH v11 6/6] net: wwan: t9xx: Add AT & MBIM WWAN ports

[Wu. JackBB (GSM)]

Thanks for the review.

> Can a stale port->enable reach READY here? mtk_port_status_update() only
> writes port->enable for channels that appear in the current enumeration:
>
>     port->enable = FIELD_GET(MTK_INFO_FLD_EN, le16_to_cpu(port_info->channel));
>
> Nothing resets the ports that are not listed.
>
> In mtk_fsm_startup_act(), mtk_fsm_parse_hs2_msg() applies the MD port
> enumeration first. If a later RTFT entry is malformed, or
> mtk_fsm_send_hs3_msg() fails, the code takes hs_err and re-arms the
> channel so the modem can retry the handshake:
>
>     ret = mtk_fsm_parse_hs2_msg(fsm, hs_info, skb);
>     if (!ret)
>         ret = mtk_fsm_send_hs3_msg(hs_info, skb);
>     ...
> hs_err:
>     if (hs_info)
>         mtk_pci_unmask_ext_evt(mdev, hs_info->mhccif_ch);
>
> Suppose the retried enumeration leaves out a port that the first one
> marked enabled. That port keeps port->enable == true. On READY,
> mtk_port_wwan_enable() would then arm its CLDMA1 queue and call
> wwan_create_port() for a channel the final enumeration did not advertise.
> That goes against the comment above.
>
> This depends on the modem firmware sending a different port list across
> handshake retries, so it may not happen in practice. FSM_STATE_OFF is
> terminal in this driver, so the handshake retry seems to be the only way
> to get here, not a later boot.
>
> Would clearing all port->enable flags before applying a new enumeration
> close this gap?

No change for the retry case.  The MD and SAP enumerations both go
through mtk_port_status_update() and arrive as independent HS2s, so
clearing every port->enable before applying one would wipe the flags the
other side had already set, and READY would enable no MD port.  A
per-table reset would need the handshake id passed through the RTFT
action signature, for a modem that would have to change its own port
list between two handshakes of the same boot.

Will fix in v12 the store you point at: mtk_port_enable_by_type() no
longer clears port->enable, and its comment goes with it.  FSM_STATE_OFF
is terminal and READY is reached at most once per bind, so nothing can
observe the cleared flag.

> Is the CCCI header subtracted twice here? mtk_port_ch_enable() already
> removes it from port->tx_mtu:
>
>     port->tx_mtu = trb_open_priv->tx_mtu - MTK_CCCI_H_ELEN;
>
> mtk_port.h documents MTK_CCCI_H_ELEN as:
>
> /* ccci header length + reserved space that is used in exception flow */
> #define MTK_CCCI_H_ELEN		(128)
>
> So port->tx_mtu already looks like the payload budget. This code and
> caps.frag_len in mtk_port_wwan_enable() both subtract
> sizeof(struct mtk_ccci_header) a second time. Each packet then carries
> 16 bytes less payload: with Q_MTU_3_5K, frag_len is 3440 instead of 3456.
>
> The error is in the safe direction, so nothing overflows. Still, this
> math, the "payload budget" comment in mtk_port_wwan_enable(), and the
> MTK_CCCI_H_ELEN definition can't all be right. Which meaning of
> port->tx_mtu is intended?

Will fix in v12: port->tx_mtu is the payload budget, as MTK_CCCI_H_ELEN
says.  The write path and caps.frag_len now use it directly and allocate
tx_mtu plus the header, so a packet carries 3456 bytes of payload.  One
core fragment still maps to exactly one CCCI packet.

> This isn't a functional bug, but does this patch make the comment on enum
> mtk_port_status in mtk_port.h inaccurate?
>
> /* State bits, not locks: callers serialise ->enable(), ->disable() and
>  * open of a port.  The atomic bitops only make a repeated call a no-op.
>  */
>
> For the WWAN ports, open comes from userspace through the WWAN core:
> mtk_port_wwan_open() runs under the core's ops_lock. ->enable() and
> ->disable() come from the FSM through mtk_port_mngr_fsm_state_handler().
> Nothing serialises the two paths, and the comment above set_bit()
> here expects open to run at the same time as enable.
>
> The open vs. enable and open vs. disable interleavings look safe because
> of the w_lock ordering, but the header comment no longer describes how
> this works. Could it be updated so later code doesn't count on caller
> serialisation that doesn't exist?

Will fix in v12: the comment will say that ->enable() and ->disable()
are serialised by their callers, while a WWAN open comes from userspace,
can only reach a node created after ->enable(), and is ordered against
->disable() by w_priv.w_lock.

--
Jack