Re: [PATCH v1 08/15] net/xsc: add ethernet TX path

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [High] net/xsc: NETDEV_TX_BUSY on active queues in xsc_eth_xmit_start()
- [High] net/xsc: race waking stopped TX queues in xsc_eth_deactivate_priv_channels()
- [High] net/xsc: CQs armed before NAPI and queue state init in xsc_eth_open_channels()
- [Medium] net/xsc: missed error CQE checks in xsc_poll_tx_cq() poll loop
- [Medium] net/xsc: missing TX doorbell on err_drop path in xsc_eth_xmit_frame()
- [Medium] net/xsc: fix CQE error check, sqcc update, and budget in xsc_poll_tx_cq()
- [Low] net/xsc: chls->num_chl zeroed before error log in xsc_eth_open_channels()
--

commit 251dc7b87bb643704bb618df57c60cfb11aec1a1
Author: Xin Tian <tianx@yunsilicon.com>

net/xsc: add ethernet TX path

This commit implements the Ethernet transmit path for the xsc driver,
including per-channel CQ and raw-TSO SQ setup, NAPI completion polling,
WQE construction with DMA mapping and BQL tracking, and offload feature
advertisement.

> diff --git a/drivers/net/ethernet/yunsilicon/xsc/net/main.c b/drivers/net/ethernet/yunsilicon/xsc/net/main.c
> index 21a50085fa879..a02f95145d018 100644
> --- a/drivers/net/ethernet/yunsilicon/xsc/net/main.c
> +++ b/drivers/net/ethernet/yunsilicon/xsc/net/main.c
> @@ -95,22 +95,825 @@ static int xsc_eth_netdev_init(struct xsc_adapter *adapter)

[ ... ]

> +static void xsc_eth_completion_event(struct xsc_core_cq *xcq)
> +{
> +	struct xsc_cq *cq = container_of(xcq, struct xsc_cq, xcq);
> +	struct xsc_core_device *xdev = cq->xdev;
> +	struct xsc_rq *rq = NULL;
> +
> +	if (unlikely(!cq->channel)) {
> +		netdev_err(xsc_dev_to_netdev(xdev), "cq%d->channel is null\n",
> +			   xcq->cqn);
> +		return;
> +	}
> +
> +	rq = &cq->channel->qp.rq;
> +
> +	if (!test_bit(XSC_ETH_RQ_STATE_ENABLED, &rq->state))
> +		netdev_err(xsc_dev_to_netdev(xdev),
> +			   "ch%d_cq%d, napi_flag=0x%lx\n",
> +			   cq->channel->chl_idx, xcq->cqn,
> +			   cq->napi->state);
> +
> +	napi_schedule(cq->napi);
> +	cq->event_ctr++;
> +}

[ ... ]

> +static int xsc_eth_open_cq(struct xsc_channel *c,
> +			   struct xsc_cq *pcq,
> +			   struct xsc_cq_param *pcq_param)
> +{
> +	int ret;
> +
> +	ret = xsc_eth_alloc_cq(c, pcq, pcq_param);
> +	if (ret)
> +		return ret;
> +
> +	ret = xsc_eth_set_cq(c, pcq, pcq_param);
> +	if (ret)
> +		goto err_free_cq;
> +
> +	xsc_cq_notify_hw_rearm(pcq);
> +
> +	pcq->napi = &c->napi;
> +	pcq->channel = c;
> +
> +	return 0;
> +
> +err_free_cq:
> +	xsc_eth_free_cq(pcq);
> +	return ret;
> +}

[ ... ]

