netdev - Re: [PATCH net-next 1/1] net: phy: marvell10g: Add WAKE

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]

Date:   Sat, 14 Aug 2021 18:26:56 +0100
From:   "Russell King (Oracle)" <linux@...linux.org.uk>
To:     Song Yoong Siang <yoong.siang.song@...el.com>
Cc:     Marek Behún <kabel@...nel.org>,
        Andrew Lunn <andrew@...n.ch>,
        Heiner Kallweit <hkallweit1@...il.com>,
        "David S . Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next 1/1] net: phy: marvell10g: Add WAKE_PHY support
 to WOL event

On Fri, Aug 13, 2021 at 04:45:36PM +0800, Song Yoong Siang wrote:
> Add Wake-on-PHY feature support by enabling the Link Status Changed
> interrupt.
> 
> Signed-off-by: Song Yoong Siang <yoong.siang.song@...el.com>
> ---
>  drivers/net/phy/marvell10g.c | 33 ++++++++++++++++++++++++++++++++-
>  1 file changed, 32 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/phy/marvell10g.c b/drivers/net/phy/marvell10g.c
> index 0b7cae118ad7..d46761c225f0 100644
> --- a/drivers/net/phy/marvell10g.c
> +++ b/drivers/net/phy/marvell10g.c
> @@ -76,6 +76,11 @@ enum {
>  	MV_PCS_CSSR1_SPD2_2500	= 0x0004,
>  	MV_PCS_CSSR1_SPD2_10000	= 0x0000,
>  
> +	/* Copper Specific Interrupt registers */
> +	MV_PCS_INTR_ENABLE	= 0x8010,
> +	MV_PCS_INTR_ENABLE_LSC	= BIT(10),
> +	MV_PCS_INTR_STS		= 0x8011,
> +
>  	/* Temperature read register (88E2110 only) */
>  	MV_PCS_TEMP		= 0x8042,
>  
> @@ -1036,7 +1041,7 @@ static void mv3110_get_wol(struct phy_device *phydev,
>  {
>  	int ret;
>  
> -	wol->supported = WAKE_MAGIC;
> +	wol->supported = WAKE_MAGIC | WAKE_PHY;
>  	wol->wolopts = 0;
>  
>  	ret = phy_read_mmd(phydev, MDIO_MMD_VEND2, MV_V2_WOL_CTRL);
> @@ -1045,6 +1050,13 @@ static void mv3110_get_wol(struct phy_device *phydev,
>  
>  	if (ret & MV_V2_WOL_CTRL_MAGIC_PKT_EN)
>  		wol->wolopts |= WAKE_MAGIC;
> +
> +	ret = phy_read_mmd(phydev, MDIO_MMD_PCS, MV_PCS_INTR_ENABLE);
> +	if (ret < 0)
> +		return;
> +
> +	if (ret & MV_PCS_INTR_ENABLE_LSC)
> +		wol->wolopts |= WAKE_PHY;
>  }
>  
>  static int mv3110_set_wol(struct phy_device *phydev,
> @@ -1099,6 +1111,25 @@ static int mv3110_set_wol(struct phy_device *phydev,
>  			return ret;
>  	}
>  
> +	if (wol->wolopts & WAKE_PHY) {
> +		/* Enable the link status changed interrupt */
> +		ret = phy_set_bits_mmd(phydev, MDIO_MMD_PCS,
> +				       MV_PCS_INTR_ENABLE,
> +				       MV_PCS_INTR_ENABLE_LSC);
> +		if (ret < 0)
> +			return ret;
> +
> +		/* Clear the interrupt status register */
> +		ret = phy_read_mmd(phydev, MDIO_MMD_PCS, MV_PCS_INTR_STS);
> +	} else {
> +		/* Disable the link status changed interrupt */
> +		ret = phy_clear_bits_mmd(phydev, MDIO_MMD_PCS,
> +					 MV_PCS_INTR_ENABLE,
> +					 MV_PCS_INTR_ENABLE_LSC);
> +		if (ret < 0)
> +			return ret;
> +	}
> +

How does this work if the driver has no interrupt support? What is
the hardware setup this has been tested with?

What if we later want to add interrupt support to this driver to
support detecting changes in link state - isn't using this bit
in the interrupt enable register going to confict with that?

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!