| Message ID | 20211027121149.45897-2-huangguangbin2@huawei.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 3bda2e5df476417b6d08967e2d84234a59d57b1c |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: hns3: add some fixes for -net | expand |
| Context | Check | Description |
|---|---|---|
| netdev/apply | success | Patch already applied to net |
| netdev/tree_selection | success | Clearly marked for net |
On Wed, Oct 27, 2021 at 08:11:43PM +0800, Guangbin Huang wrote: The semantics are not too well defined here, the ethtool documentation is not too clear. Here is how i interpret it. > If a TP port is configured by follow steps: > 1.ethtool -s ethx autoneg off speed 100 duplex full So you turn general autoneg off > 2.ethtool -A ethx rx on tx on You did not use autoneg off here. Pause autoneg is separate to general autoneg. So pause autoneg is still enabled at this point. That means you should not directly configure the MAC with the pause configuration, you only do that when pause autoneg is off. You can consider this as setting how you want pause to be negotiated once general autoneg is re-enabled. > 3.ethtool -s ethx autoneg on(rx&tx negotiated pause results are off) So you reenable general autoneg. As part of that general autoneg, pause will re-renegotiated, and it should you the preferences you set in 2, that rx and tx pause can be used. What is actually used depends on the link peer. The link_adjust callback from phylib tells you how to program the MAC. > 4.ethtool -s ethx autoneg off speed 100 duplex full So you turn general autoneg off again. It is unclear how you are supposed to program the MAC, but i guess most systems keep with the result from the last autoneg. Looking at your patch, there are suspicious calls to phy_syspend and phy_resume. They don't look correct at all, and i'm not aware of any other MAC driver doing this. Now, i know the behaviour is not well defined here, but i'm not sure your interpretation is valid and how others interpret it. Andrew
On 2021/10/28 1:23, Andrew Lunn wrote: > On Wed, Oct 27, 2021 at 08:11:43PM +0800, Guangbin Huang wrote: > > The semantics are not too well defined here, the ethtool documentation > is not too clear. Here is how i interpret it. > >> If a TP port is configured by follow steps: >> 1.ethtool -s ethx autoneg off speed 100 duplex full > > So you turn general autoneg off > >> 2.ethtool -A ethx rx on tx on > > You did not use autoneg off here. Pause autoneg is separate to general > autoneg. So pause autoneg is still enabled at this point. That means > you should not directly configure the MAC with the pause > configuration, you only do that when pause autoneg is off. You can > consider this as setting how you want pause to be negotiated once > general autoneg is re-enabled. > >> 3.ethtool -s ethx autoneg on(rx&tx negotiated pause results are off) > > So you reenable general autoneg. As part of that general autoneg, > pause will re-renegotiated, and it should you the preferences you set > in 2, that rx and tx pause can be used. What is actually used depends > on the link peer. The link_adjust callback from phylib tells you how > to program the MAC. > >> 4.ethtool -s ethx autoneg off speed 100 duplex full > > So you turn general autoneg off again. It is unclear how you are > supposed to program the MAC, but i guess most systems keep with the > result from the last autoneg. > > Looking at your patch, there are suspicious calls to phy_syspend and > phy_resume. They don't look correct at all, and i'm not aware of any > other MAC driver doing this. Now, i know the behaviour is not well > defined here, but i'm not sure your interpretation is valid and how > others interpret it. > > Andrew > . > Hi Andrew, thanks very much for your guidance on how to use pause autoneg, it confuses me before because PHY registers actually have no separate setting bit of pause autoneg. So, summarize what you mean: 1. If pause autoneg is on, driver should always use the autoneg result to program the MAC. Eventhough general autoneg is off now and link state is no changed then driver just needs to keep the last configuration for the MAC, if link state is changed and phy goes down and up then driver needs to program the MAC according to the autoneg result in the link_adjust callback. 2. If pause autoneg is off, driver should directly configure the MAC with tx pause and rx pause. Eventhough general autoneg is on, driver should ignore the autoneg result. Do I understand right? Guangbin .
> Hi Andrew, thanks very much for your guidance on how to use pause autoneg, > it confuses me before because PHY registers actually have no separate setting > bit of pause autoneg. > > So, summarize what you mean: > 1. If pause autoneg is on, driver should always use the autoneg result to program > the MAC. Eventhough general autoneg is off now and link state is no changed then > driver just needs to keep the last configuration for the MAC, if link state is > changed and phy goes down and up then driver needs to program the MAC according > to the autoneg result in the link_adjust callback. > 2. If pause autoneg is off, driver should directly configure the MAC with tx pause > and rx pause. Eventhough general autoneg is on, driver should ignore the autoneg > result. > > Do I understand right? Yes, that fits my understanding of ethtool, etc. phylink tried to clear up some of these problems by fully implementing the call within phylink. All the MAC driver needs to provide is a method to configure the MAC pause settings. Take a look at phylink_ethtool_set_pauseparam() and the commit messages related to that. Andrew
On 2021/10/28 20:30, Andrew Lunn wrote: >> Hi Andrew, thanks very much for your guidance on how to use pause autoneg, >> it confuses me before because PHY registers actually have no separate setting >> bit of pause autoneg. >> >> So, summarize what you mean: >> 1. If pause autoneg is on, driver should always use the autoneg result to program >> the MAC. Eventhough general autoneg is off now and link state is no changed then >> driver just needs to keep the last configuration for the MAC, if link state is >> changed and phy goes down and up then driver needs to program the MAC according >> to the autoneg result in the link_adjust callback. >> 2. If pause autoneg is off, driver should directly configure the MAC with tx pause >> and rx pause. Eventhough general autoneg is on, driver should ignore the autoneg >> result. >> >> Do I understand right? > > Yes, that fits my understanding of ethtool, etc. > > phylink tried to clear up some of these problems by fully implementing > the call within phylink. All the MAC driver needs to provide is a > method to configure the MAC pause settings. Take a look at > phylink_ethtool_set_pauseparam() and the commit messages related to > that. > > Andrew > . > Ok, thanks!
diff --git a/drivers/net/ethernet/hisilicon/hns3/hnae3.h b/drivers/net/ethernet/hisilicon/hns3/hnae3.h index d701451596c8..da3a593f6a56 100644 --- a/drivers/net/ethernet/hisilicon/hns3/hnae3.h +++ b/drivers/net/ethernet/hisilicon/hns3/hnae3.h @@ -568,6 +568,7 @@ struct hnae3_ae_ops { u32 *auto_neg, u32 *rx_en, u32 *tx_en); int (*set_pauseparam)(struct hnae3_handle *handle, u32 auto_neg, u32 rx_en, u32 tx_en); + int (*restore_pauseparam)(struct hnae3_handle *handle); int (*set_autoneg)(struct hnae3_handle *handle, bool enable); int (*get_autoneg)(struct hnae3_handle *handle); diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c b/drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c index 5ebd96f6833d..7d92dd273ed7 100644 --- a/drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c +++ b/drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c @@ -824,6 +824,26 @@ static int hns3_check_ksettings_param(const struct net_device *netdev, return 0; } +static int hns3_set_phy_link_ksettings(struct net_device *netdev, + const struct ethtool_link_ksettings *cmd) +{ + struct hnae3_handle *handle = hns3_get_handle(netdev); + const struct hnae3_ae_ops *ops = handle->ae_algo->ops; + int ret; + + if (cmd->base.speed == SPEED_1000 && + cmd->base.autoneg == AUTONEG_DISABLE) + return -EINVAL; + + if (cmd->base.autoneg == AUTONEG_DISABLE && ops->restore_pauseparam) { + ret = ops->restore_pauseparam(handle); + if (ret) + return ret; + } + + return phy_ethtool_ksettings_set(netdev->phydev, cmd); +} + static int hns3_set_link_ksettings(struct net_device *netdev, const struct ethtool_link_ksettings *cmd) { @@ -842,16 +862,11 @@ static int hns3_set_link_ksettings(struct net_device *netdev, cmd->base.autoneg, cmd->base.speed, cmd->base.duplex); /* Only support ksettings_set for netdev with phy attached for now */ - if (netdev->phydev) { - if (cmd->base.speed == SPEED_1000 && - cmd->base.autoneg == AUTONEG_DISABLE) - return -EINVAL; - - return phy_ethtool_ksettings_set(netdev->phydev, cmd); - } else if (test_bit(HNAE3_DEV_SUPPORT_PHY_IMP_B, ae_dev->caps) && - ops->set_phy_link_ksettings) { + if (netdev->phydev) + return hns3_set_phy_link_ksettings(netdev, cmd); + else if (test_bit(HNAE3_DEV_SUPPORT_PHY_IMP_B, ae_dev->caps) && + ops->set_phy_link_ksettings) return ops->set_phy_link_ksettings(handle, cmd); - } if (ae_dev->dev_version < HNAE3_DEVICE_VERSION_V2) return -EOPNOTSUPP; diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c index dcd40cc73082..c6b9806c75a5 100644 --- a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c +++ b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c @@ -11021,6 +11021,35 @@ static int hclge_set_pauseparam(struct hnae3_handle *handle, u32 auto_neg, return -EOPNOTSUPP; } +static int hclge_restore_pauseparam(struct hnae3_handle *handle) +{ + struct hclge_vport *vport = hclge_get_vport(handle); + struct hclge_dev *hdev = vport->back; + u32 auto_neg, rx_pause, tx_pause; + int ret; + + hclge_get_pauseparam(handle, &auto_neg, &rx_pause, &tx_pause); + /* when autoneg is disabled, the pause setting of phy has no effect + * unless the link goes down. + */ + ret = phy_suspend(hdev->hw.mac.phydev); + if (ret) + return ret; + + phy_set_asym_pause(hdev->hw.mac.phydev, rx_pause, tx_pause); + + ret = phy_resume(hdev->hw.mac.phydev); + if (ret) + return ret; + + ret = hclge_mac_pause_setup_hw(hdev); + if (ret) + dev_err(&hdev->pdev->dev, + "restore pauseparam error, ret = %d.\n", ret); + + return ret; +} + static void hclge_get_ksettings_an_result(struct hnae3_handle *handle, u8 *auto_neg, u32 *speed, u8 *duplex) { @@ -12984,6 +13013,7 @@ static const struct hnae3_ae_ops hclge_ops = { .halt_autoneg = hclge_halt_autoneg, .get_pauseparam = hclge_get_pauseparam, .set_pauseparam = hclge_set_pauseparam, + .restore_pauseparam = hclge_restore_pauseparam, .set_mtu = hclge_set_mtu, .reset_queue = hclge_reset_tqp, .get_stats = hclge_get_stats, diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_tm.c b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_tm.c index 95074e91a846..124791e4bfee 100644 --- a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_tm.c +++ b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_tm.c @@ -1435,7 +1435,7 @@ static int hclge_bp_setup_hw(struct hclge_dev *hdev, u8 tc) return 0; } -static int hclge_mac_pause_setup_hw(struct hclge_dev *hdev) +int hclge_mac_pause_setup_hw(struct hclge_dev *hdev) { bool tx_en, rx_en; diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_tm.h b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_tm.h index 2ee9b795f71d..4b2c3a788980 100644 --- a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_tm.h +++ b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_tm.h @@ -244,6 +244,7 @@ int hclge_tm_get_pri_weight(struct hclge_dev *hdev, u8 pri_id, u8 *weight); int hclge_tm_get_pri_shaper(struct hclge_dev *hdev, u8 pri_id, enum hclge_opcode_type cmd, struct hclge_tm_shaper_para *para); +int hclge_mac_pause_setup_hw(struct hclge_dev *hdev); int hclge_tm_get_q_to_qs_map(struct hclge_dev *hdev, u16 q_id, u16 *qset_id); int hclge_tm_get_q_to_tc(struct hclge_dev *hdev, u16 q_id, u8 *tc_id); int hclge_tm_get_pg_to_pri_map(struct hclge_dev *hdev, u8 pg_id,
If a TP port is configured by follow steps: 1.ethtool -s ethx autoneg off speed 100 duplex full 2.ethtool -A ethx rx on tx on 3.ethtool -s ethx autoneg on(rx&tx negotiated pause results are off) 4.ethtool -s ethx autoneg off speed 100 duplex full In step 3, driver will set rx&tx pause parameters of hardware to off as pause parameters negotiated with link partner are off. After step 4, the "ethtool -a ethx" command shows both rx and tx pause parameters are on. However, pause parameters of hardware are still off and port has no flow control function actually. To fix this problem, if autoneg is disabled, driver uses its saved parameters to restore pause of hardware. If the speed is not changed in this case, there is no link state changed for phy, it will cause the pause parameter is not taken effect, so we need to force phy to go down and up. Fixes: aacbe27e82f0 ("net: hns3: modify how pause options is displayed") Signed-off-by: Guangbin Huang <huangguangbin2@huawei.com> --- drivers/net/ethernet/hisilicon/hns3/hnae3.h | 1 + .../ethernet/hisilicon/hns3/hns3_ethtool.c | 33 ++++++++++++++----- .../hisilicon/hns3/hns3pf/hclge_main.c | 30 +++++++++++++++++ .../ethernet/hisilicon/hns3/hns3pf/hclge_tm.c | 2 +- .../ethernet/hisilicon/hns3/hns3pf/hclge_tm.h | 1 + 5 files changed, 57 insertions(+), 10 deletions(-)