Re: [PATCH net 06/15] ice: call PTP link change only from link events

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
ice: call PTP link change only from link events

The patch drops the ice_ptp_link_change() calls from ice_up_complete() and
ice_down() so that only the admin queue link event handler drives PTP link
state, seeds ptp_port->link_up during ice_ptp_init(), and re-checks the link
state at the end of ice_rebuild(). The PTP kworker creation is also moved
earlier in ice_ptp_init(), with the periodic work queued only once the state
is ICE_PTP_READY.

[ ... ]

[ ... ]

[Severity: High]
Does this re-create, in the reset path, the same double PHY
re-initialization that the changelog describes as problem 1?

With the ice_ptp_link_change(vsi->back, false) call gone from ice_down(),
nothing clears ptp_port->link_up during reset preparation.  The only
remaining writers of that field are the store in ice_ptp_link_change() and
the new seed in ice_ptp_init().

ice_prepare_for_reset() calls ice_pf_dis_all_vsi() -> ice_down() before
ice_ptp_prepare_for_reset(), so previously link_up was cleared while the
state was still ICE_PTP_READY and the local port was skipped here:

ice_ptp_restart_all_phy()
	if (port->link_up)
		ice_ptp_port_phy_restart(port);

Now link_up survives the reset, so ice_rebuild() -> ice_ptp_rebuild() ->
ice_ptp_rebuild_owner() -> ice_ptp_restart_all_phy() does a real restart of
the local port, and then the block added above calls
ice_ptp_link_change(pf, true), which re-enters ice_ptp_port_phy_restart() a
second time within the same ice_rebuild().

For ICE_MAC_GENERIC that second entry does:

ice_ptp_port_phy_restart()
	kthread_cancel_delayed_work_sync(&ptp_port->ov_work);
	...
	ptp_port->tx.calibrating = true;
	...
	err = ice_start_phy_timer_e82x(hw, port);

which cancels the offset verification work queued milliseconds earlier by
the first restart, re-asserts calibrating and restarts the timer.  On
ICE_MAC_GENERIC_3K_E825 it re-runs ice_start_phy_timer_eth56g(), i.e. a PHY
soft reset, timer stop, incval reprogram, PHC resync and clearing/setting of
the offset-ready bits.

Neither guard in ice_ptp_link_change() short-circuits this call:
ice_reset_subtask() clears pf->hw.reset_ongoing right before ice_rebuild(),
and ice_ptp_rebuild() has already set the state to ICE_PTP_READY earlier in
ice_rebuild().

One more question on the same block: ice_ptp_link_change() discards the
return value of ice_ptp_port_phy_restart(), and the E825 restart begins by
stopping the timer.  If the redundant second attempt fails, can it leave a
previously working PHY timer stopped, at a point where
ice_update_pf_netdev_link() has already turned the carrier on and woken the
Tx queues?

Would it be cleaner to have exactly one driver of the post-reset PHY
restart, either ice_ptp_restart_all_phy() or this new block, rather than
both?

[Severity: High]
Can moving the state check below the store make a half-initialized port
visible to ice_ptp_restart_all_phy()?

ice_ptp_setup_pf() publishes the port before the rest of PTP init runs:

	kref_init(&ptp->port.ref);

	spin_lock(&pf->adapter->ports.lock);
	list_add_rcu(&ptp->port.list_node, &pf->adapter->ports.list);
	spin_unlock(&pf->adapter->ports.lock);

After that, and while pf->ptp.state is still ICE_PTP_INITIALIZING, a link
event reaching ice_ptp_link_change() now sets ptp_port->link_up = true
before returning.  At that point ice_ptp_init_port() may not yet have run
mutex_init(&ptp_port->ps_lock) or spin_lock_init(&tx->lock), and
pf->ptp.kworker is still NULL.

ice_ptp_restart_all_phy() gates only on the kref and the cached flag:

		if (!kref_get_unless_zero(&port->ref))
			continue;
		rcu_read_unlock();

		if (port->link_up)
			ice_ptp_port_phy_restart(port);

