| Message ID | 20210409003904.8957-1-TheSven73@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 3bc41d6d2721d5168a8f7fea34028a5332068f5e |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net,v1] lan743x: fix ethernet frame cutoff issue | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for net |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | success | CCed 6 of 6 maintainers |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 26 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
Hi Sven
> Many thanks to Heiner Kallweit for suggesting this solution.
Adding a Suggested-by: would be good. And it might sometime help
Johnathan Corbet extract some interesting statistics from the commit
messages if everybody uses the same format.
Andrew
Hi Andrew, On Thu, Apr 8, 2021 at 9:02 PM Andrew Lunn <andrew@lunn.ch> wrote: > > Adding a Suggested-by: would be good. And it might sometime help > Johnathan Corbet extract some interesting statistics from the commit > messages if everybody uses the same format. Thank you for the suggestion. I'll definitely use that in the future.
On Thu, Apr 8, 2021 at 7:39 PM Sven Van Asbroeck <thesven73@gmail.com> wrote: > > From: Sven Van Asbroeck <thesven73@gmail.com> > > The ethernet frame length is calculated incorrectly. Depending on > the value of RX_HEAD_PADDING, this may result in ethernet frames > that are too short (cut off at the end), or too long (garbage added > to the end). > > Fix by calculating the ethernet frame length correctly. For added > clarity, use the ETH_FCS_LEN constant in the calculation. > > Many thanks to Heiner Kallweit for suggesting this solution. > > Fixes: 3e21a10fdea3 ("lan743x: trim all 4 bytes of the FCS; not just 2") > Link: https://lore.kernel.org/lkml/20210408172353.21143-1-TheSven73@gmail.com/ > Signed-off-by: Sven Van Asbroeck <thesven73@gmail.com> I'm glad everyone was able to work together to get this fixed properly without any figure pointing or mud slinging! Kudos everyone. Reviewed-by: George McCollister <george.mccollister@gmail.com> Tested-By: George McCollister <george.mccollister@gmail.com> > --- > > Tree: git://git.kernel.org/pub/scm/linux/kernel/git/davem/net.git # 864db232dc70 > > To: Bryan Whitehead <bryan.whitehead@microchip.com> > To: "David S. Miller" <davem@davemloft.net> > To: Jakub Kicinski <kuba@kernel.org> > To: George McCollister <george.mccollister@gmail.com> > Cc: Heiner Kallweit <hkallweit1@gmail.com> > Cc: Andrew Lunn <andrew@lunn.ch> > Cc: UNGLinuxDriver@microchip.com > Cc: netdev@vger.kernel.org > Cc: linux-kernel@vger.kernel.org > > drivers/net/ethernet/microchip/lan743x_main.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/drivers/net/ethernet/microchip/lan743x_main.c b/drivers/net/ethernet/microchip/lan743x_main.c > index 1c3e204d727c..7b6794aa8ea9 100644 > --- a/drivers/net/ethernet/microchip/lan743x_main.c > +++ b/drivers/net/ethernet/microchip/lan743x_main.c > @@ -885,8 +885,8 @@ static int lan743x_mac_set_mtu(struct lan743x_adapter *adapter, int new_mtu) > } > > mac_rx &= ~(MAC_RX_MAX_SIZE_MASK_); > - mac_rx |= (((new_mtu + ETH_HLEN + 4) << MAC_RX_MAX_SIZE_SHIFT_) & > - MAC_RX_MAX_SIZE_MASK_); > + mac_rx |= (((new_mtu + ETH_HLEN + ETH_FCS_LEN) > + << MAC_RX_MAX_SIZE_SHIFT_) & MAC_RX_MAX_SIZE_MASK_); > lan743x_csr_write(adapter, MAC_RX, mac_rx); > > if (enabled) { > @@ -1944,7 +1944,7 @@ static int lan743x_rx_init_ring_element(struct lan743x_rx *rx, int index) > struct sk_buff *skb; > dma_addr_t dma_ptr; > > - buffer_length = netdev->mtu + ETH_HLEN + 4 + RX_HEAD_PADDING; > + buffer_length = netdev->mtu + ETH_HLEN + ETH_FCS_LEN + RX_HEAD_PADDING; > > descriptor = &rx->ring_cpu_ptr[index]; > buffer_info = &rx->buffer_info[index]; > @@ -2040,7 +2040,7 @@ lan743x_rx_trim_skb(struct sk_buff *skb, int frame_length) > dev_kfree_skb_irq(skb); > return NULL; > } > - frame_length = max_t(int, 0, frame_length - RX_HEAD_PADDING - 4); > + frame_length = max_t(int, 0, frame_length - ETH_FCS_LEN); > if (skb->len > frame_length) { > skb->tail -= skb->len - frame_length; > skb->len = frame_length; > -- > 2.17.1 >
Hi George, On Fri, Apr 9, 2021 at 10:12 AM George McCollister <george.mccollister@gmail.com> wrote: > > I'm glad everyone was able to work together to get this fixed properly > without any figure pointing or mud slinging! Kudos everyone. Same, this is what the kernel community is supposed to be all about. And thank you for testing+reviewing. And for not freaking out too much when I lobbed that revert patch in your general direction :-]
Hello: This patch was applied to netdev/net.git (refs/heads/master): On Thu, 8 Apr 2021 20:39:04 -0400 you wrote: > From: Sven Van Asbroeck <thesven73@gmail.com> > > The ethernet frame length is calculated incorrectly. Depending on > the value of RX_HEAD_PADDING, this may result in ethernet frames > that are too short (cut off at the end), or too long (garbage added > to the end). > > [...] Here is the summary with links: - [net,v1] lan743x: fix ethernet frame cutoff issue https://git.kernel.org/netdev/net/c/3bc41d6d2721 You are awesome, thank you! -- Deet-doot-dot, I am a bot. https://korg.docs.kernel.org/patchwork/pwbot.html
On 09.04.21 02:39, Sven Van Asbroeck wrote: > From: Sven Van Asbroeck <thesven73@gmail.com> > > The ethernet frame length is calculated incorrectly. Depending on > the value of RX_HEAD_PADDING, this may result in ethernet frames > that are too short (cut off at the end), or too long (garbage added > to the end). > > Fix by calculating the ethernet frame length correctly. For added > clarity, use the ETH_FCS_LEN constant in the calculation. > > Many thanks to Heiner Kallweit for suggesting this solution. > > Fixes: 3e21a10fdea3 ("lan743x: trim all 4 bytes of the FCS; not just 2") > Link: https://lore.kernel.org/lkml/20210408172353.21143-1-TheSven73@gmail.com/ > Signed-off-by: Sven Van Asbroeck <thesven73@gmail.com> > --- > > Tree: git://git.kernel.org/pub/scm/linux/kernel/git/davem/net.git # 864db232dc70 > > To: Bryan Whitehead <bryan.whitehead@microchip.com> > To: "David S. Miller" <davem@davemloft.net> > To: Jakub Kicinski <kuba@kernel.org> > To: George McCollister <george.mccollister@gmail.com> > Cc: Heiner Kallweit <hkallweit1@gmail.com> > Cc: Andrew Lunn <andrew@lunn.ch> > Cc: UNGLinuxDriver@microchip.com > Cc: netdev@vger.kernel.org > Cc: linux-kernel@vger.kernel.org > > drivers/net/ethernet/microchip/lan743x_main.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/drivers/net/ethernet/microchip/lan743x_main.c b/drivers/net/ethernet/microchip/lan743x_main.c > index 1c3e204d727c..7b6794aa8ea9 100644 > --- a/drivers/net/ethernet/microchip/lan743x_main.c > +++ b/drivers/net/ethernet/microchip/lan743x_main.c > @@ -885,8 +885,8 @@ static int lan743x_mac_set_mtu(struct lan743x_adapter *adapter, int new_mtu) > } > > mac_rx &= ~(MAC_RX_MAX_SIZE_MASK_); > - mac_rx |= (((new_mtu + ETH_HLEN + 4) << MAC_RX_MAX_SIZE_SHIFT_) & > - MAC_RX_MAX_SIZE_MASK_); > + mac_rx |= (((new_mtu + ETH_HLEN + ETH_FCS_LEN) > + << MAC_RX_MAX_SIZE_SHIFT_) & MAC_RX_MAX_SIZE_MASK_); > lan743x_csr_write(adapter, MAC_RX, mac_rx); > > if (enabled) { > @@ -1944,7 +1944,7 @@ static int lan743x_rx_init_ring_element(struct lan743x_rx *rx, int index) > struct sk_buff *skb; > dma_addr_t dma_ptr; > > - buffer_length = netdev->mtu + ETH_HLEN + 4 + RX_HEAD_PADDING; > + buffer_length = netdev->mtu + ETH_HLEN + ETH_FCS_LEN + RX_HEAD_PADDING; > On a cursory glance, using __netdev_alloc_skb_ip_align() here should allow you to get rid of all the RX_HEAD_PADDING gymnastics. And also avoid the need for setting RX_CFG_B_RX_PAD_2_, as the NET_IP_ALIGN part would no longer get dma-mapped. > descriptor = &rx->ring_cpu_ptr[index]; > buffer_info = &rx->buffer_info[index]; > @@ -2040,7 +2040,7 @@ lan743x_rx_trim_skb(struct sk_buff *skb, int frame_length) > dev_kfree_skb_irq(skb); > return NULL; > } > - frame_length = max_t(int, 0, frame_length - RX_HEAD_PADDING - 4); > + frame_length = max_t(int, 0, frame_length - ETH_FCS_LEN); > if (skb->len > frame_length) { > skb->tail -= skb->len - frame_length; > skb->len = frame_length; >
Hi Julian, On Wed, Apr 14, 2021 at 8:53 AM Julian Wiedmann <jwi@linux.ibm.com> wrote: > > On a cursory glance, using __netdev_alloc_skb_ip_align() here should > allow you to get rid of all the RX_HEAD_PADDING gymnastics. > > And also avoid the need for setting RX_CFG_B_RX_PAD_2_, as the > NET_IP_ALIGN part would no longer get dma-mapped. That's an excellent suggestion, and I'll definitely keep that in mind for the future. In this case, I'm not sure if it could work. This NIC has multi-buffer frames. The dma-ed skbs represent frame fragments. A flag in the descriptor ring indicates if an skb is "first". If first, we can reserve the padding. Otherwise, we cannot. because that would corrupt a fragment in the middle. At the time of skb allocation, we do not know whether that skb will be "first". https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/net/ethernet/microchip/lan743x_main.c?h=v5.12-rc7#n2125 Maybe I'm missing a trick here? Feel free to suggest improvements, it's always much appreciated. Sven
On 14.04.21 15:19, Sven Van Asbroeck wrote: > Hi Julian, > > On Wed, Apr 14, 2021 at 8:53 AM Julian Wiedmann <jwi@linux.ibm.com> wrote: >> >> On a cursory glance, using __netdev_alloc_skb_ip_align() here should >> allow you to get rid of all the RX_HEAD_PADDING gymnastics. >> >> And also avoid the need for setting RX_CFG_B_RX_PAD_2_, as the >> NET_IP_ALIGN part would no longer get dma-mapped. > > That's an excellent suggestion, and I'll definitely keep that in mind > for the future. > > In this case, I'm not sure if it could work. This NIC has multi-buffer > frames. The dma-ed skbs represent frame fragments. A flag in the > descriptor ring indicates if an skb is "first". If first, we can > reserve the padding. Otherwise, we cannot. because that would corrupt > a fragment in the middle. At the time of skb allocation, we do not > know whether that skb will be "first". > __netdev_alloc_skb_ip_align() already reserves the NET_IP_ALIGN part. So when the NIC stores into the dma-mapped skb->data parts, each fragment will automatically have the required alignment - even when you only care about the first fragment's alignment. > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/net/ethernet/microchip/lan743x_main.c?h=v5.12-rc7#n2125 > > Maybe I'm missing a trick here? Feel free to suggest improvements, > it's always much appreciated. > > Sven >
Hi Julian, On Wed, Apr 14, 2021 at 9:33 AM Julian Wiedmann <jwi@linux.ibm.com> wrote: > > __netdev_alloc_skb_ip_align() already reserves the NET_IP_ALIGN part. > So when the NIC stores into the dma-mapped skb->data parts, each > fragment will automatically have the required alignment - even when > you only care about the first fragment's alignment. > That's really interesting! I'm going to try that out.
diff --git a/drivers/net/ethernet/microchip/lan743x_main.c b/drivers/net/ethernet/microchip/lan743x_main.c index 1c3e204d727c..7b6794aa8ea9 100644 --- a/drivers/net/ethernet/microchip/lan743x_main.c +++ b/drivers/net/ethernet/microchip/lan743x_main.c @@ -885,8 +885,8 @@ static int lan743x_mac_set_mtu(struct lan743x_adapter *adapter, int new_mtu) } mac_rx &= ~(MAC_RX_MAX_SIZE_MASK_); - mac_rx |= (((new_mtu + ETH_HLEN + 4) << MAC_RX_MAX_SIZE_SHIFT_) & - MAC_RX_MAX_SIZE_MASK_); + mac_rx |= (((new_mtu + ETH_HLEN + ETH_FCS_LEN) + << MAC_RX_MAX_SIZE_SHIFT_) & MAC_RX_MAX_SIZE_MASK_); lan743x_csr_write(adapter, MAC_RX, mac_rx); if (enabled) { @@ -1944,7 +1944,7 @@ static int lan743x_rx_init_ring_element(struct lan743x_rx *rx, int index) struct sk_buff *skb; dma_addr_t dma_ptr; - buffer_length = netdev->mtu + ETH_HLEN + 4 + RX_HEAD_PADDING; + buffer_length = netdev->mtu + ETH_HLEN + ETH_FCS_LEN + RX_HEAD_PADDING; descriptor = &rx->ring_cpu_ptr[index]; buffer_info = &rx->buffer_info[index]; @@ -2040,7 +2040,7 @@ lan743x_rx_trim_skb(struct sk_buff *skb, int frame_length) dev_kfree_skb_irq(skb); return NULL; } - frame_length = max_t(int, 0, frame_length - RX_HEAD_PADDING - 4); + frame_length = max_t(int, 0, frame_length - ETH_FCS_LEN); if (skb->len > frame_length) { skb->tail -= skb->len - frame_length; skb->len = frame_length;