Re: [PATCH net-next] net: phy: Trigger state machine on state change and not polling.

[David Miller]

From: Andrew Lunn 
Date: Thu, 13 Oct 2016 18:29:01 +0200

> Just for my clarification:
> 
> We are in the middle of the merge window. What does net-next and net
> mean at the moment?

Once I merge net-next to Linus, bug fixes should go to 'net'.

And for the past day or so I've been slowly merging new features and
cleanups into 'net-next'.  I realize that this latter aspect is a
departure from the past, but no matter how hard I try people still
submit net-next things during the merge window so I've basically given
up pushing back on that.

Re: [PATCH net-next] net: phy: Trigger state machine on state change and not polling.

[Andrew Lunn]

> On Thu, Oct 13, 2016 at 12:04:38PM -0400, David Miller wrote:
> From: Andrew Lunn 
> Date: Wed, 12 Oct 2016 22:14:53 +0200
> 
> > The phy_start() is used to indicate the PHY is now ready to do its
> > work. The state is changed, normally to PHY_UP which means that both
> > the MAC and the PHY are ready.
> > 
> > If the phy driver is using polling, when the next poll happens, the
> > state machine notices the PHY is now in PHY_UP, and kicks off
> > auto-negotiation, if needed.
> > 
> > If however, the PHY is using interrupts, there is no polling. The phy
> > is stuck in PHY_UP until the next interrupt comes along. And there is
> > no reason for the PHY to interrupt.
> > 
> > Have phy_start() schedule the state machine to run, which both speeds
> > up the polling use case, and makes the interrupt use case actually
> > work.
> > 
> > This problems exists whenever there is a state change which will not
> > cause an interrupt. Trigger the state machine in these cases,
> > e.g. phy_error().
> > 
> > Signed-off-by: Andrew Lunn 
> > Cc: Kyle Roeschley 
> > ---
> > 
> > This should be applied to stable, but i've no idea what fixes: tag to
> > use. It could be phylib has been broken since interrupts were added?
> 
> Since you think it should go to -stable, it is not appropriate to target
> this patch to 'net-next', only 'net' makes sense.

Hi David

Just for my clarification:

We are in the middle of the merge window. What does net-next and net
mean at the moment?

Outside of the merge window, net is patches being collected to go into
the next -rc. net-next is used to collect patches for the next merge
window.

During the merge window, i've seen you collect patches into net-next
and send additional pull requests to Linus. Which is why i based it on
net-next. I did not check, but i assumed net was still on v4.8.0,
waiting for -rc1 to come out. But this assumption seems untrue. During
the merge window does net equate to what Linus has already pulled from
net-next?

Thanks
Andrew

Re: [PATCH net-next] net: phy: Trigger state machine on state change and not polling.

[David Miller]

From: Andrew Lunn 
Date: Wed, 12 Oct 2016 22:14:53 +0200

> The phy_start() is used to indicate the PHY is now ready to do its
> work. The state is changed, normally to PHY_UP which means that both
> the MAC and the PHY are ready.
> 
> If the phy driver is using polling, when the next poll happens, the
> state machine notices the PHY is now in PHY_UP, and kicks off
> auto-negotiation, if needed.
> 
> If however, the PHY is using interrupts, there is no polling. The phy
> is stuck in PHY_UP until the next interrupt comes along. And there is
> no reason for the PHY to interrupt.
> 
> Have phy_start() schedule the state machine to run, which both speeds
> up the polling use case, and makes the interrupt use case actually
> work.
> 
> This problems exists whenever there is a state change which will not
> cause an interrupt. Trigger the state machine in these cases,
> e.g. phy_error().
> 
> Signed-off-by: Andrew Lunn 
> Cc: Kyle Roeschley 
> ---
> 
> This should be applied to stable, but i've no idea what fixes: tag to
> use. It could be phylib has been broken since interrupts were added?

Since you think it should go to -stable, it is not appropriate to target
this patch to 'net-next', only 'net' makes sense.

Therefore I applied this to 'net' and will queue it up for -stable.

Personally I think phylib was broken in this area since interrupts
were added.

Re: [PATCH net-next] net: phy: Trigger state machine on state change and not polling.

[Kyle Roeschley]

On Wed, Oct 12, 2016 at 10:14:53PM +0200, Andrew Lunn wrote:
> The phy_start() is used to indicate the PHY is now ready to do its
> work. The state is changed, normally to PHY_UP which means that both
> the MAC and the PHY are ready.
> 
> If the phy driver is using polling, when the next poll happens, the
> state machine notices the PHY is now in PHY_UP, and kicks off
> auto-negotiation, if needed.
> 
> If however, the PHY is using interrupts, there is no polling. The phy
> is stuck in PHY_UP until the next interrupt comes along. And there is
> no reason for the PHY to interrupt.
> 
> Have phy_start() schedule the state machine to run, which both speeds
> up the polling use case, and makes the interrupt use case actually
> work.
> 
> This problems exists whenever there is a state change which will not
> cause an interrupt. Trigger the state machine in these cases,
> e.g. phy_error().
> 
> Signed-off-by: Andrew Lunn 
> Cc: Kyle Roeschley 
> ---
> 
> This should be applied to stable, but i've no idea what fixes: tag to
> use. It could be phylib has been broken since interrupts were added?
> 

