On 8/26/19 7:32 PM, Yunsheng Lin wrote:

On 2019/8/27 5:33, Shannon Nelson wrote:

quoted

Add both the Tx and Rx queue setup and handling.  The related
stats display comes later.  Instead of using the generic napi
routines used by the slow-path commands, the Tx and Rx paths
are simplified and inlined in one file in order to get better
compiler optimizations.

Signed-off-by: Shannon Nelson <redacted>
---

[...]

quoted

+static int ionic_txrx_init(struct ionic_lif *lif)
+{
+	unsigned int i;
+	int err;
+
+	for (i = 0; i < lif->nxqs; i++) {
+		err = ionic_lif_txq_init(lif, lif->txqcqs[i].qcq);
+		if (err)
+			goto err_out;
+
+		err = ionic_lif_rxq_init(lif, lif->rxqcqs[i].qcq);
+		if (err) {
+			ionic_lif_qcq_deinit(lif, lif->txqcqs[i-1].qcq);
+			goto err_out;
+		}
+	}
+
+	ionic_set_rx_mode(lif->netdev);
+
+	return 0;
+
+err_out:
+	for (i--; i > 0; i--) {
+		ionic_lif_qcq_deinit(lif, lif->txqcqs[i-1].qcq);
+		ionic_lif_qcq_deinit(lif, lif->rxqcqs[i-1].qcq);
+	}

The "i--" has been done in for initialization, and
ionic_lif_qcq_deinit is called with lif->rxqcqs[i-1], which may
cause the last lif->txqcqs or lif->rxqcqs not initialized problem.

It may be more common to do the below:
while (i--) {
	ionic_lif_qcq_deinit(lif, lif->txqcqs[i].qcq);
	ionic_lif_qcq_deinit(lif, lif->rxqcqs[i].qcq);
}

Sure.

quoted

+
+	return err;
+}
+
+static int ionic_txrx_enable(struct ionic_lif *lif)
+{
+	int i, err;
+
+	for (i = 0; i < lif->nxqs; i++) {
+		err = ionic_qcq_enable(lif->txqcqs[i].qcq);
+		if (err)
+			goto err_out;
+
+		ionic_rx_fill(&lif->rxqcqs[i].qcq->q);
+		err = ionic_qcq_enable(lif->rxqcqs[i].qcq);
+		if (err) {
+			ionic_qcq_disable(lif->txqcqs[i].qcq);
+			goto err_out;
+		}
+	}
+
+	return 0;
+
+err_out:
+	for (i--; i >= 0 ; i--) {
+		ionic_qcq_disable(lif->rxqcqs[i].qcq);
+		ionic_qcq_disable(lif->txqcqs[i].qcq);
+	}

It may be better to use the above pattern too.

Okay

quoted

+static dma_addr_t ionic_tx_map_single(struct ionic_queue *q, void *data, size_t len)
+{
+	struct ionic_tx_stats *stats = q_to_tx_stats(q);
+	struct device *dev = q->lif->ionic->dev;
+	dma_addr_t dma_addr;
+
+	dma_addr = dma_map_single(dev, data, len, DMA_TO_DEVICE);
+	if (dma_mapping_error(dev, dma_addr)) {
+		net_warn_ratelimited("%s: DMA single map failed on %s!\n",
+				     q->lif->netdev->name, q->name);
+		stats->dma_map_err++;
+		return 0;

zero may be a valid dma address, maybe check the dma_mapping_error in
ionic_tx_tso instead.

Hmmm, hadn't thought of 0 as a valid address...
I'll need to make a similar adjustment to ionic_tx_map_frag() uses.

+
+static void ionic_tx_tcp_inner_pseudo_csum(struct sk_buff *skb)
+{
+	skb_cow_head(skb, 0);
May need to check for return error of skb_cow_head.

Sure, and in both places.

Thanks,
sln