and it is reachable from ice_ptp_settime64(), which does no ptp->state check
and ends with:

	/* Recalibrate and re-enable timestamp blocks for E822/E823 */
	if (hw->mac_type == ICE_MAC_GENERIC)
		ice_ptp_restart_all_phy(pf);

The PHC character device already exists at that point, because
ice_ptp_init_owner() -> ice_ptp_create_clock() runs earlier in
ice_ptp_init(), so a clock_settime() on the owner PF, or a reset of the
owner PF concurrent with the probe of a peer PF, can reach it.

On ICE_MAC_GENERIC the restart then does:

		kthread_queue_delayed_work(pf->ptp.kworker, &ptp_port->ov_work,
					   0);

with pf->ptp.kworker == NULL, and kthread_queue_delayed_work() starts with
raw_spin_lock_irqsave(&worker->lock, flags).

Before this patch link_up stayed false for the whole of ice_ptp_init()
because ice_ptp_link_change() returned before the store while the state was
not READY, so the port->link_up guard skipped the initializing port.  Should
link_up only be published after the kworker exists and after the port locks
are initialized?

 	/* Skip HW writes if reset is in progress */
 	if (pf->hw.reset_ongoing)
 		goto exit_kref_put;

[ ... ]

[Severity: Medium]
Is ptp_port->link_up now written from two contexts without any
synchronization?

This seed runs on the probe thread, while ice_ptp_link_change() writes the
same plain bool from the service task via the admin queue link event
handler, and after this patch that store happens before the ICE_PTP_READY
bail-out.  ice_init() clears ICE_SERVICE_DIS and arms pf->serv_tmr before
ice_init_features() calls ice_ptp_init(), so both writers can run
concurrently.

There is no lock, and no WRITE_ONCE/READ_ONCE pairing, between the probe
thread's read of phy.link_info and its store to link_up, so a link
transition processed inside that window is silently dropped.

Link events are edge triggered:

ice_link_event()
	if (link_up == old_link && link_speed == old_link_speed)
		return 0;

so if the surviving value is false while the link is physically up,
ice_ptp_reset_phy_timestamping() -> ice_ptp_port_phy_restart() takes:

	if (!ptp_port->link_up)
		return ice_ptp_port_phy_stop(ptp_port);

and nothing later corrects it.  On E82x the vernier timer would then never
be started and timestamping would stay off until the link physically
toggles.  Note also that ice_ptp_port_phy_restart() reads link_up before
taking ptp_port->ps_lock, so ps_lock does not cover this field.

+
+	/* Create the kworker before restarting the PHY, which queues work on
+	 * it in the E82x restart path. This prevents concurrent link events
+	 * from reaching ice_ptp_port_phy_restart() while kworker is still NULL
+	 */
+	err = ice_ptp_init_work(pf, ptp);
+	if (err)
+		goto err_clean_pf;
+

[Severity: Medium]
This isn't a bug introduced by this patch, but since the unwind path is
being reshuffled here, should err_clean_pf also release the Tx timestamp
tracker?

By the time this new goto err_clean_pf can be taken, ice_ptp_init_port() ->
ice_ptp_init_tx()/ice_ptp_init_tx_e82x() -> ice_ptp_alloc_tx_tracker() has
already allocated:

	tstamps = kzalloc_objs(*tstamps, tx->len);
	in_use = bitmap_zalloc(tx->len, GFP_KERNEL);
	stale = bitmap_zalloc(tx->len, GFP_KERNEL);

but the unwind only does:

err_clean_pf:
	mutex_destroy(&ptp->port.ps_lock);
	ice_ptp_cleanup_pf(pf);

ice_ptp_cleanup_pf() only unlinks the port from the adapter list and drops
the kref, and err_exit then sets ptp->state = ICE_PTP_UNINIT, which makes
ice_ptp_release() return at its first check, so
ice_ptp_release_tx_tracker() never runs.  The same leak existed on the old
err_exit path when ice_ptp_init_work() failed, so this predates the patch;
the trigger is a kthread_run_worker() failure in ice_ptp_init_work().