netdev - Re: [PATCH v2] ravb: add wake-on-lan support via magic packet

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: <111b5ae4-5517-7a07-3077-96ced89db407@cogentembedded.com>
Date:   Mon, 31 Jul 2017 22:47:12 +0300
From:   Sergei Shtylyov <sergei.shtylyov@...entembedded.com>
To:     Niklas Söderlund 
        <niklas.soderlund+renesas@...natech.se>
Cc:     Geert Uytterhoeven <geert@...ux-m68k.org>, netdev@...r.kernel.org,
        linux-renesas-soc@...r.kernel.org
Subject: Re: [PATCH v2] ravb: add wake-on-lan support via magic packet

Hello!

On 07/30/2017 05:06 PM, Niklas Söderlund wrote:

> WoL is enabled in the suspend callback by setting MagicPacket detection
> and disabling all interrupts expect MagicPacket. In the resume path the
> driver needs to reset the hardware to rearm the WoL logic, this prevents
> the driver from simply restoring the registers and to take advantage of
> that ravb was not suspended to reduce resume time. To reset the
> hardware the driver closes the device, sets it in reset mode and reopens
> the device just like it would do in a normal suspend/resume scenario
> without WoL enabled, but it both closes and opens the device in the
> resume callback since the device needs to be reset for WoL to work.

> One quirk needed for WoL is that the module clock needs to be prevented
> from being switched off by Runtime PM. To keep the clock alive the
> suspend callback need to call clk_enable() directly to increase the
> usage count of the clock. Then when Runtime PM decreases the clock usage
> count it won't reach 0 and be switched off.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@...natech.se>
[...]
> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> index 5931e859876c2aee..3d399f85417a83cf 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
[...]
> @@ -2179,6 +2270,32 @@ static int __maybe_unused ravb_resume(struct device *dev)
>   	struct ravb_private *priv = netdev_priv(ndev);
>   	int ret = 0;
>   
> +	/* Reduce the usecount of the clock to zero and then
> +	 * restore it to its original value. This is done to force
> +	 * the clock to be re-enabled which is a workaround
> +	 * for renesas-cpg-mssr driver which do not enable clocks
> +	 * when resuming from PSCI suspend/resume.
> +	 *
> +	 * Without this workaround the driver fails to communicate
> +	 * with the hardware if WoL was enabled when the system
> +	 * entered PSCI suspend. This is due to that if WoL is enabled
> +	 * we explicitly keep the clock from being turned off when
> +	 * suspending, but in PSCI sleep power is cut so the clock
> +	 * is disabled anyhow, the clock driver is not aware of this
> +	 * so the clock is not turned back on when resuming.
> +	 *
> +	 * TODO: once the renesas-cpg-mssr suspend/resume is working
> +	 *       this clock dance should be removed.
> +	 */
> +	clk_disable(priv->clk);
> +	clk_disable(priv->clk);
> +	clk_enable(priv->clk);
> +	clk_enable(priv->clk);

    After a small chat with Niklas, it became clear that this dance should be 
behind the *if* that follows. I'd also like to see this workaround as a 
separate patch since the isse it addresses is R-Car gen3 specific (and thus 
can be reverted once the CPG/MMSR driver is fixed)...

> +
> +	/* If WoL is enabled set reset mode to rearm the WoL logic */
> +	if (priv->wol_enabled)
> +		ravb_write(ndev, CCC_OPC_RESET, CCC);
> +
>   	/* All register have been reset to default values.
>   	 * Restore all registers which where setup at probe time and
>   	 * reopen device if it was running before system suspended.
> @@ -2202,6 +2319,11 @@ static int __maybe_unused ravb_resume(struct device *dev)
>  	ravb_write(ndev, priv->desc_bat_dma, DBAT);
>  
>  	if (netif_running(ndev)) {
> +		if (priv->wol_enabled) {
> +			ret = ravb_wol_restore(ndev);
> +			if (ret)
> +				return ret;
> +		}
>  		ret = ravb_open(ndev);

    Hm, perhaps worth calling sh_eth_open() outside sh_eth_wol_restore() as well?

MBR, Sergei