Re: [PATCH net 5/6] iavf: fix VF stats not updating due to PTP command preemption

Re: [PATCH net 5/6] iavf: fix VF stats not updating due to PTP command preemption

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] iavf_watchdog_step()
  (drivers/net/ethernet/intel/iavf/iavf_main.c) now calls…

--

iavf: fix VF stats not updating due to PTP command preemption

With this commit, iavf_watchdog_step() requests VF stats on every pass
while the adapter is in __IAVF_RUNNING. Before, it only did so when
iavf_process_aq_command() found nothing to do. The periodic PHC caching
task keeps a PTP command pending, so the old stats fallback never ran.

> diff --git a/drivers/net/ethernet/intel/iavf/iavf_main.c b/drivers/net/ethernet/intel/iavf/iavf_main.c
> index 29b8403a066bc..c0686ad5c4116 100644
> --- a/drivers/net/ethernet/intel/iavf/iavf_main.c
> +++ b/drivers/net/ethernet/intel/iavf/iavf_main.c
> @@ -2932,18 +2932,12 @@ static int iavf_watchdog_step(struct iavf_adapter *adapter)
>  				iavf_send_api_ver(adapter);
>  			}
>  		} else {
> -			int ret = iavf_process_aq_command(adapter);
> -
> -			/* An error will be returned if no commands were
> -			 * processed; use this opportunity to update stats
> -			 * if the error isn't -ENOTSUPP
> -			 */
> -			if (ret && ret != -EOPNOTSUPP &&
> -			    adapter->state == __IAVF_RUNNING)
> -				iavf_request_stats(adapter);
> +			iavf_process_aq_command(adapter);
>  		}
> -		if (adapter->state == __IAVF_RUNNING)
> +		if (adapter->state == __IAVF_RUNNING) {
> +			iavf_request_stats(adapter);

[Severity: Medium]
Can this put two virtchnl messages in flight during one watchdog pass?

Suppose iavf_process_aq_command() has just taken the
IAVF_FLAG_AQ_SEND_PTP_CMD branch. In that case
iavf_virtchnl_send_ptp_cmd() has sent VIRTCHNL_OP_1588_PTP_GET_TIME
without setting adapter->current_op:

iavf_virtchnl.c:iavf_virtchnl_send_ptp_cmd() {
    ...
	err = iavf_send_pf_msg(adapter, cmd->v_opcode, cmd->msg, cmd->msglen);
	if (!err) {
		list_del(&cmd->list);
		kfree(cmd);
    ...
}

The only guard in iavf_request_stats() then still sees an idle channel,
so VIRTCHNL_OP_GET_STATS goes out right behind GET_TIME:

	if (adapter->current_op != VIRTCHNL_OP_UNKNOWN) {
		/* no error message, this isn't crucial */
		return;
	}
	...
	adapter->current_op = VIRTCHNL_OP_GET_STATS;

When the GET_TIME reply arrives, iavf_virtchnl_completion() breaks out
of the switch and reaches the unconditional reset at the end:

	} /* switch v_opcode */
	adapter->current_op = VIRTCHNL_OP_UNKNOWN;

That clears the GET_STATS tracking while the GET_STATS reply is still
outstanding.

iavf_adminq_task() may drop the netdev lock before the GET_STATS reply
arrives. A watchdog pass can then send another tracked command. The
watchdog can be kicked by iavf_schedule_aq_request(), by the 20ms re-arm
while aq_required is set, or by gettimex64() queuing another PTP read
with mod_delayed_work(..., 0). The late GET_STATS reply would then clear
that command's current_op as well.

Flow Director is one place where this could do real damage.
iavf_add_fdir_filter() relies on current_op to keep only one
ADD_PENDING filter in flight. If two adds overlap, the first successful
VIRTCHNL_OP_ADD_FDIR_FILTER reply does this for every pending filter:

			if (fdir->state == IAVF_FDIR_FLTR_ADD_PENDING) {
				if (add_fltr->status == VIRTCHNL_FDIR_SUCCESS) {
					...
					fdir->state = IAVF_FDIR_FLTR_ACTIVE;
					fdir->flow_id = add_fltr->flow_id;

Would the second filter then get the wrong PF flow_id? If so, a later
delete of that filter could remove the wrong PF rule and leak the real
one.

The missing current_op tracking in the PTP sender predates this patch.
Before this change, though, GET_STATS was only sent in a pass where
iavf_process_aq_command() returned an error, so it never followed a PTP
message back to back. Now every PTP send is followed by a GET_STATS in
the same pass. That covers the roughly once-per-second PHC cache update
and every gettimex64() read.

This also adds one GET_STATS mailbox message per PHC read, which the
commit message doesn't mention.

>  			iavf_detect_recover_hung(&adapter->vsi);
> +		}
>  		break;

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260928230429.495442-1-anthony.l.nguyen%40intel.com

Re: [PATCH net 5/6] iavf: fix VF stats not updating due to PTP command preemption

[Tomasz Lichwala]

On 30.09.2026 02:58, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 0 · Medium: 1 · Low: 0
> 
> If you believe none of the issues are legitimate, please reply to each
> issue of Medium severity or higher and explain why the AI is wrong.
> If any of them are legitimate and you plan to respin, please let patchwork
> know by including "pw-bot: cr" as a separate line at the end of your reply
> (one such reply per series is enough).
> 
> - [Medium] iavf_watchdog_step()
>   (drivers/net/ethernet/intel/iavf/iavf_main.c) now calls…
> 
> --
> 
> iavf: fix VF stats not updating due to PTP command preemption
> 
> With this commit, iavf_watchdog_step() requests VF stats on every pass
> while the adapter is in __IAVF_RUNNING. Before, it only did so when
> iavf_process_aq_command() found nothing to do. The periodic PHC caching
> task keeps a PTP command pending, so the old stats fallback never ran.
> 
>> diff --git a/drivers/net/ethernet/intel/iavf/iavf_main.c b/drivers/net/ethernet/intel/iavf/iavf_main.c
>> index 29b8403a066bc..c0686ad5c4116 100644
>> --- a/drivers/net/ethernet/intel/iavf/iavf_main.c
>> +++ b/drivers/net/ethernet/intel/iavf/iavf_main.c
>> @@ -2932,18 +2932,12 @@ static int iavf_watchdog_step(struct iavf_adapter *adapter)
>>  				iavf_send_api_ver(adapter);
>>  			}
>>  		} else {
>> -			int ret = iavf_process_aq_command(adapter);
>> -
>> -			/* An error will be returned if no commands were
>> -			 * processed; use this opportunity to update stats
>> -			 * if the error isn't -ENOTSUPP
>> -			 */
>> -			if (ret && ret != -EOPNOTSUPP &&
>> -			    adapter->state == __IAVF_RUNNING)
>> -				iavf_request_stats(adapter);
>> +			iavf_process_aq_command(adapter);
>>  		}
>> -		if (adapter->state == __IAVF_RUNNING)
>> +		if (adapter->state == __IAVF_RUNNING) {
>> +			iavf_request_stats(adapter);
> 
> [Severity: Medium]
> Can this put two virtchnl messages in flight during one watchdog pass?
> 
> Suppose iavf_process_aq_command() has just taken the
> IAVF_FLAG_AQ_SEND_PTP_CMD branch. In that case
> iavf_virtchnl_send_ptp_cmd() has sent VIRTCHNL_OP_1588_PTP_GET_TIME
> without setting adapter->current_op:
> 
> iavf_virtchnl.c:iavf_virtchnl_send_ptp_cmd() {
>     ...
> 	err = iavf_send_pf_msg(adapter, cmd->v_opcode, cmd->msg, cmd->msglen);
> 	if (!err) {
> 		list_del(&cmd->list);
> 		kfree(cmd);
>     ...
> }
> 
> The only guard in iavf_request_stats() then still sees an idle channel,
> so VIRTCHNL_OP_GET_STATS goes out right behind GET_TIME:
> 
> 	if (adapter->current_op != VIRTCHNL_OP_UNKNOWN) {
> 		/* no error message, this isn't crucial */
> 		return;
> 	}
> 	...
> 	adapter->current_op = VIRTCHNL_OP_GET_STATS;
> 
> When the GET_TIME reply arrives, iavf_virtchnl_completion() breaks out
> of the switch and reaches the unconditional reset at the end:
> 
> 	} /* switch v_opcode */
> 	adapter->current_op = VIRTCHNL_OP_UNKNOWN;
> 
> That clears the GET_STATS tracking while the GET_STATS reply is still
> outstanding.
> 
> iavf_adminq_task() may drop the netdev lock before the GET_STATS reply
> arrives. A watchdog pass can then send another tracked command. The
> watchdog can be kicked by iavf_schedule_aq_request(), by the 20ms re-arm
> while aq_required is set, or by gettimex64() queuing another PTP read
> with mod_delayed_work(..., 0). The late GET_STATS reply would then clear
> that command's current_op as well.
> 
> Flow Director is one place where this could do real damage.
> iavf_add_fdir_filter() relies on current_op to keep only one
> ADD_PENDING filter in flight. If two adds overlap, the first successful
> VIRTCHNL_OP_ADD_FDIR_FILTER reply does this for every pending filter:
> 
> 			if (fdir->state == IAVF_FDIR_FLTR_ADD_PENDING) {
> 				if (add_fltr->status == VIRTCHNL_FDIR_SUCCESS) {
> 					...
> 					fdir->state = IAVF_FDIR_FLTR_ACTIVE;
> 					fdir->flow_id = add_fltr->flow_id;
> 
> Would the second filter then get the wrong PF flow_id? If so, a later
> delete of that filter could remove the wrong PF rule and leak the real
> one.
> 
> The missing current_op tracking in the PTP sender predates this patch.
> Before this change, though, GET_STATS was only sent in a pass where
> iavf_process_aq_command() returned an error, so it never followed a PTP
> message back to back. Now every PTP send is followed by a GET_STATS in
> the same pass. That covers the roughly once-per-second PHC cache update
> and every gettimex64() read.
> 
> This also adds one GET_STATS mailbox message per PHC read, which the
> commit message doesn't mention.
> 
>>  			iavf_detect_recover_hung(&adapter->vsi);
>> +		}
>>  		break;
> 
> [ ... ]
> 



Thank you, this is a valid finding. iavf_virtchnl_send_ptp_cmd() does not set adapter->current_op before sending VIRTCHNL_OP_1588_PTP_GET_TIME, unlike every other virtchnl sender, and iavf_virtchnl_completion() unconditionally clears current_op on any reply. This gap predates this patch, but making the stats request unconditional every watchdog pass makes it trivially reachable, since GET_STATS can now follow a PTP send back-to-back in the same pass - something the old fallback design structurally prevented.

This will be fixed.

Thanks,
Tomasz