phydev: Add sysctl variable for polling interval of phy

From d55a22be52e5a768409aa0999d6636cdfc369676 Mon Sep 17 00:00:00 2001
From: eunbonsong 

Date: Sun, 10 Mar 2013 04:57:39 -0700
Subject: [PATCH] phydev: Add sysctl variable for polling interval of phy state

This adds a dev.phy.phy_poll_interval sysctl variable. This value is represented in milliseconds.
And phy_state_machine() is scheduled as this variable. 
I think HZ is enough for PC. But sometimes especially in network devices
such as switches,routers, needs more granularity for detecting phy state change. 


---
drivers/net/phy/phy.c        |    4 +++-
drivers/net/phy/phy_device.c |   43 ++++++++++++++++++++++++++++++++++++++++++
include/linux/phy.h          |    1 -
3 files changed, 46 insertions(+), 2 deletions(-)

-- 
1.7.10.4

Hello,

On 03/11/2013 12:44 AM, EUNBONG SONG wrote:
>
>  From d55a22be52e5a768409aa0999d6636cdfc369676 Mon Sep 17 00:00:00 2001
> From: eunbonsong
> Date: Sun, 10 Mar 2013 04:57:39 -0700
> Subject: [PATCH] phydev: Add sysctl variable for polling interval of phy state
>
> This adds a dev.phy.phy_poll_interval sysctl variable. This value is represented in milliseconds.
> And phy_state_machine() is scheduled as this variable.
> I think HZ is enough for PC. But sometimes especially in network devices
> such as switches,routers, needs more granularity for detecting phy state change.

This patch should be submitted according to the rules described in: 
https://www.kernel.org/doc/Documentation/SubmittingPatches

Besides that, I do not think that a system-wide knob here is 
appropriate, you may rather introduce a new ethtool ioctl() to change 
the PHY device polling interval on a per-PHY device basis.

Having said that don't your devices support a dedicated PHY interrupt 
line? This would definitively be the way to get better latency with 
respect to PHY events reported back to the host.

>
>
> ---
> drivers/net/phy/phy.c        |    4 +++-
> drivers/net/phy/phy_device.c |   43 ++++++++++++++++++++++++++++++++++++++++++
> include/linux/phy.h          |    1 -
> 3 files changed, 46 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> index ef9ea92..126a69f 100644
> --- a/drivers/net/phy/phy.c
> +++ b/drivers/net/phy/phy.c
> @@ -42,6 +42,8 @@
> #include
> #include
>
> +extern unsigned long sysctl_phy_poll_interval;
> +
> /**
>    * phy_print_status - Convenience function to print out the current phy status
>    * @phydev: the phy_device struct
> @@ -966,7 +968,7 @@ void phy_state_machine(struct work_struct *work)
>          if (err < 0)
>                  phy_error(phydev);
>
> -       schedule_delayed_work(&phydev->state_queue, PHY_STATE_TIME * HZ);
> +       schedule_delayed_work(&phydev->state_queue, msecs_to_jiffies(sysctl_phy_poll_interval));
> }
>
> static inline void mmd_phy_indirect(struct mii_bus *bus, int prtad, int devad,
> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index 3657b4a..c2697e2 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -33,6 +33,7 @@
> #include
> #include
> #include
> +#include
>
> #include
> #include
> @@ -42,6 +43,45 @@ MODULE_DESCRIPTION("PHY library");
> MODULE_AUTHOR("Andy Fleming");
> MODULE_LICENSE("GPL");
>
> +unsigned long sysctl_phy_poll_interval = 1000;
> +static unsigned long min_phy_poll_interval = 1;
> +static unsigned long max_phy_poll_interval = 10000;

1 millisecond sounds like we are going to eat up a lot of CPU time 
polling PHY registers. Do you need that much reactivity?
--
Florian
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Mon, 2013-03-11 at 11:45 +0100, Florian Fainelli wrote:
> Hello,
> 
> On 03/11/2013 12:44 AM, EUNBONG SONG wrote:
> >
> >  From d55a22be52e5a768409aa0999d6636cdfc369676 Mon Sep 17 00:00:00 2001
> > From: eunbonsong
> > Date: Sun, 10 Mar 2013 04:57:39 -0700
> > Subject: [PATCH] phydev: Add sysctl variable for polling interval of phy state
> >
> > This adds a dev.phy.phy_poll_interval sysctl variable. This value is represented in milliseconds.
> > And phy_state_machine() is scheduled as this variable.
> > I think HZ is enough for PC. But sometimes especially in network devices
> > such as switches,routers, needs more granularity for detecting phy state change.
> 
> This patch should be submitted according to the rules described in: 
> https://www.kernel.org/doc/Documentation/SubmittingPatches
> 
> Besides that, I do not think that a system-wide knob here is 
> appropriate, you may rather introduce a new ethtool ioctl() to change 
> the PHY device polling interval on a per-PHY device basis.

That sounds like the right place - if it's really necessary to make this
configurable.

> Having said that don't your devices support a dedicated PHY interrupt 
> line? This would definitively be the way to get better latency with 
> respect to PHY events reported back to the host.
[...]

It's nice to have working link interrupts, but they're apparently easy
to get wrong and it just doesn't hurt that much to poll.  They never
worked properly on SFC4000 boards, and polling once a second was good
enough in practice.

Ben.