From mboxrd@z Thu Jan 1 00:00:00 1970 From: Keller, Jacob E Date: Tue, 23 Jun 2015 18:26:13 +0000 Subject: [Intel-wired-lan] [net-next 10/10] fm10k: fix iov_msg_mac_vlan_pf VID checks In-Reply-To: <5584EEF2.1040203@gmail.com> References: <1434757059-3384-1-git-send-email-jacob.e.keller@intel.com> <1434757059-3384-10-git-send-email-jacob.e.keller@intel.com> <5584EEF2.1040203@gmail.com> Message-ID: <1435083973.20240.14.camel@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: intel-wired-lan@osuosl.org List-ID: On Fri, 2015-06-19 at 21:41 -0700, Alexander Duyck wrote: > On 06/19/2015 04:37 PM, Jacob Keller wrote: > > The VF will send a message to request multicast addresses with the > > default vid. In the current code, if the PF has statically assigned > > a > > VLAN to a VF, then the VF will not get the multicast addresses. Fix > > up > > all of the various vlan messages to use identical checks (since > > each > > check was different). Also use set as a variable, so that it > > simplifies > > our check for whether vlan matches the pf_vid. > > > > The new logic will allow set of a vlan if it is zero, automatically > > converting to the default vid. Otherwise it will allow setting the > > PF > > vid, or any VLAN if PF has not statically assigned a VLAN. This is > > consistent behavior, and allows VF to request either 0 or the > > default_vid without silently failing. > > > > Note that we need the check for zero since VFs might not get the > > default > > VID message in time to actually request non-zero VLANs. > > > > Signed-off-by: Jacob Keller > > --- > > drivers/net/ethernet/intel/fm10k/fm10k_pf.c | 37 > > +++++++++++++++++++---------- > > 1 file changed, 25 insertions(+), 12 deletions(-) > > > > diff --git a/drivers/net/ethernet/intel/fm10k/fm10k_pf.c > > b/drivers/net/ethernet/intel/fm10k/fm10k_pf.c > > index d806d87a6192..9bb57531b5db 100644 > > --- a/drivers/net/ethernet/intel/fm10k/fm10k_pf.c > > +++ b/drivers/net/ethernet/intel/fm10k/fm10k_pf.c > > @@ -1168,9 +1168,10 @@ s32 fm10k_iov_msg_mac_vlan_pf(struct > > fm10k_hw *hw, u32 **results, > > struct fm10k_mbx_info *mbx) > > { > > struct fm10k_vf_info *vf_info = (struct fm10k_vf_info > > *)mbx; > > - int err = 0; > > u8 mac[ETH_ALEN]; > > u32 *result; > > + int err = 0; > > + bool set; > > u16 vlan; > > u32 vid; > > > > @@ -1186,19 +1187,25 @@ s32 fm10k_iov_msg_mac_vlan_pf(struct > > fm10k_hw *hw, u32 **results, > > if (err) > > return err; > > > > + /* verify upper 16 bits are zero */ > > + if (vid >> 16) > > + return FM10K_ERR_PARAM; > > + > > + set = !(vid & FM10K_VLAN_CLEAR); > > + vid &= ~FM10K_VLAN_CLEAR; > > + > > /* if VLAN ID is 0, set the default VLAN ID > > instead of 0 */ > > - if (!vid || (vid == FM10K_VLAN_CLEAR)) { > > + if (!vid) { > > if (vf_info->pf_vid) > > vid |= vf_info->pf_vid; > > else > > vid |= vf_info->sw_vid; > > - } else if (vid != vf_info->pf_vid) { > > + } else if (vf_info->pf_vid && vid != vf_info > > ->pf_vid) { > > return FM10K_ERR_PARAM; > > } > > > > /* update VSI info for VF in regards to VLAN table > > */ > > - err = hw->mac.ops.update_vlan(hw, vid, vf_info > > ->vsi, > > - !(vid & > > FM10K_VLAN_CLEAR)); > > + err = hw->mac.ops.update_vlan(hw, vid, vf_info > > ->vsi, set); > > } > > > > if (!err && !!results[FM10K_MAC_VLAN_MSG_MAC]) { > > @@ -1214,19 +1221,22 @@ s32 fm10k_iov_msg_mac_vlan_pf(struct > > fm10k_hw *hw, u32 **results, > > memcmp(mac, vf_info->mac, ETH_ALEN)) > > return FM10K_ERR_PARAM; > > > > + set = !(vlan & FM10K_VLAN_CLEAR); > > + vlan &= ~FM10K_VLAN_CLEAR; > > + > > /* if VLAN ID is 0, set the default VLAN ID > > instead of 0 */ > > - if (!vlan || (vlan == FM10K_VLAN_CLEAR)) { > > + if (!vlan) { > > if (vf_info->pf_vid) > > vlan |= vf_info->pf_vid; > > else > > vlan |= vf_info->sw_vid; > > - } else if (vf_info->pf_vid) { > > + } else if (vf_info->pf_vid && vlan != vf_info > > ->pf_vid) { > > return FM10K_ERR_PARAM; > > } > > > > /* notify switch of request for new unicast > > address */ > > - err = hw->mac.ops.update_uc_addr(hw, vf_info > > ->glort, mac, vlan, > > - !(vlan & > > FM10K_VLAN_CLEAR), 0); > > + err = hw->mac.ops.update_uc_addr(hw, vf_info > > ->glort, > > + mac, vlan, set, > > 0); > > } > > > > if (!err && !!results[FM10K_MAC_VLAN_MSG_MULTICAST]) { > > @@ -1241,19 +1251,22 @@ s32 fm10k_iov_msg_mac_vlan_pf(struct > > fm10k_hw *hw, u32 **results, > > if (!(vf_info->vf_flags & > > FM10K_VF_FLAG_MULTI_ENABLED)) > > return FM10K_ERR_PARAM; > > > > + set = !(vlan & FM10K_VLAN_CLEAR); > > + vlan &= ~FM10K_VLAN_CLEAR; > > + > > /* if VLAN ID is 0, set the default VLAN ID > > instead of 0 */ > > if (!vlan || (vlan == FM10K_VLAN_CLEAR)) { > > You missed a spot here. This should be if(!vlan) since you already > cleared the FM10K_VLAN_CLEAR flag. > Oops yep. > > if (vf_info->pf_vid) > > vlan |= vf_info->pf_vid; > > else > > vlan |= vf_info->sw_vid; > > - } else if (vf_info->pf_vid) { > > + } else if (vf_info->pf_vid && vlan != vf_info > > ->pf_vid) { > > return FM10K_ERR_PARAM; > > } > > > > /* notify switch of request for new multicast > > address */ > > - err = hw->mac.ops.update_mc_addr(hw, vf_info > > ->glort, mac, vlan, > > - !(vlan & > > FM10K_VLAN_CLEAR)); > > + err = hw->mac.ops.update_mc_addr(hw, vf_info > > ->glort, > > + mac, vlan, set); > > } > > > > return err; > > > > Really since the code is so identical you might just want to create a > > function to do all of this. You could have it return a value > representing the VLAN ID if >=0, or error if < 0. > I tried that first, but the main issue was different types. I wasn't sure how type-casting from a 32 to a 16 would do the right thing. For some flows we have a 16bit value, for others a 32bit value. (since it stores extra data). Actually, just casting it to 16 should work right? and then we can have the function just take a 16bit. I'll look at that. Regards, Jake