net: hinic3: PF initialization
Subsystems (4)
Fan Gong
maintainerv9 cover net: hinic3: PF initialization
This is [3/3] part of hinic3 Ethernet driver second submission. With this patch hinic3 becomes a complete Ethernet driver with pf and vf.
Add 20 ethtool ops for information of queue, rss, coalesce and eth data. Add MTU size validation
Config netdev watchdog timeout.
Remove unneed coalesce parameters.
Changes:
PATCH 03 V01: https://lore.kernel.org/netdev/[email protected]/
* Add rmon/pause/phy/mac/ctrl stats (Ioana Ciornei)
PATCH 03 V02: https://lore.kernel.org/netdev/[email protected]/
* Modify "return -EINVAL" intension problem (AI review)
* Use le16_to_cpu for rss_indir pair.out->buf (AI review)
* Use u32 instead of int in coalesce_limits to avoid overflow (AI review)
* Remove redundant u64_stats_update_begin/end when reading stats without concurrent reader (AI review)
* Modify nic_dev->stats.syncp logic (AI review)
* Complete rxq/txq stats stats fileds in hinic3_rx/txq_get_stats (AI review)
* Remove statistics values in rtnl_link_stats64 from ethtool statistics values (AI review)
* Add channel_cfg_lock & channel_res_lock to protect resources access (AI review)
* Remove OutOfRangeLengthField, FrameToolong and InRangeLengthErrors (Ioana Ciornei)
* Remove redundant mtu commit (Maxime Chevialler)
PATCH 03 V03: https://lore.kernel.org/netdev/[email protected]/
* Change unnedd to unneeded (AI review)
* Remove packets,bytes,errors and dropped in hinic3_rx/tx_queue_stats (AI review)
* Remove duplicated entried in hinic3_port_stats[] (AI review)
* change stats_info.head.status to ps->head.status (AI review)
PATCH 03 V04: https://lore.kernel.org/netdev/[email protected]/
* Remove restore_drop_sge in hinic3_rx_queue_stats (AI review)
* Remove hinic3_nic_stats (AI review)
* Use old_q_param to store old config and use it in error handling (Mohsin Bashir)
* Add netdev_info to inform the user that depth is trimmed (Mohsin Bashir)
* Remove const in hinic3_get_qp_stats_strings parameters (Mohsin Bashir)
* Change EOPNOTSUPP to ERANGE in is_coalesce_exceed_limit (Mohsin Bashir)
* Update nic_dev->rss_type after hinic3_set_rss_type (Mohsin Bashir)
* Modify MGMT_STATUS_CMD_UNSUPPORTED to EOPNOTSUPP for complying with the error code specifications (Mohsin Bashir)
PATCH 03 V05: https://lore.kernel.org/netdev/[email protected]/
* Clear HINIC3_CHANGE_RES_INVALID bit in error handling (AI review)
* Use low >= high to avoid low=high in is_coalesce_legal (AI review)
* As tx and rx share interrupts, we only use ETHTOOL_COALESCE_RX_USECS for user setting to avoid user misunderstanding. So we do not add ETHTOOL_COALESCE_TX_USECS. (Mohsin Bashir & AI review)
PATCH 03 V06: https://lore.kernel.org/netdev/[email protected]/
* Remove redundant rx_jumbo_pending and rx_mini_pending judgement (Jakub Kicinski)
* Remove redundant max tx_pending judgement when .get_ringparam already got the max value (Jakub Kicinski)
* Use extack instead of netdev_err/netdev_info/netdev_warning (Jakub Kicinski)
* Remove HINIC3_CHANNEL_RES_VALID and only use HINIC3_CHANGE_RES_INVALID
bit (Jakub Kicinski)
* Deference freed pointers in hinic3_change_channel_settings error
handling (Jakub Kicinski)
* Modify hinic3_open_channel (Jakub Kicinski)
PATCH 03 V07: https://lore.kernel.org/netdev/[email protected]/
* Remove the trailing '\n' in NL_SET_ERR_MSG* (Jakub Kicinski)
PATCH 03 V08: https://lore.kernel.org/netdev/[email protected]/
* Update patch commit to be more accurate for change information (AI review)
* Modify <net/devlink.h> to <linux/netlink.h> (AI review)
* Use sq/rq_depth instead of rx/txqs[0].q_depth (AI review)
* hinic3_change_channel_settings() runs under change_res_mutex
to protect dynamic channel updates (AI review)
* hinic3_close() serializes close and channel reconfiguration paths (AI review)
* Remove useless u64_stats_init in hinic3_get_drv_queue_stats() (AI review)
* hinic3_get_drv_queue_stats() fills rxq stats at correct idx
when txq is null (AI review)
* Remove unnecessary semicolons after the closing bracket of
static inline functions (AI review)
* Disable preemption in hinic3_rx_fill_buffers() (AI review)
* Add spinlock to protect per-queue coalesce parameters from
concurrent access (AI review)
* Reject static coalesce config when adaptive RX coalesce is
enabled (AI review)
* Validate interface state and queue ID before modifying coalesce parameters (AI review)
* Differentiate get_coalesce output for adaptive/static modes and zero-fill the struct (AI review)
* Return -ERANGE for invalid coalesce frame limits and simplify
error message (AI review)
* Remove unused watchdog timeout definition and assignment (AI review)
* Return -EOPNOTSUPP when RSS type query is unsupported by firmware (AI review)
* Reject L4 hash bits for pure L3 flow types (AI review)
* Split RSS type conversion to correctly handle L3-only flows (AI review)
* Reprogram RSS indir table and HW parameters after channel count changes (AI review)
* Propagate exact error code from channel parameter validation (AI review)
* Update SW RSS indir copy only after HW programming succeeds (AI review)
* Program RSS hash type to HW before updating SW state to prevent inconsistency (AI review)
PATCH 03 V09:
* Remove useless messages in hinic3_set_ringparam() (Jakub Kicinski)
* Solve the big-endian and little-endian issue (Jakub Kicinski)
Fan Gong (5):
hinic3: Add ethtool queue ops
hinic3: Add ethtool statistic ops
hinic3: Add ethtool coalesce ops
hinic3: Add ethtool rss ops
hinic3: Remove unneeded coalesce parameters
.../ethernet/huawei/hinic3/hinic3_ethtool.c | 834 +++++++++++++++++- .../ethernet/huawei/hinic3/hinic3_hw_intf.h | 11 + .../net/ethernet/huawei/hinic3/hinic3_irq.c | 14 +- .../net/ethernet/huawei/hinic3/hinic3_main.c | 7 + .../huawei/hinic3/hinic3_mgmt_interface.h | 39 + .../huawei/hinic3/hinic3_netdev_ops.c | 104 ++- .../ethernet/huawei/hinic3/hinic3_nic_cfg.c | 64 ++ .../ethernet/huawei/hinic3/hinic3_nic_cfg.h | 109 +++ .../ethernet/huawei/hinic3/hinic3_nic_dev.h | 11 + .../ethernet/huawei/hinic3/hinic3_nic_io.c | 4 +- .../ethernet/huawei/hinic3/hinic3_nic_io.h | 8 +- .../net/ethernet/huawei/hinic3/hinic3_rss.c | 539 ++++++++++- .../net/ethernet/huawei/hinic3/hinic3_rss.h | 19 + .../net/ethernet/huawei/hinic3/hinic3_rx.c | 72 +- .../net/ethernet/huawei/hinic3/hinic3_rx.h | 18 +- .../net/ethernet/huawei/hinic3/hinic3_tx.c | 62 +- .../net/ethernet/huawei/hinic3/hinic3_tx.h | 2 + 17 files changed, 1883 insertions(+), 34 deletions(-)
base-commit: 903db046d5579bef0ea699eae4b279dd6455fc9f
Signature · 1 line
2.43.0
Fan Gong
maintainerPATCH v9 1/5 hinic3: Add ethtool queue ops
Implement following ethtool callback function:
.get_ringparam
.set_ringparam
These callbacks allow users to utilize ethtool for detailed
queue depth configuration and monitoring.
Co-developed-by Wu Di <[email protected]>
Signed-off-by Wu Di <[email protected]>
Co-developed-by Teng Peisen <[email protected]>
Signed-off-by Teng Peisen <[email protected]>
Signed-off-by Fan Gong <[email protected]>
--- .../ethernet/huawei/hinic3/hinic3_ethtool.c | 93 ++++++++++++++++ .../net/ethernet/huawei/hinic3/hinic3_irq.c | 5 +- .../net/ethernet/huawei/hinic3/hinic3_main.c | 6 + .../huawei/hinic3/hinic3_netdev_ops.c | 104 ++++++++++++++++-- .../ethernet/huawei/hinic3/hinic3_nic_dev.h | 9 ++ .../ethernet/huawei/hinic3/hinic3_nic_io.c | 4 +- .../ethernet/huawei/hinic3/hinic3_nic_io.h | 8 +- .../net/ethernet/huawei/hinic3/hinic3_rx.c | 2 +- 8 files changed, 217 insertions(+), 14 deletions(-)
drivers/net/ethernet/huawei/hinic3/hinic3_ethtool.c +93 -0
@@ -9,6 +9,7 @@#include <linux/errno.h> #include <linux/etherdevice.h> #include <linux/netdevice.h> +#include <linux/netlink.h> #include <linux/ethtool.h> #include "hinic3_lld.h"
@@ -409,6 +410,96 @@ hinic3_get_link_ksettings(struct net_device *netdev,return 0 ; } +static void hinic3_get_ringparam (struct net_device *netdev, +struct ethtool_ringparam *ring, +struct kernel_ethtool_ringparam *kernel_ring, +struct netlink_ext_ack *extack) +{ +struct hinic3_nic_dev *nic_dev =netdev_priv (netdev); + + ring->rx_max_pending =HINIC3_MAX_RX_QUEUE_DEPTH ; + ring->tx_max_pending =HINIC3_MAX_TX_QUEUE_DEPTH ; + ring->rx_pending = nic_dev->q_params.rq_depth; + ring->rx_pending = nic_dev->q_params.sq_depth; +} + +static void hinic3_update_qp_depth (struct net_device *netdev, +u32 sq_depth,u32 rq_depth) +{ +struct hinic3_nic_dev *nic_dev =netdev_priv (netdev); +u16 i; + + nic_dev->q_params.sq_depth = sq_depth; + nic_dev->q_params.rq_depth = rq_depth; +for (i =0 ; i < nic_dev->max_qps; i++) { + nic_dev->txqs[i].q_depth = sq_depth; + nic_dev->txqs[i].q_mask = sq_depth -1 ; + nic_dev->rxqs[i].q_depth = rq_depth; + nic_dev->rxqs[i].q_mask = rq_depth -1 ; + } +} + +static int hinic3_check_ringparam_valid (struct net_device *netdev, +const struct ethtool_ringparam *ring, +struct netlink_ext_ack *extack) +{ +if (ring->tx_pending <HINIC3_MIN_QUEUE_DEPTH || + ring->rx_pending <HINIC3_MIN_QUEUE_DEPTH ) { +NL_SET_ERR_MSG_FMT_MOD (extack, +"Queue depth out of range tx[%d-%d] rx[%d-%d]" , +HINIC3_MIN_QUEUE_DEPTH , +HINIC3_MAX_TX_QUEUE_DEPTH , +HINIC3_MIN_QUEUE_DEPTH , +HINIC3_MAX_RX_QUEUE_DEPTH ); + +return -EINVAL ; + } + +return 0 ; +} + +static int hinic3_set_ringparam (struct net_device *netdev, +struct ethtool_ringparam *ring, +struct kernel_ethtool_ringparam *kernel_ring, +struct netlink_ext_ack *extack) +{ +struct hinic3_nic_dev *nic_dev =netdev_priv (netdev); +struct hinic3_dyna_txrxq_params q_params = {}; +u32 new_sq_depth, new_rq_depth; +int err; + + err =hinic3_check_ringparam_valid (netdev, ring, extack); +if (err) +return err; + + new_sq_depth =1U <<ilog2 (ring->tx_pending); + new_rq_depth =1U <<ilog2 (ring->rx_pending); +if (new_sq_depth == nic_dev->q_params.sq_depth && + new_rq_depth == nic_dev->q_params.rq_depth) +return 0 ; + +if (new_sq_depth != ring->tx_pending || + new_rq_depth != ring->rx_pending) +NL_SET_ERR_MSG_FMT_MOD (extack, +"Requested Tx/Rx ring depth %u/%u trimmed to %u/%u" , + ring->tx_pending, ring->rx_pending, + new_sq_depth, new_rq_depth); + +if (!netif_running (netdev)) { +hinic3_update_qp_depth (netdev, new_sq_depth, new_rq_depth); + }else { + q_params = nic_dev->q_params; + q_params.sq_depth = new_sq_depth; + q_params.rq_depth = new_rq_depth; + + err =hinic3_change_channel_settings (netdev, &q_params); +if (err) +return err; + } + +return 0 ; +} +static const struct ethtool_ops hinic3_ethtool_ops = { .supported_coalesce_params =ETHTOOL_COALESCE_USECS |ETHTOOL_COALESCE_PKT_RATE_RX_USECS ,
@@ -417,6 +508,8 @@ static const struct ethtool_ops hinic3_ethtool_ops = { .get_msglevel = hinic3_get_msglevel, .set_msglevel = hinic3_set_msglevel, .get_link = ethtool_op_get_link, + .get_ringparam = hinic3_get_ringparam, + .set_ringparam = hinic3_set_ringparam, };void hinic3_set_ethtool_ops (struct net_device *netdev)
drivers/net/ethernet/huawei/hinic3/hinic3_irq.c +4 -1
@@ -137,7 +137,8 @@ static int hinic3_set_interrupt_moder(struct net_device *netdev, u16 q_id,struct hinic3_interrupt_info info = {};int err; -if (q_id >= nic_dev->q_params.num_qps) +if (q_id >= nic_dev->q_params.num_qps || + !mutex_trylock (&nic_dev->change_res_mutex))return 0 ; info.interrupt_coalesc_set =1 ;
@@ -156,6 +157,8 @@ static int hinic3_set_interrupt_moder(struct net_device *netdev, u16 q_id, nic_dev->rxqs[q_id].last_pending_limit = pending_limit; } +mutex_unlock (&nic_dev->change_res_mutex); +return err; }
drivers/net/ethernet/huawei/hinic3/hinic3_main.c +6 -0
@@ -179,6 +179,7 @@ static int hinic3_sw_init(struct net_device *netdev)int err;mutex_init (&nic_dev->port_state_mutex); +mutex_init (&nic_dev->change_res_mutex); nic_dev->q_params.sq_depth =HINIC3_SQ_DEPTH ; nic_dev->q_params.rq_depth =HINIC3_RQ_DEPTH ;
@@ -315,6 +316,9 @@ static void hinic3_link_status_change(struct net_device *netdev, {struct hinic3_nic_dev *nic_dev =netdev_priv (netdev); +if (!mutex_trylock (&nic_dev->change_res_mutex)) +return ; +if (link_status_up) {if (netif_carrier_ok (netdev))return ;
@@ -330,6 +334,8 @@ static void hinic3_link_status_change(struct net_device *netdev,netif_carrier_off (netdev);netdev_dbg (netdev,"Link is down\n" ); } + +mutex_unlock (&nic_dev->change_res_mutex); } static void hinic3_port_module_event_handler(struct net_device *netdev,
drivers/net/ethernet/huawei/hinic3/hinic3_netdev_ops.c +96 -8
156 lines not on this page · raw message
drivers/net/ethernet/huawei/hinic3/hinic3_nic_dev.h +9 -0
@@ -10,6 +10,9 @@#include "hinic3_hw_cfg.h" #include "hinic3_hwdev.h" #include "hinic3_mgmt_interface.h" +#include "hinic3_nic_io.h" +#include "hinic3_tx.h" +#include "hinic3_rx.h" #define HINIC3_VLAN_BITMAP_BYTE_SIZE (nic_dev) (sizeof(*(nic_dev)->vlan_bitmap))#define HINIC3_VLAN_BITMAP_SIZE (nic_dev) \
@@ -129,6 +132,8 @@ struct hinic3_nic_dev {struct work_struct rx_mode_work;/* lock for enable/disable port */ struct mutex port_state_mutex; +/* mutex to serialize channel/resource changes */ +struct mutex change_res_mutex;struct list_head uc_filter_list;struct list_head mc_filter_list;
@@ -143,6 +148,10 @@ struct hinic3_nic_dev {void hinic3_set_netdev_ops (struct net_device *netdev);int hinic3_set_hw_features (struct net_device *netdev); +int +hinic3_change_channel_settings (struct net_device *netdev, +struct hinic3_dyna_txrxq_params *trxq_params); +int hinic3_qps_irq_init (struct net_device *netdev);void hinic3_qps_irq_uninit (struct net_device *netdev);
drivers/net/ethernet/huawei/hinic3/hinic3_nic_io.c +2 -2
@@ -484,8 +484,8 @@ void hinic3_init_qps(struct hinic3_nic_dev *nic_dev, } } -void hinic3_uninit_qps (struct hinic3_nic_dev *nic_dev, -struct hinic3_dyna_qp_params *qp_params) +void hinic3_get_cur_qps (struct hinic3_nic_dev *nic_dev, +struct hinic3_dyna_qp_params *qp_params) {struct hinic3_nic_io *nic_io = nic_dev->nic_io;
drivers/net/ethernet/huawei/hinic3/hinic3_nic_io.h +6 -2
@@ -14,6 +14,10 @@ struct hinic3_nic_dev;#define HINIC3_RQ_WQEBB_SHIFT 3#define HINIC3_SQ_WQEBB_SIZE BIT(HINIC3_SQ_WQEBB_SHIFT) +#define HINIC3_MAX_TX_QUEUE_DEPTH 65536 +#define HINIC3_MAX_RX_QUEUE_DEPTH 16384 +#define HINIC3_MIN_QUEUE_DEPTH 128 +/* ******************** RQ_CTRL ******************** */ enum hinic3_rq_wqe_type {HINIC3_NORMAL_RQ_WQE =1 ,
@@ -136,8 +140,8 @@ void hinic3_free_qps(struct hinic3_nic_dev *nic_dev, struct hinic3_dyna_qp_params *qp_params);void hinic3_init_qps (struct hinic3_nic_dev *nic_dev,struct hinic3_dyna_qp_params *qp_params); -void hinic3_uninit_qps (struct hinic3_nic_dev *nic_dev, -struct hinic3_dyna_qp_params *qp_params); +void hinic3_get_cur_qps (struct hinic3_nic_dev *nic_dev, +struct hinic3_dyna_qp_params *qp_params);int hinic3_init_qp_ctxts (struct hinic3_nic_dev *nic_dev);void hinic3_free_qp_ctxts (struct hinic3_nic_dev *nic_dev);
drivers/net/ethernet/huawei/hinic3/hinic3_rx.c +1 -1
@@ -541,7 +541,7 @@ int hinic3_configure_rxqs(struct net_device *netdev, u16 num_rq,rq_associate_cqes (rxq); pkts =hinic3_rx_fill_buffers (rxq); -if (!pkts) { +if (pkts < rxq->q_depth -1 ) {netdev_err (netdev,"Failed to fill Rx buffer\n" );return -ENOMEM ; }
Signature · 1 line
2.43.0
Re: [PATCH net-next v09 1/5] hinic3: Add ethtool queue ops
Quoted 177 lines · Fan Gong
Implement following ethtool callback function:
.get_ringparam
.set_ringparamThese callbacks allow users to utilize ethtool for detailed
queue depth configuration and monitoring.Co-developed-by Wu Di <[email protected]>Signed-off-by Wu Di <[email protected]>Co-developed-by Teng Peisen <[email protected]>Signed-off-by Teng Peisen <[email protected]>Signed-off-by Fan Gong <[email protected]>--- .../ethernet/huawei/hinic3/hinic3_ethtool.c | 93 ++++++++++++++++ .../net/ethernet/huawei/hinic3/hinic3_irq.c | 5 +- .../net/ethernet/huawei/hinic3/hinic3_main.c | 6 + .../huawei/hinic3/hinic3_netdev_ops.c | 104 ++++++++++++++++-- .../ethernet/huawei/hinic3/hinic3_nic_dev.h | 9 ++ .../ethernet/huawei/hinic3/hinic3_nic_io.c | 4 +- .../ethernet/huawei/hinic3/hinic3_nic_io.h | 8 +- .../net/ethernet/huawei/hinic3/hinic3_rx.c | 2 +- 8 files changed, 217 insertions(+), 14 deletions(-)drivers/net/ethernet/huawei/hinic3/hinic3_ethtool.c +93
-0@@ -9,6 +9,7 @@#include <linux/errno.h> #include <linux/etherdevice.h> #include <linux/netdevice.h> +#include <linux/netlink.h> #include <linux/ethtool.h> #include "hinic3_lld.h" @@ -409,6 +410,96 @@ hinic3_get_link_ksettings(struct net_device *netdev,return 0 ; } +static void hinic3_get_ringparam (struct net_device *netdev, +struct ethtool_ringparam *ring, +struct kernel_ethtool_ringparam *kernel_ring, +struct netlink_ext_ack *extack) +{ +struct hinic3_nic_dev *nic_dev =netdev_priv (netdev); + + ring->rx_max_pending =HINIC3_MAX_RX_QUEUE_DEPTH ; + ring->tx_max_pending =HINIC3_MAX_TX_QUEUE_DEPTH ; + ring->rx_pending = nic_dev->q_params.rq_depth; + ring->rx_pending = nic_dev->q_params.sq_depth; +} + +static void hinic3_update_qp_depth (struct net_device *netdev, +u32 sq_depth,u32 rq_depth) +{ +struct hinic3_nic_dev *nic_dev =netdev_priv (netdev); +u16 i; + + nic_dev->q_params.sq_depth = sq_depth; + nic_dev->q_params.rq_depth = rq_depth; +for (i =0 ; i < nic_dev->max_qps; i++) { + nic_dev->txqs[i].q_depth = sq_depth; + nic_dev->txqs[i].q_mask = sq_depth -1 ; + nic_dev->rxqs[i].q_depth = rq_depth; + nic_dev->rxqs[i].q_mask = rq_depth -1 ; + } +} + +static int hinic3_check_ringparam_valid (struct net_device *netdev, +const struct ethtool_ringparam *ring, +struct netlink_ext_ack *extack) +{ +if (ring->tx_pending <HINIC3_MIN_QUEUE_DEPTH || + ring->rx_pending <HINIC3_MIN_QUEUE_DEPTH ) { +NL_SET_ERR_MSG_FMT_MOD (extack, +"Queue depth out of range tx[%d-%d] rx[%d-%d]" , +HINIC3_MIN_QUEUE_DEPTH , +HINIC3_MAX_TX_QUEUE_DEPTH , +HINIC3_MIN_QUEUE_DEPTH , +HINIC3_MAX_RX_QUEUE_DEPTH ); + +return -EINVAL ; + } + +return 0 ; +} + +static int hinic3_set_ringparam (struct net_device *netdev, +struct ethtool_ringparam *ring, +struct kernel_ethtool_ringparam *kernel_ring, +struct netlink_ext_ack *extack) +{ +struct hinic3_nic_dev *nic_dev =netdev_priv (netdev); +struct hinic3_dyna_txrxq_params q_params = {}; +u32 new_sq_depth, new_rq_depth; +int err; + + err =hinic3_check_ringparam_valid (netdev, ring, extack); +if (err) +return err; + + new_sq_depth =1U <<ilog2 (ring->tx_pending); + new_rq_depth =1U <<ilog2 (ring->rx_pending); +if (new_sq_depth == nic_dev->q_params.sq_depth && + new_rq_depth == nic_dev->q_params.rq_depth) +return 0 ; + +if (new_sq_depth != ring->tx_pending || + new_rq_depth != ring->rx_pending) +NL_SET_ERR_MSG_FMT_MOD (extack, +"Requested Tx/Rx ring depth %u/%u trimmed to %u/%u" , + ring->tx_pending, ring->rx_pending, + new_sq_depth, new_rq_depth); + +if (!netif_running (netdev)) { +hinic3_update_qp_depth (netdev, new_sq_depth, new_rq_depth); + }else { + q_params = nic_dev->q_params; + q_params.sq_depth = new_sq_depth; + q_params.rq_depth = new_rq_depth; + + err =hinic3_change_channel_settings (netdev, &q_params); +if (err) +return err; + } + +return 0 ; +} +static const struct ethtool_ops hinic3_ethtool_ops = { .supported_coalesce_params =ETHTOOL_COALESCE_USECS |ETHTOOL_COALESCE_PKT_RATE_RX_USECS ,@@ -417,6 +508,8 @@ static const struct ethtool_ops hinic3_ethtool_ops = { .get_msglevel = hinic3_get_msglevel, .set_msglevel = hinic3_set_msglevel, .get_link = ethtool_op_get_link, + .get_ringparam = hinic3_get_ringparam, + .set_ringparam = hinic3_set_ringparam, };void hinic3_set_ethtool_ops (struct net_device *netdev)drivers/net/ethernet/huawei/hinic3/hinic3_irq.c +4
-1@@ -137,7 +137,8 @@ static int hinic3_set_interrupt_moder(struct net_device *netdev, u16 q_id,struct hinic3_interrupt_info info = {};int err; -if (q_id >= nic_dev->q_params.num_qps) +if (q_id >= nic_dev->q_params.num_qps || + !mutex_trylock (&nic_dev->change_res_mutex))return 0 ; info.interrupt_coalesc_set =1 ;@@ -156,6 +157,8 @@ static int hinic3_set_interrupt_moder(struct net_device *netdev, u16 q_id, nic_dev->rxqs[q_id].last_pending_limit = pending_limit; } +mutex_unlock (&nic_dev->change_res_mutex); +return err; }drivers/net/ethernet/huawei/hinic3/hinic3_main.c +1
-0@@ -179,6 +179,7 @@ static int hinic3_sw_init(struct net_device *netdev)int err;mutex_init (&nic_dev->port_state_mutex); +mutex_init (&nic_dev->change_res_mutex); nic_dev->q_params.sq_depth =HINIC3_SQ_DEPTH ; nic_dev->q_params.rq_depth =HINIC3_RQ_DEPTH ;
drivers/net/ethernet/huawei/hinic3/hinic3_main.c static void hinic3_link_status_change(struct net_device *netdev, · line 324
Quoted 9 lines
drivers/net/ethernet/huawei/hinic3/hinic3_main.c +3
-0@@ -315,6 +316,9 @@ static void hinic3_link_status_change(struct net_device *netdev, {struct hinic3_nic_dev *nic_dev =netdev_priv (netdev); +if (!mutex_trylock (&nic_dev->change_res_mutex)) +return ; +if (link_status_up) {if (netif_carrier_ok (netdev))return ;
There's a couple returns in this function that will cause the lock to never be released. Probably need a goto unlock.
Quoted 256 lines
drivers/net/ethernet/huawei/hinic3/hinic3_main.c +2
-0@@ -330,6 +334,8 @@ static void hinic3_link_status_change(struct net_device *netdev,netif_carrier_off (netdev);netdev_dbg (netdev,"Link is down\n" ); } + +mutex_unlock (&nic_dev->change_res_mutex); } static void hinic3_port_module_event_handler(struct net_device *netdev,drivers/net/ethernet/huawei/hinic3/hinic3_netdev_ops.c +96
-8@@ -288,7 +288,8 @@ static void hinic3_free_channel_resources(struct net_device *netdev,hinic3_free_qps (nic_dev, qp_params); } -static int hinic3_open_channel (struct net_device *netdev) +static int hinic3_prepare_channel (struct net_device *netdev, +struct hinic3_dyna_txrxq_params *qp_params) {struct hinic3_nic_dev *nic_dev =netdev_priv (netdev);int err;@@ -299,16 +300,28 @@ static int hinic3_open_channel(struct net_device *netdev)return err; } - err =hinic3_configure_txrxqs (netdev, &nic_dev->q_params); + err =hinic3_configure_txrxqs (netdev, qp_params);if (err) {netdev_err (netdev,"Failed to configure txrxqs\n" ); goto err_free_qp_ctxts; } +return 0 ; + +err_free_qp_ctxts: +hinic3_free_qp_ctxts (nic_dev); + +return err; +} + +static int hinic3_open_channel (struct net_device *netdev) +{ +int err; + err =hinic3_qps_irq_init (netdev);if (err) {netdev_err (netdev,"Failed to init txrxq irq\n" ); - goto err_free_qp_ctxts; +return err; } err =hinic3_configure (netdev);@@ -321,8 +334,6 @@ static int hinic3_open_channel(struct net_device *netdev) err_uninit_qps_irq:hinic3_qps_irq_uninit (netdev); -err_free_qp_ctxts: -hinic3_free_qp_ctxts (nic_dev);return err; }@@ -428,6 +439,74 @@ static void hinic3_vport_down(struct net_device *netdev) } } +int +hinic3_change_channel_settings (struct net_device *netdev, +struct hinic3_dyna_txrxq_params *trxq_params) +{ +struct hinic3_nic_dev *nic_dev =netdev_priv (netdev); +struct hinic3_dyna_txrxq_params cur_trxq_params = {}; +struct hinic3_dyna_qp_params new_qp_params = {}; +struct hinic3_dyna_qp_params cur_qp_params = {}; +int err; + + cur_trxq_params = nic_dev->q_params; + +hinic3_config_num_qps (netdev, trxq_params); + + err =hinic3_alloc_channel_resources (netdev, &new_qp_params, + trxq_params); +if (err) { +netdev_err (netdev,"Failed to alloc channel resources\n" ); +return err; + } + +mutex_lock (&nic_dev->change_res_mutex); +hinic3_vport_down (netdev); +hinic3_close_channel (netdev); +hinic3_get_cur_qps (nic_dev, &cur_qp_params); + +hinic3_init_qps (nic_dev, &new_qp_params); + + err =hinic3_prepare_channel (netdev, trxq_params); +if (err) + goto err_uninit_qps; + +if (nic_dev->num_qp_irq > trxq_params->num_qps) +hinic3_qp_irq_change (netdev, trxq_params->num_qps); + + nic_dev->q_params = *trxq_params; + + err =hinic3_open_channel (netdev); +if (err) + goto err_qp_irq_reset; + + err =hinic3_vport_up (netdev); +if (err) + goto err_close_channel; + +hinic3_free_channel_resources (netdev, &cur_qp_params, &cur_trxq_params); + +mutex_unlock (&nic_dev->change_res_mutex); + +return 0 ; + +err_close_channel: +hinic3_close_channel (netdev); +err_qp_irq_reset: + nic_dev->q_params = cur_trxq_params; + +if (trxq_params->num_qps > cur_trxq_params.num_qps) +hinic3_qp_irq_change (netdev, cur_trxq_params.num_qps); +hinic3_free_qp_ctxts (nic_dev); +err_uninit_qps: +hinic3_get_cur_qps (nic_dev, &new_qp_params); +hinic3_free_channel_resources (netdev, &new_qp_params, trxq_params); +hinic3_free_channel_resources (netdev, &cur_qp_params, &cur_trxq_params); +mutex_unlock (&nic_dev->change_res_mutex); + +return err; +} +static int hinic3_open (struct net_device *netdev) {struct hinic3_nic_dev *nic_dev =netdev_priv (netdev);@@ -458,6 +537,10 @@ static int hinic3_open(struct net_device *netdev)hinic3_init_qps (nic_dev, &qp_params); + err =hinic3_prepare_channel (netdev, &nic_dev->q_params); +if (err) + goto err_uninit_qps; + err =hinic3_open_channel (netdev);if (err) goto err_uninit_qps;@@ -473,7 +556,7 @@ static int hinic3_open(struct net_device *netdev) err_close_channel:hinic3_close_channel (netdev); err_uninit_qps: -hinic3_uninit_qps (nic_dev, &qp_params); +hinic3_get_cur_qps (nic_dev, &qp_params);hinic3_free_channel_resources (netdev, &qp_params, &nic_dev->q_params); err_destroy_num_qps:hinic3_destroy_num_qps (netdev);@@ -493,10 +576,15 @@ static int hinic3_close(struct net_device *netdev)return 0 ; } +mutex_lock (&nic_dev->change_res_mutex);hinic3_vport_down (netdev);hinic3_close_channel (netdev); -hinic3_uninit_qps (nic_dev, &qp_params); -hinic3_free_channel_resources (netdev, &qp_params, &nic_dev->q_params); +hinic3_get_cur_qps (nic_dev, &qp_params); +hinic3_free_channel_resources (netdev, &qp_params, + &nic_dev->q_params); +hinic3_free_nicio_res (nic_dev); +hinic3_destroy_num_qps (netdev); +mutex_unlock (&nic_dev->change_res_mutex);return 0 ; }drivers/net/ethernet/huawei/hinic3/hinic3_nic_dev.h +9
-0@@ -10,6 +10,9 @@#include "hinic3_hw_cfg.h" #include "hinic3_hwdev.h" #include "hinic3_mgmt_interface.h" +#include "hinic3_nic_io.h" +#include "hinic3_tx.h" +#include "hinic3_rx.h" #define HINIC3_VLAN_BITMAP_BYTE_SIZE (nic_dev) (sizeof(*(nic_dev)->vlan_bitmap))#define HINIC3_VLAN_BITMAP_SIZE (nic_dev) \@@ -129,6 +132,8 @@ struct hinic3_nic_dev {struct work_struct rx_mode_work;/* lock for enable/disable port */ struct mutex port_state_mutex; +/* mutex to serialize channel/resource changes */ +struct mutex change_res_mutex;struct list_head uc_filter_list;struct list_head mc_filter_list;@@ -143,6 +148,10 @@ struct hinic3_nic_dev {void hinic3_set_netdev_ops (struct net_device *netdev);int hinic3_set_hw_features (struct net_device *netdev); +int +hinic3_change_channel_settings (struct net_device *netdev, +struct hinic3_dyna_txrxq_params *trxq_params); +int hinic3_qps_irq_init (struct net_device *netdev);void hinic3_qps_irq_uninit (struct net_device *netdev);drivers/net/ethernet/huawei/hinic3/hinic3_nic_io.c +2
-2@@ -484,8 +484,8 @@ void hinic3_init_qps(struct hinic3_nic_dev *nic_dev, } } -void hinic3_uninit_qps (struct hinic3_nic_dev *nic_dev, -struct hinic3_dyna_qp_params *qp_params) +void hinic3_get_cur_qps (struct hinic3_nic_dev *nic_dev, +struct hinic3_dyna_qp_params *qp_params) {struct hinic3_nic_io *nic_io = nic_dev->nic_io;drivers/net/ethernet/huawei/hinic3/hinic3_nic_io.h +6
-2@@ -14,6 +14,10 @@ struct hinic3_nic_dev;#define HINIC3_RQ_WQEBB_SHIFT 3#define HINIC3_SQ_WQEBB_SIZE BIT(HINIC3_SQ_WQEBB_SHIFT) +#define HINIC3_MAX_TX_QUEUE_DEPTH 65536 +#define HINIC3_MAX_RX_QUEUE_DEPTH 16384 +#define HINIC3_MIN_QUEUE_DEPTH 128 +/* ******************** RQ_CTRL ******************** */ enum hinic3_rq_wqe_type {HINIC3_NORMAL_RQ_WQE =1 ,@@ -136,8 +140,8 @@ void hinic3_free_qps(struct hinic3_nic_dev *nic_dev, struct hinic3_dyna_qp_params *qp_params);void hinic3_init_qps (struct hinic3_nic_dev *nic_dev,struct hinic3_dyna_qp_params *qp_params); -void hinic3_uninit_qps (struct hinic3_nic_dev *nic_dev, -struct hinic3_dyna_qp_params *qp_params); +void hinic3_get_cur_qps (struct hinic3_nic_dev *nic_dev, +struct hinic3_dyna_qp_params *qp_params);int hinic3_init_qp_ctxts (struct hinic3_nic_dev *nic_dev);void hinic3_free_qp_ctxts (struct hinic3_nic_dev *nic_dev);drivers/net/ethernet/huawei/hinic3/hinic3_rx.c +0
-0
drivers/net/ethernet/huawei/hinic3/hinic3_rx.c int hinic3_configure_rxqs(struct net_device *netdev, u16 num_rq, · line 544
Quoted 5 lines
drivers/net/ethernet/huawei/hinic3/hinic3_rx.c +1
-1@@ -541,7 +541,7 @@ int hinic3_configure_rxqs(struct net_device *netdev, u16 num_rq,rq_associate_cqes (rxq); pkts =hinic3_rx_fill_buffers (rxq); -if (!pkts) { +if (pkts < rxq->q_depth -1 ) {
nit: just use rxq->q_mask?
Quoted 3 lines
drivers/net/ethernet/huawei/hinic3/hinic3_rx.c +0
-0netdev_err (netdev,"Failed to fill Rx buffer\n" );return -ENOMEM ; }
Simon Horman
reviewerRe: [PATCH net-next v09 1/5] hinic3: Add ethtool queue ops
In reply to Fan GongThis is an AI-generated review of your patch. The human sending this email has considered the AI review valid, or at least plausible. Full review at: https://sashiko.dev
---
hinic3: Add ethtool queue ops
This commit implements ethtool callback functions .get_ringparam and .set_ringparam. These callbacks allow users to utilize ethtool for detailed queue depth configuration and monitoring.
Quoted 4 lines
drivers/net/ethernet/huawei/hinic3/hinic3_ethtool.c +0
-0
[ ... ]
Quoted 5 lines
drivers/net/ethernet/huawei/hinic3/hinic3_ethtool.c +1
-0@@ -409,6 +410,96 @@ hinic3_get_link_ksettings(struct net_device *netdev,return 0 ; } +static void hinic3_get_ringparam(struct net_device *netdev,
drivers/net/ethernet/huawei/hinic3/hinic3_ethtool.c hinic3_get_link_ksettings(struct net_device *netdev, · line 423
Quoted 10 lines
drivers/net/ethernet/huawei/hinic3/hinic3_ethtool.c +10
-0+struct ethtool_ringparam *ring, + struct kernel_ethtool_ringparam *kernel_ring, + struct netlink_ext_ack *extack) +{ + struct hinic3_nic_dev *nic_dev = netdev_priv(netdev); + + ring->rx_max_pending =HINIC3_MAX_RX_QUEUE_DEPTH ; + ring->tx_max_pending =HINIC3_MAX_TX_QUEUE_DEPTH ; + ring->rx_pending = nic_dev->q_params.rq_depth; + ring->rx_pending = nic_dev->q_params.sq_depth;
[Severity: High]
Does this code accidentally assign rx_pending twice and omit tx_pending? Querying the interface will return 0 for the Tx queue depth and an incorrect value for the Rx queue depth, which also breaks partial ring updates.
Quoted 2 lines
drivers/net/ethernet/huawei/hinic3/hinic3_ethtool.c +2
-0+} +
[ ... ]
drivers/net/ethernet/huawei/hinic3/hinic3_irq.c static int hinic3_set_interrupt_moder(struct net_device *netdev, u16 q_id, · line 142
Quoted 12 lines
drivers/net/ethernet/huawei/hinic3/hinic3_irq.c +2
-1@@ -137,7 +137,8 @@ static int hinic3_set_interrupt_moder(struct net_device *netdev, u16 q_id,struct hinic3_interrupt_info info = {};int err; -if (q_id >= nic_dev->q_params.num_qps) +if (q_id >= nic_dev->q_params.num_qps || + !mutex_trylock (&nic_dev->change_res_mutex))return 0 ;
[Severity: High]
Is it safe to silently return 0 (success) without applying the interrupt moderation if the mutex is contended? This appears to drop hardware configuration updates without informing the caller.
Quoted 2 lines
drivers/net/ethernet/huawei/hinic3/hinic3_irq.c +0
-0info.interrupt_coalesc_set =1 ;
[ ... ]
Quoted 4 lines
drivers/net/ethernet/huawei/hinic3/hinic3_main.c +0
-0
[ ... ]
drivers/net/ethernet/huawei/hinic3/hinic3_main.c static void hinic3_link_status_change(struct net_device *netdev, · line 321
Quoted 7 lines
drivers/net/ethernet/huawei/hinic3/hinic3_main.c +3
-0@@ -315,6 +316,9 @@ static void hinic3_link_status_change(struct net_device *netdev, {struct hinic3_nic_dev *nic_dev =netdev_priv (netdev); +if (!mutex_trylock (&nic_dev->change_res_mutex)) +return ; +
[Severity: High]
Can this usage of mutex_trylock cause physical link status events to be discarded? If the lock is held (e.g., during an ethtool ring size change), the OS link state could permanently diverge from the physical link state.
drivers/net/ethernet/huawei/hinic3/hinic3_main.c static void hinic3_link_status_change(struct net_device *netdev, · line 324
Quoted 3 lines
drivers/net/ethernet/huawei/hinic3/hinic3_main.c +0
-0if (link_status_up) {if (netif_carrier_ok (netdev))return ;
[Severity: Critical]
Does this early return path leak the mutex? We acquired change_res_mutex earlier, but it looks like we exit without unlocking it, which would cause subsequent deadlocks.
drivers/net/ethernet/huawei/hinic3/hinic3_main.c static void hinic3_link_status_change(struct net_device *netdev, · line 339
Quoted 7 lines
drivers/net/ethernet/huawei/hinic3/hinic3_main.c +2
-0@@ -330,6 +334,8 @@ static void hinic3_link_status_change(struct net_device *netdev,netif_carrier_off (netdev);netdev_dbg (netdev,"Link is down\n" ); } + +mutex_unlock (&nic_dev->change_res_mutex); }
[Severity: Critical]
There is another early return just above this code:
if (!netif_carrier_ok(netdev))
return;Similarly, does this early return also forget to call mutex_unlock()?
Quoted 6 lines
drivers/net/ethernet/huawei/hinic3/hinic3_main.c +0
-0static void hinic3_port_module_event_handler(struct net_device *netdev,drivers/net/ethernet/huawei/hinic3/hinic3_netdev_ops.c +0
-0
[ ... ]
Quoted 8 lines
drivers/net/ethernet/huawei/hinic3/hinic3_netdev_ops.c +4
-0@@ -428,6 +439,74 @@ static void hinic3_vport_down(struct net_device *netdev) } } +int +hinic3_change_channel_settings (struct net_device *netdev, +struct hinic3_dyna_txrxq_params *trxq_params) +{
[ ... ]
Quoted 8 lines
drivers/net/ethernet/huawei/hinic3/hinic3_netdev_ops.c +8
-0+ err =hinic3_vport_up (netdev); +if (err) + goto err_close_channel; + +hinic3_free_channel_resources (netdev, &cur_qp_params, &cur_trxq_params); + +mutex_unlock (&nic_dev->change_res_mutex); +
drivers/net/ethernet/huawei/hinic3/hinic3_netdev_ops.c static void hinic3_vport_down(struct net_device *netdev) · line 500
Quoted 10 lines
drivers/net/ethernet/huawei/hinic3/hinic3_netdev_ops.c +10
-0+return 0 ; + +err_close_channel: +hinic3_close_channel (netdev); +err_qp_irq_reset: + nic_dev->q_params = cur_trxq_params; + +if (trxq_params->num_qps > cur_trxq_params.num_qps) +hinic3_qp_irq_change (netdev, cur_trxq_params.num_qps); +hinic3_free_qp_ctxts (nic_dev);
[Severity: High]
Is hinic3_free_qp_ctxts() called twice sequentially in this error path? The err_close_channel block calls hinic3_close_channel(), which internally invokes hinic3_free_qp_ctxts(). The flow then falls through to err_qp_irq_reset which explicitly calls hinic3_free_qp_ctxts() again.
drivers/net/ethernet/huawei/hinic3/hinic3_netdev_ops.c static void hinic3_vport_down(struct net_device *netdev) · line 504
Quoted 4 lines
drivers/net/ethernet/huawei/hinic3/hinic3_netdev_ops.c +4
-0+err_uninit_qps: +hinic3_get_cur_qps (nic_dev, &new_qp_params); +hinic3_free_channel_resources (netdev, &new_qp_params, trxq_params); +hinic3_free_channel_resources (netdev, &cur_qp_params, &cur_trxq_params);
[Severity: Critical]
Does this error path destroy both the new and the old working channel resources? By calling hinic3_free_channel_resources() on cur_qp_params, we leave dangling pointers in nic_dev->q_params and nic_io. It seems subsequent packet transmissions would trigger a use-after-free, and closing the interface later would trigger a double-free.
Quoted 9 lines
drivers/net/ethernet/huawei/hinic3/hinic3_netdev_ops.c +5
-0+mutex_unlock (&nic_dev->change_res_mutex); + +return err; +} +static int hinic3_open (struct net_device *netdev) {struct hinic3_nic_dev *nic_dev =netdev_priv (netdev);
drivers/net/ethernet/huawei/hinic3/hinic3_netdev_ops.c static int hinic3_open(struct net_device *netdev) · line 546
Quoted 10 lines
drivers/net/ethernet/huawei/hinic3/hinic3_netdev_ops.c +4
-0@@ -458,6 +537,10 @@ static int hinic3_open(struct net_device *netdev)hinic3_init_qps (nic_dev, &qp_params); + err =hinic3_prepare_channel (netdev, &nic_dev->q_params); +if (err) + goto err_uninit_qps; + err =hinic3_open_channel (netdev);if (err) goto err_uninit_qps;
[Severity: High]
Does jumping to err_uninit_qps leak the QP contexts allocated by hinic3_prepare_channel()?
Since hinic3_prepare_channel() calls hinic3_init_qp_ctxts(), it looks like we bypass the cleanup that happens inside hinic3_close_channel(), which would permanently leak the root context configuration and associated DMA memory.
Re: [PATCH net-next v09 1/5] hinic3: Add ethtool queue ops
In reply to Fan GongQuoted 7 lines · Fan Gong
Implement following ethtool callback function:
.get_ringparam
.set_ringparamThese callbacks allow users to utilize ethtool for detailed
queue depth configuration and monitoring.
The patch adds a new mutex. Would be good to call it out in the commit message.
Quoted 34 lines
Co-developed-by Wu Di <[email protected]>Signed-off-by Wu Di <[email protected]>Co-developed-by Teng Peisen <[email protected]>Signed-off-by Teng Peisen <[email protected]>Signed-off-by Fan Gong <[email protected]>--- .../ethernet/huawei/hinic3/hinic3_ethtool.c | 93 ++++++++++++++++ .../net/ethernet/huawei/hinic3/hinic3_irq.c | 5 +- .../net/ethernet/huawei/hinic3/hinic3_main.c | 6 + .../huawei/hinic3/hinic3_netdev_ops.c | 104 ++++++++++++++++-- .../ethernet/huawei/hinic3/hinic3_nic_dev.h | 9 ++ .../ethernet/huawei/hinic3/hinic3_nic_io.c | 4 +- .../ethernet/huawei/hinic3/hinic3_nic_io.h | 8 +- .../net/ethernet/huawei/hinic3/hinic3_rx.c | 2 +- 8 files changed, 217 insertions(+), 14 deletions(-)drivers/net/ethernet/huawei/hinic3/hinic3_ethtool.c +2
-0@@ -9,6 +9,7 @@#include <linux/errno.h> #include <linux/etherdevice.h> #include <linux/netdevice.h> +#include <linux/netlink.h> #include <linux/ethtool.h> #include "hinic3_lld.h" @@ -409,6 +410,96 @@ hinic3_get_link_ksettings(struct net_device *netdev,return 0 ; } +static void hinic3_get_ringparam(struct net_device *netdev,
drivers/net/ethernet/huawei/hinic3/hinic3_ethtool.c hinic3_get_link_ksettings(struct net_device *netdev, · line 423
Quoted 10 lines
drivers/net/ethernet/huawei/hinic3/hinic3_ethtool.c +10
-0+struct ethtool_ringparam *ring, + struct kernel_ethtool_ringparam *kernel_ring, + struct netlink_ext_ack *extack) +{ + struct hinic3_nic_dev *nic_dev = netdev_priv(netdev); + + ring->rx_max_pending =HINIC3_MAX_RX_QUEUE_DEPTH ; + ring->tx_max_pending =HINIC3_MAX_TX_QUEUE_DEPTH ; + ring->rx_pending = nic_dev->q_params.rq_depth; + ring->rx_pending = nic_dev->q_params.sq_depth;
copy-paste error
Quoted 20 lines
drivers/net/ethernet/huawei/hinic3/hinic3_ethtool.c +20
-0+} + +static void hinic3_update_qp_depth (struct net_device *netdev, +u32 sq_depth,u32 rq_depth) +{ +struct hinic3_nic_dev *nic_dev =netdev_priv (netdev); +u16 i; + + nic_dev->q_params.sq_depth = sq_depth; + nic_dev->q_params.rq_depth = rq_depth; +for (i =0 ; i < nic_dev->max_qps; i++) { + nic_dev->txqs[i].q_depth = sq_depth; + nic_dev->txqs[i].q_mask = sq_depth -1 ; + nic_dev->rxqs[i].q_depth = rq_depth; + nic_dev->rxqs[i].q_mask = rq_depth -1 ; + } +} + +static int hinic3_check_ringparam_valid(struct net_device *netdev, + const struct ethtool_ringparam *ring,
drivers/net/ethernet/huawei/hinic3/hinic3_ethtool.c hinic3_get_link_ksettings(struct net_device *netdev, · line 453
Quoted 10 lines
drivers/net/ethernet/huawei/hinic3/hinic3_ethtool.c +10
-0+ struct netlink_ext_ack *extack) +{ +if (ring->tx_pending <HINIC3_MIN_QUEUE_DEPTH || + ring->rx_pending <HINIC3_MIN_QUEUE_DEPTH ) { +NL_SET_ERR_MSG_FMT_MOD (extack, +"Queue depth out of range tx[%d-%d] rx[%d-%d]" , +HINIC3_MIN_QUEUE_DEPTH , +HINIC3_MAX_TX_QUEUE_DEPTH , +HINIC3_MIN_QUEUE_DEPTH , +HINIC3_MAX_RX_QUEUE_DEPTH );
Consider updating this error message to only call out when the ring sizes are below the minimum supported value - since that's the check introduced here and also since ethtool core will reject any values that are higher than the maximum supported ring sizes.
Quoted 13 lines
drivers/net/ethernet/huawei/hinic3/hinic3_ethtool.c +13
-0+ +return -EINVAL ; + } + +return 0 ; +} + +static int hinic3_set_ringparam (struct net_device *netdev, +struct ethtool_ringparam *ring, +struct kernel_ethtool_ringparam *kernel_ring, +struct netlink_ext_ack *extack) +{ +struct hinic3_nic_dev *nic_dev =netdev_priv (netdev);
drivers/net/ethernet/huawei/hinic3/hinic3_ethtool.c hinic3_get_link_ksettings(struct net_device *netdev, · line 476
Quoted 10 lines
drivers/net/ethernet/huawei/hinic3/hinic3_ethtool.c +10
-0+struct hinic3_dyna_txrxq_params q_params = {}; +u32 new_sq_depth, new_rq_depth; +int err; + + err =hinic3_check_ringparam_valid (netdev, ring, extack); +if (err) +return err; + + new_sq_depth =1U <<ilog2 (ring->tx_pending); + new_rq_depth =1U <<ilog2 (ring->rx_pending);
Why not use rounddown_pow_of_two()? More readable...
Quoted 351 lines
drivers/net/ethernet/huawei/hinic3/hinic3_ethtool.c +28
-0+if (new_sq_depth == nic_dev->q_params.sq_depth && + new_rq_depth == nic_dev->q_params.rq_depth) +return 0 ; + +if (new_sq_depth != ring->tx_pending || + new_rq_depth != ring->rx_pending) +NL_SET_ERR_MSG_FMT_MOD (extack, +"Requested Tx/Rx ring depth %u/%u trimmed to %u/%u" , + ring->tx_pending, ring->rx_pending, + new_sq_depth, new_rq_depth); + +if (!netif_running (netdev)) { +hinic3_update_qp_depth (netdev, new_sq_depth, new_rq_depth); + }else { + q_params = nic_dev->q_params; + q_params.sq_depth = new_sq_depth; + q_params.rq_depth = new_rq_depth; + + err =hinic3_change_channel_settings (netdev, &q_params); +if (err) +return err; + } + +return 0 ; +} +static const struct ethtool_ops hinic3_ethtool_ops = { .supported_coalesce_params =ETHTOOL_COALESCE_USECS |ETHTOOL_COALESCE_PKT_RATE_RX_USECS ,@@ -417,6 +508,8 @@ static const struct ethtool_ops hinic3_ethtool_ops = { .get_msglevel = hinic3_get_msglevel, .set_msglevel = hinic3_set_msglevel, .get_link = ethtool_op_get_link, + .get_ringparam = hinic3_get_ringparam, + .set_ringparam = hinic3_set_ringparam, };void hinic3_set_ethtool_ops (struct net_device *netdev)drivers/net/ethernet/huawei/hinic3/hinic3_irq.c +4
-1@@ -137,7 +137,8 @@ static int hinic3_set_interrupt_moder(struct net_device *netdev, u16 q_id,struct hinic3_interrupt_info info = {};int err; -if (q_id >= nic_dev->q_params.num_qps) +if (q_id >= nic_dev->q_params.num_qps || + !mutex_trylock (&nic_dev->change_res_mutex))return 0 ; info.interrupt_coalesc_set =1 ;@@ -156,6 +157,8 @@ static int hinic3_set_interrupt_moder(struct net_device *netdev, u16 q_id, nic_dev->rxqs[q_id].last_pending_limit = pending_limit; } +mutex_unlock (&nic_dev->change_res_mutex); +return err; }drivers/net/ethernet/huawei/hinic3/hinic3_main.c +6
-0@@ -179,6 +179,7 @@ static int hinic3_sw_init(struct net_device *netdev)int err;mutex_init (&nic_dev->port_state_mutex); +mutex_init (&nic_dev->change_res_mutex); nic_dev->q_params.sq_depth =HINIC3_SQ_DEPTH ; nic_dev->q_params.rq_depth =HINIC3_RQ_DEPTH ;@@ -315,6 +316,9 @@ static void hinic3_link_status_change(struct net_device *netdev, {struct hinic3_nic_dev *nic_dev =netdev_priv (netdev); +if (!mutex_trylock (&nic_dev->change_res_mutex)) +return ; +if (link_status_up) {if (netif_carrier_ok (netdev))return ;@@ -330,6 +334,8 @@ static void hinic3_link_status_change(struct net_device *netdev,netif_carrier_off (netdev);netdev_dbg (netdev,"Link is down\n" ); } + +mutex_unlock (&nic_dev->change_res_mutex); } static void hinic3_port_module_event_handler(struct net_device *netdev,drivers/net/ethernet/huawei/hinic3/hinic3_netdev_ops.c +96
-8@@ -288,7 +288,8 @@ static void hinic3_free_channel_resources(struct net_device *netdev,hinic3_free_qps (nic_dev, qp_params); } -static int hinic3_open_channel (struct net_device *netdev) +static int hinic3_prepare_channel (struct net_device *netdev, +struct hinic3_dyna_txrxq_params *qp_params) {struct hinic3_nic_dev *nic_dev =netdev_priv (netdev);int err;@@ -299,16 +300,28 @@ static int hinic3_open_channel(struct net_device *netdev)return err; } - err =hinic3_configure_txrxqs (netdev, &nic_dev->q_params); + err =hinic3_configure_txrxqs (netdev, qp_params);if (err) {netdev_err (netdev,"Failed to configure txrxqs\n" ); goto err_free_qp_ctxts; } +return 0 ; + +err_free_qp_ctxts: +hinic3_free_qp_ctxts (nic_dev); + +return err; +} + +static int hinic3_open_channel (struct net_device *netdev) +{ +int err; + err =hinic3_qps_irq_init (netdev);if (err) {netdev_err (netdev,"Failed to init txrxq irq\n" ); - goto err_free_qp_ctxts; +return err; } err =hinic3_configure (netdev);@@ -321,8 +334,6 @@ static int hinic3_open_channel(struct net_device *netdev) err_uninit_qps_irq:hinic3_qps_irq_uninit (netdev); -err_free_qp_ctxts: -hinic3_free_qp_ctxts (nic_dev);return err; }@@ -428,6 +439,74 @@ static void hinic3_vport_down(struct net_device *netdev) } } +int +hinic3_change_channel_settings (struct net_device *netdev, +struct hinic3_dyna_txrxq_params *trxq_params) +{ +struct hinic3_nic_dev *nic_dev =netdev_priv (netdev); +struct hinic3_dyna_txrxq_params cur_trxq_params = {}; +struct hinic3_dyna_qp_params new_qp_params = {}; +struct hinic3_dyna_qp_params cur_qp_params = {}; +int err; + + cur_trxq_params = nic_dev->q_params; + +hinic3_config_num_qps (netdev, trxq_params); + + err =hinic3_alloc_channel_resources (netdev, &new_qp_params, + trxq_params); +if (err) { +netdev_err (netdev,"Failed to alloc channel resources\n" ); +return err; + } + +mutex_lock (&nic_dev->change_res_mutex); +hinic3_vport_down (netdev); +hinic3_close_channel (netdev); +hinic3_get_cur_qps (nic_dev, &cur_qp_params); + +hinic3_init_qps (nic_dev, &new_qp_params); + + err =hinic3_prepare_channel (netdev, trxq_params); +if (err) + goto err_uninit_qps; + +if (nic_dev->num_qp_irq > trxq_params->num_qps) +hinic3_qp_irq_change (netdev, trxq_params->num_qps); + + nic_dev->q_params = *trxq_params; + + err =hinic3_open_channel (netdev); +if (err) + goto err_qp_irq_reset; + + err =hinic3_vport_up (netdev); +if (err) + goto err_close_channel; + +hinic3_free_channel_resources (netdev, &cur_qp_params, &cur_trxq_params); + +mutex_unlock (&nic_dev->change_res_mutex); + +return 0 ; + +err_close_channel: +hinic3_close_channel (netdev); +err_qp_irq_reset: + nic_dev->q_params = cur_trxq_params; + +if (trxq_params->num_qps > cur_trxq_params.num_qps) +hinic3_qp_irq_change (netdev, cur_trxq_params.num_qps); +hinic3_free_qp_ctxts (nic_dev); +err_uninit_qps: +hinic3_get_cur_qps (nic_dev, &new_qp_params); +hinic3_free_channel_resources (netdev, &new_qp_params, trxq_params); +hinic3_free_channel_resources (netdev, &cur_qp_params, &cur_trxq_params); +mutex_unlock (&nic_dev->change_res_mutex); + +return err; +} +static int hinic3_open (struct net_device *netdev) {struct hinic3_nic_dev *nic_dev =netdev_priv (netdev);@@ -458,6 +537,10 @@ static int hinic3_open(struct net_device *netdev)hinic3_init_qps (nic_dev, &qp_params); + err =hinic3_prepare_channel (netdev, &nic_dev->q_params); +if (err) + goto err_uninit_qps; + err =hinic3_open_channel (netdev);if (err) goto err_uninit_qps;@@ -473,7 +556,7 @@ static int hinic3_open(struct net_device *netdev) err_close_channel:hinic3_close_channel (netdev); err_uninit_qps: -hinic3_uninit_qps (nic_dev, &qp_params); +hinic3_get_cur_qps (nic_dev, &qp_params);hinic3_free_channel_resources (netdev, &qp_params, &nic_dev->q_params); err_destroy_num_qps:hinic3_destroy_num_qps (netdev);@@ -493,10 +576,15 @@ static int hinic3_close(struct net_device *netdev)return 0 ; } +mutex_lock (&nic_dev->change_res_mutex);hinic3_vport_down (netdev);hinic3_close_channel (netdev); -hinic3_uninit_qps (nic_dev, &qp_params); -hinic3_free_channel_resources (netdev, &qp_params, &nic_dev->q_params); +hinic3_get_cur_qps (nic_dev, &qp_params); +hinic3_free_channel_resources (netdev, &qp_params, + &nic_dev->q_params); +hinic3_free_nicio_res (nic_dev); +hinic3_destroy_num_qps (netdev); +mutex_unlock (&nic_dev->change_res_mutex);return 0 ; }drivers/net/ethernet/huawei/hinic3/hinic3_nic_dev.h +9
-0@@ -10,6 +10,9 @@#include "hinic3_hw_cfg.h" #include "hinic3_hwdev.h" #include "hinic3_mgmt_interface.h" +#include "hinic3_nic_io.h" +#include "hinic3_tx.h" +#include "hinic3_rx.h" #define HINIC3_VLAN_BITMAP_BYTE_SIZE (nic_dev) (sizeof(*(nic_dev)->vlan_bitmap))#define HINIC3_VLAN_BITMAP_SIZE (nic_dev) \@@ -129,6 +132,8 @@ struct hinic3_nic_dev {struct work_struct rx_mode_work;/* lock for enable/disable port */ struct mutex port_state_mutex; +/* mutex to serialize channel/resource changes */ +struct mutex change_res_mutex;struct list_head uc_filter_list;struct list_head mc_filter_list;@@ -143,6 +148,10 @@ struct hinic3_nic_dev {void hinic3_set_netdev_ops (struct net_device *netdev);int hinic3_set_hw_features (struct net_device *netdev); +int +hinic3_change_channel_settings (struct net_device *netdev, +struct hinic3_dyna_txrxq_params *trxq_params); +int hinic3_qps_irq_init (struct net_device *netdev);void hinic3_qps_irq_uninit (struct net_device *netdev);drivers/net/ethernet/huawei/hinic3/hinic3_nic_io.c +2
-2@@ -484,8 +484,8 @@ void hinic3_init_qps(struct hinic3_nic_dev *nic_dev, } } -void hinic3_uninit_qps (struct hinic3_nic_dev *nic_dev, -struct hinic3_dyna_qp_params *qp_params) +void hinic3_get_cur_qps (struct hinic3_nic_dev *nic_dev, +struct hinic3_dyna_qp_params *qp_params) {struct hinic3_nic_io *nic_io = nic_dev->nic_io;drivers/net/ethernet/huawei/hinic3/hinic3_nic_io.h +6
-2@@ -14,6 +14,10 @@ struct hinic3_nic_dev;#define HINIC3_RQ_WQEBB_SHIFT 3#define HINIC3_SQ_WQEBB_SIZE BIT(HINIC3_SQ_WQEBB_SHIFT) +#define HINIC3_MAX_TX_QUEUE_DEPTH 65536 +#define HINIC3_MAX_RX_QUEUE_DEPTH 16384 +#define HINIC3_MIN_QUEUE_DEPTH 128 +/* ******************** RQ_CTRL ******************** */ enum hinic3_rq_wqe_type {HINIC3_NORMAL_RQ_WQE =1 ,@@ -136,8 +140,8 @@ void hinic3_free_qps(struct hinic3_nic_dev *nic_dev, struct hinic3_dyna_qp_params *qp_params);void hinic3_init_qps (struct hinic3_nic_dev *nic_dev,struct hinic3_dyna_qp_params *qp_params); -void hinic3_uninit_qps (struct hinic3_nic_dev *nic_dev, -struct hinic3_dyna_qp_params *qp_params); +void hinic3_get_cur_qps (struct hinic3_nic_dev *nic_dev, +struct hinic3_dyna_qp_params *qp_params);int hinic3_init_qp_ctxts (struct hinic3_nic_dev *nic_dev);void hinic3_free_qp_ctxts (struct hinic3_nic_dev *nic_dev);drivers/net/ethernet/huawei/hinic3/hinic3_rx.c +1
-2@@ -541,7 +541,7 @@ int hinic3_configure_rxqs(struct net_device *netdev, u16 num_rq,rq_associate_cqes (rxq); pkts =hinic3_rx_fill_buffers (rxq); -if (!pkts) { +if (pkts < rxq->q_depth -1 ) {netdev_err (netdev,"Failed to fill Rx buffer\n" );return -ENOMEM ; } --2.43.0
Fan Gong
maintainerPATCH v9 2/5 hinic3: Add ethtool statistic ops
In reply to Fan Gong Add PF/VF statistics functions in TX and RX processing. Implement following ethtool callback function:
.get_sset_count
.get_ethtool_stats
.get_strings
.get_eth_phy_stats
.get_eth_mac_stats
.get_eth_ctrl_stats
.get_rmon_stats
.get_pause_stats
These callbacks allow users to utilize ethtool for detailed
TX and RX netdev stats monitoring.
Add mgmt_msg_params_init_in_out() to support management
commands that require separate input and output buffers. This is needed for retrieving the expanded PF/VF MAC statistics from HW, which no longer fit in the input buffer used by the existing path.
Remove unused stats "restore_drop_sge" in struct hinic3_rxq_stats.
Co-developed-by Wu Di <[email protected]>
Signed-off-by Wu Di <[email protected]>
Co-developed-by Teng Peisen <[email protected]>
Signed-off-by Teng Peisen <[email protected]>
Signed-off-by Fan Gong <[email protected]>
--- .../ethernet/huawei/hinic3/hinic3_ethtool.c | 483 ++++++++++++++++++ .../ethernet/huawei/hinic3/hinic3_hw_intf.h | 11 + .../huawei/hinic3/hinic3_mgmt_interface.h | 37 ++ .../ethernet/huawei/hinic3/hinic3_nic_cfg.c | 64 +++ .../ethernet/huawei/hinic3/hinic3_nic_cfg.h | 109 ++++ .../net/ethernet/huawei/hinic3/hinic3_rx.c | 70 ++- .../net/ethernet/huawei/hinic3/hinic3_rx.h | 15 +- .../net/ethernet/huawei/hinic3/hinic3_tx.c | 62 ++- .../net/ethernet/huawei/hinic3/hinic3_tx.h | 2 + 9 files changed, 845 insertions(+), 8 deletions(-)
drivers/net/ethernet/huawei/hinic3/hinic3_ethtool.c +483 -0
@@ -500,6 +500,481 @@ static int hinic3_set_ringparam(struct net_device *netdev,return 0 ; } +struct hinic3_stats { +char name[ETH_GSTRING_LEN ]; +u32 size; +int offset; +}; + +#define HINIC3_RXQ_STAT (_stat_item) { \ + .name = "rxq%d_"#_stat_item, \ + .size = sizeof_field(struct hinic3_rxq_stats, _stat_item), \ + .offset = offsetof(struct hinic3_rxq_stats, _stat_item) \ +} + +#define HINIC3_TXQ_STAT (_stat_item) { \ + .name = "txq%d_"#_stat_item, \ + .size = sizeof_field(struct hinic3_txq_stats, _stat_item), \ + .offset = offsetof(struct hinic3_txq_stats, _stat_item) \ +} + +static struct hinic3_stats hinic3_rx_queue_stats[] = { +HINIC3_RXQ_STAT (csum_errors), +HINIC3_RXQ_STAT (other_errors), +HINIC3_RXQ_STAT (rx_buf_empty), +HINIC3_RXQ_STAT (alloc_skb_err), +HINIC3_RXQ_STAT (alloc_rx_buf_err), +}; + +static struct hinic3_stats hinic3_tx_queue_stats[] = { +HINIC3_TXQ_STAT (busy), +HINIC3_TXQ_STAT (skb_pad_err), +HINIC3_TXQ_STAT (frag_len_overflow), +HINIC3_TXQ_STAT (offload_cow_skb_err), +HINIC3_TXQ_STAT (map_frag_err), +HINIC3_TXQ_STAT (unknown_tunnel_pkt), +HINIC3_TXQ_STAT (frag_size_err), +}; + +#define HINIC3_FUNC_STAT (_stat_item) { \ + .name = #_stat_item, \ + .size = sizeof_field(struct l2nic_vport_stats, _stat_item), \ + .offset = offsetof(struct l2nic_vport_stats, _stat_item) \ +} + +static struct hinic3_stats hinic3_function_stats[] = { +HINIC3_FUNC_STAT (tx_unicast_pkts_vport), +HINIC3_FUNC_STAT (tx_unicast_bytes_vport), +HINIC3_FUNC_STAT (tx_multicast_pkts_vport), +HINIC3_FUNC_STAT (tx_multicast_bytes_vport), +HINIC3_FUNC_STAT (tx_broadcast_pkts_vport), +HINIC3_FUNC_STAT (tx_broadcast_bytes_vport), + +HINIC3_FUNC_STAT (rx_unicast_pkts_vport), +HINIC3_FUNC_STAT (rx_unicast_bytes_vport), +HINIC3_FUNC_STAT (rx_multicast_pkts_vport), +HINIC3_FUNC_STAT (rx_multicast_bytes_vport), +HINIC3_FUNC_STAT (rx_broadcast_pkts_vport), +HINIC3_FUNC_STAT (rx_broadcast_bytes_vport), + +HINIC3_FUNC_STAT (tx_discard_vport), +HINIC3_FUNC_STAT (rx_discard_vport), +HINIC3_FUNC_STAT (tx_err_vport), +HINIC3_FUNC_STAT (rx_err_vport), +}; + +#define HINIC3_PORT_STAT (_stat_item) { \ + .name = #_stat_item, \ + .size = sizeof_field(struct mag_cmd_port_stats, _stat_item), \ + .offset = offsetof(struct mag_cmd_port_stats, _stat_item) \ +} + +static struct hinic3_stats hinic3_port_stats[] = { +HINIC3_PORT_STAT (mac_tx_fragment_pkt_num), +HINIC3_PORT_STAT (mac_tx_undersize_pkt_num), +HINIC3_PORT_STAT (mac_tx_undermin_pkt_num), +HINIC3_PORT_STAT (mac_tx_1519_max_bad_pkt_num), +HINIC3_PORT_STAT (mac_tx_1519_max_good_pkt_num), +HINIC3_PORT_STAT (mac_tx_oversize_pkt_num), +HINIC3_PORT_STAT (mac_tx_jabber_pkt_num), +HINIC3_PORT_STAT (mac_tx_bad_pkt_num), +HINIC3_PORT_STAT (mac_tx_bad_oct_num), +HINIC3_PORT_STAT (mac_tx_good_oct_num), +HINIC3_PORT_STAT (mac_tx_total_pkt_num), +HINIC3_PORT_STAT (mac_tx_uni_pkt_num), +HINIC3_PORT_STAT (mac_tx_pfc_pkt_num), +HINIC3_PORT_STAT (mac_tx_pfc_pri0_pkt_num), +HINIC3_PORT_STAT (mac_tx_pfc_pri1_pkt_num), +HINIC3_PORT_STAT (mac_tx_pfc_pri2_pkt_num), +HINIC3_PORT_STAT (mac_tx_pfc_pri3_pkt_num), +HINIC3_PORT_STAT (mac_tx_pfc_pri4_pkt_num), +HINIC3_PORT_STAT (mac_tx_pfc_pri5_pkt_num), +HINIC3_PORT_STAT (mac_tx_pfc_pri6_pkt_num), +HINIC3_PORT_STAT (mac_tx_pfc_pri7_pkt_num), +HINIC3_PORT_STAT (mac_tx_err_all_pkt_num), +HINIC3_PORT_STAT (mac_tx_from_app_good_pkt_num), +HINIC3_PORT_STAT (mac_tx_from_app_bad_pkt_num), + +HINIC3_PORT_STAT (mac_rx_undermin_pkt_num), +HINIC3_PORT_STAT (mac_rx_1519_max_bad_pkt_num), +HINIC3_PORT_STAT (mac_rx_1519_max_good_pkt_num), +HINIC3_PORT_STAT (mac_rx_bad_pkt_num), +HINIC3_PORT_STAT (mac_rx_bad_oct_num), +HINIC3_PORT_STAT (mac_rx_good_oct_num), +HINIC3_PORT_STAT (mac_rx_total_pkt_num), +HINIC3_PORT_STAT (mac_rx_uni_pkt_num), +HINIC3_PORT_STAT (mac_rx_pfc_pkt_num), +HINIC3_PORT_STAT (mac_rx_pfc_pri0_pkt_num), +HINIC3_PORT_STAT (mac_rx_pfc_pri1_pkt_num), +HINIC3_PORT_STAT (mac_rx_pfc_pri2_pkt_num), +HINIC3_PORT_STAT (mac_rx_pfc_pri3_pkt_num), +HINIC3_PORT_STAT (mac_rx_pfc_pri4_pkt_num), +HINIC3_PORT_STAT (mac_rx_pfc_pri5_pkt_num), +HINIC3_PORT_STAT (mac_rx_pfc_pri6_pkt_num), +HINIC3_PORT_STAT (mac_rx_pfc_pri7_pkt_num), +HINIC3_PORT_STAT (mac_rx_send_app_good_pkt_num), +HINIC3_PORT_STAT (mac_rx_send_app_bad_pkt_num), +HINIC3_PORT_STAT (mac_rx_unfilter_pkt_num), +}; + +static int hinic3_get_sset_count (struct net_device *netdev,int sset) +{ +struct hinic3_nic_dev *nic_dev =netdev_priv (netdev); +int count, q_num; + +switch (sset) { +case ETH_SS_STATS : + q_num = nic_dev->q_params.num_qps; + count =ARRAY_SIZE (hinic3_function_stats) + + (ARRAY_SIZE (hinic3_tx_queue_stats) + +ARRAY_SIZE (hinic3_rx_queue_stats)) * + q_num; + +if (!HINIC3_IS_VF (nic_dev->hwdev)) + count +=ARRAY_SIZE (hinic3_port_stats); + +return count; +default : +return -EOPNOTSUPP ; + } +} + +static u64 get_val_of_ptr (u32 size,const void *ptr) +{ +u64 ret = size ==sizeof (u64) ? *(u64 *)ptr : + size ==sizeof (u32) ? *(u32 *)ptr : + size ==sizeof (u16) ? *(u16 *)ptr : + *(u8 *)ptr; + +return ret; +} + +static void hinic3_get_drv_queue_stats (struct net_device *netdev,u64 *data) +{ +struct hinic3_nic_dev *nic_dev =netdev_priv (netdev); +struct hinic3_txq_stats txq_stats = {}; +struct hinic3_rxq_stats rxq_stats = {}; +u16 i =0 , j, qid; +char *p; + +for (qid =0 ; qid < nic_dev->q_params.num_qps; qid++) { +if (!nic_dev->txqs) +break ; + +hinic3_txq_get_stats (&nic_dev->txqs[qid], &txq_stats); +for (j =0 ; j <ARRAY_SIZE (hinic3_tx_queue_stats); j++, i++) { + p = (char *)&txq_stats + + hinic3_tx_queue_stats[j].offset; + data[i] =get_val_of_ptr (hinic3_tx_queue_stats[j].size, + p); + } + } + + i = nic_dev->q_params.num_qps *ARRAY_SIZE (hinic3_tx_queue_stats); +for (qid =0 ; qid < nic_dev->q_params.num_qps; qid++) { +if (!nic_dev->rxqs) +break ; + +hinic3_rxq_get_stats (&nic_dev->rxqs[qid], &rxq_stats); +for (j =0 ; j <ARRAY_SIZE (hinic3_rx_queue_stats); j++, i++) { + p = (char *)&rxq_stats + + hinic3_rx_queue_stats[j].offset; + data[i] =get_val_of_ptr (hinic3_rx_queue_stats[j].size, + p); + } + } +} + +static u16 hinic3_get_ethtool_port_stats (struct net_device *netdev,u64 *data) +{ +struct hinic3_nic_dev *nic_dev =netdev_priv (netdev); +struct mag_cmd_port_stats *ps; +u16 i =0 , j; +char *p; +int err; + + ps =kmalloc_obj (*ps); +if (!ps) + goto err_zero_stats; + + err =hinic3_get_phy_port_stats (nic_dev->hwdev, ps); +if (err) { +kfree (ps); +netdev_err (netdev,"Failed to get port stats from fw\n" ); + goto err_zero_stats; + } + +for (j =0 ; j <ARRAY_SIZE (hinic3_port_stats); j++, i++) { + p = (char *)ps + hinic3_port_stats[j].offset; + data[i] =get_val_of_ptr (hinic3_port_stats[j].size, p); + } + +kfree (ps); + +return i; + +err_zero_stats: +memset (&data[i],0 ,ARRAY_SIZE (hinic3_port_stats) *sizeof (*data)); + +return i +ARRAY_SIZE (hinic3_port_stats); +} + +static void hinic3_get_ethtool_stats (struct net_device *netdev, +struct ethtool_stats *stats,u64 *data) +{ +struct hinic3_nic_dev *nic_dev =netdev_priv (netdev); +struct l2nic_vport_stats vport_stats = {}; +u16 i =0 , j; +char *p; +int err; + + err =hinic3_get_vport_stats (nic_dev->hwdev, +hinic3_global_func_id (nic_dev->hwdev), + &vport_stats); +if (err) +netdev_err (netdev,"Failed to get function stats from fw\n" ); + +for (j =0 ; j <ARRAY_SIZE (hinic3_function_stats); j++, i++) { + p = (char *)&vport_stats + hinic3_function_stats[j].offset; + data[i] =get_val_of_ptr (hinic3_function_stats[j].size, p); + } + +if (!HINIC3_IS_VF (nic_dev->hwdev)) + i +=hinic3_get_ethtool_port_stats (netdev, data + i); + +hinic3_get_drv_queue_stats (netdev, data + i); +} + +static u16 hinic3_get_hw_stats_strings (struct net_device *netdev,char *p) +{ +struct hinic3_nic_dev *nic_dev =netdev_priv (netdev); +u16 i, cnt =0 ; + +for (i =0 ; i <ARRAY_SIZE (hinic3_function_stats); i++) { +memcpy (p, hinic3_function_stats[i].name,ETH_GSTRING_LEN ); + p +=ETH_GSTRING_LEN ; + cnt++; + } + +if (!HINIC3_IS_VF (nic_dev->hwdev)) { +for (i =0 ; i <ARRAY_SIZE (hinic3_port_stats); i++) { +memcpy (p, hinic3_port_stats[i].name,ETH_GSTRING_LEN ); + p +=ETH_GSTRING_LEN ; + cnt++; + } + } + +return cnt; +} + +static void hinic3_get_qp_stats_strings (struct net_device *netdev,char *p) +{ +struct hinic3_nic_dev *nic_dev =netdev_priv (netdev); +u8 *data = p; +u16 i, j; + +for (i =0 ; i < nic_dev->q_params.num_qps; i++) { +for (j =0 ; j <ARRAY_SIZE (hinic3_tx_queue_stats); j++) +ethtool_sprintf (&data, + hinic3_tx_queue_stats[j].name, i); + } + +for (i =0 ; i < nic_dev->q_params.num_qps; i++) { +for (j =0 ; j <ARRAY_SIZE (hinic3_rx_queue_stats); j++) +ethtool_sprintf (&data, + hinic3_rx_queue_stats[j].name, i); + } +} + +static void hinic3_get_strings (struct net_device *netdev, +u32 stringset,u8 *data) +{ +char *p = (char *)data; +u16 offset; + +switch (stringset) { +case ETH_SS_STATS : + offset =hinic3_get_hw_stats_strings (netdev, p); +hinic3_get_qp_stats_strings (netdev, + p + offset *ETH_GSTRING_LEN ); + +return ; +default : +netdev_err (netdev,"Invalid string set %u.\n" , stringset); +return ; + } +} + +static void hinic3_get_eth_phy_stats (struct net_device *netdev, +struct ethtool_eth_phy_stats *phy_stats) +{ +struct hinic3_nic_dev *nic_dev =netdev_priv (netdev); +struct mag_cmd_port_stats *ps; +int err; + + ps =kmalloc_obj (*ps); +if (!ps) +return ; + + err =hinic3_get_phy_port_stats (nic_dev->hwdev, ps); +if (err) { +kfree (ps); +netdev_err (netdev,"Failed to get eth phy stats from fw\n" ); +return ; + } + + phy_stats->SymbolErrorDuringCarrier = ps->mac_rx_sym_err_pkt_num; + +kfree (ps); +} + +static void hinic3_get_eth_mac_stats (struct net_device *netdev, +struct ethtool_eth_mac_stats *mac_stats) +{ +struct hinic3_nic_dev *nic_dev =netdev_priv (netdev); +struct mag_cmd_port_stats *ps; +int err; + + ps =kmalloc_obj (*ps); +if (!ps) +return ; + + err =hinic3_get_phy_port_stats (nic_dev->hwdev, ps); +if (err) { +kfree (ps); +netdev_err (netdev,"Failed to get eth mac stats from fw\n" ); +return ; + } + + mac_stats->FramesTransmittedOK = ps->mac_tx_good_pkt_num; + mac_stats->FramesReceivedOK = ps->mac_rx_good_pkt_num; + mac_stats->FrameCheckSequenceErrors = ps->mac_rx_fcs_err_pkt_num; + mac_stats->OctetsTransmittedOK = ps->mac_tx_total_oct_num; + mac_stats->OctetsReceivedOK = ps->mac_rx_total_oct_num; + mac_stats->MulticastFramesXmittedOK = ps->mac_tx_multi_pkt_num; + mac_stats->BroadcastFramesXmittedOK = ps->mac_tx_broad_pkt_num; + mac_stats->MulticastFramesReceivedOK = ps->mac_rx_multi_pkt_num; + mac_stats->BroadcastFramesReceivedOK = ps->mac_rx_broad_pkt_num; + +kfree (ps); +} + +static void hinic3_get_eth_ctrl_stats (struct net_device *netdev, +struct ethtool_eth_ctrl_stats *ctrl_stats) +{ +struct hinic3_nic_dev *nic_dev =netdev_priv (netdev); +struct mag_cmd_port_stats *ps; +int err; + + ps =kmalloc_obj (*ps); +if (!ps) +return ; + + err =hinic3_get_phy_port_stats (nic_dev->hwdev, ps); +if (err) { +kfree (ps); +netdev_err (netdev,"Failed to get eth ctrl stats from fw\n" ); +return ; + } + + ctrl_stats->MACControlFramesTransmitted = ps->mac_tx_control_pkt_num; + ctrl_stats->MACControlFramesReceived = ps->mac_rx_control_pkt_num; + +kfree (ps); +} + +static const struct ethtool_rmon_hist_range hinic3_rmon_ranges[] = { + {0 ,64 }, + {65 ,127 }, + {128 ,255 }, + {256 ,511 }, + {512 ,1023 }, + {1024 ,1518 }, + {1519 ,2047 }, + {2048 ,4095 }, + {4096 ,8191 }, + {8192 ,9216 }, + {9217 ,12287 }, + {} +}; + +static void hinic3_get_rmon_stats (struct net_device *netdev, +struct ethtool_rmon_stats *rmon_stats, +const struct ethtool_rmon_hist_range **ranges) +{ +struct hinic3_nic_dev *nic_dev =netdev_priv (netdev); +struct mag_cmd_port_stats *ps; +int err; + + ps =kmalloc_obj (*ps); +if (!ps) +return ; + + err =hinic3_get_phy_port_stats (nic_dev->hwdev, ps); +if (err) { +kfree (ps); +netdev_err (netdev,"Failed to get eth rmon stats from fw\n" ); +return ; + } + + rmon_stats->undersize_pkts = ps->mac_rx_undersize_pkt_num; + rmon_stats->oversize_pkts = ps->mac_rx_oversize_pkt_num; + rmon_stats->fragments = ps->mac_rx_fragment_pkt_num; + rmon_stats->jabbers = ps->mac_rx_jabber_pkt_num; + + rmon_stats->hist[0 ] = ps->mac_rx_64_oct_pkt_num; + rmon_stats->hist[1 ] = ps->mac_rx_65_127_oct_pkt_num; + rmon_stats->hist[2 ] = ps->mac_rx_128_255_oct_pkt_num; + rmon_stats->hist[3 ] = ps->mac_rx_256_511_oct_pkt_num; + rmon_stats->hist[4 ] = ps->mac_rx_512_1023_oct_pkt_num; + rmon_stats->hist[5 ] = ps->mac_rx_1024_1518_oct_pkt_num; + rmon_stats->hist[6 ] = ps->mac_rx_1519_2047_oct_pkt_num; + rmon_stats->hist[7 ] = ps->mac_rx_2048_4095_oct_pkt_num; + rmon_stats->hist[8 ] = ps->mac_rx_4096_8191_oct_pkt_num; + rmon_stats->hist[9 ] = ps->mac_rx_8192_9216_oct_pkt_num; + rmon_stats->hist[10 ] = ps->mac_rx_9217_12287_oct_pkt_num; + + rmon_stats->hist_tx[0 ] = ps->mac_tx_64_oct_pkt_num; + rmon_stats->hist_tx[1 ] = ps->mac_tx_65_127_oct_pkt_num; + rmon_stats->hist_tx[2 ] = ps->mac_tx_128_255_oct_pkt_num; + rmon_stats->hist_tx[3 ] = ps->mac_tx_256_511_oct_pkt_num; + rmon_stats->hist_tx[4 ] = ps->mac_tx_512_1023_oct_pkt_num; + rmon_stats->hist_tx[5 ] = ps->mac_tx_1024_1518_oct_pkt_num; + rmon_stats->hist_tx[6 ] = ps->mac_tx_1519_2047_oct_pkt_num; + rmon_stats->hist_tx[7 ] = ps->mac_tx_2048_4095_oct_pkt_num; + rmon_stats->hist_tx[8 ] = ps->mac_tx_4096_8191_oct_pkt_num; + rmon_stats->hist_tx[9 ] = ps->mac_tx_8192_9216_oct_pkt_num; + rmon_stats->hist_tx[10 ] = ps->mac_tx_9217_12287_oct_pkt_num; + + *ranges = hinic3_rmon_ranges; + +kfree (ps); +} + +static void hinic3_get_pause_stats (struct net_device *netdev, +struct ethtool_pause_stats *pause_stats) +{ +struct hinic3_nic_dev *nic_dev =netdev_priv (netdev); +struct mag_cmd_port_stats *ps; +int err; + + ps =kmalloc_obj (*ps); +if (!ps) +return ; + + err =hinic3_get_phy_port_stats (nic_dev->hwdev, ps); +if (err) { +kfree (ps); +netdev_err (netdev,"Failed to get eth pause stats from fw\n" ); +return ; + } + + pause_stats->tx_pause_frames = ps->mac_tx_pause_num; + pause_stats->rx_pause_frames = ps->mac_rx_pause_num; + +kfree (ps); +} +static const struct ethtool_ops hinic3_ethtool_ops = { .supported_coalesce_params =ETHTOOL_COALESCE_USECS |ETHTOOL_COALESCE_PKT_RATE_RX_USECS ,
@@ -510,6 +985,14 @@ static const struct ethtool_ops hinic3_ethtool_ops = { .get_link = ethtool_op_get_link, .get_ringparam = hinic3_get_ringparam, .set_ringparam = hinic3_set_ringparam, + .get_sset_count = hinic3_get_sset_count, + .get_ethtool_stats = hinic3_get_ethtool_stats, + .get_strings = hinic3_get_strings, + .get_eth_phy_stats = hinic3_get_eth_phy_stats, + .get_eth_mac_stats = hinic3_get_eth_mac_stats, + .get_eth_ctrl_stats = hinic3_get_eth_ctrl_stats, + .get_rmon_stats = hinic3_get_rmon_stats, + .get_pause_stats = hinic3_get_pause_stats, };void hinic3_set_ethtool_ops (struct net_device *netdev)
drivers/net/ethernet/huawei/hinic3/hinic3_hw_intf.h +11 -0
@@ -53,6 +53,17 @@ static inline void mgmt_msg_params_init_default(struct mgmt_msg_params *msg_para msg_params->timeout_ms =0 ; } +static inline void +mgmt_msg_params_init_in_out (struct mgmt_msg_params *msg_params,void *in_buf, +void *out_buf,u32 in_buf_size,u32 out_buf_size) +{ + msg_params->buf_in = in_buf; + msg_params->buf_out = out_buf; + msg_params->in_size = in_buf_size; + msg_params->expected_out_size = out_buf_size; + msg_params->timeout_ms =0 ; +} +enum cfg_cmd {CFG_CMD_GET_DEV_CAP =0 , };
drivers/net/ethernet/huawei/hinic3/hinic3_mgmt_interface.h +37 -0
@@ -143,6 +143,41 @@ struct l2nic_cmd_set_dcb_state {u8 rsvd[7 ]; }; +struct l2nic_port_stats_info { +struct mgmt_msg_head msg_head; +u16 func_id; +u16 rsvd1; +}; + +struct l2nic_vport_stats { +u64 tx_unicast_pkts_vport; +u64 tx_unicast_bytes_vport; +u64 tx_multicast_pkts_vport; +u64 tx_multicast_bytes_vport; +u64 tx_broadcast_pkts_vport; +u64 tx_broadcast_bytes_vport; + +u64 rx_unicast_pkts_vport; +u64 rx_unicast_bytes_vport; +u64 rx_multicast_pkts_vport; +u64 rx_multicast_bytes_vport; +u64 rx_broadcast_pkts_vport; +u64 rx_broadcast_bytes_vport; + +u64 tx_discard_vport; +u64 rx_discard_vport; +u64 tx_err_vport; +u64 rx_err_vport; +}; + +struct l2nic_cmd_vport_stats { +struct mgmt_msg_head msg_head; +u32 stats_size; +u32 rsvd1; +struct l2nic_vport_stats stats; +u64 rsvd2[6 ]; +}; +struct l2nic_cmd_lro_config {struct mgmt_msg_head msg_head;u16 func_id;
@@ -234,6 +269,7 @@ enum l2nic_cmd {L2NIC_CMD_SET_VPORT_ENABLE =6 ,L2NIC_CMD_SET_RX_MODE =7 ,L2NIC_CMD_SET_SQ_CI_ATTR =8 , +L2NIC_CMD_GET_VPORT_STAT =9 ,L2NIC_CMD_CLEAR_QP_RESOURCE =11 ,L2NIC_CMD_CFG_RX_LRO =13 , L2NIC_CMD_CFG_LRO_TIMER = 14,
@@ -272,6 +308,7 @@ enum mag_cmd {MAG_CMD_SET_PORT_ENABLE =6 ,MAG_CMD_GET_LINK_STATUS =7 , +MAG_CMD_GET_PORT_STAT =151 , MAG_CMD_GET_PORT_INFO = 153, };
drivers/net/ethernet/huawei/hinic3/hinic3_nic_cfg.c +64 -0
73 lines not on this page · raw message
drivers/net/ethernet/huawei/hinic3/hinic3_nic_cfg.h +109 -0
121 lines not on this page · raw message
drivers/net/ethernet/huawei/hinic3/hinic3_rx.c +65 -5
138 lines not on this page · raw message
drivers/net/ethernet/huawei/hinic3/hinic3_rx.h +14 -1
@@ -8,6 +8,17 @@#include <linux/dim.h> #include <linux/netdevice.h> +/* rx cqe checksum err */ +#define HINIC3_RX_CSUM_IP_CSUM_ERR BIT(0) +#define HINIC3_RX_CSUM_TCP_CSUM_ERR BIT(1) +#define HINIC3_RX_CSUM_UDP_CSUM_ERR BIT(2) +#define HINIC3_RX_CSUM_IGMP_CSUM_ERR BIT(3) +#define HINIC3_RX_CSUM_ICMPV4_CSUM_ERR BIT(4) +#define HINIC3_RX_CSUM_ICMPV6_CSUM_ERR BIT(5) +#define HINIC3_RX_CSUM_SCTP_CRC_ERR BIT(6) +#define HINIC3_RX_CSUM_HW_CHECK_NONE BIT(7) +#define HINIC3_RX_CSUM_IPSU_OTHER_ERR BIT(8) +#define RQ_CQE_OFFOLAD_TYPE_PKT_TYPE_MASK GENMASK(4, 0)#define RQ_CQE_OFFOLAD_TYPE_IP_TYPE_MASK GENMASK(6, 5)#define RQ_CQE_OFFOLAD_TYPE_TUNNEL_PKT_FORMAT_MASK GENMASK(11, 8)
@@ -39,7 +50,6 @@ struct hinic3_rxq_stats {u64 rx_buf_empty;u64 alloc_skb_err;u64 alloc_rx_buf_err; -u64 restore_drop_sge;struct u64_stats_sync syncp; };
@@ -123,6 +133,9 @@ void hinic3_free_rxqs_res(struct net_device *netdev, u16 num_rq, u32 rq_depth, struct hinic3_dyna_rxq_res *rxqs_res);int hinic3_configure_rxqs (struct net_device *netdev,u16 num_rq,u32 rq_depth,struct hinic3_dyna_rxq_res *rxqs_res); + +void hinic3_rxq_get_stats (struct hinic3_rxq *rxq, +struct hinic3_rxq_stats *stats);int hinic3_rx_poll (struct hinic3_rxq *rxq,int budget);#endif
drivers/net/ethernet/huawei/hinic3/hinic3_tx.c +60 -2
112 lines not on this page · raw message
drivers/net/ethernet/huawei/hinic3/hinic3_tx.h +2 -0
@@ -157,6 +157,8 @@ int hinic3_configure_txqs(struct net_device *netdev, u16 num_sq, u32 sq_depth, struct hinic3_dyna_txq_res *txqs_res);netdev_tx_t hinic3_xmit_frame (struct sk_buff *skb,struct net_device *netdev); +void hinic3_txq_get_stats (struct hinic3_txq *txq, +struct hinic3_txq_stats *stats);bool hinic3_tx_poll (struct hinic3_txq *txq,int budget);void hinic3_flush_txqs (struct net_device *netdev);
Signature · 1 line
2.43.0
Simon Horman
reviewerRe: [PATCH net-next v09 2/5] hinic3: Add ethtool statistic ops
This is an AI-generated review of your patch. The human sending this email has considered the AI review valid, or at least plausible. Full review at: https://sashiko.dev
---
Subject: hinic3: Add ethtool statistic ops
This commit adds PF/VF statistics functions for TX and RX processing in the hinic3 driver. It implements various ethtool callbacks to allow detailed netdev stats monitoring. It also adds a helper for management commands requiring separate I/O buffers to retrieve expanded MAC stats.
drivers/net/ethernet/huawei/hinic3/hinic3_ethtool.c +0 -0
[ ... ]
Quoted 2 lines
(quoted diff) +2
-0+static struct hinic3_stats hinic3_function_stats[] = { + HINIC3_FUNC_STAT(tx_unicast_pkts_vport),
[ ... ]
Review
Quoted 2 lines
(quoted diff) +2
-0+static struct hinic3_stats hinic3_port_stats[] = { + HINIC3_PORT_STAT(mac_tx_fragment_pkt_num),
[Severity: Medium]
Does this duplicate standard network counters into the private ethtool strings interface?
The networking driver guidelines note that stats with a standard UAPI should not be duplicated in the private strings.
The arrays for function and port stats export standard statistics like unicast packets or MAC errors. Could these be exposed exclusively through standard interfaces like rtnl_link_stats64 or ethtool_mac_stats instead?
[ ... ]
drivers/net/ethernet/huawei/hinic3/hinic3_rx.c +0 -0
[ ... ]
Review int hinic3_rx_poll(struct hinic3_rxq *rxq, int budget) · line 649
Quoted 8 lines
(quoted diff) +4
-0@@ -588,5 +643,10 @@ int hinic3_rx_poll(struct hinic3_rxq *rxq, int budget) if (rxq->delta >= HINIC3_RX_BUFFER_WRITE) hinic3_rx_fill_buffers(rxq); + u64_stats_update_begin(&rxq->rxq_stats.syncp); + rxq->rxq_stats.packets += (u64)nr_pkts; + rxq->rxq_stats.bytes += rx_bytes; + u64_stats_update_end(&rxq->rxq_stats.syncp);
[Severity: Medium]
Does this calculation undercount the number of physical packets received when Large Receive Offload is active?
Earlier in the loop, the code accounts for aggregated wire packets by adding the length of the headers for the aggregated packets to the bytes counter.
However, the rxq_stats.packets update only adds the number of host SKBs, omitting the extra wire packets entirely.
[ ... ]
drivers/net/ethernet/huawei/hinic3/hinic3_tx.c +0 -0
[ ... ]
Review static int hinic3_tx_csum(struct hinic3_txq *txq, struct hinic3_sq_task *task, · line 275
Quoted 10 lines
(quoted diff) +3
-0@@ -260,6 +267,9 @@ static int hinic3_tx_csum(struct hinic3_txq *txq, struct hinic3_sq_task *task, if (l4_proto != IPPROTO_UDP || ((struct udphdr *)skb_transport_header(skb))->dest != VXLAN_OFFLOAD_PORT_LE) { + u64_stats_update_begin(&txq->txq_stats.syncp); + txq->txq_stats.unknown_tunnel_pkt++; + u64_stats_update_end(&txq->txq_stats.syncp); /* Unsupported tunnel packet, disable csum offload */ skb_checksum_help(skb); return 0;
[Severity: High]
This is a pre-existing issue, but does ignoring the return value of the software checksum fallback risk silent data corruption?
If the calculation in skb_checksum_help fails, for example due to an allocation failure when ensuring the buffer is writable, the packet checksum remains uncalculated.
Since the error is ignored and 0 is returned, the driver proceeds to transmit the packet without hardware offload, resulting in a corrupted packet being sent onto the network.
[ ... ]
Review
Quoted 9 lines
(quoted diff) +9
-0+static void hinic3_get_pkt_stats(struct hinic3_txq *txq, struct sk_buff *skb) +{ + u32 hdr_len, tx_bytes; + unsigned short pkts; + + if (skb_is_gso(skb)) { + hdr_len = (skb_shinfo(skb)->gso_segs - 1) * + skb_tcp_all_headers(skb); + tx_bytes = skb->len + hdr_len;
[Severity: High]
Can this sequence cause an out-of-bounds read and statistics corruption for UDP GSO packets?
The code assumes all GSO packets use TCP by calling a helper that reads the TCP header length. If the driver handles UDP-encapsulated offloads, the transport header is much shorter.
Reading the data offset from a TCP header location accesses bytes past the end of the UDP header. This could read arbitrary payload data as a header length multiplier, corrupting the tx_bytes statistic and potentially causing a page fault.
[ ... ]
Fan Gong
maintainerPATCH v9 3/5 hinic3: Add ethtool coalesce ops
In reply to Fan Gong Implement following ethtool callback function:
.get_coalesce
.set_coalesce
These callbacks allow users to utilize ethtool for detailed
RX coalesce configuration and monitoring.
Co-developed-by Wu Di <[email protected]>
Signed-off-by Wu Di <[email protected]>
Co-developed-by Teng Peisen <[email protected]>
Signed-off-by Teng Peisen <[email protected]>
Signed-off-by Fan Gong <[email protected]>
--- .../ethernet/huawei/hinic3/hinic3_ethtool.c | 249 +++++++++++++++++- .../net/ethernet/huawei/hinic3/hinic3_irq.c | 3 + .../net/ethernet/huawei/hinic3/hinic3_main.c | 1 + .../ethernet/huawei/hinic3/hinic3_nic_dev.h | 2 + 4 files changed, 253 insertions(+), 2 deletions(-)
drivers/net/ethernet/huawei/hinic3/hinic3_ethtool.c +247 -2
@@ -18,6 +18,11 @@#include "hinic3_nic_cfg.h" #define HINIC3_MGMT_VERSION_MAX_LEN 32 +/* Coalesce time properties in microseconds */ +#define COALESCE_PENDING_LIMIT_UNIT 8 +#define COALESCE_TIMER_CFG_UNIT 5 +#define COALESCE_MAX_PENDING_LIMIT (255 * COALESCE_PENDING_LIMIT_UNIT) +#define COALESCE_MAX_TIMER_CFG (255 * COALESCE_TIMER_CFG_UNIT)static void hinic3_get_drvinfo (struct net_device *netdev,struct ethtool_drvinfo *info)
@@ -975,9 +980,247 @@ static void hinic3_get_pause_stats(struct net_device *netdev,kfree (ps); } +static int hinic3_set_queue_coalesce (struct net_device *netdev,u16 q_id, +struct hinic3_intr_coal_info *coal, +struct netlink_ext_ack *extack) +{ +struct hinic3_nic_dev *nic_dev =netdev_priv (netdev); +struct hinic3_intr_coal_info *intr_coal; +struct hinic3_interrupt_info info = {}; +int err; + +if (nic_dev->adaptive_rx_coal) { +NL_SET_ERR_MSG_MOD (extack, +"Static coalesce not allowed in adaptive RX mode" ); +return -EINVAL ; + } + +if (!test_bit (HINIC3_INTF_UP , &nic_dev->flags) || + q_id >= nic_dev->q_params.num_qps) +return 0 ; + +spin_lock (&nic_dev->coal_lock); + + intr_coal = &nic_dev->intr_coalesce[q_id]; + + intr_coal->coalesce_timer_cfg = coal->coalesce_timer_cfg; + intr_coal->pending_limit = coal->pending_limit; + intr_coal->rx_pending_limit_low = coal->rx_pending_limit_low; + intr_coal->rx_pending_limit_high = coal->rx_pending_limit_high; +spin_unlock (&nic_dev->coal_lock); + + info.msix_index = nic_dev->q_params.irq_cfg[q_id].msix_entry_idx; + info.interrupt_coalesc_set =1 ; + info.coalesc_timer_cfg = intr_coal->coalesce_timer_cfg; + info.pending_limit = intr_coal->pending_limit; + info.resend_timer_cfg = intr_coal->resend_timer_cfg; + err =hinic3_set_interrupt_cfg (nic_dev->hwdev, info); +if (err) { +NL_SET_ERR_MSG_FMT_MOD (extack, +"Failed to set queue%u coalesce" , + q_id); +return err; + } + +return 0 ; +} + +static int is_coalesce_exceed_limit (const struct ethtool_coalesce *coal, +struct netlink_ext_ack *extack) +{ +const struct { +const char *name; +u32 value; +u32 limit; + } coalesce_limits[] = { + {"rx_coalesce_usecs" , + coal->rx_coalesce_usecs, +COALESCE_MAX_TIMER_CFG }, + {"rx_max_coalesced_frames" , + coal->rx_max_coalesced_frames, +COALESCE_MAX_PENDING_LIMIT }, + {"rx_max_coalesced_frames_low" , + coal->rx_max_coalesced_frames_low, +COALESCE_MAX_PENDING_LIMIT }, + {"rx_max_coalesced_frames_high" , + coal->rx_max_coalesced_frames_high, +COALESCE_MAX_PENDING_LIMIT }, + }; + +for (int i =0 ; i <ARRAY_SIZE (coalesce_limits); i++) { +if (coalesce_limits[i].value > coalesce_limits[i].limit) { +NL_SET_ERR_MSG_FMT_MOD (extack, +"%s out of range %d-%d" , + coalesce_limits[i].name, +0 , + coalesce_limits[i].limit); +return -ERANGE ; + } + } +return 0 ; +} + +static int is_coalesce_legal (const struct ethtool_coalesce *coal, +struct netlink_ext_ack *extack) +{ +int err; + + err =is_coalesce_exceed_limit (coal, extack); +if (err) +return err; + +if (coal->rx_max_coalesced_frames_low > + coal->rx_max_coalesced_frames_high) { +NL_SET_ERR_MSG_FMT_MOD (extack, +"invalid coalesce frame high %u, low %u" , + coal->rx_max_coalesced_frames_high, + coal->rx_max_coalesced_frames_low); +return -ERANGE ; + } + +return 0 ; +} + +static void check_coalesce_align (struct net_device *netdev, +u32 item,u32 unit,const char *str) +{ +if (item % unit) +netdev_warn (netdev,"%s in %d units, change to %u\n" , + str, unit, item - item % unit); +} + +#define CHECK_COALESCE_ALIGN (member, unit) \ + check_coalesce_align(netdev, member, unit, #member) + +static void check_coalesce_changed (struct net_device *netdev, +u32 item,u32 unit,u32 ori_val, +const char *obj_str,const char *str) +{ +if ((item / unit) != ori_val) +netdev_dbg (netdev,"Change %s from %d to %u %s\n" , + str, ori_val * unit, item - item % unit, obj_str); +} + +#define CHECK_COALESCE_CHANGED (member, unit, ori_val, obj_str) \ + check_coalesce_changed(netdev, member, unit, ori_val, obj_str, #member) + +static int hinic3_set_hw_coal_param (struct net_device *netdev, +struct hinic3_intr_coal_info *intr_coal, +struct netlink_ext_ack *extack) +{ +struct hinic3_nic_dev *nic_dev =netdev_priv (netdev); +int err; +u16 i; + +for (i =0 ; i < nic_dev->max_qps; i++) { + err =hinic3_set_queue_coalesce (netdev, i, intr_coal, extack); +if (err) +return err; + } + +return 0 ; +} + +static int hinic3_get_coalesce (struct net_device *netdev, +struct ethtool_coalesce *coal, +struct kernel_ethtool_coalesce *kernel_coal, +struct netlink_ext_ack *extack) +{ +struct hinic3_nic_dev *nic_dev =netdev_priv (netdev); +struct hinic3_intr_coal_info *interrupt_info; + +memset (coal,0 ,sizeof (*coal)); + + interrupt_info = &nic_dev->intr_coalesce[0 ]; + + coal->use_adaptive_rx_coalesce = nic_dev->adaptive_rx_coal; + +if (nic_dev->adaptive_rx_coal) { + coal->rx_max_coalesced_frames_low = + interrupt_info->rx_pending_limit_low * +COALESCE_PENDING_LIMIT_UNIT ; + coal->rx_max_coalesced_frames_high = + interrupt_info->rx_pending_limit_high * +COALESCE_PENDING_LIMIT_UNIT ; + }else { +/* TX/RX uses the same interrupt. +* So we only declare RX ethtool_coalesce parameters. +*/ + coal->rx_coalesce_usecs = interrupt_info->coalesce_timer_cfg * +COALESCE_TIMER_CFG_UNIT ; + coal->rx_max_coalesced_frames = interrupt_info->pending_limit * +COALESCE_PENDING_LIMIT_UNIT ; + } + +return 0 ; +} + +static int hinic3_set_coalesce (struct net_device *netdev, +struct ethtool_coalesce *coal, +struct kernel_ethtool_coalesce *kernel_coal, +struct netlink_ext_ack *extack) +{ +struct hinic3_nic_dev *nic_dev =netdev_priv (netdev); +struct hinic3_intr_coal_info *ori_intr_coal; +struct hinic3_intr_coal_info intr_coal = {}; +const char *obj_str ="for netdev" ; +int err; + + err =is_coalesce_legal (coal, extack); +if (err) +return err; + +CHECK_COALESCE_ALIGN (coal->rx_coalesce_usecs,COALESCE_TIMER_CFG_UNIT ); +CHECK_COALESCE_ALIGN (coal->rx_max_coalesced_frames, +COALESCE_PENDING_LIMIT_UNIT ); +CHECK_COALESCE_ALIGN (coal->rx_max_coalesced_frames_high, +COALESCE_PENDING_LIMIT_UNIT ); +CHECK_COALESCE_ALIGN (coal->rx_max_coalesced_frames_low, +COALESCE_PENDING_LIMIT_UNIT ); + + ori_intr_coal = &nic_dev->intr_coalesce[0 ]; + +CHECK_COALESCE_CHANGED (coal->rx_coalesce_usecs,COALESCE_TIMER_CFG_UNIT , + ori_intr_coal->coalesce_timer_cfg, obj_str); +CHECK_COALESCE_CHANGED (coal->rx_max_coalesced_frames, +COALESCE_PENDING_LIMIT_UNIT , + ori_intr_coal->pending_limit, obj_str); +CHECK_COALESCE_CHANGED (coal->rx_max_coalesced_frames_high, +COALESCE_PENDING_LIMIT_UNIT , + ori_intr_coal->rx_pending_limit_high, obj_str); +CHECK_COALESCE_CHANGED (coal->rx_max_coalesced_frames_low, +COALESCE_PENDING_LIMIT_UNIT , + ori_intr_coal->rx_pending_limit_low, obj_str); + + intr_coal.coalesce_timer_cfg = + (u8)(coal->rx_coalesce_usecs /COALESCE_TIMER_CFG_UNIT ); + intr_coal.pending_limit = (u8)(coal->rx_max_coalesced_frames / +COALESCE_PENDING_LIMIT_UNIT ); + + nic_dev->adaptive_rx_coal = coal->use_adaptive_rx_coalesce; + + intr_coal.rx_pending_limit_high = + (u8)(coal->rx_max_coalesced_frames_high / +COALESCE_PENDING_LIMIT_UNIT ); + + intr_coal.rx_pending_limit_low = + (u8)(coal->rx_max_coalesced_frames_low / +COALESCE_PENDING_LIMIT_UNIT ); + +/* coalesce timer or pending set to zero will disable coalesce */ +if (!nic_dev->adaptive_rx_coal && + (!intr_coal.coalesce_timer_cfg || !intr_coal.pending_limit)) +NL_SET_ERR_MSG_MOD (extack,"Coalesce will be disabled" ); + +return hinic3_set_hw_coal_param (netdev, &intr_coal, extack); +} +static const struct ethtool_ops hinic3_ethtool_ops = { - .supported_coalesce_params =ETHTOOL_COALESCE_USECS | -ETHTOOL_COALESCE_PKT_RATE_RX_USECS , + .supported_coalesce_params =ETHTOOL_COALESCE_RX_USECS | +ETHTOOL_COALESCE_RX_MAX_FRAMES | +ETHTOOL_COALESCE_USE_ADAPTIVE_RX | +ETHTOOL_COALESCE_RX_MAX_FRAMES_LOW | +ETHTOOL_COALESCE_RX_MAX_FRAMES_HIGH , .get_link_ksettings = hinic3_get_link_ksettings, .get_drvinfo = hinic3_get_drvinfo, .get_msglevel = hinic3_get_msglevel,
@@ -993,6 +1236,8 @@ static const struct ethtool_ops hinic3_ethtool_ops = { .get_eth_ctrl_stats = hinic3_get_eth_ctrl_stats, .get_rmon_stats = hinic3_get_rmon_stats, .get_pause_stats = hinic3_get_pause_stats, + .get_coalesce = hinic3_get_coalesce, + .set_coalesce = hinic3_set_coalesce, };void hinic3_set_ethtool_ops (struct net_device *netdev)
drivers/net/ethernet/huawei/hinic3/hinic3_irq.c +3 -0
@@ -173,9 +173,12 @@ static void hinic3_update_queue_coal(struct net_device *netdev, u16 q_id, q_coal = &nic_dev->intr_coalesce[q_id]; coalesc_timer_cfg = (u8 )coal_timer; + +spin_lock (&nic_dev->coal_lock); pending_limit =clamp_t (u8, coal_pkts >>HINIC3_COAL_PKT_SHIFT , q_coal->rx_pending_limit_low, q_coal->rx_pending_limit_high); +spin_unlock (&nic_dev->coal_lock);hinic3_set_interrupt_moder (nic_dev->netdev, q_id, coalesc_timer_cfg, pending_limit);
drivers/net/ethernet/huawei/hinic3/hinic3_main.c +1 -0
@@ -180,6 +180,7 @@ static int hinic3_sw_init(struct net_device *netdev)mutex_init (&nic_dev->port_state_mutex);mutex_init (&nic_dev->change_res_mutex); +spin_lock_init (&nic_dev->coal_lock); nic_dev->q_params.sq_depth =HINIC3_SQ_DEPTH ; nic_dev->q_params.rq_depth =HINIC3_RQ_DEPTH ;
drivers/net/ethernet/huawei/hinic3/hinic3_nic_dev.h +2 -0
@@ -134,6 +134,8 @@ struct hinic3_nic_dev {struct mutex port_state_mutex;/* mutex to serialize channel/resource changes */ struct mutex change_res_mutex; +/* lock for set queue coalesce */ +spinlock_t coal_lock;struct list_head uc_filter_list;struct list_head mc_filter_list;
Signature · 1 line
2.43.0
Simon Horman
reviewerRe: [PATCH net-next v09 3/5] hinic3: Add ethtool coalesce ops
This is an AI-generated review of your patch. The human sending this email has considered the AI review valid, or at least plausible. Full review at: https://sashiko.dev
---
hinic3: Add ethtool coalesce ops
This patch implements the .get_coalesce and .set_coalesce ethtool callbacks for the hinic3 driver. It allows users to configure and monitor RX coalesce parameters.
Quoted 3 lines
drivers/net/ethernet/huawei/hinic3/hinic3_ethtool.c +0
-0
[ ... ]
Quoted 5 lines
drivers/net/ethernet/huawei/hinic3/hinic3_ethtool.c +4
-0@@ -975,9 +980,247 @@ static void hinic3_get_pause_stats(struct net_device *netdev, +static int hinic3_set_queue_coalesce (struct net_device *netdev,u16 q_id, +struct hinic3_intr_coal_info *coal, +struct netlink_ext_ack *extack) +{
drivers/net/ethernet/huawei/hinic3/hinic3_ethtool.c static void hinic3_get_pause_stats(struct net_device *netdev, · line 993
Quoted 10 lines
drivers/net/ethernet/huawei/hinic3/hinic3_ethtool.c +10
-0+struct hinic3_nic_dev *nic_dev =netdev_priv (netdev); +struct hinic3_intr_coal_info *intr_coal; +struct hinic3_interrupt_info info = {}; +int err; + +if (nic_dev->adaptive_rx_coal) { +NL_SET_ERR_MSG_MOD (extack, +"Static coalesce not allowed in adaptive RX mode" ); +return -EINVAL ; + }
[Severity: High]
When adaptive RX coalesce is enabled via ethtool, hinic3_set_coalesce() sets nic_dev->adaptive_rx_coal to 1 before calling this function.
Will this check then unconditionally return -EINVAL, failing the operation while leaving the software state permanently incorrectly recording that adaptive mode is on?
drivers/net/ethernet/huawei/hinic3/hinic3_ethtool.c static void hinic3_get_pause_stats(struct net_device *netdev, · line 1004
Quoted 8 lines
drivers/net/ethernet/huawei/hinic3/hinic3_ethtool.c +8
-0+ +if (!test_bit (HINIC3_INTF_UP , &nic_dev->flags) || + q_id >= nic_dev->q_params.num_qps) +return 0 ; + +spin_lock (&nic_dev->coal_lock); + + intr_coal = &nic_dev->intr_coalesce[q_id];
[Severity: Medium]
If the interface is down, this early return skips updating the software configuration struct.
Will this cause the new coalesce parameters to be silently lost instead of being applied when the interface is later brought up?
drivers/net/ethernet/huawei/hinic3/hinic3_ethtool.c static void hinic3_get_pause_stats(struct net_device *netdev, · line 1017
Quoted 13 lines
drivers/net/ethernet/huawei/hinic3/hinic3_ethtool.c +13
-0+ + intr_coal->coalesce_timer_cfg = coal->coalesce_timer_cfg; + intr_coal->pending_limit = coal->pending_limit; + intr_coal->rx_pending_limit_low = coal->rx_pending_limit_low; + intr_coal->rx_pending_limit_high = coal->rx_pending_limit_high; +spin_unlock (&nic_dev->coal_lock); + + info.msix_index = nic_dev->q_params.irq_cfg[q_id].msix_entry_idx; + info.interrupt_coalesc_set =1 ; + info.coalesc_timer_cfg = intr_coal->coalesce_timer_cfg; + info.pending_limit = intr_coal->pending_limit; + info.resend_timer_cfg = intr_coal->resend_timer_cfg; + err =hinic3_set_interrupt_cfg (nic_dev->hwdev, info);
[Severity: High]
Does hinic3_set_interrupt_cfg() need to be protected by change_res_mutex here?
The DIM code path in hinic3_set_interrupt_moder() holds change_res_mutex when performing this same hardware configuration. Since ethtool commands and the DIM workqueue can run concurrently, could this lead to a race condition corrupting the read-modify-write hardware state?
drivers/net/ethernet/huawei/hinic3/hinic3_ethtool.c static void hinic3_get_pause_stats(struct net_device *netdev, · line 1023
Quoted 6 lines
drivers/net/ethernet/huawei/hinic3/hinic3_ethtool.c +6
-0+if (err) { +NL_SET_ERR_MSG_FMT_MOD (extack, +"Failed to set queue%u coalesce" , + q_id); +return err; + }
[Severity: Medium]
If hinic3_set_interrupt_cfg() fails, the software state in intr_coal has already been updated.
Because hinic3_set_hw_coal_param() runs this in a loop over all queues, will a hardware configuration failure leave the software state permanently diverging from the actual physical hardware state?
[ ... ]
drivers/net/ethernet/huawei/hinic3/hinic3_ethtool.c static void hinic3_get_pause_stats(struct net_device *netdev, · line 1132
Quoted 9 lines
drivers/net/ethernet/huawei/hinic3/hinic3_ethtool.c +9
-0+static int hinic3_get_coalesce (struct net_device *netdev, +struct ethtool_coalesce *coal, +struct kernel_ethtool_coalesce *kernel_coal, +struct netlink_ext_ack *extack) +{ +struct hinic3_nic_dev *nic_dev =netdev_priv (netdev); +struct hinic3_intr_coal_info *interrupt_info; + +memset (coal,0 ,sizeof (*coal));
[Severity: Medium]
Is it safe to memset the entire ethtool_coalesce structure here?
The core ethtool ioctl code pre-initializes the cmd field to ETHTOOL_GCOALESCE, so zeroing the entire struct will wipe the cmd field and break the ethtool UAPI.
Quoted 11 lines
drivers/net/ethernet/huawei/hinic3/hinic3_ethtool.c +11
-0+ + interrupt_info = &nic_dev->intr_coalesce[0 ]; + + coal->use_adaptive_rx_coalesce = nic_dev->adaptive_rx_coal; + +if (nic_dev->adaptive_rx_coal) { + coal->rx_max_coalesced_frames_low = + interrupt_info->rx_pending_limit_low * +COALESCE_PENDING_LIMIT_UNIT ; + coal->rx_max_coalesced_frames_high = + interrupt_info->rx_pending_limit_high *
drivers/net/ethernet/huawei/hinic3/hinic3_ethtool.c static void hinic3_get_pause_stats(struct net_device *netdev, · line 1153
Quoted 10 lines
drivers/net/ethernet/huawei/hinic3/hinic3_ethtool.c +10
-0+COALESCE_PENDING_LIMIT_UNIT ; + }else { +/* TX/RX uses the same interrupt. +* So we only declare RX ethtool_coalesce parameters. +*/ + coal->rx_coalesce_usecs = interrupt_info->coalesce_timer_cfg * +COALESCE_TIMER_CFG_UNIT ; + coal->rx_max_coalesced_frames = interrupt_info->pending_limit * +COALESCE_PENDING_LIMIT_UNIT ; + }
[Severity: High]
When adaptive mode is active, the static coalesce fields are left as 0, and vice versa.
During an ethtool read-modify-write cycle, will the intermediate 0 values be passed back to hinic3_set_coalesce() and unconditionally overwrite the driver state, wiping out the inactive configuration?
[ ... ]
Quoted 5 lines
drivers/net/ethernet/huawei/hinic3/hinic3_ethtool.c +5
-0+static int hinic3_set_coalesce (struct net_device *netdev, +struct ethtool_coalesce *coal, +struct kernel_ethtool_coalesce *kernel_coal, +struct netlink_ext_ack *extack) +{
[ ... ]
drivers/net/ethernet/huawei/hinic3/hinic3_ethtool.c static void hinic3_get_pause_stats(struct net_device *netdev, · line 1200
Quoted 6 lines
drivers/net/ethernet/huawei/hinic3/hinic3_ethtool.c +6
-0+ intr_coal.coalesce_timer_cfg = + (u8)(coal->rx_coalesce_usecs /COALESCE_TIMER_CFG_UNIT ); + intr_coal.pending_limit = (u8)(coal->rx_max_coalesced_frames / +COALESCE_PENDING_LIMIT_UNIT ); + + nic_dev->adaptive_rx_coal = coal->use_adaptive_rx_coalesce;
[Severity: High]
When disabling adaptive RX coalesce, is it necessary to cancel any pending DIM work items before programming the hardware?
If a rxq->dim.work item is already executing concurrently or scheduled, could it fire after the ethtool command completes and overwrite these newly configured static limits with stale dynamically calculated values?
Quoted 16 lines
drivers/net/ethernet/huawei/hinic3/hinic3_ethtool.c +16
-0+ + intr_coal.rx_pending_limit_high = + (u8)(coal->rx_max_coalesced_frames_high / +COALESCE_PENDING_LIMIT_UNIT ); + + intr_coal.rx_pending_limit_low = + (u8)(coal->rx_max_coalesced_frames_low / +COALESCE_PENDING_LIMIT_UNIT ); + +/* coalesce timer or pending set to zero will disable coalesce */ +if (!nic_dev->adaptive_rx_coal && + (!intr_coal.coalesce_timer_cfg || !intr_coal.pending_limit)) +NL_SET_ERR_MSG_MOD (extack,"Coalesce will be disabled" ); + +return hinic3_set_hw_coal_param (netdev, &intr_coal, extack); +}
Fan Gong
maintainerPATCH v9 4/5 hinic3: Add ethtool rss ops
In reply to Fan Gong Implement following ethtool callback function:
.get_rxnfc
.set_rxnfc
.get_channels
.set_channels
.get_rxfh_indir_size
.get_rxfh_key_size
.get_rxfh
.set_rxfh
These callbacks allow users to utilize ethtool for detailed
RSS parameters configuration and monitoring.
Co-developed-by Wu Di <[email protected]>
Signed-off-by Wu Di <[email protected]>
Co-developed-by Teng Peisen <[email protected]>
Signed-off-by Teng Peisen <[email protected]>
Signed-off-by Fan Gong <[email protected]>
--- .../ethernet/huawei/hinic3/hinic3_ethtool.c | 9 + .../huawei/hinic3/hinic3_mgmt_interface.h | 2 + .../net/ethernet/huawei/hinic3/hinic3_rss.c | 539 +++++++++++++++++- .../net/ethernet/huawei/hinic3/hinic3_rss.h | 19 + 4 files changed, 567 insertions(+), 2 deletions(-)
drivers/net/ethernet/huawei/hinic3/hinic3_ethtool.c +9 -0
@@ -16,6 +16,7 @@#include "hinic3_hw_comm.h" #include "hinic3_nic_dev.h" #include "hinic3_nic_cfg.h" +#include "hinic3_rss.h" #define HINIC3_MGMT_VERSION_MAX_LEN 32/* Coalesce time properties in microseconds */
@@ -1238,6 +1239,14 @@ static const struct ethtool_ops hinic3_ethtool_ops = { .get_pause_stats = hinic3_get_pause_stats, .get_coalesce = hinic3_get_coalesce, .set_coalesce = hinic3_set_coalesce, + .get_rxnfc = hinic3_get_rxnfc, + .set_rxnfc = hinic3_set_rxnfc, + .get_channels = hinic3_get_channels, + .set_channels = hinic3_set_channels, + .get_rxfh_indir_size = hinic3_get_rxfh_indir_size, + .get_rxfh_key_size = hinic3_get_rxfh_key_size, + .get_rxfh = hinic3_get_rxfh, + .set_rxfh = hinic3_set_rxfh, };void hinic3_set_ethtool_ops (struct net_device *netdev)
drivers/net/ethernet/huawei/hinic3/hinic3_mgmt_interface.h +2 -0
@@ -282,6 +282,7 @@ enum l2nic_cmd {L2NIC_CMD_SET_VLAN_FILTER_EN =26 ,L2NIC_CMD_SET_RX_VLAN_OFFLOAD =27 ,L2NIC_CMD_CFG_RSS =60 , +L2NIC_CMD_GET_RSS_CTX_TBL =62 ,L2NIC_CMD_CFG_RSS_HASH_KEY =63 ,L2NIC_CMD_CFG_RSS_HASH_ENGINE =64 , L2NIC_CMD_SET_RSS_CTX_TBL = 65,
@@ -301,6 +302,7 @@ enum l2nic_ucode_cmd {L2NIC_UCODE_CMD_MODIFY_QUEUE_CTX =0 ,L2NIC_UCODE_CMD_CLEAN_QUEUE_CTX =1 ,L2NIC_UCODE_CMD_SET_RSS_INDIR_TBL =4 , + L2NIC_UCODE_CMD_GET_RSS_INDIR_TBL = 6, };/* hilink mac group command */
drivers/net/ethernet/huawei/hinic3/hinic3_rss.c +537 -2
560 lines not on this page · raw message
drivers/net/ethernet/huawei/hinic3/hinic3_rss.h +19 -0
@@ -5,10 +5,29 @@#define _HINIC3_RSS_H_#include <linux/netdevice.h> +#include <linux/ethtool.h> int hinic3_rss_init (struct net_device *netdev);void hinic3_rss_uninit (struct net_device *netdev);void hinic3_try_to_enable_rss (struct net_device *netdev);void hinic3_clear_rss_config (struct net_device *netdev); +int hinic3_get_rxnfc (struct net_device *netdev, +struct ethtool_rxnfc *cmd,u32 *rule_locs); +int hinic3_set_rxnfc (struct net_device *netdev,struct ethtool_rxnfc *cmd); + +void hinic3_get_channels (struct net_device *netdev, +struct ethtool_channels *channels); +int hinic3_set_channels (struct net_device *netdev, +struct ethtool_channels *channels); + +u32 hinic3_get_rxfh_indir_size (struct net_device *netdev); +u32 hinic3_get_rxfh_key_size (struct net_device *netdev); + +int hinic3_get_rxfh (struct net_device *netdev, +struct ethtool_rxfh_param *rxfh); +int hinic3_set_rxfh (struct net_device *netdev, +struct ethtool_rxfh_param *rxfh, +struct netlink_ext_ack *extack); +#endif
Signature · 1 line
2.43.0
Re: [PATCH net-next v09 4/5] hinic3: Add ethtool rss ops
Quoted 322 lines · Fan Gong
Implement following ethtool callback function:
.get_rxnfc
.set_rxnfc
.get_channels
.set_channels
.get_rxfh_indir_size
.get_rxfh_key_size
.get_rxfh
.set_rxfhThese callbacks allow users to utilize ethtool for detailed
RSS parameters configuration and monitoring.Co-developed-by Wu Di <[email protected]>Signed-off-by Wu Di <[email protected]>Co-developed-by Teng Peisen <[email protected]>Signed-off-by Teng Peisen <[email protected]>Signed-off-by Fan Gong <[email protected]>--- .../ethernet/huawei/hinic3/hinic3_ethtool.c | 9 + .../huawei/hinic3/hinic3_mgmt_interface.h | 2 + .../net/ethernet/huawei/hinic3/hinic3_rss.c | 539 +++++++++++++++++- .../net/ethernet/huawei/hinic3/hinic3_rss.h | 19 + 4 files changed, 567 insertions(+), 2 deletions(-)drivers/net/ethernet/huawei/hinic3/hinic3_ethtool.c +9
-0@@ -16,6 +16,7 @@#include "hinic3_hw_comm.h" #include "hinic3_nic_dev.h" #include "hinic3_nic_cfg.h" +#include "hinic3_rss.h" #define HINIC3_MGMT_VERSION_MAX_LEN 32/* Coalesce time properties in microseconds */ @@ -1238,6 +1239,14 @@ static const struct ethtool_ops hinic3_ethtool_ops = { .get_pause_stats = hinic3_get_pause_stats, .get_coalesce = hinic3_get_coalesce, .set_coalesce = hinic3_set_coalesce, + .get_rxnfc = hinic3_get_rxnfc, + .set_rxnfc = hinic3_set_rxnfc, + .get_channels = hinic3_get_channels, + .set_channels = hinic3_set_channels, + .get_rxfh_indir_size = hinic3_get_rxfh_indir_size, + .get_rxfh_key_size = hinic3_get_rxfh_key_size, + .get_rxfh = hinic3_get_rxfh, + .set_rxfh = hinic3_set_rxfh, };void hinic3_set_ethtool_ops (struct net_device *netdev)drivers/net/ethernet/huawei/hinic3/hinic3_mgmt_interface.h +2
-0@@ -282,6 +282,7 @@ enum l2nic_cmd {L2NIC_CMD_SET_VLAN_FILTER_EN =26 ,L2NIC_CMD_SET_RX_VLAN_OFFLOAD =27 ,L2NIC_CMD_CFG_RSS =60 , +L2NIC_CMD_GET_RSS_CTX_TBL =62 ,L2NIC_CMD_CFG_RSS_HASH_KEY =63 ,L2NIC_CMD_CFG_RSS_HASH_ENGINE =64 , L2NIC_CMD_SET_RSS_CTX_TBL = 65,@@ -301,6 +302,7 @@ enum l2nic_ucode_cmd {L2NIC_UCODE_CMD_MODIFY_QUEUE_CTX =0 ,L2NIC_UCODE_CMD_CLEAN_QUEUE_CTX =1 ,L2NIC_UCODE_CMD_SET_RSS_INDIR_TBL =4 , + L2NIC_UCODE_CMD_GET_RSS_INDIR_TBL = 6, };/* hilink mac group command */ drivers/net/ethernet/huawei/hinic3/hinic3_rss.c +219
-2@@ -155,7 +155,7 @@ static int hinic3_set_rss_type(struct hinic3_hwdev *hwdev, L2NIC_CMD_SET_RSS_CTX_TBL, &msg_params);if (ctx_tbl.msg_head.status ==MGMT_STATUS_CMD_UNSUPPORTED ) { -return MGMT_STATUS_CMD_UNSUPPORTED ; +return -EOPNOTSUPP ; }else if (err || ctx_tbl.msg_head.status) {dev_err (hwdev->dev,"mgmt Failed to set rss context offload, err: %d, status: 0x%x\n" , err, ctx_tbl.msg_head.status);@@ -165,6 +165,41 @@ static int hinic3_set_rss_type(struct hinic3_hwdev *hwdev,return 0 ; } +static int hinic3_get_rss_type (struct hinic3_hwdev *hwdev, +struct hinic3_rss_type *rss_type) +{ +struct l2nic_cmd_rss_ctx_tbl ctx_tbl = {}; +struct mgmt_msg_params msg_params = {}; +int err; + + ctx_tbl.func_id =hinic3_global_func_id (hwdev); + +mgmt_msg_params_init_default (&msg_params, &ctx_tbl,sizeof (ctx_tbl)); + + err =hinic3_send_mbox_to_mgmt (hwdev,MGMT_MOD_L2NIC , +L2NIC_CMD_GET_RSS_CTX_TBL , + &msg_params); +if (ctx_tbl.msg_head.status ==MGMT_STATUS_CMD_UNSUPPORTED ) { +return -EOPNOTSUPP ; + }else if (err || ctx_tbl.msg_head.status) { +dev_err (hwdev->dev,"Failed to get hash type, err: %d, status: 0x%x\n" , + err, ctx_tbl.msg_head.status); +return -EINVAL ; + } + + rss_type->ipv4 =L2NIC_RSS_TYPE_GET (ctx_tbl.context,IPV4 ); + rss_type->ipv6 =L2NIC_RSS_TYPE_GET (ctx_tbl.context,IPV6 ); + rss_type->ipv6_ext =L2NIC_RSS_TYPE_GET (ctx_tbl.context,IPV6_EXT ); + rss_type->tcp_ipv4 =L2NIC_RSS_TYPE_GET (ctx_tbl.context,TCP_IPV4 ); + rss_type->tcp_ipv6 =L2NIC_RSS_TYPE_GET (ctx_tbl.context,TCP_IPV6 ); + rss_type->tcp_ipv6_ext =L2NIC_RSS_TYPE_GET (ctx_tbl.context, +TCP_IPV6_EXT ); + rss_type->udp_ipv4 =L2NIC_RSS_TYPE_GET (ctx_tbl.context,UDP_IPV4 ); + rss_type->udp_ipv6 =L2NIC_RSS_TYPE_GET (ctx_tbl.context,UDP_IPV6 ); + +return 0 ; +} +static int hinic3_rss_cfg_hash_type (struct hinic3_hwdev *hwdev,u8 opcode,enum hinic3_rss_hash_type *type) {@@ -264,7 +299,8 @@ static int hinic3_set_hw_rss_parameters(struct net_device *netdev, u8 rss_en)if (err)return err; -hinic3_fillout_indir_tbl (netdev, nic_dev->rss_indir); +if (!netif_is_rxfh_configured (netdev)) +hinic3_fillout_indir_tbl (netdev, nic_dev->rss_indir); err =hinic3_config_rss_hw_resource (netdev, nic_dev->rss_indir);if (err)@@ -334,3 +370,502 @@ void hinic3_try_to_enable_rss(struct net_device *netdev)clear_bit (HINIC3_RSS_ENABLE , &nic_dev->flags); nic_dev->q_params.num_qps = nic_dev->max_qps; } + +static int hinic3_set_l4_rss_hash_ops (const struct ethtool_rxnfc *cmd, +struct hinic3_rss_type *rss_type) +{ +u8 rss_l4_en; + +switch (cmd->data & (RXH_L4_B_0_1 |RXH_L4_B_2_3 )) { +case 0 : + rss_l4_en =0 ; +break ; +case (RXH_L4_B_0_1 |RXH_L4_B_2_3 ): + rss_l4_en =1 ; +break ; +default : +return -EINVAL ; + } + +switch (cmd->flow_type) { +case TCP_V4_FLOW : + rss_type->tcp_ipv4 = rss_l4_en; +break ; +case TCP_V6_FLOW : + rss_type->tcp_ipv6 = rss_l4_en; +break ; +case UDP_V4_FLOW : + rss_type->udp_ipv4 = rss_l4_en; +break ; +case UDP_V6_FLOW : + rss_type->udp_ipv6 = rss_l4_en; +break ; +default : +return -EINVAL ; + } + +return 0 ; +} + +static int hinic3_update_rss_hash_opts (struct net_device *netdev, +struct ethtool_rxnfc *cmd, +struct hinic3_rss_type *rss_type) +{ +int err; + +switch (cmd->flow_type) { +case TCP_V4_FLOW : +case TCP_V6_FLOW : +case UDP_V4_FLOW : +case UDP_V6_FLOW : + err =hinic3_set_l4_rss_hash_ops (cmd, rss_type); +if (err) +return err; + +break ; +case IPV4_FLOW : + rss_type->ipv4 =1 ; +break ; +case IPV6_FLOW : + rss_type->ipv6 =1 ; +break ; +default : +netdev_err (netdev,"Unsupported flow type\n" ); +return -EINVAL ; + } + +return 0 ; +} + +static int hinic3_set_rss_hash_opts (struct net_device *netdev, +struct ethtool_rxnfc *cmd) +{ +struct hinic3_nic_dev *nic_dev =netdev_priv (netdev); +struct hinic3_rss_type rss_type; +int err; + +if (!test_bit (HINIC3_RSS_ENABLE , &nic_dev->flags)) { + cmd->data =0 ; +netdev_err (netdev,"RSS is disable, not support to set flow-hash\n" ); +return -EOPNOTSUPP ; + } + +/* RSS only supports hashing of IP addresses and L4 ports */ +if (cmd->data & ~(RXH_IP_SRC |RXH_IP_DST | +RXH_L4_B_0_1 |RXH_L4_B_2_3 )) +return -EINVAL ; + +/* Both IP addresses must be part of the hash tuple */ +if (!(cmd->data &RXH_IP_SRC ) || !(cmd->data &RXH_IP_DST )) +return -EINVAL ; + +/* L4 hash bits are not valid for pure L3 flow types */ +if ((cmd->flow_type ==IPV4_FLOW || cmd->flow_type ==IPV6_FLOW ) && + (cmd->data & (RXH_L4_B_0_1 |RXH_L4_B_2_3 ))) +return -EINVAL ; + + err =hinic3_get_rss_type (nic_dev->hwdev, &rss_type); +if (err) { +netdev_err (netdev,"Failed to get rss type\n" ); +return err; + } + + err =hinic3_update_rss_hash_opts (netdev, cmd, &rss_type); +if (err) +return err; + + err =hinic3_set_rss_type (nic_dev->hwdev, rss_type); +if (err) { +netdev_err (netdev,"Failed to set rss type\n" ); +return err; + } + + nic_dev->rss_type = rss_type; + +return 0 ; +} + +static void convert_rss_l3_type (u8 rss_opt,struct ethtool_rxnfc *cmd) +{ +if (!rss_opt) + cmd->data &= ~(RXH_IP_SRC |RXH_IP_DST ); +} + +static void convert_rss_l4_type (u8 rss_opt,struct ethtool_rxnfc *cmd) +{ +if (rss_opt) + cmd->data |=RXH_L4_B_0_1 |RXH_L4_B_2_3 ; +} + +static int hinic3_convert_rss_type (struct net_device *netdev, +struct hinic3_rss_type *rss_type, +struct ethtool_rxnfc *cmd) +{ + cmd->data =RXH_IP_SRC |RXH_IP_DST ; +switch (cmd->flow_type) { +case TCP_V4_FLOW : +convert_rss_l4_type (rss_type->tcp_ipv4, cmd); +break ; +case TCP_V6_FLOW : +convert_rss_l4_type (rss_type->tcp_ipv6, cmd); +break ; +case UDP_V4_FLOW : +convert_rss_l4_type (rss_type->udp_ipv4, cmd); +break ; +case UDP_V6_FLOW : +convert_rss_l4_type (rss_type->udp_ipv6, cmd); +break ; +case IPV4_FLOW : +convert_rss_l3_type (rss_type->ipv4, cmd); +break ; +case IPV6_FLOW : +convert_rss_l3_type (rss_type->ipv6, cmd); +break ; +default : +netdev_err (netdev,"Unsupported flow type\n" ); + cmd->data =0 ; +return -EINVAL ; + } + +return 0 ; +} + +static int hinic3_get_rss_hash_opts (struct net_device *netdev, +struct ethtool_rxnfc *cmd) +{ +struct hinic3_nic_dev *nic_dev =netdev_priv (netdev); +struct hinic3_rss_type rss_type; +int err; + + cmd->data =0 ; + +if (!test_bit (HINIC3_RSS_ENABLE , &nic_dev->flags)) +return 0 ; + + err =hinic3_get_rss_type (nic_dev->hwdev, &rss_type); +if (err) { +netdev_err (netdev,"Failed to get rss type\n" ); +return err; + } + +return hinic3_convert_rss_type (netdev, &rss_type, cmd); +} +
drivers/net/ethernet/huawei/hinic3/hinic3_rss.c void hinic3_try_to_enable_rss(struct net_device *netdev) · line 563
Quoted 10 lines
drivers/net/ethernet/huawei/hinic3/hinic3_rss.c +10
-0+int hinic3_get_rxnfc (struct net_device *netdev, +struct ethtool_rxnfc *cmd,u32 *rule_locs) +{ +struct hinic3_nic_dev *nic_dev =netdev_priv (netdev); +int err =0 ; + +switch (cmd->cmd) { +case ETHTOOL_GRXRINGS : + cmd->data = nic_dev->q_params.num_qps; +break ;
You should probably implement the get_rx_ring_count ethtool op instead. See
https://lore.kernel.org/netdev/[email protected]/
Simon Horman
reviewerRe: [PATCH net-next v09 4/5] hinic3: Add ethtool rss ops
In reply to Fan GongThis is an AI-generated review of your patch. The human sending this email has considered the AI review valid, or at least plausible. Full review at: https://sashiko.dev
---
hinic3: Add ethtool rss ops
This commit implements ethtool callback functions for the hinic3 driver. These callbacks allow users to utilize ethtool for detailed RSS parameters configuration and monitoring.
Quoted 2 lines
drivers/net/ethernet/huawei/hinic3/hinic3_rss.c +0
-0
[ ... ]
Quoted 3 lines
drivers/net/ethernet/huawei/hinic3/hinic3_rss.c +3
-0+int hinic3_set_channels (struct net_device *netdev, +struct ethtool_channels *channels) +{
[ ... ]
Quoted 9 lines
drivers/net/ethernet/huawei/hinic3/hinic3_rss.c +9
-0+if (netif_running (netdev)) { + q_params = nic_dev->q_params; + q_params.num_qps = (u16 )count; + q_params.txqs_res =NULL ; + q_params.rxqs_res =NULL ; + q_params.irq_cfg =NULL ; + + err =hinic3_change_channel_settings (netdev, &q_params); +if (err) {
drivers/net/ethernet/huawei/hinic3/hinic3_rss.c void hinic3_try_to_enable_rss(struct net_device *netdev) · line 709
Quoted 10 lines
drivers/net/ethernet/huawei/hinic3/hinic3_rss.c +10
-0+netdev_err (netdev,"Failed to change channel settings\n" ); +return err; + } + }else { + nic_dev->q_params.num_qps = (u16 )count; + } + + err =hinic3_rss_update_num_qps_and_reprogram (netdev); +if (err) +return err;
[Severity: Medium]
If hinic3_rss_update_num_qps_and_reprogram() fails, the error code is returned to the caller, but the queue changes made earlier by hinic3_change_channel_settings() remain active. Is it expected that a failure leaves the newly requested channel configuration running?
[ ... ]
Quoted 3 lines
drivers/net/ethernet/huawei/hinic3/hinic3_rss.c +3
-0+static int hinic3_set_rss_rxfh (struct net_device *netdev, +const u32 *indir,u8 *key) +{
[ ... ]
drivers/net/ethernet/huawei/hinic3/hinic3_rss.c void hinic3_try_to_enable_rss(struct net_device *netdev) · line 737
Quoted 11 lines
drivers/net/ethernet/huawei/hinic3/hinic3_rss.c +11
-0+if (indir) { +for (i =0 ; i <L2NIC_RSS_INDIR_SIZE ; i++) + temp_indir[i] = (u16 )indir[i]; + + err =hinic3_rss_set_indir_tbl (nic_dev->hwdev, temp_indir); +if (err) { +netdev_err (netdev,"Failed to set rss indir table\n" ); +return err; + } + +memcpy (nic_dev->rss_indir, temp_indir,L2NIC_RSS_INDIR_SIZE );
[Severity: High]
Does this copy the entire indirection table? Since temp_indir is an array of u16 elements, copying L2NIC_RSS_INDIR_SIZE bytes instead of L2NIC_RSS_INDIR_SIZE * sizeof(u16) will only update the first half of the nic_dev->rss_indir array.
[ ... ]
Quoted 3 lines
drivers/net/ethernet/huawei/hinic3/hinic3_rss.c +3
-0+static int hinic3_rss_get_indir_tbl (struct hinic3_hwdev *hwdev, +u32 *indir_table) +{
[ ... ]
drivers/net/ethernet/huawei/hinic3/hinic3_rss.c void hinic3_try_to_enable_rss(struct net_device *netdev) · line 783
Quoted 11 lines
drivers/net/ethernet/huawei/hinic3/hinic3_rss.c +11
-0+ err =hinic3_cmdq_detail_resp (hwdev,MGMT_MOD_L2NIC , +L2NIC_UCODE_CMD_GET_RSS_INDIR_TBL , + pair.in, pair.out,NULL ); +if (err) { +dev_err (hwdev->dev,"Failed to get rss indir table\n" ); + goto err_get_indir_tbl; + } + + indir_tbl = (__force __le16 *)pair.out->buf; +for (i =0 ; i <L2NIC_RSS_INDIR_SIZE ; i++) + indir_table[i] =le16_to_cpu (*(indir_tbl + i));
[Severity: High]
Should the response buffer words be swabbed using hinic3_cmdq_buf_swab32() before reading, similar to how it is handled in the set operation? Also, does the data start immediately at offset 0, or is there a 16-byte reserved prefix like the one defined in struct l2nic_cmd_rss_set_indir_tbl that needs to be skipped?
[ ... ]
Quoted 5 lines
drivers/net/ethernet/huawei/hinic3/hinic3_rss.c +5
-0+static int hinic3_update_hash_func_type (struct net_device *netdev,u8 hfunc) +{ +struct hinic3_nic_dev *nic_dev =netdev_priv (netdev); +enum hinic3_rss_hash_type new_rss_hash_type; +int err;
[ ... ]
drivers/net/ethernet/huawei/hinic3/hinic3_rss.c void hinic3_try_to_enable_rss(struct net_device *netdev) · line 841
Quoted 4 lines
drivers/net/ethernet/huawei/hinic3/hinic3_rss.c +4
-0+if (new_rss_hash_type == nic_dev->rss_hash_type) +return 0 ; + + err =hinic3_rss_set_hash_type (nic_dev->hwdev, nic_dev->rss_hash_type);
[Severity: Medium]
Will this push the old hash type to the hardware instead of the newly requested one? It passes nic_dev->rss_hash_type rather than new_rss_hash_type.
Quoted 9 lines
drivers/net/ethernet/huawei/hinic3/hinic3_rss.c +9
-0+if (err) { +netdev_err (netdev,"Failed to set RSS hash type to HW\n" ); +return err; + } + + nic_dev->rss_hash_type = new_rss_hash_type; + +return 0 ; +}
Fan Gong
maintainerPATCH v9 5/5 hinic3: Remove unneeded coalesce parameters
In reply to Fan GongRemove unneeded coalesce parameters in irq handling.
Co-developed-by Wu Di <[email protected]>
Signed-off-by Wu Di <[email protected]>
Co-developed-by Teng Peisen <[email protected]>
Signed-off-by Teng Peisen <[email protected]>
Signed-off-by Fan Gong <[email protected]>
--- drivers/net/ethernet/huawei/hinic3/hinic3_irq.c | 6 +----- drivers/net/ethernet/huawei/hinic3/hinic3_rx.h | 3 --- 2 files changed, 1 insertion(+), 8 deletions(-)
drivers/net/ethernet/huawei/hinic3/hinic3_irq.c +1 -5
@@ -149,13 +149,9 @@ static int hinic3_set_interrupt_moder(struct net_device *netdev, u16 q_id, nic_dev->intr_coalesce[q_id].resend_timer_cfg; err =hinic3_set_interrupt_cfg (nic_dev->hwdev, info); -if (err) { +if (err)netdev_err (netdev,"Failed to modify moderation for Queue: %u\n" , q_id); - }else { - nic_dev->rxqs[q_id].last_coalesc_timer_cfg = coalesc_timer_cfg; - nic_dev->rxqs[q_id].last_pending_limit = pending_limit; - }mutex_unlock (&nic_dev->change_res_mutex);
drivers/net/ethernet/huawei/hinic3/hinic3_rx.h +0 -3
@@ -111,9 +111,6 @@ struct hinic3_rxq {dma_addr_t cqe_start_paddr;struct dim dim; - -u8 last_coalesc_timer_cfg; -u8 last_pending_limit; }____cacheline_aligned ;struct hinic3_dyna_rxq_res {
Signature · 1 line
2.43.0