netdev - RE: [PATCH RFC] ethernet: stmmac: clean up the code for release/suspend/resume function

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: <DB8PR04MB67952071DBA50ECF03BF9B98E6CD0@DB8PR04MB6795.eurprd04.prod.outlook.com>
Date:   Tue, 8 Dec 2020 10:49:03 +0000
From:   Joakim Zhang <qiangqing.zhang@....com>
To:     Jisheng Zhang <Jisheng.Zhang@...aptics.com>
CC:     "peppe.cavallaro@...com" <peppe.cavallaro@...com>,
        "alexandre.torgue@...com" <alexandre.torgue@...com>,
        "joabreu@...opsys.com" <joabreu@...opsys.com>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "kuba@...nel.org" <kuba@...nel.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        dl-linux-imx <linux-imx@....com>
Subject: RE: [PATCH RFC] ethernet: stmmac: clean up the code for
 release/suspend/resume function


> -----Original Message-----
> From: Jisheng Zhang <Jisheng.Zhang@...aptics.com>
> Sent: 2020年12月8日 18:24
> To: Joakim Zhang <qiangqing.zhang@....com>
> Cc: peppe.cavallaro@...com; alexandre.torgue@...com;
> joabreu@...opsys.com; davem@...emloft.net; kuba@...nel.org;
> netdev@...r.kernel.org; dl-linux-imx <linux-imx@....com>
> Subject: Re: [PATCH RFC] ethernet: stmmac: clean up the code for
> release/suspend/resume function
> 
> On Mon,  7 Dec 2020 19:38:49 +0800 Joakim Zhang wrote:
> 
> 
> >
> > commit 1c35cc9cf6a0 ("net: stmmac: remove redundant null check before
> > clk_disable_unprepare()"), have not clean up check NULL clock parameter
> completely, this patch did it.
> >
> > commit e8377e7a29efb ("net: stmmac: only call pmt() during
> > suspend/resume if HW enables PMT"), after this patch, we use if
> > (device_may_wakeup(priv->device) && priv->plat->pmt) check MAC wakeup
> > if (device_may_wakeup(priv->device)) check PHY wakeup Add oneline
> > comment for readability.
> >
> > commit 77b2898394e3b ("net: stmmac: Speed down the PHY if WoL to save
> > energy"), slow down phy speed when release net device under any condition.
> >
> > Slightly adjust the order of the codes so that suspend/resume look
> > more symmetrical, generally speaking they should appear symmetrically.
> >
> > Signed-off-by: Joakim Zhang <qiangqing.zhang@....com>
> > ---
> >  .../net/ethernet/stmicro/stmmac/stmmac_main.c | 22
> > +++++++++----------
> >  1 file changed, 10 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > index c33db79cdd0a..a46e865c4acc 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > @@ -2908,8 +2908,7 @@ static int stmmac_release(struct net_device *dev)
> >         struct stmmac_priv *priv = netdev_priv(dev);
> >         u32 chan;
> >
> > -       if (device_may_wakeup(priv->device))
> 
> This check is to prevent link speed down if the stmmac isn't a wakeup device.

When we invoke .ndo_stop, we down the net device. Per my understanding, we can speed down the phy, no matter it is a wakeup device or not.
Since when invoke .ndo_open to up the net devce, we will re-config mac and phy. Please point out to me if I mis-understand something. Thanks.

> > -               phylink_speed_down(priv->phylink, false);
> > +       phylink_speed_down(priv->phylink, false);
> >         /* Stop and disconnect the PHY */
> >         phylink_stop(priv->phylink);
> >         phylink_disconnect_phy(priv->phylink);
> > @@ -5183,6 +5182,7 @@ int stmmac_suspend(struct device *dev)
> >         } else {
> >                 mutex_unlock(&priv->lock);
> >                 rtnl_lock();
> > +               /* For PHY wakeup case */
> >                 if (device_may_wakeup(priv->device))
> >                         phylink_speed_down(priv->phylink, false);
> >                 phylink_stop(priv->phylink); @@ -5260,11 +5260,17 @@
> > int stmmac_resume(struct device *dev)
> >                 /* enable the clk previously disabled */
> >                 clk_prepare_enable(priv->plat->stmmac_clk);
> >                 clk_prepare_enable(priv->plat->pclk);
> > -               if (priv->plat->clk_ptp_ref)
> > -                       clk_prepare_enable(priv->plat->clk_ptp_ref);
> > +               clk_prepare_enable(priv->plat->clk_ptp_ref);
> 
> I think this 3 line modifications can be a separated patch.

Yes, this just a RFC to export issue.

> >                 /* reset the phy so that it's ready */
> >                 if (priv->mii)
> >                         stmmac_mdio_reset(priv->mii);
> > +
> > +               rtnl_lock();
> > +               phylink_start(priv->phylink);
> > +               /* We may have called phylink_speed_down before */
> > +               if (device_may_wakeup(priv->device))
> > +                       phylink_speed_up(priv->phylink);
> > +               rtnl_unlock();
> 
> This is moving phylink op before mac setup, I'm not sure whether this is safe.

We encounter an issue, need move phylink before mac setup, please see below patch.
https://www.spinics.net/lists/netdev/msg706458.html

Have not found problems after test. Is there ang risk?

Best Regards,
Joakim Zhang
> >         }
> >
> >         if (priv->plat->serdes_powerup) { @@ -5275,14 +5281,6 @@ 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();
> >         mutex_lock(&priv->lock);
> >
> > --
> > 2.17.1
> >