lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Message-ID: <299d74d5-2d56-23f6-affc-78bb3ae3e03c@prolan.hu> Date: Wed, 10 Aug 2022 13:36:54 +0200 From: Csókás Bence <csokas.bence@...lan.hu> To: Andrew Lunn <andrew@...n.ch> CC: <netdev@...r.kernel.org>, Richard Cochran <richardcochran@...il.com>, Fugang Duan <fugang.duan@....com> Subject: Re: [PATCH] fec: Restart PPS after link state change On 2022. 08. 09. 19:28, Andrew Lunn wrote: > On Tue, Aug 09, 2022 at 02:41:19PM +0200, Csókás Bence wrote: >> On link state change, the controller gets reset, >> causing PPS to drop out. So we restart it if needed. >> >> Signed-off-by: Csókás Bence <csokas.bence@...lan.hu> >> --- >> drivers/net/ethernet/freescale/fec_main.c | 27 ++++++++++++++++++++++- >> 1 file changed, 26 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c >> index ca5d49361fdf..c264b1dd5286 100644 >> --- a/drivers/net/ethernet/freescale/fec_main.c >> +++ b/drivers/net/ethernet/freescale/fec_main.c >> @@ -954,6 +954,7 @@ fec_restart(struct net_device *ndev) >> u32 temp_mac[2]; >> u32 rcntl = OPT_FRAME_SIZE | 0x04; >> u32 ecntl = 0x2; /* ETHEREN */ >> + struct ptp_clock_request ptp_rq = { .type = PTP_CLK_REQ_PPS }; > > Is it safe to hard code this? What if the user configured > PTP_CLK_REQ_EXTTS or PTP_CLK_REQ_PEROUT? The fec driver doesn't support anything other than PTP_CLK_REQ_PPS. And if it will at some point, this will need to be amended anyways. > >> /* Whack a reset. We should wait for this. >> * For i.MX6SX SOC, enet use AXI bus, we use disable MAC >> @@ -1119,6 +1120,13 @@ fec_restart(struct net_device *ndev) >> if (fep->bufdesc_ex) >> fec_ptp_start_cyclecounter(ndev); >> >> + /* Restart PPS if needed */ >> + if (fep->pps_enable) { >> + /* Clear flag so fec_ptp_enable_pps() doesn't return immediately */ >> + fep->pps_enable = 0; > > If reset causes PPS to stop, maybe it would be better to do this > unconditionally? But if it wasn't enabled before the reset in the first place, we wouldn't want to unexpectedly start it. > > fep->pps_enable = 0; > fep->ptp_caps.enable(&fep->ptp_caps, &ptp_rq, 1); > >> + if (fep->bufdesc_ex) >> + ecntl |= (1 << 4); > > Please replace (1 << 4) with a #define to make it clear what this is doing. I took it from the original source, line 1138 as of commit #504148f. It is the EN1588 bit by the way. I shall replace it with a #define in both places then. Though the code is riddled with other magic numbers without explanation, and I probably won't be bothered to fix them all. > >> + >> /* We have to keep ENET enabled to have MII interrupt stay working */ >> if (fep->quirks & FEC_QUIRK_ENET_MAC && >> !(fep->wol_flag & FEC_WOL_FLAG_SLEEP_ON)) { >> - writel(2, fep->hwp + FEC_ECNTRL); >> + ecntl |= 0x2; >> writel(rmii_mode, fep->hwp + FEC_R_CNTRL); >> } >> + >> + writel(ecntl, fep->hwp + FEC_ECNTRL); >> + >> + if (fep->bufdesc_ex) >> + fec_ptp_start_cyclecounter(ndev); >> + >> + /* Restart PPS if needed */ >> + if (fep->pps_enable) { >> + /* Clear flag so fec_ptp_enable_pps() doesn't return immediately */ >> + fep->pps_enable = 0; >> + fep->ptp_caps.enable(&fep->ptp_caps, &ptp_rq, 1); >> + } > > So you re-start PPS in stop()? Should it keep outputting when the > interface is down? Yes. We use PPS to synchronize devices on a common backplane. We use PTP to sync this PPS to a master clock. But if PTP sync drops out, we wouldn't want the backplane-level synchronization to fail. The PPS needs to stay on as long as userspace *explicitly* disables it, regardless of what happens to the link. > > Also, if it is always outputting, don't you need to stop it in > fec_drv_remove(). You probably don't want to still going after the > driver is unloaded. Good point, that is one exception we could make to the above statement (though even in this case, userspace *really* should disable PPS before unloading the module). > > Andrew Thanks for the insights, Bence
Powered by blists - more mailing lists