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

[Tomasz Lichwala <tomasz.lichwala@linux.intel.com>]

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