Re: [PATCH net v2 1/5] ionic: Allow the first devcmd to trigger deferred probe

[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: Allow the first devcmd to trigger deferred probe
    
    There's a chance the register signature value is set before the
    firmware is ready to respond to the driver. This doesn't mean the
    device isn't there, but just means it's not yet ready. If the first
    devcmd fails, then return -EPROBE_DEFER so the device can be probed
    at a later time. As part of this make sure the reset devcmd, which
    is the first devcmd, is not so alarming when it fails by printing
    an information message instead of the standard devcmd failure
    messages.

[ ... ]

> diff --git a/drivers/net/ethernet/pensando/ionic/ionic_bus_pci.c b/drivers/net/ethernet/pensando/ionic/ionic_bus_pci.c
> index 05f19489ec5c8..59ce35404e532 100644
> --- a/drivers/net/ethernet/pensando/ionic/ionic_bus_pci.c
> +++ b/drivers/net/ethernet/pensando/ionic/ionic_bus_pci.c
> @@ -260,7 +260,8 @@ static int ionic_setup_one(struct ionic *ionic)
>  	/* Configure the device */
>  	err = ionic_setup(ionic);
>  	if (err) {
> -		dev_err(dev, "Cannot setup device: %d, aborting\n", err);
> +		if (err != -EPROBE_DEFER)
> +			dev_err(dev, "Cannot setup device: %d, aborting\n", err);
>  		goto err_out_clear_pci;

Does this code leak the workqueue?

ionic_setup() first calls ionic_dev_setup(), which allocates the workqueue
via ionic_watchdog_init(). If ionic_reset() then fails and returns
-EPROBE_DEFER, the goto here jumps to err_out_clear_pci.

This bypasses err_out_teardown and skips ionic_dev_teardown(), which is
responsible for calling destroy_workqueue(ionic->wq). Because -EPROBE_DEFER
will cause the kernel to retry probing, could this leak a new workqueue on
every failed probe attempt?

>  	}

[ ... ]

> diff --git a/drivers/net/ethernet/pensando/ionic/ionic_main.c b/drivers/net/ethernet/pensando/ionic/ionic_main.c
> index 3c5200e2fdb7d..91f89b9ff8075 100644
> --- a/drivers/net/ethernet/pensando/ionic/ionic_main.c
> +++ b/drivers/net/ethernet/pensando/ionic/ionic_main.c
> @@ -603,7 +603,11 @@ int ionic_setup(struct ionic *ionic)
>  	err = ionic_dev_setup(ionic);
>  	if (err)
>  		return err;
> -	ionic_reset(ionic);
> +
> +	err = ionic_reset(ionic);
> +	/* firmware may not be ready to respond yet */
> +	if (err == -EAGAIN || err == -ETIMEDOUT)
> +		return -EPROBE_DEFER;

Will this permanently fail the probe with -ENXIO if the firmware is so early
in its boot process that the IONIC_FW_STS_F_RUNNING bit isn't set yet?

If ionic_is_fw_running() is false, ionic_reset() skips execution and
returns 0:

drivers/net/ethernet/pensando/ionic/ionic_main.c:ionic_reset() {
	if (!ionic_is_fw_running(idev))
		return 0;
    ...
}

If this happens, ionic_setup() returns success, and the driver proceeds to
ionic_identify(). When it issues a command and calls __ionic_dev_cmd_wait(),
the wait loop is skipped entirely because the firmware isn't running,
returning -ENXIO. This bypasses the -EPROBE_DEFER logic here and causes a
permanent probe failure.

>  
>  	return 0;
>  }
> @@ -687,7 +691,7 @@ int ionic_reset(struct ionic *ionic)
>  
>  	mutex_lock(&ionic->dev_cmd_lock);
>  	ionic_dev_cmd_reset(idev);
> -	err = ionic_dev_cmd_wait(ionic, DEVCMD_TIMEOUT);
> +	err = ionic_dev_cmd_wait_nomsg(ionic, DEVCMD_TIMEOUT);

Does this actually suppress the timeout warning?

The commit message says this aims to make the first devcmd failure "not so
alarming" by suppressing standard devcmd failure messages. However,
ionic_dev_cmd_wait_nomsg() passes do_msg = false to __ionic_dev_cmd_wait(),
which only suppresses firmware-returned error codes parsed by
ionic_dev_cmd_dev_err_print().

If the command times out, __ionic_dev_cmd_wait() will still unconditionally
print the dev_warn() for the timeout, so the alarming message might still be
printed.

>  	mutex_unlock(&ionic->dev_cmd_lock);