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

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: wwan: t9xx: Add control port

This adds the t9xx control port layer: a port manager that creates and
destroys ports, keeps them in a global "stale list" across driver
re-creation, and moves data between port I/O and CLDMA via TRBs. It also
adds the internal port operations (open, close, write, recv_register) and
wires the port configuration into the control plane init path.

A few questions about the new code below.

> diff --git a/drivers/net/wwan/t9xx/mtk_port.c b/drivers/net/wwan/t9xx/mtk_port.c
> new file mode 100644
> index 0000000000000..d2a5db11afd6b
> --- /dev/null
> +++ b/drivers/net/wwan/t9xx/mtk_port.c
> @@ -0,0 +1,873 @@

[ ... ]

> +static int mtk_port_tbl_add(struct mtk_port_mngr *port_mngr, struct mtk_port *port)
> +{
> +	int ret;
> +
> +	ret = radix_tree_insert(&port_mngr->port_tbl[MTK_PORT_TBL_TYPE(port->info.rx_ch)],
> +				port->info.rx_ch & 0xFFF, port);
> +	if (ret)
> +		dev_err(port_mngr->ctrl_blk->mdev->dev,
> +			"port(%s) add to port_tbl failed, return %d\n",
> +			port->info.name, ret);
> +	else
> +		port_mngr->port_cnt++;
> +
> +	return ret;
> +}
> +
> +static void mtk_port_tbl_del(struct mtk_port_mngr *port_mngr, struct mtk_port *port)
> +{
> +	radix_tree_delete(&port_mngr->port_tbl[MTK_PORT_TBL_TYPE(port->info.rx_ch)],
> +			  port->info.rx_ch & 0xFFF);
> +	port_mngr->port_cnt--;
> +}
> +
> +static struct mtk_port *mtk_port_restore_from_stale_list(struct mtk_port_mngr *port_mngr,
> +							 struct mtk_stale_list *s_list)
> +{
> +	struct mtk_port *port, *next_port;
> +	int ret;
> +
> +	mutex_lock(&port_mngr_grp_mtx);
> +	list_for_each_entry_safe(port, next_port, &s_list->ports, stale_entry) {
> +		kref_get(&port->kref);
> +		list_del(&port->stale_entry);
> +		ret = mtk_port_tbl_add(port_mngr, port);
> +		if (ret) {
> +			list_add_tail(&port->stale_entry, &s_list->ports);
> +			kref_put(&port->kref, mtk_port_release);
> +			mutex_unlock(&port_mngr_grp_mtx);
> +			dev_err(port_mngr->ctrl_blk->mdev->dev,
> +				"Failed when adding (%s) to port mngr\n",
> +				port->info.name);
> +			return ERR_PTR(ret);
> +		}

[ ... ]

> +static int mtk_port_tbl_create(struct mtk_port_mngr *port_mngr, struct mtk_port_cfg *cfg,
> +			       const int port_cnt, struct mtk_stale_list *s_list)
> +{
> +	struct mtk_port_cfg *dflt_port;
> +	struct mtk_port *port;
> +	int i;
> +
> +	INIT_RADIX_TREE(&port_mngr->port_tbl[PORT_TBL_SAP], GFP_KERNEL);
> +	INIT_RADIX_TREE(&port_mngr->port_tbl[PORT_TBL_MD], GFP_KERNEL);
> +
> +	mtk_port_restore_from_stale_list(port_mngr, s_list);

[Severity: Medium]

mtk_port_restore_from_stale_list() reports failure through ERR_PTR(), but
mtk_port_tbl_create() discards the result. Should this be checked with
IS_ERR(), keeping in mind the success return is NULL rather than a port?

When mtk_port_tbl_add() -> radix_tree_insert() fails (-ENOMEM for a tree
node, or -EEXIST), the helper re-queues the current port, drops the extra
kref and returns early, so the remaining stale ports are never restored:
they keep PORT_S_ON_STALE_LIST set and port->port_mngr == NULL.

Since the error is swallowed, the loop below then finds nothing for those
rx_ch values:

	if (!mtk_port_search_by_id(port_mngr, dflt_port->rx_ch)) {
		port = mtk_port_alloc_and_add(port_mngr, dflt_port);

and creates a second port object for the same channel, while
mtk_port_mngr_init() still returns 0.

Does the user still holding the stale handle then end up with a port whose
port_mngr is NULL and PORT_S_ENABLE clear, so every mtk_port_send_data()
on it fails in mtk_port_status_check() with -ENODEV?

> +
> +	/* copy ports from static port cfg table */
> +	for (i = 0; i < port_cnt; i++) {
> +		dflt_port = cfg + i;
> +		if (!mtk_port_search_by_id(port_mngr, dflt_port->rx_ch)) {
> +			port = mtk_port_alloc_and_add(port_mngr, dflt_port);
> +			if (IS_ERR(port))
> +				return PTR_ERR(port);
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static void mtk_port_tbl_destroy(struct mtk_port_mngr *port_mngr, struct mtk_stale_list *s_list)
> +{
> +	struct radix_tree_iter iter;
> +	struct mtk_port *port;
> +	void __rcu **slot;
> +	int tbl_type;
> +
> +	tbl_type = PORT_TBL_SAP;
> +	do {
> +		radix_tree_for_each_slot(slot, &port_mngr->port_tbl[tbl_type], &iter, 0) {
> +			port = radix_tree_deref_slot(slot);
> +			if (!port)
> +				continue;
> +			ports_ops[port->info.type]->disable(port);
> +		}

[Severity: Medium]

radix_tree_deref_slot() is rcu_dereference(*slot), and this loop runs in
plain process context (driver removal and the mtk_port_mngr_init() error
path) with no rcu_read_lock() held. With CONFIG_PROVE_RCU=y, does this
produce a "suspicious rcu_dereference_check() usage" splat on every
teardown?

Wrapping the loop in rcu_read_lock() would not work either, since
ports_ops[]->disable() sleeps: mtk_port_ch_disable() does a GFP_KERNEL
skb allocation and then wait_event_timeout() for up to
MTK_DFLT_TRB_TIMEOUT.

More generally, the synchronisation the radix-tree API asks callers to
provide looks incomplete here. The mutators run under port_mngr_grp_mtx
from mtk_port_restore_from_stale_list() and mtk_port_free_or_backup(), but
with no lock at all from mtk_port_tbl_create() -> mtk_port_alloc_and_add()
-> mtk_port_tbl_add(); the readers mtk_port_search_by_id() and
mtk_port_search_by_name() hold neither the mutex nor rcu_read_lock(), and
mtk_port_release() frees a tree-published object with a plain kfree():

	ports_ops[port->info.type]->exit(port);
	kfree(port);

Would a single writer lock covering all mutators (including the port_cnt
updates) plus RCU-deferred freeing be more appropriate?

[ ... ]

> +static struct mtk_stale_list *mtk_port_stale_list_init(struct mtk_ctrl_blk *ctrl_blk, int *dev_id)
> +{
> +	struct mtk_stale_list *s_list;
> +
> +	s_list = mtk_port_stale_list_search(ctrl_blk->mdev->dev_str);
> +	if (!s_list) {
> +		s_list = mtk_port_stale_list_create(ctrl_blk);
> +		if (unlikely(!s_list))
> +			return NULL;
> +	}
> +
> +	mutex_lock(&port_mngr_grp_mtx);
> +	if (s_list->dev_id < 0) {
> +		*dev_id = ida_alloc_range(&ccci_dev_ids, 0, MTK_DFLT_MAX_DEV_CNT - 1, GFP_KERNEL);

[Severity: Low]

The ida_alloc_range() return value is stored in *dev_id without being
tested, and mtk_port_stale_list_init() still reports success. On -ENOMEM,
or -ENOSPC once MTK_DFLT_MAX_DEV_CNT (10) devices are bound, does an errno
end up in port_mngr->dev_id while mtk_port_mngr_init() returns 0?

That value is later passed to ida_free() unconditionally in
mtk_port_stale_list_exit() below, while the other two ida_free() call
sites in this file guard it, in mtk_port_release():

	list_empty(&s_list->ports) && s_list->dev_id >= 0) {
		ida_free(&ccci_dev_ids, s_list->dev_id);

and in mtk_port_stale_list_grp_cleanup():

	if (s_list->dev_id >= 0)
		ida_free(&ccci_dev_ids, s_list->dev_id);

The bogus free itself is harmless, since ida_free() starts with
"if ((int)id < 0) return;", so this is about the unchecked allocation and
the inconsistency with the guarded call sites.

> +	} else {
> +		*dev_id = s_list->dev_id;
> +		s_list->dev_id = -1;
> +	}
> +	mutex_unlock(&port_mngr_grp_mtx);
> +
> +	return s_list;
> +}
> +
> +static void mtk_port_stale_list_exit(struct mtk_ctrl_blk *ctrl_blk,
> +				     struct mtk_stale_list *s_list, int dev_id)
> +{
> +	if (!s_list)
> +		return;
> +	mutex_lock(&port_mngr_grp_mtx);
> +	if (list_empty(&s_list->ports)) {
> +		ida_free(&ccci_dev_ids, dev_id);

[ ... ]

> diff --git a/drivers/net/wwan/t9xx/mtk_port.h b/drivers/net/wwan/t9xx/mtk_port.h
> new file mode 100644
> index 0000000000000..a0248d7f939ae
> --- /dev/null
> +++ b/drivers/net/wwan/t9xx/mtk_port.h
> @@ -0,0 +1,150 @@

[ ... ]

> +enum mtk_port_flag {
> +	PORT_F_DFLT = 0,
> +	PORT_F_BLOCKING = BIT(1),
> +	PORT_F_ALLOW_DROP = BIT(2),

[Severity: Low]

This isn't a bug, but PORT_F_ALLOW_DROP has no reader anywhere. It is set
on every entry of port_cfg_m9xx[], yet the only runtime consumer of
port->info.flags is mtk_port_internal_write():

	return mtk_port_send_data(port, skb,
				  !!(port->info.flags & PORT_F_BLOCKING),
				  !!(port->info.flags &
				     (PORT_F_BLOCKING | PORT_F_FORCE_SEND)));

Ports with and without the flag behave identically. Could it be dropped
until the code that honours it is added?

> +	PORT_F_FORCE_SEND = BIT(6),
> +};

[ ... ]

> diff --git a/drivers/net/wwan/t9xx/mtk_port_io.c b/drivers/net/wwan/t9xx/mtk_port_io.c
> new file mode 100644
> index 0000000000000..27b490fde40f2
> --- /dev/null
> +++ b/drivers/net/wwan/t9xx/mtk_port_io.c
> @@ -0,0 +1,247 @@

[ ... ]

> +static void mtk_port_internal_enable(struct mtk_port *port)
> +{
> +	int ret;
> +
> +	if (test_bit(PORT_S_ENABLE, &port->status))
> +		return;
> +
> +	ret = mtk_port_ch_enable(port);
> +	if (ret && ret != -EBUSY) {
> +		/* On -ETIMEDOUT the ENABLE trb may still be queued; queue a
> +		 * DISABLE behind it so the channel does not end up armed with
> +		 * no software owner.
> +		 */
> +		mtk_port_ch_disable(port);
> +		return;
> +	}
> +
> +	set_bit(PORT_S_WR, &port->status);
> +	set_bit(PORT_S_ENABLE, &port->status);
> +}
> +
> +static void mtk_port_internal_disable(struct mtk_port *port)
> +{
> +	if (!test_and_clear_bit(PORT_S_ENABLE, &port->status))
> +		return;
> +
> +	clear_bit(PORT_S_WR, &port->status);
> +	mtk_port_ch_disable(port);
> +}

[Severity: High]

mtk_port_internal_disable() clears PORT_S_ENABLE and then discards the
return value of mtk_port_ch_disable(), which can fail with -ENOMEM (skb
allocation), -EIO (mtk_pcie_hif_submit_skb() whenever
atomic_read(&trans->available) == 0) or -ETIMEDOUT after
MTK_DFLT_TRB_TIMEOUT. Can this leave the software state claiming the
channel is down while trans->usr_cnt[hif_id][txqno] still counts a user?

The only decrement is in mtk_ch_status_check():

	case TRB_CMD_DISABLE:
		if (trans->usr_cnt[que->hif_id][que->txqno] > 0) {
			trans->usr_cnt[que->hif_id][que->txqno]--;
			if (!trans->usr_cnt[que->hif_id][que->txqno])
				break;
		}
		trb->status = -EBUSY;

and mtk_ctrl_ch_flush() / mtk_ctrl_chs_flush() complete queued
ENABLE/DISABLE TRBs with

	trb->status = -EIO;
	trb->trb_complete(skb);

without passing through mtk_ch_status_check() at all.

For a modem that stops responding during teardown:

FSM_STATE_OFF
  mtk_port_disable() -> mtk_port_ch_disable() returns -ETIMEDOUT, the
  DISABLE skb is still queued
  mtk_port_internal_disable() ignores it and clears PORT_S_ENABLE
ops->exit() -> mtk_pcie_hif_exit()
  mtk_cldma_exit() frees drv_info->txq[]/rxq[]
  mtk_ctrl_trb_srv_exit() -> kthread_stop() -> mtk_ctrl_chs_flush()
  completes the queued DISABLE with -EIO, usr_cnt stays at 1

On the next bootup the ENABLE raises usr_cnt to 2, so
mtk_ch_status_check() takes the "already open" branch,
mtk_cldma_check_ch_cfg() finds drv_info->txq/rxq == NULL and returns
-EINVAL, and mtk_port_ch_enable() fails. The compensating
mtk_port_ch_disable() in mtk_port_internal_enable() then only takes
usr_cnt from 2 back to 1, never to 0.

Does that make the control channel unusable for the rest of the probe, so
the MD/SAP handshake never completes and the modem never reaches
FSM_STATE_READY, with every later boot cycle repeating it?

Related question on the same function: is -EBUSY safe to treat as a
successful enable without distinguishing where it came from?
mtk_cldma_open() rolls the usr_cnt reference back at its out: label when
it returns -EBUSY, while mtk_ch_status_check() keeps it.

[ ... ]

> diff --git a/drivers/net/wwan/t9xx/pcie/mtk_ctrl_cfg_m9xx.c b/drivers/net/wwan/t9xx/pcie/mtk_ctrl_cfg_m9xx.c
> index 0019b64b037af..050416e0b914d 100644
> --- a/drivers/net/wwan/t9xx/pcie/mtk_ctrl_cfg_m9xx.c
> +++ b/drivers/net/wwan/t9xx/pcie/mtk_ctrl_cfg_m9xx.c
> @@ -21,7 +22,24 @@ static const struct queue_info mtk_queue_info_m9xx[] = {
>  	 Q_MTU_3_5K, Q_MTU_3_5K, TX_GPD_NUM, RX_GPD_NUM, Q_FRAG_3_5K, Q_FRAG_3_5K, 0},
>  };
>  
> +static const struct mtk_port_cfg port_cfg_m9xx[] = {
> +	{CCCI_CONTROL_TX, CCCI_CONTROL_RX, PORT_TYPE_INTERNAL, "MDCTRL",
> +		PORT_F_ALLOW_DROP},
> +	{CCCI_SAP_CONTROL_TX, CCCI_SAP_CONTROL_RX, PORT_TYPE_INTERNAL, "SAPCTRL",
> +		PORT_F_ALLOW_DROP},
> +};

These are the two descriptors that request the PORT_F_ALLOW_DROP behaviour
mentioned above.

[ ... ]