| Message ID | 20090723082552.GA602@serverengines.com |
|---|---|
| State | Rejected, archived |
| Delegated to: | David Miller |
| Headers | show |
Sarveshwar Bandi wrote: > Please review and apply patch to net-next tree. Patch implements the > support for q-in-q mode. Please describe your change more precisely. How does this interact with the stack and which VID is propagated to the VLAN code? -- 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 23/07/09 10:47 +0200, Patrick McHardy wrote: > Sarveshwar Bandi wrote: > > Please review and apply patch to net-next tree. Patch implements the > > support for q-in-q mode. > > Please describe your change more precisely. How does this interact with > the stack and which VID is propagated to the VLAN code? Patch has code to check if the controller is in q-in-q mode. When the packet has two vids, only the inner vid is passed onto the stack. The stack is never made aware of the outer vid. > -- > 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 -- 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
Sarveshwar Bandi wrote: > On 23/07/09 10:47 +0200, Patrick McHardy wrote: >> Sarveshwar Bandi wrote: >>> Please review and apply patch to net-next tree. Patch implements the >>> support for q-in-q mode. >> >> Please describe your change more precisely. How does this interact with >> the stack and which VID is propagated to the VLAN code? > > Patch has code to check if the controller is in q-in-q mode. When the packet > has two vids, only the inner vid is passed onto the stack. The stack is never > made aware of the outer vid. But you're still using the outer VLAN group when passing the packet to the VLAN code, so the association to the correct VLAN device can't work. -- 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 23/07/09 11:07 +0200, Patrick McHardy wrote: > Sarveshwar Bandi wrote: > > On 23/07/09 10:47 +0200, Patrick McHardy wrote: > >> Sarveshwar Bandi wrote: > >>> Please review and apply patch to net-next tree. Patch implements the > >>> support for q-in-q mode. > >> > >> Please describe your change more precisely. How does this interact with > >> the stack and which VID is propagated to the VLAN code? > > > > Patch has code to check if the controller is in q-in-q mode. When the packet > > has two vids, only the inner vid is passed onto the stack. The stack is never > > made aware of the outer vid. > > But you're still using the outer VLAN group when passing the packet to > the VLAN code, so the association to the correct VLAN device can't work. In the case where packet comes with two vlan ids, the rx descriptor contains the inner vlan id and qnq is set to 1, the driver indicates this vid to the stack. In the case where packet comes with single vlan id, the rx descriptor contains the outer vlan id and qnq is set to 0, the driver indicates this packet as a non-vlan packet to the stack by calling netif_receive_skb. > -- > 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 -- 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
Sarveshwar Bandi wrote: > On 23/07/09 11:07 +0200, Patrick McHardy wrote: >>> Patch has code to check if the controller is in q-in-q mode. When the packet >>> has two vids, only the inner vid is passed onto the stack. The stack is never >>> made aware of the outer vid. >> >> But you're still using the outer VLAN group when passing the packet to >> the VLAN code, so the association to the correct VLAN device can't work. > > In the case where packet comes with two vlan ids, the rx descriptor contains > the inner vlan id and qnq is set to 1, the driver indicates this vid to the > stack. > In the case where packet comes with single vlan id, the rx descriptor > contains the outer vlan id and qnq is set to 0, the driver indicates this > packet as a non-vlan packet to the stack by calling netif_receive_skb. I understand that. But the driver does: if (vtp) { if (!adapter->vlan_grp || adapter->num_vlans == 0) { kfree_skb(skb); return; } vid = AMAP_GET_BITS(struct amap_eth_rx_compl, vlan_tag, rxcp); vid = be16_to_cpu(vid); vlan_hwaccel_receive_skb(skb, adapter->vlan_grp, vid); } adapter->vlan_grp will always be the VLAN group associated directly with the device, which is the group for the outer tag, not the inner one. So this can't properly associate packets with the correct 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
On 23/07/09 11:25 +0200, Patrick McHardy wrote: > Sarveshwar Bandi wrote: > > On 23/07/09 11:07 +0200, Patrick McHardy wrote: > >>> Patch has code to check if the controller is in q-in-q mode. When the packet > >>> has two vids, only the inner vid is passed onto the stack. The stack is never > >>> made aware of the outer vid. > >> > >> But you're still using the outer VLAN group when passing the packet to > >> the VLAN code, so the association to the correct VLAN device can't work. > > > > In the case where packet comes with two vlan ids, the rx descriptor contains > > the inner vlan id and qnq is set to 1, the driver indicates this vid to the > > stack. > > In the case where packet comes with single vlan id, the rx descriptor > > contains the outer vlan id and qnq is set to 0, the driver indicates this > > packet as a non-vlan packet to the stack by calling netif_receive_skb. > > I understand that. But the driver does: > > if (vtp) { > if (!adapter->vlan_grp || adapter->num_vlans == 0) { > kfree_skb(skb); > return; > } > vid = AMAP_GET_BITS(struct amap_eth_rx_compl, vlan_tag, rxcp); > vid = be16_to_cpu(vid); > vlan_hwaccel_receive_skb(skb, adapter->vlan_grp, vid); > } > > adapter->vlan_grp will always be the VLAN group associated directly with > the device, which is the group for the outer tag, not the inner one. So > this can't properly associate packets with the correct VLAN device. > In this case, vid is the inner vlan id in the packet. This is also the vlan id configured by vconfig. In the other case where packet had a single vlan tag, the following code will set vlanf to 0 (vtp now renamed to vlanf in the patch) and will be indicated as a non-vlan packet. if (adapter->qnq && !qnq) vlanf = 0; > -- > 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 -- 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
Sarveshwar Bandi wrote: > On 23/07/09 11:25 +0200, Patrick McHardy wrote: >> Sarveshwar Bandi wrote: >>> On 23/07/09 11:07 +0200, Patrick McHardy wrote: >>>>> Patch has code to check if the controller is in q-in-q mode. When the packet >>>>> has two vids, only the inner vid is passed onto the stack. The stack is never >>>>> made aware of the outer vid. >>>> But you're still using the outer VLAN group when passing the packet to >>>> the VLAN code, so the association to the correct VLAN device can't work. >>> In the case where packet comes with two vlan ids, the rx descriptor contains >>> the inner vlan id and qnq is set to 1, the driver indicates this vid to the >>> stack. >>> In the case where packet comes with single vlan id, the rx descriptor >>> contains the outer vlan id and qnq is set to 0, the driver indicates this >>> packet as a non-vlan packet to the stack by calling netif_receive_skb. >> I understand that. But the driver does: >> >> if (vtp) { >> if (!adapter->vlan_grp || adapter->num_vlans == 0) { >> kfree_skb(skb); >> return; >> } >> vid = AMAP_GET_BITS(struct amap_eth_rx_compl, vlan_tag, rxcp); >> vid = be16_to_cpu(vid); >> vlan_hwaccel_receive_skb(skb, adapter->vlan_grp, vid); >> } >> >> adapter->vlan_grp will always be the VLAN group associated directly with >> the device, which is the group for the outer tag, not the inner one. So >> this can't properly associate packets with the correct VLAN device. >> > In this case, vid is the inner vlan id in the packet. This is also the > vlan id configured by vconfig. So where does the outer tag come from then? Please provide an example how to configure this using Q-in-Q. > In the other case where packet had a single vlan tag, the following code > will set vlanf to 0 (vtp now renamed to vlanf in the patch) and will be > indicated as a non-vlan packet. > if (adapter->qnq && !qnq) > vlanf = 0; -- 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 23/07/09 11:48 +0200, Patrick McHardy wrote: > Sarveshwar Bandi wrote: > > On 23/07/09 11:25 +0200, Patrick McHardy wrote: > >> Sarveshwar Bandi wrote: > >>> On 23/07/09 11:07 +0200, Patrick McHardy wrote: > >>>>> Patch has code to check if the controller is in q-in-q mode. When the packet > >>>>> has two vids, only the inner vid is passed onto the stack. The stack is never > >>>>> made aware of the outer vid. > >>>> But you're still using the outer VLAN group when passing the packet to > >>>> the VLAN code, so the association to the correct VLAN device can't work. > >>> In the case where packet comes with two vlan ids, the rx descriptor contains > >>> the inner vlan id and qnq is set to 1, the driver indicates this vid to the > >>> stack. > >>> In the case where packet comes with single vlan id, the rx descriptor > >>> contains the outer vlan id and qnq is set to 0, the driver indicates this > >>> packet as a non-vlan packet to the stack by calling netif_receive_skb. > >> I understand that. But the driver does: > >> > >> if (vtp) { > >> if (!adapter->vlan_grp || adapter->num_vlans == 0) { > >> kfree_skb(skb); > >> return; > >> } > >> vid = AMAP_GET_BITS(struct amap_eth_rx_compl, vlan_tag, rxcp); > >> vid = be16_to_cpu(vid); > >> vlan_hwaccel_receive_skb(skb, adapter->vlan_grp, vid); > >> } > >> > >> adapter->vlan_grp will always be the VLAN group associated directly with > >> the device, which is the group for the outer tag, not the inner one. So > >> this can't properly associate packets with the correct VLAN device. > >> > > In this case, vid is the inner vlan id in the packet. This is also the > > vlan id configured by vconfig. > > So where does the outer tag come from then? Please provide an example > how to configure this using Q-in-Q. > The outer vlan is totally transparent to the host. It is used by the NIC to demux packets across multiple pci network functions. Currently the outer vlan tags are configured on the NIC by OEM provided utilities. > > In the other case where packet had a single vlan tag, the following code > > will set vlanf to 0 (vtp now renamed to vlanf in the patch) and will be > > indicated as a non-vlan packet. > > if (adapter->qnq && !qnq) > > vlanf = 0; > > -- > 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 -- 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
Sarveshwar Bandi wrote: > On 23/07/09 11:48 +0200, Patrick McHardy wrote: >> Sarveshwar Bandi wrote: >>> On 23/07/09 11:25 +0200, Patrick McHardy wrote: >>>> adapter->vlan_grp will always be the VLAN group associated directly with >>>> the device, which is the group for the outer tag, not the inner one. So >>>> this can't properly associate packets with the correct VLAN device. >>>> >>> In this case, vid is the inner vlan id in the packet. This is also the >>> vlan id configured by vconfig. >> >> So where does the outer tag come from then? Please provide an example >> how to configure this using Q-in-Q. >> > The outer vlan is totally transparent to the host. It is used by the NIC > to demux packets across multiple pci network functions. Currently the > outer vlan tags are configured on the NIC by OEM provided utilities. I see. A proper changelog entry would have explained that and avoided all this confusion. Not that I think using another tool for this is a good solution, but no objections from a functional POV. -- 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: Patrick McHardy <kaber@trash.net> Date: Thu, 23 Jul 2009 12:12:56 +0200 > Sarveshwar Bandi wrote: >> On 23/07/09 11:48 +0200, Patrick McHardy wrote: >>> Sarveshwar Bandi wrote: >>>> On 23/07/09 11:25 +0200, Patrick McHardy wrote: >>>>> adapter->vlan_grp will always be the VLAN group associated directly with >>>>> the device, which is the group for the outer tag, not the inner one. So >>>>> this can't properly associate packets with the correct VLAN device. >>>>> >>>> In this case, vid is the inner vlan id in the packet. This is also the >>>> vlan id configured by vconfig. >>> >>> So where does the outer tag come from then? Please provide an example >>> how to configure this using Q-in-Q. >>> >> The outer vlan is totally transparent to the host. It is used by the NIC >> to demux packets across multiple pci network functions. Currently the >> outer vlan tags are configured on the NIC by OEM provided utilities. > > I see. A proper changelog entry would have explained that and avoided > all this confusion. Not that I think using another tool for this is a > good solution, but no objections from a functional POV. Using OEM tools makes no sense, there should be something like an ethtool interface for changing this setting and appropriate changes to the common userland tools to provide access to them. I'm not putting this change in until there is common infrastructure submitted to common tools to control this configuration. -- 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 wrote: >> Sarveshwar Bandi wrote: >>> The outer vlan is totally transparent to the host. It is used by the NIC >>> to demux packets across multiple pci network functions. Currently the >>> outer vlan tags are configured on the NIC by OEM provided utilities. >> I see. A proper changelog entry would have explained that and avoided >> all this confusion. Not that I think using another tool for this is a >> good solution, but no objections from a functional POV. > > Using OEM tools makes no sense, there should be something like an > ethtool interface for changing this setting and appropriate changes > to the common userland tools to provide access to them. > > I'm not putting this change in until there is common infrastructure > submitted to common tools to control this configuration. Thanks, thats my opinion as well. But I think we should handle Q-in-Q using the VLAN netlink API and iproute instead of ethtool for consistency. -- 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: Patrick McHardy <kaber@trash.net> Date: Thu, 23 Jul 2009 18:33:05 +0200 > David Miller wrote: >>> Sarveshwar Bandi wrote: >>>> The outer vlan is totally transparent to the host. It is used by the NIC >>>> to demux packets across multiple pci network functions. Currently the >>>> outer vlan tags are configured on the NIC by OEM provided utilities. >>> I see. A proper changelog entry would have explained that and avoided >>> all this confusion. Not that I think using another tool for this is a >>> good solution, but no objections from a functional POV. >> >> Using OEM tools makes no sense, there should be something like an >> ethtool interface for changing this setting and appropriate changes >> to the common userland tools to provide access to them. >> >> I'm not putting this change in until there is common infrastructure >> submitted to common tools to control this configuration. > > Thanks, thats my opinion as well. But I think we should handle > Q-in-Q using the VLAN netlink API and iproute instead of ethtool > for consistency. That works for me too. -- 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/drivers/net/benet/be.h b/drivers/net/benet/be.h index 41cddbe..317b77d 100644 --- a/drivers/net/benet/be.h +++ b/drivers/net/benet/be.h @@ -270,6 +270,7 @@ struct be_adapter { bool link_up; u32 port_num; bool promiscuous; + bool qnq; }; extern struct ethtool_ops be_ethtool_ops; diff --git a/drivers/net/benet/be_cmds.c b/drivers/net/benet/be_cmds.c index 583517e..848a75b 100644 --- a/drivers/net/benet/be_cmds.c +++ b/drivers/net/benet/be_cmds.c @@ -1037,11 +1037,12 @@ int be_cmd_get_flow_control(struct be_ct return status; } -int be_cmd_query_fw_cfg(struct be_ctrl_info *ctrl, u32 *port_num) +int be_cmd_query_fw_cfg(struct be_ctrl_info *ctrl, u32 *port_num, bool *qnq) { struct be_mcc_wrb *wrb = wrb_from_mbox(&ctrl->mbox_mem); struct be_cmd_req_query_fw_cfg *req = embedded_payload(wrb); int status; + int mode; spin_lock(&ctrl->mbox_lock); @@ -1056,6 +1057,11 @@ int be_cmd_query_fw_cfg(struct be_ctrl_i if (!status) { struct be_cmd_resp_query_fw_cfg *resp = embedded_payload(wrb); *port_num = le32_to_cpu(resp->phys_port); + mode = le32_to_cpu(resp->function_mode); + if (mode & QNQ_MODE) + *qnq = true; + else + *qnq = false; } spin_unlock(&ctrl->mbox_lock); diff --git a/drivers/net/benet/be_cmds.h b/drivers/net/benet/be_cmds.h index 747626d..4cfe995 100644 --- a/drivers/net/benet/be_cmds.h +++ b/drivers/net/benet/be_cmds.h @@ -683,6 +683,7 @@ struct be_cmd_resp_modify_eq_delay { } __packed; /******************** Get FW Config *******************/ +#define QNQ_MODE 0x400 struct be_cmd_req_query_fw_cfg { struct be_cmd_req_hdr hdr; u32 rsvd[30]; @@ -744,5 +745,6 @@ extern int be_cmd_set_flow_control(struc u32 tx_fc, u32 rx_fc); extern int be_cmd_get_flow_control(struct be_ctrl_info *ctrl, u32 *tx_fc, u32 *rx_fc); -extern int be_cmd_query_fw_cfg(struct be_ctrl_info *ctrl, u32 *port_num); +extern int be_cmd_query_fw_cfg(struct be_ctrl_info *ctrl, u32 *port_num, + bool *qnq); extern void be_process_mcc(struct be_ctrl_info *ctrl); diff --git a/drivers/net/benet/be_main.c b/drivers/net/benet/be_main.c index 45df8e2..59c2a86 100644 --- a/drivers/net/benet/be_main.c +++ b/drivers/net/benet/be_main.c @@ -747,9 +747,16 @@ static void be_rx_compl_process(struct b struct be_eth_rx_compl *rxcp) { struct sk_buff *skb; - u32 vtp, vid; + u32 vlanf, vid; + u8 qnq; - vtp = AMAP_GET_BITS(struct amap_eth_rx_compl, vtp, rxcp); + vlanf = AMAP_GET_BITS(struct amap_eth_rx_compl, vtp, rxcp); + qnq = AMAP_GET_BITS(struct amap_eth_rx_compl, qnq, rxcp); + + /* In QnQ mode, if qnq is 0 it is a non-vlan packet + irrespective of vlanf */ + if (adapter->qnq && !qnq) + vlanf = 0; skb = netdev_alloc_skb(adapter->netdev, BE_HDR_LEN + NET_IP_ALIGN); if (!skb) { @@ -772,7 +779,7 @@ static void be_rx_compl_process(struct b skb->protocol = eth_type_trans(skb, adapter->netdev); skb->dev = adapter->netdev; - if (vtp) { + if (vlanf) { if (!adapter->vlan_grp || adapter->num_vlans == 0) { kfree_skb(skb); return; @@ -799,11 +806,18 @@ static void be_rx_compl_process_gro(stru struct be_eq_obj *eq_obj = &adapter->rx_eq; u32 num_rcvd, pkt_size, remaining, vlanf, curr_frag_len; u16 i, rxq_idx = 0, vid, j; + u8 qnq; num_rcvd = AMAP_GET_BITS(struct amap_eth_rx_compl, numfrags, rxcp); pkt_size = AMAP_GET_BITS(struct amap_eth_rx_compl, pktsize, rxcp); vlanf = AMAP_GET_BITS(struct amap_eth_rx_compl, vtp, rxcp); rxq_idx = AMAP_GET_BITS(struct amap_eth_rx_compl, fragndx, rxcp); + qnq = AMAP_GET_BITS(struct amap_eth_rx_compl, qnq, rxcp); + + /* In QnQ mode, if qnq is 0 it is a non-vlan packet + irrespective of vlanf */ + if (adapter->qnq && !qnq) + vlanf = 0; skb = napi_get_frags(&eq_obj->napi); if (!skb) { @@ -1851,7 +1865,7 @@ static int be_hw_up(struct be_adapter *a if (status) return status; - status = be_cmd_query_fw_cfg(ctrl, &adapter->port_num); + status = be_cmd_query_fw_cfg(ctrl, &adapter->port_num, &adapter->qnq); return status; }
Please review and apply patch to net-next tree. Patch implements the support for q-in-q mode. - Sarvesh Signed-off-by: Sarveshwar Bandi <sarveshwarb@serverengines.com> --- drivers/net/benet/be.h | 1 + drivers/net/benet/be_cmds.c | 8 +++++++- drivers/net/benet/be_cmds.h | 4 +++- drivers/net/benet/be_main.c | 22 ++++++++++++++++++---- 4 files changed, 29 insertions(+), 6 deletions(-)