diff mbox

[net-next-2.6] macvlan: add VLAN filters to lowerdev

Message ID 20110606142715.2692.6311.stgit@jf-dev1-dcblab
State Accepted, archived
Delegated to: David Miller
Headers show

Commit Message

John Fastabend June 6, 2011, 2:27 p.m. UTC 
Stacking VLANs on top of the macvlan device does not
work if the lowerdev device is using vlan filters set
by NETIF_F_HW_VLAN_FILTER. Add ndo ops to pass vlan
calls to lowerdev.

Signed-off-by: John Fastabend <john.r.fastabend@intel.com>
---

 drivers/net/macvlan.c |   39 ++++++++++++++++++++++++++++++++++++++-
 1 files changed, 38 insertions(+), 1 deletions(-)


--
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

Comments

David Miller June 6, 2011, 10:03 p.m. UTC | #1 
From: John Fastabend <john.r.fastabend@intel.com>
Date: Mon, 06 Jun 2011 07:27:16 -0700

> Stacking VLANs on top of the macvlan device does not
> work if the lowerdev device is using vlan filters set
> by NETIF_F_HW_VLAN_FILTER. Add ndo ops to pass vlan
> calls to lowerdev.
> 
> Signed-off-by: John Fastabend <john.r.fastabend@intel.com>

I think this might have unintended side-effects.

Much of the VLAN code makes decisions based upon whether these
ops are NULL or not.

Now, no matter what is implemented in the lower device, the VLAN
code will see them non-NULL in the macvlan device.
--
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
John Fastabend June 6, 2011, 10:44 p.m. UTC | #2 
On 6/6/2011 3:03 PM, David Miller wrote:
> From: John Fastabend <john.r.fastabend@intel.com>
> Date: Mon, 06 Jun 2011 07:27:16 -0700
> 
>> Stacking VLANs on top of the macvlan device does not
>> work if the lowerdev device is using vlan filters set
>> by NETIF_F_HW_VLAN_FILTER. Add ndo ops to pass vlan
>> calls to lowerdev.
>>
>> Signed-off-by: John Fastabend <john.r.fastabend@intel.com>
> 
> I think this might have unintended side-effects.
> 
> Much of the VLAN code makes decisions based upon whether these
> ops are NULL or not.
> 
> Now, no matter what is implemented in the lower device, the VLAN
> code will see them non-NULL in the macvlan device.

I would expect these decisions to be wrapped in the feature flag
like this,

        if (vlan_id && (real_dev->features & NETIF_F_HW_VLAN_FILTER))
                ops->ndo_vlan_rx_kill_vid(real_dev, vlan_id);

Although grep found two call sites not wrapped,

int register_vlan_dev(struct net_device *dev)
	[...]
        if (ngrp) {
                if (ops->ndo_vlan_rx_register)
                        ops->ndo_vlan_rx_register(real_dev, ngrp);
                rcu_assign_pointer(real_dev->vlgrp, ngrp);
        }


And,

void unregister_vlan_dev(struct net_device *dev, struct list_head *head)
	[...]

        /* If the group is now empty, kill off the group. */
        if (grp->nr_vlans == 0) {
                vlan_gvrp_uninit_applicant(real_dev);

                rcu_assign_pointer(real_dev->vlgrp, NULL);
                if (ops->ndo_vlan_rx_register)
                        ops->ndo_vlan_rx_register(real_dev, NULL);

                /* Free the group, after all cpu's are done. */
                call_rcu(&grp->rcu, vlan_rcu_free);
        }


I could wrap these in feature flag checks as well but I see no harm
in letting these fall through to the macvlan driver and failing.

Thanks,
John.




--
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
David Miller June 6, 2011, 11:44 p.m. UTC | #3 
From: John Fastabend <john.r.fastabend@intel.com>
Date: Mon, 06 Jun 2011 15:44:39 -0700

