[PATCH net] net: phy: fix duplex out of sync problem while changing settings

[PATCH net] net: phy: fix duplex out of sync problem while changing settings

[Heiner Kallweit]

As reported by Zhang there's a small issue if in forced mode the duplex
mode changes with the link staying up [0]. In this case the MAC isn't
notified about the change.

The proposed patch relies on the phylib state machine and ignores the
fact that there are drivers that uses phylib but not the phylib state
machine. So let's don't change the behavior for such drivers and fix
it w/o re-adding state PHY_FORCING for the case that phylib state
machine is used.

[0] https://lore.kernel.org/netdev/a5c26ffd-4ee4-a5e6-4103-873208ce0dc5@huawei.com/T/

Fixes: 2bd229df5e2e ("net: phy: remove state PHY_FORCING")
Reported-by: Zhang Changzhong <zhangchangzhong@huawei.com>
Tested-by: Zhang Changzhong <zhangchangzhong@huawei.com>
Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/net/phy/phy.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index a3bfb156c..beb2b66da 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -815,7 +815,12 @@ int phy_ethtool_ksettings_set(struct phy_device *phydev,
 	phydev->mdix_ctrl = cmd->base.eth_tp_mdix_ctrl;
 
 	/* Restart the PHY */
-	_phy_start_aneg(phydev);
+	if (phy_is_started(phydev)) {
+		phydev->state = PHY_UP;
+		phy_trigger_machine(phydev);
+	} else {
+		_phy_start_aneg(phydev);
+	}
 
 	mutex_unlock(&phydev->lock);
 	return 0;
-- 
2.33.1

Re: [PATCH net] net: phy: fix duplex out of sync problem while changing settings

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (master)
by Jakub Kicinski <kuba@kernel.org>:

On Wed, 3 Nov 2021 22:08:28 +0100 you wrote:
> As reported by Zhang there's a small issue if in forced mode the duplex
> mode changes with the link staying up [0]. In this case the MAC isn't
> notified about the change.
> 
> The proposed patch relies on the phylib state machine and ignores the
> fact that there are drivers that uses phylib but not the phylib state
> machine. So let's don't change the behavior for such drivers and fix
> it w/o re-adding state PHY_FORCING for the case that phylib state
> machine is used.
> 
> [...]

Here is the summary with links:
  - [net] net: phy: fix duplex out of sync problem while changing settings
    https://git.kernel.org/netdev/net/c/a4db9055fdb9

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

[PATCH net] net: phy: fix duplex out of sync problem while changing settings

[Zhang Changzhong]

Before commit 2bd229df5e2e ("net: phy: remove state PHY_FORCING") if
phy_ethtool_ksettings_set() is called with autoneg off, phy_start_aneg()
will set phydev->state to PHY_FORCING, so adjust_link will always be
called.

But after that commit, if phy_ethtool_ksettings_set() changes the local
link from 10Mbps/Half to 10Mbps/Full when the link partner is
10Mbps/Full, phy_check_link_status() will not trigger the link change,
because phydev->link and phydev->state has not changed. This will causes
the duplex of the PHY and MAC to be out of sync.

Fix it by re-adding the PHY_FORCING state to force adjust_link to be
called in fixed mode.

Fixes: 2bd229df5e2e ("net: phy: remove state PHY_FORCING")
Signed-off-by: Zhang Changzhong <zhangchangzhong@huawei.com>
---
 drivers/net/phy/phy.c | 10 ++++++++--
 include/linux/phy.h   |  6 ++++++
 2 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index a3bfb15..b114f15 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -49,6 +49,7 @@ static const char *phy_state_to_str(enum phy_state st)
 	PHY_STATE_STR(UP)
 	PHY_STATE_STR(RUNNING)
 	PHY_STATE_STR(NOLINK)
+	PHY_STATE_STR(FORCING)
 	PHY_STATE_STR(CABLETEST)
 	PHY_STATE_STR(HALTED)
 	}