> +static int xsc_eth_open_channel(struct xsc_adapter *adapter,
> +				int idx,
> +				struct xsc_channel *c,
> +				struct xsc_channel_param *chl_param)
> +{
> +	struct xsc_core_device *xdev = adapter->xdev;
> +	struct net_device *netdev = adapter->netdev;
> +	const struct cpumask *aff;
> +	unsigned int irqn;
> +	u32 eqn;
> +	int ret;
> +
> +	c->adapter = adapter;
> +	c->netdev = adapter->netdev;
> +	c->chl_idx = idx;
> +
> +	ret = xsc_core_vector2eqn(xdev, c->chl_idx, &eqn, &irqn);
> +	if (ret)
> +		goto err_out;
> +	aff = irq_get_affinity_mask(irqn);
> +	c->aff_mask = aff;
> +	c->cpu = cpumask_first(aff);
> +
> +	ret = xsc_eth_open_cq(c, &c->qp.rq.cq, &chl_param->rqcq_param);
> +	if (ret)
> +		goto err_out;
> +
> +	ret = xsc_eth_open_cq(c, &c->qp.sq.cq, &chl_param->sqcq_param);
> +	if (ret)
> +		goto err_close_rq_cq;
> +
> +	ret = xsc_eth_open_qp_sq(c, &c->qp.sq, &chl_param->sq_param);
> +	if (ret)
> +		goto err_close_sq_cq;
> +
> +	netif_napi_add(netdev, &c->napi, xsc_eth_napi_poll);

[Severity: High]
Could this ordering between CQ creation, NAPI initialization, and queue
state enablement cause a NULL pointer dereference or unratelimited log
spam from hardirq context?

In xsc_eth_open_cq(), xsc_eth_set_cq() calls xsc_core_eth_create_cq(),
which inserts &pcq->xcq into xdev->dev_res->cq_table.tree while MSI-X EQs
are active, and then calls xsc_cq_notify_hw_rearm(pcq) before setting
pcq->napi and pcq->channel.

Furthermore, in xsc_eth_open_channel(), both CQs (rq.cq and sq.cq) are
opened and armed before netif_napi_add(netdev, &c->napi, xsc_eth_napi_poll)
initializes c->napi. Because c is zero-allocated with kcalloc_node(),
c->napi.state has NAPIF_STATE_SCHED cleared and c->napi.poll is NULL prior
to netif_napi_add(). If xsc_eq_int() dispatches XSC_EVENT_TYPE_COMP or
XSC_EVENT_TYPE_INTERNAL_ERROR for the CQ after pcq->channel is assigned
and before netif_napi_add() runs, xsc_eth_completion_event() will call
napi_schedule(cq->napi) on an uninitialized napi_struct.

In addition, xsc_eth_completion_event() checks
!test_bit(XSC_ETH_RQ_STATE_ENABLED, &rq->state) and logs an unratelimited
netdev_err() from hardirq context. In this commit XSC_ETH_RQ_STATE_ENABLED
is never set anywhere, so every TX completion interrupt triggers
netdev_err(). Even once RX sets XSC_ETH_RQ_STATE_ENABLED in
xsc_eth_activate_priv_channels(), xsc_eth_open_channels() transitions QPs
to RTS before xsc_eth_activate_priv_channels() sets the enabled state bits
(and xsc_eth_deactivate_channels() clears them before tearing down the QPs
and CQs), during which time xsc_poll_tx_cq() returns early when
XSC_ETH_SQ_STATE_ENABLED is clear while xsc_eth_napi_poll() re-arms the CQ
without draining it.

Should netif_napi_add() and pcq->napi / pcq->channel initialization happen
before registering and arming the CQs, and should the queue state bits be
set before arming CQs and transitioning QPs to RTS?

> +
> +	netdev_dbg(adapter->netdev, "open channel%d ok\n", idx);
> +	return 0;

[ ... ]

> +err_close_channel:
> +	for (--i; i >= 0; i--)
> +		xsc_eth_close_channel(&chls->c[i], false);
> +
> +	kvfree(chl_param);
> +err_free_ch:
> +	kfree(chls->c);
> +err_out:
> +	chls->num_chl = 0;
> +	netdev_err(adapter->netdev, "failed to open %d channels, err=%d\n",
> +		   chls->num_chl, ret);
> +	return ret;
> +}

[Severity: Low]
In xsc_eth_open_channels(), chls->num_chl is set to 0 right before the
netdev_err() call at err_out. Won't this always log "failed to open 0
channels" on failure instead of the number of channels that failed to
open?

[ ... ]

