From: Tony Nguyen <anthony.l.nguyen@intel.com> Date: 2021-01-28 21:39:19
This series contains updates to igc and i40e drivers.
Kai-Heng Feng fixes igc to report unknown speed and duplex during suspend
as an attempted read will cause errors.
Kevin Lo sets the default value to -IGC_ERR_NVM instead of success for
writing shadow RAM as this could miss a timeout. Also propagates the return
value for Flow Control configuration to properly pass on errors for igc.
Aleksandr reverts commit 2ad1274fa35a ("i40e: don't report link up for a VF
who hasn't enabled") as this can cause link flapping.
The following are changes since commit 44a674d6f79867d5652026f1cc11f7ba8a390183:
Merge tag 'mlx5-fixes-2021-01-26' of git://git.kernel.org/pub/scm/linux/kernel/git/saeed/linux
and are available in the git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/tnguy/net-queue 1GbE
Aleksandr Loktionov (1):
i40e: Revert "i40e: don't report link up for a VF who hasn't enabled
queues"
Kai-Heng Feng (1):
igc: Report speed and duplex as unknown when device is runtime
suspended
Kevin Lo (2):
igc: set the default return value to -IGC_ERR_NVM in
igc_write_nvm_srwr
igc: check return value of ret_val in igc_config_fc_after_link_up
drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c | 13 +------------
drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.h | 1 -
drivers/net/ethernet/intel/igc/igc_ethtool.c | 3 ++-
drivers/net/ethernet/intel/igc/igc_i225.c | 3 +--
drivers/net/ethernet/intel/igc/igc_mac.c | 2 +-
5 files changed, 5 insertions(+), 17 deletions
--
2.26.2
From: Tony Nguyen <anthony.l.nguyen@intel.com> Date: 2021-01-28 21:39:53
From: Kevin Lo <redacted>
Check return value from ret_val to make error check actually work.
Fixes: 4eb8080143a9 ("igc: Add setup link functionality")
Signed-off-by: Kevin Lo <redacted>
Acked-by: Sasha Neftin <redacted>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
drivers/net/ethernet/intel/igc/igc_mac.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
From: Tony Nguyen <anthony.l.nguyen@intel.com> Date: 2021-01-28 21:39:53
From: Kevin Lo <redacted>
This patch sets the default return value to -IGC_ERR_NVM in
igc_write_nvm_srwr. Without this change it wouldn't lead to a shadow RAM
write EEWR timeout.
Fixes: ab4056126813 ("igc: Add NVM support")
Signed-off-by: Kevin Lo <redacted>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
drivers/net/ethernet/intel/igc/igc_i225.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
@@ -219,9 +219,9 @@ static s32 igc_write_nvm_srwr(struct igc_hw *hw, u16 offset, u16 words,u16*data){structigc_nvm_info*nvm=&hw->nvm;+s32ret_val=-IGC_ERR_NVM;u32attempts=100000;u32i,k,eewr=0;-s32ret_val=0;/* A check for invalid values: offset too large, too many words,*toomanywordsfortheoffset,andnotenoughwords.
@@ -229,7 +229,6 @@ static s32 igc_write_nvm_srwr(struct igc_hw *hw, u16 offset, u16 words,if(offset>=nvm->word_size||(words>(nvm->word_size-offset))||words==0){hw_dbg("nvm parameter(s) out of bounds\n");-ret_val=-IGC_ERR_NVM;gotoout;}
From: Tony Nguyen <anthony.l.nguyen@intel.com> Date: 2021-01-28 21:39:53
From: Aleksandr Loktionov <redacted>
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 <redacted>
Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>
Tested-by: Konrad Jankowski <redacted>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c | 13 +------------
drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.h | 1 -
2 files changed, 1 insertion(+), 13 deletions(-)
@@ -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;-}elseif(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);
@@ -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 */returni40e_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)structi40e_pf*pf=vf->pf;i40e_statusaq_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;gotoerror_param;
@@ -98,7 +98,6 @@ struct i40e_vf {unsignedinttx_rate;/* Tx bandwidth limit in Mbps */boollink_forced;boollink_up;/* only valid if VF link is forced */-boolqueues_enabled;/* true if the VF queues are enabled */boolspoofchk;u16num_vlan;
From: Willem de Bruijn <willemdebruijn.kernel@gmail.com> Date: 2021-01-29 20:26:41
On Thu, Jan 28, 2021 at 4:45 PM Tony Nguyen [off-list ref] wrote:
From: Aleksandr Loktionov <redacted>
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 <redacted>
Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>
Tested-by: Konrad Jankowski <redacted>
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.
From: Jacob Keller <jacob.e.keller@intel.com> Date: 2021-01-30 00:10:28
On 1/29/2021 12:23 PM, Willem de Bruijn wrote:
On Thu, Jan 28, 2021 at 4:45 PM Tony Nguyen [off-list ref] wrote:
quoted
From: Aleksandr Loktionov <redacted>
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 <redacted>
Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>
Tested-by: Konrad Jankowski <redacted>
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
From: Willem de Bruijn <willemdebruijn.kernel@gmail.com> Date: 2021-01-30 09:05:51
On Fri, Jan 29, 2021 at 7:09 PM Jacob Keller [off-list ref] wrote:
On 1/29/2021 12:23 PM, Willem de Bruijn wrote:
quoted
On Thu, Jan 28, 2021 at 4:45 PM Tony Nguyen [off-list ref] wrote:
quoted
From: Aleksandr Loktionov <redacted>
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 <redacted>
Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>
Tested-by: Konrad Jankowski <redacted>
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.
From: Jakub Kicinski <kuba@kernel.org> Date: 2021-01-30 09:40:20
On Fri, 29 Jan 2021 21:00:02 -0500 Willem de Bruijn wrote:
On Fri, Jan 29, 2021 at 7:09 PM Jacob Keller [off-list ref] wrote:
quoted
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.
From: Jakub Kicinski <kuba@kernel.org> Date: 2021-01-30 09:45:42
On Thu, 28 Jan 2021 13:38:48 -0800 Tony Nguyen wrote:
From: Kai-Heng Feng <redacted>
Similar to commit 165ae7a8feb5 ("igb: Report speed and duplex as unknown
when device is runtime suspended"), if we try to read speed and duplex
sysfs while the device is runtime suspended, igc will complain and
stops working:
The more generic approach will be wrap get_link_ksettings() with begin()
and complete() callbacks, and calls runtime resume and runtime suspend
routine respectively. However, igc is like igb, runtime resume routine
uses rtnl_lock() which upper ethtool layer also uses.
So to prevent a deadlock on rtnl, take a different approach, use
pm_runtime_suspended() to avoid reading register while device is runtime
suspended.
Is someone working on the full fix to how PM operates?
There is another rd32(IGC_STATUS) in this file which I don't think
is protected either.
On Thu, 28 Jan 2021 13:38:48 -0800 Tony Nguyen wrote:
quoted
From: Kai-Heng Feng <redacted>
Similar to commit 165ae7a8feb5 ("igb: Report speed and duplex as unknown
when device is runtime suspended"), if we try to read speed and duplex
sysfs while the device is runtime suspended, igc will complain and
stops working:
quoted
The more generic approach will be wrap get_link_ksettings() with begin()
and complete() callbacks, and calls runtime resume and runtime suspend
routine respectively. However, igc is like igb, runtime resume routine
uses rtnl_lock() which upper ethtool layer also uses.
So to prevent a deadlock on rtnl, take a different approach, use
pm_runtime_suspended() to avoid reading register while device is runtime
suspended.
Is someone working on the full fix to how PM operates?
There is another rd32(IGC_STATUS) in this file which I don't think
is protected either.
Hello Jakub,
What is another rd32(IGC_STATUS) you meant? in igc_ethtool_get_regs?
While the device in D3 state there is no configuration space registers
access.
sasha
From: Jakub Kicinski <kuba@kernel.org> Date: 2021-01-30 18:13:34
On Sat, 30 Jan 2021 16:00:06 +0200 Neftin, Sasha wrote:
On 1/30/2021 08:22, Jakub Kicinski wrote:
quoted
On Thu, 28 Jan 2021 13:38:48 -0800 Tony Nguyen wrote:
quoted
From: Kai-Heng Feng <redacted>
Similar to commit 165ae7a8feb5 ("igb: Report speed and duplex as unknown
when device is runtime suspended"), if we try to read speed and duplex
sysfs while the device is runtime suspended, igc will complain and
stops working:
quoted
The more generic approach will be wrap get_link_ksettings() with begin()
and complete() callbacks, and calls runtime resume and runtime suspend
routine respectively. However, igc is like igb, runtime resume routine
uses rtnl_lock() which upper ethtool layer also uses.
So to prevent a deadlock on rtnl, take a different approach, use
pm_runtime_suspended() to avoid reading register while device is runtime
suspended.
Is someone working on the full fix to how PM operates?
There is another rd32(IGC_STATUS) in this file which I don't think
is protected either.
What is another rd32(IGC_STATUS) you meant? in igc_ethtool_get_regs?
Yes.
While the device in D3 state there is no configuration space registers
access.
That's to say similar stack trace will be generated to the one fixed
here, if someone runs ethtool -d, correct? I don't see anything
checking runtime there either.
To be clear I'm not asking for this to be addressed in this series.
Rather for a strong commitment that PM handling will be restructured.
It seems to me you should depend on refcounting / locking that the PM
subsystem does more rather than involving rtnl_lock.
On Sat, 30 Jan 2021 16:00:06 +0200 Neftin, Sasha wrote:
quoted
On 1/30/2021 08:22, Jakub Kicinski wrote:
quoted
On Thu, 28 Jan 2021 13:38:48 -0800 Tony Nguyen wrote:
quoted
From: Kai-Heng Feng <redacted>
Similar to commit 165ae7a8feb5 ("igb: Report speed and duplex as unknown
when device is runtime suspended"), if we try to read speed and duplex
sysfs while the device is runtime suspended, igc will complain and
stops working:
quoted
The more generic approach will be wrap get_link_ksettings() with begin()
and complete() callbacks, and calls runtime resume and runtime suspend
routine respectively. However, igc is like igb, runtime resume routine
uses rtnl_lock() which upper ethtool layer also uses.
So to prevent a deadlock on rtnl, take a different approach, use
pm_runtime_suspended() to avoid reading register while device is runtime
suspended.
Is someone working on the full fix to how PM operates?
There is another rd32(IGC_STATUS) in this file which I don't think
is protected either.
What is another rd32(IGC_STATUS) you meant? in igc_ethtool_get_regs?
Yes.
quoted
While the device in D3 state there is no configuration space registers
access.
That's to say similar stack trace will be generated to the one fixed
here, if someone runs ethtool -d, correct? I don't see anything
checking runtime there either.
yes.
This problem crosses many drivers. (not only igb, igc,...)
specific to this one (igc), can we check 'netif_running at begin of the
_get_regs method:
if (!netif_running(netdev))
return;
what do you think? (only OS can put device to the D3)
To be clear I'm not asking for this to be addressed in this series.
Rather for a strong commitment that PM handling will be restructured.
It seems to me you should depend on refcounting / locking that the PM
subsystem does more rather than involving rtnl_lock.
From: Jakub Kicinski <kuba@kernel.org> Date: 2021-02-01 22:06:26
On Sun, 31 Jan 2021 12:22:25 +0200 Neftin, Sasha wrote:
On 1/30/2021 20:12, Jakub Kicinski wrote:
quoted
On Sat, 30 Jan 2021 16:00:06 +0200 Neftin, Sasha wrote:
quoted
What is another rd32(IGC_STATUS) you meant? in igc_ethtool_get_regs?
Yes.
quoted
While the device in D3 state there is no configuration space registers
access.
That's to say similar stack trace will be generated to the one fixed
here, if someone runs ethtool -d, correct? I don't see anything
checking runtime there either.
yes.
This problem crosses many drivers. (not only igb, igc,...)
specific to this one (igc), can we check 'netif_running at begin of the
_get_regs method:
if (!netif_running(netdev))
return;
what do you think? (only OS can put device to the D3)
That'd address the particular issue we noticed in the 5min review of
this patch, but similar, less obvious problems may still be lurking?
I wish I knew more about PM so I could suggest a solution. It'd be
ideal to avoid the rtnl_lock calls in resume, so that the driver can
just wake up the device from within the callbacks. Maybe embedded
experts can chime in and suggest how it's usually handled..