Mailing List Archive: [PATCH net-next 4/4] net: dsa: Do not clobber PHY link outside of state machine

[PATCH net-next 4/4] net: dsa: Do not clobber PHY link outside of state machine

Feb 6, 2017, 4:20 PM

Post #1 of 4 (24 views)

Calling phy_read_status() means that we may call into
genphy_read_status() which in turn will use genphy_update_link() which
can make changes to phydev->link outside of the state machine's state
transitions. This is an invalid behavior that is now caught as of
811a919135b9 ("phy state machine: failsafe leave invalid RUNNING state")

Reported-by: Zefir Kurtisi <zefir.kurtisi@neratec.com>
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
net/dsa/slave.c | 10 +++-------
1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index 09fc3e9462c1..4b6fb6b14de4 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -651,14 +651,10 @@ dsa_slave_get_link_ksettings(struct net_device *dev,
struct ethtool_link_ksettings *cmd)
{
struct dsa_slave_priv *p = netdev_priv(dev);
- int err;
+ int err = -EOPNOTSUPP;

- err = -EOPNOTSUPP;
- if (p->phy != NULL) {
- err = phy_read_status(p->phy);
- if (err == 0)
- err = phy_ethtool_ksettings_get(p->phy, cmd);
- }
+ if (p->phy != NULL)
+ err = phy_ethtool_ksettings_get(p->phy, cmd);

return err;
}
--
2.9.3

Re: [PATCH net-next 4/4] net: dsa: Do not clobber PHY link outside of state machine [ In reply to ]

andrew at lunn

Feb 6, 2017, 4:20 PM

Post #2 of 4 (24 views)

Permalink

On Mon, Feb 06, 2017 at 03:55:23PM -0800, Florian Fainelli wrote:
> Calling phy_read_status() means that we may call into
> genphy_read_status() which in turn will use genphy_update_link() which
> can make changes to phydev->link outside of the state machine's state
> transitions. This is an invalid behavior that is now caught as of
> 811a919135b9 ("phy state machine: failsafe leave invalid RUNNING state")
>
> Reported-by: Zefir Kurtisi <zefir.kurtisi@neratec.com>
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
> net/dsa/slave.c | 10 +++-------
> 1 file changed, 3 insertions(+), 7 deletions(-)
>
> diff --git a/net/dsa/slave.c b/net/dsa/slave.c
> index 09fc3e9462c1..4b6fb6b14de4 100644
> --- a/net/dsa/slave.c
> +++ b/net/dsa/slave.c
> @@ -651,14 +651,10 @@ dsa_slave_get_link_ksettings(struct net_device *dev,
> struct ethtool_link_ksettings *cmd)
> {
> struct dsa_slave_priv *p = netdev_priv(dev);
> - int err;
> + int err = -EOPNOTSUPP;
>
> - err = -EOPNOTSUPP;
> - if (p->phy != NULL) {
> - err = phy_read_status(p->phy);
> - if (err == 0)
> - err = phy_ethtool_ksettings_get(p->phy, cmd);
> - }
> + if (p->phy != NULL)
> + err = phy_ethtool_ksettings_get(p->phy, cmd);

Hi Florian

So what we are effectively doing is returning the state from the last
poll/interrupt. The poll information could be up to 1 second out of
date, but those PHYs using interrupts should give more fresh
information.

Seems reasonable.

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

Andrew

RE: [PATCH net-next 4/4] net: dsa: Do not clobber PHY link outside of state machine [ In reply to ]

David.Laight at ACULAB

Feb 7, 2017, 4:20 AM

Post #3 of 4 (20 views)

Permalink

From: Andrew Lunn
> Sent: 07 February 2017 00:12
> On Mon, Feb 06, 2017 at 03:55:23PM -0800, Florian Fainelli wrote:
> > Calling phy_read_status() means that we may call into
> > genphy_read_status() which in turn will use genphy_update_link() which
> > can make changes to phydev->link outside of the state machine's state
> > transitions. This is an invalid behavior that is now caught as of
> > 811a919135b9 ("phy state machine: failsafe leave invalid RUNNING state")
> >
> > Reported-by: Zefir Kurtisi <zefir.kurtisi@neratec.com>
> > Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> > ---
> > net/dsa/slave.c | 10 +++-------
> > 1 file changed, 3 insertions(+), 7 deletions(-)
> >
> > diff --git a/net/dsa/slave.c b/net/dsa/slave.c
> > index 09fc3e9462c1..4b6fb6b14de4 100644
> > --- a/net/dsa/slave.c
> > +++ b/net/dsa/slave.c
> > @@ -651,14 +651,10 @@ dsa_slave_get_link_ksettings(struct net_device *dev,
> > struct ethtool_link_ksettings *cmd)
> > {
> > struct dsa_slave_priv *p = netdev_priv(dev);
> > - int err;
> > + int err = -EOPNOTSUPP;
> >
> > - err = -EOPNOTSUPP;
> > - if (p->phy != NULL) {
> > - err = phy_read_status(p->phy);
> > - if (err == 0)
> > - err = phy_ethtool_ksettings_get(p->phy, cmd);
> > - }
> > + if (p->phy != NULL)
> > + err = phy_ethtool_ksettings_get(p->phy, cmd);
>
> Hi Florian
>
> So what we are effectively doing is returning the state from the last
> poll/interrupt. The poll information could be up to 1 second out of
> date, but those PHYs using interrupts should give more fresh
> information.

Another option is to request the state machine re-read the phy status
and wakeup the caller when it has the result.

Related is that polling the phy status every second can be bad news for
system latency.
Typically it involves bit-bashing an interface with spin-delays to
meet the slow timings (with extra delays to allow for posted writes).
An alternative would be to do a very slow bit-bang on each timer tick,
this would need to be sped up and finished before any other actions.

David

Re: [PATCH net-next 4/4] net: dsa: Do not clobber PHY link outside of state machine [ In reply to ]

andrew at lunn

Feb 7, 2017, 6:20 AM

Post #4 of 4 (19 views)

Permalink

> Related is that polling the phy status every second can be bad news for
> system latency.
> Typically it involves bit-bashing an interface with spin-delays to
> meet the slow timings (with extra delays to allow for posted writes).
> An alternative would be to do a very slow bit-bang on each timer tick,
> this would need to be sped up and finished before any other actions.

Or just use PHY interrupts. That completely stops the MDIO polling.

Andrew