Patchwork 2.6.37 regression: adding main interface to a bridge breaks vlan interface RX

login
register
mail settings
Submitter françois romieu
Date March 1, 2011, 10:16 a.m.
Message ID <20110301101609.GA13261@electric-eye.fr.zoreil.com>
Download mbox | patch
Permalink /patch/84922/
State RFC
Delegated to: David Miller
Headers show

Comments

françois romieu - March 1, 2011, 10:16 a.m.
Jesse Gross <jesse@nicira.com> :
[...]
> Putting a vlan device on eth1 actually affects the behavior of the
> driver, which is why that works.  I suspect that if you put the vlan
> device on both the physical interface and the bridge you would see
> packets with tags on the bridge.
> 
> Regardless, the solution is to remove the dependency on vlan devices
> by converting over to the new vlan model.  Francois, any chance that
> you might have some time to look at this?

The completely untested patch below is available for the (really) desperate
souls.

I should do a -next testing session this afternoon for this patch + 
Oliver's Neukum advertisement control + Hayes 8105e stuff (in reverse order).

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jesse Gross - March 1, 2011, 8:04 p.m.
On Tue, Mar 1, 2011 at 2:16 AM, Francois Romieu <romieu@fr.zoreil.com> wrote:
> Jesse Gross <jesse@nicira.com> :
> [...]
>> Putting a vlan device on eth1 actually affects the behavior of the
>> driver, which is why that works.  I suspect that if you put the vlan
>> device on both the physical interface and the bridge you would see
>> packets with tags on the bridge.
>>
>> Regardless, the solution is to remove the dependency on vlan devices
>> by converting over to the new vlan model.  Francois, any chance that
>> you might have some time to look at this?
>
> The completely untested patch below is available for the (really) desperate
> souls.
>
> I should do a -next testing session this afternoon for this patch +
> Oliver's Neukum advertisement control + Hayes 8105e stuff (in reverse order).

Thanks Francois.  When you are ready this looks good to me.

Reviewed-by: Jesse Gross <jesse@nicira.com>
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
françois romieu - March 1, 2011, 9:52 p.m.
Francois Romieu <romieu@fr.zoreil.com> :
> Jesse Gross <jesse@nicira.com> :
> [...]
> > Regardless, the solution is to remove the dependency on vlan devices
> > by converting over to the new vlan model.  Francois, any chance that
> > you might have some time to look at this?
> 
> The completely untested patch below is available for the (really) desperate
> souls.
> 
> I should do a -next testing session this afternoon for this patch + 
> Oliver's Neukum advertisement control + Hayes 8105e stuff (in reverse order).

It took a bit longer than expected but the patch seems to work.

Chriss, can you give it a try against current -rc / -git and see if it makes
a difference ?
chriss - March 4, 2011, 7:32 p.m.
Francois Romieu <romieu <at> fr.zoreil.com> writes:

> 
> Chriss, can you give it a try against current -rc / -git and see if it makes
> a difference ?
> 

I hope i'm doing it right. i checked out the
'git://git.kernel.org/pub/scm/linux/kernel/git/sfr/linux-next.git'

and will try to apply the patch you provided. please correct me if i'm wrong.
I will report back.

regards//chriss

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
françois romieu - March 4, 2011, 10:41 p.m.
chriss <mail_to_chriss@gmx.net> :
[...]
> I hope i'm doing it right. i checked out the
> 'git://git.kernel.org/pub/scm/linux/kernel/git/sfr/linux-next.git'
> 
> and will try to apply the patch you provided. please correct me if i'm wrong.
> I will report back.

I have tested it against davem's -next somewhere along -rc5 but it should be
fine as well.
chriss - March 5, 2011, 10:53 a.m.
Francois Romieu <romieu <at> fr.zoreil.com> writes:

> 
> chriss <mail_to_chriss <at> gmx.net> :
> [...]
> > I hope i'm doing it right. i checked out the
> > 'git://git.kernel.org/pub/scm/linux/kernel/git/sfr/linux-next.git'
> > 
> > and will try to apply the patch you provided. please correct me if i'm wrong.
> > I will report back.
> 
> I have tested it against davem's -next somewhere along -rc5 but it should be
> fine as well.
> 

My hat gows of to you kindly sir! Everything works as expected. im looking
forward to see this patch in the stable releases.

thank you very much
//chriss

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig
index 0382332..8e7da78 100644
--- a/drivers/net/Kconfig
+++ b/drivers/net/Kconfig
@@ -2235,15 +2235,6 @@  config R8169
 	  To compile this driver as a module, choose M here: the module
 	  will be called r8169.  This is recommended.
 
