Re: [PATCH net-next 3/3] ionic: add devlink param for command timeout

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] ionic: hardware getattr issued during reset in ionic_dl_cmd_timeout_get()
- [Medium] ionic: last_hb_time not back-dated on reset in ionic_lif_handle_fw_up()
--

commit 0a0b7719e75bbcb6ea7fc807a50ee80589eafae1
Author: Arun Kumar Prakash <ArunKumar.Prakash@amd.com>

ionic: add devlink param for command timeout

This commit adds a permanent cmd_timeout devlink parameter stored in device
NVM to configure device command, admin queue, and firmware heartbeat check
timeouts on supported firmware.

> diff --git a/drivers/net/ethernet/pensando/ionic/ionic_devlink.c b/drivers/net/ethernet/pensando/ionic/ionic_devlink.c
> index 4ec66a6be0738..d775fa5bb43d7 100644
> --- a/drivers/net/ethernet/pensando/ionic/ionic_devlink.c
> +++ b/drivers/net/ethernet/pensando/ionic/ionic_devlink.c
> @@ -51,6 +51,55 @@ static int ionic_dl_info_get(struct devlink *dl, struct devlink_info_req *req,
>  	return err;
>  }
>  
> +enum ionic_devlink_param_id {
> +	IONIC_DEVLINK_PARAM_ID_BASE = DEVLINK_PARAM_GENERIC_ID_MAX,
> +	IONIC_DEVLINK_PARAM_ID_CMD_TIMEOUT,
> +};
> +
> +static int ionic_dl_cmd_timeout_get(struct devlink *dl, u32 id,
> +				    struct devlink_param_gset_ctx *ctx,
> +				    struct netlink_ext_ack *extack)
> +{
> +	struct ionic *ionic = devlink_priv(dl);
> +
> +	return ionic_dev_getattr_cmd_timeout(ionic, &ctx->val.vu8);
> +}
> +
> +static int ionic_dl_cmd_timeout_set(struct devlink *dl, u32 id,
> +				    struct devlink_param_gset_ctx *ctx,
> +				    struct netlink_ext_ack *extack)
> +{
> +	struct ionic *ionic = devlink_priv(dl);
> +
> +	if (test_bit(IONIC_LIF_F_FW_RESET, ionic->lif->state))
> +		return -EBUSY;
> +
> +	return ionic_dev_setattr_cmd_timeout(ionic, ctx->val.vu8);
> +}

[Severity: Medium]
Unlike ionic_dl_cmd_timeout_set(), ionic_dl_cmd_timeout_get() does not check
IONIC_LIF_F_FW_RESET (or IONIC_LIF_F_FW_STOPPING) before calling
ionic_dev_getattr_cmd_timeout().

During firmware down or PCI reset preparation in ionic_lif_handle_fw_down()
and ionic_reset_prepare() (including when ionic_pci_error_detected() is
called with pci_channel_io_frozen), IONIC_LIF_F_FW_RESET is set and then
ionic_devlink_params_unregister() is called while the devlink instance is
still registered:

ionic_lif_handle_fw_down() / ionic_reset_prepare()
  ionic_devlink_params_unregister()
    devl_params_unregister()
      devlink_param_unregister()
        devlink_param_notify(..., DEVLINK_CMD_PARAM_DEL)
          devlink_nl_param_fill()
            ionic_dl_cmd_timeout_get()
              ionic_dev_getattr_cmd_timeout()

Similarly, ionic_devlink_unregister() calls devlink_unregister() before
ionic_devlink_params_unregister(), which also invokes devlink_nl_param_fill()
via devlink_params_notify_unregister().

If a userspace netlink listener is monitoring devlink events, or if a user
queries devlink parameters during the window before the parameter is
unregistered, won't ionic_dl_cmd_timeout_get() issue a synchronous MMIO
IONIC_CMD_GETATTR command while the firmware is down, the device is in
reset, or the PCI channel is frozen, potentially stalling reset recovery for
up to cmd_timeout (255) seconds under devl_lock and dev_cmd_lock?

[ ... ]

> diff --git a/drivers/net/ethernet/pensando/ionic/ionic_lif.c b/drivers/net/ethernet/pensando/ionic/ionic_lif.c
> index b0868ab139dd5..a89f9c72eb39d 100644
> --- a/drivers/net/ethernet/pensando/ionic/ionic_lif.c
> +++ b/drivers/net/ethernet/pensando/ionic/ionic_lif.c

[ ... ]

> @@ -3495,6 +3497,7 @@ static void ionic_lif_handle_fw_up(struct ionic_lif *lif)
>  	err = ionic_identify(ionic);
>  	if (err)
>  		goto err_out;
> +	ionic_update_cmd_timeout(ionic);
>  	err = ionic_port_identify(ionic);
>  	if (err)
>  		goto err_out;

[Severity: Medium]
The commit message notes that idev->last_hb_time is back-dated by the
largest possible interval so that a larger cmd_timeout value does not delay
the first heartbeat sample after probe or reset.

However, ionic_watchdog_init() is only called from ionic_dev_setup() during
initial probe and PCI function reset, and is not called when recovering from
a firmware reset in ionic_lif_handle_fw_up().

When firmware comes back up in ionic_lif_handle_fw_up() and
ionic_update_cmd_timeout() applies a large timeout, idev->last_hb_time still
holds the timestamp from before the firmware reset. In
ionic_heartbeat_check():

	last_check_time = idev->last_hb_time;
	if (time_before(check_time,
			last_check_time +
			READ_ONCE(ionic->cmd_timeout) * 2 * HZ))
		return 0;

Could this cause ionic_heartbeat_check() to skip sampling fw_heartbeat for
up to 2 * cmd_timeout (510 seconds) after a firmware reset unless
idev->last_hb_time is also back-dated in ionic_lif_handle_fw_up()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261102-b4-ionic-cmd-timeout-v1-0-2f75c2d0a0c7@amd.com?part=3