All of lore.kernel.org
 help / color / mirror / Atom feed
From: "Russell King (Oracle)" <linux@armlinux.org.uk>
To: Joakim Zhang <qiangqing.zhang@nxp.com>
Cc: "peppe.cavallaro@st.com" <peppe.cavallaro@st.com>,
	"alexandre.torgue@foss.st.com" <alexandre.torgue@foss.st.com>,
	"joabreu@synopsys.com" <joabreu@synopsys.com>,
	"davem@davemloft.net" <davem@davemloft.net>,
	"kuba@kernel.org" <kuba@kernel.org>,
	"mcoquelin.stm32@gmail.com" <mcoquelin.stm32@gmail.com>,
	"netdev@vger.kernel.org" <netdev@vger.kernel.org>,
	"andrew@lunn.ch" <andrew@lunn.ch>,
	"f.fainelli@gmail.com" <f.fainelli@gmail.com>,
	"hkallweit1@gmail.com" <hkallweit1@gmail.com>,
	dl-linux-imx <linux-imx@nxp.com>
Subject: Re: [PATCH] net: stmmac: fix MAC not working when system resume back with WoL enabled
Date: Wed, 1 Sep 2021 13:56:36 +0100	[thread overview]
Message-ID: <20210901125636.GA22278@shell.armlinux.org.uk> (raw)
In-Reply-To: <DB8PR04MB67959C4B1D1AFEC5AEEB73F3E6CD9@DB8PR04MB6795.eurprd04.prod.outlook.com>

On Wed, Sep 01, 2021 at 10:21:59AM +0000, Joakim Zhang wrote:
> 
> Hi Russell,
> 
> > -----Original Message-----
> > From: Russell King <linux@armlinux.org.uk>
> > Sent: 2021年9月1日 17:14
> > To: Joakim Zhang <qiangqing.zhang@nxp.com>
> > Cc: peppe.cavallaro@st.com; alexandre.torgue@foss.st.com;
> > joabreu@synopsys.com; davem@davemloft.net; kuba@kernel.org;
> > mcoquelin.stm32@gmail.com; netdev@vger.kernel.org; andrew@lunn.ch;
> > f.fainelli@gmail.com; hkallweit1@gmail.com; dl-linux-imx <linux-imx@nxp.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://elixir.bootlin.com/linux/v5.4.143/source/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c#L852

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.

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

Also, what configuration of WoL are you using? I see stmmac supports
several different configurations, but I assume priv->plat->pmt is
NULL here?

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

Thanks.

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

  reply	other threads:[~2021-09-01 13:07 UTC|newest]

Thread overview: 32+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-09-01  9:02 Joakim Zhang
2021-09-01  9:13 ` Russell King (Oracle)
2021-09-01 10:21   ` Joakim Zhang
2021-09-01 12:56     ` Russell King (Oracle) [this message]
2021-09-02  7:01       ` Joakim Zhang
2021-09-01 15:40     ` Heiner Kallweit
2021-09-02  7:35       ` Joakim Zhang
2021-09-01  9:21 ` Vladimir Oltean
2021-09-01 10:25   ` Joakim Zhang
2021-09-01 10:56     ` Vladimir Oltean
2021-09-01 11:42       ` Joakim Zhang
2021-09-01 13:25         ` Russell King (Oracle)
2021-09-02  7:28           ` Joakim Zhang
2021-09-02  8:32             ` Russell King (Oracle)
2021-09-02 10:26               ` Joakim Zhang
2021-09-02 10:49                 ` Russell King (Oracle)
2021-09-02 11:15                   ` Joakim Zhang
2021-09-02 12:24                     ` Andrew Lunn
2021-09-03  6:51                       ` Joakim Zhang
2021-09-03  8:01                         ` Russell King (Oracle)
2021-09-03  8:39                           ` Joakim Zhang
2021-09-03  9:32                             ` Russell King (Oracle)
2021-09-03 11:04                               ` Joakim Zhang
2021-09-03 12:01                                 ` Russell King (Oracle)
2021-09-03 20:12                                   ` Russell King - ARM Linux admin
2021-09-06  2:29                                     ` Joakim Zhang
2021-09-06  9:34                                       ` Russell King (Oracle)
2021-09-06 10:41                                         ` Joakim Zhang
2021-09-06 11:21                                           ` Russell King (Oracle)
2021-09-06 13:23                                             ` Andrew Lunn
2021-09-07  8:52                                             ` Russell King (Oracle)
2021-09-06  2:21                                   ` Joakim Zhang

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210901125636.GA22278@shell.armlinux.org.uk \
    --to=linux@armlinux.org.uk \
    --cc=alexandre.torgue@foss.st.com \
    --cc=andrew@lunn.ch \
    --cc=davem@davemloft.net \
    --cc=f.fainelli@gmail.com \
    --cc=hkallweit1@gmail.com \
    --cc=joabreu@synopsys.com \
    --cc=kuba@kernel.org \
    --cc=linux-imx@nxp.com \
    --cc=mcoquelin.stm32@gmail.com \
    --cc=netdev@vger.kernel.org \
    --cc=peppe.cavallaro@st.com \
    --cc=qiangqing.zhang@nxp.com \
    --subject='Re: [PATCH] net: stmmac: fix MAC not working when system resume back with WoL enabled' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.