> On 6/6/2011 3:03 PM, David Miller wrote:
>> From: John Fastabend <john.r.fastabend@intel.com>
>> Date: Mon, 06 Jun 2011 07:27:16 -0700
>> 
>>> Stacking VLANs on top of the macvlan device does not
>>> work if the lowerdev device is using vlan filters set
>>> by NETIF_F_HW_VLAN_FILTER. Add ndo ops to pass vlan
>>> calls to lowerdev.
>>>
>>> Signed-off-by: John Fastabend <john.r.fastabend@intel.com>
>> 
>> I think this might have unintended side-effects.
>> 
>> Much of the VLAN code makes decisions based upon whether these
>> ops are NULL or not.
>> 
>> Now, no matter what is implemented in the lower device, the VLAN
>> code will see them non-NULL in the macvlan device.
> 
> I would expect these decisions to be wrapped in the feature flag
> like this,
> 
>         if (vlan_id && (real_dev->features & NETIF_F_HW_VLAN_FILTER))
>                 ops->ndo_vlan_rx_kill_vid(real_dev, vlan_id);
> 
> Although grep found two call sites not wrapped,
 ...
> I could wrap these in feature flag checks as well but I see no harm
> in letting these fall through to the macvlan driver and failing.

Fair enough, patch applied.
--
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
diff mbox

Patch

diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c
index d6aeaa5..cc67cbe 100644
--- a/drivers/net/macvlan.c
+++ b/drivers/net/macvlan.c
@@ -414,7 +414,8 @@  static struct lock_class_key macvlan_netdev_addr_lock_key;
 #define MACVLAN_FEATURES \
 	(NETIF_F_SG | NETIF_F_ALL_CSUM | NETIF_F_HIGHDMA | NETIF_F_FRAGLIST | \
 	 NETIF_F_GSO | NETIF_F_TSO | NETIF_F_UFO | NETIF_F_GSO_ROBUST | \
-	 NETIF_F_TSO_ECN | NETIF_F_TSO6 | NETIF_F_GRO | NETIF_F_RXCSUM)
+	 NETIF_F_TSO_ECN | NETIF_F_TSO6 | NETIF_F_GRO | NETIF_F_RXCSUM | \
+	 NETIF_F_HW_VLAN_FILTER)
 
 #define MACVLAN_STATE_MASK \
 	((1<<__LINK_STATE_NOCARRIER) | (1<<__LINK_STATE_DORMANT))
@@ -509,6 +510,39 @@  static struct rtnl_link_stats64 *macvlan_dev_get_stats64(struct net_device *dev,
 	return stats;
 }
 
+static void macvlan_vlan_rx_register(struct net_device *dev,
+				     struct vlan_group *grp)
+{
+	struct macvlan_dev *vlan = netdev_priv(dev);
+	struct net_device *lowerdev = vlan->lowerdev;
+	const struct net_device_ops *ops = lowerdev->netdev_ops;
+
+	if (ops->ndo_vlan_rx_register)
+		ops->ndo_vlan_rx_register(lowerdev, grp);
+}
+
+static void macvlan_vlan_rx_add_vid(struct net_device *dev,
+				    unsigned short vid)
+{
+	struct macvlan_dev *vlan = netdev_priv(dev);
+	struct net_device *lowerdev = vlan->lowerdev;
+	const struct net_device_ops *ops = lowerdev->netdev_ops;
+
+	if (ops->ndo_vlan_rx_add_vid)
+		ops->ndo_vlan_rx_add_vid(lowerdev, vid);
+}
+
+static void macvlan_vlan_rx_kill_vid(struct net_device *dev,
+				     unsigned short vid)
+{
+	struct macvlan_dev *vlan = netdev_priv(dev);
+	struct net_device *lowerdev = vlan->lowerdev;
+	const struct net_device_ops *ops = lowerdev->netdev_ops;
+
+	if (ops->ndo_vlan_rx_kill_vid)
+		ops->ndo_vlan_rx_kill_vid(lowerdev, vid);
+}
+
 static void macvlan_ethtool_get_drvinfo(struct net_device *dev,
 					struct ethtool_drvinfo *drvinfo)
 {
@@ -541,6 +575,9 @@  static const struct net_device_ops macvlan_netdev_ops = {
 	.ndo_set_multicast_list	= macvlan_set_multicast_list,
 	.ndo_get_stats64	= macvlan_dev_get_stats64,
 	.ndo_validate_addr	= eth_validate_addr,
+	.ndo_vlan_rx_register	= macvlan_vlan_rx_register,
+	.ndo_vlan_rx_add_vid	= macvlan_vlan_rx_add_vid,
+	.ndo_vlan_rx_kill_vid	= macvlan_vlan_rx_kill_vid,
 };
 
 void macvlan_common_setup(struct net_device *dev)