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:
 <BL3PR12MB645041618E60758B5DBAD149C302A@BL3PR12MB6450.namprd12.prod.outlook.com>
Date: Mon, 24 Jul 2023 11:30:09 +0000
From: Revanth Kumar Uppala <ruppala@...dia.com>
To: Andrew Lunn <andrew@...n.ch>
CC: "linux@...linux.org.uk" <linux@...linux.org.uk>, "hkallweit1@...il.com"
	<hkallweit1@...il.com>, "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"linux-tegra@...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)



> -----Original Message-----
> From: Andrew Lunn <andrew@...n.ch>
> Sent: Wednesday, June 28, 2023 7:48 PM
> 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)
> 
> External email: Use caution opening links or attachments
> 
> 
> > +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.
As per my reply to Russel's comment, we are following the app note AN-N4209 by Marvell semiconductors for enabling and disabling of WOL and above logic is part of the same app note.
> 
> > @@ -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.
Yes, your assumption is correct.
Will move this logic to aqr113c_wol_enable() function to take care of above scenario.
Thanks,
Revanth Uppala

> 
> > +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