On Tue, 7 Nov 2023 at 10:13, Michael S. Tsirkin <[email protected]> wrote:
>
> From: Davidlohr Bueso <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Jonathan Cameron <[email protected]>
>
> Message-Id: <[email protected]>
> Reviewed-by: Michael S. Tsirkin <[email protected]>
> Signed-off-by: Michael S. Tsirkin <[email protected]>
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.
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.
> + } else {
> + /* estimate only */
> + cci->bg.complete_pct = 100 * now / total_time;
> + timer_mod(cci->bg.timer, now + CXL_MBOX_BG_UPDATE_FREQ);
> + }
What was the intention here ?
thanks
-- PMM
