Re: [PATCH v4 07/17] octeontx2-pf: Add packet transmission support
From: Jakub Kicinski <kuba@kernel.org>
Date: 2020-01-21 16:54:29
On Tue, 21 Jan 2020 18:51:41 +0530, sunil.kovvuri@gmail.com wrote:
From: Sunil Goutham <sgoutham@marvell.com> This patch adds the packet transmission support. For a given skb prepares send queue descriptors (SQEs) and pushes them to HW. Here driver doesn't maintain it's own SQ rings, SQEs are pushed to HW using a silicon specific operations called LMTST. From the instuction HW derives the transmit queue number and queues the SQE to that queue. These LMTST instructions are designed to avoid queue maintenance in SW and lockless behavior ie when multiple cores are trying to add SQEs to same queue then HW will takecare of serialization, no need for SW to hold locks. Also supports scatter/gather. Co-developed-by: Geetha sowjanya <gakula@marvell.com> Signed-off-by: Geetha sowjanya <gakula@marvell.com> Signed-off-by: Sunil Goutham <sgoutham@marvell.com>
+static netdev_tx_t otx2_xmit(struct sk_buff *skb, struct net_device *netdev) +
Spurious new line
+{
+ struct otx2_nic *pf = netdev_priv(netdev);
+ int qidx = skb_get_queue_mapping(skb);
+ struct otx2_snd_queue *sq;
+ struct netdev_queue *txq;
+
+ /* Check for minimum and maximum packet length */You only check for min
+ if (skb->len <= ETH_HLEN) {
+ dev_kfree_skb(skb);
+ return NETDEV_TX_OK;
+ }
+
+ sq = &pf->qset.sq[qidx];
+ txq = netdev_get_tx_queue(netdev, qidx);
+
+ if (netif_tx_queue_stopped(txq)) {
+ dev_kfree_skb(skb);This should never happen.
+ } else if (!otx2_sq_append_skb(netdev, sq, skb, qidx)) {
+ netif_tx_stop_queue(txq);
+
+ /* Check again, incase SQBs got freed up */
+ smp_mb();
+ if (((sq->num_sqbs - *sq->aura_fc_addr) * sq->sqe_per_sqb)
+ > sq->sqe_thresh)
+ netif_tx_wake_queue(txq);
+
+ return NETDEV_TX_BUSY;
+ }
+
+ return NETDEV_TX_OK;
+}+/* NIX send memory subdescriptor structure */
+struct nix_sqe_mem_s {
+#if defined(__BIG_ENDIAN_BITFIELD) /* W0 */
+ u64 subdc : 4;
+ u64 alg : 4;
+ u64 dsz : 2;
+ u64 wmem : 1;
+ u64 rsvd_52_16 : 37;
+ u64 offset : 16;
+#else
+ u64 offset : 16;
+ u64 rsvd_52_16 : 37;
+ u64 wmem : 1;
+ u64 dsz : 2;
+ u64 alg : 4;
+ u64 subdc : 4;
+#endifTraditionally we prefer to extract the bitfields with masks and shifts manually in the kernel, rather than having those (subjectively) ugly and finicky bitfield structs. But I guess if nobody else complains this can stay :/
+ u64 addr;
Why do you care about big endian bitfields tho, if you don't care about endianness of the data itself?
quoted hunk ↗ jump to hunk
+}; + #endif /* OTX2_STRUCT_H */diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_txrx.c b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_txrx.c index e6be18d..f416603 100644 --- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_txrx.c +++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_txrx.c@@ -32,6 +32,78 @@ static struct nix_cqe_hdr_s *otx2_get_next_cqe(struct otx2_cq_queue *cq) return cqe_hdr; } +static unsigned int frag_num(unsigned int i) +{ +#ifdef __BIG_ENDIAN + return (i & ~3) + 3 - (i & 3); +#else + return i; +#endif +} + +static dma_addr_t otx2_dma_map_skb_frag(struct otx2_nic *pfvf, + struct sk_buff *skb, int seg, int *len) +{ + const skb_frag_t *frag; + struct page *page; + int offset; + + /* First segment is always skb->data */ + if (!seg) { + page = virt_to_page(skb->data); + offset = offset_in_page(skb->data); + *len = skb_headlen(skb); + } else { + frag = &skb_shinfo(skb)->frags[seg - 1]; + page = skb_frag_page(frag); + offset = skb_frag_off(frag); + *len = skb_frag_size(frag); + } + return otx2_dma_map_page(pfvf, page, offset, *len, DMA_TO_DEVICE); +} + +static void otx2_dma_unmap_skb_frags(struct otx2_nic *pfvf, struct sg_list *sg) +{ + int seg; + + for (seg = 0; seg < sg->num_segs; seg++) { + otx2_dma_unmap_page(pfvf, sg->dma_addr[seg], + sg->size[seg], DMA_TO_DEVICE); + }
no need for parenthesis
+ sg->num_segs = 0;
+}
+
+static void otx2_snd_pkt_handler(struct otx2_nic *pfvf,
+ struct otx2_cq_queue *cq,
+ struct otx2_snd_queue *sq,
+ struct nix_cqe_tx_s *cqe,
+ int budget, int *tx_pkts, int *tx_bytes)
+{
+ struct nix_send_comp_s *snd_comp = &cqe->comp;
+ struct sk_buff *skb = NULL;
+ struct sg_list *sg;
+
+ if (unlikely(snd_comp->status)) {
+ netdev_info(pfvf->netdev,
+ "TX%d: Error in send CQ status:%x\n",
+ cq->cint_idx, snd_comp->status);This should probably be ratelimitted
+ }
unnecessary parenthesis
+ + /* Barrier, so that update to sq by other cpus is visible */ + smp_mb();
Could you please rephrase this comment to explain between what and what this barrier is? :S
+ sg = &sq->sg[snd_comp->sqe_id]; + + skb = (struct sk_buff *)sg->skb; + if (unlikely(!skb)) + return; + + *tx_bytes += skb->len; + (*tx_pkts)++; + otx2_dma_unmap_skb_frags(pfvf, sg); + napi_consume_skb(skb, budget); + sg->skb = (u64)NULL; +} +
quoted hunk ↗ jump to hunk
@@ -225,6 +331,169 @@ int otx2_napi_handler(struct napi_struct *napi, int budget) return workdone; } +static void otx2_sqe_flush(struct otx2_snd_queue *sq, int size) +{ + u64 status; + + /* Packet data stores should finish before SQE is flushed to HW */
Packet data is synced by the dma operations the barrier shouldn't be needed AFAIK (and if it would be, dma_wmb() would not be the one, as it only works for iomem AFAIU).
+ dma_wmb();
+
+ do {
+ memcpy(sq->lmt_addr, sq->sqe_base, size);
+ status = otx2_lmt_flush(sq->io_addr);
+ } while (status == 0);
+
+ sq->head++;
+ sq->head &= (sq->sqe_cnt - 1);
+}