> +static void xsc_eth_deactivate_channels(struct xsc_eth_channels *chs)
> +{
> +	int i;
> +
> +	for (i = 0; i < chs->num_chl; i++)
> +		xsc_eth_deactivate_channel(&chs->c[i]);
> +
> +	/* Sync with all NAPIs to wait until they stop using queues. */
> +	synchronize_net();
> +
> +	for (i = 0; i < chs->num_chl; i++)
> +		/* last doorbell out */
> +		napi_disable(&chs->c[i].napi);
> +}
> +
> +static void xsc_eth_activate_priv_channels(struct xsc_adapter *adapter)
> +{
> +	struct net_device *netdev = adapter->netdev;
> +
> +	netif_set_real_num_tx_queues(netdev, adapter->channels.num_chl);
> +	netif_set_real_num_rx_queues(netdev, adapter->channels.num_chl);
> +
> +	xsc_eth_build_tx2sq_maps(adapter);
> +	xsc_eth_activate_channels(&adapter->channels);
> +	netif_tx_start_all_queues(adapter->netdev);
> +}
> +
> +static void xsc_eth_deactivate_priv_channels(struct xsc_adapter *adapter)
> +{
> +	netif_tx_disable(adapter->netdev);
> +	xsc_eth_deactivate_channels(&adapter->channels);
> +}

[Severity: High]
Can a concurrent NAPI poll re-wake a TX queue during teardown in
xsc_eth_deactivate_priv_channels()?

xsc_eth_deactivate_priv_channels() calls netif_tx_disable(adapter->netdev)
before xsc_eth_deactivate_channels() clears XSC_ETH_SQ_STATE_ENABLED and
disables NAPI. If xsc_poll_tx_cq() is running concurrently on another CPU,
it can still see XSC_ETH_SQ_STATE_ENABLED set, observe
netif_tx_queue_stopped(sq->txq) is true (just set by netif_tx_disable()),
and call netif_tx_wake_queue(sq->txq):

drivers/net/ethernet/yunsilicon/xsc/net/xsc_eth_txrx.c:xsc_poll_tx_cq() {
    ...
	if (netif_tx_queue_stopped(sq->txq) &&
	    xsc_wqc_has_room_for(&sq->wq, sq->cc, sq->pc, sq->stop_room)) {
		netif_tx_wake_queue(sq->txq);
	}
}

Wouldn't that clear __QUEUE_STATE_DRV_XOFF and leave the TX queue awake
while xsc_eth_sw_deinit() proceeds to free TX WQEs and close the channels?
Should xsc_eth_deactivate_channels() run before netif_tx_disable()?

[ ... ]

