| Message ID | 20240426100045.1631295-6-shaojijie@huawei.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | There are some bugfix for the HNS3 ethernet driver | expand |
On Fri, Apr 26, 2024 at 06:00:43PM +0800, Jijie Shao wrote: > From: Peiyang Wang <wangpeiyang1@huawei.com> > > When a reset occurring, it's supposed to recover user's configuration. > Currently, the port info(speed, duplex and autoneg) is stored in hclge_mac > and will be scheduled updated. Consider the case that reset was happened > consecutively. During the first reset, the port info is configured with > a temporary value cause the PHY is reset and looking for best link config. > Second reset start and use pervious configuration which is not the user's. > The specific process is as follows: > > +------+ +----+ +----+ > | USER | | PF | | HW | > +---+--+ +-+--+ +-+--+ > | ethtool --reset | | > +------------------->| reset command | > | ethtool --reset +-------------------->| > +------------------->| +---+ > | +---+ | | > | | |reset currently | | HW RESET > | | |and wait to do | | > | |<--+ | | > | | send pervious cfg |<--+ > | | (1000M FULL AN_ON) | > | +-------------------->| > | | read cfg(time task) | > | | (10M HALF AN_OFF) +---+ > | |<--------------------+ | cfg take effect > | | reset command |<--+ > | +-------------------->| > | | +---+ > | | send pervious cfg | | HW RESET > | | (10M HALF AN_OFF) |<--+ > | +-------------------->| > | | read cfg(time task) | > | | (10M HALF AN_OFF) +---+ > | |<--------------------+ | cfg take effect > | | | | > | | read cfg(time task) |<--+ > | | (10M HALF AN_OFF) | > | |<--------------------+ > | | | > v v v > > To avoid aboved situation, this patch introduced req_speed, req_duplex, > req_autoneg to store user's configuration and it only be used after > hardware reset and to recover user's configuration Hi Peiyang Wang and Jijie Shao, In reviewing this patch it would be helpful if the diagram above could be related to driver code. I'm sure it is obvious enough, but I'm having a bit of trouble. F.e., is it hclge_tp_port_init() where "port info is configured with a temporary value cause the PHY is reset and looking for best link config." ? > > Fixes: f5f2b3e4dcc0 ("net: hns3: add support for imp-controlled PHYs") > Signed-off-by: Peiyang Wang <wangpeiyang1@huawei.com> > Signed-off-by: Jijie Shao <shaojijie@huawei.com> > --- > .../ethernet/hisilicon/hns3/hns3pf/hclge_main.c | 15 +++++++++------ > .../ethernet/hisilicon/hns3/hns3pf/hclge_main.h | 3 +++ > 2 files changed, 12 insertions(+), 6 deletions(-) > > diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c > index 6eda73f1e6ad..5dc8593c97be 100644 > --- a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c > +++ b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c > @@ -1537,6 +1537,9 @@ static int hclge_configure(struct hclge_dev *hdev) > cfg.default_speed, ret); > return ret; > } > + hdev->hw.mac.req_speed = hdev->hw.mac.speed; > + hdev->hw.mac.req_autoneg = AUTONEG_ENABLE; > + hdev->hw.mac.req_duplex = DUPLEX_FULL; I am curious to know why the initialisation of req_autoneg and req_duplex is not: hdev->hw.mac.req_autoneg = hdev->hw.mac.autoneg; hdev->hw.mac.req_duplex = hdev->hw.mac.duplex As it seems to me the value .autoneg is 0 (AUTONEG_DISABLE) and the value of .duplex is 0 (DUPLEX_HALF). > hclge_parse_link_mode(hdev, cfg.speed_ability); > > @@ -3344,9 +3347,9 @@ hclge_set_phy_link_ksettings(struct hnae3_handle *handle, > return ret; > } > > - hdev->hw.mac.autoneg = cmd->base.autoneg; > - hdev->hw.mac.speed = cmd->base.speed; > - hdev->hw.mac.duplex = cmd->base.duplex; > + hdev->hw.mac.req_autoneg = cmd->base.autoneg; > + hdev->hw.mac.req_speed = cmd->base.speed; > + hdev->hw.mac.req_duplex = cmd->base.duplex; > linkmode_copy(hdev->hw.mac.advertising, cmd->link_modes.advertising); > > return 0; > @@ -3364,9 +3367,9 @@ static int hclge_update_tp_port_info(struct hclge_dev *hdev) > if (ret) > return ret; > > - hdev->hw.mac.autoneg = cmd.base.autoneg; > - hdev->hw.mac.speed = cmd.base.speed; > - hdev->hw.mac.duplex = cmd.base.duplex; > + cmd.base.autoneg = hdev->hw.mac.req_autoneg; > + cmd.base.speed = hdev->hw.mac.req_speed; > + cmd.base.duplex = hdev->hw.mac.req_duplex; It is unclear to me why fields of cmd are set here, cmd is a local variable and they don't seem to be used for the rest of the function. > linkmode_copy(hdev->hw.mac.advertising, cmd.link_modes.advertising); > > return 0; ...
on 2024/4/26 22:25, Simon Horman wrote: > On Fri, Apr 26, 2024 at 06:00:43PM +0800, Jijie Shao wrote: >> From: Peiyang Wang <wangpeiyang1@huawei.com> >> >> When a reset occurring, it's supposed to recover user's configuration. >> Currently, the port info(speed, duplex and autoneg) is stored in hclge_mac >> and will be scheduled updated. Consider the case that reset was happened >> consecutively. During the first reset, the port info is configured with >> a temporary value cause the PHY is reset and looking for best link config. >> Second reset start and use pervious configuration which is not the user's. >> The specific process is as follows: >> >> +------+ +----+ +----+ >> | USER | | PF | | HW | >> +---+--+ +-+--+ +-+--+ >> | ethtool --reset | | >> +------------------->| reset command | >> | ethtool --reset +-------------------->| >> +------------------->| +---+ >> | +---+ | | >> | | |reset currently | | HW RESET >> | | |and wait to do | | >> | |<--+ | | >> | | send pervious cfg |<--+ >> | | (1000M FULL AN_ON) | >> | +-------------------->| >> | | read cfg(time task) | >> | | (10M HALF AN_OFF) +---+ >> | |<--------------------+ | cfg take effect >> | | reset command |<--+ >> | +-------------------->| >> | | +---+ >> | | send pervious cfg | | HW RESET >> | | (10M HALF AN_OFF) |<--+ >> | +-------------------->| >> | | read cfg(time task) | >> | | (10M HALF AN_OFF) +---+ >> | |<--------------------+ | cfg take effect >> | | | | >> | | read cfg(time task) |<--+ >> | | (10M HALF AN_OFF) | >> | |<--------------------+ >> | | | >> v v v >> >> To avoid aboved situation, this patch introduced req_speed, req_duplex, >> req_autoneg to store user's configuration and it only be used after >> hardware reset and to recover user's configuration > Hi Peiyang Wang and Jijie Shao, > > In reviewing this patch it would be helpful if the diagram above could be > related to driver code. I'm sure it is obvious enough, but I'm having a > bit of trouble. F.e., is it hclge_tp_port_init() where "port info is > configured with a temporary value cause the PHY is reset and looking for > best link config." ? Sorry, the description here is a bit confusing. driver periodically updates port info. If only one reset occurs, driver updates the port info based on the actual port info of the HW after the reset is complete. However, When two resets occur consecutively, If the port info of the HW is not updated in time before the second reset, The driver may query a temporary info instead of the actual port info. As a result, the actual port info is overwritten, Therefore, the port info restored by the driver after the second reset is incorrect. >> Fixes: f5f2b3e4dcc0 ("net: hns3: add support for imp-controlled PHYs") >> Signed-off-by: Peiyang Wang <wangpeiyang1@huawei.com> >> Signed-off-by: Jijie Shao <shaojijie@huawei.com> >> --- >> .../ethernet/hisilicon/hns3/hns3pf/hclge_main.c | 15 +++++++++------ >> .../ethernet/hisilicon/hns3/hns3pf/hclge_main.h | 3 +++ >> 2 files changed, 12 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c >> index 6eda73f1e6ad..5dc8593c97be 100644 >> --- a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c >> +++ b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c >> @@ -1537,6 +1537,9 @@ static int hclge_configure(struct hclge_dev *hdev) >> cfg.default_speed, ret); >> return ret; >> } >> + hdev->hw.mac.req_speed = hdev->hw.mac.speed; >> + hdev->hw.mac.req_autoneg = AUTONEG_ENABLE; >> + hdev->hw.mac.req_duplex = DUPLEX_FULL; > I am curious to know why the initialisation of req_autoneg and req_duplex > is not: > > hdev->hw.mac.req_autoneg = hdev->hw.mac.autoneg; > hdev->hw.mac.req_duplex = hdev->hw.mac.duplex > > As it seems to me the value .autoneg is 0 (AUTONEG_DISABLE) > and the value of .duplex is 0 (DUPLEX_HALF). Yes, the value .autoneg is 0 and the value of .duplex is 0 here. We hope that the network port is initialized in AUTONEG_ENABLE and DUPLEX_FULL, so req_autoneg and req_duplex are fixed. > >> hclge_parse_link_mode(hdev, cfg.speed_ability); >> >> @@ -3344,9 +3347,9 @@ hclge_set_phy_link_ksettings(struct hnae3_handle *handle, >> return ret; >> } >> >> - hdev->hw.mac.autoneg = cmd->base.autoneg; >> - hdev->hw.mac.speed = cmd->base.speed; >> - hdev->hw.mac.duplex = cmd->base.duplex; >> + hdev->hw.mac.req_autoneg = cmd->base.autoneg; >> + hdev->hw.mac.req_speed = cmd->base.speed; >> + hdev->hw.mac.req_duplex = cmd->base.duplex; >> linkmode_copy(hdev->hw.mac.advertising, cmd->link_modes.advertising); >> >> return 0; >> @@ -3364,9 +3367,9 @@ static int hclge_update_tp_port_info(struct hclge_dev *hdev) >> if (ret) >> return ret; >> >> - hdev->hw.mac.autoneg = cmd.base.autoneg; >> - hdev->hw.mac.speed = cmd.base.speed; >> - hdev->hw.mac.duplex = cmd.base.duplex; >> + cmd.base.autoneg = hdev->hw.mac.req_autoneg; >> + cmd.base.speed = hdev->hw.mac.req_speed; >> + cmd.base.duplex = hdev->hw.mac.req_duplex; > It is unclear to me why fields of cmd are set here, cmd is a local variable > and they don't seem to be used for the rest of the function. I'm so sorry, the code here is wrong. I will fix it in next version of this patch Thank you very much for your review.
diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c index 6eda73f1e6ad..5dc8593c97be 100644 --- a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c +++ b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c @@ -1537,6 +1537,9 @@ static int hclge_configure(struct hclge_dev *hdev) cfg.default_speed, ret); return ret; } + hdev->hw.mac.req_speed = hdev->hw.mac.speed; + hdev->hw.mac.req_autoneg = AUTONEG_ENABLE; + hdev->hw.mac.req_duplex = DUPLEX_FULL; hclge_parse_link_mode(hdev, cfg.speed_ability); @@ -3344,9 +3347,9 @@ hclge_set_phy_link_ksettings(struct hnae3_handle *handle, return ret; } - hdev->hw.mac.autoneg = cmd->base.autoneg; - hdev->hw.mac.speed = cmd->base.speed; - hdev->hw.mac.duplex = cmd->base.duplex; + hdev->hw.mac.req_autoneg = cmd->base.autoneg; + hdev->hw.mac.req_speed = cmd->base.speed; + hdev->hw.mac.req_duplex = cmd->base.duplex; linkmode_copy(hdev->hw.mac.advertising, cmd->link_modes.advertising); return 0; @@ -3364,9 +3367,9 @@ static int hclge_update_tp_port_info(struct hclge_dev *hdev) if (ret) return ret; - hdev->hw.mac.autoneg = cmd.base.autoneg; - hdev->hw.mac.speed = cmd.base.speed; - hdev->hw.mac.duplex = cmd.base.duplex; + cmd.base.autoneg = hdev->hw.mac.req_autoneg; + cmd.base.speed = hdev->hw.mac.req_speed; + cmd.base.duplex = hdev->hw.mac.req_duplex; linkmode_copy(hdev->hw.mac.advertising, cmd.link_modes.advertising); return 0; diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.h b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.h index 37527b847f2f..3a9186457ad8 100644 --- a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.h +++ b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.h @@ -279,11 +279,14 @@ struct hclge_mac { u8 media_type; /* port media type, e.g. fibre/copper/backplane */ u8 mac_addr[ETH_ALEN]; u8 autoneg; + u8 req_autoneg; u8 duplex; + u8 req_duplex; u8 support_autoneg; u8 speed_type; /* 0: sfp speed, 1: active speed */ u8 lane_num; u32 speed; + u32 req_speed; u32 max_speed; u32 speed_ability; /* speed ability supported by current media */ u32 module_type; /* sub media type, e.g. kr/cr/sr/lr */