netdev - Re: [PATCH] net: stmicro: implement basic Wake-On-LAN support

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] [day] [month] [year] [list]

Message-ID: <YozVIEhUYpWk6atX@lunn.ch>
Date:   Tue, 24 May 2022 14:52:48 +0200
From:   Andrew Lunn <andrew@...n.ch>
To:     Qiang Yang <line_walker2016@....com>
Cc:     peppe.cavallaro@...com, alexandre.torgue@...com,
        joabreu@...opsys.com, davem@...emloft.net, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org, Weiqiang Su <David.suwq@...look.com>
Subject: Re: [PATCH] net: stmicro: implement basic Wake-On-LAN support

> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac1000_core.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac1000_core.c
> @@ -267,7 +267,14 @@ static void dwmac1000_flow_ctrl(struct mac_device_info *hw, unsigned int duplex,
>  static void dwmac1000_pmt(struct mac_device_info *hw, unsigned long mode)
>  {
>  	void __iomem *ioaddr = hw->pcsr;
> -	unsigned int pmt = 0;
> +	unsigned int pmt = 0, i = 0;
> +
> +	writel(pointer_reset, ioaddr + GMAC_PMT);
> +	mdelay(100);

That is quite a long delay. Is there a bit which can be polled to let
you know it is ready?

> +
> +	for (i = 0; i < WAKEUP_REG_LENGTH; i++)
> +		writel(*(stmmac_wakeup_filter_config + i),
> +		       ioaddr + GMAC_WAKEUP_FILTER);
>  
>  	if (mode & WAKE_MAGIC) {
>  		pr_debug("GMAC: WOL Magic frame\n");
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index 0a4d093adfc9..7866f3ec5ef6 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -4513,6 +4513,7 @@ int stmmac_suspend(struct device *dev)
>  
>  	/* Enable Power down mode by programming the PMT regs */
>  	if (device_may_wakeup(priv->device)) {
> +		priv->wolopts |= WAKE_MAGIC;
>  		stmmac_pmt(priv, priv->hw, priv->wolopts);
>  		priv->irq_wake = 1;
>  	} else {
> @@ -4598,6 +4599,7 @@ int stmmac_resume(struct device *dev)
>  			stmmac_mdio_reset(priv->mii);
>  	}
>  
> +	device_set_wakeup_enable(dev, 0);
>  	netif_device_attach(ndev);
>  
>  	mutex_lock(&priv->lock);
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c
> index cc1e887e47b5..ec69521f061c 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c
> @@ -322,6 +322,7 @@ static int __maybe_unused stmmac_pci_suspend(struct device *dev)
>  	struct pci_dev *pdev = to_pci_dev(dev);
>  	int ret;
>  
> +	device_set_wakeup_enable(dev, 1);
>  	ret = stmmac_suspend(dev);
>  	if (ret)
>  		return ret;

This looks too minimum. I would expect there to be code in set_wol and
get_wol, to indicate WAKE_MAGIC is available, and to turn it
on/off. You also need to deal with when the PHY also implements
WAKE_MAGIC. Ideally, the PHY should implement it, since it can do it
with less power. But when the PHY does not have support, then the MAC
should implement it.

Maybe some of this code already exists, i've not looked.

       Andrew