From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753249AbbFJFSI (ORCPT ); Wed, 10 Jun 2015 01:18:08 -0400 Received: from mail-oi0-f53.google.com ([209.85.218.53]:34051 "EHLO mail-oi0-f53.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751431AbbFJFSB (ORCPT ); Wed, 10 Jun 2015 01:18:01 -0400 Message-ID: <5577C887.2030604@gmail.com> Date: Tue, 09 Jun 2015 22:17:59 -0700 From: Florian Fainelli User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.7.0 MIME-Version: 1.0 To: Keng Soon Cheah , netdev@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH RFC] net: phy: Introduced the PHY_AN_PENDING state References: <20150610043635.GA6430@pendev.apac.corp.natinst.com> In-Reply-To: <20150610043635.GA6430@pendev.apac.corp.natinst.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Le 06/09/15 21:36, Keng Soon Cheah a écrit : > The PHY_AN_PENDING state is put as a gate to enter the PHY_AN state > where it will wait for any uncomplete auto-negotiation session to > finish before starting a new one. > > This extra state could be used to workaround some auto-negotation > issues from certain vendors. The typical way to work around these problems are to fix them at the PHY driver level, see below. > > an_pending_timeout module parameter is used to enable the AN_PENDING > transition state. Set it to 0 to disable AN_PENDING state transition, > set it to any non-zero value to specify the timeout period for > PHY_AN_PENDING state in second. The default value is 0. > > an_pending_guard module parameter serves as a guard band to delay > the auto-negotiation firing after the previous auto-negotiation > finish. > > Signed-off-by: Keng Soon Cheah > > Conflicts: > > drivers/net/phy/phy.c > --- > We observed failure in the ethernet link operation when our board pairs > with some network switch model. The problem happens when an > auto-negotiation is started around the time the previous auto-negotiation > complete. We believe this might be an interoperatibility issue between > the PHYs but we need a short-term solution in software to workaround the > issue. > > We found that we are able to avoid from hitting the problem by waiting any > pending auto-negotiation to complete before starting a new one and this > patch is designed to serve the purpose. That sounds like a bug in the PHY state machine and/or the PHY driver if you are allowed to restart auto-negotiation while one is pending. Now that the PHY state machine has debug prints built-in, could you capture a trace of this failing case? Is this observed with the generic PHY driver or a custom PHY driver? > > A PHY_AN_PENDING state is introduced and it will act as a gate to enter > the PHY_AN state. This state will check for auto-negotiation completion > or timeout after an_pending_timeout period, then it will wait for > an_pending_guard before triggering another auto-negotiation. > > The following diagram shows the timing diagram > > > an_pending_timeout an_pending_guard > V V auto-nego > |--------------------------------->|....................| > ^ > auto-negotiation complete/timeout > > We do not have plan to submit this patch upstream (unless the community > feels this patch is useful in general) but we would like to seek for > feedback or advice if this patch could introduce new problems. As usual with state machines, introducing a new state needs to be carefully done in order to make sure that all transitions are correct, so far I would rather work on finding the root cause/extending the timeout and/or making it configurable on a PHY-driver basis rather than having this additional state which is more error prone. Thanks! > > --- > drivers/net/phy/phy.c | 44 +++++++++++++++++++++++++++++++++++++++++++- > include/linux/phy.h | 3 ++- > 2 files changed, 45 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c > index b2197b5..35e6484 100644 > --- a/drivers/net/phy/phy.c > +++ b/drivers/net/phy/phy.c > @@ -38,6 +38,16 @@ > > #include > > +static unsigned int an_pending_timeout; > +module_param(an_pending_timeout, uint, 0644); > +MODULE_PARM_DESC(an_pending_timeout, > + "Timeout period for PHY_AN_PENDING state in second. 0 to disable PHY_AN_PENDING state (default)"); > + > +static unsigned int an_pending_guard; > +module_param(an_pending_guard, uint, 0644); > +MODULE_PARM_DESC(an_pending_guard, > + "Guard band period before firing auto-negotiation from PHY_AN_PENDING state in second. Default to 0"); > + > static const char *phy_speed_to_str(int speed) > { > switch (speed) { > @@ -82,7 +92,6 @@ static const char *phy_state_to_str(enum phy_state st) > return NULL; > } > > - > /** > * phy_print_status - Convenience function to print out the current phy status > * @phydev: the phy_device struct > @@ -485,6 +494,18 @@ int phy_start_aneg(struct phy_device *phydev) > > /* Invalidate LP advertising flags */ > phydev->lp_advertising = 0; > + if (an_pending_timeout) { > + switch (phydev->state) { > + case PHY_AN_PENDING: > + case PHY_HALTED: > + break; > + default: > + phydev->state = PHY_AN_PENDING; > + phydev->link_timeout = an_pending_timeout; > + goto out_unlock; > + } > + > + } > > err = phydev->drv->config_aneg(phydev); > if (err < 0) > @@ -831,6 +852,27 @@ void phy_state_machine(struct work_struct *work) > phydev->link_timeout = PHY_AN_TIMEOUT; > > break; > + case PHY_AN_PENDING: > + /* Check if negotiation is done. Break if there's an error */ > + err = phy_aneg_done(phydev); > + if (err < 0) > + break; > + > + /* If AN is done, we'll proceed with the real aneg triggering */ > + if (err > 0) { > + if (phydev->link_timeout > 0) > + phydev->link_timeout = -(an_pending_guard); > + else if (phydev->link_timeout < 0) > + phydev->link_timeout++; > + } else > + phydev->link_timeout--; > + > + if (0 == phydev->link_timeout) { > + needs_aneg = true; > + > + phydev->link_timeout = PHY_AN_TIMEOUT; > + } > + break; > case PHY_AN: > err = phy_read_status(phydev); > if (err < 0) > diff --git a/include/linux/phy.h b/include/linux/phy.h > index a26c3f8..a63afdc 100644 > --- a/include/linux/phy.h > +++ b/include/linux/phy.h > @@ -308,7 +308,8 @@ enum phy_state { > PHY_FORCING, > PHY_CHANGELINK, > PHY_HALTED, > - PHY_RESUMING > + PHY_RESUMING, > + PHY_AN_PENDING > }; > > /** > -- Florian