| Message ID | 1352501644-8444-1-git-send-email-mchan@broadcom.com |
|---|---|
| State | Rejected, archived |
| Delegated to: | David Miller |
| Headers | show |
On Fri, 2012-11-09 at 14:54 -0800, Michael Chan wrote: > Some bnx2x multi-function devices are configured for FCoE only in > a PCI function and only has a SAN MAC address. The current code > will fail ->ndo_validate_addr() when a VLAN device is brought up > over such a device. > > Check the real device's dev_addrs[] for a valid SAN MAC address > when validating the MAC address if the regular MAC address is not > valid. So the VLAN device's own address is being completely ignored? Ben. > Signed-off-by: Michael Chan <mchan@broadcom.com> > --- > net/8021q/vlan_dev.c | 25 ++++++++++++++++++++++++- > 1 files changed, 24 insertions(+), 1 deletions(-) > > diff --git a/net/8021q/vlan_dev.c b/net/8021q/vlan_dev.c > index 6da96d4..9ef2d2e 100644 > --- a/net/8021q/vlan_dev.c > +++ b/net/8021q/vlan_dev.c > @@ -346,6 +346,29 @@ static int vlan_dev_stop(struct net_device *dev) > return 0; > } > > +static int vlan_dev_validate_addr(struct net_device *dev) > +{ > + if (!is_valid_ether_addr(dev->dev_addr)) { > + struct net_device *real_dev = vlan_dev_priv(dev)->real_dev; > + > + if (real_dev) { > + struct netdev_hw_addr *ha; > + > + rcu_read_lock(); > + for_each_dev_addr(real_dev, ha) { > + if ((ha->type == NETDEV_HW_ADDR_T_SAN) && > + (is_valid_ether_addr(ha->addr))) { > + rcu_read_unlock(); > + return 0; > + } > + } > + rcu_read_unlock(); > + } > + return -EADDRNOTAVAIL; > + } > + return 0; > +} > + > static int vlan_dev_set_mac_address(struct net_device *dev, void *p) > { > struct net_device *real_dev = vlan_dev_priv(dev)->real_dev; > @@ -734,7 +757,7 @@ static const struct net_device_ops vlan_netdev_ops = { > .ndo_open = vlan_dev_open, > .ndo_stop = vlan_dev_stop, > .ndo_start_xmit = vlan_dev_hard_start_xmit, > - .ndo_validate_addr = eth_validate_addr, > + .ndo_validate_addr = vlan_dev_validate_addr, > .ndo_set_mac_address = vlan_dev_set_mac_address, > .ndo_set_rx_mode = vlan_dev_set_rx_mode, > .ndo_change_rx_flags = vlan_dev_change_rx_flags,
On Fri, 2012-11-09 at 23:02 +0000, Ben Hutchings wrote: > > Some bnx2x multi-function devices are configured for FCoE only in > > a PCI function and only has a SAN MAC address. The current code > > will fail ->ndo_validate_addr() when a VLAN device is brought up > > over such a device. > > > > Check the real device's dev_addrs[] for a valid SAN MAC address > > when validating the MAC address if the regular MAC address is not > > valid. > > So the VLAN device's own address is being completely ignored? No, it is not being ignored. We validate the VLAN's regular MAC address first. If it is invalid, check further to see if the real device has a SAN MAC address. -- 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
On Fri, 2012-11-09 at 16:25 -0800, Michael Chan wrote: > On Fri, 2012-11-09 at 23:02 +0000, Ben Hutchings wrote: > > > Some bnx2x multi-function devices are configured for FCoE only in > > > a PCI function and only has a SAN MAC address. The current code > > > will fail ->ndo_validate_addr() when a VLAN device is brought up > > > over such a device. > > > > > > Check the real device's dev_addrs[] for a valid SAN MAC address > > > when validating the MAC address if the regular MAC address is not > > > valid. > > > > So the VLAN device's own address is being completely ignored? > > No, it is not being ignored. We validate the VLAN's regular MAC address > first. If it is invalid, check further to see if the real device has a > SAN MAC address. Sure, but in the case that the VLAN device is layered on one of these FCoE SAN devices, the VLAN device's address isn't validated. Which presumably means it's not going to be used at all... What does the VLAN device actually do in this case? Is it a way of setting a VID to be used for the FCoE encapsulation? Ben.
On Sat, 2012-11-10 at 00:36 +0000, Ben Hutchings wrote: > On Fri, 2012-11-09 at 16:25 -0800, Michael Chan wrote: > > On Fri, 2012-11-09 at 23:02 +0000, Ben Hutchings wrote: > > > > Some bnx2x multi-function devices are configured for FCoE only in > > > > a PCI function and only has a SAN MAC address. The current code > > > > will fail ->ndo_validate_addr() when a VLAN device is brought up > > > > over such a device. > > > > > > > > Check the real device's dev_addrs[] for a valid SAN MAC address > > > > when validating the MAC address if the regular MAC address is not > > > > valid. > > > > > > So the VLAN device's own address is being completely ignored? > > > > No, it is not being ignored. We validate the VLAN's regular MAC address > > first. If it is invalid, check further to see if the real device has a > > SAN MAC address. > > Sure, but in the case that the VLAN device is layered on one of these > FCoE SAN devices, the VLAN device's address isn't validated. Which > presumably means it's not going to be used at all... > > What does the VLAN device actually do in this case? Is it a way of > setting a VID to be used for the FCoE encapsulation? Yeah, fcoemon first gets the SAN MAC from the physical device (using DCBNL) and uses it send out packets for VLAN discovery. After the VLAN has been discovered, it will create the VLAN device over the physical device and will try to bring it up. After that, fcoemon will continue to use the SAN MAC over the VLAN 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
From: "Michael Chan" <mchan@broadcom.com> Date: Fri, 9 Nov 2012 14:54:04 -0800 > Some bnx2x multi-function devices are configured for FCoE only in > a PCI function and only has a SAN MAC address. The current code > will fail ->ndo_validate_addr() when a VLAN device is brought up > over such a device. > > Check the real device's dev_addrs[] for a valid SAN MAC address > when validating the MAC address if the regular MAC address is not > valid. > > Signed-off-by: Michael Chan <mchan@broadcom.com> I've read this thread and I still don't think this is a nice change at all. It's beyond hackish to allow configuring a VLAN with a completely invalid MAC address just because the underlying real device just so happens to have a storage MAC address. I can only assume that, in this case, this code block is triggered in vlan_dev_open(): if (!ether_addr_equal(dev->dev_addr, real_dev->dev_addr)) { err = dev_uc_add(real_dev, dev->dev_addr); if (err < 0) goto out; } And if so we're adding an invalid ethernet address to the underlying device's UC list. Yet another terrible aspect of this situation. The more I look into this situation the more it looks like piling crap on top of crap on top of crap. It's a long depency chain of special cases just to get this weird setup working. I'm sorry, I'm not making the situation even worse by applying this patch. If you want to have real bypasses to allow VLAN VIDs to be configured on SAN devices, create a real interface for it, rather than hack to shreds the stuff we have already. Thanks. -- 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
On Tue, 2012-11-13 at 16:53 -0500, David Miller wrote: > I can only assume that, in this case, this code block is triggered > in vlan_dev_open(): > > if (!ether_addr_equal(dev->dev_addr, real_dev->dev_addr)) { > err = dev_uc_add(real_dev, dev->dev_addr); > if (err < 0) > goto out; > } No, this is not triggered. The addresses will be equal (and invalid) in this case. No addresses will be added. > > And if so we're adding an invalid ethernet address to the underlying > device's UC list. Yet another terrible aspect of this situation. > > The more I look into this situation the more it looks like piling crap > on top of crap on top of crap. It's a long depency chain of special > cases just to get this weird setup working. > > I'm sorry, I'm not making the situation even worse by applying this > patch. If you want to have real bypasses to allow VLAN VIDs to > be configured on SAN devices, create a real interface for it, rather > than hack to shreds the stuff we have already. I don't understand. We need to have VLAN tags inserted to packets using that interface. The VID is dynamically discovered by application. How do suggest we do that if we don't use 8021q? > > -- 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
From: "Michael Chan" <mchan@broadcom.com> Date: Tue, 13 Nov 2012 14:29:48 -0800 > We need to have VLAN tags inserted to packets using that interface. > The VID is dynamically discovered by application. How do suggest we > do that if we don't use 8021q? Ok. So what's the reason that the VLAN device isn't given a valid MAC address, for example why not use the SAN one? -- 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
On Tue, 2012-11-13 at 18:16 -0500, David Miller wrote: > From: "Michael Chan" <mchan@broadcom.com> > Date: Tue, 13 Nov 2012 14:29:48 -0800 > > > We need to have VLAN tags inserted to packets using that interface. > > The VID is dynamically discovered by application. How do suggest we > > do that if we don't use 8021q? > > Ok. So what's the reason that the VLAN device isn't given a > valid MAC address, for example why not use the SAN one? > The VLAN netdev automatically gets the MAC address from the real netdev->dev_addr. In this case, the real netdev does not have a valid dev_addr because the PCI function is a SAN device. The SAN MAC is in a different hw address list in netdev->dev_addrs. I suppose we can just put the SAN MAC into the real netdev->dev_addr so that the VLAN will automatically get it. But this doesn't seem very nice as we would be pretending to have a normal MAC address (for networking) in this SAN device. The networking MAC address is in a different PCI function. -- 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
From: "Michael Chan" <mchan@broadcom.com> Date: Tue, 13 Nov 2012 15:40:29 -0800 > I suppose we can just put the SAN MAC into the real netdev->dev_addr so > that the VLAN will automatically get it. But this doesn't seem very > nice as we would be pretending to have a normal MAC address (for > networking) in this SAN device. The networking MAC address is in a > different PCI function. I certainly would prefer if you took that approach. At least in that way the addressing of the netdev objects would appear more consistent. Thanks. -- 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
On Tue, 2012-11-13 at 20:24 -0500, David Miller wrote: > From: "Michael Chan" <mchan@broadcom.com> > Date: Tue, 13 Nov 2012 15:40:29 -0800 > > > I suppose we can just put the SAN MAC into the real netdev->dev_addr so > > that the VLAN will automatically get it. But this doesn't seem very > > nice as we would be pretending to have a normal MAC address (for > > networking) in this SAN device. The networking MAC address is in a > > different PCI function. > > I certainly would prefer if you took that approach. At least in that > way the addressing of the netdev objects would appear more consistent. > Ok. To be more complete, I think we need to add a flag or something to such a netdev to indicate that it is a SAN device only. What's your opinion on that? -- 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
On 11/13/2012 5:50 PM, Michael Chan wrote: > On Tue, 2012-11-13 at 20:24 -0500, David Miller wrote: >> From: "Michael Chan" <mchan@broadcom.com> >> Date: Tue, 13 Nov 2012 15:40:29 -0800 >> >>> I suppose we can just put the SAN MAC into the real netdev->dev_addr so >>> that the VLAN will automatically get it. But this doesn't seem very >>> nice as we would be pretending to have a normal MAC address (for >>> networking) in this SAN device. The networking MAC address is in a >>> different PCI function. >> >> I certainly would prefer if you took that approach. At least in that >> way the addressing of the netdev objects would appear more consistent. >> > Ok. To be more complete, I think we need to add a flag or something to > such a netdev to indicate that it is a SAN device only. What's your > opinion on that? Michael, how do you determine a L2 packet is a SAN type? Do you have ACLs in the FW/hardware to prevent other types of L2 traffic from being sent? I guess I'm asking what makes it a SAN only device. .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
From: "Michael Chan" <mchan@broadcom.com> Date: Tue, 13 Nov 2012 17:50:50 -0800 > On Tue, 2012-11-13 at 20:24 -0500, David Miller wrote: >> From: "Michael Chan" <mchan@broadcom.com> >> Date: Tue, 13 Nov 2012 15:40:29 -0800 >> >> > I suppose we can just put the SAN MAC into the real netdev->dev_addr so >> > that the VLAN will automatically get it. But this doesn't seem very >> > nice as we would be pretending to have a normal MAC address (for >> > networking) in this SAN device. The networking MAC address is in a >> > different PCI function. >> >> I certainly would prefer if you took that approach. At least in that >> way the addressing of the netdev objects would appear more consistent. >> > Ok. To be more complete, I think we need to add a flag or something to > such a netdev to indicate that it is a SAN device only. What's your > opinion on that? Yes, that would describe the limitation you mentioned. Maybe add a new netdev->priv_flags for this. -- 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
On Tue, 2012-11-13 at 17:58 -0800, John Fastabend wrote: > On 11/13/2012 5:50 PM, Michael Chan wrote: > > On Tue, 2012-11-13 at 20:24 -0500, David Miller wrote: > >> From: "Michael Chan" <mchan@broadcom.com> > >> Date: Tue, 13 Nov 2012 15:40:29 -0800 > >> > >>> I suppose we can just put the SAN MAC into the real netdev->dev_addr so > >>> that the VLAN will automatically get it. But this doesn't seem very > >>> nice as we would be pretending to have a normal MAC address (for > >>> networking) in this SAN device. The networking MAC address is in a > >>> different PCI function. > >> > >> I certainly would prefer if you took that approach. At least in that > >> way the addressing of the netdev objects would appear more consistent. > >> > > Ok. To be more complete, I think we need to add a flag or something to > > such a netdev to indicate that it is a SAN device only. What's your > > opinion on that? > > Michael, how do you determine a L2 packet is a SAN type? Do you have > ACLs in the FW/hardware to prevent other types of L2 traffic from being > sent? I guess I'm asking what makes it a SAN only device. > Adding Eilon and Ariel from the bnx2x team to help answer the question. I think we use ndo_select_queue() to put all FCoE related packets onto the proper ring that is programmed with the proper DCB parameters. Packets to the other rings will be dropped on such a device. That's why a flag will be useful so that tools will know its limitation. > -- 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 --git a/net/8021q/vlan_dev.c b/net/8021q/vlan_dev.c index 6da96d4..9ef2d2e 100644 --- a/net/8021q/vlan_dev.c +++ b/net/8021q/vlan_dev.c @@ -346,6 +346,29 @@ static int vlan_dev_stop(struct net_device *dev) return 0; } +static int vlan_dev_validate_addr(struct net_device *dev) +{ + if (!is_valid_ether_addr(dev->dev_addr)) { + struct net_device *real_dev = vlan_dev_priv(dev)->real_dev; + + if (real_dev) { + struct netdev_hw_addr *ha; + + rcu_read_lock(); + for_each_dev_addr(real_dev, ha) { + if ((ha->type == NETDEV_HW_ADDR_T_SAN) && + (is_valid_ether_addr(ha->addr))) { + rcu_read_unlock(); + return 0; + } + } + rcu_read_unlock(); + } + return -EADDRNOTAVAIL; + } + return 0; +} + static int vlan_dev_set_mac_address(struct net_device *dev, void *p) { struct net_device *real_dev = vlan_dev_priv(dev)->real_dev; @@ -734,7 +757,7 @@ static const struct net_device_ops vlan_netdev_ops = { .ndo_open = vlan_dev_open, .ndo_stop = vlan_dev_stop, .ndo_start_xmit = vlan_dev_hard_start_xmit, - .ndo_validate_addr = eth_validate_addr, + .ndo_validate_addr = vlan_dev_validate_addr, .ndo_set_mac_address = vlan_dev_set_mac_address, .ndo_set_rx_mode = vlan_dev_set_rx_mode, .ndo_change_rx_flags = vlan_dev_change_rx_flags,
Some bnx2x multi-function devices are configured for FCoE only in a PCI function and only has a SAN MAC address. The current code will fail ->ndo_validate_addr() when a VLAN device is brought up over such a device. Check the real device's dev_addrs[] for a valid SAN MAC address when validating the MAC address if the regular MAC address is not valid. Signed-off-by: Michael Chan <mchan@broadcom.com> --- net/8021q/vlan_dev.c | 25 ++++++++++++++++++++++++- 1 files changed, 24 insertions(+), 1 deletions(-)