* [PATCH net-next 1/4] net: mv643xx_eth: Do not clobber PHY link outside of state machine
2017-02-06 23:55 [PATCH net-next 0/4] net: Incorrect use of phy_read_status() Florian Fainelli
@ 2017-02-06 23:55 ` Florian Fainelli
2017-02-07 0:13 ` Andrew Lunn
2017-02-06 23:55 ` [PATCH net-next 2/4] net: pxa168_eth: " Florian Fainelli
` (3 subsequent siblings)
4 siblings, 1 reply; 10+ messages in thread
From: Florian Fainelli @ 2017-02-06 23:55 UTC (permalink / raw)
To: netdev
Cc: zefir.kurtisi, Florian Fainelli, Sebastian Hesselbarth,
Wingman Kwok, Murali Karicheri, Andrew Lunn, Vivien Didelot,
David S. Miller, Philippe Reynes, Sergei Shtylyov, Jisheng Zhang,
Jarod Wilson, open list
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")
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
drivers/net/ethernet/marvell/mv643xx_eth.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/net/ethernet/marvell/mv643xx_eth.c b/drivers/net/ethernet/marvell/mv643xx_eth.c
index 20cb7f0de601..25642dee49d3 100644
--- a/drivers/net/ethernet/marvell/mv643xx_eth.c
+++ b/drivers/net/ethernet/marvell/mv643xx_eth.c
@@ -1504,9 +1504,7 @@ mv643xx_eth_get_link_ksettings_phy(struct mv643xx_eth_private *mp,
int err;
u32 supported, advertising;
- err = phy_read_status(dev->phydev);
- if (err == 0)
- err = phy_ethtool_ksettings_get(dev->phydev, cmd);
+ err = phy_ethtool_ksettings_get(dev->phydev, cmd);
/*
* The MAC does not support 1000baseT_Half.
--
2.9.3
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH net-next 1/4] net: mv643xx_eth: Do not clobber PHY link outside of state machine
2017-02-06 23:55 ` [PATCH net-next 1/4] net: mv643xx_eth: Do not clobber PHY link outside of state machine Florian Fainelli
@ 2017-02-07 0:13 ` Andrew Lunn
0 siblings, 0 replies; 10+ messages in thread
From: Andrew Lunn @ 2017-02-07 0:13 UTC (permalink / raw)
To: Florian Fainelli
Cc: netdev, zefir.kurtisi, Sebastian Hesselbarth, Wingman Kwok,
Murali Karicheri, Vivien Didelot, David S. Miller,
Philippe Reynes, Sergei Shtylyov, Jisheng Zhang, Jarod Wilson,
open list
On Mon, Feb 06, 2017 at 03:55:20PM -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")
>
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Andrew
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH net-next 2/4] net: pxa168_eth: Do not clobber PHY link outside of state machine
2017-02-06 23:55 [PATCH net-next 0/4] net: Incorrect use of phy_read_status() Florian Fainelli
2017-02-06 23:55 ` [PATCH net-next 1/4] net: mv643xx_eth: Do not clobber PHY link outside of state machine Florian Fainelli
@ 2017-02-06 23:55 ` Florian Fainelli
2017-02-06 23:55 ` [PATCH net-next 3/4] net: netcp: " Florian Fainelli
` (2 subsequent siblings)
4 siblings, 0 replies; 10+ messages in thread
From: Florian Fainelli @ 2017-02-06 23:55 UTC (permalink / raw)
To: netdev
Cc: zefir.kurtisi, Florian Fainelli, Sebastian Hesselbarth,
Wingman Kwok, Murali Karicheri, Andrew Lunn, Vivien Didelot,
David S. Miller, Philippe Reynes, Sergei Shtylyov, Jisheng Zhang,
Jarod Wilson, open list
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")
Since we don't have anything special, switch to the generic
phy_ethtool_get_link_ksettings() function now.
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
drivers/net/ethernet/marvell/pxa168_eth.c | 20 +-------------------
1 file changed, 1 insertion(+), 19 deletions(-)
diff --git a/drivers/net/ethernet/marvell/pxa168_eth.c b/drivers/net/ethernet/marvell/pxa168_eth.c
index 3376a19f1e19..28cb36d9e50a 100644
--- a/drivers/net/ethernet/marvell/pxa168_eth.c
+++ b/drivers/net/ethernet/marvell/pxa168_eth.c
@@ -274,8 +274,6 @@ enum hash_table_entry {
HASH_ENTRY_RECEIVE_DISCARD_BIT = 2
};
-static int pxa168_get_link_ksettings(struct net_device *dev,
- struct ethtool_link_ksettings *cmd);
static int pxa168_init_hw(struct pxa168_eth_private *pep);
static int pxa168_init_phy(struct net_device *dev);
static void eth_port_reset(struct net_device *dev);
@@ -987,10 +985,6 @@ static int pxa168_init_phy(struct net_device *dev)
if (err)
return err;
- err = pxa168_get_link_ksettings(dev, &cmd);
- if (err)
- return err;
-
cmd.base.phy_address = pep->phy_addr;
cmd.base.speed = pep->phy_speed;
cmd.base.duplex = pep->phy_duplex;
@@ -1370,18 +1364,6 @@ static int pxa168_eth_do_ioctl(struct net_device *dev, struct ifreq *ifr,
return -EOPNOTSUPP;
}
-static int pxa168_get_link_ksettings(struct net_device *dev,
- struct ethtool_link_ksettings *cmd)
-{
- int err;
-
- err = phy_read_status(dev->phydev);
- if (err == 0)
- err = phy_ethtool_ksettings_get(dev->phydev, cmd);
-
- return err;
-}
-
static void pxa168_get_drvinfo(struct net_device *dev,
struct ethtool_drvinfo *info)
{
@@ -1396,7 +1378,7 @@ static const struct ethtool_ops pxa168_ethtool_ops = {
.nway_reset = phy_ethtool_nway_reset,
.get_link = ethtool_op_get_link,
.get_ts_info = ethtool_op_get_ts_info,
- .get_link_ksettings = pxa168_get_link_ksettings,
+ .get_link_ksettings = phy_ethtool_get_link_ksettings,
.set_link_ksettings = phy_ethtool_set_link_ksettings,
};
--
2.9.3
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH net-next 3/4] net: netcp: Do not clobber PHY link outside of state machine
2017-02-06 23:55 [PATCH net-next 0/4] net: Incorrect use of phy_read_status() Florian Fainelli
2017-02-06 23:55 ` [PATCH net-next 1/4] net: mv643xx_eth: Do not clobber PHY link outside of state machine Florian Fainelli
2017-02-06 23:55 ` [PATCH net-next 2/4] net: pxa168_eth: " Florian Fainelli
@ 2017-02-06 23:55 ` Florian Fainelli
2017-02-06 23:55 ` [PATCH net-next 4/4] net: dsa: " Florian Fainelli
2017-02-07 18:48 ` [PATCH net-next 0/4] net: Incorrect use of phy_read_status() David Miller
4 siblings, 0 replies; 10+ messages in thread
From: Florian Fainelli @ 2017-02-06 23:55 UTC (permalink / raw)
To: netdev
Cc: zefir.kurtisi, Florian Fainelli, Sebastian Hesselbarth,
Wingman Kwok, Murali Karicheri, Andrew Lunn, Vivien Didelot,
David S. Miller, Philippe Reynes, Sergei Shtylyov, Jisheng Zhang,
Jarod Wilson, open list
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 off
811a919135b9 ("phy state machine: failsafe leave invalid RUNNING state")
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
drivers/net/ethernet/ti/netcp_ethss.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/net/ethernet/ti/netcp_ethss.c b/drivers/net/ethernet/ti/netcp_ethss.c
index f7bb241b17ab..eece3e2eec14 100644
--- a/drivers/net/ethernet/ti/netcp_ethss.c
+++ b/drivers/net/ethernet/ti/netcp_ethss.c
@@ -2313,7 +2313,6 @@ static int gbe_slave_open(struct gbe_intf *gbe_intf)
dev_dbg(priv->dev, "phy found: id is: 0x%s\n",
phydev_name(slave->phy));
phy_start(slave->phy);
- phy_read_status(slave->phy);
}
return 0;
}
@@ -3119,7 +3118,6 @@ static void init_secondary_ports(struct gbe_priv *gbe_dev,
dev_dbg(dev, "phy found: id is: 0x%s\n",
phydev_name(slave->phy));
phy_start(slave->phy);
- phy_read_status(slave->phy);
}
}
}
--
2.9.3
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH net-next 4/4] net: dsa: Do not clobber PHY link outside of state machine
2017-02-06 23:55 [PATCH net-next 0/4] net: Incorrect use of phy_read_status() Florian Fainelli
` (2 preceding siblings ...)
2017-02-06 23:55 ` [PATCH net-next 3/4] net: netcp: " Florian Fainelli
@ 2017-02-06 23:55 ` Florian Fainelli
2017-02-07 0:11 ` Andrew Lunn
2017-02-07 18:48 ` [PATCH net-next 0/4] net: Incorrect use of phy_read_status() David Miller
4 siblings, 1 reply; 10+ messages in thread
From: Florian Fainelli @ 2017-02-06 23:55 UTC (permalink / raw)
To: netdev
Cc: zefir.kurtisi, Florian Fainelli, Sebastian Hesselbarth,
Wingman Kwok, Murali Karicheri, Andrew Lunn, Vivien Didelot,
David S. Miller, Philippe Reynes, Sergei Shtylyov, Jisheng Zhang,
Jarod Wilson, open list
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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH net-next 4/4] net: dsa: Do not clobber PHY link outside of state machine
2017-02-06 23:55 ` [PATCH net-next 4/4] net: dsa: " Florian Fainelli
@ 2017-02-07 0:11 ` Andrew Lunn
2017-02-07 11:42 ` David Laight
0 siblings, 1 reply; 10+ messages in thread
From: Andrew Lunn @ 2017-02-07 0:11 UTC (permalink / raw)
To: Florian Fainelli
Cc: netdev, zefir.kurtisi, Sebastian Hesselbarth, Wingman Kwok,
Murali Karicheri, Vivien Didelot, David S. Miller,
Philippe Reynes, Sergei Shtylyov, Jisheng Zhang, Jarod Wilson,
open list
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* RE: [PATCH net-next 4/4] net: dsa: Do not clobber PHY link outside of state machine
2017-02-07 0:11 ` Andrew Lunn
@ 2017-02-07 11:42 ` David Laight
2017-02-07 13:14 ` Andrew Lunn
0 siblings, 1 reply; 10+ messages in thread
From: David Laight @ 2017-02-07 11:42 UTC (permalink / raw)
To: 'Andrew Lunn', Florian Fainelli
Cc: netdev, zefir.kurtisi, Sebastian Hesselbarth, Wingman Kwok,
Murali Karicheri, Vivien Didelot, David S. Miller,
Philippe Reynes, Sergei Shtylyov, Jisheng Zhang, Jarod Wilson,
open list
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net-next 4/4] net: dsa: Do not clobber PHY link outside of state machine
2017-02-07 11:42 ` David Laight
@ 2017-02-07 13:14 ` Andrew Lunn
0 siblings, 0 replies; 10+ messages in thread
From: Andrew Lunn @ 2017-02-07 13:14 UTC (permalink / raw)
To: David Laight
Cc: Florian Fainelli, netdev, zefir.kurtisi, Sebastian Hesselbarth,
Wingman Kwok, Murali Karicheri, Vivien Didelot, David S. Miller,
Philippe Reynes, Sergei Shtylyov, Jisheng Zhang, Jarod Wilson,
open list
> 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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net-next 0/4] net: Incorrect use of phy_read_status()
2017-02-06 23:55 [PATCH net-next 0/4] net: Incorrect use of phy_read_status() Florian Fainelli
` (3 preceding siblings ...)
2017-02-06 23:55 ` [PATCH net-next 4/4] net: dsa: " Florian Fainelli
@ 2017-02-07 18:48 ` David Miller
4 siblings, 0 replies; 10+ messages in thread
From: David Miller @ 2017-02-07 18:48 UTC (permalink / raw)
To: f.fainelli
Cc: netdev, zefir.kurtisi, sebastian.hesselbarth, w-kwok2,
m-karicheri2, andrew, vivien.didelot, tremyfr, sergei.shtylyov,
jszhang, jarod, linux-kernel
From: Florian Fainelli <f.fainelli@gmail.com>
Date: Mon, 6 Feb 2017 15:55:19 -0800
> This patch series removes incorrect uses of phy_read_status() which can clobber
> the PHY device link while we are executing with the state machine running.
>
> greth was potentially another candidate, but it does funky stuff with
> auto-negotation that I am still trying to understand.
Series applied, thanks.
^ permalink raw reply [flat|nested] 10+ messages in thread