Re: [PATCH net v2 3/5] ionic: Fix unexpected dev_cmd failures

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
ionic: Fix unexpected dev_cmd failures

This commit adds a final check of the done bit after exiting the dev_cmd
poll loop. It attempts to prevent timing out commands that have actually
completed.

> diff --git a/drivers/net/ethernet/pensando/ionic/ionic_main.c b/drivers/net/ethernet/pensando/ionic/ionic_main.c
> --- a/drivers/net/ethernet/pensando/ionic/ionic_main.c
> +++ b/drivers/net/ethernet/pensando/ionic/ionic_main.c
> @@ -541,6 +541,10 @@ static int __ionic_dev_cmd_wait(struct ionic *ionic, unsigned long max_seconds,
>  	}
>  	duration = jiffies - start_time;
>  
> +	/* one final check to prevent unexpected timeout */
> +	if (!done)
> +		done = ionic_dev_cmd_done(idev);

If the polling loop exits early because the firmware crashed or the device
was hot-removed, fw_up evaluates to false. Will this final check then read
from a dead device?

On a disconnected device, MMIO reads typically return all 1s (0xffffffff).
Looking at ionic_dev_cmd_done():

drivers/net/ethernet/pensando/ionic/ionic_dev.c:ionic_dev_cmd_done() {
        ...
        return ioread32(&idev->dev_cmd_regs->done) & IONIC_DEV_CMD_DONE;
}

Since IONIC_DEV_CMD_DONE is 0x1, the bitwise AND with 0xffffffff evaluates
to true, making done = true.

If done becomes true on a dead device, does it bypass the firmware down
error path further down in this function?

__ionic_dev_cmd_wait() {
        ...
        if (!done && !fw_up) {
                ionic_dev_cmd_clean(ionic);
                dev_warn(ionic->dev, "DEVCMD %s (%d) interrupted - FW is down\n",
                         ionic_opcode_to_str(opcode), opcode);
                return -ENXIO;
        }
        ...
}

It appears the driver will instead proceed to ionic_dev_cmd_status(), read
0xff from the status register, log an unknown error, and return -EIO rather
than correctly handling the device removal with -ENXIO.