Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues: - [High] The patch prevents destroying uninitialized TX WQ objects that hold the `INVALID_MANA_HANDLE` sentinel, but bypasses unreached elements in the zero-initialized array, causing handle `0` to be erroneously passed to firmware. - [High] Hardware vport registrations and configurations are not cleanly reverted if subsequent queue allocations fail inside `mana_create_vport()`, permanently breaking the network interface. -- commit 27abeb920ad813c7f2d581fe8f249fdc9e94666a Author: Aditya Garg <[email protected]> net: mana: guard TX wq object destroy with INVALID_MANA_HANDLE check This commit adds a check in mana_destroy_txq() to prevent destroying uninitialized TX WQ objects that hold the INVALID_MANA_HANDLE sentinel. This avoids passing invalid handles to the firmware, which previously caused errors and logs. The change mirrors existing RX-side cleanup patterns. > diff --git a/drivers/net/ethernet/microsoft/mana/mana_en.c > b/drivers/net/ethernet/microsoft/mana/mana_en.c > index c9b1df1ed1098..d7de4c4d25bbe 100644 > --- a/drivers/net/ethernet/microsoft/mana/mana_en.c > +++ b/drivers/net/ethernet/microsoft/mana/mana_en.c > @@ -2334,7 +2334,8 @@ static void mana_destroy_txq(struct mana_port_context > *apc) > netif_napi_del_locked(napi); > apc->tx_qp[i].txq.napi_initialized = false; > } > - mana_destroy_wq_obj(apc, GDMA_SQ, apc->tx_qp[i].tx_object); > + if (apc->tx_qp[i].tx_object != INVALID_MANA_HANDLE) > + mana_destroy_wq_obj(apc, GDMA_SQ, > apc->tx_qp[i].tx_object); [Severity: High] This is a pre-existing issue, but does this check safely handle all uninitialized tx_object values? In mana_create_txq(), apc->tx_qp is allocated using kzalloc_objs(), which zero-initializes the array: mana_create_txq() { apc->tx_qp = kzalloc_objs(struct mana_tx_qp, apc->num_queues); if (!apc->tx_qp) return -ENOMEM; for (i = 0; i < apc->num_queues; i++) { apc->tx_qp[i].tx_object = INVALID_MANA_HANDLE; ... } If mana_create_txq() fails during the loop, the unvisited elements (where j > i) will still have their zero-initialized values, meaning tx_qp[j].tx_object == 0. Since mana_destroy_txq() iterates over all apc->num_queues, and 0 is not INVALID_MANA_HANDLE, the condition apc->tx_qp[i].tx_object != INVALID_MANA_HANDLE will be true for these unvisited queues. Could this lead to erroneously calling mana_destroy_wq_obj() with handle 0, potentially destroying an active WQ object used by another queue or port? > > mana_deinit_cq(apc, &apc->tx_qp[i].tx_cq); > [Severity: High] This isn't a bug introduced by this patch, but is there a resource leak in mana_create_vport() if mana_create_txq() fails? mana_create_vport() { ... err = mana_pf_register_hw_vport(apc); if (err) return err; err = mana_cfg_vport(apc, gd->pdid, gd->doorbell); if (err) return err; return mana_create_txq(apc, net); } If mana_create_txq() fails, it returns the error directly to mana_alloc_queues() without jumping to a destroy_vport label. This bypasses mana_destroy_vport() and skips cleaning up the hardware vport registrations. Because the interface fails to open, apc->port_is_up remains false. Future teardown attempts will skip mana_dealloc_queues(), leaving vport_use_count permanently incremented. Would this prevent the interface from ever being brought up again, as mana_cfg_vport() enforces apc->vport_use_count == 0, requiring a driver reload to recover? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=2