@@ -724,8 +725,12 @@ static int _phy_start_aneg(struct phy_device *phydev)
 	if (err < 0)
 		return err;
 
-	if (phy_is_started(phydev))
-		err = phy_check_link_status(phydev);
+	if (phy_is_started(phydev)) {
+		if (phydev->autoneg == AUTONEG_ENABLE)
+			err = phy_check_link_status(phydev);
+		else
+			phydev->state = PHY_FORCING;
+	}
 
 	return err;
 }
@@ -1120,6 +1125,7 @@ void phy_state_machine(struct work_struct *work)
 		needs_aneg = true;
 
 		break;
+	case PHY_FORCING:
 	case PHY_NOLINK:
 	case PHY_RUNNING:
 		err = phy_check_link_status(phydev);
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 736e1d1..e639729 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -446,6 +446,11 @@ struct phy_device *mdiobus_scan(struct mii_bus *bus, int addr);
  * - irq or timer will set @PHY_NOLINK if link goes down
  * - phy_stop moves to @PHY_HALTED
  *
+ * @PHY_FORCING: PHY is being configured with forced settings.
+ * - if link is up, move to @PHY_RUNNING
+ * - if link is down, move to @PHY_NOLINK
+ * - phy_stop moves to @PHY_HALTED
+ *
  * @PHY_CABLETEST: PHY is performing a cable test. Packet reception/sending
  * is not expected to work, carrier will be indicated as down. PHY will be
  * poll once per second, or on interrupt for it current state.
@@ -463,6 +468,7 @@ enum phy_state {
 	PHY_UP,
 	PHY_RUNNING,
 	PHY_NOLINK,
+	PHY_FORCING,
 	PHY_CABLETEST,
 };
 
-- 
2.9.5

Re: [PATCH net] net: phy: fix duplex out of sync problem while changing settings

[Heiner Kallweit]

On 01.11.2021 10:44, Zhang Changzhong wrote:
> Before commit 2bd229df5e2e ("net: phy: remove state PHY_FORCING") if
> phy_ethtool_ksettings_set() is called with autoneg off, phy_start_aneg()
> will set phydev->state to PHY_FORCING, so adjust_link will always be
> called.
> 
> But after that commit, if phy_ethtool_ksettings_set() changes the local
> link from 10Mbps/Half to 10Mbps/Full when the link partner is
> 10Mbps/Full, phy_check_link_status() will not trigger the link change,
> because phydev->link and phydev->state has not changed. This will causes
> the duplex of the PHY and MAC to be out of sync.
> 
> Fix it by re-adding the PHY_FORCING state to force adjust_link to be
> called in fixed mode.
> 
> Fixes: 2bd229df5e2e ("net: phy: remove state PHY_FORCING")
> Signed-off-by: Zhang Changzhong <zhangchangzhong@huawei.com>
> ---
>  drivers/net/phy/phy.c | 10 ++++++++--
>  include/linux/phy.h   |  6 ++++++
>  2 files changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> index a3bfb15..b114f15 100644
> --- a/drivers/net/phy/phy.c
> +++ b/drivers/net/phy/phy.c
> @@ -49,6 +49,7 @@ static const char *phy_state_to_str(enum phy_state st)
>  	PHY_STATE_STR(UP)
>  	PHY_STATE_STR(RUNNING)
>  	PHY_STATE_STR(NOLINK)
> +	PHY_STATE_STR(FORCING)
>  	PHY_STATE_STR(CABLETEST)
>  	PHY_STATE_STR(HALTED)
>  	}
> @@ -724,8 +725,12 @@ static int _phy_start_aneg(struct phy_device *phydev)
>  	if (err < 0)
>  		return err;
>  
> -	if (phy_is_started(phydev))
> -		err = phy_check_link_status(phydev);
> +	if (phy_is_started(phydev)) {
> +		if (phydev->autoneg == AUTONEG_ENABLE)
> +			err = phy_check_link_status(phydev);
> +		else
> +			phydev->state = PHY_FORCING;
> +	}
>  
>  	return err;
>  }
> @@ -1120,6 +1125,7 @@ void phy_state_machine(struct work_struct *work)
>  		needs_aneg = true;
>  
>  		break;
> +	case PHY_FORCING:
>  	case PHY_NOLINK:
>  	case PHY_RUNNING:
>  		err = phy_check_link_status(phydev);
> diff --git a/include/linux/phy.h b/include/linux/phy.h
> index 736e1d1..e639729 100644
> --- a/include/linux/phy.h
> +++ b/include/linux/phy.h
> @@ -446,6 +446,11 @@ struct phy_device *mdiobus_scan(struct mii_bus *bus, int addr);
>   * - irq or timer will set @PHY_NOLINK if link goes down
>   * - phy_stop moves to @PHY_HALTED
>   *
> + * @PHY_FORCING: PHY is being configured with forced settings.
> + * - if link is up, move to @PHY_RUNNING
> + * - if link is down, move to @PHY_NOLINK
> + * - phy_stop moves to @PHY_HALTED
> + *
>   * @PHY_CABLETEST: PHY is performing a cable test. Packet reception/sending
>   * is not expected to work, carrier will be indicated as down. PHY will be
>   * poll once per second, or on interrupt for it current state.
> @@ -463,6 +468,7 @@ enum phy_state {
>  	PHY_UP,
>  	PHY_RUNNING,
>  	PHY_NOLINK,
> +	PHY_FORCING,
>  	PHY_CABLETEST,
>  };
>  
> 

