| 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: <DB8PR04MB679569CA5E4A317CD40CF6CCE6CE9@DB8PR04MB6795.eurprd04.prod.outlook.com>
Date: Thu, 2 Sep 2021 07:01:32 +0000
From: Joakim Zhang <qiangqing.zhang@....com>
To: Russell King <linux@...linux.org.uk>
CC: "peppe.cavallaro@...com" <peppe.cavallaro@...com>,
"alexandre.torgue@...s.st.com" <alexandre.torgue@...s.st.com>,
"joabreu@...opsys.com" <joabreu@...opsys.com>,
"davem@...emloft.net" <davem@...emloft.net>,
"kuba@...nel.org" <kuba@...nel.org>,
"mcoquelin.stm32@...il.com" <mcoquelin.stm32@...il.com>,
"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
"andrew@...n.ch" <andrew@...n.ch>,
"f.fainelli@...il.com" <f.fainelli@...il.com>,
"hkallweit1@...il.com" <hkallweit1@...il.com>,
dl-linux-imx <linux-imx@....com>
Subject: RE: [PATCH] net: stmmac: fix MAC not working when system resume back
with WoL enabled
Hi Russell,
> -----Original Message-----
> From: Russell King <linux@...linux.org.uk>
> Sent: 2021年9月1日 20:57
> To: Joakim Zhang <qiangqing.zhang@....com>
> Cc: peppe.cavallaro@...com; alexandre.torgue@...s.st.com;
> joabreu@...opsys.com; davem@...emloft.net; kuba@...nel.org;
> mcoquelin.stm32@...il.com; netdev@...r.kernel.org; andrew@...n.ch;
> f.fainelli@...il.com; hkallweit1@...il.com; dl-linux-imx
> <linux-imx@....com>
> Subject: Re: [PATCH] net: stmmac: fix MAC not working when system resume
> back with WoL enabled
>
> On Wed, Sep 01, 2021 at 10:21:59AM +0000, Joakim Zhang wrote:
> >
> > Hi Russell,
> >
> > > -----Original Message-----
> > > From: Russell King <linux@...linux.org.uk>
> > > Sent: 2021年9月1日 17:14
> > > To: Joakim Zhang <qiangqing.zhang@....com>
> > > Cc: peppe.cavallaro@...com; alexandre.torgue@...s.st.com;
> > > joabreu@...opsys.com; davem@...emloft.net; kuba@...nel.org;
> > > mcoquelin.stm32@...il.com; netdev@...r.kernel.org; andrew@...n.ch;
> > > f.fainelli@...il.com; hkallweit1@...il.com; dl-linux-imx
> > > <linux-imx@....com>
> > > Subject: Re: [PATCH] net: stmmac: fix MAC not working when system
> > > resume back with WoL enabled
> > >
> > > NAK. Please read the phylink documentation. speed/duplex/pause is
> > > undefined in .mac_config.
> >
> > Speed/duplex/pause also the field of " struct phylink_link_state", so
> > these can be refered in .mac_config, please see the link which stmmac did
> before:
> > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Felix
> >
> ir.bootlin.com%2Flinux%2Fv5.4.143%2Fsource%2Fdrivers%2Fnet%2Fethernet%
> >
> 2Fstmicro%2Fstmmac%2Fstmmac_main.c%23L852&data=04%7C01%7Cq
> iangqing
> > .zhang%40nxp.com%7C431c0f2b1b904ad6f1ec08d96d47f399%7C686ea1d3b
> c2b4c6f
> >
> a92cd99c5c301635%7C0%7C0%7C637660978077417864%7CUnknown%7CTW
> FpbGZsb3d8
> >
> eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D
> %7C1
> >
> 000&sdata=GOmisORcQtC4MvLbOD6s7wlfdMahhoVv6z79xsjeTrI%3D&a
> mp;reser
> > ved=0
>
> The phylink documentation says:
>
> /**
> * mac_config() - configure the MAC for the selected mode and state
> * @config: a pointer to a &struct phylink_config.
> * @mode: one of %MLO_AN_FIXED, %MLO_AN_PHY, %MLO_AN_INBAND.
> * @state: a pointer to a &struct phylink_link_state.
> *
> * Note - not all members of @state are valid. In particular,
> * @state->lp_advertising, @state->link, @state->an_complete are never
> * guaranteed to be correct, and so any mac_config() implementation must
> * never reference these fields.
> ...
> * %MLO_AN_FIXED, %MLO_AN_PHY:
> ...
> * Older drivers (prior to the mac_link_up() change) may use
> @state->speed,
> * @state->duplex and @state->pause to configure the MAC, but this is
> * deprecated; such drivers should be converted to use mac_link_up().
> * Valid state members: interface, advertising.
> * Deprecated state members: speed, duplex, pause.
> ...
> * %MLO_AN_INBAND:
> ...
> * Valid state members: interface, an_enabled, pause, advertising.
>
> The reason for this is there have _always_ been code paths through phylink
> where particularly speed and duplex are _not_ _set_ according to the current
> link settings. For example, a call to ksettings_set.
> This is why I revised the interface so that mac_link_up() receives the link
> settings and depreciated these members in mac_config().
>
> In any case, as can be seen from the documentation, speed and duplex have
> _never_ been valid when operating in inband mode in mac_config.
Ok, thanks for your detailed explanation.
> > > I think the problem here is that you're not calling phylink_stop()
> > > when WoL is enabled, which means phylink will continue to maintain
> > > the state as per the hardware state, and phylib will continue to run
> > > its state machine reporting the link state to phylink.
> >
> > Yes, I also tried do below code change, but the host would not be
> > wakeup, phylink_stop() would call phy_stop(), phylib would call
> > phy_suspend() finally, it will not suspend phy if it detect WoL enabled, so now
> I don't know why system can't be wakeup with this code change.
> >
> > @@ -5374,7 +5374,6 @@ int stmmac_suspend(struct device *dev)
> > rtnl_lock();
> > if (device_may_wakeup(priv->device))
> > phylink_speed_down(priv->phylink, false);
> > - phylink_stop(priv->phylink);
> > rtnl_unlock();
> > mutex_lock(&priv->lock);
> >
> > @@ -5385,6 +5384,10 @@ int stmmac_suspend(struct device *dev)
> > }
> > mutex_unlock(&priv->lock);
> >
> > + rtnl_lock();
> > + phylink_stop(priv->phylink);
> > + rtnl_unlock();
> > +
> > priv->speed = SPEED_UNKNOWN;
> > return 0;
> > }
> > @@ -5448,6 +5451,12 @@ int stmmac_resume(struct device *dev)
> > pinctrl_pm_select_default_state(priv->device);
> > if (priv->plat->clk_ptp_ref)
> > clk_prepare_enable(priv->plat->clk_ptp_ref);
> > +
> > + rtnl_lock();
> > + /* We may have called phylink_speed_down before */
> > + phylink_speed_up(priv->phylink);
> > + rtnl_unlock();
> > +
> > /* reset the phy so that it's ready */
> > if (priv->mii && priv->mdio_rst_after_resume)
> > stmmac_mdio_reset(priv->mii); @@ -5461,13
> > +5470,9 @@ int stmmac_resume(struct device *dev)
> > return ret;
> > }
> >
> > - if (!device_may_wakeup(priv->device) || !priv->plat->pmt) {
> > - rtnl_lock();
> > - phylink_start(priv->phylink);
> > - /* We may have called phylink_speed_down before */
> > - phylink_speed_up(priv->phylink);
> > - rtnl_unlock();
> > - }
> > + rtnl_lock();
> > + phylink_start(priv->phylink);
> > + rtnl_unlock();
> >
> > rtnl_lock();
> > mutex_lock(&priv->lock);
>
> You also need to remove the calls to phylink_mac_change() from the
> suspend/resume functions. Without knowing how WoL is configured to work in
> your setup, I couldn't comment why it isn't working. Can you give some hints
> please?
The wakeup pattern is WAKE_MAGIC.
For WoL active case,
I tried remove phylink_mac_change()=false, add phylink_stop() from suspend patch;
And remove phylink_mac_change()=true, add phylink_start() from resume patch;
System still can't be waked up, I will follow up to debug why system can't be waked up
by magic packets if we call phylink_stop() when suspend. If you have any experience, happy you
can share with me. The only difference is that whether we call phylink_stop() or not.
> Also, what configuration of WoL are you using? I see stmmac supports several
> different configurations, but I assume priv->plat->pmt is NULL here?
WoL pattern is WAKE_MAGIC, and priv->plat->pmt is set since we use MAC-based WoL, not PHY-based WoL.
> > > phylink_stop() (and therefore phy_stop()) should be called even if
> > > WoL is active to shut down this state reporting, as other network drivers
> do.
> >
> > Ok, you mean that phylink_stop() also should be called even if WoL is
> > active, I would look in this direction since you are a professional.
>
> Yes. If the system is suspending for whatever reason, you want to bring the
> MAC down so that when it resumes, the MAC will see a link up event
> afterwards.
Ok, thanks.
Best Regards,
Joakim Zhang
Powered by blists - more mailing lists