From: Michael Chan <michael.chan@broadcom.com> Date: 2021-07-18 19:36:51
Most of the fixes in this series have to do with error recovery. They
include error path handling when the error recovery has to abort, and
the rediscovery of capabilities (PTP and RoCE) after firmware reset
that may result in capability changes.
Two other fixes are to reject invalid ETS settings and to validate
VLAN protocol in the RX path.
Edwin Peer (1):
bnxt_en: reject ETS settings that will starve a TC
Kalesh AP (1):
bnxt_en: don't disable an already disabled PCI device
Michael Chan (5):
bnxt_en: Refresh RoCE capabilities in bnxt_ulp_probe()
bnxt_en: Add missing check for BNXT_STATE_ABORT_ERR in
bnxt_fw_rset_task()
bnxt_en: Validate vlan protocol ID on RX packets
bnxt_en: Move bnxt_ptp_init() to bnxt_open()
bnxt_en: Fix PTP capability discovery
Somnath Kotur (2):
bnxt_en: fix error path of FW reset
bnxt_en: Check abort error state in bnxt_half_open_nic()
drivers/net/ethernet/broadcom/bnxt/bnxt.c | 85 ++++++++++++++-----
drivers/net/ethernet/broadcom/bnxt/bnxt_dcb.c | 10 ++-
drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c | 24 ++----
drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h | 1 -
drivers/net/ethernet/broadcom/bnxt/bnxt_ulp.c | 9 +-
5 files changed, 83 insertions(+), 46 deletions(-)
--
2.18.1
From: Michael Chan <michael.chan@broadcom.com> Date: 2021-07-18 19:36:52
From: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>
If device is already disabled in reset path and PCI io error is
detected before the device could be enabled, driver could
call pci_disable_device() for already disabled device. Fix this
problem by calling pci_disable_device() only if the device is already
enabled.
Fixes: 6316ea6db93d ("bnxt_en: Enable AER support.")
Signed-off-by: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>
Signed-off-by: Michael Chan <michael.chan@broadcom.com>
---
drivers/net/ethernet/broadcom/bnxt/bnxt.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
From: Michael Chan <michael.chan@broadcom.com> Date: 2021-07-18 19:36:53
From: Edwin Peer <redacted>
ETS proportions are presented to HWRM_QUEUE_COS2BW_CFG as minimum
bandwidth constraints. Thus, zero is a legal value for a given TC.
However, if all the other TCs sum up to 100%, then at least one
hardware queue will starve, resulting in guaranteed TX timeouts.
Reject such nonsensical configurations.
Reviewed-by: Pavan Chebbi <pavan.chebbi@broadcom.com>
Signed-off-by: Edwin Peer <redacted>
Signed-off-by: Michael Chan <michael.chan@broadcom.com>
---
drivers/net/ethernet/broadcom/bnxt/bnxt_dcb.c | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)
From: Michael Chan <michael.chan@broadcom.com> Date: 2021-07-18 19:36:54
The capabilities can change after firmware upgrade/downgrade, so we
should get the up-to-date RoCE capabilities everytime bnxt_ulp_probe()
is called.
Fixes: 2151fe0830fd ("bnxt_en: Handle RESET_NOTIFY async event from firmware.")
Reviewed-by: Somnath Kotur <redacted>
Reviewed-by: Edwin Peer <redacted>
Signed-off-by: Michael Chan <michael.chan@broadcom.com>
---
drivers/net/ethernet/broadcom/bnxt/bnxt_ulp.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
From: Michael Chan <michael.chan@broadcom.com> Date: 2021-07-18 19:36:56
In the BNXT_FW_RESET_STATE_POLL_VF state in bnxt_fw_reset_task() after all
VFs have unregistered, we need to check for BNXT_STATE_ABORT_ERR after
we acquire the rtnl_lock. If the flag is set, we need to abort.
Fixes: 230d1f0de754 ("bnxt_en: Handle firmware reset.")
Signed-off-by: Michael Chan <michael.chan@broadcom.com>
---
drivers/net/ethernet/broadcom/bnxt/bnxt.c | 4 ++++
1 file changed, 4 insertions(+)
From: Michael Chan <michael.chan@broadcom.com> Date: 2021-07-18 19:36:58
From: Somnath Kotur <redacted>
When bnxt_open() fails in the firmware reset path, the driver needs to
gracefully abort, but it is executing code that should be invoked only
in the success path. Define a function to abort FW reset and
consolidate all error paths to call this new function.
Fixes: dab62e7c2de7 ("bnxt_en: Implement faster recovery for firmware fatal error.")
Signed-off-by: Somnath Kotur <redacted>
Signed-off-by: Michael Chan <michael.chan@broadcom.com>
---
drivers/net/ethernet/broadcom/bnxt/bnxt.c | 31 +++++++++++++++--------
1 file changed, 21 insertions(+), 10 deletions(-)
@@ -11959,10 +11959,21 @@ static bool bnxt_fw_reset_timeout(struct bnxt *bp)(bp->fw_reset_max_dsecs*HZ/10));}+staticvoidbnxt_fw_reset_abort(structbnxt*bp,intrc)+{+clear_bit(BNXT_STATE_IN_FW_RESET,&bp->state);+if(bp->fw_reset_state!=BNXT_FW_RESET_STATE_POLL_VF){+bnxt_ulp_start(bp,rc);+bnxt_dl_health_status_update(bp,false);+}+bp->fw_reset_state=0;+dev_close(bp->dev);+}+staticvoidbnxt_fw_reset_task(structwork_struct*work){structbnxt*bp=container_of(work,structbnxt,fw_reset_task.work);-intrc;+intrc=0;if(!test_bit(BNXT_STATE_IN_FW_RESET,&bp->state)){netdev_err(bp->dev,"bnxt_fw_reset_task() called when not in fw reset mode!\n");
From: Michael Chan <michael.chan@broadcom.com> Date: 2021-07-18 19:37:00
From: Somnath Kotur <redacted>
bnxt_half_open_nic() is called during during ethtool self test and is
protected by rtnl_lock. Firmware reset can be happening at the same
time. Only critical portions of the entire firmware reset sequence
are protected by the rtnl_lock. It is possible that bnxt_half_open_nic()
can be called when the firmware reset sequence is aborting. In that
case, bnxt_half_open_nic() needs to check if the ABORT_ERR flag is set
and abort if it is. The ethtool self test will fail but the NIC will be
brought to a consistent IF_DOWN state.
Without this patch, if bnxt_half_open_nic() were to continue in this
error state, it may crash like this:
bnxt_en 0000:82:00.1 enp130s0f1np1: FW reset in progress during close, FW reset will be aborted
Unable to handle kernel NULL pointer dereference at virtual address 0000000000000000
...
Process ethtool (pid: 333327, stack limit = 0x0000000046476577)
Call trace:
bnxt_alloc_mem+0x444/0xef0 [bnxt_en]
bnxt_half_open_nic+0x24/0xb8 [bnxt_en]
bnxt_self_test+0x2dc/0x390 [bnxt_en]
ethtool_self_test+0xe0/0x1f8
dev_ethtool+0x1744/0x22d0
dev_ioctl+0x190/0x3e0
sock_ioctl+0x238/0x480
do_vfs_ioctl+0xc4/0x758
ksys_ioctl+0x84/0xb8
__arm64_sys_ioctl+0x28/0x38
el0_svc_handler+0xb0/0x180
el0_svc+0x8/0xc
Fixes: a1301f08c5ac ("bnxt_en: Check abort error state in bnxt_open_nic().")
Signed-off-by: Somnath Kotur <redacted>
Signed-off-by: Michael Chan <michael.chan@broadcom.com>
---
drivers/net/ethernet/broadcom/bnxt/bnxt.c | 6 ++++++
1 file changed, 6 insertions(+)
From: Michael Chan <michael.chan@broadcom.com> Date: 2021-07-18 19:37:01
The device needs to be in ifup state for PTP to function, so move
bnxt_ptp_init() to bnxt_open(). This means that the PHC will be
registered during bnxt_open().
This also makes firmware reset work correctly. PTP configurations
may change after firmware upgrade or downgrade. bnxt_open() will
be called after firmware reset, so it will work properly.
bnxt_ptp_start() is now incorporated into bnxt_ptp_init(). We now
also need to call bnxt_ptp_clear() in bnxt_close().
Fixes: 93cb62d98e9c ("bnxt_en: Enable hardware PTP support")
Cc: Richard Cochran <richardcochran@gmail.com>
Reviewed-by: Pavan Chebbi <pavan.chebbi@broadcom.com>
Signed-off-by: Michael Chan <michael.chan@broadcom.com>
---
drivers/net/ethernet/broadcom/bnxt/bnxt.c | 16 +++++++------
drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c | 24 ++++++-------------
drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h | 1 -
3 files changed, 16 insertions(+), 25 deletions(-)
From: Michael Chan <michael.chan@broadcom.com> Date: 2021-07-18 19:37:04
Only pass supported VLAN protocol IDs for stripped VLAN tags to the
stack. The stack will hit WARN() if the protocol ID is unsupported.
Existing firmware sets up the chip to strip 0x8100, 0x88a8, 0x9100.
Only the 1st two protocols are supported by the kernel.
Fixes: a196e96bb68f ("bnxt_en: clean up VLAN feature bit handling")
Reviewed-by: Somnath Kotur <redacted>
Signed-off-by: Michael Chan <michael.chan@broadcom.com>
---
drivers/net/ethernet/broadcom/bnxt/bnxt.c | 21 ++++++++++++++++-----
1 file changed, 16 insertions(+), 5 deletions(-)
From: Michael Chan <michael.chan@broadcom.com> Date: 2021-07-18 19:37:09
The current PTP initialization logic does not account for firmware
reset that may cause PTP capability to change. The valid pointer
bp->ptp_cfg is used to indicate that the device is capable of PTP
and that it has been initialized. So we must clean up bp->ptp_cfg
and free it if the firmware after reset does not support PTP.
Fixes: 93cb62d98e9c ("bnxt_en: Enable hardware PTP support")
Cc: Richard Cochran <richardcochran@gmail.com>
Reviewed-by: Pavan Chebbi <pavan.chebbi@broadcom.com>
Signed-off-by: Michael Chan <michael.chan@broadcom.com>
---
drivers/net/ethernet/broadcom/bnxt/bnxt.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
From: Jakub Kicinski <kuba@kernel.org> Date: 2021-07-19 10:43:31
On Sun, 18 Jul 2021 15:36:32 -0400, Michael Chan wrote:
The device needs to be in ifup state for PTP to function, so move
bnxt_ptp_init() to bnxt_open(). This means that the PHC will be
registered during bnxt_open().
I think it's an anti-pattern to have the clock registered only when
the device is up. Right, Richard?
IIRC Intel parts did it in the past because they had the clock hooked
up to the MAC/PHY so the clock was not actually ticking. But seems like
a wrong trade off to unreg PTP for SW convenience. Or maybe I'm
biased against the live FW reset :) Let's see if Richard agrees.
This also makes firmware reset work correctly. PTP configurations
may change after firmware upgrade or downgrade. bnxt_open() will
be called after firmware reset, so it will work properly.
bnxt_ptp_start() is now incorporated into bnxt_ptp_init(). We now
also need to call bnxt_ptp_clear() in bnxt_close().
Hello:
This series was applied to netdev/net.git (refs/heads/master):
On Sun, 18 Jul 2021 15:36:24 -0400 you wrote:
Most of the fixes in this series have to do with error recovery. They
include error path handling when the error recovery has to abort, and
the rediscovery of capabilities (PTP and RoCE) after firmware reset
that may result in capability changes.
Two other fixes are to reject invalid ETS settings and to validate
VLAN protocol in the RX path.
[...]
From: Richard Cochran <richardcochran@gmail.com> Date: 2021-07-19 17:22:35
On Mon, Jul 19, 2021 at 12:43:23PM +0200, Jakub Kicinski wrote:
On Sun, 18 Jul 2021 15:36:32 -0400, Michael Chan wrote:
quoted
The device needs to be in ifup state for PTP to function, so move
bnxt_ptp_init() to bnxt_open(). This means that the PHC will be
registered during bnxt_open().
I think it's an anti-pattern to have the clock registered only when
the device is up. Right, Richard?
Yes, indeed.
IIRC Intel parts did it in the past because they had the clock hooked
up to the MAC/PHY so the clock was not actually ticking. But seems like
a wrong trade off to unreg PTP for SW convenience. Or maybe I'm
biased against the live FW reset :) Let's see if Richard agrees.
Totally agree!
Ideally the PHC appears as soon as the driver instantiates for a HW
device. Telling time from the clock is independent from the network
interface being up or down.
Some drivers are lazy and fail to decouple these two orthogonal
issues. In some (all?) cases, there is no HW limitation involved,
just sloppy driver work.
Thanks,
Richard
From: Michael Chan <michael.chan@broadcom.com> Date: 2021-07-20 05:30:06
On Mon, Jul 19, 2021 at 10:07 AM Richard Cochran
[off-list ref] wrote:
Ideally the PHC appears as soon as the driver instantiates for a HW
device. Telling time from the clock is independent from the network
interface being up or down.
OK. Will improve this in follow on patches to keep the PHC registered
unless the firmware after reset no longer supports PTP. Thanks.