I see your point, but the proposed fix may not be fully correct. You rely
on the phylib state machine, but there are drivers that use
phy_ethtool_ksettings_set() and some parts of phylib but not the phylib
state machine. One example is AMD xgbe.

Maybe the following is better, could you please give it a try?
By the way: With which MAC driver did you encounter the issue?


diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index a3bfb156c..beb2b66da 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -815,7 +815,12 @@ int phy_ethtool_ksettings_set(struct phy_device *phydev,
 	phydev->mdix_ctrl = cmd->base.eth_tp_mdix_ctrl;
 
 	/* Restart the PHY */
-	_phy_start_aneg(phydev);
+	if (phy_is_started(phydev)) {
+		phydev->state = PHY_UP;
+		phy_trigger_machine(phydev);
+	} else {
+		_phy_start_aneg(phydev);
+	}
 
 	mutex_unlock(&phydev->lock);
 	return 0;
-- 
2.33.1

Re: [PATCH net] net: phy: fix duplex out of sync problem while changing settings

[Zhang Changzhong]

On 2021/11/2 5:37, Heiner Kallweit wrote:
> On 01.11.2021 10:44, Zhang Changzhong wrote:
>> Before commit 2bd229df5e2e ("net: phy: remove state PHY_FORCING") if
>> phy_ethtool_ksettings_set() is called with autoneg off, phy_start_aneg()
>> will set phydev->state to PHY_FORCING, so adjust_link will always be
>> called.
>>
>> But after that commit, if phy_ethtool_ksettings_set() changes the local
>> link from 10Mbps/Half to 10Mbps/Full when the link partner is
>> 10Mbps/Full, phy_check_link_status() will not trigger the link change,
>> because phydev->link and phydev->state has not changed. This will causes
>> the duplex of the PHY and MAC to be out of sync.
>>
>> Fix it by re-adding the PHY_FORCING state to force adjust_link to be
>> called in fixed mode.
>>
>> Fixes: 2bd229df5e2e ("net: phy: remove state PHY_FORCING")
>> Signed-off-by: Zhang Changzhong <zhangchangzhong@huawei.com>
>> ---
>>  drivers/net/phy/phy.c | 10 ++++++++--
>>  include/linux/phy.h   |  6 ++++++
>>  2 files changed, 14 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
>> index a3bfb15..b114f15 100644
>> --- a/drivers/net/phy/phy.c
>> +++ b/drivers/net/phy/phy.c
>> @@ -49,6 +49,7 @@ static const char *phy_state_to_str(enum phy_state st)
>>  	PHY_STATE_STR(UP)
>>  	PHY_STATE_STR(RUNNING)
>>  	PHY_STATE_STR(NOLINK)
>> +	PHY_STATE_STR(FORCING)
>>  	PHY_STATE_STR(CABLETEST)
>>  	PHY_STATE_STR(HALTED)
>>  	}
>> @@ -724,8 +725,12 @@ static int _phy_start_aneg(struct phy_device *phydev)
>>  	if (err < 0)
>>  		return err;
>>  
>> -	if (phy_is_started(phydev))
>> -		err = phy_check_link_status(phydev);
>> +	if (phy_is_started(phydev)) {
>> +		if (phydev->autoneg == AUTONEG_ENABLE)
>> +			err = phy_check_link_status(phydev);
>> +		else
>> +			phydev->state = PHY_FORCING;
>> +	}
>>  
>>  	return err;
>>  }
>> @@ -1120,6 +1125,7 @@ void phy_state_machine(struct work_struct *work)
>>  		needs_aneg = true;
>>  
>>  		break;
>> +	case PHY_FORCING:
>>  	case PHY_NOLINK:
>>  	case PHY_RUNNING:
>>  		err = phy_check_link_status(phydev);
>> diff --git a/include/linux/phy.h b/include/linux/phy.h
>> index 736e1d1..e639729 100644
>> --- a/include/linux/phy.h
>> +++ b/include/linux/phy.h
>> @@ -446,6 +446,11 @@ struct phy_device *mdiobus_scan(struct mii_bus *bus, int addr);
>>   * - irq or timer will set @PHY_NOLINK if link goes down
>>   * - phy_stop moves to @PHY_HALTED
>>   *
>> + * @PHY_FORCING: PHY is being configured with forced settings.
>> + * - if link is up, move to @PHY_RUNNING
>> + * - if link is down, move to @PHY_NOLINK
>> + * - phy_stop moves to @PHY_HALTED
>> + *
>>   * @PHY_CABLETEST: PHY is performing a cable test. Packet reception/sending
>>   * is not expected to work, carrier will be indicated as down. PHY will be
>>   * poll once per second, or on interrupt for it current state.
>> @@ -463,6 +468,7 @@ enum phy_state {
>>  	PHY_UP,
>>  	PHY_RUNNING,
>>  	PHY_NOLINK,
>> +	PHY_FORCING,
>>  	PHY_CABLETEST,
>>  };
>>  
>>
> 
> I see your point, but the proposed fix may not be fully correct. You rely
> on the phylib state machine, but there are drivers that use
> phy_ethtool_ksettings_set() and some parts of phylib but not the phylib
> state machine. One example is AMD xgbe.

Yes, I notice that, thanks for remind!

> 
> Maybe the following is better, could you please give it a try?
> By the way: With which MAC driver did you encounter the issue?

We encounter the issue with the driver for our in-house MAC.

> 
> 
> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> index a3bfb156c..beb2b66da 100644
> --- a/drivers/net/phy/phy.c
> +++ b/drivers/net/phy/phy.c
> @@ -815,7 +815,12 @@ int phy_ethtool_ksettings_set(struct phy_device *phydev,
>  	phydev->mdix_ctrl = cmd->base.eth_tp_mdix_ctrl;
>  
>  	/* Restart the PHY */
> -	_phy_start_aneg(phydev);
> +	if (phy_is_started(phydev)) {
> +		phydev->state = PHY_UP;
> +		phy_trigger_machine(phydev);
> +	} else {
> +		_phy_start_aneg(phydev);
> +	}
>  
>  	mutex_unlock(&phydev->lock);
>  	return 0;
> 

We tested it and it works fine, thanks!