| 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: <59a1f246-9062-20e1-3f85-a1c5f6fcfc29@cogentembedded.com>
Date: Thu, 8 Dec 2016 15:28:48 +0300
From: Sergei Shtylyov <sergei.shtylyov@...entembedded.com>
To: Niklas Söderlund
<niklas.soderlund+renesas@...natech.se>,
Simon Horman <horms+renesas@...ge.net.au>,
netdev@...r.kernel.org, linux-renesas-soc@...r.kernel.org
Subject: Re: [PATCH] sh_eth: add wake-on-lan support via magic packet
Hello!
Good to see that somebody cares still about this driver, one more task off
my back. :-)
On 12/07/2016 07:28 PM, Niklas Söderlund wrote:
You only enable the WOL support fo the R-Car gen2 chips but never say that
explicitly, neither in the subject nor here.
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@...natech.se>
> ---
> drivers/net/ethernet/renesas/sh_eth.c | 120 +++++++++++++++++++++++++++++++---
> drivers/net/ethernet/renesas/sh_eth.h | 4 ++
> 2 files changed, 116 insertions(+), 8 deletions(-)
> diff --git a/drivers/net/ethernet/renesas/sh_eth.c b/drivers/net/ethernet/renesas/sh_eth.c
> index 05b0dc5..3974046 100644
> --- a/drivers/net/ethernet/renesas/sh_eth.c
> +++ b/drivers/net/ethernet/renesas/sh_eth.c
> @@ -624,7 +624,7 @@ static struct sh_eth_cpu_data r8a779x_data = {
>
> .register_type = SH_ETH_REG_FAST_RCAR,
>
> - .ecsr_value = ECSR_PSRTO | ECSR_LCHNG | ECSR_ICD,
> + .ecsr_value = ECSR_PSRTO | ECSR_LCHNG | ECSR_ICD | ECSR_MPD,
> .ecsipr_value = ECSIPR_PSRTOIP | ECSIPR_LCHNGIP | ECSIPR_ICDIP,
> .eesipr_value = 0x01ff009f,
>
> @@ -641,6 +641,7 @@ static struct sh_eth_cpu_data r8a779x_data = {
> .tpauser = 1,
> .hw_swap = 1,
> .rmiimode = 1,
> + .magic = 1,
> };
> #endif /* CONFIG_OF */
So I suggest that you add the general WOL support code in the 1st patch
and enable the new feature per SoC family in the follow up patches.
> @@ -1657,6 +1658,10 @@ static irqreturn_t sh_eth_interrupt(int irq, void *netdev)
> goto out;
>
> if (!likely(mdp->irq_enabled)) {
Oops, I guess unlikely(!mdp->irq_enabled) was meant here...
> + /* Handle MagicPacket interrupt */
> + if (sh_eth_read(ndev, ECSR) & ECSR_MPD)
Why do it here?
> + sh_eth_modify(ndev, ECSR, 0, ECSR_MPD);
> +
> sh_eth_write(ndev, 0, EESIPR);
> goto out;
> }
[...]
> @@ -3017,6 +3051,11 @@ static int sh_eth_drv_probe(struct platform_device *pdev)
> goto out_release;
> }
>
> + /* Get clock, if not found that's OK but Wake-On-Lan is unavailable */
> + mdp->clk = devm_clk_get(&pdev->dev, NULL);
Luckily, we have <linux/clk.h> #include'd...
> + if (IS_ERR(mdp->clk))
> + mdp->clk = NULL;
> +
> ndev->base_addr = res->start;
>
> spin_lock_init(&mdp->lock);
> @@ -3111,6 +3150,10 @@ static int sh_eth_drv_probe(struct platform_device *pdev)
> if (ret)
> goto out_napi_del;
>
> + mdp->wol_enabled = false;
No need, the '*mdp' was kzalloc'ed.
> + if (mdp->cd->magic && mdp->clk)
> + device_set_wakeup_capable(&pdev->dev, 1);
> +
> /* print device information */
> netdev_info(ndev, "Base address at 0x%x, %pM, IRQ %d.\n",
> (u32)ndev->base_addr, ndev->dev_addr, ndev->irq);
> @@ -3150,15 +3193,71 @@ static int sh_eth_drv_remove(struct platform_device *pdev)
>
> #ifdef CONFIG_PM
> #ifdef CONFIG_PM_SLEEP
> +static int sh_eth_wol_setup(struct net_device *ndev)
> +{
> + struct sh_eth_private *mdp = netdev_priv(ndev);
> +
> + /* Only allow ECI interrupts */
> + mdp->irq_enabled = false;
Why 'false' if you enable IRQs below?
> + synchronize_irq(ndev->irq);
> + napi_disable(&mdp->napi);
> + sh_eth_write(ndev, DMAC_M_ECI, EESIPR);
> +
> + /* Enable ECI MagicPacket interrupt */
> + sh_eth_write(ndev, ECSIPR_MPDIP, ECSIPR);
> +
> + /* Enable MagicPacket */
> + sh_eth_modify(ndev, ECMR, 0, ECMR_PMDE);
> +
> + /* Increased clock usage so device won't be suspended */
> + clk_enable(mdp->clk);
Hum, intermixiggn runtime PM with clock API doesn't look good...
> +
> + return enable_irq_wake(ndev->irq);
> +}
> +
> +static int sh_eth_wol_restore(struct net_device *ndev)
> +{
> + struct sh_eth_private *mdp = netdev_priv(ndev);
> + int ret;
> +
> + napi_enable(&mdp->napi);
> +
> + /* Disable MagicPacket */
> + sh_eth_modify(ndev, ECMR, ECMR_PMDE, 0);
> +
> + /* The device need to be reset to restore MagicPacket logic
> + * for next wakeup. If we close and open the device it will
> + * both be reset and all registers restored. This is what
> + * happens during suspend and resume without WoL enabled.
> + */
> + ret = sh_eth_close(ndev);
> + if (ret < 0)
> + return ret;
> + ret = sh_eth_open(ndev);
> + if (ret < 0)
> + return ret;
> +
> + /* Restore clock usage count */
> + clk_disable(mdp->clk);
Hm... and RPM will think the clock is still enabled?
Why disable the clock here anyway if we're resuming?
> +
> + return disable_irq_wake(ndev->irq);
> +}
> +
[...]
> @@ -3166,14 +3265,19 @@ static int sh_eth_suspend(struct device *dev)
> static int sh_eth_resume(struct device *dev)
> {
> struct net_device *ndev = dev_get_drvdata(dev);
> + struct sh_eth_private *mdp = netdev_priv(ndev);
> int ret = 0;
>
> - if (netif_running(ndev)) {
> + if (!netif_running(ndev))
> + return 0;
> +
> + if (mdp->wol_enabled)
> + ret = sh_eth_wol_restore(ndev);
> + else
> ret = sh_eth_open(ndev);
> - if (ret < 0)
> - return ret;
> +
> + if (!ret)
Please keep the original error return logic, so that you can return 0 at
the end...
> netif_device_attach(ndev);
> - }
>
> return ret;
> }
> diff --git a/drivers/net/ethernet/renesas/sh_eth.h b/drivers/net/ethernet/renesas/sh_eth.h
> index d050f37..26c6620 100644
> --- a/drivers/net/ethernet/renesas/sh_eth.h
> +++ b/drivers/net/ethernet/renesas/sh_eth.h
> @@ -493,6 +493,7 @@ struct sh_eth_cpu_data {
> unsigned shift_rd0:1; /* shift Rx descriptor word 0 right by 16 */
> unsigned rmiimode:1; /* EtherC has RMIIMODE register */
> unsigned rtrate:1; /* EtherC has RTRATE register */
> + unsigned magic:1; /* EtherC have PMDE in ECMR and MPDIP in ECSIPR */
OK, e.g. RZ/A1 doesn't have these bits...
[...]
MBR, Sergei
Powered by blists - more mailing lists