On 12/10/2018 10:44 PM, David Miller wrote: >> +#define RAVB_CSUM_LEN 2 >> + > ... >> priv->rx_buf_sz = (ndev->mtu <= 1492 ? PKT_BUF_SZ : ndev->mtu) + >> - ETH_HLEN + VLAN_HLEN; >> + ETH_HLEN + VLAN_HLEN + RAVB_CSUM_LEN; > ... >> + if (unlikely(skb->len < RAVB_CSUM_LEN)) > ... >> - hw_csum = skb_tail_pointer(skb) - 2; >> + hw_csum = skb_tail_pointer(skb) - RAVB_CSUM_LEN; > ... >> - skb_trim(skb, skb->len - 2); >> + skb_trim(skb, skb->len - RAVB_CSUM_LEN); > > Unlike Sergei, I think this macro define should be kept in the fix. > > It is absolutely crucial for anyone reading this code to understand > what this value is all about. > > People reading the code aren't able to go automatically back to a > commit to learn what this value means, and even if they could they > shouldn't have to do so for a bunch of magic '2' constants placed all > over. We already have a comment in ravb_tx_csum(), I only asked for another one (the place where we fix up the packet size). > Even in the most fundamental way, the macro is required to satisfy > the "no magic constants" rule for kernel code. I'm also somewhat opposed to the RAVB_ prefix on something not really h/w or driver specific... but I guess we don't have such #define anywhere in the TCP/IP stack. Well, your call, anyway... MBR. Sergei
