[PATCH] net: phy: fix PHY_RUNNING in phy_state_machine
From: Shaohui Xie
Currently, if phy state is PHY_RUNNING, we always register a CHANGE
when phy works in polling or interrupt ignored, this will make the
adjust_link being called even the phy link did Not changed.
checking the phy link to make sure the link did changed before we
register a CHANGE, if link did not changed, we do nothing.
Signed-off-by: Shaohui Xie
---
drivers/net/phy/phy.c | 16
1 file changed, 12 insertions(+), 4 deletions(-)
diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 84b1fba..d972851 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -814,6 +814,7 @@ void phy_state_machine(struct work_struct *work)
bool needs_aneg = false, do_suspend = false;
enum phy_state old_state;
int err = 0;
+ int old_link;
mutex_lock(&phydev->lock);
@@ -899,11 +900,18 @@ void phy_state_machine(struct work_struct *work)
phydev->adjust_link(phydev->attached_dev);
break;
case PHY_RUNNING:
- /* Only register a CHANGE if we are
-* polling or ignoring interrupts
+ /* Only register a CHANGE if we are polling or ignoring
+* interrupts and link changed since latest checking.
*/
- if (!phy_interrupt_is_valid(phydev))
- phydev->state = PHY_CHANGELINK;
+ if (!phy_interrupt_is_valid(phydev)) {
+ old_link = phydev->link;
+ err = phy_read_status(phydev);
+ if (err)
+ break;
+
+ if (old_link != phydev->link)
+ phydev->state = PHY_CHANGELINK;
+ }
break;
case PHY_CHANGELINK:
err = phy_read_status(phydev);
--
2.1.0.27.g96db324
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] net: phy: fix PHY_RUNNING in phy_state_machine
On Thu, Mar 17, 2016 at 4:28 PM, Andrew Lunn wrote: >> > You should however consider writing a DSA driver for the switch. >> >> Do you mean SWITCHDEV or is this more or less the same? From time to >> time I'm looking at DSA/switchdev patches in the mailing list, but >> there seems to be not so many example in kernel. What are the latest >> slides, papers aside from Documentation/networking/switchdev.txt? > > I don't have access to the datasheet for this device. So i've no idea > how easy/hard it would be. > > Documentation/networking/switchdev.txt and > Documentation/networking/dsa/dsa.txt would be a good place to start. > > The mv88e6060.c is the simplest driver and gives you the minimum you > need to start with. Looking at the marketing brief, it looks like the > device can do more. But it is best to start simple, get the minimal > accepted, and then incrementally add more. Will do. Thanks. Yegor
Re: [PATCH] net: phy: fix PHY_RUNNING in phy_state_machine
On Thu, Mar 17, 2016 at 09:14:17AM +0100, Yegor Yefremov wrote:
> On Thu, Mar 17, 2016 at 12:05 AM, Andrew Lunn wrote:
> > On Wed, Mar 16, 2016 at 11:23:59PM +0100, Yegor Yefremov wrote:
> >> Hi Andrew,
> >>
> >> On Wed, Mar 16, 2016 at 5:18 PM, Andrew Lunn wrote:
> >> > On Wed, Mar 16, 2016 at 04:59:23PM +0100, Yegor Yefremov wrote:
> >> >
> >> >> This patch breaks my am335x based board, where one of the CPSW slaves
> >> >> is connected to IP175D switch chip via RMII interface. Since this
> >> >> patch packet reception is not working.
> >> >
> >> > Hi Yegor
> >> >
> >> > Which phy is causing the problem? A PHY inside the switch?
> >> >
> >> > Do you have two back to back PHYs between the MAC and the switch, or
> >> > is the CPSW RMII connected directly to the switch?
> >>
> >> CPSW RMII is connected directly to the switch.
> >
> > So which PHY is causing you problems?
Hi Yegor
Thanks for the details. This helps explain what is going on.
I'm looking at:
http://www.icplus.com.tw/pp-IP175D.html
> First of all this is the system in question [1]. am335x CPSW has two
> slaves and in this particular configuration CPSW is working in Dual
> EMAC mode, so that both slaves are independent interfaces eth0 and
> eth1.
>
> eth1 is connected to Atheros 8035 PHY via RGMII channel and is working
> as expected. eth0 is connected to ICPlus IP175D via RMII interface, so
> from CPSW point of view ICPlus IP175D is just an ordinary PHY. Both
> Atheros 8035 and ICPlus IP175D are connected via MDIO, so that both of
> them will be detected at runtime:
>
> davinci_mdio 4a101000.mdio: davinci mdio revision 1.6
> davinci_mdio 4a101000.mdio: detected phy mask f00fff00
> Atheros 8035 ethernet 4a101000.mdio:07: GPIO lookup for consumer reset
> Atheros 8035 ethernet 4a101000.mdio:07: using lookup tables for GPIO lookup
> Atheros 8035 ethernet 4a101000.mdio:07: lookup for GPIO reset failed
> libphy: 4a101000.mdio: probed
> davinci_mdio 4a101000.mdio: phy[0]: device 4a101000.mdio:00, driver
> ICPlus IP175C
> davinci_mdio 4a101000.mdio: phy[1]: device 4a101000.mdio:01, driver
> ICPlus IP175C
> davinci_mdio 4a101000.mdio: phy[2]: device 4a101000.mdio:02, driver
> ICPlus IP175C
> davinci_mdio 4a101000.mdio: phy[3]: device 4a101000.mdio:03, driver
> ICPlus IP175C
> davinci_mdio 4a101000.mdio: phy[4]: device 4a101000.mdio:04, driver
> ICPlus IP175C
So here we see the 5 PHYs connected to the switch. What we don't see
is what phy it connected to eth0. Since there is no PHY connected to
eth0, you should have a fixed_link node in your device tree.
I assume you are using am335x-baltos-ir5221.dts?
&cpsw_emac0 {
phy_id = <&davinci_mdio>, <0>;
phy-mode = "rmii";
dual_emac_res_vlan = <1>;
};
I'm assuming this means use the PHY at address 0 on the MDIO bus. This
is your problem. You have logically connected PHY0 to the eth0. So if
PHY0 is down, the MAC logically has no carrier. Hence you don't see
any packets. You should be able to quickly prove this. See what
happens when you connect a peer to port0 so the link comes up.
In reality, your hardware does not have a PHY connected to the MAC. It
goes straight to the switch. So you should be using a fixed-link here.
Try something like:
&cpsw_emac0 {
ixed-link {
speed = <100>;
full-duplex;
};
phy-mode = "rmii";
dual_emac_res_vlan = <1>;
};
Andrew
Re: [PATCH] net: phy: fix PHY_RUNNING in phy_state_machine
On Thu, Mar 17, 2016 at 4:12 PM, Andrew Lunn wrote:
>> After changing cpsw_emac0 entry to:
>>
>> &cpsw_emac0 {
>> phy-mode = "rmii";
>> dual_emac_res_vlan = <1>;
>> fixed-link {
>> speed = <100>;
>> full-duplex;
>> };
>> };
>>
>> I've got packets running in both directions.
>
> Great.
>
>> Now I have another problem: I cannot disable ICPlus IP175D ports via
>> SIOCSMIIREG as I could do previously. I get not ioctl errors [1], but
>> the ports are always on.
>>
>> [1]
>> https://github.com/visionsystemsgmbh/libonrisc/blob/master/src/onrisc.c#L83
>
> The MDIO bus is now logically not connected to eth0. Instead you have
> the fixed-link mdio device connected to eth0. So SIOCSMIIREG calls on
> eth0 now try to manipulate the fixed link phys.
>
> However, i think you can still access the MDIO bus, just use eth1 in
> your ioctl call.
Good trick :-)
> You should however consider writing a DSA driver for the switch.
Do you mean SWITCHDEV or is this more or less the same? From time to
time I'm looking at DSA/switchdev patches in the mailing list, but
there seems to be not so many example in kernel. What are the latest
slides, papers aside from Documentation/networking/switchdev.txt?
Yegor
Re: [PATCH] net: phy: fix PHY_RUNNING in phy_state_machine
On Wed, Mar 16, 2016 at 11:23:59PM +0100, Yegor Yefremov wrote: > Hi Andrew, > > On Wed, Mar 16, 2016 at 5:18 PM, Andrew Lunn wrote: > > On Wed, Mar 16, 2016 at 04:59:23PM +0100, Yegor Yefremov wrote: > > > >> This patch breaks my am335x based board, where one of the CPSW slaves > >> is connected to IP175D switch chip via RMII interface. Since this > >> patch packet reception is not working. > > > > Hi Yegor > > > > Which phy is causing the problem? A PHY inside the switch? > > > > Do you have two back to back PHYs between the MAC and the switch, or > > is the CPSW RMII connected directly to the switch? > > CPSW RMII is connected directly to the switch. So which PHY is causing you problems? Andrew
Re: [PATCH] net: phy: fix PHY_RUNNING in phy_state_machine
> > You should however consider writing a DSA driver for the switch. > > Do you mean SWITCHDEV or is this more or less the same? From time to > time I'm looking at DSA/switchdev patches in the mailing list, but > there seems to be not so many example in kernel. What are the latest > slides, papers aside from Documentation/networking/switchdev.txt? I don't have access to the datasheet for this device. So i've no idea how easy/hard it would be. Documentation/networking/switchdev.txt and Documentation/networking/dsa/dsa.txt would be a good place to start. The mv88e6060.c is the simplest driver and gives you the minimum you need to start with. Looking at the marketing brief, it looks like the device can do more. But it is best to start simple, get the minimal accepted, and then incrementally add more. Andrew
Re: [PATCH] net: phy: fix PHY_RUNNING in phy_state_machine
On Fri, Aug 14, 2015 at 6:23 AM, wrote:
> From: Shaohui Xie
>
> Currently, if phy state is PHY_RUNNING, we always register a CHANGE
> when phy works in polling or interrupt ignored, this will make the
> adjust_link being called even the phy link did Not changed.
>
> checking the phy link to make sure the link did changed before we
> register a CHANGE, if link did not changed, we do nothing.
>
> Signed-off-by: Shaohui Xie
> ---
> drivers/net/phy/phy.c | 16
> 1 file changed, 12 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> index 84b1fba..d972851 100644
> --- a/drivers/net/phy/phy.c
> +++ b/drivers/net/phy/phy.c
> @@ -814,6 +814,7 @@ void phy_state_machine(struct work_struct *work)
> bool needs_aneg = false, do_suspend = false;
> enum phy_state old_state;
> int err = 0;
> + int old_link;
>
> mutex_lock(&phydev->lock);
>
> @@ -899,11 +900,18 @@ void phy_state_machine(struct work_struct *work)
> phydev->adjust_link(phydev->attached_dev);
> break;
> case PHY_RUNNING:
> - /* Only register a CHANGE if we are
> -* polling or ignoring interrupts
> + /* Only register a CHANGE if we are polling or ignoring
> +* interrupts and link changed since latest checking.
> */
> - if (!phy_interrupt_is_valid(phydev))
> - phydev->state = PHY_CHANGELINK;
> + if (!phy_interrupt_is_valid(phydev)) {
> + old_link = phydev->link;
> + err = phy_read_status(phydev);
> + if (err)
> + break;
> +
> + if (old_link != phydev->link)
> + phydev->state = PHY_CHANGELINK;
> + }
> break;
> case PHY_CHANGELINK:
> err = phy_read_status(phydev);
This patch breaks my am335x based board, where one of the CPSW slaves
is connected to IP175D switch chip via RMII interface. Since this
patch packet reception is not working.
Yegor
Re: [PATCH] net: phy: fix PHY_RUNNING in phy_state_machine
On Thu, Mar 17, 2016 at 12:05 AM, Andrew Lunn wrote: > On Wed, Mar 16, 2016 at 11:23:59PM +0100, Yegor Yefremov wrote: >> Hi Andrew, >> >> On Wed, Mar 16, 2016 at 5:18 PM, Andrew Lunn wrote: >> > On Wed, Mar 16, 2016 at 04:59:23PM +0100, Yegor Yefremov wrote: >> > >> >> This patch breaks my am335x based board, where one of the CPSW slaves >> >> is connected to IP175D switch chip via RMII interface. Since this >> >> patch packet reception is not working. >> > >> > Hi Yegor >> > >> > Which phy is causing the problem? A PHY inside the switch? >> > >> > Do you have two back to back PHYs between the MAC and the switch, or >> > is the CPSW RMII connected directly to the switch? >> >> CPSW RMII is connected directly to the switch. > > So which PHY is causing you problems? First of all this is the system in question [1]. am335x CPSW has two slaves and in this particular configuration CPSW is working in Dual EMAC mode, so that both slaves are independent interfaces eth0 and eth1. eth1 is connected to Atheros 8035 PHY via RGMII channel and is working as expected. eth0 is connected to ICPlus IP175D via RMII interface, so from CPSW point of view ICPlus IP175D is just an ordinary PHY. Both Atheros 8035 and ICPlus IP175D are connected via MDIO, so that both of them will be detected at runtime: davinci_mdio 4a101000.mdio: davinci mdio revision 1.6 davinci_mdio 4a101000.mdio: detected phy mask f00fff00 Atheros 8035 ethernet 4a101000.mdio:07: GPIO lookup for consumer reset Atheros 8035 ethernet 4a101000.mdio:07: using lookup tables for GPIO lookup Atheros 8035 ethernet 4a101000.mdio:07: lookup for GPIO reset failed libphy: 4a101000.mdio: probed davinci_mdio 4a101000.mdio: phy[0]: device 4a101000.mdio:00, driver ICPlus IP175C davinci_mdio 4a101000.mdio: phy[1]: device 4a101000.mdio:01, driver ICPlus IP175C davinci_mdio 4a101000.mdio: phy[2]: device 4a101000.mdio:02, driver ICPlus IP175C davinci_mdio 4a101000.mdio: phy[3]: device 4a101000.mdio:03, driver ICPlus IP175C davinci_mdio 4a101000.mdio: phy[4]: device 4a101000.mdio:04, driver ICPlus IP175C davinci_mdio 4a101000.mdio: phy[5]: device 4a101000.mdio:05, driver unknown davinci_mdio 4a101000.mdio: phy[6]: device 4a101000.mdio:06, driver unknown davinci_mdio 4a101000.mdio: phy[7]: device 4a101000.mdio:07, driver Atheros 8035 ethernet davinci_mdio 4a101000.mdio: phy[20]: device 4a101000.mdio:14, driver unknown davinci_mdio 4a101000.mdio: phy[21]: device 4a101000.mdio:15, driver unknown davinci_mdio 4a101000.mdio: phy[22]: device 4a101000.mdio:16, driver unknown davinci_mdio 4a101000.mdio: phy[23]: device 4a101000.mdio:17, driver unknown davinci_mdio 4a101000.mdio: phy[24]: device 4a101000.mdio:18, driver unknown davinci_mdio 4a101000.mdio: phy[25]: device 4a101000.mdio:19, driver unknown davinci_mdio 4a101000.mdio: phy[26]: device 4a101000.mdio:1a, driver unknown davinci_mdio 4a101000.mdio: phy[27]: device 4a101000.mdio:1b, driver unknown >From ICPlus IP175D point of view eth0 is just a fifth port in the switch. ICPlus IP175D is in its default configuration, i.e. just acts as an unmanaged Ethernet switch. As soon as eth0 will be brought up it has constant 100Mbps connection. I assume, that from MDIO signalling this connection differs from physical cable insertion. In prior kernels when I make ip link set eth0 up I get: cpsw 4a10.ethernet eth0: Link is Up - 100Mbps/Full - flow control off After this patch I don't get this state message. Though I cannot see differences in "ethtool eth0" or "ip addr show eth0" for both kernels. So I assume, that ICPlus IP175D stays in PHY_RUNNING state. Let me know, what additional info do you need. [1] http://www.visionsystems.de/produkte/baltos-ir-5221.html Yegor
Re: [PATCH] net: phy: fix PHY_RUNNING in phy_state_machine
Hi Andrew, On Wed, Mar 16, 2016 at 5:18 PM, Andrew Lunn wrote: > On Wed, Mar 16, 2016 at 04:59:23PM +0100, Yegor Yefremov wrote: > >> This patch breaks my am335x based board, where one of the CPSW slaves >> is connected to IP175D switch chip via RMII interface. Since this >> patch packet reception is not working. > > Hi Yegor > > Which phy is causing the problem? A PHY inside the switch? > > Do you have two back to back PHYs between the MAC and the switch, or > is the CPSW RMII connected directly to the switch? CPSW RMII is connected directly to the switch. Yegor
Re: [PATCH] net: phy: fix PHY_RUNNING in phy_state_machine
> After changing cpsw_emac0 entry to:
>
> &cpsw_emac0 {
> phy-mode = "rmii";
> dual_emac_res_vlan = <1>;
> fixed-link {
> speed = <100>;
> full-duplex;
> };
> };
>
> I've got packets running in both directions.
Great.
> Now I have another problem: I cannot disable ICPlus IP175D ports via
> SIOCSMIIREG as I could do previously. I get not ioctl errors [1], but
> the ports are always on.
>
> [1]
> https://github.com/visionsystemsgmbh/libonrisc/blob/master/src/onrisc.c#L83
The MDIO bus is now logically not connected to eth0. Instead you have
the fixed-link mdio device connected to eth0. So SIOCSMIIREG calls on
eth0 now try to manipulate the fixed link phys.
However, i think you can still access the MDIO bus, just use eth1 in
your ioctl call.
You should however consider writing a DSA driver for the switch.
Andrew
Re: [PATCH] net: phy: fix PHY_RUNNING in phy_state_machine
On Wed, Mar 16, 2016 at 04:59:23PM +0100, Yegor Yefremov wrote: > This patch breaks my am335x based board, where one of the CPSW slaves > is connected to IP175D switch chip via RMII interface. Since this > patch packet reception is not working. Hi Yegor Which phy is causing the problem? A PHY inside the switch? Do you have two back to back PHYs between the MAC and the switch, or is the CPSW RMII connected directly to the switch? Andrew
Re: [PATCH] net: phy: fix PHY_RUNNING in phy_state_machine
On Thu, Mar 17, 2016 at 2:41 PM, Andrew Lunn wrote:
> On Thu, Mar 17, 2016 at 09:14:17AM +0100, Yegor Yefremov wrote:
>> On Thu, Mar 17, 2016 at 12:05 AM, Andrew Lunn wrote:
>> > On Wed, Mar 16, 2016 at 11:23:59PM +0100, Yegor Yefremov wrote:
>> >> Hi Andrew,
>> >>
>> >> On Wed, Mar 16, 2016 at 5:18 PM, Andrew Lunn wrote:
>> >> > On Wed, Mar 16, 2016 at 04:59:23PM +0100, Yegor Yefremov wrote:
>> >> >
>> >> >> This patch breaks my am335x based board, where one of the CPSW slaves
>> >> >> is connected to IP175D switch chip via RMII interface. Since this
>> >> >> patch packet reception is not working.
>> >> >
>> >> > Hi Yegor
>> >> >
>> >> > Which phy is causing the problem? A PHY inside the switch?
>> >> >
>> >> > Do you have two back to back PHYs between the MAC and the switch, or
>> >> > is the CPSW RMII connected directly to the switch?
>> >>
>> >> CPSW RMII is connected directly to the switch.
>> >
>> > So which PHY is causing you problems?
>
> Hi Yegor
>
> Thanks for the details. This helps explain what is going on.
>
> I'm looking at:
>
> http://www.icplus.com.tw/pp-IP175D.html
>
>> First of all this is the system in question [1]. am335x CPSW has two
>> slaves and in this particular configuration CPSW is working in Dual
>> EMAC mode, so that both slaves are independent interfaces eth0 and
>> eth1.
>>
>> eth1 is connected to Atheros 8035 PHY via RGMII channel and is working
>> as expected. eth0 is connected to ICPlus IP175D via RMII interface, so
>> from CPSW point of view ICPlus IP175D is just an ordinary PHY. Both
>> Atheros 8035 and ICPlus IP175D are connected via MDIO, so that both of
>> them will be detected at runtime:
>>
>> davinci_mdio 4a101000.mdio: davinci mdio revision 1.6
>> davinci_mdio 4a101000.mdio: detected phy mask f00fff00
>> Atheros 8035 ethernet 4a101000.mdio:07: GPIO lookup for consumer reset
>> Atheros 8035 ethernet 4a101000.mdio:07: using lookup tables for GPIO lookup
>> Atheros 8035 ethernet 4a101000.mdio:07: lookup for GPIO reset failed
>> libphy: 4a101000.mdio: probed
>> davinci_mdio 4a101000.mdio: phy[0]: device 4a101000.mdio:00, driver
>> ICPlus IP175C
>> davinci_mdio 4a101000.mdio: phy[1]: device 4a101000.mdio:01, driver
>> ICPlus IP175C
>> davinci_mdio 4a101000.mdio: phy[2]: device 4a101000.mdio:02, driver
>> ICPlus IP175C
>> davinci_mdio 4a101000.mdio: phy[3]: device 4a101000.mdio:03, driver
>> ICPlus IP175C
>> davinci_mdio 4a101000.mdio: phy[4]: device 4a101000.mdio:04, driver
>> ICPlus IP175C
>
> So here we see the 5 PHYs connected to the switch. What we don't see
> is what phy it connected to eth0. Since there is no PHY connected to
> eth0, you should have a fixed_link node in your device tree.
>
> I assume you are using am335x-baltos-ir5221.dts?
>
> &cpsw_emac0 {
> phy_id = <&davinci_mdio>, <0>;
> phy-mode = "rmii";
> dual_emac_res_vlan = <1>;
> };
>
> I'm assuming this means use the PHY at address 0 on the MDIO bus. This
> is your problem. You have logically connected PHY0 to the eth0. So if
> PHY0 is down, the MAC logically has no carrier. Hence you don't see
> any packets. You should be able to quickly prove this. See what
> happens when you connect a peer to port0 so the link comes up.
>
> In reality, your hardware does not have a PHY connected to the MAC. It
> goes straight to the switch. So you should be using a fixed-link here.
>
> Try something like:
>
> &cpsw_emac0 {
> ixed-link {
> speed = <100>;
> full-duplex;
> };
>
> phy-mode = "rmii";
> dual_emac_res_vlan = <1>;
> };
After changing cpsw_emac0 entry to:
&cpsw_emac0 {
phy-mode = "rmii";
dual_emac_res_vlan = <1>;
fixed-link {
speed = <100>;
full-duplex;
};
};
I've got packets running in both directions.
Now I have another problem: I cannot disable ICPlus IP175D ports via
SIOCSMIIREG as I could do previously. I get not ioctl errors [1], but
the ports are always on.
[1] https://github.com/visionsystemsgmbh/libonrisc/blob/master/src/onrisc.c#L83
Re: [PATCH] net: phy: fix PHY_RUNNING in phy_state_machine
Le 08/13/15 21:23, shh@gmail.com a écrit :
> From: Shaohui Xie
>
> Currently, if phy state is PHY_RUNNING, we always register a CHANGE
> when phy works in polling or interrupt ignored, this will make the
> adjust_link being called even the phy link did Not changed.
Right, which is why most drivers do implement a caching scheme.
>
> checking the phy link to make sure the link did changed before we
> register a CHANGE, if link did not changed, we do nothing.
With your change we will end-up with virtually polling a PHY twice as
fast as we used to with the RUNNING -> CHANGELINK -> RUNNING transition
(current state transitions), which is probably fine, but puts a bit more
pressure on the (slow) MDIO bus since we end-up with two additional
reads to latch the link status register.
PS: I would appreciate if you could CC me on future libphy submissions.
>
> Signed-off-by: Shaohui Xie
> ---
> drivers/net/phy/phy.c | 16
> 1 file changed, 12 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> index 84b1fba..d972851 100644
> --- a/drivers/net/phy/phy.c
> +++ b/drivers/net/phy/phy.c
> @@ -814,6 +814,7 @@ void phy_state_machine(struct work_struct *work)
> bool needs_aneg = false, do_suspend = false;
> enum phy_state old_state;
> int err = 0;
> + int old_link;
>
> mutex_lock(&phydev->lock);
>
> @@ -899,11 +900,18 @@ void phy_state_machine(struct work_struct *work)
> phydev->adjust_link(phydev->attached_dev);
> break;
> case PHY_RUNNING:
> - /* Only register a CHANGE if we are
> - * polling or ignoring interrupts
> + /* Only register a CHANGE if we are polling or ignoring
> + * interrupts and link changed since latest checking.
>*/
> - if (!phy_interrupt_is_valid(phydev))
> - phydev->state = PHY_CHANGELINK;
> + if (!phy_interrupt_is_valid(phydev)) {
> + old_link = phydev->link;
> + err = phy_read_status(phydev);
> + if (err)
> + break;
> +
> + if (old_link != phydev->link)
> + phydev->state = PHY_CHANGELINK;
> + }
> break;
> case PHY_CHANGELINK:
> err = phy_read_status(phydev);
>
--
Florian
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH] net: phy: fix PHY_RUNNING in phy_state_machine
> -Original Message- > From: Florian Fainelli [mailto:f.faine...@gmail.com] > Sent: Saturday, August 15, 2015 1:02 AM > To: shh@gmail.com; netdev@vger.kernel.org; da...@davemloft.net > Cc: Xie Shaohui-B21989 > Subject: Re: [PATCH] net: phy: fix PHY_RUNNING in phy_state_machine > > Le 08/13/15 21:23, shh@gmail.com a écrit : > > From: Shaohui Xie > > > > Currently, if phy state is PHY_RUNNING, we always register a CHANGE > > when phy works in polling or interrupt ignored, this will make the > > adjust_link being called even the phy link did Not changed. > > Right, which is why most drivers do implement a caching scheme. > > > > > checking the phy link to make sure the link did changed before we > > register a CHANGE, if link did not changed, we do nothing. > > With your change we will end-up with virtually polling a PHY twice as fast as > we > used to with the RUNNING -> CHANGELINK -> RUNNING transition (current state > transitions), [S.H] Yes. If the link did changed, we'll polling the PHY status twice with my change, but if the link did not changed, we'll only need polling the PHY status without calling adjust_link, for phy_state_machine works in polling mode at frequency of HZ, many adjust_link can be saved. > which is probably fine, but puts a bit more pressure on the (slow) > MDIO bus since we end-up with two additional reads to latch the link status > register. [S.H] Yes, but adjust_link in most driver is more complex than reading PHY status. And the link won't be changed very frequently. > > PS: I would appreciate if you could CC me on future libphy submissions. [S.H] OK. Thanks for reviewing! Shaohui
RE: [PATCH] net: phy: fix PHY_RUNNING in phy_state_machine
> -Original Message-
> From: Florian Fainelli [mailto:f.faine...@gmail.com]
> Sent: Saturday, August 15, 2015 1:02 AM
> To: shh@gmail.com; netdev@vger.kernel.org; da...@davemloft.net
> Cc: Xie Shaohui-B21989
> Subject: Re: [PATCH] net: phy: fix PHY_RUNNING in phy_state_machine
>
> Le 08/13/15 21:23, shh@gmail.com a écrit :
> > From: Shaohui Xie
> >
> > Currently, if phy state is PHY_RUNNING, we always register a CHANGE
> > when phy works in polling or interrupt ignored, this will make the
> > adjust_link being called even the phy link did Not changed.
>
> Right, which is why most drivers do implement a caching scheme.
>
> >
> > checking the phy link to make sure the link did changed before we
> > register a CHANGE, if link did not changed, we do nothing.
>
> With your change we will end-up with virtually polling a PHY twice as fast as
> we
> used to with the RUNNING -> CHANGELINK -> RUNNING transition (current state
> transitions), which is probably fine, but puts a bit more pressure on the
> (slow)
> MDIO bus since we end-up with two additional reads to latch the link status
> register.
[S.H] How about put the link checking in state PHY_CHANGELINK, if the link did
changed,
Continue to original handle, if the link did not changed, modify the state to
PHY_RUNNING?
case PHY_CHANGELINK:
+ old_link = phydev->link;
err = phy_read_status(phydev);
if (err)
break;
+ if (old_link == phydev->link) {
+ phydev->state = PHY_RUNNING;
+ break;
+ }
+
Thanks!
Shaohui
N�r��yb�X��ǧv�^�){.n�+���z�^�)w* jg��� �ݢj/���z�ޖ��2�ޙ&�)ߡ�a���� �G���h��j:+v���w��٥
Re: [PATCH] net: phy: fix PHY_RUNNING in phy_state_machine
From: Date: Fri, 14 Aug 2015 12:23:40 +0800 > From: Shaohui Xie > > Currently, if phy state is PHY_RUNNING, we always register a CHANGE > when phy works in polling or interrupt ignored, this will make the > adjust_link being called even the phy link did Not changed. > > checking the phy link to make sure the link did changed before we > register a CHANGE, if link did not changed, we do nothing. > > Signed-off-by: Shaohui Xie Applied, thank you. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] net: phy: fix PHY_RUNNING in phy_state_machine
On Mon, Aug 17, 2015 at 2:18 PM, David Miller wrote: > > From: > Date: Fri, 14 Aug 2015 12:23:40 +0800 > > > From: Shaohui Xie > > > > Currently, if phy state is PHY_RUNNING, we always register a CHANGE > > when phy works in polling or interrupt ignored, this will make the > > adjust_link being called even the phy link did Not changed. > > > > checking the phy link to make sure the link did changed before we > > register a CHANGE, if link did not changed, we do nothing. > > > > Signed-off-by: Shaohui Xie > > Applied, thank you. My 3rd attempt to send this, so I apologize. Hopefully this final attempt is finally in plain text. Clearly, it's been a while... Florian had some good objections to this patch. Doing a PHY status read is a very time-consuming operation to do every iteration (it's probably thousands of cycles), and the idea is supposed to be that the read only happens in the PHY_CHANGELINK state. Every cycle of the state machine will read the status, and a change will require that the status be read twice. Worst of all, if you were to change between two links quickly, the state machine will NEVER notice the change, because it is only looking for the link up/down state to change. One solution might be to modify genphy_read_status() so that it doesn't do the dummy read up front to clear the sticky low bit. This *might* make it so that PHY_RUNNING always catches a link change. What I'm not 100% sure of is whether you can guarantee that any link change will require the link to first go down. We'd have to read the spec, though I suspect this would work. The other concern with this solution is that something might be depending on the old behavior. Certainly phy_change() would need to make sure to do that "dummy" read sometime after interrupts are enabled. Another solution is to revert this patch, and modify adjust_link() functions to only make changes if the state has changed (They all should be doing this, anyway, but I can imagine that some drivers/devices might have difficulty determining what's changed). Anyone have other ideas? I'm happy to implement the first solution, but I should note I don't have a lot of test hardware these days. Andy -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
