Re: [PATCH net-next v2 1/5] ionic: Small improvements in devcmd retry logic

[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: Small improvements in devcmd retry logic

This commit aims to preserve the -EAGAIN error code when a device command
times out on its final retry. It also reduces the sleep time between retries
from 1000ms to 50ms to allow more attempts before timing out.

> diff --git a/drivers/net/ethernet/pensando/ionic/ionic_main.c b/drivers/net/ethernet/pensando/ionic/ionic_main.c
> index 3c5200e2fdb7d..92f2ec0bd5af9 100644
> --- a/drivers/net/ethernet/pensando/ionic/ionic_main.c
> +++ b/drivers/net/ethernet/pensando/ionic/ionic_main.c
> @@ -554,6 +554,11 @@ static int __ionic_dev_cmd_wait(struct ionic *ionic, unsigned long max_seconds,
>  
>  	if (!done && !time_before(jiffies, max_wait)) {
>  		ionic_dev_cmd_clean(ionic);
> +
> +		/* allow caller to manage EAGAIN from previous attempt */
> +		if (err == IONIC_RC_EAGAIN)
> +			return -EAGAIN;

Could err be uninitialized here?

Looking at __ionic_dev_cmd_wait(), err is declared at the start of the
function but is not assigned until later via ionic_dev_cmd_status(). If the
initial wait loop times out on the very first attempt (for example, if the
firmware is unresponsive or the system stalls), execution falls through to
this block. Checking err here would read an uninitialized stack variable,
potentially leading to unpredictable driver initialization behavior.

> +
>  		dev_warn(ionic->dev, "DEVCMD %s (%d) timeout after %ld secs\n",
>  			 ionic_opcode_to_str(opcode), opcode, max_seconds);
>  		return -ETIMEDOUT;

[ ... ]

> @@ -568,7 +573,7 @@ static int __ionic_dev_cmd_wait(struct ionic *ionic, unsigned long max_seconds,
>  				ionic_error_to_str(err), err);
>  
>  			iowrite32(0, &idev->dev_cmd_regs->done);
> -			msleep(1000);
> +			msleep(50);

Does this logic abandon retries prematurely?

The surrounding retry condition still requires at least one full second of
remaining time:

    if (err == IONIC_RC_EAGAIN &&
        time_before(jiffies, (max_wait - HZ))) {

The - HZ check was likely originally added to ensure there was enough time
left for the 1000ms sleep. Now that the sleep is reduced to 50ms, requiring
a full second of remaining time means the driver will abort retries during
the final second of the timeout. If max_seconds is small, it may not retry
at all, which seems to contradict the intent of increasing the number of
retries.

>  			iowrite32(1, &idev->dev_cmd_regs->doorbell);
>  			goto try_again;
>  		}
-- 
pw-bot: cr