Message-ID: <CAMet4B6+dUU+2pPhPm7hf08+Q=dpwwq3JeT__Sv8vmktgRvMBg@mail.gmail.com>
Date: Tue, 25 Nov 2025 12:03:48 +0530
From: Siva Reddy Kallam <siva.kallam@...adcom.com>
To: Simon Horman <horms@...nel.org>
Cc: leonro@...dia.com, jgg@...dia.com, linux-rdma@...r.kernel.org, 
	netdev@...r.kernel.org, vikas.gupta@...adcom.com, selvin.xavier@...adcom.com, 
	anand.subramanian@...adcom.com, usman.ansari@...adcom.com
Subject: Re: [PATCH v3 5/8] RDMA/bng_re: Add infrastructure for enabling
 Firmware channel

On Mon, Nov 24, 2025 at 10:14 PM Simon Horman <horms@...nel.org> wrote:
>
> On Mon, Nov 17, 2025 at 05:11:23PM +0000, Siva Reddy Kallam wrote:
>
> ...
>
> > diff --git a/drivers/infiniband/hw/bng_re/bng_dev.c b/drivers/infiniband/hw/bng_re/bng_dev.c
>
> ...
>
> > @@ -105,6 +105,69 @@ static void bng_re_fill_fw_msg(struct bnge_fw_msg *fw_msg, void *msg,
> >       fw_msg->timeout = timeout;
> >  }
> >
> > +static int bng_re_net_ring_free(struct bng_re_dev *rdev,
> > +                             u16 fw_ring_id, int type)
> > +{
> > +     struct bnge_auxr_dev *aux_dev = rdev->aux_dev;
>
> Hi Siva,
>
> rdev is dereferenced unconditionally here...
>
> > +     struct hwrm_ring_free_input req = {};
> > +     struct hwrm_ring_free_output resp;
> > +     struct bnge_fw_msg fw_msg = {};
> > +     int rc = -EINVAL;
> > +
> > +     if (!rdev)
> > +             return rc;
>
> ... but it is assumed that rdev may be NULL here.
>
> This does not seem consistent.
>
> IMHO a good approach would be to drop this check, and the one below,
> and only call bng_re_net_ring_free() in contexts where  rdev
> and aux_dev are not NULL.
>
> But I didn't look carefully to see if that idea matches the rest
> of the code.
>
> Flagged by Smatch.
Thanks Simon, it is a good catch. I will send a patch to fix this.
>
> > +
> > +     if (!aux_dev)
> > +             return rc;
> > +
> > +     bng_re_init_hwrm_hdr((void *)&req, HWRM_RING_FREE);
> > +     req.ring_type = type;
> > +     req.ring_id = cpu_to_le16(fw_ring_id);
> > +     bng_re_fill_fw_msg(&fw_msg, (void *)&req, sizeof(req), (void *)&resp,
> > +                         sizeof(resp), BNGE_DFLT_HWRM_CMD_TIMEOUT);
> > +     rc = bnge_send_msg(aux_dev, &fw_msg);
> > +     if (rc)
> > +             ibdev_err(&rdev->ibdev, "Failed to free HW ring:%d :%#x",
> > +                       req.ring_id, rc);
> > +     return rc;
> > +}
>
> ...
>
> > diff --git a/drivers/infiniband/hw/bng_re/bng_fw.c b/drivers/infiniband/hw/bng_re/bng_fw.c
>
> ...
>
> > +static int bng_re_process_qp_event(struct bng_re_rcfw *rcfw,
> > +                                struct creq_qp_event *qp_event,
> > +                                u32 *num_wait)
> > +{
> > +     struct bng_re_hwq *hwq = &rcfw->cmdq.hwq;
> > +     struct bng_re_crsqe *crsqe;
> > +     u32 req_size;
> > +     u16 cookie;
> > +     bool is_waiter_alive;
> > +     struct pci_dev *pdev;
> > +     u32 wait_cmds = 0;
> > +     int rc = 0;
>
> rc is always 0, so it may be slightly nicer to remove this variable and
> simply return 0.
>
> Flagged by Coccinelle.
Thanks Simon. I will send a patch to fix this.
>
> > +
> > +     pdev = rcfw->pdev;
> > +     switch (qp_event->event) {
> > +     case CREQ_QP_EVENT_EVENT_QP_ERROR_NOTIFICATION:
> > +             dev_err(&pdev->dev, "Received QP error notification\n");
> > +             break;
> > +     default:
> > +             /*
> > +              * Command Response
> > +              * cmdq->lock needs to be acquired to synchronie
> > +              * the command send and completion reaping. This function
> > +              * is always called with creq->lock held. Using
> > +              * the nested variant of spin_lock.
> > +              *
> > +              */
> > +
> > +             spin_lock_nested(&hwq->lock, SINGLE_DEPTH_NESTING);
> > +             cookie = le16_to_cpu(qp_event->cookie);
> > +             cookie &= BNG_FW_MAX_COOKIE_VALUE;
> > +             crsqe = &rcfw->crsqe_tbl[cookie];
> > +
> > +             if (WARN_ONCE(test_bit(FIRMWARE_STALL_DETECTED,
> > +                                    &rcfw->cmdq.flags),
> > +                 "Unreponsive rcfw channel detected.!!")) {
> > +                     dev_info(&pdev->dev,
> > +                              "rcfw timedout: cookie = %#x, free_slots = %d",
> > +                              cookie, crsqe->free_slots);
> > +                     spin_unlock(&hwq->lock);
> > +                     return rc;
> > +             }
> > +
> > +             if (crsqe->is_waiter_alive) {
> > +                     if (crsqe->resp) {
> > +                             memcpy(crsqe->resp, qp_event, sizeof(*qp_event));
> > +                             /* Insert write memory barrier to ensure that
> > +                              * response data is copied before clearing the
> > +                              * flags
> > +                              */
> > +                             smp_wmb();
> > +                     }
> > +             }
> > +
> > +             wait_cmds++;
> > +
> > +             req_size = crsqe->req_size;
> > +             is_waiter_alive = crsqe->is_waiter_alive;
> > +
> > +             crsqe->req_size = 0;
> > +             if (!is_waiter_alive)
> > +                     crsqe->resp = NULL;
> > +
> > +             crsqe->is_in_used = false;
> > +
> > +             hwq->cons += req_size;
> > +
> > +             spin_unlock(&hwq->lock);
> > +     }
> > +     *num_wait += wait_cmds;
> > +     return rc;
> > +}
>
> ...

Download attachment "smime.p7s" of type "application/pkcs7-signature" (5471 bytes)

Powered by blists - more mailing lists