On 12-10-2026 14:39, [email protected] wrote: > Thank you for your contribution! Sashiko AI review found 5 potential > issue(s) to consider. > > Critical: 0 · High: 2 · Medium: 2 · Low: 1 > > If you believe none of the issues are legitimate, please reply to each > issue of Medium severity or higher and explain why the AI is wrong. > If any of them are legitimate and you plan to respin, please let patchwork > know by including "pw-bot: cr" as a separate line at the end of your reply > (one such reply per series is enough). > > - [High] The throttle-EQE fix only reaches CQs that mana_gd_create_cq() > registered in parent->eq.child_cq[], but the patch asks for throttled… > - [High] During HWC teardown, the HWC CQ stays published in the HWC EQ's > RCU-protected child_cq[] after the state its callback uses has been… > - [Medium] The commit message presents the change as a fix: hardware > sends type-4 throttle EQEs, the driver drops them, and CQs stall. > - [Medium] mana_gd_destroy_cq() now calls synchronize_rcu() once for > every CQ that has a parent EQ. > - [Low] eq->eq.throttle_count is incremented with a plain '++' in hardirq > context (mana_gd_intr -> mana_gd_process_eqe). > > -- > > net: mana: Add support to process throttled EQEs > > This patch handles the type 4 throttle EQE. The hardware sends it in place > of per-CQ completion notifications when an EQ nears full. Each EQ records > up to two child CQs and schedules them when a throttle EQE arrives. The > patch also advertises a new driver capability bit and adds an ethtool -S > counter. > >> diff --git a/drivers/net/ethernet/microsoft/mana/gdma_main.c >> b/drivers/net/ethernet/microsoft/mana/gdma_main.c >> index ae0ed700b3b93..b8e5ad2c6d312 100644 >> --- a/drivers/net/ethernet/microsoft/mana/gdma_main.c >> +++ b/drivers/net/ethernet/microsoft/mana/gdma_main.c >> @@ -747,6 +747,18 @@ void mana_gd_free_service_wq(struct gdma_context *gc) >> gc->service_wq = NULL; >> } >> >> +static void mana_gd_schedule_eq_cqs(struct gdma_queue *eq) >> +{ >> + struct gdma_queue *cq; >> + u8 i; >> + >> + for (i = 0; i < GDMA_EQ_MAX_CHILD_CQ; i++) { >> + cq = rcu_dereference(eq->eq.child_cq[i]); >> + if (cq && cq->cq.callback) >> + cq->cq.callback(cq->cq.context, cq); >> + } >> +} > > [Severity: High] > Can this path reach the HWC CQ callback after its state has been freed? > > Before this patch, the only way to reach the HWC CQ callback was the > GDMA_EQE_COMPLETION case. That case is gated by gc->max_num_cqs and > gc->cq_table[]. The throttle path walks child_cq[] directly and skips > that gate. > > mana_hwc_destroy_channel() tears things down in this order: > > drivers/net/ethernet/microsoft/mana/hw_channel.c:mana_hwc_destroy_channel() { > ... > if (gc->max_num_cqs > 0) { > mana_smc_teardown_hwc(&gc->shm_channel, false); > gc->max_num_cqs = 0; > } > > if (hwc->txq) > mana_hwc_destroy_wq(hwc, hwc->txq); > > if (hwc->rxq) > mana_hwc_destroy_wq(hwc, hwc->rxq); > > if (hwc->cq) > mana_hwc_destroy_cq(hwc->gdma_dev->gdma_context, hwc->cq); > ... > } > > mana_hwc_destroy_cq() then frees comp_buf before the CQ is removed from > child_cq[]: > > drivers/net/ethernet/microsoft/mana/hw_channel.c:mana_hwc_destroy_cq() { > kfree(hwc_cq->comp_buf); > > if (hwc_cq->gdma_cq) > mana_gd_destroy_queue(gc, hwc_cq->gdma_cq); > ... > } > > The HWC EQ interrupt is still registered at that point. Suppose a throttle > EQE arrives on the HWC EQ in this window: > > mana_gd_intr() > mana_gd_process_eqe() > mana_gd_schedule_eq_cqs() > mana_hwc_comp_event() > mana_gd_poll_cq(q_self, hwc_cq->comp_buf, ...) > mana_hwc_rx_event_handler() / mana_hwc_tx_event_handler() > > Wouldn't this write CQEs into the freed comp_buf, and then dereference the > freed hwc->rxq and hwc->txq? > > A well-behaved device probably would not post a throttle EQE after SMC > teardown. In confidential VMs, though, the driver treats the host as > untrusted, and the max_num_cqs gate used to block this route. > > Should the CQ be removed from child_cq[], followed by a grace period, > before the HWC teardown frees comp_buf, txq and rxq? > I will fix this in v2. >> + >> static void mana_gd_process_eqe(struct gdma_queue *eq) >> { >> u32 head = eq->head % (eq->queue_size / GDMA_EQE_SIZE); >> @@ -777,6 +789,11 @@ static void mana_gd_process_eqe(struct gdma_queue *eq) >> >> break; >> >> + case GDMA_EQE_THROTTLE: >> + eq->eq.throttle_count++; > > [Severity: Low] > This counter is incremented with a plain ++ in hardirq context, via > mana_gd_intr()->mana_gd_process_eqe(). mana_get_ethtool_stats() reads it > with a plain load in process context, with no shared lock: > > apc->eth_stats.eq_throttle_events += > apc->eqs[q].eq->eq.throttle_count; > > The value can't tear because MANA only builds on 64-bit. Do these accesses > still need READ_ONCE()/WRITE_ONCE() annotations? Without them KCSAN will > report a data race, and the compiler is free to fuse or reload the > accesses. > >> + mana_gd_schedule_eq_cqs(eq); >> + break; >> + > I will fix this in v2. > [ ... ] > >> @@ -1063,17 +1080,41 @@ static void mana_gd_create_cq(const struct >> gdma_queue_spec *spec, >> struct gdma_queue *queue) >> { >> u32 log2_num_entries = ilog2(spec->queue_size / GDMA_CQE_SIZE); >> + struct gdma_queue *parent; >> + u8 i; >> >> queue->head |= INITIALIZED_OWNER_BIT(log2_num_entries); >> - queue->cq.parent = spec->cq.parent_eq; >> + parent = spec->cq.parent_eq; >> + queue->cq.parent = parent; >> queue->cq.context = spec->cq.context; >> queue->cq.callback = spec->cq.callback; >> + >> + if (!parent) >> + return; >> + >> + /* For throttled EQE store the child CQ */ >> + for (i = 0; i < GDMA_EQ_MAX_CHILD_CQ; i++) >> + if (!rcu_access_pointer(parent->eq.child_cq[i])) { >> + rcu_assign_pointer(parent->eq.child_cq[i], queue); >> + return; >> + } >> } > > [Severity: High] > Does this registration cover every CQ the hardware can attach to an EQ? > > The capability bit added to GDMA_DRV_CAP_FLAGS1 requests throttled EQEs > for the whole function. However, child_cq[] is only filled here. It is > only filled when spec->cq.parent_eq is set, and only for the first two > CQs. > > The mana_ib CQs never go through this path. RNIC CQs are bound in > hardware to the mana_ib EQs: > > drivers/infiniband/hw/mana/main.c:mana_ib_gd_create_cq() { > ... > req.eq_id = mdev->eqs[cq->comp_vector]->id; > ... > } > > RSS and raw QP CQs are attached to the Ethernet per-vPort EQs: > > drivers/infiniband/hw/mana/qp.c:mana_ib_create_qp_rss() { > ... > eq = &mpc->eqs[cq->comp_vector % mpc->num_queues]; > cq_spec.attached_eq = eq->eq->id; > ... > } > > mana_ib_create_qp_raw() does the same. Both kinds of CQ are published only > through gc->cq_table[]: > > drivers/infiniband/hw/mana/cq.c:mana_ib_install_cq_cb() { > ... > gdma_cq->cq.callback = mana_ib_cq_handler; > gdma_cq->id = cq->queue.id; > gc->cq_table[cq->queue.id] = gdma_cq; > ... > } > > Also, mana_ib_create_kernel_queue() builds its GDMA_CQ spec without > spec.cq.parent_eq, so mana_gd_create_cq() returns before reaching this > loop. > > On an RDMA EQ, a throttle EQE would find every child_cq[] slot NULL and > only increment throttle_count. On an Ethernet EQ, the two slots already > hold the Ethernet TX and RX CQs, so the loop silently skips the RDMA CQ. > > In both cases, wouldn't mana_ib_cq_handler() lose its only notification > and the CQ stall? That is the stall the commit message says this patch > fixes. > > Would it work to dispatch to every CQ bound to the EQ, or to scope the > capability so it only covers EQs whose CQs are all tracked? > This is similar to sashiko review comment, after discussing this internally we decided to handle rdma seperately.>> >> static void mana_gd_destroy_cq(struct gdma_context *gc, >> struct gdma_queue *queue) >> { >> + struct gdma_queue *parent = queue->cq.parent; >> u32 id = queue->id; >> + u8 i; >> + >> + if (parent) { >> + for (i = 0; i < GDMA_EQ_MAX_CHILD_CQ; i++) { >> + if (rcu_access_pointer(parent->eq.child_cq[i]) == queue) >> + RCU_INIT_POINTER(parent->eq.child_cq[i], NULL); >> + } >> + >> + synchronize_rcu(); >> + } > > [Severity: Medium] > Does this add a full RCU grace period to every CQ destroy? > > synchronize_rcu() runs whenever the CQ has a parent, even when the CQ was > never found in child_cq[]. mana_destroy_txq() and mana_destroy_rxq() loop > over apc->num_queues and, for each queue, reach: > > mana_deinit_cq() > mana_gd_destroy_queue() > mana_gd_destroy_cq() > > These loops run under the netdev instance lock and RTNL. Ifdown, > set_channels, ring size, MTU and XDP changes, and the reset/servicing > paths would all wait for 2 * num_queues back-to-back grace periods while > holding RTNL. With 64 queues that is 128 serialized grace periods. > > Could all child slots be cleared first and synchronize_rcu() called once? > Alternatively, the free could be deferred with call_rcu() or kfree_rcu(). > >> >> if (id >= gc->max_num_cqs) >> return; > > [ ... ] > I will fix this in v2. >> diff --git a/include/net/mana/gdma.h b/include/net/mana/gdma.h >> index c610fc1067e0d..03fe9395604d6 100644 >> --- a/include/net/mana/gdma.h >> +++ b/include/net/mana/gdma.h > > [ ... ] > >> @@ -752,6 +758,9 @@ enum { >> /* Driver supports non-contiguous queue buffers */ >> #define GDMA_DRV_CAP_FLAG_1_NON_CONTIGUOUS_BUFFERS BIT(30) >> >> +/* Driver supports handling throttled EQEs */ >> +#define GDMA_DRV_CAP_FLAG_1_THROTTLED_EVENT_QUEUE BIT_ULL(32) >> + > > [Severity: Medium] > The commit message describes this as a fix: > > The driver does not handle this event type, so > it falls through to the default case and is dropped. Any CQ whose > completion notification was replaced by the throttle EQE is then never > scheduled again and its queue stalls. > > It doesn't mention that the patch also advertises a new capability bit to > the device. GDMA_DRV_CAP_FLAG_1_THROTTLED_EVENT_QUEUE is added to the > common GDMA_DRV_CAP_FLAGS1, which mana_gd_verify_vf_version() sends > unconditionally. > > If the device only sends type 4 EQEs after the driver sets this bit, can > the described stall happen on current kernels at all? If not, this is a > feature opt-in rather than a fix. > > If the stall can happen today, should the patch carry a Fixes: tag? > > The commit message also doesn't mention the new eq_throttle_events > counter in ethtool -S. Could both changes be described in the commit > message? > >> /* Capabilities in the PCI-only group below rely on dynamic MSI-X allocation >> * and on the servicing and reset paths reached through >> * mana_schedule_serv_work(). Transports that provide neither leave >> @@ -776,7 +785,8 @@ enum { >> GDMA_DRV_CAP_FLAG_1_HANDLE_STALL_SQ_RECOVERY | \ >> GDMA_DRV_CAP_FLAG_1_EQ_MSI_UNSHARE_MULTI_VPORT | \ >> GDMA_DRV_CAP_FLAG_1_DYN_INTERRUPT_MODERATION | \ >> - GDMA_DRV_CAP_FLAG_1_NON_CONTIGUOUS_BUFFERS) >> + GDMA_DRV_CAP_FLAG_1_NON_CONTIGUOUS_BUFFERS | \ >> + GDMA_DRV_CAP_FLAG_1_THROTTLED_EVENT_QUEUE) >> >> #define GDMA_DRV_CAP_FLAGS2 0 > I will fix this in v2.
pw-bot: cr
