RE: [PATCH net v3 1/2] net: fec: stop the PTP clock after the netdev is unregistered

quoted

+	unregister_netdev(ndev);
 	if (fep->bufdesc_ex)
 		fec_ptp_stop(pdev);

[Severity: Medium]
With this ordering, does fec_ptp_stop() now touch the 1588 registers while
clk_ptp is gated?

unregister_netdev() -> dev_close() -> fec_enet_close() runs first, and for
bufdesc_ex it goes through fec_stop():

fec_main.c:fec_stop() {
	...
		fec_ptp_start_cyclecounter(ndev);
		fec_ptp_restore_state(fep);
}

fec_ptp_restore_state() re-arms PPS via fec_ptp_enable_pps(fep, 1) when PPS
was enabled, so fep->pps_enable is 1 on return.  fec_enet_close() then
does:

fec_main.c:fec_enet_close() {
	...
	fec_enet_clk_enable(ndev, false);
}

which does clk_disable_unprepare(fep->clk_ptp) and clears fep->ptp_clk_on.

Control then returns to fec_drv_remove() and fec_ptp_stop() runs:

fec_ptp.c:fec_ptp_stop() {
	if (fep->pps_enable)
		fec_ptp_enable_pps(fep, 0);

	cancel_delayed_work_sync(&fep->time_keep);
	hrtimer_cancel(&fep->perout_timer);
	...
}

fec_ptp_enable_pps() reaches the disable branch with no ptp_clk_on or
ptp_clk_mutex guard:

fec_ptp.c:fec_ptp_enable_pps() {
	...
	} else {
		writel(0, fep->hwp + FEC_TCSR(fep->pps_channel));
	}
}

Every other 1588 register path in the driver (fec_ptp_gettime, fec_ptp_settime,
the PEROUT branch of fec_ptp_enable, fec_time_keep) bails out
when !fep->ptp_clk_on.  If this write is not latched with the clock gated, does
the output-compare channel and its TIE interrupt enable stay armed while
software records pps_enable = 0?

I think this is a false positive, the register cannot be accessed when the ipg
clock is disabled rather than the ptp clock. And the ipg clock is enabled at
the beginning of fec_drv_remove(), it calls pm_runtime_get_sync() -->
fec_runtime_resume() --> clk_prepare_enable(fep->clk_ipg).

See commit 91c0d987a978 ("net: fec: ptp: avoid register access when ipg clock is disabled")
for more details.