On Wed, Aug 13, 2025 at 03:13:15PM -0700, Mohsin Bashir wrote:
> @@ -1251,6 +1293,7 @@ static void fbnic_free_napi_vector(struct fbnic_net *fbn,
> }
>
> for (j = 0; j < nv->rxt_count; j++, i++) {
> + xdp_rxq_info_unreg(&nv->qt[i].xdp_rxq);
> fbnic_remove_rx_ring(fbn, &nv->qt[i].sub0);
> fbnic_remove_rx_ring(fbn, &nv->qt[i].sub1);
> fbnic_remove_rx_ring(fbn, &nv->qt[i].cmpl);
> @@ -1423,6 +1466,11 @@ static int fbnic_alloc_napi_vector(struct fbnic_dev *fbd, struct fbnic_net *fbn,
> fbnic_ring_init(&qt->cmpl, db, rxq_idx, FBNIC_RING_F_STATS);
> fbn->rx[rxq_idx] = &qt->cmpl;
>
> + err = xdp_rxq_info_reg(&qt->xdp_rxq, fbn->netdev, rxq_idx,
> + nv->napi.napi_id);
> + if (err)
> + goto free_ring_cur_qt;
> +
> /* Update Rx queue index */
> rxt_count--;
> rxq_idx += v_count;
> @@ -1433,6 +1481,25 @@ static int fbnic_alloc_napi_vector(struct fbnic_dev *fbd, struct fbnic_net *fbn,
>
> return 0;
>
> + while (rxt_count < nv->rxt_count) {
^^^^^^^^^^^^^^^^^^^^^^^^^
This should be <= otherwise it won't free enough. Then qt will point to
the wrong thing and the next loop will crash.
The loops in this function are mind bendingly complicated. It might be
easiter to write them as:
for (i = 0; i < nv->txt_count; i++) {
qt = &nv->qt[i];
...
}
for (i = 0; i < nv->rxt_count; i++) {
qt = &nv->qt[txt_count + i];
...
}
Generally, I would just unwind the partial loop before the goto instead
of doing a jump to the middle of the goto. It's more lines of code, but
I'm stupid, so I prefer code which is easy even if it's longer.
regards,
dan carpenter
> + qt--;
> +
> + xdp_rxq_info_unreg(&qt->xdp_rxq);
> +free_ring_cur_qt:
> + fbnic_remove_rx_ring(fbn, &qt->sub0);
> + fbnic_remove_rx_ring(fbn, &qt->sub1);
> + fbnic_remove_rx_ring(fbn, &qt->cmpl);
> + rxt_count++;
> + }
> + while (txt_count < nv->txt_count) {
> + qt--;
> +
> + fbnic_remove_tx_ring(fbn, &qt->sub0);
> + fbnic_remove_tx_ring(fbn, &qt->cmpl);
> +
> + txt_count++;
> + }
> + fbnic_napi_free_irq(fbd, nv);
> pp_destroy:
> page_pool_destroy(nv->page_pool);
> napi_del:
