| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAH-L+nMPOyhkjt530-L9EvAAQ87nBJ7RdShgHJ+VOC4fpvLXoA@mail.gmail.com>
Date: Tue, 10 Sep 2024 13:50:36 +0530
From: Kalesh Anakkur Purayil <kalesh-anakkur.purayil@...adcom.com>
To: Jijie Shao <shaojijie@...wei.com>
Cc: davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
pabeni@...hat.com, shenjian15@...wei.com, wangpeiyang1@...wei.com,
liuyonglong@...wei.com, chenhao418@...wei.com, sudongming1@...wei.com,
xujunsheng@...wei.com, shiyongbang@...wei.com, libaihan@...wei.com,
andrew@...n.ch, jdamato@...tly.com, horms@...nel.org,
jonathan.cameron@...wei.com, shameerali.kolothum.thodi@...wei.com,
salil.mehta@...wei.com, netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH V9 net-next 05/11] net: hibmcge: Implement some .ndo functions
Thank you Jijie for taking care of the comments. One comment in line
though it is not directly related to your changes.
On Tue, Sep 10, 2024 at 1:36 PM Jijie Shao <shaojijie@...wei.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@...wei.com>
Reviewed-by: Kalesh AP <kalesh-anakkur.purayil@...adcom.com>
> ---
> ChangeLog:
> v8 -> v9:
> - Remove HBG_NIC_STATE_OPEN in ndo.open() and ndo.stop(),
> suggested by Kalesh and Andrew.
> - Use netif_running() instead of hbg_nic_is_open() in ndo.change_mtu(),
> suggested by Kalesh and Andrew
> v8: https://lore.kernel.org/all/20240909023141.3234567-1-shaojijie@huawei.com/
> 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/
> ---
> .../net/ethernet/hisilicon/hibmcge/hbg_hw.c | 39 ++++++++
> .../net/ethernet/hisilicon/hibmcge/hbg_hw.h | 3 +
> .../net/ethernet/hisilicon/hibmcge/hbg_main.c | 97 +++++++++++++++++++
> .../net/ethernet/hisilicon/hibmcge/hbg_reg.h | 11 ++-
> 4 files changed, 149 insertions(+), 1 deletion(-)
>
> 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..d882a7822299 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,97 @@
> #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);
> +
> + 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);
> +
> + 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_running = netif_running(netdev);
> +
> + if (is_running)
> + hbg_net_stop(netdev);
> +
> + hbg_change_mtu(priv, new_mtu);
> + WRITE_ONCE(netdev->mtu, new_mtu);
[Kalesh] IMO the setting of "netdev->mtu" should be moved to the core
layer so that not all drivers have to do this.
__dev_set_mtu() can be modified to incorporate this. Just a thought.
> +
> + dev_dbg(&priv->pdev->dev,
> + "change mtu from %u to %u\n", netdev->mtu, new_mtu);
> + if (is_running)
> + 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 +165,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 +181,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
>
--
Regards,
Kalesh A P
Download attachment "smime.p7s" of type "application/pkcs7-signature" (4239 bytes)
Powered by blists - more mailing lists