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

[Wei Fang]

> > +	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.