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

["Rao, Nikhil" <nikhirao@amd.com>]

On 11/3/2026 2:28 PM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
>> +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().

On remove, get runs from devlink_unregister() with the firmware running
and completes, or fails immediately if the firmware is already down.

> 
> 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?
> 

__ionic_dev_cmd_wait() returns -ENXIO as soon as fw_status shows the
firmware is not running, which is the case after firmware down (running
bit clear) and with a frozen channel (reads return all-ones). For a PCI
reset, get is only called from reset_prepare, before the reset starts
and while the firmware is running, and the parameter stays unregistered
until reset_done.

get has no FW_RESET check on purpose: devlink calls it from
reset_prepare, while the firmware is still running, to fill the
PARAM_DEL notification, and the check would make it fail there and drop
that notification.

I'll send a v2 with a comment in get explaining why it has no FW_RESET
check.

>> @@ -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()?
> 

The next sample can be up to 2 * cmd_timeout away after firmware up, but
that is the normal sampling interval.

The back-dating at probe and PCI reset exists because the first sample
there only records a baseline (last_fw_hb is 0), so without it a stall
would take 4 * cmd_timeout to detect.

After firmware up, last_fw_hb already holds a real reading, so the first
sample can detect a stall and detection takes 2 * cmd_timeout, as in
steady state.

v2 also changes the changelog's "after probe or reset" to "after probe
or PCI reset".

Nikhil