| Message ID | 1485889401-13909-6-git-send-email-isubramanian@apm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 01/31/2017 11:03 AM, Iyappan Subramanian wrote: > This patch adds, > - Transmit > - Transmit completion poll > - Receive poll > - NAPI handler > > and enables the driver. > > Signed-off-by: Iyappan Subramanian <isubramanian@apm.com> > Signed-off-by: Keyur Chudgar <kchudgar@apm.com> > --- > + > + tx_ring = pdata->tx_ring; > + tail = tx_ring->tail; > + len = skb_headlen(skb); > + raw_desc = &tx_ring->raw_desc[tail]; > + > + /* Tx descriptor not available */ > + if (!GET_BITS(E, le64_to_cpu(raw_desc->m0)) || > + GET_BITS(PKT_SIZE, le64_to_cpu(raw_desc->m0))) > + return NETDEV_TX_BUSY; > + > + /* Packet buffers should be 64B aligned */ > + pkt_buf = dma_alloc_coherent(dev, XGENE_ENET_STD_MTU, &dma_addr, > + GFP_ATOMIC); > + if (unlikely(!pkt_buf)) > + goto out; Can't you obtain a DMA-API mapping for skb->data and pass it down to the hardware? This copy here is inefficient. > + > + memcpy(pkt_buf, skb->data, len); > + > + addr_hi = GET_BITS(NEXT_DESC_ADDRH, le64_to_cpu(raw_desc->m1)); > + addr_lo = GET_BITS(NEXT_DESC_ADDRL, le64_to_cpu(raw_desc->m1)); > + raw_desc->m1 = cpu_to_le64(SET_BITS(NEXT_DESC_ADDRL, addr_lo) | > + SET_BITS(NEXT_DESC_ADDRH, addr_hi) | > + SET_BITS(PKT_ADDRH, > + dma_addr >> PKT_ADDRL_LEN)); > + > + dma_wmb(); > + > + raw_desc->m0 = cpu_to_le64(SET_BITS(PKT_ADDRL, dma_addr) | > + SET_BITS(PKT_SIZE, len) | > + SET_BITS(E, 0)); > + > + skb_tx_timestamp(skb); > + xge_wr_csr(pdata, DMATXCTRL, 1); > + > + pdata->stats.tx_packets++; > + pdata->stats.tx_bytes += skb->len; This is both racy and incorrect. Racy because after you wrote DMATXCTRL, your TX completion can run, and it can do that while interrupting your CPU presumably, and free the SKB, therefore making you access a freed SKB (or it should, if it does not), it's also incorrect, because before you get signaled a TX completion, there is no guarantee that the packets did actually make it through, you must update your stats in the TX completion handler. > + > + tx_ring->skbs[tail] = skb; > + tx_ring->pkt_bufs[tail] = pkt_buf; > + tx_ring->tail = (tail + 1) & (XGENE_ENET_NUM_DESC - 1); > + > +out: > + dev_kfree_skb_any(skb); Don't do this, remember a pointer to the SKB, free the SKB in TX completion handler, preferably in NAPI context. > + > + return NETDEV_TX_OK; > +} > + > +static void xge_txc_poll(struct net_device *ndev, unsigned int budget) > +{ > + struct xge_pdata *pdata = netdev_priv(ndev); > + struct device *dev = &pdata->pdev->dev; > + struct xge_desc_ring *tx_ring; > + struct xge_raw_desc *raw_desc; > + u64 addr_lo, addr_hi; > + dma_addr_t dma_addr; > + void *pkt_buf; > + bool pktsent; > + u32 data; > + u8 head; > + int i; > + > + tx_ring = pdata->tx_ring; > + head = tx_ring->head; > + > + data = xge_rd_csr(pdata, DMATXSTATUS); > + pktsent = data & TX_PKT_SENT; > + if (unlikely(!pktsent)) > + return; > + > + for (i = 0; i < budget; i++) { TX completion handlers should run unbound and free the entire TX ring, don't make it obey to an upper bound. > + raw_desc = &tx_ring->raw_desc[head]; > + > + if (!GET_BITS(E, le64_to_cpu(raw_desc->m0))) > + break; > + > + dma_rmb(); > + > + addr_hi = GET_BITS(PKT_ADDRH, le64_to_cpu(raw_desc->m1)); > + addr_lo = GET_BITS(PKT_ADDRL, le64_to_cpu(raw_desc->m0)); > + dma_addr = (addr_hi << PKT_ADDRL_LEN) | addr_lo; > + > + pkt_buf = tx_ring->pkt_bufs[head]; > + > + /* clear pktstart address and pktsize */ > + raw_desc->m0 = cpu_to_le64(SET_BITS(E, 1) | > + SET_BITS(PKT_SIZE, 0)); > + xge_wr_csr(pdata, DMATXSTATUS, 1); > + > + dma_free_coherent(dev, XGENE_ENET_STD_MTU, pkt_buf, dma_addr); > + > + head = (head + 1) & (XGENE_ENET_NUM_DESC - 1); > + } > + > + tx_ring->head = head; > +} > + > +static int xge_rx_poll(struct net_device *ndev, unsigned int budget) > +{ > + struct xge_pdata *pdata = netdev_priv(ndev); > + struct device *dev = &pdata->pdev->dev; > + dma_addr_t addr_hi, addr_lo, dma_addr; > + struct xge_desc_ring *rx_ring; > + struct xge_raw_desc *raw_desc; > + struct sk_buff *skb; > + int i, npkts, ret = 0; > + bool pktrcvd; > + u32 data; > + u8 head; > + u16 len; > + > + rx_ring = pdata->rx_ring; > + head = rx_ring->head; > + > + data = xge_rd_csr(pdata, DMARXSTATUS); > + pktrcvd = data & RXSTATUS_RXPKTRCVD; > + > + if (unlikely(!pktrcvd)) > + return 0; > + > + npkts = 0; > + for (i = 0; i < budget; i++) { So pktrcvd is not an indication of the produced number of packets, just that there are packets, that's not very convenient, and it's redundant with the very fact of being interrupted. > + raw_desc = &rx_ring->raw_desc[head]; > + > + if (GET_BITS(E, le64_to_cpu(raw_desc->m0))) > + break; > + > + dma_rmb(); > + > + addr_hi = GET_BITS(PKT_ADDRH, le64_to_cpu(raw_desc->m1)); > + addr_lo = GET_BITS(PKT_ADDRL, le64_to_cpu(raw_desc->m0)); > + dma_addr = (addr_hi << PKT_ADDRL_LEN) | addr_lo; > + len = GET_BITS(PKT_SIZE, le64_to_cpu(raw_desc->m0)); Is not there some kind of additional status that would indicate if the packet is possibly invalid (oversize, undersize, etc.)? > + > + dma_unmap_single(dev, dma_addr, XGENE_ENET_STD_MTU, > + DMA_FROM_DEVICE); > + > + skb = rx_ring->skbs[head]; > + skb_put(skb, len); > + > + skb->protocol = eth_type_trans(skb, ndev); > + > + pdata->stats.rx_packets++; > + pdata->stats.rx_bytes += len; > + napi_gro_receive(&pdata->napi, skb); > + npkts++;
Hi Iyappan,
[auto build test WARNING on net-next/master]
url: https://github.com/0day-ci/linux/commits/Iyappan-Subramanian/drivers-net-xgene-v2-Add-RGMII-based-1G-driver/20170201-034317
config: parisc-allyesconfig (attached as .config)
compiler: hppa-linux-gnu-gcc (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=parisc
All warnings (new ones prefixed by >>):
In file included from include/linux/swab.h:4:0,
from include/uapi/linux/byteorder/big_endian.h:12,
from include/linux/byteorder/big_endian.h:4,
from arch/parisc/include/uapi/asm/byteorder.h:4,
from arch/parisc/include/asm/bitops.h:10,
from include/linux/bitops.h:36,
from include/linux/kernel.h:10,
from include/linux/list.h:8,
from include/linux/resource_ext.h:17,
from include/linux/acpi.h:26,
from drivers/net/ethernet/apm/xgene-v2/main.h:25,
from drivers/net/ethernet/apm/xgene-v2/main.c:22:
drivers/net/ethernet/apm/xgene-v2/main.c: In function 'xge_refill_buffers':
drivers/net/ethernet/apm/xgene-v2/main.c:162:20: warning: right shift count >= width of type [-Wshift-count-overflow]
dma_addr >> PKT_ADDRL_LEN));
^
include/uapi/linux/swab.h:129:32: note: in definition of macro '__swab64'
(__builtin_constant_p((__u64)(x)) ? \
^
>> include/linux/byteorder/generic.h:85:21: note: in expansion of macro '__cpu_to_le64'
#define cpu_to_le64 __cpu_to_le64
^~~~~~~~~~~~~
drivers/net/ethernet/apm/xgene-v2/main.c:161:9: note: in expansion of macro 'SET_BITS'
SET_BITS(PKT_ADDRH,
^~~~~~~~
drivers/net/ethernet/apm/xgene-v2/main.c:162:20: warning: right shift count >= width of type [-Wshift-count-overflow]
dma_addr >> PKT_ADDRL_LEN));
^
include/uapi/linux/swab.h:131:12: note: in definition of macro '__swab64'
__fswab64(x))
^
>> include/linux/byteorder/generic.h:85:21: note: in expansion of macro '__cpu_to_le64'
#define cpu_to_le64 __cpu_to_le64
^~~~~~~~~~~~~
drivers/net/ethernet/apm/xgene-v2/main.c:161:9: note: in expansion of macro 'SET_BITS'
SET_BITS(PKT_ADDRH,
^~~~~~~~
drivers/net/ethernet/apm/xgene-v2/main.c: In function 'xge_start_xmit':
drivers/net/ethernet/apm/xgene-v2/main.c:346:19: warning: right shift count >= width of type [-Wshift-count-overflow]
dma_addr >> PKT_ADDRL_LEN));
^
include/uapi/linux/swab.h:129:32: note: in definition of macro '__swab64'
(__builtin_constant_p((__u64)(x)) ? \
^
>> include/linux/byteorder/generic.h:85:21: note: in expansion of macro '__cpu_to_le64'
#define cpu_to_le64 __cpu_to_le64
^~~~~~~~~~~~~
drivers/net/ethernet/apm/xgene-v2/main.c:345:8: note: in expansion of macro 'SET_BITS'
SET_BITS(PKT_ADDRH,
^~~~~~~~
drivers/net/ethernet/apm/xgene-v2/main.c:346:19: warning: right shift count >= width of type [-Wshift-count-overflow]
dma_addr >> PKT_ADDRL_LEN));
^
include/uapi/linux/swab.h:131:12: note: in definition of macro '__swab64'
__fswab64(x))
^
>> include/linux/byteorder/generic.h:85:21: note: in expansion of macro '__cpu_to_le64'
#define cpu_to_le64 __cpu_to_le64
^~~~~~~~~~~~~
drivers/net/ethernet/apm/xgene-v2/main.c:345:8: note: in expansion of macro 'SET_BITS'
SET_BITS(PKT_ADDRH,
^~~~~~~~
drivers/net/ethernet/apm/xgene-v2/main.c: In function 'xge_rx_poll':
drivers/net/ethernet/apm/xgene-v2/main.c:453:23: warning: left shift count >= width of type [-Wshift-count-overflow]
dma_addr = (addr_hi << PKT_ADDRL_LEN) | addr_lo;
^~
vim +/__cpu_to_le64 +85 include/linux/byteorder/generic.h
^1da177e Linus Torvalds 2005-04-16 69 * cpu_to_[bl]eXX(__uXX x)
^1da177e Linus Torvalds 2005-04-16 70 * [bl]eXX_to_cpu(__uXX x)
^1da177e Linus Torvalds 2005-04-16 71 *
^1da177e Linus Torvalds 2005-04-16 72 * The same, but takes a pointer to the value to convert
^1da177e Linus Torvalds 2005-04-16 73 * cpu_to_[bl]eXXp(__uXX x)
^1da177e Linus Torvalds 2005-04-16 74 * [bl]eXX_to_cpup(__uXX x)
^1da177e Linus Torvalds 2005-04-16 75 *
^1da177e Linus Torvalds 2005-04-16 76 * The same, but change in situ
^1da177e Linus Torvalds 2005-04-16 77 * cpu_to_[bl]eXXs(__uXX x)
^1da177e Linus Torvalds 2005-04-16 78 * [bl]eXX_to_cpus(__uXX x)
^1da177e Linus Torvalds 2005-04-16 79 *
^1da177e Linus Torvalds 2005-04-16 80 * See asm-foo/byteorder.h for examples of how to provide
^1da177e Linus Torvalds 2005-04-16 81 * architecture-optimized versions
^1da177e Linus Torvalds 2005-04-16 82 *
^1da177e Linus Torvalds 2005-04-16 83 */
^1da177e Linus Torvalds 2005-04-16 84
^1da177e Linus Torvalds 2005-04-16 @85 #define cpu_to_le64 __cpu_to_le64
^1da177e Linus Torvalds 2005-04-16 86 #define le64_to_cpu __le64_to_cpu
^1da177e Linus Torvalds 2005-04-16 87 #define cpu_to_le32 __cpu_to_le32
^1da177e Linus Torvalds 2005-04-16 88 #define le32_to_cpu __le32_to_cpu
^1da177e Linus Torvalds 2005-04-16 89 #define cpu_to_le16 __cpu_to_le16
^1da177e Linus Torvalds 2005-04-16 90 #define le16_to_cpu __le16_to_cpu
^1da177e Linus Torvalds 2005-04-16 91 #define cpu_to_be64 __cpu_to_be64
^1da177e Linus Torvalds 2005-04-16 92 #define be64_to_cpu __be64_to_cpu
^1da177e Linus Torvalds 2005-04-16 93 #define cpu_to_be32 __cpu_to_be32
:::::: The code at line 85 was first introduced by commit
:::::: 1da177e4c3f41524e886b7f1b8a0c1fc7321cac2 Linux-2.6.12-rc2
:::::: TO: Linus Torvalds <torvalds@ppc970.osdl.org>
:::::: CC: Linus Torvalds <torvalds@ppc970.osdl.org>
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
From Florian Fainelli > Sent: 31 January 2017 20:33 > On 01/31/2017 11:03 AM, Iyappan Subramanian wrote: > > This patch adds, > > - Transmit > > - Transmit completion poll > > - Receive poll > > - NAPI handler > > > > and enables the driver. > > > > Signed-off-by: Iyappan Subramanian <isubramanian@apm.com> > > Signed-off-by: Keyur Chudgar <kchudgar@apm.com> > > --- > > > + > > + tx_ring = pdata->tx_ring; > > + tail = tx_ring->tail; > > + len = skb_headlen(skb); > > + raw_desc = &tx_ring->raw_desc[tail]; > > + > > + /* Tx descriptor not available */ > > + if (!GET_BITS(E, le64_to_cpu(raw_desc->m0)) || > > + GET_BITS(PKT_SIZE, le64_to_cpu(raw_desc->m0))) > > + return NETDEV_TX_BUSY; Aren't you supposed to detect 'ring full' and stop the code giving you packets to transmit. > > + > > + /* Packet buffers should be 64B aligned */ Is that really a requirement of the hardware? Almost all ethernet frames are 4n+2 aligned. > > + pkt_buf = dma_alloc_coherent(dev, XGENE_ENET_STD_MTU, &dma_addr, > > + GFP_ATOMIC); > > + if (unlikely(!pkt_buf)) > > + goto out; > > Can't you obtain a DMA-API mapping for skb->data and pass it down to the > hardware? This copy here is inefficient. > > > + > > + memcpy(pkt_buf, skb->data, len); You really need to verify that the len <= XGENE_ENET_STD_MTU. Isn't this code only transmitting the 'head' of the packet? What about the fragments?? ... David
Hi Florian, On Tue, Jan 31, 2017 at 12:33 PM, Florian Fainelli <f.fainelli@gmail.com> wrote: > On 01/31/2017 11:03 AM, Iyappan Subramanian wrote: >> This patch adds, >> - Transmit >> - Transmit completion poll >> - Receive poll >> - NAPI handler >> >> and enables the driver. >> >> Signed-off-by: Iyappan Subramanian <isubramanian@apm.com> >> Signed-off-by: Keyur Chudgar <kchudgar@apm.com> >> --- > >> + >> + tx_ring = pdata->tx_ring; >> + tail = tx_ring->tail; >> + len = skb_headlen(skb); >> + raw_desc = &tx_ring->raw_desc[tail]; >> + >> + /* Tx descriptor not available */ >> + if (!GET_BITS(E, le64_to_cpu(raw_desc->m0)) || >> + GET_BITS(PKT_SIZE, le64_to_cpu(raw_desc->m0))) >> + return NETDEV_TX_BUSY; >> + >> + /* Packet buffers should be 64B aligned */ >> + pkt_buf = dma_alloc_coherent(dev, XGENE_ENET_STD_MTU, &dma_addr, >> + GFP_ATOMIC); >> + if (unlikely(!pkt_buf)) >> + goto out; > > Can't you obtain a DMA-API mapping for skb->data and pass it down to the > hardware? This copy here is inefficient. This hardware requires 64-byte alignment. > >> + >> + memcpy(pkt_buf, skb->data, len); >> + >> + addr_hi = GET_BITS(NEXT_DESC_ADDRH, le64_to_cpu(raw_desc->m1)); >> + addr_lo = GET_BITS(NEXT_DESC_ADDRL, le64_to_cpu(raw_desc->m1)); >> + raw_desc->m1 = cpu_to_le64(SET_BITS(NEXT_DESC_ADDRL, addr_lo) | >> + SET_BITS(NEXT_DESC_ADDRH, addr_hi) | >> + SET_BITS(PKT_ADDRH, >> + dma_addr >> PKT_ADDRL_LEN)); >> + >> + dma_wmb(); >> + >> + raw_desc->m0 = cpu_to_le64(SET_BITS(PKT_ADDRL, dma_addr) | >> + SET_BITS(PKT_SIZE, len) | >> + SET_BITS(E, 0)); >> + >> + skb_tx_timestamp(skb); >> + xge_wr_csr(pdata, DMATXCTRL, 1); >> + >> + pdata->stats.tx_packets++; >> + pdata->stats.tx_bytes += skb->len; > > This is both racy and incorrect. Racy because after you wrote DMATXCTRL, > your TX completion can run, and it can do that while interrupting your > CPU presumably, and free the SKB, therefore making you access a freed > SKB (or it should, if it does not), it's also incorrect, because before > you get signaled a TX completion, there is no guarantee that the packets > did actually make it through, you must update your stats in the TX > completion handler. Thanks. I'll move the tx stats part to Tx completion. > >> + >> + tx_ring->skbs[tail] = skb; >> + tx_ring->pkt_bufs[tail] = pkt_buf; >> + tx_ring->tail = (tail + 1) & (XGENE_ENET_NUM_DESC - 1); >> + >> +out: >> + dev_kfree_skb_any(skb); > > Don't do this, remember a pointer to the SKB, free the SKB in TX > completion handler, preferably in NAPI context. I'll implement this. > >> + >> + return NETDEV_TX_OK; >> +} >> + >> +static void xge_txc_poll(struct net_device *ndev, unsigned int budget) >> +{ >> + struct xge_pdata *pdata = netdev_priv(ndev); >> + struct device *dev = &pdata->pdev->dev; >> + struct xge_desc_ring *tx_ring; >> + struct xge_raw_desc *raw_desc; >> + u64 addr_lo, addr_hi; >> + dma_addr_t dma_addr; >> + void *pkt_buf; >> + bool pktsent; >> + u32 data; >> + u8 head; >> + int i; >> + >> + tx_ring = pdata->tx_ring; >> + head = tx_ring->head; >> + >> + data = xge_rd_csr(pdata, DMATXSTATUS); >> + pktsent = data & TX_PKT_SENT; >> + if (unlikely(!pktsent)) >> + return; >> + >> + for (i = 0; i < budget; i++) { > > TX completion handlers should run unbound and free the entire TX ring, > don't make it obey to an upper bound. I'll do as suggested. > >> + raw_desc = &tx_ring->raw_desc[head]; >> + >> + if (!GET_BITS(E, le64_to_cpu(raw_desc->m0))) >> + break; >> + >> + dma_rmb(); >> + >> + addr_hi = GET_BITS(PKT_ADDRH, le64_to_cpu(raw_desc->m1)); >> + addr_lo = GET_BITS(PKT_ADDRL, le64_to_cpu(raw_desc->m0)); >> + dma_addr = (addr_hi << PKT_ADDRL_LEN) | addr_lo; >> + >> + pkt_buf = tx_ring->pkt_bufs[head]; >> + >> + /* clear pktstart address and pktsize */ >> + raw_desc->m0 = cpu_to_le64(SET_BITS(E, 1) | >> + SET_BITS(PKT_SIZE, 0)); >> + xge_wr_csr(pdata, DMATXSTATUS, 1); >> + >> + dma_free_coherent(dev, XGENE_ENET_STD_MTU, pkt_buf, dma_addr); >> + >> + head = (head + 1) & (XGENE_ENET_NUM_DESC - 1); >> + } >> + >> + tx_ring->head = head; >> +} >> + >> +static int xge_rx_poll(struct net_device *ndev, unsigned int budget) >> +{ >> + struct xge_pdata *pdata = netdev_priv(ndev); >> + struct device *dev = &pdata->pdev->dev; >> + dma_addr_t addr_hi, addr_lo, dma_addr; >> + struct xge_desc_ring *rx_ring; >> + struct xge_raw_desc *raw_desc; >> + struct sk_buff *skb; >> + int i, npkts, ret = 0; >> + bool pktrcvd; >> + u32 data; >> + u8 head; >> + u16 len; >> + >> + rx_ring = pdata->rx_ring; >> + head = rx_ring->head; >> + >> + data = xge_rd_csr(pdata, DMARXSTATUS); >> + pktrcvd = data & RXSTATUS_RXPKTRCVD; >> + >> + if (unlikely(!pktrcvd)) >> + return 0; >> + >> + npkts = 0; >> + for (i = 0; i < budget; i++) { > > So pktrcvd is not an indication of the produced number of packets, just > that there are packets, that's not very convenient, and it's redundant > with the very fact of being interrupted. Agree, but the interrupt is common for Tx completion and Rx, this check is still required. > >> + raw_desc = &rx_ring->raw_desc[head]; >> + >> + if (GET_BITS(E, le64_to_cpu(raw_desc->m0))) >> + break; >> + >> + dma_rmb(); >> + >> + addr_hi = GET_BITS(PKT_ADDRH, le64_to_cpu(raw_desc->m1)); >> + addr_lo = GET_BITS(PKT_ADDRL, le64_to_cpu(raw_desc->m0)); >> + dma_addr = (addr_hi << PKT_ADDRL_LEN) | addr_lo; >> + len = GET_BITS(PKT_SIZE, le64_to_cpu(raw_desc->m0)); > > Is not there some kind of additional status that would indicate if the > packet is possibly invalid (oversize, undersize, etc.)? I'll add error checking. > >> + >> + dma_unmap_single(dev, dma_addr, XGENE_ENET_STD_MTU, >> + DMA_FROM_DEVICE); >> + >> + skb = rx_ring->skbs[head]; >> + skb_put(skb, len); >> + >> + skb->protocol = eth_type_trans(skb, ndev); >> + >> + pdata->stats.rx_packets++; >> + pdata->stats.rx_bytes += len; >> + napi_gro_receive(&pdata->napi, skb); >> + npkts++; > > -- > Florian
On Wed, Feb 1, 2017 at 3:09 AM, David Laight <David.Laight@aculab.com> wrote: > From Florian Fainelli >> Sent: 31 January 2017 20:33 >> On 01/31/2017 11:03 AM, Iyappan Subramanian wrote: >> > This patch adds, >> > - Transmit >> > - Transmit completion poll >> > - Receive poll >> > - NAPI handler >> > >> > and enables the driver. >> > >> > Signed-off-by: Iyappan Subramanian <isubramanian@apm.com> >> > Signed-off-by: Keyur Chudgar <kchudgar@apm.com> >> > --- >> >> > + >> > + tx_ring = pdata->tx_ring; >> > + tail = tx_ring->tail; >> > + len = skb_headlen(skb); >> > + raw_desc = &tx_ring->raw_desc[tail]; >> > + >> > + /* Tx descriptor not available */ >> > + if (!GET_BITS(E, le64_to_cpu(raw_desc->m0)) || >> > + GET_BITS(PKT_SIZE, le64_to_cpu(raw_desc->m0))) >> > + return NETDEV_TX_BUSY; > > Aren't you supposed to detect 'ring full' and stop the code > giving you packets to transmit. I'll add stop queue and wake queue. > >> > + >> > + /* Packet buffers should be 64B aligned */ > > Is that really a requirement of the hardware? > Almost all ethernet frames are 4n+2 aligned. Yes, it's a hardware requirement. > >> > + pkt_buf = dma_alloc_coherent(dev, XGENE_ENET_STD_MTU, &dma_addr, >> > + GFP_ATOMIC); >> > + if (unlikely(!pkt_buf)) >> > + goto out; >> >> Can't you obtain a DMA-API mapping for skb->data and pass it down to the >> hardware? This copy here is inefficient. >> >> > + >> > + memcpy(pkt_buf, skb->data, len); > > You really need to verify that the len <= XGENE_ENET_STD_MTU. This version of the driver, doesn't support jumbo frame. So, the check is not required. > > Isn't this code only transmitting the 'head' of the packet? > What about the fragments?? This driver doesn't enable SG yet. > ... > David >
diff --git a/drivers/net/ethernet/apm/Kconfig b/drivers/net/ethernet/apm/Kconfig index ec63d70..59efe5b 100644 --- a/drivers/net/ethernet/apm/Kconfig +++ b/drivers/net/ethernet/apm/Kconfig @@ -1 +1,2 @@ source "drivers/net/ethernet/apm/xgene/Kconfig" +source "drivers/net/ethernet/apm/xgene-v2/Kconfig" diff --git a/drivers/net/ethernet/apm/Makefile b/drivers/net/ethernet/apm/Makefile index 65ce32a..946b2a4 100644 --- a/drivers/net/ethernet/apm/Makefile +++ b/drivers/net/ethernet/apm/Makefile @@ -3,3 +3,4 @@ # obj-$(CONFIG_NET_XGENE) += xgene/ +obj-$(CONFIG_NET_XGENE_V2) += xgene-v2/ diff --git a/drivers/net/ethernet/apm/xgene-v2/Kconfig b/drivers/net/ethernet/apm/xgene-v2/Kconfig new file mode 100644 index 0000000..1205861 --- /dev/null +++ b/drivers/net/ethernet/apm/xgene-v2/Kconfig @@ -0,0 +1,11 @@ +config NET_XGENE_V2 + tristate "APM X-Gene SoC Ethernet-v2 Driver" + depends on HAS_DMA + depends on ARCH_XGENE || COMPILE_TEST + help + This is the Ethernet driver for the on-chip ethernet interface + which uses a linked list of DMA descriptor architecture (v2) for + APM X-Gene SoCs. + + To compile this driver as a module, choose M here. This module will + be called xgene-enet-v2. diff --git a/drivers/net/ethernet/apm/xgene-v2/Makefile b/drivers/net/ethernet/apm/xgene-v2/Makefile new file mode 100644 index 0000000..735309c --- /dev/null +++ b/drivers/net/ethernet/apm/xgene-v2/Makefile @@ -0,0 +1,6 @@ +# +# Makefile for APM X-Gene Ethernet v2 driver +# + +xgene-enet-v2-objs := main.o mac.o enet.o ring.o +obj-$(CONFIG_NET_XGENE_V2) += xgene-enet-v2.o diff --git a/drivers/net/ethernet/apm/xgene-v2/main.c b/drivers/net/ethernet/apm/xgene-v2/main.c index 3881f27..fc90298 100644 --- a/drivers/net/ethernet/apm/xgene-v2/main.c +++ b/drivers/net/ethernet/apm/xgene-v2/main.c @@ -164,9 +164,11 @@ static int xge_refill_buffers(struct net_device *ndev, u32 nbuf) dma_wmb(); raw_desc->m0 = cpu_to_le64(SET_BITS(PKT_ADDRL, dma_addr) | SET_BITS(E, 1)); + ring->skbs[tail] = skb; tail = (tail + 1) & slots; } + xge_wr_csr(pdata, DMARXCTRL, 1); ring->tail = tail; @@ -278,13 +280,14 @@ static int xge_open(struct net_device *ndev) struct xge_pdata *pdata = netdev_priv(ndev); int ret; + napi_enable(&pdata->napi); + ret = xge_request_irq(ndev); if (ret) return ret; xge_intr_enable(pdata); - xge_wr_csr(pdata, DMARXCTRL, 1); xge_mac_enable(pdata); netif_start_queue(ndev); @@ -298,11 +301,204 @@ static int xge_close(struct net_device *ndev) netif_stop_queue(ndev); xge_mac_disable(pdata); + xge_intr_disable(pdata); xge_free_irq(ndev); + napi_disable(&pdata->napi); return 0; } +static netdev_tx_t xge_start_xmit(struct sk_buff *skb, struct net_device *ndev) +{ + struct xge_pdata *pdata = netdev_priv(ndev); + struct device *dev = &pdata->pdev->dev; + static dma_addr_t dma_addr; + struct xge_desc_ring *tx_ring; + struct xge_raw_desc *raw_desc; + u64 addr_lo, addr_hi; + void *pkt_buf; + u8 tail; + u16 len; + + tx_ring = pdata->tx_ring; + tail = tx_ring->tail; + len = skb_headlen(skb); + raw_desc = &tx_ring->raw_desc[tail]; + + /* Tx descriptor not available */ + if (!GET_BITS(E, le64_to_cpu(raw_desc->m0)) || + GET_BITS(PKT_SIZE, le64_to_cpu(raw_desc->m0))) + return NETDEV_TX_BUSY; + + /* Packet buffers should be 64B aligned */ + pkt_buf = dma_alloc_coherent(dev, XGENE_ENET_STD_MTU, &dma_addr, + GFP_ATOMIC); + if (unlikely(!pkt_buf)) + goto out; + + memcpy(pkt_buf, skb->data, len); + + addr_hi = GET_BITS(NEXT_DESC_ADDRH, le64_to_cpu(raw_desc->m1)); + addr_lo = GET_BITS(NEXT_DESC_ADDRL, le64_to_cpu(raw_desc->m1)); + raw_desc->m1 = cpu_to_le64(SET_BITS(NEXT_DESC_ADDRL, addr_lo) | + SET_BITS(NEXT_DESC_ADDRH, addr_hi) | + SET_BITS(PKT_ADDRH, + dma_addr >> PKT_ADDRL_LEN)); + + dma_wmb(); + + raw_desc->m0 = cpu_to_le64(SET_BITS(PKT_ADDRL, dma_addr) | + SET_BITS(PKT_SIZE, len) | + SET_BITS(E, 0)); + + skb_tx_timestamp(skb); + xge_wr_csr(pdata, DMATXCTRL, 1); + + pdata->stats.tx_packets++; + pdata->stats.tx_bytes += skb->len; + + tx_ring->skbs[tail] = skb; + tx_ring->pkt_bufs[tail] = pkt_buf; + tx_ring->tail = (tail + 1) & (XGENE_ENET_NUM_DESC - 1); + +out: + dev_kfree_skb_any(skb); + + return NETDEV_TX_OK; +} + +static void xge_txc_poll(struct net_device *ndev, unsigned int budget) +{ + struct xge_pdata *pdata = netdev_priv(ndev); + struct device *dev = &pdata->pdev->dev; + struct xge_desc_ring *tx_ring; + struct xge_raw_desc *raw_desc; + u64 addr_lo, addr_hi; + dma_addr_t dma_addr; + void *pkt_buf; + bool pktsent; + u32 data; + u8 head; + int i; + + tx_ring = pdata->tx_ring; + head = tx_ring->head; + + data = xge_rd_csr(pdata, DMATXSTATUS); + pktsent = data & TX_PKT_SENT; + if (unlikely(!pktsent)) + return; + + for (i = 0; i < budget; i++) { + raw_desc = &tx_ring->raw_desc[head]; + + if (!GET_BITS(E, le64_to_cpu(raw_desc->m0))) + break; + + dma_rmb(); + + addr_hi = GET_BITS(PKT_ADDRH, le64_to_cpu(raw_desc->m1)); + addr_lo = GET_BITS(PKT_ADDRL, le64_to_cpu(raw_desc->m0)); + dma_addr = (addr_hi << PKT_ADDRL_LEN) | addr_lo; + + pkt_buf = tx_ring->pkt_bufs[head]; + + /* clear pktstart address and pktsize */ + raw_desc->m0 = cpu_to_le64(SET_BITS(E, 1) | + SET_BITS(PKT_SIZE, 0)); + xge_wr_csr(pdata, DMATXSTATUS, 1); + + dma_free_coherent(dev, XGENE_ENET_STD_MTU, pkt_buf, dma_addr); + + head = (head + 1) & (XGENE_ENET_NUM_DESC - 1); + } + + tx_ring->head = head; +} + +static int xge_rx_poll(struct net_device *ndev, unsigned int budget) +{ + struct xge_pdata *pdata = netdev_priv(ndev); + struct device *dev = &pdata->pdev->dev; + dma_addr_t addr_hi, addr_lo, dma_addr; + struct xge_desc_ring *rx_ring; + struct xge_raw_desc *raw_desc; + struct sk_buff *skb; + int i, npkts, ret = 0; + bool pktrcvd; + u32 data; + u8 head; + u16 len; + + rx_ring = pdata->rx_ring; + head = rx_ring->head; + + data = xge_rd_csr(pdata, DMARXSTATUS); + pktrcvd = data & RXSTATUS_RXPKTRCVD; + + if (unlikely(!pktrcvd)) + return 0; + + npkts = 0; + for (i = 0; i < budget; i++) { + raw_desc = &rx_ring->raw_desc[head]; + + if (GET_BITS(E, le64_to_cpu(raw_desc->m0))) + break; + + dma_rmb(); + + addr_hi = GET_BITS(PKT_ADDRH, le64_to_cpu(raw_desc->m1)); + addr_lo = GET_BITS(PKT_ADDRL, le64_to_cpu(raw_desc->m0)); + dma_addr = (addr_hi << PKT_ADDRL_LEN) | addr_lo; + len = GET_BITS(PKT_SIZE, le64_to_cpu(raw_desc->m0)); + + dma_unmap_single(dev, dma_addr, XGENE_ENET_STD_MTU, + DMA_FROM_DEVICE); + + skb = rx_ring->skbs[head]; + skb_put(skb, len); + + skb->protocol = eth_type_trans(skb, ndev); + + pdata->stats.rx_packets++; + pdata->stats.rx_bytes += len; + napi_gro_receive(&pdata->napi, skb); + npkts++; + + ret = xge_refill_buffers(ndev, 1); + xge_wr_csr(pdata, DMARXSTATUS, 1); + + if (ret) + break; + + head = (head + 1) & (XGENE_ENET_NUM_DESC - 1); + } + + rx_ring->head = head; + + return npkts; +} + +static int xge_napi(struct napi_struct *napi, const int budget) +{ + struct net_device *ndev = napi->dev; + struct xge_pdata *pdata = netdev_priv(ndev); + int processed; + + pdata = netdev_priv(ndev); + + xge_txc_poll(ndev, budget); + processed = xge_rx_poll(ndev, budget); + + if (processed < budget) { + napi_complete(napi); + xge_intr_enable(pdata); + } + + return processed; +} + static int xge_set_mac_addr(struct net_device *ndev, void *addr) { struct xge_pdata *pdata = netdev_priv(ndev); @@ -345,6 +541,7 @@ static void xge_get_stats64(struct net_device *ndev, static const struct net_device_ops xgene_ndev_ops = { .ndo_open = xge_open, .ndo_stop = xge_close, + .ndo_start_xmit = xge_start_xmit, .ndo_set_mac_address = xge_set_mac_addr, .ndo_tx_timeout = xge_timeout, .ndo_get_stats64 = xge_get_stats64, @@ -388,6 +585,7 @@ static int xge_probe(struct platform_device *pdev) if (ret) goto err; + netif_napi_add(ndev, &pdata->napi, xge_napi, NAPI_POLL_WEIGHT); ret = register_netdev(ndev); if (ret) { netdev_err(ndev, "Failed to register netdev\n");
This patch adds, - Transmit - Transmit completion poll - Receive poll - NAPI handler and enables the driver. Signed-off-by: Iyappan Subramanian <isubramanian@apm.com> Signed-off-by: Keyur Chudgar <kchudgar@apm.com> --- drivers/net/ethernet/apm/Kconfig | 1 + drivers/net/ethernet/apm/Makefile | 1 + drivers/net/ethernet/apm/xgene-v2/Kconfig | 11 ++ drivers/net/ethernet/apm/xgene-v2/Makefile | 6 + drivers/net/ethernet/apm/xgene-v2/main.c | 200 ++++++++++++++++++++++++++++- 5 files changed, 218 insertions(+), 1 deletion(-) create mode 100644 drivers/net/ethernet/apm/xgene-v2/Kconfig create mode 100644 drivers/net/ethernet/apm/xgene-v2/Makefile