This patched fixed another problem had with the phy state machine which was
caused by d5c3d8465 ("net: phy: Avoid polling PHY with PHY_IGNORE_INTERRUPTS").
That might be the commit you're looking for. Also,

Tested-by: Kyle Roeschley 

>  drivers/net/phy/phy.c | 22 --
>  1 file changed, 20 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> index c6f66832a1a6..f424b867f73e 100644
> --- a/drivers/net/phy/phy.c
> +++ b/drivers/net/phy/phy.c
> @@ -608,6 +608,21 @@ void phy_start_machine(struct phy_device *phydev)
>  }
>  
>  /**
> + * phy_trigger_machine - trigger the state machine to run
> + *
> + * @phydev: the phy_device struct
> + *
> + * Description: There has been a change in state which requires that the
> + *   state machine runs.
> + */
> +

You've got a bonus newline here.

> +static void phy_trigger_machine(struct phy_device *phydev)
> +{
> + cancel_delayed_work_sync(>state_queue);
> + queue_delayed_work(system_power_efficient_wq, >state_queue, 0);
> +}
> +
> +/**
>   * phy_stop_machine - stop the PHY state machine tracking
>   * @phydev: target phy_device struct
>   *
> @@ -639,6 +654,8 @@ static void phy_error(struct phy_device *phydev)
>   mutex_lock(>lock);
>   phydev->state = PHY_HALTED;
>   mutex_unlock(>lock);
> +
> + phy_trigger_machine(phydev);
>  }
>  
>  /**
> @@ -800,8 +817,7 @@ void phy_change(struct work_struct *work)
>   }
>  
>   /* reschedule state queue work to run as soon as possible */
> - cancel_delayed_work_sync(>state_queue);
> - queue_delayed_work(system_power_efficient_wq, >state_queue, 0);
> + phy_trigger_machine(phydev);
>   return;
>  
>  ignore:
> @@ -890,6 +906,8 @@ void phy_start(struct phy_device *phydev)
>   /* if phy was suspended, bring the physical link up again */
>   if (do_resume)
>   phy_resume(phydev);
> +
> + phy_trigger_machine(phydev);
>  }
>  EXPORT_SYMBOL(phy_start);
>  
> -- 
> 2.9.3
> 

-- 
Kyle Roeschley
Software Engineer
National Instruments

[PATCH net-next] net: phy: Trigger state machine on state change and not polling.

[Andrew Lunn]

The phy_start() is used to indicate the PHY is now ready to do its
work. The state is changed, normally to PHY_UP which means that both
the MAC and the PHY are ready.

If the phy driver is using polling, when the next poll happens, the
state machine notices the PHY is now in PHY_UP, and kicks off
auto-negotiation, if needed.

If however, the PHY is using interrupts, there is no polling. The phy
is stuck in PHY_UP until the next interrupt comes along. And there is
no reason for the PHY to interrupt.

Have phy_start() schedule the state machine to run, which both speeds
up the polling use case, and makes the interrupt use case actually
work.

This problems exists whenever there is a state change which will not
cause an interrupt. Trigger the state machine in these cases,
e.g. phy_error().

Signed-off-by: Andrew Lunn 
Cc: Kyle Roeschley 
---

This should be applied to stable, but i've no idea what fixes: tag to
use. It could be phylib has been broken since interrupts were added?

 drivers/net/phy/phy.c | 22 --
 1 file changed, 20 insertions(+), 2 deletions(-)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index c6f66832a1a6..f424b867f73e 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -608,6 +608,21 @@ void phy_start_machine(struct phy_device *phydev)
 }
 
 /**
+ * phy_trigger_machine - trigger the state machine to run
+ *
+ * @phydev: the phy_device struct
+ *
+ * Description: There has been a change in state which requires that the
+ *   state machine runs.
+ */
+
+static void phy_trigger_machine(struct phy_device *phydev)
+{
+   cancel_delayed_work_sync(>state_queue);
+   queue_delayed_work(system_power_efficient_wq, >state_queue, 0);
+}
+
+/**
  * phy_stop_machine - stop the PHY state machine tracking
  * @phydev: target phy_device struct
  *
@@ -639,6 +654,8 @@ static void phy_error(struct phy_device *phydev)
mutex_lock(>lock);
phydev->state = PHY_HALTED;
mutex_unlock(>lock);
+
+   phy_trigger_machine(phydev);
 }
 
 /**
@@ -800,8 +817,7 @@ void phy_change(struct work_struct *work)
}
 
/* reschedule state queue work to run as soon as possible */
-   cancel_delayed_work_sync(>state_queue);
-   queue_delayed_work(system_power_efficient_wq, >state_queue, 0);
+   phy_trigger_machine(phydev);
return;
 
 ignore:
@@ -890,6 +906,8 @@ void phy_start(struct phy_device *phydev)
/* if phy was suspended, bring the physical link up again */
if (do_resume)
phy_resume(phydev);
+
+   phy_trigger_machine(phydev);
 }
 EXPORT_SYMBOL(phy_start);
 
-- 
2.9.3