From: Jacob Keller <jacob.e.keller@intel.com>
To: Willem de Bruijn <willemdebruijn.kernel@gmail.com>,
Tony Nguyen <anthony.l.nguyen@intel.com>
Cc: David Miller <davem@davemloft.net>,
Jakub Kicinski <kuba@kernel.org>,
Aleksandr Loktionov <aleksandr.loktionov@intel.com>,
Network Development <netdev@vger.kernel.org>,
sassmann@redhat.com,
Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>,
Konrad Jankowski <konrad0.jankowski@intel.com>
Subject: Re: [PATCH net 4/4] i40e: Revert "i40e: don't report link up for a VF who hasn't enabled queues"
Date: Fri, 29 Jan 2021 16:09:20 -0800 [thread overview]
Message-ID: <e27cb35b-a413-ccdd-fa42-d65e7162747f@intel.com> (raw)
In-Reply-To: <CA+FuTScbEK+1NBUNCbHNnwOoSB0JtsEv3wEisYAbm082P+K0Rw@mail.gmail.com>
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
next prev parent reply other threads:[~2021-01-30 0:10 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-01-28 21:38 [PATCH net 0/4][pull request] Intel Wired LAN Driver Updates 2021-01-28 Tony Nguyen
2021-01-28 21:38 ` [PATCH net 1/4] igc: Report speed and duplex as unknown when device is runtime suspended Tony Nguyen
2021-01-30 6:22 ` Jakub Kicinski
2021-01-30 14:00 ` Neftin, Sasha
2021-01-30 18:12 ` Jakub Kicinski
2021-01-31 10:22 ` Neftin, Sasha
2021-02-01 22:04 ` Jakub Kicinski
2021-01-28 21:38 ` [PATCH net 2/4] igc: set the default return value to -IGC_ERR_NVM in igc_write_nvm_srwr Tony Nguyen
2021-01-28 21:38 ` [PATCH net 3/4] igc: check return value of ret_val in igc_config_fc_after_link_up Tony Nguyen
2021-01-28 21:38 ` [PATCH net 4/4] i40e: Revert "i40e: don't report link up for a VF who hasn't enabled queues" Tony Nguyen
2021-01-29 20:23 ` Willem de Bruijn
2021-01-30 0:09 ` Jacob Keller [this message]
2021-01-30 2:00 ` Willem de Bruijn
2021-01-30 6:18 ` Jakub Kicinski
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=e27cb35b-a413-ccdd-fa42-d65e7162747f@intel.com \
--to=jacob.e.keller@intel.com \
--cc=aleksandr.loktionov@intel.com \
--cc=anthony.l.nguyen@intel.com \
--cc=arkadiusz.kubalewski@intel.com \
--cc=davem@davemloft.net \
--cc=konrad0.jankowski@intel.com \
--cc=kuba@kernel.org \
--cc=netdev@vger.kernel.org \
--cc=sassmann@redhat.com \
--cc=willemdebruijn.kernel@gmail.com \
/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;
as well as URLs for NNTP newsgroup(s).