> @@ -172,14 +1053,26 @@ static int xsc_set_port_admin_status(struct xsc_adapter *adapter,
>  static int xsc_eth_open(struct net_device *netdev)
>  {
>  	struct xsc_adapter *adapter = netdev_priv(netdev);
> +	bool link_up;
>  	int ret;
>  
>  	if (adapter->opened) {
>  		netdev_err(adapter->netdev, "ndo_open on an already opened device\n");
>  		return 0;
>  	}
> +	ret = xsc_eth_sw_init(adapter);
> +	if (ret)
> +		return ret;
>  
> -	if (xsc_eth_get_link_status(adapter))	{
> +	ret = xsc_eth_enable_nic_hca(adapter);
> +	if (ret)
> +		goto err_sw_deinit;
> +
> +	ret = xsc_eth_get_link_status(adapter, &link_up);
> +	if (ret)
> +		netdev_warn(netdev, "failed to query link status, err=%d\n", ret);
> +
> +	if (!ret && link_up) {
>  		netdev_info(netdev, "Link up\n");
>  		netif_carrier_on(adapter->netdev);
>  	} else {
> @@ -196,6 +1089,10 @@ static int xsc_eth_open(struct net_device *netdev)
>  			   ret);
>  
>  	return 0;
> +
> +err_sw_deinit:
> +	xsc_eth_sw_deinit(adapter);
> +	return ret;
>  }
>  
>  static int xsc_eth_close(struct net_device *netdev)
> @@ -209,6 +1106,13 @@ static int xsc_eth_close(struct net_device *netdev)
>  
>  	netif_carrier_off(adapter->netdev);
>  
> +	xsc_eth_sw_deinit(adapter);

[Severity: High]
Can xsc_eth_xmit_start() return NETDEV_TX_BUSY on an active, unstopped TX
queue during xsc_eth_open() and xsc_eth_close()?

In xsc_eth_open(), xsc_eth_sw_init() calls xsc_eth_activate_priv_channels(),
which calls netif_set_real_num_tx_queues() and
netif_set_real_num_rx_queues() (without checking their return values) and
starts all TX queues via netif_tx_start_queue() and
netif_tx_start_all_queues() while adapter->opened is still false.
xsc_eth_open() then sleeps in xsc_eth_enable_nic_hca() and
xsc_eth_get_link_status() and calls netif_carrier_on() before setting
adapter->opened = true (and xsc_eth_link_event_handler() can also turn the
carrier on asynchronously).

Similarly, in xsc_eth_close(), adapter->opened = false is set before
netif_carrier_off() and before xsc_eth_sw_deinit() calls
netif_tx_disable().

During both windows, TX queues are active and unstopped while
adapter->opened is false. Any packet reaching xsc_eth_xmit_start() will
hit:

drivers/net/ethernet/yunsilicon/xsc/net/xsc_eth_tx.c:xsc_eth_xmit_start() {
	if (!adapter->opened)
		return NETDEV_TX_BUSY;
}

without stopping the TX queue, causing sch_direct_xmit() to requeue the skb
and reschedule NET_TX_SOFTIRQ in a tight retry loop (and leaving any
earlier skbs in a netdev_xmit_more() batch without a doorbell ring).

Should adapter->opened be set before starting the TX queues (or TX queues
started only after HCA enablement and adapter->opened = true), and in
xsc_eth_close() should the TX queues be disabled before clearing
adapter->opened?

> diff --git a/drivers/net/ethernet/yunsilicon/xsc/net/xsc_eth_tx.c b/drivers/net/ethernet/yunsilicon/xsc/net/xsc_eth_tx.c
> new file mode 100644
> index 0000000000000..376b2cd0d8f29
> --- /dev/null
> +++ b/drivers/net/ethernet/yunsilicon/xsc/net/xsc_eth_tx.c

[ ... ]

> +static void xsc_txwqe_complete(struct xsc_sq *sq, struct sk_buff *skb,
> +			       u8 opcode, u16 ds_cnt,
> +			       u8 num_wqebbs, u32 num_bytes, u8 num_dma,
> +			       struct xsc_tx_wqe_info *wi)
> +{
> +	struct xsc_wq_cyc *wq = &sq->wq;
> +
> +	wi->num_bytes = num_bytes;
> +	wi->num_dma = num_dma;
> +	wi->num_wqebbs = num_wqebbs;
> +	wi->skb = skb;
> +
> +	netdev_tx_sent_queue(sq->txq, num_bytes);
> +
> +	sq->pc += wi->num_wqebbs;
> +
> +	if (unlikely(!xsc_wqc_has_room_for(wq, sq->cc, sq->pc, sq->stop_room)))
> +		netif_tx_stop_queue(sq->txq);
> +
> +	if (!netdev_xmit_more() || netif_xmit_stopped(sq->txq))
> +		xsc_sq_notify_hw(wq, sq->pc, sq);
> +}
> +
> +static uint32_t xsc_eth_xmit_frame(struct sk_buff *skb,
> +				   struct xsc_sq *sq,
> +				   struct xsc_tx_wqe *wqe,
> +				   u16 pi)
> +{

[ ... ]

> +	xsc_txwqe_complete(sq, skb, opcode, ds_cnt, num_wqebbs, num_bytes,
> +			   num_dma, wi);
> +	stats->bytes     += num_bytes;
> +	stats->packets   += num_pkts;
> +
> +	return NETDEV_TX_OK;
> +
> +err_drop:
> +	stats->dropped++;
> +	dev_kfree_skb_any(skb);
> +
> +	return NETDEV_TX_OK;
> +}

[Severity: Medium]
What happens to previously queued WQEs in a netdev_xmit_more() batch if a
subsequent packet in that batch takes the err_drop path in
xsc_eth_xmit_frame()?

When earlier packets in a batch are queued with netdev_xmit_more() true and
the queue is not stopped, xsc_txwqe_complete() advances sq->pc and skips
xsc_sq_notify_hw(). If a later packet (such as the final packet in the
batch where netdev_xmit_more() is false) jumps to err_drop (for example on
skb->len > sq->hw_mtu, skb_linearize() failure, skb->len == 0, or DMA
mapping failure in xsc_txwqe_build_dsegs()), err_drop frees the skb and
returns without ringing the doorbell for the earlier WQEs. Should err_drop
call xsc_sq_notify_hw() when !netdev_xmit_more() or when uncommitted WQEs
are pending?

> diff --git a/drivers/net/ethernet/yunsilicon/xsc/net/xsc_eth_txrx.c b/drivers/net/ethernet/yunsilicon/xsc/net/xsc_eth_txrx.c
> new file mode 100644
> index 0000000000000..262421740e974
> --- /dev/null
> +++ b/drivers/net/ethernet/yunsilicon/xsc/net/xsc_eth_txrx.c

[ ... ]

> +static bool xsc_poll_tx_cq(struct xsc_cq *cq, int napi_budget)
> +{
> +	struct xsc_adapter *adapter;
> +	struct xsc_cqe *cqe;
> +	struct device *dev;
> +	struct xsc_sq *sq;
> +	u32 dma_fifo_cc;
> +	u32 nbytes = 0;
> +	u16 npkts = 0;
> +	int i = 0;
> +	u16 sqcc;
> +
> +	sq = container_of(cq, struct xsc_sq, cq);
> +	if (!test_bit(XSC_ETH_SQ_STATE_ENABLED, &sq->state))
> +		return false;
> +
> +	adapter = sq->channel->adapter;
> +	dev = adapter->dev;
> +
> +	cqe = xsc_cqwq_get_cqe(&cq->wq);
> +	if (!cqe)
> +		goto out;
> +
> +	if (unlikely(get_cqe_opcode(cqe) & BIT(7)))
> +		xsc_dump_error_sqcqe(sq, cqe);
> +
> +	sqcc = sq->cc;
> +
> +	/* avoid dirtying sq cache line every cqe */
> +	dma_fifo_cc = sq->dma_fifo_cc;
> +	i = 0;
> +	do {
> +		struct xsc_tx_wqe_info *wi;
> +		struct sk_buff *skb;
> +		int j;
> +		u16 ci;
> +
> +		xsc_cqwq_pop(&cq->wq);
> +
> +		ci = xsc_wq_cyc_ctr2ix(&sq->wq, sqcc);
> +		wi = &sq->db.wqe_info[ci];
> +		skb = wi->skb;
> +
> +		if (unlikely(!skb))
> +			continue;
> +
> +		for (j = 0; j < wi->num_dma; j++) {
> +			struct xsc_sq_dma *dma = xsc_dma_get(sq, dma_fifo_cc++);
> +
> +			xsc_tx_dma_unmap(dev, dma);
> +		}
> +
> +		npkts++;
> +		nbytes += wi->num_bytes;
> +		sqcc += wi->num_wqebbs;
> +		napi_consume_skb(skb, 0);
> +
> +	} while ((++i < napi_budget) && (cqe = xsc_cqwq_get_cqe(&cq->wq)));

[Severity: Medium]
There are a few issues in the xsc_poll_tx_cq() loop:

1. get_cqe_opcode(cqe) & BIT(7) is only checked once before the do-while
loop rather than inside the loop before xsc_cqwq_pop(&cq->wq). Won't any
error CQEs fetched after the first CQE in a NAPI poll batch be silently
ignored instead of logged via xsc_dump_error_sqcqe()?

2. If unlikely(!skb) is taken after xsc_cqwq_pop(&cq->wq), the loop
executes continue without advancing sqcc (unlike xsc_free_tx_wqe(), which
increments sq->cc++), and wi->skb is never cleared when a WQE completes.
Could that desynchronize sqcc from the CQ if a nop WQE is encountered?

3. napi_consume_skb(skb, 0) is called with a hardcoded budget of 0, which
causes napi_consume_skb() to always take the !budget non-NAPI fallback
(dev_consume_skb_any()) instead of using the NAPI per-CPU skb cache. Should
the NAPI poll budget be passed through to napi_consume_skb() here?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261104155650.37053-1-tianx@yunsilicon.com?part=8