On Mon 17 Jun 2013 03:48:37 PM PDT, John Fastabend wrote: > > On 6/13/2013 1:54 PM, Robert Love wrote: >> >> fcoe_xmit was coded such that it would skip the vlan net device/layer >> and instead set some vlan flags and transmit on the real net device. >> The real net device has code that would add the vlan tag for fcoe skbs. >> This avoids some extra processing for data frames and provides a small >> performance improvement. >> >> Since fcoe_xmit was not using the vlan net device, __vlan_put_tag >> within the real net device's xmit routine was ultimately being >> called to set the vlan tag. >> >> With the below change the behavior of __vlan_put_tag changed slightly, >> it now sets the skb->protocol = vlan_proto. vlan_proto was not a field >> being set by fcoe_xmit, so the skb->protocol is now not being set to >> ETH_P_8021Q, as it should be. >> >> This patch converts fcoe_xmit to use the vlan_put_tag routine which >> will tag the skb and fcoe will continue to transmit fcoe skbs on the >> real net device. >> >> For reference, the below change was the one that altered the >> __vlan_put_tag behavior. >> >> commit 86a9bad3ab6b6f858fd4443b48738cabbb6d094c >> Author: Patrick McHardy <[email protected]> >> Date: Fri Apr 19 02:04:30 2013 +0000 >> >> net: vlan: add protocol argument to packet tagging functions >> >> Add a protocol argument to the VLAN packet tagging functions. >> In case of HW >> tagging, we need that protocol available in the ndo_start_xmit >> functions, >> so it is stored in a new field in the skb. The new field fits >> into a hole >> (on 64 bit) and doesn't increase the sks's size. >> >> Signed-off-by: Robert Love <[email protected]> >> Acked-by: Neil Horman <[email protected]> >> Tested-by: Ross Brattain <[email protected]> >> --- >> drivers/scsi/fcoe/fcoe.c | 12 +++++------- >> 1 file changed, 5 insertions(+), 7 deletions(-) >> >> diff --git a/drivers/scsi/fcoe/fcoe.c b/drivers/scsi/fcoe/fcoe.c >> index fc7bb1f..cb7ac9c 100644 >> --- a/drivers/scsi/fcoe/fcoe.c >> +++ b/drivers/scsi/fcoe/fcoe.c >> @@ -1658,13 +1658,11 @@ static int fcoe_xmit(struct fc_lport *lport, >> struct fc_frame *fp) >> skb->protocol = htons(ETH_P_FCOE); >> skb->priority = fcoe->priority; >> >> - if (fcoe->netdev->priv_flags & IFF_802_1Q_VLAN && >> - fcoe->realdev->features & NETIF_F_HW_VLAN_CTAG_TX) { >> - skb->vlan_tci = VLAN_TAG_PRESENT | >> - vlan_dev_vlan_id(fcoe->netdev); >> - skb->dev = fcoe->realdev; >> - } else >> - skb->dev = fcoe->netdev; >> + /* must set skb->dev before calling vlan_put_tag */ >> + skb->dev = fcoe->realdev; > > > Should there be a check to see if a vlan tag is needed? I'm not sure > but is there ever a case where we really want to run over the real > device without any tag? > > With this new block a tag will always be inserted perhaps a zero id tag. > Good point. I don't want to change the behavior with this patch only adjust for the networking vlan changes.
>> >> + skb = vlan_put_tag(skb, htons(ETH_P_8021Q), >> vlan_dev_vlan_id(fcoe->netdev)); > > > Also and likely more importantly I think this may do a bad pointer > dereference if fcoe->netdev is not a vlan dev. Just take a look at > vlan_dev_vlan_id in ./net/8021q/vlan_core.c > Another good point. I've re-spun the patch and I've addressed both of your concerns. I'll send it out now. I've basically gone back to the previous if/else statement. The change now is that when it's a VLAN device I use vlan_put_tag instead of setting vlan_tci directly. Thanks, //Rob _______________________________________________ fcoe-devel mailing list [email protected] http://lists.open-fcoe.org/mailman/listinfo/fcoe-devel
