Re: [PULL 55/63] hw/cxl/mbox: Add support for background operations

[Davidlohr Bueso]

On Thu, 09 Nov 2023, Peter Maydell wrote:

> On Tue, 7 Nov 2023 at 10:13, Michael S. Tsirkin <mst@redhat.com> wrote:
> > From: Davidlohr Bueso <dave@stgolabs.net>
> >
> > Support background commands in the mailbox, and update
> > cmd_infostat_bg_op_sts() accordingly. This patch does not implement mbox
> > interrupts upon completion, so the kernel driver must rely on polling to
> > know when the operation is done.
> >
> > Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>
> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> >
> > Message-Id: <20231023160806.13206-12-Jonathan.Cameron@huawei.com>
> > Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>
> Hi; Coverity points out dead code in this function (CID 1523907):
>
> > +static void bg_timercb(void *opaque)
> > +{
> > +    CXLCCI *cci = opaque;
> > +    CXLDeviceState *cxl_dstate = &CXL_TYPE3(cci->d)->cxl_dstate;
> > +    uint64_t bg_status_reg = 0;
> > +    uint64_t now = qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL);
> > +    uint64_t total_time = cci->bg.starttime + cci->bg.runtime;
> > +
> > +    assert(cci->bg.runtime > 0);
> > +    bg_status_reg = FIELD_DP64(bg_status_reg, CXL_DEV_BG_CMD_STS,
> > +                               OP, cci->bg.opcode);
> > +
> > +    if (now >= total_time) { /* we are done */
> > +        uint64_t status_reg;
> > +        uint16_t ret = CXL_MBOX_SUCCESS;
>
> Here we set 'ret' to CXL_MBOX_SUCCESS...
>
> > +
> > +        cci->bg.complete_pct = 100;
> > +        /* Clear bg */
> > +        status_reg = FIELD_DP64(0, CXL_DEV_MAILBOX_STS, BG_OP, 0);
> > +        cxl_dstate->mbox_reg_state64[R_CXL_DEV_MAILBOX_STS] = status_reg;
> > +
> > +        bg_status_reg = FIELD_DP64(bg_status_reg, CXL_DEV_BG_CMD_STS,
> > +                                   RET_CODE, ret);
>
> ...and nothing here changes 'ret'...
>
> > +
> > +        /* TODO add ad-hoc cmd succesful completion handling */
> > +
> > +        qemu_log("Background command %04xh finished: %s\n",
> > +                 cci->bg.opcode,
> > +                 ret == CXL_MBOX_SUCCESS ? "success" : "aborted");
>
> ...but here we check whether ret is CXL_MBOX_SUCCESS or not:
> the "aborted" half of this condition is dead code.

Yeah this had also been removed in a later version:
https://lore.kernel.org/linux-cxl/20230418172337.19207-2-dave@stgolabs.net/

> A later commit adds an "if (ret == CXL_MBOX_SUCCESS) {" block
> where the TODO currently is, and that is an unnecessary check,
> because ret cannot be anything else at that point.

I'll send a patch along with the other dead code report.

Thanks,
Davidlohr