-config R8169_VLAN
-	bool "VLAN support"
-	depends on R8169 && VLAN_8021Q
-	---help---
-	  Say Y here for the r8169 driver to support the functions required
-	  by the kernel 802.1Q code.
-
-	  If in doubt, say Y.
-
 config SB1250_MAC
 	tristate "SB1250 Gigabit Ethernet support"
 	depends on SIBYTE_SB1xxx_SOC
diff --git a/drivers/net/r8169.c b/drivers/net/r8169.c
index bde7d61..5c4f03d 100644
--- a/drivers/net/r8169.c
+++ b/drivers/net/r8169.c
@@ -526,9 +526,6 @@  struct rtl8169_private {
 	u16 napi_event;
 	u16 intr_mask;
 	int phy_1000_ctrl_reg;
-#ifdef CONFIG_R8169_VLAN
-	struct vlan_group *vlgrp;
-#endif
 
 	struct mdio_ops {
 		void (*write)(void __iomem *, int, int);
@@ -1254,8 +1251,6 @@  static int rtl8169_set_rx_csum(struct net_device *dev, u32 data)
 	return 0;
 }
 
-#ifdef CONFIG_R8169_VLAN
-
 static inline u32 rtl8169_tx_vlan_tag(struct rtl8169_private *tp,
 				      struct sk_buff *skb)
 {
@@ -1263,64 +1258,37 @@  static inline u32 rtl8169_tx_vlan_tag(struct rtl8169_private *tp,
 		TxVlanTag | swab16(vlan_tx_tag_get(skb)) : 0x00;
 }
 
-static void rtl8169_vlan_rx_register(struct net_device *dev,
-				     struct vlan_group *grp)
+#define NETIF_F_HW_VLAN_TX_RX	(NETIF_F_HW_VLAN_TX | NETIF_F_HW_VLAN_RX)
+
+static void rtl8169_vlan_mode(struct net_device *dev)
 {
 	struct rtl8169_private *tp = netdev_priv(dev);
 	void __iomem *ioaddr = tp->mmio_addr;
 	unsigned long flags;
 
 	spin_lock_irqsave(&tp->lock, flags);
-	tp->vlgrp = grp;
-	/*
-	 * Do not disable RxVlan on 8110SCd.
-	 */
-	if (tp->vlgrp || (tp->mac_version == RTL_GIGA_MAC_VER_05))
+	if (dev->features & NETIF_F_HW_VLAN_RX)
 		tp->cp_cmd |= RxVlan;
 	else
 		tp->cp_cmd &= ~RxVlan;
 	RTL_W16(CPlusCmd, tp->cp_cmd);
+	/* PCI commit */
 	RTL_R16(CPlusCmd);
 	spin_unlock_irqrestore(&tp->lock, flags);
+
+	dev->vlan_features = dev->features &~ NETIF_F_HW_VLAN_TX_RX;
 }
 
-static int rtl8169_rx_vlan_skb(struct rtl8169_private *tp, struct RxDesc *desc,
-			       struct sk_buff *skb, int polling)
+static void rtl8169_rx_vlan_tag(struct RxDesc *desc, struct sk_buff *skb)
 {
 	u32 opts2 = le32_to_cpu(desc->opts2);
-	struct vlan_group *vlgrp = tp->vlgrp;
-	int ret;
 
-	if (vlgrp && (opts2 & RxVlanTag)) {
-		u16 vtag = swab16(opts2 & 0xffff);
+	if (opts2 & RxVlanTag)
+		__vlan_hwaccel_put_tag(skb, swab16(opts2 & 0xffff));
 
-		if (likely(polling))
-			vlan_gro_receive(&tp->napi, vlgrp, vtag, skb);
-		else
-			__vlan_hwaccel_rx(skb, vlgrp, vtag, polling);
-		ret = 0;
-	} else
-		ret = -1;
 	desc->opts2 = 0;
-	return ret;
-}
-
-#else /* !CONFIG_R8169_VLAN */
-
-static inline u32 rtl8169_tx_vlan_tag(struct rtl8169_private *tp,
-				      struct sk_buff *skb)
-{
-	return 0;
-}
-
-static int rtl8169_rx_vlan_skb(struct rtl8169_private *tp, struct RxDesc *desc,
-			       struct sk_buff *skb, int polling)
-{
-	return -1;
 }
 
-#endif
-
 static int rtl8169_gset_tbi(struct net_device *dev, struct ethtool_cmd *cmd)
 {
 	struct rtl8169_private *tp = netdev_priv(dev);
@@ -1491,6 +1459,28 @@  static void rtl8169_get_strings(struct net_device *dev, u32 stringset, u8 *data)
 	}
 }
 
+static int rtl8169_set_flags(struct net_device *dev, u32 data)
+{
+	struct rtl8169_private *tp = netdev_priv(dev);
+	unsigned long old_feat = dev->features;
+	int rc;
+
+	if ((tp->mac_version == RTL_GIGA_MAC_VER_05) &&
+	    !(data & ETH_FLAG_RXVLAN)) {
+		netif_info(tp, drv, dev, "8110SCd requires hardware Rx VLAN\n");
+		return -EINVAL;
+	}
+
+	rc = ethtool_op_set_flags(dev, data, ETH_FLAG_TXVLAN | ETH_FLAG_RXVLAN);
+	if (rc)
+		return rc;
+
+	if ((old_feat ^ dev->features) & NETIF_F_HW_VLAN_RX)
+		rtl8169_vlan_mode(dev);
+
+	return 0;
+}
+
 static const struct ethtool_ops rtl8169_ethtool_ops = {
 	.get_drvinfo		= rtl8169_get_drvinfo,
 	.get_regs_len		= rtl8169_get_regs_len,
@@ -1510,6 +1500,8 @@  static const struct ethtool_ops rtl8169_ethtool_ops = {
 	.get_strings		= rtl8169_get_strings,
 	.get_sset_count		= rtl8169_get_sset_count,
 	.get_ethtool_stats	= rtl8169_get_ethtool_stats,
+	.set_flags		= rtl8169_set_flags,
+	.get_flags		= ethtool_op_get_flags,
 };
 
 static void rtl8169_get_mac_version(struct rtl8169_private *tp,
@@ -2792,9 +2784,6 @@  static const struct net_device_ops rtl8169_netdev_ops = {
 	.ndo_set_mac_address	= rtl_set_mac_address,
 	.ndo_do_ioctl		= rtl8169_ioctl,
 	.ndo_set_multicast_list	= rtl_set_rx_mode,
-#ifdef CONFIG_R8169_VLAN
-	.ndo_vlan_rx_register	= rtl8169_vlan_rx_register,
-#endif
 #ifdef CONFIG_NET_POLL_CONTROLLER
 	.ndo_poll_controller	= rtl8169_netpoll,
 #endif
@@ -3086,6 +3075,13 @@  rtl8169_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 	/* Identify chip attached to board */
 	rtl8169_get_mac_version(tp, ioaddr);
 
+	/*
+	 * Pretend we are using VLANs; This bypasses a nasty bug where
+	 * Interrupts stop flowing on high load on 8110SCd controllers.
+	 */
+	if (tp->mac_version == RTL_GIGA_MAC_VER_05)
+		tp->cp_cmd |= RxVlan;
+
 	rtl_init_mdio_ops(tp);
 	rtl_init_pll_power_ops(tp);
 
@@ -3154,10 +3150,7 @@  rtl8169_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 
 	netif_napi_add(dev, &tp->napi, rtl8169_poll, R8169_NAPI_WEIGHT);
 
-#ifdef CONFIG_R8169_VLAN
-	dev->features |= NETIF_F_HW_VLAN_TX | NETIF_F_HW_VLAN_RX;
-#endif
-	dev->features |= NETIF_F_GRO;
+	dev->features |= NETIF_F_HW_VLAN_TX_RX | NETIF_F_GRO;
 
 	tp->intr_mask = 0xffff;
 	tp->hw_start = cfg->hw_start;
@@ -3273,12 +3266,7 @@  static int rtl8169_open(struct net_device *dev)
 
 	rtl8169_init_phy(dev, tp);
 
-	/*
-	 * Pretend we are using VLANs; This bypasses a nasty bug where
-	 * Interrupts stop flowing on high load on 8110SCd controllers.
-	 */
-	if (tp->mac_version == RTL_GIGA_MAC_VER_05)
-		RTL_W16(CPlusCmd, RTL_R16(CPlusCmd) | RxVlan);
+	rtl8169_vlan_mode(dev);
 
 	rtl_pll_power_up(tp);
 
@@ -4589,12 +4577,12 @@  static int rtl8169_rx_interrupt(struct net_device *dev,
 			skb_put(skb, pkt_size);
 			skb->protocol = eth_type_trans(skb, dev);
 
-			if (rtl8169_rx_vlan_skb(tp, desc, skb, polling) < 0) {
-				if (likely(polling))
-					napi_gro_receive(&tp->napi, skb);
-				else
-					netif_rx(skb);
-			}
+			rtl8169_rx_vlan_tag(desc, skb);
+
+			if (likely(polling))
+				napi_gro_receive(&tp->napi, skb);
+			else
+				netif_rx(skb);
 
 			dev->stats.rx_bytes += pkt_size;
 			dev->stats.rx_packets++;