| Message ID | 20240909023141.3234567-6-shaojijie@huawei.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Add support of HIBMCGE Ethernet Driver | expand |
On Mon, Sep 9, 2024 at 8:11 AM Jijie Shao <shaojijie@huawei.com> wrote: > > Implement the .ndo_open() .ndo_stop() .ndo_set_mac_address() > .ndo_change_mtu functions() and ndo.get_stats64() > And .ndo_validate_addr calls the eth_validate_addr function directly > > Signed-off-by: Jijie Shao <shaojijie@huawei.com> > --- > ChangeLog: > v6 -> v7: > - Add implement ndo.get_stats64(), suggested by Paolo. > v6: https://lore.kernel.org/all/20240830121604.2250904-6-shaojijie@huawei.com/ > v5 -> v6: > - Delete netif_carrier_off() in .ndo_open() and .ndo_stop(), > suggested by Jakub and Andrew. > v5: https://lore.kernel.org/all/20240827131455.2919051-1-shaojijie@huawei.com/ > v3 -> v4: > - Delete INITED_STATE in priv, suggested by Andrew. > - Delete unnecessary defensive code in hbg_phy_start() > and hbg_phy_stop(), suggested by Andrew. > v3: https://lore.kernel.org/all/20240822093334.1687011-1-shaojijie@huawei.com/ > RFC v1 -> RFC v2: > - Delete validation for mtu in hbg_net_change_mtu(), suggested by Andrew. > - Delete validation for mac address in hbg_net_set_mac_address(), > suggested by Andrew. > - Add a patch to add is_valid_ether_addr check in dev_set_mac_address, > suggested by Andrew. > RFC v1: https://lore.kernel.org/all/20240731094245.1967834-1-shaojijie@huawei.com/ > --- > .../ethernet/hisilicon/hibmcge/hbg_common.h | 3 + > .../net/ethernet/hisilicon/hibmcge/hbg_hw.c | 39 +++++++ > .../net/ethernet/hisilicon/hibmcge/hbg_hw.h | 3 + > .../net/ethernet/hisilicon/hibmcge/hbg_main.c | 104 ++++++++++++++++++ > .../net/ethernet/hisilicon/hibmcge/hbg_reg.h | 11 +- > 5 files changed, 159 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/ethernet/hisilicon/hibmcge/hbg_common.h b/drivers/net/ethernet/hisilicon/hibmcge/hbg_common.h > index e94ae2be5c4c..d11ef081f4da 100644 > --- a/drivers/net/ethernet/hisilicon/hibmcge/hbg_common.h > +++ b/drivers/net/ethernet/hisilicon/hibmcge/hbg_common.h > @@ -17,8 +17,11 @@ > > enum hbg_nic_state { > HBG_NIC_STATE_EVENT_HANDLING = 0, > + HBG_NIC_STATE_OPEN, > }; > > +#define hbg_nic_is_open(priv) test_bit(HBG_NIC_STATE_OPEN, &(priv)->state) > + > enum hbg_hw_event_type { > HBG_HW_EVENT_NONE = 0, > HBG_HW_EVENT_INIT, /* driver is loading */ > diff --git a/drivers/net/ethernet/hisilicon/hibmcge/hbg_hw.c b/drivers/net/ethernet/hisilicon/hibmcge/hbg_hw.c > index 8e971e9f62a0..97fee714155a 100644 > --- a/drivers/net/ethernet/hisilicon/hibmcge/hbg_hw.c > +++ b/drivers/net/ethernet/hisilicon/hibmcge/hbg_hw.c > @@ -15,6 +15,7 @@ > * ctrl means packet description, data means skb packet data > */ > #define HBG_ENDIAN_CTRL_LE_DATA_BE 0x0 > +#define HBG_PCU_FRAME_LEN_PLUS 4 > > static bool hbg_hw_spec_is_valid(struct hbg_priv *priv) > { > @@ -129,6 +130,44 @@ void hbg_hw_irq_enable(struct hbg_priv *priv, u32 mask, bool enable) > hbg_reg_write(priv, HBG_REG_CF_INTRPT_MSK_ADDR, value); > } > > +void hbg_hw_set_uc_addr(struct hbg_priv *priv, u64 mac_addr) > +{ > + hbg_reg_write64(priv, HBG_REG_STATION_ADDR_LOW_2_ADDR, mac_addr); > +} > + > +static void hbg_hw_set_pcu_max_frame_len(struct hbg_priv *priv, > + u16 max_frame_len) > +{ > + max_frame_len = max_t(u32, max_frame_len, HBG_DEFAULT_MTU_SIZE); > + > + /* lower two bits of value must be set to 0. Otherwise, the value is ignored */ > + max_frame_len = round_up(max_frame_len, HBG_PCU_FRAME_LEN_PLUS); > + > + hbg_reg_write_field(priv, HBG_REG_MAX_FRAME_LEN_ADDR, > + HBG_REG_MAX_FRAME_LEN_M, max_frame_len); > +} > + > +static void hbg_hw_set_mac_max_frame_len(struct hbg_priv *priv, > + u16 max_frame_size) > +{ > + hbg_reg_write_field(priv, HBG_REG_MAX_FRAME_SIZE_ADDR, > + HBG_REG_MAX_FRAME_LEN_M, max_frame_size); > +} > + > +void hbg_hw_set_mtu(struct hbg_priv *priv, u16 mtu) > +{ > + hbg_hw_set_pcu_max_frame_len(priv, mtu); > + hbg_hw_set_mac_max_frame_len(priv, mtu); > +} > + > +void hbg_hw_mac_enable(struct hbg_priv *priv, u32 enable) > +{ > + hbg_reg_write_field(priv, HBG_REG_PORT_ENABLE_ADDR, > + HBG_REG_PORT_ENABLE_TX_B, enable); > + hbg_reg_write_field(priv, HBG_REG_PORT_ENABLE_ADDR, > + HBG_REG_PORT_ENABLE_RX_B, enable); > +} > + > void hbg_hw_adjust_link(struct hbg_priv *priv, u32 speed, u32 duplex) > { > hbg_reg_write_field(priv, HBG_REG_PORT_MODE_ADDR, > diff --git a/drivers/net/ethernet/hisilicon/hibmcge/hbg_hw.h b/drivers/net/ethernet/hisilicon/hibmcge/hbg_hw.h > index 4d09bdd41c76..0ce500e907b3 100644 > --- a/drivers/net/ethernet/hisilicon/hibmcge/hbg_hw.h > +++ b/drivers/net/ethernet/hisilicon/hibmcge/hbg_hw.h > @@ -49,5 +49,8 @@ u32 hbg_hw_get_irq_status(struct hbg_priv *priv); > void hbg_hw_irq_clear(struct hbg_priv *priv, u32 mask); > bool hbg_hw_irq_is_enabled(struct hbg_priv *priv, u32 mask); > void hbg_hw_irq_enable(struct hbg_priv *priv, u32 mask, bool enable); > +void hbg_hw_set_mtu(struct hbg_priv *priv, u16 mtu); > +void hbg_hw_mac_enable(struct hbg_priv *priv, u32 enable); > +void hbg_hw_set_uc_addr(struct hbg_priv *priv, u64 mac_addr); > > #endif > diff --git a/drivers/net/ethernet/hisilicon/hibmcge/hbg_main.c b/drivers/net/ethernet/hisilicon/hibmcge/hbg_main.c > index 29e0513fa836..f4e9f6205f04 100644 > --- a/drivers/net/ethernet/hisilicon/hibmcge/hbg_main.c > +++ b/drivers/net/ethernet/hisilicon/hibmcge/hbg_main.c > @@ -2,6 +2,7 @@ > // Copyright (c) 2024 Hisilicon Limited. > > #include <linux/etherdevice.h> > +#include <linux/if_vlan.h> > #include <linux/netdevice.h> > #include <linux/pci.h> > #include "hbg_common.h" > @@ -9,6 +10,104 @@ > #include "hbg_irq.h" > #include "hbg_mdio.h" > > +static void hbg_all_irq_enable(struct hbg_priv *priv, bool enabled) > +{ > + struct hbg_irq_info *info; > + u32 i; > + > + for (i = 0; i < priv->vectors.info_array_len; i++) { > + info = &priv->vectors.info_array[i]; > + hbg_hw_irq_enable(priv, info->mask, enabled); > + } > +} > + > +static int hbg_net_open(struct net_device *netdev) > +{ > + struct hbg_priv *priv = netdev_priv(netdev); > + > + if (test_and_set_bit(HBG_NIC_STATE_OPEN, &priv->state)) > + return 0; [Kalesh] Is there a possibility that dev_open() can be invoked twice? > + > + hbg_all_irq_enable(priv, true); > + hbg_hw_mac_enable(priv, HBG_STATUS_ENABLE); > + netif_start_queue(netdev); > + hbg_phy_start(priv); > + > + return 0; > +} > + > +static int hbg_net_stop(struct net_device *netdev) > +{ > + struct hbg_priv *priv = netdev_priv(netdev); > + > + if (!hbg_nic_is_open(priv)) > + return 0; [Kalesh] Is there any reason to not check HBG_NIC_STATE_OPEN here? > + > + clear_bit(HBG_NIC_STATE_OPEN, &priv->state); > + > + hbg_phy_stop(priv); > + netif_stop_queue(netdev); > + hbg_hw_mac_enable(priv, HBG_STATUS_DISABLE); > + hbg_all_irq_enable(priv, false); > + > + return 0; > +} > + > +static int hbg_net_set_mac_address(struct net_device *netdev, void *addr) > +{ > + struct hbg_priv *priv = netdev_priv(netdev); > + u8 *mac_addr; > + > + mac_addr = ((struct sockaddr *)addr)->sa_data; > + > + hbg_hw_set_uc_addr(priv, ether_addr_to_u64(mac_addr)); > + dev_addr_set(netdev, mac_addr); > + > + return 0; > +} > + > +static void hbg_change_mtu(struct hbg_priv *priv, int new_mtu) > +{ > + u32 frame_len; > + > + frame_len = new_mtu + VLAN_HLEN * priv->dev_specs.vlan_layers + > + ETH_HLEN + ETH_FCS_LEN; > + hbg_hw_set_mtu(priv, frame_len); > +} > + > +static int hbg_net_change_mtu(struct net_device *netdev, int new_mtu) > +{ > + struct hbg_priv *priv = netdev_priv(netdev); > + bool is_opened = hbg_nic_is_open(priv); > + > + hbg_net_stop(netdev); [Kalesh] Do you still need to call stop when NIC is not opened yet? Instead of a new variable, I think you can check netif_running here. > + > + hbg_change_mtu(priv, new_mtu); > + WRITE_ONCE(netdev->mtu, new_mtu); > + > + dev_dbg(&priv->pdev->dev, > + "change mtu from %u to %u\n", netdev->mtu, new_mtu); > + if (is_opened) > + hbg_net_open(netdev); > + return 0; > +} > + > +static void hbg_net_get_stats64(struct net_device *netdev, > + struct rtnl_link_stats64 *stats) > +{ > + netdev_stats_to_stats64(stats, &netdev->stats); > + dev_fetch_sw_netstats(stats, netdev->tstats); > +} > + > +static const struct net_device_ops hbg_netdev_ops = { > + .ndo_open = hbg_net_open, > + .ndo_stop = hbg_net_stop, > + .ndo_validate_addr = eth_validate_addr, > + .ndo_set_mac_address = hbg_net_set_mac_address, > + .ndo_change_mtu = hbg_net_change_mtu, > + .ndo_get_stats64 = hbg_net_get_stats64, > +}; > + > static int hbg_init(struct hbg_priv *priv) > { > int ret; > @@ -73,6 +172,7 @@ static int hbg_probe(struct pci_dev *pdev, const struct pci_device_id *ent) > priv = netdev_priv(netdev); > priv->netdev = netdev; > priv->pdev = pdev; > + netdev->netdev_ops = &hbg_netdev_ops; > > netdev->tstats = devm_netdev_alloc_pcpu_stats(&pdev->dev, > struct pcpu_sw_netstats); > @@ -88,6 +188,10 @@ static int hbg_probe(struct pci_dev *pdev, const struct pci_device_id *ent) > if (ret) > return ret; > > + netdev->max_mtu = priv->dev_specs.max_mtu; > + netdev->min_mtu = priv->dev_specs.min_mtu; > + hbg_change_mtu(priv, HBG_DEFAULT_MTU_SIZE); > + hbg_net_set_mac_address(priv->netdev, &priv->dev_specs.mac_addr); > ret = devm_register_netdev(dev, netdev); > if (ret) > return dev_err_probe(dev, ret, "failed to register netdev\n"); > diff --git a/drivers/net/ethernet/hisilicon/hibmcge/hbg_reg.h b/drivers/net/ethernet/hisilicon/hibmcge/hbg_reg.h > index b0991063ccba..63bb1bead8c0 100644 > --- a/drivers/net/ethernet/hisilicon/hibmcge/hbg_reg.h > +++ b/drivers/net/ethernet/hisilicon/hibmcge/hbg_reg.h > @@ -37,18 +37,24 @@ > #define HBG_REG_SGMII_BASE 0x10000 > #define HBG_REG_DUPLEX_TYPE_ADDR (HBG_REG_SGMII_BASE + 0x0008) > #define HBG_REG_DUPLEX_B BIT(0) > +#define HBG_REG_MAX_FRAME_SIZE_ADDR (HBG_REG_SGMII_BASE + 0x003C) > #define HBG_REG_PORT_MODE_ADDR (HBG_REG_SGMII_BASE + 0x0040) > #define HBG_REG_PORT_MODE_M GENMASK(3, 0) > +#define HBG_REG_PORT_ENABLE_ADDR (HBG_REG_SGMII_BASE + 0x0044) > +#define HBG_REG_PORT_ENABLE_RX_B BIT(1) > +#define HBG_REG_PORT_ENABLE_TX_B BIT(2) > #define HBG_REG_TRANSMIT_CONTROL_ADDR (HBG_REG_SGMII_BASE + 0x0060) > #define HBG_REG_TRANSMIT_CONTROL_PAD_EN_B BIT(7) > #define HBG_REG_TRANSMIT_CONTROL_CRC_ADD_B BIT(6) > #define HBG_REG_TRANSMIT_CONTROL_AN_EN_B BIT(5) > #define HBG_REG_CF_CRC_STRIP_ADDR (HBG_REG_SGMII_BASE + 0x01B0) > -#define HBG_REG_CF_CRC_STRIP_B BIT(0) > +#define HBG_REG_CF_CRC_STRIP_B BIT(1) > #define HBG_REG_MODE_CHANGE_EN_ADDR (HBG_REG_SGMII_BASE + 0x01B4) > #define HBG_REG_MODE_CHANGE_EN_B BIT(0) > #define HBG_REG_RECV_CONTROL_ADDR (HBG_REG_SGMII_BASE + 0x01E0) > #define HBG_REG_RECV_CONTROL_STRIP_PAD_EN_B BIT(3) > +#define HBG_REG_STATION_ADDR_LOW_2_ADDR (HBG_REG_SGMII_BASE + 0x0210) > +#define HBG_REG_STATION_ADDR_HIGH_2_ADDR (HBG_REG_SGMII_BASE + 0x0214) > > /* PCU */ > #define HBG_REG_CF_INTRPT_MSK_ADDR (HBG_REG_SGMII_BASE + 0x042C) > @@ -72,6 +78,8 @@ > #define HBG_INT_MSK_RX_B BIT(0) /* just used in driver */ > #define HBG_REG_CF_INTRPT_STAT_ADDR (HBG_REG_SGMII_BASE + 0x0434) > #define HBG_REG_CF_INTRPT_CLR_ADDR (HBG_REG_SGMII_BASE + 0x0438) > +#define HBG_REG_MAX_FRAME_LEN_ADDR (HBG_REG_SGMII_BASE + 0x0444) > +#define HBG_REG_MAX_FRAME_LEN_M GENMASK(15, 0) > #define HBG_REG_RX_BUF_SIZE_ADDR (HBG_REG_SGMII_BASE + 0x04E4) > #define HBG_REG_RX_BUF_SIZE_M GENMASK(15, 0) > #define HBG_REG_BUS_CTRL_ADDR (HBG_REG_SGMII_BASE + 0x04E8) > @@ -86,6 +94,7 @@ > #define HBG_REG_RX_PKT_MODE_ADDR (HBG_REG_SGMII_BASE + 0x04F4) > #define HBG_REG_RX_PKT_MODE_PARSE_MODE_M GENMASK(22, 21) > #define HBG_REG_CF_IND_TXINT_MSK_ADDR (HBG_REG_SGMII_BASE + 0x0694) > +#define HBG_REG_IND_INTR_MASK_B BIT(0) > #define HBG_REG_CF_IND_TXINT_STAT_ADDR (HBG_REG_SGMII_BASE + 0x0698) > #define HBG_REG_CF_IND_TXINT_CLR_ADDR (HBG_REG_SGMII_BASE + 0x069C) > #define HBG_REG_CF_IND_RXINT_MSK_ADDR (HBG_REG_SGMII_BASE + 0x06a0) > -- > 2.33.0 > >
on 2024/9/9 11:05, Kalesh Anakkur Purayil wrote: > On Mon, Sep 9, 2024 at 8:11 AM Jijie Shao <shaojijie@huawei.com> wrote: >> +} >> + >> +static int hbg_net_open(struct net_device *netdev) >> +{ >> + struct hbg_priv *priv = netdev_priv(netdev); >> + >> + if (test_and_set_bit(HBG_NIC_STATE_OPEN, &priv->state)) >> + return 0; > [Kalesh] Is there a possibility that dev_open() can be invoked twice? We want stop NIC when chang_mtu 、self_test or FLR. So, driver will directly invoke hbg_net_stop() not dev_open() if need. Therefore, driver must ensure that hbg_net_open() or hbg_net_stop() can not be invoked twice. >> + >> + hbg_all_irq_enable(priv, true); >> + hbg_hw_mac_enable(priv, HBG_STATUS_ENABLE); >> + netif_start_queue(netdev); >> + hbg_phy_start(priv); >> + >> + return 0; >> +} >> + >> +static int hbg_net_stop(struct net_device *netdev) >> +{ >> + struct hbg_priv *priv = netdev_priv(netdev); >> + >> + if (!hbg_nic_is_open(priv)) >> + return 0; > [Kalesh] Is there any reason to not check HBG_NIC_STATE_OPEN here? Actually, hbg_nic_is_open() is used to check HBG_NIC_STATE_OPEN. : #define hbg_nic_is_open(priv) test_bit(HBG_NIC_STATE_OPEN, &(priv)->state) >> + >> + clear_bit(HBG_NIC_STATE_OPEN, &priv->state); >> + >> + hbg_phy_stop(priv); >> + netif_stop_queue(netdev); >> + hbg_hw_mac_enable(priv, HBG_STATUS_DISABLE); >> + hbg_all_irq_enable(priv, false); >> + >> + return 0; >> +} >> + >> +static int hbg_net_change_mtu(struct net_device *netdev, int new_mtu) >> +{ >> + struct hbg_priv *priv = netdev_priv(netdev); >> + bool is_opened = hbg_nic_is_open(priv); >> + >> + hbg_net_stop(netdev); > [Kalesh] Do you still need to call stop when NIC is not opened yet? > Instead of a new variable, I think you can check netif_running here. hbg_net_stop() will check HBG_NIC_STATE_OPEN by hbg_nic_is_open(), So, if NIC is not opened, hbg_net_stop() will do nothing Thanks! Jijie Shao
On Mon, Sep 09, 2024 at 12:04:53PM +0800, Jijie Shao wrote: > > on 2024/9/9 11:05, Kalesh Anakkur Purayil wrote: > > On Mon, Sep 9, 2024 at 8:11 AM Jijie Shao <shaojijie@huawei.com> wrote: > > > +} > > > + > > > +static int hbg_net_open(struct net_device *netdev) > > > +{ > > > + struct hbg_priv *priv = netdev_priv(netdev); > > > + > > > + if (test_and_set_bit(HBG_NIC_STATE_OPEN, &priv->state)) > > > + return 0; > > [Kalesh] Is there a possibility that dev_open() can be invoked twice? > > We want stop NIC when chang_mtu 、self_test or FLR. > So, driver will directly invoke hbg_net_stop() not dev_open() if need. > Therefore, driver must ensure that hbg_net_open() or hbg_net_stop() can not be invoked twice. Generally, we don't want defensive programming. You seem to suggest hbg_net_open and hbg_net_stop are called in pairs. If this is not true, you have a bug? Rather than paper over the bug with a return, let bad things happen so the bug is obvious. > > > + > > > + hbg_all_irq_enable(priv, true); > > > + hbg_hw_mac_enable(priv, HBG_STATUS_ENABLE); > > > + netif_start_queue(netdev); > > > + hbg_phy_start(priv); > > > + > > > + return 0; > > > +} > > > + > > > +static int hbg_net_stop(struct net_device *netdev) > > > +{ > > > + struct hbg_priv *priv = netdev_priv(netdev); > > > + > > > + if (!hbg_nic_is_open(priv)) > > > + return 0; > > [Kalesh] Is there any reason to not check HBG_NIC_STATE_OPEN here? > > Actually, hbg_nic_is_open() is used to check HBG_NIC_STATE_OPEN. > : > #define hbg_nic_is_open(priv) test_bit(HBG_NIC_STATE_OPEN, &(priv)->state) Which is horrible. Why hide this, when it is in full view in other places. Please take a step back. Take time to understand the driver locking with RTNL, look at what state is already available, and try very hard to remove priv->state. > > > > + > > > + clear_bit(HBG_NIC_STATE_OPEN, &priv->state); > > > + While we are at it, why is there not a race condition here between testing and clearing the bit? > > > + hbg_phy_stop(priv); > > > + netif_stop_queue(netdev); > > > + hbg_hw_mac_enable(priv, HBG_STATUS_DISABLE); > > > + hbg_all_irq_enable(priv, false); > > > + > > > + return 0; > > > +} > > > + > > > +static int hbg_net_change_mtu(struct net_device *netdev, int new_mtu) > > > +{ > > > + struct hbg_priv *priv = netdev_priv(netdev); > > > + bool is_opened = hbg_nic_is_open(priv); > > > + > > > + hbg_net_stop(netdev); > > [Kalesh] Do you still need to call stop when NIC is not opened yet? > > Instead of a new variable, I think you can check netif_running here. Correct. running is used by every driver, it has withstood years of testing, so is guaranteed to be set/cleared in race free ways. It should be used instead of this racy priv->state. Andrew
on 2024/9/9 20:19, Andrew Lunn wrote: > On Mon, Sep 09, 2024 at 12:04:53PM +0800, Jijie Shao wrote: >> on 2024/9/9 11:05, Kalesh Anakkur Purayil wrote: >>> On Mon, Sep 9, 2024 at 8:11 AM Jijie Shao <shaojijie@huawei.com> wrote: >>>> +} >>>> + >>>> +static int hbg_net_open(struct net_device *netdev) >>>> +{ >>>> + struct hbg_priv *priv = netdev_priv(netdev); >>>> + >>>> + if (test_and_set_bit(HBG_NIC_STATE_OPEN, &priv->state)) >>>> + return 0; >>> [Kalesh] Is there a possibility that dev_open() can be invoked twice? >> We want stop NIC when chang_mtu 、self_test or FLR. >> So, driver will directly invoke hbg_net_stop() not dev_open() if need. >> Therefore, driver must ensure that hbg_net_open() or hbg_net_stop() can not be invoked twice. > Generally, we don't want defensive programming. You seem to suggest > hbg_net_open and hbg_net_stop are called in pairs. If this is not > true, you have a bug? Rather than paper over the bug with a return, > let bad things happen so the bug is obvious. No, HBG_NIC_STATE_OPEN is not intended to ensure that hbg_net_open() and hbg_net_stop() are mutually exclusive. Actually, when the driver do reset or self-test(ethtool -t or ethtool --reset or FLR). We hope that no other data is transmitted or received at this time. Therefore, the driver directly uses hbg_net_stop() to stop the NIC. In this case, IFF_UP may be set. Therefore, dev_close() can be invoked. As a result, hbg_net_stop is invoked twice. In my opinion, driver is not suitable for directly using dev_open(), dev_close(), or modifying dev->state. Therefore, HBG_NIC_STATE_OPEN is added to ensure that hbg_net_stop() is not invoked twice. if I remove HBG_NIC_STATE_OPEN, I may get log(10s sleep time is added during reset): #ifconfig enp131s0f1 up #echo 1 > /sys/bus/pci/devices/0000\:83\:00.1/reset & sleep 2; ifconfig enp131s0f1 down [ 213.332855] hibmcge 0000:83:00.1: FLR prepare [ 213.337905] [STUB][hbg_net_stop 81] HBG_NIC_STATE_OPEN already clear [ 213.345064] hibmcge 0000:83:00.1 enp131s0f1: Link is Down [ 213.351408] [STUB][hbg_reset_prepare 126] reset sleep 10s after hbg_net_stop [ 215.359812] [STUB][hbg_net_stop 81] HBG_NIC_STATE_OPEN already clear [ 223.991959] hibmcge 0000:83:00.1: rebuild success [ 223.998656] ------------[ cut here ]------------ [ 223.998987] hibmcge 0000:83:00.1: reset done [ 224.003950] called from state HALTED [ 224.008898] hibmcge 0000:83:00.1: FLR done [ 224.008926] WARNING: CPU: 7 PID: 4381 at drivers/net/phy/phy.c:1330 phy_stop+0x118/0x160 [ 224.022412] Modules linked in: hibmcge(OE) nft_fib_inet nft_fib_ipv4 nft_fib_ipv6 nft_fib nft_reject_inet nf_reject_ipv4 nf_reject_ipv6 nft_reject nft_ct nft_chain_nat nf_tables ebtable_nat ebtable_broute ip6table_nat ip6table_mangle ip6table_raw ip6table_security iptable_nat nf_nat nf_conntrack nf_defrag_ipv6 nf_defrag_ipv4 libcrc32c iptable_mangle iptable_raw iptable_security ip_set nfnetlink rfkill ebtable_filter ebtables ip6table_filter ip6_tables iptable_filter ip_tables sunrpc nls_cp437 vfat fat ipmi_ssif hns_roce_hw_v2 sg ib_uverbs ib_core acpi_ipmi hisi_uncore_ddrc_pmu hisi_uncore_l3c_pmu hisi_uncore_hha_pmu ipmi_si hisi_uncore_pmu ipmi_devintf ipmi_msghandler sch_fq_codel fuse ext4 mbcache jbd2 sd_mod realtek t10_pi crc64_rocksoft_generic crc64_rocksoft crc64 hclge hisi_sas_v3_hw crct10dif_ce hisi_sas_main uas ghash_ce libsas sha2_ce ahci libahci scsi_transport_sas sha256_arm64 sha1_ce sbsa_gwdt usb_storage hns3 nfit libata hnae3 libnvdimm i2c_designware_platform i2c_designware_core dm_mirror dm_region_hash [ 224.022504] dm_log dm_mod aes_neon_bs aes_neon_blk aes_ce_blk aes_ce_cipher [ 224.022514] CPU: 7 PID: 4381 Comm: ifconfig Kdump: loaded Tainted: G OE 6.4.0+ #1 [ 224.022517] Hardware name: Huawei TaiShan 200 (Model 2280)/BC82AMDD, BIOS 1.93 10/13/2022 [ 224.022519] pstate: 60400009 (nZCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--) [ 224.022521] pc : phy_stop+0x118/0x160 [ 224.022524] lr : phy_stop+0x118/0x160 [ 224.022527] sp : ffff80003375ba80 [ 224.022528] x29: ffff80003375ba80 x28: ffff20200a7f1a00 x27: 0000000000000000 [ 224.022533] x26: 0000000000000001 x25: 0000000000000000 x24: ffff20200c1d2260 [ 224.022537] x23: ffff20200c1d2070 x22: ffff20200c1d2000 x21: ffffb9747e21e514 [ 224.022541] x20: ffff20200c1d2980 x19: ffff002092d1d800 x18: 0000000000000020 [ 224.022544] x17: 0000000000000000 x16: ffffb9747e068f30 x15: ffffffffffffffff [ 224.022548] x14: 0000000000000000 x13: 4445544c41482065 x12: 74617473206d6f72 [ 224.022552] x11: 00000000ffff7fff x10: 00000000ffff7fff x9 : ffffb9747d978500 [ 224.022556] x8 : 00000000000bffe8 x7 : c0000000ffff7fff x6 : 000000000005fff4 [ 224.022560] x5 : 00000000002bffa8 x4 : 0000000000000000 x3 : 0000000000000000 [ 224.022564] x2 : 0000000000000000 x1 : 0000000000000000 x0 : ffff0021099c4800 [ 224.022568] Call trace: [ 224.022571] phy_stop+0x118/0x160 [ 224.022574] hbg_phy_stop+0x20/0x38 [hibmcge] [ 224.022583] hbg_net_stop+0x74/0xd0 [hibmcge] [ 224.022590] __dev_close_many+0xbc/0x170 [ 224.022595] __dev_change_flags+0x120/0x300 [ 224.022600] dev_change_flags+0x2c/0x80 [ 224.022602] devinet_ioctl+0x63c/0x700 [ 224.022609] inet_ioctl+0x1e4/0x200 [ 224.022611] sock_do_ioctl+0x50/0x108 [ 224.022616] sock_ioctl+0x120/0x388 [ 224.022618] __arm64_sys_ioctl+0xb0/0x100 [ 224.022623] invoke_syscall+0x50/0x128 [ 224.022627] el0_svc_common.constprop.0+0x158/0x188 [ 224.022629] do_el0_svc+0x34/0x50 [ 224.022631] el0_svc+0x28/0xe0 [ 224.022637] el0t_64_sync_handler+0xb8/0xc0 [ 224.022640] el0t_64_sync+0x188/0x190 [ 224.022643] ---[ end trace 0000000000000000 ]--- If it is suitable to call dev_close() directly in driver, I think HBG_NIC_STATE_OPEN can be removed. Thanks Jijie Shao
> No, HBG_NIC_STATE_OPEN is not intended to ensure that hbg_net_open() and > hbg_net_stop() are mutually exclusive. > > Actually, when the driver do reset or self-test(ethtool -t or ethtool --reset or FLR). > We hope that no other data is transmitted or received at this time. That is an invalid assumption. You could be receiving line rate broadcast traffic for example, because there is a broadcast storm happening. I assume for testing, you are configuring a loopback somewhere? PHY loopback or PCS loopback? I've seen some PHYs do 'broken' loopback where egress traffic is looped back, but ingress traffic is also still received. Is this true for your hardware? Is this why you make this assumption? What is your use case for ethtool --reset? Are you working around hardware bugs? Why not simply return -EBUSY if the user tries to use --reset when the interface is admin up. You then know the interface is down, you don't need open/close to be re-entrant safe. Same for testing. Return -ENETDOWN if the user tries to do self test on an interface which is admin down. Andrew
on 2024/9/9 22:48, Andrew Lunn wrote: >> No, HBG_NIC_STATE_OPEN is not intended to ensure that hbg_net_open() and >> hbg_net_stop() are mutually exclusive. >> >> Actually, when the driver do reset or self-test(ethtool -t or ethtool --reset or FLR). >> We hope that no other data is transmitted or received at this time. > That is an invalid assumption. You could be receiving line rate > broadcast traffic for example, because there is a broadcast storm > happening. > > I assume for testing, you are configuring a loopback somewhere? PHY > loopback or PCS loopback? I've seen some PHYs do 'broken' loopback > where egress traffic is looped back, but ingress traffic is also still > received. Is this true for your hardware? Is this why you make this > assumption? > > What is your use case for ethtool --reset? Are you working around > hardware bugs? Why not simply return -EBUSY if the user tries to use > --reset when the interface is admin up. You then know the interface is > down, you don't need open/close to be re-entrant safe. > > Same for testing. Return -ENETDOWN if the user tries to do self test > on an interface which is admin down. > > Andrew Good idea. I'll remove priv->state first in v9. We'll discuss this internally. Thank you very much. Jiji Shao
diff --git a/drivers/net/ethernet/hisilicon/hibmcge/hbg_common.h b/drivers/net/ethernet/hisilicon/hibmcge/hbg_common.h index e94ae2be5c4c..d11ef081f4da 100644 --- a/drivers/net/ethernet/hisilicon/hibmcge/hbg_common.h +++ b/drivers/net/ethernet/hisilicon/hibmcge/hbg_common.h @@ -17,8 +17,11 @@ enum hbg_nic_state { HBG_NIC_STATE_EVENT_HANDLING = 0, + HBG_NIC_STATE_OPEN, }; +#define hbg_nic_is_open(priv) test_bit(HBG_NIC_STATE_OPEN, &(priv)->state) + enum hbg_hw_event_type { HBG_HW_EVENT_NONE = 0, HBG_HW_EVENT_INIT, /* driver is loading */ diff --git a/drivers/net/ethernet/hisilicon/hibmcge/hbg_hw.c b/drivers/net/ethernet/hisilicon/hibmcge/hbg_hw.c index 8e971e9f62a0..97fee714155a 100644 --- a/drivers/net/ethernet/hisilicon/hibmcge/hbg_hw.c +++ b/drivers/net/ethernet/hisilicon/hibmcge/hbg_hw.c @@ -15,6 +15,7 @@ * ctrl means packet description, data means skb packet data */ #define HBG_ENDIAN_CTRL_LE_DATA_BE 0x0 +#define HBG_PCU_FRAME_LEN_PLUS 4 static bool hbg_hw_spec_is_valid(struct hbg_priv *priv) { @@ -129,6 +130,44 @@ void hbg_hw_irq_enable(struct hbg_priv *priv, u32 mask, bool enable) hbg_reg_write(priv, HBG_REG_CF_INTRPT_MSK_ADDR, value); } +void hbg_hw_set_uc_addr(struct hbg_priv *priv, u64 mac_addr) +{ + hbg_reg_write64(priv, HBG_REG_STATION_ADDR_LOW_2_ADDR, mac_addr); +} + +static void hbg_hw_set_pcu_max_frame_len(struct hbg_priv *priv, + u16 max_frame_len) +{ + max_frame_len = max_t(u32, max_frame_len, HBG_DEFAULT_MTU_SIZE); + + /* lower two bits of value must be set to 0. Otherwise, the value is ignored */ + max_frame_len = round_up(max_frame_len, HBG_PCU_FRAME_LEN_PLUS); + + hbg_reg_write_field(priv, HBG_REG_MAX_FRAME_LEN_ADDR, + HBG_REG_MAX_FRAME_LEN_M, max_frame_len); +} + +static void hbg_hw_set_mac_max_frame_len(struct hbg_priv *priv, + u16 max_frame_size) +{ + hbg_reg_write_field(priv, HBG_REG_MAX_FRAME_SIZE_ADDR, + HBG_REG_MAX_FRAME_LEN_M, max_frame_size); +} + +void hbg_hw_set_mtu(struct hbg_priv *priv, u16 mtu) +{ + hbg_hw_set_pcu_max_frame_len(priv, mtu); + hbg_hw_set_mac_max_frame_len(priv, mtu); +} + +void hbg_hw_mac_enable(struct hbg_priv *priv, u32 enable) +{ + hbg_reg_write_field(priv, HBG_REG_PORT_ENABLE_ADDR, + HBG_REG_PORT_ENABLE_TX_B, enable); + hbg_reg_write_field(priv, HBG_REG_PORT_ENABLE_ADDR, + HBG_REG_PORT_ENABLE_RX_B, enable); +} + void hbg_hw_adjust_link(struct hbg_priv *priv, u32 speed, u32 duplex) { hbg_reg_write_field(priv, HBG_REG_PORT_MODE_ADDR, diff --git a/drivers/net/ethernet/hisilicon/hibmcge/hbg_hw.h b/drivers/net/ethernet/hisilicon/hibmcge/hbg_hw.h index 4d09bdd41c76..0ce500e907b3 100644 --- a/drivers/net/ethernet/hisilicon/hibmcge/hbg_hw.h +++ b/drivers/net/ethernet/hisilicon/hibmcge/hbg_hw.h @@ -49,5 +49,8 @@ u32 hbg_hw_get_irq_status(struct hbg_priv *priv); void hbg_hw_irq_clear(struct hbg_priv *priv, u32 mask); bool hbg_hw_irq_is_enabled(struct hbg_priv *priv, u32 mask); void hbg_hw_irq_enable(struct hbg_priv *priv, u32 mask, bool enable); +void hbg_hw_set_mtu(struct hbg_priv *priv, u16 mtu); +void hbg_hw_mac_enable(struct hbg_priv *priv, u32 enable); +void hbg_hw_set_uc_addr(struct hbg_priv *priv, u64 mac_addr); #endif diff --git a/drivers/net/ethernet/hisilicon/hibmcge/hbg_main.c b/drivers/net/ethernet/hisilicon/hibmcge/hbg_main.c index 29e0513fa836..f4e9f6205f04 100644 --- a/drivers/net/ethernet/hisilicon/hibmcge/hbg_main.c +++ b/drivers/net/ethernet/hisilicon/hibmcge/hbg_main.c @@ -2,6 +2,7 @@ // Copyright (c) 2024 Hisilicon Limited. #include <linux/etherdevice.h> +#include <linux/if_vlan.h> #include <linux/netdevice.h> #include <linux/pci.h> #include "hbg_common.h" @@ -9,6 +10,104 @@ #include "hbg_irq.h" #include "hbg_mdio.h" +static void hbg_all_irq_enable(struct hbg_priv *priv, bool enabled) +{ + struct hbg_irq_info *info; + u32 i; + + for (i = 0; i < priv->vectors.info_array_len; i++) { + info = &priv->vectors.info_array[i]; + hbg_hw_irq_enable(priv, info->mask, enabled); + } +} + +static int hbg_net_open(struct net_device *netdev) +{ + struct hbg_priv *priv = netdev_priv(netdev); + + if (test_and_set_bit(HBG_NIC_STATE_OPEN, &priv->state)) + return 0; + + hbg_all_irq_enable(priv, true); + hbg_hw_mac_enable(priv, HBG_STATUS_ENABLE); + netif_start_queue(netdev); + hbg_phy_start(priv); + + return 0; +} + +static int hbg_net_stop(struct net_device *netdev) +{ + struct hbg_priv *priv = netdev_priv(netdev); + + if (!hbg_nic_is_open(priv)) + return 0; + + clear_bit(HBG_NIC_STATE_OPEN, &priv->state); + + hbg_phy_stop(priv); + netif_stop_queue(netdev); + hbg_hw_mac_enable(priv, HBG_STATUS_DISABLE); + hbg_all_irq_enable(priv, false); + + return 0; +} + +static int hbg_net_set_mac_address(struct net_device *netdev, void *addr) +{ + struct hbg_priv *priv = netdev_priv(netdev); + u8 *mac_addr; + + mac_addr = ((struct sockaddr *)addr)->sa_data; + + hbg_hw_set_uc_addr(priv, ether_addr_to_u64(mac_addr)); + dev_addr_set(netdev, mac_addr); + + return 0; +} + +static void hbg_change_mtu(struct hbg_priv *priv, int new_mtu) +{ + u32 frame_len; + + frame_len = new_mtu + VLAN_HLEN * priv->dev_specs.vlan_layers + + ETH_HLEN + ETH_FCS_LEN; + hbg_hw_set_mtu(priv, frame_len); +} + +static int hbg_net_change_mtu(struct net_device *netdev, int new_mtu) +{ + struct hbg_priv *priv = netdev_priv(netdev); + bool is_opened = hbg_nic_is_open(priv); + + hbg_net_stop(netdev); + + hbg_change_mtu(priv, new_mtu); + WRITE_ONCE(netdev->mtu, new_mtu); + + dev_dbg(&priv->pdev->dev, + "change mtu from %u to %u\n", netdev->mtu, new_mtu); + if (is_opened) + hbg_net_open(netdev); + return 0; +} + +static void hbg_net_get_stats64(struct net_device *netdev, + struct rtnl_link_stats64 *stats) +{ + netdev_stats_to_stats64(stats, &netdev->stats); + dev_fetch_sw_netstats(stats, netdev->tstats); +} + +static const struct net_device_ops hbg_netdev_ops = { + .ndo_open = hbg_net_open, + .ndo_stop = hbg_net_stop, + .ndo_validate_addr = eth_validate_addr, + .ndo_set_mac_address = hbg_net_set_mac_address, + .ndo_change_mtu = hbg_net_change_mtu, + .ndo_get_stats64 = hbg_net_get_stats64, +}; + static int hbg_init(struct hbg_priv *priv) { int ret; @@ -73,6 +172,7 @@ static int hbg_probe(struct pci_dev *pdev, const struct pci_device_id *ent) priv = netdev_priv(netdev); priv->netdev = netdev; priv->pdev = pdev; + netdev->netdev_ops = &hbg_netdev_ops; netdev->tstats = devm_netdev_alloc_pcpu_stats(&pdev->dev, struct pcpu_sw_netstats); @@ -88,6 +188,10 @@ static int hbg_probe(struct pci_dev *pdev, const struct pci_device_id *ent) if (ret) return ret; + netdev->max_mtu = priv->dev_specs.max_mtu; + netdev->min_mtu = priv->dev_specs.min_mtu; + hbg_change_mtu(priv, HBG_DEFAULT_MTU_SIZE); + hbg_net_set_mac_address(priv->netdev, &priv->dev_specs.mac_addr); ret = devm_register_netdev(dev, netdev); if (ret) return dev_err_probe(dev, ret, "failed to register netdev\n"); diff --git a/drivers/net/ethernet/hisilicon/hibmcge/hbg_reg.h b/drivers/net/ethernet/hisilicon/hibmcge/hbg_reg.h index b0991063ccba..63bb1bead8c0 100644 --- a/drivers/net/ethernet/hisilicon/hibmcge/hbg_reg.h +++ b/drivers/net/ethernet/hisilicon/hibmcge/hbg_reg.h @@ -37,18 +37,24 @@ #define HBG_REG_SGMII_BASE 0x10000 #define HBG_REG_DUPLEX_TYPE_ADDR (HBG_REG_SGMII_BASE + 0x0008) #define HBG_REG_DUPLEX_B BIT(0) +#define HBG_REG_MAX_FRAME_SIZE_ADDR (HBG_REG_SGMII_BASE + 0x003C) #define HBG_REG_PORT_MODE_ADDR (HBG_REG_SGMII_BASE + 0x0040) #define HBG_REG_PORT_MODE_M GENMASK(3, 0) +#define HBG_REG_PORT_ENABLE_ADDR (HBG_REG_SGMII_BASE + 0x0044) +#define HBG_REG_PORT_ENABLE_RX_B BIT(1) +#define HBG_REG_PORT_ENABLE_TX_B BIT(2) #define HBG_REG_TRANSMIT_CONTROL_ADDR (HBG_REG_SGMII_BASE + 0x0060) #define HBG_REG_TRANSMIT_CONTROL_PAD_EN_B BIT(7) #define HBG_REG_TRANSMIT_CONTROL_CRC_ADD_B BIT(6) #define HBG_REG_TRANSMIT_CONTROL_AN_EN_B BIT(5) #define HBG_REG_CF_CRC_STRIP_ADDR (HBG_REG_SGMII_BASE + 0x01B0) -#define HBG_REG_CF_CRC_STRIP_B BIT(0) +#define HBG_REG_CF_CRC_STRIP_B BIT(1) #define HBG_REG_MODE_CHANGE_EN_ADDR (HBG_REG_SGMII_BASE + 0x01B4) #define HBG_REG_MODE_CHANGE_EN_B BIT(0) #define HBG_REG_RECV_CONTROL_ADDR (HBG_REG_SGMII_BASE + 0x01E0) #define HBG_REG_RECV_CONTROL_STRIP_PAD_EN_B BIT(3) +#define HBG_REG_STATION_ADDR_LOW_2_ADDR (HBG_REG_SGMII_BASE + 0x0210) +#define HBG_REG_STATION_ADDR_HIGH_2_ADDR (HBG_REG_SGMII_BASE + 0x0214) /* PCU */ #define HBG_REG_CF_INTRPT_MSK_ADDR (HBG_REG_SGMII_BASE + 0x042C) @@ -72,6 +78,8 @@ #define HBG_INT_MSK_RX_B BIT(0) /* just used in driver */ #define HBG_REG_CF_INTRPT_STAT_ADDR (HBG_REG_SGMII_BASE + 0x0434) #define HBG_REG_CF_INTRPT_CLR_ADDR (HBG_REG_SGMII_BASE + 0x0438) +#define HBG_REG_MAX_FRAME_LEN_ADDR (HBG_REG_SGMII_BASE + 0x0444) +#define HBG_REG_MAX_FRAME_LEN_M GENMASK(15, 0) #define HBG_REG_RX_BUF_SIZE_ADDR (HBG_REG_SGMII_BASE + 0x04E4) #define HBG_REG_RX_BUF_SIZE_M GENMASK(15, 0) #define HBG_REG_BUS_CTRL_ADDR (HBG_REG_SGMII_BASE + 0x04E8) @@ -86,6 +94,7 @@ #define HBG_REG_RX_PKT_MODE_ADDR (HBG_REG_SGMII_BASE + 0x04F4) #define HBG_REG_RX_PKT_MODE_PARSE_MODE_M GENMASK(22, 21) #define HBG_REG_CF_IND_TXINT_MSK_ADDR (HBG_REG_SGMII_BASE + 0x0694) +#define HBG_REG_IND_INTR_MASK_B BIT(0) #define HBG_REG_CF_IND_TXINT_STAT_ADDR (HBG_REG_SGMII_BASE + 0x0698) #define HBG_REG_CF_IND_TXINT_CLR_ADDR (HBG_REG_SGMII_BASE + 0x069C) #define HBG_REG_CF_IND_RXINT_MSK_ADDR (HBG_REG_SGMII_BASE + 0x06a0)
Implement the .ndo_open() .ndo_stop() .ndo_set_mac_address() .ndo_change_mtu functions() and ndo.get_stats64() And .ndo_validate_addr calls the eth_validate_addr function directly Signed-off-by: Jijie Shao <shaojijie@huawei.com> --- ChangeLog: v6 -> v7: - Add implement ndo.get_stats64(), suggested by Paolo. v6: https://lore.kernel.org/all/20240830121604.2250904-6-shaojijie@huawei.com/ v5 -> v6: - Delete netif_carrier_off() in .ndo_open() and .ndo_stop(), suggested by Jakub and Andrew. v5: https://lore.kernel.org/all/20240827131455.2919051-1-shaojijie@huawei.com/ v3 -> v4: - Delete INITED_STATE in priv, suggested by Andrew. - Delete unnecessary defensive code in hbg_phy_start() and hbg_phy_stop(), suggested by Andrew. v3: https://lore.kernel.org/all/20240822093334.1687011-1-shaojijie@huawei.com/ RFC v1 -> RFC v2: - Delete validation for mtu in hbg_net_change_mtu(), suggested by Andrew. - Delete validation for mac address in hbg_net_set_mac_address(), suggested by Andrew. - Add a patch to add is_valid_ether_addr check in dev_set_mac_address, suggested by Andrew. RFC v1: https://lore.kernel.org/all/20240731094245.1967834-1-shaojijie@huawei.com/ --- .../ethernet/hisilicon/hibmcge/hbg_common.h | 3 + .../net/ethernet/hisilicon/hibmcge/hbg_hw.c | 39 +++++++ .../net/ethernet/hisilicon/hibmcge/hbg_hw.h | 3 + .../net/ethernet/hisilicon/hibmcge/hbg_main.c | 104 ++++++++++++++++++ .../net/ethernet/hisilicon/hibmcge/hbg_reg.h | 11 +- 5 files changed, 159 insertions(+), 1 deletion(-)