Re: [PATCH net v4] net: wwan: qcom_bam_dmux: account network packets

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider:

Critical: 0 · High: 0 · Medium: 1 · Low: 0

- [Medium] In `bam_dmux_netdev_start_xmit()` the new
  `dev_sw_netstats_tx_add(netdev, 1, len)`…

--

net: wwan: qcom_bam_dmux: account network packets

The BAM-DMUX data path did not update the netdev packet and byte counters,
so userspace saw zero traffic.  This adds per-CPU software statistics with
NETDEV_PCPU_STAT_TSTATS and dev_get_tstats64(), counting TX in
bam_dmux_netdev_start_xmit() and RX in bam_dmux_cmd_data().

> diff --git a/drivers/net/wwan/qcom_bam_dmux.c b/drivers/net/wwan/qcom_bam_dmux.c
> index cc6ace8d64371..03360301c8ee5 100644
> --- a/drivers/net/wwan/qcom_bam_dmux.c
> +++ b/drivers/net/wwan/qcom_bam_dmux.c

[ ... ]

> @@ -357,17 +358,19 @@ static netdev_tx_t bam_dmux_netdev_start_xmit(struct sk_buff *skb,
>  		if (!atomic_long_fetch_or(BIT(skb_dma - dmux->tx_skbs),
>  					  &dmux->tx_deferred_skb))
>  			queue_pm_work(&dmux->tx_wakeup_work);
> -		return NETDEV_TX_OK;
> -	}
> +	} else {
> +		if (!bam_dmux_skb_dma_submit_tx(skb_dma))
> +			goto drop;
>  
> -	if (!bam_dmux_skb_dma_submit_tx(skb_dma))
> -		goto drop;
> +		dma_async_issue_pending(dmux->tx);
> +	}
>  
> -	dma_async_issue_pending(dmux->tx);
> +	dev_sw_netstats_tx_add(netdev, 1, len);
>  	return NETDEV_TX_OK;

[Severity: Medium]
Is it intended that dev_sw_netstats_tx_add() now also covers the deferred
branch, where nothing has been submitted to the DMA engine yet?

In the active <= 0 case the skb is only marked in dmux->tx_deferred_skb and
bam_dmux_tx_wakeup_work() is queued.  The descriptor is prepared later in
that worker, which can bail out before submitting anything:

drivers/net/wwan/qcom_bam_dmux.c:bam_dmux_tx_wakeup_work() {
	ret = pm_runtime_resume_and_get(dmux->dev);
	if (ret < 0) {
		dev_err(dmux->dev, "Failed to resume: %d\n", ret);
		return;
	}
	...
	for_each_set_bit(i, &pending, BAM_DMUX_NUM_SKB) {
		bam_dmux_skb_dma_submit_tx(&dmux->tx_skbs[i]);
	}
	...
}

bam_dmux_runtime_resume() can return -ETIMEDOUT from any of its three 2
second waits on the remote modem, or -ENXIO when the TX channel cannot be
requested, for example after a modem crash or power collapse.  In that case
the worker returns with the packet still sitting in dmux->tx_skbs[], while
tx_packets/tx_bytes have already been incremented for it and tx_dropped is
never touched.

The other failure mode is the return value of the same helper being ignored
in the worker:

drivers/net/wwan/qcom_bam_dmux.c:bam_dmux_skb_dma_submit_tx() {
	desc = dmaengine_prep_slave_single(dmux->tx, skb_dma->addr,
					   skb_dma->skb->len, DMA_MEM_TO_DEV,
					   DMA_PREP_INTERRUPT);
	if (!desc) {
		dev_err(dmux->dev, "Failed to prepare TX DMA buffer\n");
		return false;
	}
	...
}

So the two callers of bam_dmux_skb_dma_submit_tx() end up with opposite
accounting: start_xmit() counts a drop via dev_core_stats_tx_dropped_inc(),
the worker counts nothing at all.

The commit message says the counters are updated "after the driver has
prepared and accepted them", which explains counting at accept time, but
would it be better to account the deferred packets from
bam_dmux_tx_wakeup_work() once submission succeeds, and to increment
tx_dropped there when it does not?

> drop:
>  	bam_dmux_tx_done(skb_dma);
> +	dev_core_stats_tx_dropped_inc(netdev);
>  	dev_kfree_skb_any(skb);
>  	return NETDEV_TX_OK;
>  }

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260904183737.2994139-1-sinyavin%40gmail.com