| Message ID | 1576616549-39097-4-git-send-email-opendmb@gmail.com |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | net: bcmgenet: Turn on offloads by default | expand |
From: Doug Berger <opendmb@gmail.com> Date: Tue, 17 Dec 2019 13:02:24 -0800 > This commit updates the Rx checksum offload behavior of the driver > to use the more generic CHECKSUM_COMPLETE method that supports all > protocols over the CHECKSUM_UNNECESSARY method that only applies > to some protocols known by the hardware. > > This behavior is perceived to be superior. > > Signed-off-by: Doug Berger <opendmb@gmail.com> This has to be done in the same patch that you change to use the NETIF_F_HW_CSUM feature flag.
On 12/17/19 1:16 PM, David Miller wrote: > From: Doug Berger <opendmb@gmail.com> > Date: Tue, 17 Dec 2019 13:02:24 -0800 > >> This commit updates the Rx checksum offload behavior of the driver >> to use the more generic CHECKSUM_COMPLETE method that supports all >> protocols over the CHECKSUM_UNNECESSARY method that only applies >> to some protocols known by the hardware. >> >> This behavior is perceived to be superior. >> >> Signed-off-by: Doug Berger <opendmb@gmail.com> > > This has to be done in the same patch that you change to use > the NETIF_F_HW_CSUM feature flag. Even if we were already advertising support for NETIF_F_RXCSUM before patch #2? Not questioning your comment, just trying to understand why this is deemed necessary here since it does not affect the same "direction".
On 12/17/19 1:02 PM, Doug Berger wrote: > This commit updates the Rx checksum offload behavior of the driver > to use the more generic CHECKSUM_COMPLETE method that supports all > protocols over the CHECKSUM_UNNECESSARY method that only applies > to some protocols known by the hardware. > > This behavior is perceived to be superior. > > Signed-off-by: Doug Berger <opendmb@gmail.com> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com> You could also remove priv->dma_rx_chk_bit which is now write only after this patch and not used anymore.
On Tue, 17 Dec 2019 13:02:24 -0800, Doug Berger wrote: > @@ -1793,6 +1792,10 @@ static unsigned int bcmgenet_desc_rx(struct bcmgenet_rx_ring *ring, > > status = (struct status_64 *)skb->data; > dma_length_status = status->length_status; > + if (priv->desc_rxchk_en) { > + skb->csum = ntohs(status->rx_csum & 0xffff); This adds a new warning for a W=1 C=1 build: ../drivers/net/ethernet/broadcom/genet/bcmgenet.c:1796:45: warning: cast to res\ tricted __be16 ../drivers/net/ethernet/broadcom/genet/bcmgenet.c:1796:45: warning: cast to res\ tricted __be16 ../drivers/net/ethernet/broadcom/genet/bcmgenet.c:1796:45: warning: cast to res\ tricted __be16 ../drivers/net/ethernet/broadcom/genet/bcmgenet.c:1796:45: warning: cast to res\ tricted __be16 ../drivers/net/ethernet/broadcom/genet/bcmgenet.c:1796:43: warning: incorrect t\ ype in assignment (different base types) ../drivers/net/ethernet/broadcom/genet/bcmgenet.c:1796:43: expected restrict\ ed __wsum [usertype] csum ../drivers/net/ethernet/broadcom/genet/bcmgenet.c:1796:43: got int Could you perhaps consider a __force cast or such? > + skb->ip_summed = CHECKSUM_COMPLETE; > + } > } > > /* DMA flags and length are still valid no matter how
From: Florian Fainelli <f.fainelli@gmail.com> Date: Tue, 17 Dec 2019 14:52:07 -0800 > On 12/17/19 1:16 PM, David Miller wrote: >> From: Doug Berger <opendmb@gmail.com> >> Date: Tue, 17 Dec 2019 13:02:24 -0800 >> >>> This commit updates the Rx checksum offload behavior of the driver >>> to use the more generic CHECKSUM_COMPLETE method that supports all >>> protocols over the CHECKSUM_UNNECESSARY method that only applies >>> to some protocols known by the hardware. >>> >>> This behavior is perceived to be superior. >>> >>> Signed-off-by: Doug Berger <opendmb@gmail.com> >> >> This has to be done in the same patch that you change to use >> the NETIF_F_HW_CSUM feature flag. > > Even if we were already advertising support for NETIF_F_RXCSUM before > patch #2? Not questioning your comment, just trying to understand why > this is deemed necessary here since it does not affect the same "direction". My bad... I misunderstood the situation. Ignore me :-)
On 12/17/19 2:52 PM, Florian Fainelli wrote: > On 12/17/19 1:02 PM, Doug Berger wrote: >> This commit updates the Rx checksum offload behavior of the driver >> to use the more generic CHECKSUM_COMPLETE method that supports all >> protocols over the CHECKSUM_UNNECESSARY method that only applies >> to some protocols known by the hardware. >> >> This behavior is perceived to be superior. >> >> Signed-off-by: Doug Berger <opendmb@gmail.com> > > Reviewed-by: Florian Fainelli <f.fainelli@gmail.com> > > You could also remove priv->dma_rx_chk_bit which is now write only after > this patch and not used anymore. > Good idea. I'll resubmit shortly. -Doug
Hi Doug,
I love your patch! Perhaps something to improve:
[auto build test WARNING on net-next/master]
[also build test WARNING on net/master v5.5-rc3 next-20191220]
[cannot apply to sparc-next/master]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
url: https://github.com/0day-ci/linux/commits/Doug-Berger/net-bcmgenet-Turn-on-offloads-by-default/20191221-050220
base: https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git d8e419da048e45a8258b9721e85232dbb25ac618
reproduce:
# apt-get install sparse
# sparse version: v0.6.1-129-g341daf20-dirty
make ARCH=x86_64 allmodconfig
make C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__'
If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>
sparse warnings: (new ones prefixed by >>)
>> drivers/net/ethernet/broadcom/genet/bcmgenet.c:1796:45: sparse: sparse: cast to restricted __be16
>> drivers/net/ethernet/broadcom/genet/bcmgenet.c:1796:45: sparse: sparse: cast to restricted __be16
>> drivers/net/ethernet/broadcom/genet/bcmgenet.c:1796:45: sparse: sparse: cast to restricted __be16
>> drivers/net/ethernet/broadcom/genet/bcmgenet.c:1796:45: sparse: sparse: cast to restricted __be16
drivers/net/ethernet/broadcom/genet/bcmgenet.c:1796:43: sparse: sparse: incorrect type in assignment (different base types)
>> drivers/net/ethernet/broadcom/genet/bcmgenet.c:1796:43: sparse: expected restricted __wsum [usertype] csum
>> drivers/net/ethernet/broadcom/genet/bcmgenet.c:1796:43: sparse: got int
include/linux/netdevice.h:3931:20: sparse: sparse: shift count is negative (-1)
vim +1796 drivers/net/ethernet/broadcom/genet/bcmgenet.c
1724
1725 /* bcmgenet_desc_rx - descriptor based rx process.
1726 * this could be called from bottom half, or from NAPI polling method.
1727 */
1728 static unsigned int bcmgenet_desc_rx(struct bcmgenet_rx_ring *ring,
1729 unsigned int budget)
1730 {
1731 struct bcmgenet_priv *priv = ring->priv;
1732 struct net_device *dev = priv->dev;
1733 struct enet_cb *cb;
1734 struct sk_buff *skb;
1735 u32 dma_length_status;
1736 unsigned long dma_flag;
1737 int len;
1738 unsigned int rxpktprocessed = 0, rxpkttoprocess;
1739 unsigned int bytes_processed = 0;
1740 unsigned int p_index, mask;
1741 unsigned int discards;
1742
1743 /* Clear status before servicing to reduce spurious interrupts */
1744 if (ring->index == DESC_INDEX) {
1745 bcmgenet_intrl2_0_writel(priv, UMAC_IRQ_RXDMA_DONE,
1746 INTRL2_CPU_CLEAR);
1747 } else {
1748 mask = 1 << (UMAC_IRQ1_RX_INTR_SHIFT + ring->index);
1749 bcmgenet_intrl2_1_writel(priv,
1750 mask,
1751 INTRL2_CPU_CLEAR);
1752 }
1753
1754 p_index = bcmgenet_rdma_ring_readl(priv, ring->index, RDMA_PROD_INDEX);
1755
1756 discards = (p_index >> DMA_P_INDEX_DISCARD_CNT_SHIFT) &
1757 DMA_P_INDEX_DISCARD_CNT_MASK;
1758 if (discards > ring->old_discards) {
1759 discards = discards - ring->old_discards;
1760 ring->errors += discards;
1761 ring->old_discards += discards;
1762
1763 /* Clear HW register when we reach 75% of maximum 0xFFFF */
1764 if (ring->old_discards >= 0xC000) {
1765 ring->old_discards = 0;
1766 bcmgenet_rdma_ring_writel(priv, ring->index, 0,
1767 RDMA_PROD_INDEX);
1768 }
1769 }
1770
1771 p_index &= DMA_P_INDEX_MASK;
1772 rxpkttoprocess = (p_index - ring->c_index) & DMA_C_INDEX_MASK;
1773
1774 netif_dbg(priv, rx_status, dev,
1775 "RDMA: rxpkttoprocess=%d\n", rxpkttoprocess);
1776
1777 while ((rxpktprocessed < rxpkttoprocess) &&
1778 (rxpktprocessed < budget)) {
1779 cb = &priv->rx_cbs[ring->read_ptr];
1780 skb = bcmgenet_rx_refill(priv, cb);
1781
1782 if (unlikely(!skb)) {
1783 ring->dropped++;
1784 goto next;
1785 }
1786
1787 if (!priv->desc_64b_en) {
1788 dma_length_status =
1789 dmadesc_get_length_status(priv, cb->bd_addr);
1790 } else {
1791 struct status_64 *status;
1792
1793 status = (struct status_64 *)skb->data;
1794 dma_length_status = status->length_status;
1795 if (priv->desc_rxchk_en) {
> 1796 skb->csum = ntohs(status->rx_csum & 0xffff);
1797 skb->ip_summed = CHECKSUM_COMPLETE;
1798 }
1799 }
1800
1801 /* DMA flags and length are still valid no matter how
1802 * we got the Receive Status Vector (64B RSB or register)
1803 */
1804 dma_flag = dma_length_status & 0xffff;
1805 len = dma_length_status >> DMA_BUFLENGTH_SHIFT;
1806
1807 netif_dbg(priv, rx_status, dev,
1808 "%s:p_ind=%d c_ind=%d read_ptr=%d len_stat=0x%08x\n",
1809 __func__, p_index, ring->c_index,
1810 ring->read_ptr, dma_length_status);
1811
1812 if (unlikely(!(dma_flag & DMA_EOP) || !(dma_flag & DMA_SOP))) {
1813 netif_err(priv, rx_status, dev,
1814 "dropping fragmented packet!\n");
1815 ring->errors++;
1816 dev_kfree_skb_any(skb);
1817 goto next;
1818 }
1819
1820 /* report errors */
1821 if (unlikely(dma_flag & (DMA_RX_CRC_ERROR |
1822 DMA_RX_OV |
1823 DMA_RX_NO |
1824 DMA_RX_LG |
1825 DMA_RX_RXER))) {
1826 netif_err(priv, rx_status, dev, "dma_flag=0x%x\n",
1827 (unsigned int)dma_flag);
1828 if (dma_flag & DMA_RX_CRC_ERROR)
1829 dev->stats.rx_crc_errors++;
1830 if (dma_flag & DMA_RX_OV)
1831 dev->stats.rx_over_errors++;
1832 if (dma_flag & DMA_RX_NO)
1833 dev->stats.rx_frame_errors++;
1834 if (dma_flag & DMA_RX_LG)
1835 dev->stats.rx_length_errors++;
1836 dev->stats.rx_errors++;
1837 dev_kfree_skb_any(skb);
1838 goto next;
1839 } /* error packet */
1840
1841 skb_put(skb, len);
1842 if (priv->desc_64b_en) {
1843 skb_pull(skb, 64);
1844 len -= 64;
1845 }
1846
1847 /* remove hardware 2bytes added for IP alignment */
1848 skb_pull(skb, 2);
1849 len -= 2;
1850
1851 if (priv->crc_fwd_en) {
1852 skb_trim(skb, len - ETH_FCS_LEN);
1853 len -= ETH_FCS_LEN;
1854 }
1855
1856 bytes_processed += len;
1857
1858 /*Finish setting up the received SKB and send it to the kernel*/
1859 skb->protocol = eth_type_trans(skb, priv->dev);
1860 ring->packets++;
1861 ring->bytes += len;
1862 if (dma_flag & DMA_RX_MULT)
1863 dev->stats.multicast++;
1864
1865 /* Notify kernel */
1866 napi_gro_receive(&ring->napi, skb);
1867 netif_dbg(priv, rx_status, dev, "pushed up to kernel\n");
1868
1869 next:
1870 rxpktprocessed++;
1871 if (likely(ring->read_ptr < ring->end_ptr))
1872 ring->read_ptr++;
1873 else
1874 ring->read_ptr = ring->cb_ptr;
1875
1876 ring->c_index = (ring->c_index + 1) & DMA_C_INDEX_MASK;
1877 bcmgenet_rdma_ring_writel(priv, ring->index, ring->c_index, RDMA_CONS_INDEX);
1878 }
1879
1880 ring->dim.bytes = bytes_processed;
1881 ring->dim.packets = rxpktprocessed;
1882
1883 return rxpktprocessed;
1884 }
1885
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org Intel Corporation
diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c index cd07b3ad1d53..5674dc304032 100644 --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c @@ -521,7 +521,7 @@ static int bcmgenet_set_rx_csum(struct net_device *dev, /* enable rx checksumming */ if (rx_csum_en) - rbuf_chk_ctrl |= RBUF_RXCHK_EN; + rbuf_chk_ctrl |= RBUF_RXCHK_EN | RBUF_L3_PARSE_DIS; else rbuf_chk_ctrl &= ~RBUF_RXCHK_EN; priv->desc_rxchk_en = rx_csum_en; @@ -1739,7 +1739,6 @@ static unsigned int bcmgenet_desc_rx(struct bcmgenet_rx_ring *ring, unsigned int bytes_processed = 0; unsigned int p_index, mask; unsigned int discards; - unsigned int chksum_ok = 0; /* Clear status before servicing to reduce spurious interrupts */ if (ring->index == DESC_INDEX) { @@ -1793,6 +1792,10 @@ static unsigned int bcmgenet_desc_rx(struct bcmgenet_rx_ring *ring, status = (struct status_64 *)skb->data; dma_length_status = status->length_status; + if (priv->desc_rxchk_en) { + skb->csum = ntohs(status->rx_csum & 0xffff); + skb->ip_summed = CHECKSUM_COMPLETE; + } } /* DMA flags and length are still valid no matter how @@ -1835,18 +1838,12 @@ static unsigned int bcmgenet_desc_rx(struct bcmgenet_rx_ring *ring, goto next; } /* error packet */ - chksum_ok = (dma_flag & priv->dma_rx_chk_bit) && - priv->desc_rxchk_en; - skb_put(skb, len); if (priv->desc_64b_en) { skb_pull(skb, 64); len -= 64; } - if (likely(chksum_ok)) - skb->ip_summed = CHECKSUM_UNNECESSARY; - /* remove hardware 2bytes added for IP alignment */ skb_pull(skb, 2); len -= 2; diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.h b/drivers/net/ethernet/broadcom/genet/bcmgenet.h index a5659197598f..11645cccc207 100644 --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.h +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.h @@ -251,6 +251,7 @@ struct bcmgenet_mib_counters { #define RBUF_CHK_CTRL 0x14 #define RBUF_RXCHK_EN (1 << 0) #define RBUF_SKIP_FCS (1 << 4) +#define RBUF_L3_PARSE_DIS (1 << 5) #define RBUF_ENERGY_CTRL 0x9c #define RBUF_EEE_EN (1 << 0)
This commit updates the Rx checksum offload behavior of the driver to use the more generic CHECKSUM_COMPLETE method that supports all protocols over the CHECKSUM_UNNECESSARY method that only applies to some protocols known by the hardware. This behavior is perceived to be superior. Signed-off-by: Doug Berger <opendmb@gmail.com> --- drivers/net/ethernet/broadcom/genet/bcmgenet.c | 13 +++++-------- drivers/net/ethernet/broadcom/genet/bcmgenet.h | 1 + 2 files changed, 6 insertions(+), 8 deletions(-)