Re: [PATCH net v2 07/15] ice: E822: cancel offset verification work during reset preparation

[netdev-bot+sashiko@kernel.org]

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] The commit message gives a false reason for leaving PF resets
  out of the new cancellation.

--

ice: E822: cancel offset verification work during reset preparation

This patch cancels the E822 offset verification work (ov_work) in
ice_ptp_prepare_for_reset() for CORE, GLOBAL and EMP resets, so the
work does not keep polling while the PHY is restarted. PF resets
return early and skip the cancel. The commit message says this is
because nothing else would restart the task after a PF reset.

> The larger resets will restart the PHY, canceling any outstanding vernier
> calibration and forcing a restart. Preemptively cancel the task for non-PF
> resets to avoid doing unnecessary checks while resetting. The task is not
> canceled for a PF reset, otherwise nothing would restart the task to finish
> vernier calibration after the reset.

[ ... ]

> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c
> index 7f82439c47ee5..7cc151b01a13e 100644
> --- a/drivers/net/ethernet/intel/ice/ice_ptp.c
> +++ b/drivers/net/ethernet/intel/ice/ice_ptp.c
> @@ -3006,6 +3006,9 @@ void ice_ptp_prepare_for_reset(struct ice_pf *pf, enum ice_reset_req reset_type)
>  	if (reset_type == ICE_RESET_PFR)
>  		return;
>  
> +	if (hw->mac_type == ICE_MAC_GENERIC)
> +		kthread_cancel_delayed_work_sync(&ptp->port.ov_work);
> +

[Severity: Medium]
Is the statement "nothing would restart the task to finish vernier
calibration after the reset" accurate for this tree?

Near the end of ice_rebuild(), ice_ptp_link_change() is called for
every reset type, PF reset included:

drivers/net/ethernet/intel/ice/ice_main.c:ice_rebuild() {
    ...
	if (test_bit(ICE_FLAG_PTP_SUPPORTED, pf->flags) && pf->hw.port_info) {
		bool link_up;

		link_up = !!(pf->hw.port_info->phy.link_info.link_info &
			     ICE_AQ_LINK_UP);
		ice_ptp_link_change(pf, link_up);
	}
    ...
}

ice_misc_intr() sets hw->reset_ongoing only on the OICR path for
CORER/GLOBR/EMPR. For a PF reset, ice_ptp_link_change() therefore does
not skip the hardware writes. On ICE_MAC_GENERIC it reaches
ice_ptp_port_phy_restart(), which cancels ov_work and queues it again:

drivers/net/ethernet/intel/ice/ice_ptp.c:ice_ptp_port_phy_restart() {
    ...
	case ICE_MAC_GENERIC:
		/* Start the PHY timer in Vernier mode */
		kthread_cancel_delayed_work_sync(&ptp_port->ov_work);
		...
		kthread_queue_delayed_work(pf->ptp.kworker, &ptp_port->ov_work,
					   0);
		break;
    ...
}

When the link is down, the same function calls ice_ptp_port_phy_stop()
instead, which also cancels ov_work.

The call chain for a PF reset looks like this:

ice_do_reset(PFR)->ice_rebuild()->ice_ptp_link_change()
    ->ice_ptp_port_phy_restart()

If that is right, cancelling ov_work for PF resets would also be safe.
With the early ICE_RESET_PFR return placed before the new cancel, PF
resets still go through the 100 ms reschedule loop in
ice_ptp_wait_for_offsets(), because ICE_PFR_REQ makes
ice_is_reset_in_progress() return true.

Could the cancel be moved above the PFR early return? If not, could the
commit message give the actual reason for exempting PF resets?

>  	if (ice_pf_src_tmr_owned(pf) && hw->mac_type == ICE_MAC_GENERIC_3K_E825)
>  		ice_ptp_prepare_rebuild_sec(pf, false, reset_type);
>

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