From: Shannon Nelson <shannon.nelson@oracle.com>
To: intel-wired-lan@osuosl.org
Subject: [Intel-wired-lan] [next PATCH S95 07/12] i40e: Unset promiscuous settings on VF reset
Date: Tue, 21 Aug 2018 09:28:04 -0700 [thread overview]
Message-ID: <731b5135-2bda-0eee-b668-ccfb540cfe67@oracle.com> (raw)
In-Reply-To: <20180820151233.14629-7-alice.michael@intel.com>
On 8/20/2018 8:12 AM, Alice Michael wrote:
> From: Mariusz Stachura <mariusz.stachura@intel.com>
>
> This patch cleans up promiscuous configuration when a VF reset occurs.
> Previously the promiscuous mode settings were still there after the VF
> driver removal.
>
> Signed-off-by: Mariusz Stachura <mariusz.stachura@intel.com>
> ---
> drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c | 267 ++++++++++++---------
> 1 file changed, 157 insertions(+), 110 deletions(-)
>
> diff --git a/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c b/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
> index f56c645..ca423be 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
> +++ b/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
> @@ -1084,6 +1084,136 @@ static int i40e_quiesce_vf_pci(struct i40e_vf *vf)
> return -EIO;
> }
>
> +static inline int i40e_getnum_vf_vsi_vlan_filters(struct i40e_vsi *vsi);
> +
> +/**
> + * i40e_config_vf_promiscuous_mode
> + * @vf: pointer to the VF info
> + * @vsi_id: VSI id
> + * @allmulti: set MAC L2 layer multicast promiscuous enable/disable
> + * @alluni: set MAC L2 layer unicast promiscuous enable/disable
> + *
> + * Called from the VF to configure the promiscuous mode of
> + * VF vsis and from the VF reset path to reset promiscuous mode.
> + **/
> +static i40e_status i40e_config_vf_promiscuous_mode(struct i40e_vf *vf,
> + u16 vsi_id,
> + bool allmulti,
> + bool alluni)
> +{
> + i40e_status aq_ret = 0;
> + struct i40e_pf *pf = vf->pf;
> + struct i40e_hw *hw = &pf->hw;
> + struct i40e_mac_filter *f;
> + struct i40e_vsi *vsi;
> + int bkt;
> +
> + vsi = i40e_find_vsi_from_id(pf, vsi_id);
> + if (!i40e_vc_isvalid_vsi_id(vf, vsi_id) || !vsi)
> + return I40E_ERR_PARAM;
> +
> + if (!test_bit(I40E_VIRTCHNL_VF_CAP_PRIVILEGE, &vf->vf_caps)) {
> + dev_err(&pf->pdev->dev,
> + "Unprivileged VF %d is attempting to configure promiscuous mode\n",
> + vf->vf_id);
> + /* Lie to the VF on purpose. */
Can you explain why in the comment?
> + return 0;
> + }
> +
> + if (vf->port_vlan_id) {
> + aq_ret = i40e_aq_set_vsi_mc_promisc_on_vlan(hw, vsi->seid,
> + allmulti,
> + vf->port_vlan_id,
> + NULL);
> + if (aq_ret) {
> + int aq_err = pf->hw.aq.asq_last_status;
> +
> + dev_err(&pf->pdev->dev,
> + "VF %d failed to set multicast promiscuous mode err %s aq_err %s\n",
> + vf->vf_id,
> + i40e_stat_str(&pf->hw, aq_ret),
> + i40e_aq_str(&pf->hw, aq_err));
> + return aq_ret;
> + }
> +
> + aq_ret = i40e_aq_set_vsi_uc_promisc_on_vlan(hw, vsi->seid,
> + alluni,
> + vf->port_vlan_id,
> + NULL);
> + if (aq_ret) {
> + int aq_err = pf->hw.aq.asq_last_status;
> +
> + dev_err(&pf->pdev->dev,
> + "VF %d failed to set unicast promiscuous mode err %s aq_err %s\n",
> + vf->vf_id,
> + i40e_stat_str(&pf->hw, aq_ret),
> + i40e_aq_str(&pf->hw, aq_err));
If uc_promisc fails, perhaps you should undo the mc_promisc change? Of
course, you might need to find out what it was before you set it to see
if there was a change requested.
> + }
> + return aq_ret;
> + } else if (i40e_getnum_vf_vsi_vlan_filters(vsi)) {
> + hash_for_each(vsi->mac_filter_hash, bkt, f, hlist) {
> + if (f->vlan < 0 || f->vlan > I40E_MAX_VLANID)
> + continue;
> + aq_ret = i40e_aq_set_vsi_mc_promisc_on_vlan(hw,
> + vsi->seid,
> + allmulti,
> + f->vlan,
> + NULL);
> + if (aq_ret) {
> + int aq_err = pf->hw.aq.asq_last_status;
> +
> + dev_err(&pf->pdev->dev,
> + "Could not add VLAN %d to multicast promiscuous domain err %s aq_err %s\n",
> + f->vlan,
> + i40e_stat_str(&pf->hw, aq_ret),
> + i40e_aq_str(&pf->hw, aq_err));
> + }
> +
> + aq_ret = i40e_aq_set_vsi_uc_promisc_on_vlan(hw,
> + vsi->seid,
> + alluni,
> + f->vlan,
> + NULL);
> + if (aq_ret) {
> + int aq_err = pf->hw.aq.asq_last_status;
> +
> + dev_err(&pf->pdev->dev,
> + "Could not add VLAN %d to Unicast promiscuous domain err %s aq_err %s\n",
> + f->vlan,
> + i40e_stat_str(&pf->hw, aq_ret),
> + i40e_aq_str(&pf->hw, aq_err));
Undo the mc_promisc change?
> + }
> + }
> + return aq_ret;
> + }
> + aq_ret = i40e_aq_set_vsi_multicast_promiscuous(hw, vsi->seid, allmulti,
> + NULL);
> + if (aq_ret) {
> + int aq_err = pf->hw.aq.asq_last_status;
> +
> + dev_err(&pf->pdev->dev,
> + "VF %d failed to set multicast promiscuous mode err %s aq_err %s\n",
> + vf->vf_id,
> + i40e_stat_str(&pf->hw, aq_ret),
> + i40e_aq_str(&pf->hw, aq_err));
> + return aq_ret;
> + }
> +
> + aq_ret = i40e_aq_set_vsi_unicast_promiscuous(hw, vsi->seid, alluni,
> + NULL, true);
> + if (aq_ret) {
> + int aq_err = pf->hw.aq.asq_last_status;
> +
> + dev_err(&pf->pdev->dev,
> + "VF %d failed to set unicast promiscuous mode err %s aq_err %s\n",
> + vf->vf_id,
> + i40e_stat_str(&pf->hw, aq_ret),
> + i40e_aq_str(&pf->hw, aq_err));
Undo the mc_promisc change?
> + }
> +
> + return aq_ret;
> +}
> +
> /**
> * i40e_trigger_vf_reset
> * @vf: pointer to the VF structure
> @@ -1145,6 +1275,9 @@ static void i40e_cleanup_reset_vf(struct i40e_vf *vf)
> struct i40e_hw *hw = &pf->hw;
> u32 reg;
>
> + /* disable promisc modes in case they were enabled */
> + i40e_config_vf_promiscuous_mode(vf, vf->lan_vsi_id, false, false);
> +
> /* free VF resources to begin resetting the VSI state */
> i40e_free_vf_res(vf);
>
> @@ -1851,132 +1984,46 @@ static int i40e_vc_config_promiscuous_mode_msg(struct i40e_vf *vf,
> struct virtchnl_promisc_info *info =
> (struct virtchnl_promisc_info *)msg;
> struct i40e_pf *pf = vf->pf;
> - struct i40e_hw *hw = &pf->hw;
> - struct i40e_mac_filter *f;
> i40e_status aq_ret = 0;
> bool allmulti = false;
> - struct i40e_vsi *vsi;
> bool alluni = false;
> - int aq_err = 0;
> - int bkt;
>
> - vsi = i40e_find_vsi_from_id(pf, info->vsi_id);
> - if (!test_bit(I40E_VF_STATE_ACTIVE, &vf->vf_states) ||
> - !i40e_vc_isvalid_vsi_id(vf, info->vsi_id) ||
> - !vsi) {
> - aq_ret = I40E_ERR_PARAM;
> - goto error_param;
> - }
> - if (!test_bit(I40E_VIRTCHNL_VF_CAP_PRIVILEGE, &vf->vf_caps)) {
> - dev_err(&pf->pdev->dev,
> - "Unprivileged VF %d is attempting to configure promiscuous mode\n",
> - vf->vf_id);
> - /* Lie to the VF on purpose. */
> - aq_ret = 0;
> - goto error_param;
> - }
> + if (!test_bit(I40E_VF_STATE_ACTIVE, &vf->vf_states))
> + return I40E_ERR_PARAM;
> +
> /* Multicast promiscuous handling*/
> if (info->flags & FLAG_VF_MULTICAST_PROMISC)
> allmulti = true;
>
> - if (vf->port_vlan_id) {
> - aq_ret = i40e_aq_set_vsi_mc_promisc_on_vlan(hw, vsi->seid,
> - allmulti,
> - vf->port_vlan_id,
> - NULL);
> - } else if (i40e_getnum_vf_vsi_vlan_filters(vsi)) {
> - hash_for_each(vsi->mac_filter_hash, bkt, f, hlist) {
> - if (f->vlan < 0 || f->vlan > I40E_MAX_VLANID)
> - continue;
> - aq_ret = i40e_aq_set_vsi_mc_promisc_on_vlan(hw,
> - vsi->seid,
> - allmulti,
> - f->vlan,
> - NULL);
> - aq_err = pf->hw.aq.asq_last_status;
> - if (aq_ret) {
> - dev_err(&pf->pdev->dev,
> - "Could not add VLAN %d to multicast promiscuous domain err %s aq_err %s\n",
> - f->vlan,
> - i40e_stat_str(&pf->hw, aq_ret),
> - i40e_aq_str(&pf->hw, aq_err));
> - break;
> - }
> - }
> - } else {
> - aq_ret = i40e_aq_set_vsi_multicast_promiscuous(hw, vsi->seid,
> - allmulti, NULL);
> - aq_err = pf->hw.aq.asq_last_status;
> - if (aq_ret) {
> - dev_err(&pf->pdev->dev,
> - "VF %d failed to set multicast promiscuous mode err %s aq_err %s\n",
> - vf->vf_id,
> - i40e_stat_str(&pf->hw, aq_ret),
> - i40e_aq_str(&pf->hw, aq_err));
> - goto error_param;
> - }
> - }
> -
> + if (info->flags & FLAG_VF_UNICAST_PROMISC)
> + alluni = true;
> + aq_ret = i40e_config_vf_promiscuous_mode(vf, info->vsi_id, allmulti,
> + alluni);
> if (!aq_ret) {
> - dev_info(&pf->pdev->dev,
> - "VF %d successfully set multicast promiscuous mode\n",
> - vf->vf_id);
> - if (allmulti)
> + if (allmulti) {
> + dev_info(&pf->pdev->dev,
> + "VF %d successfully set multicast promiscuous mode\n",
> + vf->vf_id);
> set_bit(I40E_VF_STATE_MC_PROMISC, &vf->vf_states);
> - else
> + } else {
> + dev_info(&pf->pdev->dev,
> + "VF %d successfully unset multicast promiscuous mode\n",
> + vf->vf_id);
> clear_bit(I40E_VF_STATE_MC_PROMISC, &vf->vf_states);
> - }
> -
> - if (info->flags & FLAG_VF_UNICAST_PROMISC)
> - alluni = true;
> - if (vf->port_vlan_id) {
> - aq_ret = i40e_aq_set_vsi_uc_promisc_on_vlan(hw, vsi->seid,
> - alluni,
> - vf->port_vlan_id,
> - NULL);
> - } else if (i40e_getnum_vf_vsi_vlan_filters(vsi)) {
> - hash_for_each(vsi->mac_filter_hash, bkt, f, hlist) {
> - if (f->vlan < 0 || f->vlan > I40E_MAX_VLANID)
> - continue;
> - aq_ret = i40e_aq_set_vsi_uc_promisc_on_vlan(hw,
> - vsi->seid,
> - alluni,
> - f->vlan,
> - NULL);
> - aq_err = pf->hw.aq.asq_last_status;
> - if (aq_ret)
> - dev_err(&pf->pdev->dev,
> - "Could not add VLAN %d to Unicast promiscuous domain err %s aq_err %s\n",
> - f->vlan,
> - i40e_stat_str(&pf->hw, aq_ret),
> - i40e_aq_str(&pf->hw, aq_err));
> }
> - } else {
> - aq_ret = i40e_aq_set_vsi_unicast_promiscuous(hw, vsi->seid,
> - alluni, NULL,
> - true);
> - aq_err = pf->hw.aq.asq_last_status;
> - if (aq_ret) {
> - dev_err(&pf->pdev->dev,
> - "VF %d failed to set unicast promiscuous mode %8.8x err %s aq_err %s\n",
> - vf->vf_id, info->flags,
> - i40e_stat_str(&pf->hw, aq_ret),
> - i40e_aq_str(&pf->hw, aq_err));
> - goto error_param;
> - }
> - }
> -
> - if (!aq_ret) {
> - dev_info(&pf->pdev->dev,
> - "VF %d successfully set unicast promiscuous mode\n",
> - vf->vf_id);
> - if (alluni)
> + if (alluni) {
> + dev_info(&pf->pdev->dev,
> + "VF %d successfully set unicast promiscuous mode\n",
> + vf->vf_id);
> set_bit(I40E_VF_STATE_UC_PROMISC, &vf->vf_states);
> - else
> + } else {
> + dev_info(&pf->pdev->dev,
> + "VF %d successfully unset unicast promiscuous mode\n",
> + vf->vf_id);
It looks to me that all these status messages are getting printed
whether there actually was a change or not. In other words, if the
unicast promisc was already on, and the VF again calls for it to be set,
this will be printed again, even though there was no change in state.
Is this what you intended?
sln
> clear_bit(I40E_VF_STATE_UC_PROMISC, &vf->vf_states);
> + }
> }
>
> -error_param:
> /* send the response to the VF */
> return i40e_vc_send_resp_to_vf(vf,
> VIRTCHNL_OP_CONFIG_PROMISCUOUS_MODE,
>
next prev parent reply other threads:[~2018-08-21 16:28 UTC|newest]
Thread overview: 29+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-08-20 15:12 [Intel-wired-lan] [next PATCH S95 01/12] i40e: Fix VF's link state notification Alice Michael
2018-08-20 15:12 ` [Intel-wired-lan] [next PATCH S95 02/12] i40e: convert queue stats to i40e_stats array Alice Michael
2018-08-21 19:16 ` Bowers, AndrewX
2018-08-20 15:12 ` [Intel-wired-lan] [next PATCH S95 03/12] i40e: move ethtool stats boiler plate code to i40e_ethtool_stats.h Alice Michael
2018-08-21 19:17 ` Bowers, AndrewX
2018-08-20 15:12 ` [Intel-wired-lan] [next PATCH S95 04/12] i40evf: update ethtool stats code and use helper functions Alice Michael
2018-08-21 19:19 ` Bowers, AndrewX
2018-08-20 15:12 ` [Intel-wired-lan] [next PATCH S95 05/12] i40evf: Change a VF mac without reloading the VF driver Alice Michael
2018-08-21 19:29 ` Bowers, AndrewX
2018-08-29 16:34 ` Bowers, AndrewX
2018-08-20 15:12 ` [Intel-wired-lan] [next PATCH S95 06/12] i40e: fix condition of WARN_ONCE for stat strings Alice Michael
2018-08-21 19:36 ` Bowers, AndrewX
2018-08-24 1:50 ` Mauro Rodrigues
2018-08-20 15:12 ` [Intel-wired-lan] [next PATCH S95 07/12] i40e: Unset promiscuous settings on VF reset Alice Michael
2018-08-21 16:28 ` Shannon Nelson [this message]
2018-08-29 20:53 ` Bowers, AndrewX
2018-08-20 15:12 ` [Intel-wired-lan] [next PATCH S95 08/12] i40evf: Validate the number of queues a PF sends Alice Michael
2018-08-29 20:54 ` Bowers, AndrewX
2018-08-20 15:12 ` [Intel-wired-lan] [next PATCH S95 09/12] i40e: use correct length for strncpy Alice Michael
2018-08-21 16:28 ` Shannon Nelson
2018-08-23 16:11 ` Bowers, AndrewX
2018-08-20 15:12 ` [Intel-wired-lan] [next PATCH S95 10/12] i40evf: set IFF_UNICAST_FLT flag for the vf Alice Michael
2018-08-21 19:41 ` Bowers, AndrewX
2018-08-20 15:12 ` [Intel-wired-lan] [next PATCH S95 11/12] virtchnl: use u8 type for a field in the virtchnl_filter struct Alice Michael
2018-08-21 19:41 ` Bowers, AndrewX
2018-08-20 15:12 ` [Intel-wired-lan] [next PATCH S95 12/12] i40e: static analysis report from community Alice Michael
2018-08-21 19:42 ` Bowers, AndrewX
2018-08-21 16:27 ` [Intel-wired-lan] [next PATCH S95 01/12] i40e: Fix VF's link state notification Shannon Nelson
2018-08-29 20:53 ` Bowers, AndrewX
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=731b5135-2bda-0eee-b668-ccfb540cfe67@oracle.com \
--to=shannon.nelson@oracle.com \
--cc=intel-wired-lan@osuosl.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox