netdev - Re: [PATCH 4/4] net: phy: aqr113c: Enable Wake-on-LAN (WOL)

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: <c1aedb1e-e750-40ce-a19a-dfb21e2a971f@lunn.ch>
Date:   Wed, 28 Jun 2023 16:17:47 +0200
From:   Andrew Lunn <andrew@...n.ch>
To:     Revanth Kumar Uppala <ruppala@...dia.com>
Cc:     linux@...linux.org.uk, hkallweit1@...il.com,
        netdev@...r.kernel.org, linux-tegra@...r.kernel.org,
        Narayan Reddy <narayanr@...dia.com>
Subject: Re: [PATCH 4/4] net: phy: aqr113c: Enable Wake-on-LAN (WOL)

> +static int aqr113c_wol_enable(struct phy_device *phydev)
> +{
> +	struct aqr107_priv *priv = phydev->priv;
> +	u16 val;
> +	int ret;
> +
> +	/* Disables all advertised speeds except for the WoL
> +	 * speed (100BASE-TX FD or 1000BASE-T)
> +	 * This is set as per the APP note from Marvel
> +	 */
> +	ret = phy_set_bits_mmd(phydev, MDIO_MMD_AN, MDIO_AN_10GBT_CTRL,
> +			       MDIO_AN_LD_LOOP_TIMING_ABILITY);
> +	if (ret < 0)
> +		return ret;

Please take a look at phylink_speed_down() and
phylink_speed_up(). Assuming the PHY is not reporting it can do 10Full
and 10Half, it should end up in 100BaseFull. Assuming the link partner
can do 100BaseFull....

Russell points out you are making a lot of assumptions about the
system side link. Ideally, you want to leave that to the PHY. Once the
auto-neg at the lower speed has completed, it might change the system
side link, e.g. to SGMII and the normal machinery should pass that
onto the MAC, so it can follow. I would not force anything.

> @@ -619,6 +784,31 @@ static int aqr107_config_init(struct phy_device *phydev)
>  	if (ret < 0)
>  		return ret;
>  
> +	/* Configure Magic packet frame pattern (MAC address) */
> +	ret = phy_write_mmd(phydev, MDIO_MMD_C22EXT, MDIO_C22EXT_MAGIC_PKT_PATTERN_0_2_15,
> +			    phydev->attached_dev->dev_addr[0] |
> +			    (phydev->attached_dev->dev_addr[1] << 8));

I think most PHY drivers do this as part of enabling WOL. Doing it in
aqr107_config_init() is early, is the MAC address stable yet? The user
could change it. It could still be changed after wol is enabled, but
at least the user has a clear point in time when WoL configuration
happens.

> +static void aqr113c_get_wol(struct phy_device *phydev, struct ethtool_wolinfo *wol)
> +{
> +	int val;
> +
> +	val = phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_AN_RSVD_VEND_STATUS3);
> +	if (val < 0)
> +		return;
> +
> +	wol->supported = WAKE_MAGIC;
> +	if (val & 0x1)
> +		wol->wolopts = WAKE_MAGIC;

WoL seems to be tried to interrupts. So maybe you should actually
check an interrupt is available? This is not going to work if the PHY
is being polled. It does however get a bit messy, some boards might
connect the 'interrupt' pin to PMIC. So there is not a true interrupt,
but the PMIC can turn the power back on.

    Andrew