From mboxrd@z Thu Jan 1 00:00:00 1970 From: Florian Fainelli Subject: Re: DSA and skb->protocol Date: Mon, 15 Sep 2014 10:08:52 -0700 Message-ID: <54171D24.9090009@gmail.com> References: <20140914153751.GA28585@lunn.ch> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Cc: seugene@marvell.com, Andrew Lunn , netdev@vger.kernel.org, kernel@wantstofly.org To: Andrew Lunn Return-path: Received: from mail-pa0-f42.google.com ([209.85.220.42]:46562 "EHLO mail-pa0-f42.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753799AbaIORJE (ORCPT ); Mon, 15 Sep 2014 13:09:04 -0400 Received: by mail-pa0-f42.google.com with SMTP id lj1so6837685pab.29 for ; Mon, 15 Sep 2014 10:09:03 -0700 (PDT) In-Reply-To: <20140914153751.GA28585@lunn.ch> Sender: netdev-owner@vger.kernel.org List-ID: Hi Andrew, On 09/14/2014 08:37 AM, Andrew Lunn wrote: > Hi Florian > > I've been debugging a WARNING when using DSA with a D-Link > DIR665. I've had reports of the same warning with another device. > > WARNING: CPU: 0 PID: 2014 at net/core/dev.c:2260 skb_warn_bad_offload+0xd0/0x104() > mv643xx_eth_port: caps=(0x0000000400014803, 0x00000000001b482b) len=1722 data_len=16 gso_size=1448 gso_type=1 gso_segs 2 ip_summed=3 encapsulation 0 features 400014801 > Modules linked in: > CPU: 0 PID: 2014 Comm: sshd Tainted: G W 3.17.0-rc1-00007-g2f06b2c08099-dirty #228 > [] (unwind_backtrace) from [] (show_stack+0x10/0x14) > [] (show_stack) from [] (warn_slowpath_common+0x6c/0x8c) > [] (warn_slowpath_common) from [] (warn_slowpath_fmt+0x30/0x40) > [] (warn_slowpath_fmt) from [] (skb_warn_bad_offload+0xd0/0x104) > [] (skb_warn_bad_offload) from [] (skb_checksum_help+0x160/0x170) > [] (skb_checksum_help) from [] (dev_hard_start_xmit+0x3f8/0x4c0) > [] (dev_hard_start_xmit) from [] (sch_direct_xmit+0x148/0x250) > [] (sch_direct_xmit) from [] (__dev_queue_xmit+0x278/0x5f4) > [] (__dev_queue_xmit) from [] (edsa_xmit+0xf8/0x2c8) > [] (edsa_xmit) from [] (dev_hard_start_xmit+0x2dc/0x4c0) > [] (dev_hard_start_xmit) from [] (__dev_queue_xmit+0x3c4/0x5f4) > [] (__dev_queue_xmit) from [] (ip_finish_output+0x64c/0x920) > [] (ip_finish_output) from [] (ip_local_out_sk+0x34/0x38) > [] (ip_local_out_sk) from [] (ip_queue_xmit+0x128/0x388) > [] (ip_queue_xmit) from [] (tcp_transmit_skb+0x534/0x93c) > [] (tcp_transmit_skb) from [] (tcp_write_xmit+0x148/0xbf8) > [] (tcp_write_xmit) from [] (__tcp_push_pending_frames+0x30/0x9c) > [] (__tcp_push_pending_frames) from [] (tcp_sendmsg+0xc0/0xcec) > [] (tcp_sendmsg) from [] (inet_sendmsg+0x3c/0x70) > [] (inet_sendmsg) from [] (sock_aio_write+0xcc/0xec) > [] (sock_aio_write) from [] (do_sync_write+0x7c/0xa4) > [] (do_sync_write) from [] (vfs_write+0x108/0x1b0) > [] (vfs_write) from [] (SyS_write+0x40/0x94) > [] (SyS_write) from [] (ret_fast_syscall+0x0/0x2c) > ---[ end trace b1b02d15aba4766a ]--- > > I think i understand what is going on, and one of your recent patches > may indicate you have seen something similar. > > int dev_hard_start_xmit() we have: > > 2607 > 2608 features = netif_skb_features(skb); > > ... > 2637 /* If packet is not checksummed and device does not > 2638 * support checksumming for this protocol, complete > 2639 * checksumming here. > 2640 */ > 2641 if (skb->ip_summed == CHECKSUM_PARTIAL) { > 2642 if (skb->encapsulation) > 2643 skb_set_inner_transport_header(skb, > 2644 skb_checksum_start_offset(skb)); > 2645 else > 2646 skb_set_transport_header(skb, > 2647 skb_checksum_start_offset(skb)); > 2648 if (!(features & NETIF_F_ALL_CSUM) && > 2649 skb_checksum_help(skb)) > 2650 goto out_kfree_skb; > 2651 } > > This packet is CHECKSUM_PARTIAL, but it is also a GSO packet, with two > segments, so the skb_checksum_help() is throwing the warning. It is > expected that the hardware with do the checksum when using GSO. The > reason it is trying to do software checksums, not hardware, is because > features does not indicate the protocol is supported by hardware > checksumming. This i initially thought was odd, since this is a TCP/IP > packet, as you can see from the stack trace. However, > > netif_skb_features(skb) calls harmonize_features() which calls > can_checksum_protocol(): > > 3206 static inline bool can_checksum_protocol(netdev_features_t features, > 3207 __be16 protocol) > 3208 { > 3209 return ((features & NETIF_F_GEN_CSUM) || > 3210 ((features & NETIF_F_V4_CSUM) && > 3211 protocol == htons(ETH_P_IP)) || > 3212 ((features & NETIF_F_V6_CSUM) && > 3213 protocol == htons(ETH_P_IPV6)) || > 3214 ((features & NETIF_F_FCOE_CRC) && > 3215 protocol == htons(ETH_P_FCOE))); > 3216 } > > However looking in the skb, we see protocol is now ETH_P_EDSA, not > ETH_P_IP. Hence the hardware does not support this protocol. Usually, the hardware needs to be told there is a DSA/EDSA tag before the actual Ethernet frame, I don't have the mv643xx_eth documentation handy, but I suppose there should be something like this available. > > This protocol value is because edsa_xmit() changes it in the slave > device TX path. > > Your patch "net: dsa: change tag_protocol to an enum" > has a hunk: > > diff --git a/net/dsa/tag_brcm.c b/net/dsa/tag_brcm.c > index e0b759ec209e..8fbc21c0de78 100644 > --- a/net/dsa/tag_brcm.c > +++ b/net/dsa/tag_brcm.c > @@ -91,7 +91,6 @@ static netdev_tx_t brcm_tag_xmit(struct sk_buff *skb, struct net_device *dev) > /* Queue the SKB for transmission on the parent interface, but > * do not modify its EtherType > */ > - skb->protocol = htons(ETH_P_BRCMTAG); > skb->dev = p->parent->dst->master_netdev; > dev_queue_xmit(skb); > > making me think it is not actually needed to change > skb->protocol. When i remove this from edsa_xmit(), the warning goes > away and networking seems to work O.K. > > Is this the right fix? Should we remove this setting of skb->protocol > in the other dsa xmit functions? Adding Lennert here, I suspect that having the skb->protocol assignment was initially done to help drivers such as mv643xx_eth and others doing DSA to be able to identify these packets properly in the transmit path. Unless there is something else, I would be inclined to remove the skb->protocol assignment from tag_dsa.c and tag_edsa.c. In case the driver needs to consult what is configured, it should either: - look at dev->dsa_ptr->tag_protocol if we do not need to know on a per-packet basis what's the tagging protocol used (using netdev_uses_dsa() + a helper function we'd introduce) - or, if we we need that information to be per-packet, have a DSA-specific control block and a set of helpers to retrieve that information -- Florian