netdev - Re: [PATCH] fec: Restart PPS after link state change

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <YvKZNcVfYdLw7bkm@lunn.ch>
Date:   Tue, 9 Aug 2022 19:28:21 +0200
From:   Andrew Lunn <andrew@...n.ch>
To:     Csókás Bence <csokas.bence@...lan.hu>
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 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?

>  	/* 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?

	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.

> +
>  	/* 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?

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.

       Andrew