hwrm_ring_free_send_msg() reports failure to its caller, returning -EIO when the firmware rejects HWRM_RING_FREE or never answers it. All three ring free helpers that send the command discard the value. Return it instead. No caller acts on it yet, so there is no functional change. Fixes: 74608fc98d28 ("bnxt_en: Ring free response from close path should use completion ring") Signed-off-by: Joe Damato --- drivers/net/ethernet/broadcom/bnxt/bnxt.c | 48 +++++++++++++---------- 1 file changed, 27 insertions(+), 21 deletions(-) diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c index 33c4cacc8ffe..ea8529c78994 100644 --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c @@ -7679,50 +7679,55 @@ static int hwrm_ring_free_send_msg(struct bnxt *bp, return 0; } -static void bnxt_hwrm_tx_ring_free(struct bnxt *bp, - struct bnxt_tx_ring_info *txr, - bool close_path) +static int bnxt_hwrm_tx_ring_free(struct bnxt *bp, + struct bnxt_tx_ring_info *txr, + bool close_path) { struct bnxt_ring_struct *ring = &txr->tx_ring_struct; u32 cmpl_ring_id; + int rc; if (ring->fw_ring_id == INVALID_HW_RING_ID) - return; + return 0; cmpl_ring_id = close_path ? bnxt_cp_ring_for_tx(bp, txr) : INVALID_HW_RING_ID; - hwrm_ring_free_send_msg(bp, ring, RING_FREE_REQ_RING_TYPE_TX, - cmpl_ring_id); + rc = hwrm_ring_free_send_msg(bp, ring, RING_FREE_REQ_RING_TYPE_TX, + cmpl_ring_id); ring->fw_ring_id = INVALID_HW_RING_ID; + return rc; } -static void bnxt_hwrm_rx_ring_free(struct bnxt *bp, - struct bnxt_rx_ring_info *rxr, - bool close_path) +static int bnxt_hwrm_rx_ring_free(struct bnxt *bp, + struct bnxt_rx_ring_info *rxr, + bool close_path) { struct bnxt_ring_struct *ring = &rxr->rx_ring_struct; u32 grp_idx = rxr->bnapi->index; u32 cmpl_ring_id; + int rc; if (ring->fw_ring_id == INVALID_HW_RING_ID) - return; + return 0; cmpl_ring_id = bnxt_cp_ring_for_rx(bp, rxr); - hwrm_ring_free_send_msg(bp, ring, - RING_FREE_REQ_RING_TYPE_RX, - close_path ? cmpl_ring_id : - INVALID_HW_RING_ID); + rc = hwrm_ring_free_send_msg(bp, ring, + RING_FREE_REQ_RING_TYPE_RX, + close_path ? cmpl_ring_id : + INVALID_HW_RING_ID); ring->fw_ring_id = INVALID_HW_RING_ID; bp->grp_info[grp_idx].rx_fw_ring_id = INVALID_HW_RING_ID; + return rc; } -static void bnxt_hwrm_rx_agg_ring_free(struct bnxt *bp, - struct bnxt_rx_ring_info *rxr, - bool close_path) +static int bnxt_hwrm_rx_agg_ring_free(struct bnxt *bp, + struct bnxt_rx_ring_info *rxr, + bool close_path) { struct bnxt_ring_struct *ring = &rxr->rx_agg_ring_struct; u32 grp_idx = rxr->bnapi->index; u32 type, cmpl_ring_id; + int rc; if (bp->flags & BNXT_FLAG_CHIP_P5_PLUS) type = RING_FREE_REQ_RING_TYPE_RX_AGG; @@ -7730,14 +7735,15 @@ static void bnxt_hwrm_rx_agg_ring_free(struct bnxt *bp, type = RING_FREE_REQ_RING_TYPE_RX; if (ring->fw_ring_id == INVALID_HW_RING_ID) - return; + return 0; cmpl_ring_id = bnxt_cp_ring_for_rx(bp, rxr); - hwrm_ring_free_send_msg(bp, ring, type, - close_path ? cmpl_ring_id : - INVALID_HW_RING_ID); + rc = hwrm_ring_free_send_msg(bp, ring, type, + close_path ? cmpl_ring_id : + INVALID_HW_RING_ID); ring->fw_ring_id = INVALID_HW_RING_ID; bp->grp_info[grp_idx].agg_fw_ring_id = INVALID_HW_RING_ID; + return rc; } static void bnxt_hwrm_cp_ring_free(struct bnxt *bp, -- 2.53.0-Meta When a command is sent over a completion ring, __hwrm_send() waits for NAPI to consume the completion and gives up if it never arrives, without looking at the response. If a completion is not posted within the timeout, check the response before giving up. If resp_len is set, the sequence id matches, and the valid byte is set then the firmware completed the command and only the notification was lost. Fall through to the normal error_code handling in that case. Several seconds are spent waiting for the completion, so a response that was written at all is complete by the time the wait gives up. There is no need to poll for the valid byte here the way the polling path below has to, where the poll is for a non-zero length and the valid byte at the end of the message may still be on its way. Log the response state on both paths so there is more data when this rare event occurs. Fixes: 74608fc98d28 ("bnxt_en: Ring free response from close path should use completion ring") Signed-off-by: Joe Damato --- .../net/ethernet/broadcom/bnxt/bnxt_hwrm.c | 33 ++++++++++++++++--- 1 file changed, 29 insertions(+), 4 deletions(-) diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_hwrm.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_hwrm.c index 5bfabdca7d0e..4feba90f0bf6 100644 --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_hwrm.c +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_hwrm.c @@ -582,11 +582,36 @@ static int __hwrm_send(struct bnxt *bp, struct bnxt_hwrm_ctx *ctx) } if (READ_ONCE(token->state) != BNXT_HWRM_COMPLETE) { - hwrm_err(bp, ctx, "Resp cmpl intr err msg: 0x%x\n", - req_type); - goto exit; + __le16 resp_seq_id; + u8 valid_byte = 0; + + /* The completion ring entry was not delivered for + * some reason. It might be possible that the command + * was carried out even without a completion being + * posted. Check the response before giving up and log + * the state. + */ + dma_rmb(); + resp_seq_id = READ_ONCE(ctx->resp->seq_id); + len = le16_to_cpu(READ_ONCE(ctx->resp->resp_len)); + if (len && resp_seq_id == ctx->req->seq_id) + valid_byte = *((u8 *)ctx->resp + len - 1); + + if (!valid_byte) { + hwrm_err(bp, ctx, + "Resp cmpl intr err msg: 0x%x len:%d seq:0x%x/0x%x\n", + req_type, len, + le16_to_cpu(resp_seq_id), + le16_to_cpu(ctx->req->seq_id)); + goto exit; + } + netdev_warn(bp->dev, + "Resp cmpl intr not delivered, msg: 0x%x completed anyway (len:%d valid:0x%x err:0x%x)\n", + req_type, len, valid_byte, + le16_to_cpu(ctx->resp->error_code)); + } else { + len = le16_to_cpu(READ_ONCE(ctx->resp->resp_len)); } - len = le16_to_cpu(READ_ONCE(ctx->resp->resp_len)); valid = ((u8 *)ctx->resp) + len - 1; } else { __le16 seen_out_of_seq = ctx->req->seq_id; /* will never see */ -- 2.53.0-Meta When HWRM_RING_FREE is not answered, bnxt_hwrm_ring_free() clears fw_ring_id and __bnxt_close_nic() goes on to call bnxt_free_mem(), which unmaps the ring memory and the RX buffers that the FW may still be using. This is reachable in production. On a BCM57504 the first sign is the TX watchdog; the close that follows times out a subset of its RING_FREEs and the driver releases those rings anyway: 05:30:12 NETDEV WATCHDOG: transmit queue 0 timed out 6073 ms 05:30:12 Resp cmpl intr err msg: 0x51 x20 05:30:12 hwrm_ring_free type 1 failed x12 05:30:12 hwrm_ring_free type 2 failed x8 05:30:12 AMD-Vi: IO_PAGE_FAULT x3 Count the rings the firmware did not free and report that to the caller so it can decide what to do. bnxt_hwrm_resource_free() still frees the remaining firmware resources before returning the error, so the shutdown can send every message it needs to before the device is stopped. The device stays unusable until the driver is rebound. Fixes: 74608fc98d28 ("bnxt_en: Ring free response from close path should use completion ring") Signed-off-by: Joe Damato --- drivers/net/ethernet/broadcom/bnxt/bnxt.c | 48 +++++++++++++++++------ 1 file changed, 35 insertions(+), 13 deletions(-) diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c index ea8529c78994..b5ef86bf92bf 100644 --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c @@ -7773,21 +7773,25 @@ static void bnxt_clear_one_cp_ring(struct bnxt *bp, struct bnxt_cp_ring_info *cp memset(cpr->cp_desc_ring[i], 0, size); } -static void bnxt_hwrm_ring_free(struct bnxt *bp, bool close_path) +static int bnxt_hwrm_ring_free(struct bnxt *bp, bool close_path) { + int stuck = 0; u32 type; int i; if (!bp->bnapi) - return; + return 0; for (i = 0; i < bp->tx_nr_rings; i++) - bnxt_hwrm_tx_ring_free(bp, &bp->tx_ring[i], close_path); + if (bnxt_hwrm_tx_ring_free(bp, &bp->tx_ring[i], close_path)) + stuck++; bnxt_cancel_dim(bp); for (i = 0; i < bp->rx_nr_rings; i++) { - bnxt_hwrm_rx_ring_free(bp, &bp->rx_ring[i], close_path); - bnxt_hwrm_rx_agg_ring_free(bp, &bp->rx_ring[i], close_path); + if (bnxt_hwrm_rx_ring_free(bp, &bp->rx_ring[i], close_path)) + stuck++; + if (bnxt_hwrm_rx_agg_ring_free(bp, &bp->rx_ring[i], close_path)) + stuck++; } /* The completion rings are about to be freed. After that the @@ -7817,6 +7821,19 @@ static void bnxt_hwrm_ring_free(struct bnxt *bp, bool close_path) bp->grp_info[i].cp_fw_ring_id = INVALID_HW_RING_ID; } } + + if (!stuck) + return 0; + + netdev_err(bp->dev, "Firmware did not free %d ring(s)\n", stuck); + return -EIO; +} + +static void bnxt_stop_dma(struct bnxt *bp) +{ + netdev_err(bp->dev, + "Disabling DMA before releasing ring memory, the driver must be rebound to recover\n"); + pci_clear_master(bp->pdev); } static int __bnxt_trim_rings(struct bnxt *bp, int *rx, int *tx, int max, @@ -10851,16 +10868,19 @@ static void bnxt_clear_vnic(struct bnxt *bp) bnxt_hwrm_vnic_ctx_free(bp); } -static void bnxt_hwrm_resource_free(struct bnxt *bp, bool close_path, - bool irq_re_init) +static int bnxt_hwrm_resource_free(struct bnxt *bp, bool close_path, + bool irq_re_init) { + int rc; + bnxt_clear_vnic(bp); - bnxt_hwrm_ring_free(bp, close_path); + rc = bnxt_hwrm_ring_free(bp, close_path); bnxt_hwrm_ring_grp_free(bp); if (irq_re_init) { bnxt_hwrm_stat_ctx_free(bp); bnxt_hwrm_free_tunnel_ports(bp); } + return rc; } static int bnxt_hwrm_set_br_mode(struct bnxt *bp, u16 br_mode) @@ -11382,15 +11402,15 @@ static int bnxt_init_chip(struct bnxt *bp, bool irq_re_init) return 0; err_out: - bnxt_hwrm_resource_free(bp, 0, true); + if (bnxt_hwrm_resource_free(bp, 0, true)) + bnxt_stop_dma(bp); return rc; } static int bnxt_shutdown_nic(struct bnxt *bp, bool irq_re_init) { - bnxt_hwrm_resource_free(bp, 1, irq_re_init); - return 0; + return bnxt_hwrm_resource_free(bp, 1, irq_re_init); } static int bnxt_init_nic(struct bnxt *bp, bool irq_re_init) @@ -13461,7 +13481,8 @@ int bnxt_half_open_nic(struct bnxt *bp) */ void bnxt_half_close_nic(struct bnxt *bp) { - bnxt_hwrm_resource_free(bp, false, true); + if (bnxt_hwrm_resource_free(bp, false, true)) + bnxt_stop_dma(bp); bnxt_del_napi(bp); bnxt_free_skbs(bp); bnxt_free_mem(bp, true); @@ -13540,7 +13561,8 @@ static void __bnxt_close_nic(struct bnxt *bp, bool irq_re_init, if (BNXT_SUPPORTS_MULTI_RSS_CTX(bp)) bnxt_clear_rss_ctxs(bp); /* Flush rings and disable interrupts */ - bnxt_shutdown_nic(bp, irq_re_init); + if (bnxt_shutdown_nic(bp, irq_re_init)) + bnxt_stop_dma(bp); /* TODO CHIMP_FW: Link/PHY related cleanup if (link_re_init) */ -- 2.53.0-Meta Add BNXT_STATE_DMA_STOPPED and set it in bnxt_stop_dma() to signal that the device had DMA disabled. When opening the device later, check this bit and exit with an error. This is a very rare case, but when it happens the device needs manual intervention by the user to become usable again. Fixes: 74608fc98d28 ("bnxt_en: Ring free response from close path should use completion ring") Signed-off-by: Joe Damato --- drivers/net/ethernet/broadcom/bnxt/bnxt.c | 26 ++++++++++++++++++++--- drivers/net/ethernet/broadcom/bnxt/bnxt.h | 1 + 2 files changed, 24 insertions(+), 3 deletions(-) diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c index b5ef86bf92bf..eb885fd74064 100644 --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c @@ -7833,9 +7833,20 @@ static void bnxt_stop_dma(struct bnxt *bp) { netdev_err(bp->dev, "Disabling DMA before releasing ring memory, the driver must be rebound to recover\n"); + set_bit(BNXT_STATE_DMA_STOPPED, &bp->state); pci_clear_master(bp->pdev); } +static int bnxt_check_dma_stopped(struct bnxt *bp) +{ + if (!test_bit(BNXT_STATE_DMA_STOPPED, &bp->state)) + return 0; + + netdev_err(bp->dev, + "DMA was disabled after the firmware failed to free rings, rebind the driver to recover\n"); + return -ENODEV; +} + static int __bnxt_trim_rings(struct bnxt *bp, int *rx, int *tx, int max, bool shared); static int bnxt_trim_rings(struct bnxt *bp, int *rx, int *tx, int max, @@ -13426,9 +13437,10 @@ static int __bnxt_open_nic(struct bnxt *bp, bool irq_re_init, bool link_re_init) int bnxt_open_nic(struct bnxt *bp, bool irq_re_init, bool link_re_init) { - int rc = 0; + int rc; - if (test_bit(BNXT_STATE_ABORT_ERR, &bp->state)) + rc = bnxt_check_dma_stopped(bp); + if (!rc && test_bit(BNXT_STATE_ABORT_ERR, &bp->state)) rc = -EIO; if (!rc) rc = __bnxt_open_nic(bp, irq_re_init, link_re_init); @@ -13445,7 +13457,11 @@ int bnxt_open_nic(struct bnxt *bp, bool irq_re_init, bool link_re_init) */ int bnxt_half_open_nic(struct bnxt *bp) { - int rc = 0; + int rc; + + rc = bnxt_check_dma_stopped(bp); + if (rc) + goto half_open_err; if (test_bit(BNXT_STATE_ABORT_ERR, &bp->state)) { netdev_err(bp->dev, "A previous firmware reset has not completed, aborting half open\n"); @@ -13505,6 +13521,10 @@ static int bnxt_open(struct net_device *dev) struct bnxt *bp = netdev_priv(dev); int rc; + rc = bnxt_check_dma_stopped(bp); + if (rc) + return rc; + if (test_bit(BNXT_STATE_ABORT_ERR, &bp->state)) { rc = bnxt_reinit_after_abort(bp); if (rc) { diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.h b/drivers/net/ethernet/broadcom/bnxt/bnxt.h index 41d05efd6307..7e65b61b6bf8 100644 --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.h +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.h @@ -2474,6 +2474,7 @@ struct bnxt { #define BNXT_STATE_FW_NON_FATAL_COND 13 #define BNXT_STATE_FW_ACTIVATE_RESET 14 #define BNXT_STATE_HALF_OPEN 15 /* For offline ethtool tests */ +#define BNXT_STATE_DMA_STOPPED 16 #define BNXT_NO_FW_ACCESS(bp) \ (test_bit(BNXT_STATE_FW_FATAL_COND, &(bp)->state) || \ -- 2.53.0-Meta