| Message ID | 20210128213851.2499012-5-anthony.l.nguyen@intel.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Intel Wired LAN Driver Updates 2021-01-28 | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for net |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | warning | 4 maintainers not CCed: jesse.brandeburg@intel.com jeffrey.t.kirsher@intel.com jacob.e.keller@intel.com intel-wired-lan@lists.osuosl.org |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 7 this patch: 7 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 44 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 7 this patch: 7 |
| netdev/header_inline | success | Link |
| netdev/stable | success | Stable not CCed |
On Thu, Jan 28, 2021 at 4:45 PM Tony Nguyen <anthony.l.nguyen@intel.com> wrote: > > From: Aleksandr Loktionov <aleksandr.loktionov@intel.com> > > This reverts commit 2ad1274fa35ace5c6360762ba48d33b63da2396c > > VF queues were not brought up when PF was brought up after being > downed if the VF driver disabled VFs queues during PF down. > This could happen in some older or external VF driver implementations. > The problem was that PF driver used vf->queues_enabled as a condition > to decide what link-state it would send out which caused the issue. > > Remove the check for vf->queues_enabled in the VF link notify. > Now VF will always be notified of the current link status. > Also remove the queues_enabled member from i40e_vf structure as it is > not used anymore. Otherwise VNF implementation was broken and caused > a link flap. > > Fixes: 2ad1274fa35a ("i40e: don't report link up for a VF who hasn't enabled") > Signed-off-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com> > Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com> > Tested-by: Konrad Jankowski <konrad0.jankowski@intel.com> > Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com> Doesn't this reintroduce the bug that the original patch aimed to solve? Commit 2ad1274fa35a itself was also a fix.
On 1/29/2021 12:23 PM, Willem de Bruijn wrote: > On Thu, Jan 28, 2021 at 4:45 PM Tony Nguyen <anthony.l.nguyen@intel.com> wrote: >> >> From: Aleksandr Loktionov <aleksandr.loktionov@intel.com> >> >> This reverts commit 2ad1274fa35ace5c6360762ba48d33b63da2396c >> >> VF queues were not brought up when PF was brought up after being >> downed if the VF driver disabled VFs queues during PF down. >> This could happen in some older or external VF driver implementations. >> The problem was that PF driver used vf->queues_enabled as a condition >> to decide what link-state it would send out which caused the issue. >> >> Remove the check for vf->queues_enabled in the VF link notify. >> Now VF will always be notified of the current link status. >> Also remove the queues_enabled member from i40e_vf structure as it is >> not used anymore. Otherwise VNF implementation was broken and caused >> a link flap. >> >> Fixes: 2ad1274fa35a ("i40e: don't report link up for a VF who hasn't enabled") >> Signed-off-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com> >> Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com> >> Tested-by: Konrad Jankowski <konrad0.jankowski@intel.com> >> Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com> > > Doesn't this reintroduce the bug that the original patch aimed to solve? > > Commit 2ad1274fa35a itself was also a fix. > Yea this might re-introduce the issue described in that commit. However I believe the bug in question was due to very old versions of VF drivers, (including an ancient version of FreeBSD if I recall). Perhaps there is some better mechanism for handling this, but I think reverting this is ok given that it causes problems in certain situations where the link status wasn't reported properly. Maybe there is a solution for both cases? but I would worry less about an issue with the incredibly old VFs because we know that the issue is fixed in newer VF code and the real problem is that the VF driver is incorrectly assuming link up means it is ready to send. Thus, I am comfortable with this revert: It simplifies the state for both the PF and VF. I would be open to alternatives as long as the issue described here is also fixed. Caveat: I was not involved in the decision to revert this and wasn't aware of it until now, so I almost certainly have out of date information. Thanks, Jake
On Fri, Jan 29, 2021 at 7:09 PM Jacob Keller <jacob.e.keller@intel.com> wrote: > > > > On 1/29/2021 12:23 PM, Willem de Bruijn wrote: > > On Thu, Jan 28, 2021 at 4:45 PM Tony Nguyen <anthony.l.nguyen@intel.com> wrote: > >> > >> From: Aleksandr Loktionov <aleksandr.loktionov@intel.com> > >> > >> This reverts commit 2ad1274fa35ace5c6360762ba48d33b63da2396c > >> > >> VF queues were not brought up when PF was brought up after being > >> downed if the VF driver disabled VFs queues during PF down. > >> This could happen in some older or external VF driver implementations. > >> The problem was that PF driver used vf->queues_enabled as a condition > >> to decide what link-state it would send out which caused the issue. > >> > >> Remove the check for vf->queues_enabled in the VF link notify. > >> Now VF will always be notified of the current link status. > >> Also remove the queues_enabled member from i40e_vf structure as it is > >> not used anymore. Otherwise VNF implementation was broken and caused > >> a link flap. > >> > >> Fixes: 2ad1274fa35a ("i40e: don't report link up for a VF who hasn't enabled") > >> Signed-off-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com> > >> Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com> > >> Tested-by: Konrad Jankowski <konrad0.jankowski@intel.com> > >> Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com> > > > > Doesn't this reintroduce the bug that the original patch aimed to solve? > > > > Commit 2ad1274fa35a itself was also a fix. > > > > Yea this might re-introduce the issue described in that commit. However > I believe the bug in question was due to very old versions of VF > drivers, (including an ancient version of FreeBSD if I recall). > > Perhaps there is some better mechanism for handling this, but I think > reverting this is ok given that it causes problems in certain situations > where the link status wasn't reported properly. > > Maybe there is a solution for both cases? but I would worry less about > an issue with the incredibly old VFs because we know that the issue is > fixed in newer VF code and the real problem is that the VF driver is > incorrectly assuming link up means it is ready to send. > > Thus, I am comfortable with this revert: It simplifies the state for > both the PF and VF. > > I would be open to alternatives as long as the issue described here is > also fixed. > > Caveat: I was not involved in the decision to revert this and wasn't > aware of it until now, so I almost certainly have out of date information. That's reasonable. The original patch is over three years old. If it is considered safe to revert now, I would just articulate that point in the commit.
On Fri, 29 Jan 2021 21:00:02 -0500 Willem de Bruijn wrote: > On Fri, Jan 29, 2021 at 7:09 PM Jacob Keller <jacob.e.keller@intel.com> wrote: > > Yea this might re-introduce the issue described in that commit. However > > I believe the bug in question was due to very old versions of VF > > drivers, (including an ancient version of FreeBSD if I recall). > > > > Perhaps there is some better mechanism for handling this, but I think > > reverting this is ok given that it causes problems in certain situations > > where the link status wasn't reported properly. > > > > Maybe there is a solution for both cases? but I would worry less about > > an issue with the incredibly old VFs because we know that the issue is > > fixed in newer VF code and the real problem is that the VF driver is > > incorrectly assuming link up means it is ready to send. > > > > Thus, I am comfortable with this revert: It simplifies the state for > > both the PF and VF. > > > > I would be open to alternatives as long as the issue described here is > > also fixed. > > > > Caveat: I was not involved in the decision to revert this and wasn't > > aware of it until now, so I almost certainly have out of date information. > > That's reasonable. The original patch is over three years old. > > If it is considered safe to revert now, I would just articulate that > point in the commit. Agreed. I'd call out that the original fix was a work around for clearly buggy client drivers, and they had enough time to be fixed.
diff --git a/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c b/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c index 7efc61aacb0a..1b6ec9be155a 100644 --- a/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c +++ b/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c @@ -55,12 +55,7 @@ static void i40e_vc_notify_vf_link_state(struct i40e_vf *vf) pfe.event = VIRTCHNL_EVENT_LINK_CHANGE; pfe.severity = PF_EVENT_SEVERITY_INFO; - - /* Always report link is down if the VF queues aren't enabled */ - if (!vf->queues_enabled) { - pfe.event_data.link_event.link_status = false; - pfe.event_data.link_event.link_speed = 0; - } else if (vf->link_forced) { + if (vf->link_forced) { pfe.event_data.link_event.link_status = vf->link_up; pfe.event_data.link_event.link_speed = (vf->link_up ? i40e_virtchnl_link_speed(ls->link_speed) : 0); @@ -70,7 +65,6 @@ static void i40e_vc_notify_vf_link_state(struct i40e_vf *vf) pfe.event_data.link_event.link_speed = i40e_virtchnl_link_speed(ls->link_speed); } - i40e_aq_send_msg_to_vf(hw, abs_vf_id, VIRTCHNL_OP_EVENT, 0, (u8 *)&pfe, sizeof(pfe), NULL); } @@ -2443,8 +2437,6 @@ static int i40e_vc_enable_queues_msg(struct i40e_vf *vf, u8 *msg) } } - vf->queues_enabled = true; - error_param: /* send the response to the VF */ return i40e_vc_send_resp_to_vf(vf, VIRTCHNL_OP_ENABLE_QUEUES, @@ -2466,9 +2458,6 @@ static int i40e_vc_disable_queues_msg(struct i40e_vf *vf, u8 *msg) struct i40e_pf *pf = vf->pf; i40e_status aq_ret = 0; - /* Immediately mark queues as disabled */ - vf->queues_enabled = false; - if (!test_bit(I40E_VF_STATE_ACTIVE, &vf->vf_states)) { aq_ret = I40E_ERR_PARAM; goto error_param; diff --git a/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.h b/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.h index 5491215d81de..091e32c1bb46 100644 --- a/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.h +++ b/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.h @@ -98,7 +98,6 @@ struct i40e_vf { unsigned int tx_rate; /* Tx bandwidth limit in Mbps */ bool link_forced; bool link_up; /* only valid if VF link is forced */ - bool queues_enabled; /* true if the VF queues are enabled */ bool spoofchk; u16 num_vlan;