| Message ID | 20250113082245.2332775-8-wei.fang@nxp.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Add more feautues for ENETC v4 - round 2 | expand |
On Mon, 13 Jan 2025 16:22:39 +0800 Wei Fang wrote: > Add Receive side scaling (RSS) support for i.MX95 ENETC PF to improve the > network performance and balance the CPU loading. In addition, since both > ENETC v1 and ENETC v4 only support the toeplitz algorithm, so a check for > hfunc was added. This and previous commits are a bi hard to follow. You plumb some stuff thru in the previous commit. In this one you reshuffle things, again. Try to separate code movement / restructuring in one commit. And new additions more clearly in the next. > +static void enetc4_set_rss_key(struct enetc_hw *hw, const u8 *key) > +{ > + int i; > + > + for (i = 0; i < ENETC_RSSHASH_KEY_SIZE / 4; i++) > + enetc_port_wr(hw, ENETC4_PRSSKR(i), ((u32 *)key)[i]); > +} > + > +static void enetc4_get_rss_key(struct enetc_hw *hw, u8 *key) > +{ > + int i; > + > + for (i = 0; i < ENETC_RSSHASH_KEY_SIZE / 4; i++) > + ((u32 *)key)[i] = enetc_port_rd(hw, ENETC4_PRSSKR(i)); > +} Isn't the only difference between the chips the register offset? Why create full ops for something this trivial? > +static int enetc4_get_rxnfc(struct net_device *ndev, struct ethtool_rxnfc *rxnfc, > + u32 *rule_locs) > +{ > + struct enetc_ndev_priv *priv = netdev_priv(ndev); > + > + switch (rxnfc->cmd) { > + case ETHTOOL_GRXRINGS: > + rxnfc->data = priv->num_rx_rings; > + break; > + case ETHTOOL_GRXFH: > + return enetc_get_rsshash(rxnfc); > + default: > + return -EOPNOTSUPP; > + } > + > + return 0; > +} Why add a new function instead of returning EOPNOTSUPP for new chips in the existing one? > @@ -712,6 +730,12 @@ static int enetc_set_rxfh(struct net_device *ndev, > struct enetc_hw *hw = &si->hw; > int err = 0; > > + if (rxfh->hfunc != ETH_RSS_HASH_NO_CHANGE && > + rxfh->hfunc != ETH_RSS_HASH_TOP) { > + netdev_err(ndev, "Only toeplitz hash function is supported\n"); > + return -EOPNOTSUPP; Should be a separate commit.
> On Mon, 13 Jan 2025 16:22:39 +0800 Wei Fang wrote: > > Add Receive side scaling (RSS) support for i.MX95 ENETC PF to improve the > > network performance and balance the CPU loading. In addition, since both > > ENETC v1 and ENETC v4 only support the toeplitz algorithm, so a check for > > hfunc was added. > > This and previous commits are a bi hard to follow. You plumb some > stuff thru in the previous commit. In this one you reshuffle things, > again. Try to separate code movement / restructuring in one commit. > And new additions more clearly in the next. Okay, I will. > > > +static void enetc4_set_rss_key(struct enetc_hw *hw, const u8 *key) > > +{ > > + int i; > > + > > + for (i = 0; i < ENETC_RSSHASH_KEY_SIZE / 4; i++) > > + enetc_port_wr(hw, ENETC4_PRSSKR(i), ((u32 *)key)[i]); > > +} > > + > > +static void enetc4_get_rss_key(struct enetc_hw *hw, u8 *key) > > +{ > > + int i; > > + > > + for (i = 0; i < ENETC_RSSHASH_KEY_SIZE / 4; i++) > > + ((u32 *)key)[i] = enetc_port_rd(hw, ENETC4_PRSSKR(i)); > > +} > > Isn't the only difference between the chips the register offset? Yes. > Why create full ops for something this trivial? We add enetc_pf_hw_ops to implement different hardware ops for different chips. So that they can be called in common functions. Although the change is minor, it is consistent with the original intention of adding enetc_pf_hw_ops. > > > +static int enetc4_get_rxnfc(struct net_device *ndev, struct ethtool_rxnfc > *rxnfc, > > + u32 *rule_locs) > > +{ > > + struct enetc_ndev_priv *priv = netdev_priv(ndev); > > + > > + switch (rxnfc->cmd) { > > + case ETHTOOL_GRXRINGS: > > + rxnfc->data = priv->num_rx_rings; > > + break; > > + case ETHTOOL_GRXFH: > > + return enetc_get_rsshash(rxnfc); > > + default: > > + return -EOPNOTSUPP; > > + } > > + > > + return 0; > > +} > > Why add a new function instead of returning EOPNOTSUPP for new chips > in the existing one? We will add ETHTOOL_G/SRXCLSXXX in the future, but both the hardware and software implementation of ENETC4 are different from ENETC1, and we don't want to mix them in one function, which would look a bit messy. > > > @@ -712,6 +730,12 @@ static int enetc_set_rxfh(struct net_device *ndev, > > struct enetc_hw *hw = &si->hw; > > int err = 0; > > > > + if (rxfh->hfunc != ETH_RSS_HASH_NO_CHANGE && > > + rxfh->hfunc != ETH_RSS_HASH_TOP) { > > + netdev_err(ndev, "Only toeplitz hash function is supported\n"); > > + return -EOPNOTSUPP; > > Should be a separate commit. > -- > pw-bot: cr
On Thu, 16 Jan 2025 02:24:10 +0000 Wei Fang wrote: > > Why create full ops for something this trivial? > > We add enetc_pf_hw_ops to implement different hardware ops > for different chips. So that they can be called in common functions. > Although the change is minor, it is consistent with the original > intention of adding enetc_pf_hw_ops. In other words you prefer ops. Now imagine you have to refactor such piece of code in 10 drivers and each of them has 2 layers of indirect ops like you do. Unnecessary complexity.
> -----Original Message----- > From: Jakub Kicinski <kuba@kernel.org> > Sent: 2025年1月16日 10:41 > To: Wei Fang <wei.fang@nxp.com> > Cc: Claudiu Manoil <claudiu.manoil@nxp.com>; Vladimir Oltean > <vladimir.oltean@nxp.com>; Clark Wang <xiaoning.wang@nxp.com>; > andrew+netdev@lunn.ch; davem@davemloft.net; edumazet@google.com; > pabeni@redhat.com; christophe.leroy@csgroup.eu; netdev@vger.kernel.org; > linux-kernel@vger.kernel.org; linuxppc-dev@lists.ozlabs.org; > linux-arm-kernel@lists.infradead.org; imx@lists.linux.dev > Subject: Re: [PATCH v2 net-next 07/13] net: enetc: add RSS support for i.MX95 > ENETC PF > > On Thu, 16 Jan 2025 02:24:10 +0000 Wei Fang wrote: > > > Why create full ops for something this trivial? > > > > We add enetc_pf_hw_ops to implement different hardware ops > > for different chips. So that they can be called in common functions. > > Although the change is minor, it is consistent with the original > > intention of adding enetc_pf_hw_ops. > > In other words you prefer ops. > > Now imagine you have to refactor such piece of code in 10 drivers > and each of them has 2 layers of indirect ops like you do. > Unnecessary complexity. Okay, I will remove them from ops.
diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c b/drivers/net/ethernet/freescale/enetc/enetc.c index 6d21c133e418..233f58e57a20 100644 --- a/drivers/net/ethernet/freescale/enetc/enetc.c +++ b/drivers/net/ethernet/freescale/enetc/enetc.c @@ -2363,7 +2363,7 @@ static int enetc_setup_default_rss_table(struct enetc_si *si, int num_groups) for (i = 0; i < si->num_rss; i++) rss_table[i] = i % num_groups; - enetc_set_rss_table(si, rss_table, si->num_rss); + si->ops->set_rss_table(si, rss_table, si->num_rss); kfree(rss_table); @@ -2394,10 +2394,7 @@ int enetc_configure_si(struct enetc_ndev_priv *priv) if (si->hw_features & ENETC_SI_F_LSO) enetc_set_lso_flags_mask(hw); - /* TODO: RSS support for i.MX95 will be supported later, and the - * is_enetc_rev1() condition will be removed - */ - if (si->num_rss && is_enetc_rev1(si)) { + if (si->num_rss) { err = enetc_setup_default_rss_table(si, priv->num_rx_rings); if (err) return err; diff --git a/drivers/net/ethernet/freescale/enetc/enetc.h b/drivers/net/ethernet/freescale/enetc/enetc.h index fb53fb961364..2b0d27ed924d 100644 --- a/drivers/net/ethernet/freescale/enetc/enetc.h +++ b/drivers/net/ethernet/freescale/enetc/enetc.h @@ -290,6 +290,8 @@ struct enetc_si; struct enetc_si_ops { int (*setup_cbdr)(struct enetc_si *si); void (*teardown_cbdr)(struct enetc_si *si); + int (*get_rss_table)(struct enetc_si *si, u32 *table, int count); + int (*set_rss_table)(struct enetc_si *si, const u32 *table, int count); }; /* PCI IEP device data */ @@ -540,6 +542,8 @@ int enetc_set_fs_entry(struct enetc_si *si, struct enetc_cmd_rfse *rfse, int enetc_get_rss_table(struct enetc_si *si, u32 *table, int count); int enetc_set_rss_table(struct enetc_si *si, const u32 *table, int count); int enetc_send_cmd(struct enetc_si *si, struct enetc_cbd *cbd); +int enetc4_get_rss_table(struct enetc_si *si, u32 *table, int count); +int enetc4_set_rss_table(struct enetc_si *si, const u32 *table, int count); static inline void *enetc_cbd_alloc_data_mem(struct enetc_si *si, struct enetc_cbd *cbd, diff --git a/drivers/net/ethernet/freescale/enetc/enetc4_pf.c b/drivers/net/ethernet/freescale/enetc/enetc4_pf.c index 798c69e83c8f..adb5819c091f 100644 --- a/drivers/net/ethernet/freescale/enetc/enetc4_pf.c +++ b/drivers/net/ethernet/freescale/enetc/enetc4_pf.c @@ -69,9 +69,27 @@ static void enetc4_pf_get_si_primary_mac(struct enetc_hw *hw, int si, put_unaligned_le16(lower, addr + 4); } +static void enetc4_set_rss_key(struct enetc_hw *hw, const u8 *key) +{ + int i; + + for (i = 0; i < ENETC_RSSHASH_KEY_SIZE / 4; i++) + enetc_port_wr(hw, ENETC4_PRSSKR(i), ((u32 *)key)[i]); +} + +static void enetc4_get_rss_key(struct enetc_hw *hw, u8 *key) +{ + int i; + + for (i = 0; i < ENETC_RSSHASH_KEY_SIZE / 4; i++) + ((u32 *)key)[i] = enetc_port_rd(hw, ENETC4_PRSSKR(i)); +} + static const struct enetc_pf_ops enetc4_pf_ops = { .set_si_primary_mac = enetc4_pf_set_si_primary_mac, .get_si_primary_mac = enetc4_pf_get_si_primary_mac, + .set_rss_key = enetc4_set_rss_key, + .get_rss_key = enetc4_get_rss_key, }; static int enetc4_pf_struct_init(struct enetc_si *si) @@ -263,14 +281,6 @@ static void enetc4_set_trx_frame_size(struct enetc_pf *pf) enetc4_pf_reset_tc_msdu(&si->hw); } -static void enetc4_set_rss_key(struct enetc_hw *hw, const u8 *bytes) -{ - int i; - - for (i = 0; i < ENETC_RSSHASH_KEY_SIZE / 4; i++) - enetc_port_wr(hw, ENETC4_PRSSKR(i), ((u32 *)bytes)[i]); -} - static void enetc4_set_default_rss_key(struct enetc_pf *pf) { u8 hash_key[ENETC_RSSHASH_KEY_SIZE] = {0}; @@ -691,6 +701,14 @@ static void enetc4_pf_set_rx_mode(struct net_device *ndev) queue_work(si->workqueue, &si->rx_mode_task); } +static int enetc4_pf_set_features(struct net_device *ndev, + netdev_features_t features) +{ + enetc_set_features(ndev, features); + + return 0; +} + static const struct net_device_ops enetc4_ndev_ops = { .ndo_open = enetc_open, .ndo_stop = enetc_close, @@ -698,6 +716,7 @@ static const struct net_device_ops enetc4_ndev_ops = { .ndo_get_stats = enetc_get_stats, .ndo_set_mac_address = enetc_pf_set_mac_addr, .ndo_set_rx_mode = enetc4_pf_set_rx_mode, + .ndo_set_features = enetc4_pf_set_features, }; static struct phylink_pcs * @@ -1106,6 +1125,8 @@ static void enetc4_pf_netdev_destroy(struct enetc_si *si) static const struct enetc_si_ops enetc4_psi_ops = { .setup_cbdr = enetc4_setup_cbdr, .teardown_cbdr = enetc4_teardown_cbdr, + .get_rss_table = enetc4_get_rss_table, + .set_rss_table = enetc4_set_rss_table, }; static int enetc4_pf_wq_task_init(struct enetc_si *si) diff --git a/drivers/net/ethernet/freescale/enetc/enetc_cbdr.c b/drivers/net/ethernet/freescale/enetc/enetc_cbdr.c index 31bb82ee512d..6d85c0caa04f 100644 --- a/drivers/net/ethernet/freescale/enetc/enetc_cbdr.c +++ b/drivers/net/ethernet/freescale/enetc/enetc_cbdr.c @@ -295,3 +295,17 @@ int enetc_set_rss_table(struct enetc_si *si, const u32 *table, int count) return enetc_cmd_rss_table(si, (u32 *)table, count, false); } EXPORT_SYMBOL_GPL(enetc_set_rss_table); + +int enetc4_get_rss_table(struct enetc_si *si, u32 *table, int count) +{ + return ntmp_rsst_query_or_update_entry(&si->ntmp.cbdrs, + table, count, true); +} +EXPORT_SYMBOL_GPL(enetc4_get_rss_table); + +int enetc4_set_rss_table(struct enetc_si *si, const u32 *table, int count) +{ + return ntmp_rsst_query_or_update_entry(&si->ntmp.cbdrs, + (u32 *)table, count, false); +} +EXPORT_SYMBOL_GPL(enetc4_set_rss_table); diff --git a/drivers/net/ethernet/freescale/enetc/enetc_ethtool.c b/drivers/net/ethernet/freescale/enetc/enetc_ethtool.c index 56ba82830279..a3059498f146 100644 --- a/drivers/net/ethernet/freescale/enetc/enetc_ethtool.c +++ b/drivers/net/ethernet/freescale/enetc/enetc_ethtool.c @@ -626,6 +626,24 @@ static int enetc_get_rxnfc(struct net_device *ndev, struct ethtool_rxnfc *rxnfc, return 0; } +static int enetc4_get_rxnfc(struct net_device *ndev, struct ethtool_rxnfc *rxnfc, + u32 *rule_locs) +{ + struct enetc_ndev_priv *priv = netdev_priv(ndev); + + switch (rxnfc->cmd) { + case ETHTOOL_GRXRINGS: + rxnfc->data = priv->num_rx_rings; + break; + case ETHTOOL_GRXFH: + return enetc_get_rsshash(rxnfc); + default: + return -EOPNOTSUPP; + } + + return 0; +} + static int enetc_set_rxnfc(struct net_device *ndev, struct ethtool_rxnfc *rxnfc) { struct enetc_ndev_priv *priv = netdev_priv(ndev); @@ -698,7 +716,7 @@ static int enetc_get_rxfh(struct net_device *ndev, /* return RSS table */ if (rxfh->indir) - err = enetc_get_rss_table(si, rxfh->indir, si->num_rss); + err = si->ops->get_rss_table(si, rxfh->indir, si->num_rss); return err; } @@ -712,6 +730,12 @@ static int enetc_set_rxfh(struct net_device *ndev, struct enetc_hw *hw = &si->hw; int err = 0; + if (rxfh->hfunc != ETH_RSS_HASH_NO_CHANGE && + rxfh->hfunc != ETH_RSS_HASH_TOP) { + netdev_err(ndev, "Only toeplitz hash function is supported\n"); + return -EOPNOTSUPP; + } + /* set hash key, if PF */ if (rxfh->key && enetc_si_is_pf(si)) { struct enetc_pf *pf = enetc_si_priv(si); @@ -721,7 +745,7 @@ static int enetc_set_rxfh(struct net_device *ndev, /* set RSS table */ if (rxfh->indir) - err = enetc_set_rss_table(si, rxfh->indir, si->num_rss); + err = si->ops->set_rss_table(si, rxfh->indir, si->num_rss); return err; } @@ -1233,6 +1257,11 @@ const struct ethtool_ops enetc4_pf_ethtool_ops = { .set_wol = enetc_set_wol, .get_pauseparam = enetc_get_pauseparam, .set_pauseparam = enetc_set_pauseparam, + .get_rxnfc = enetc4_get_rxnfc, + .get_rxfh_key_size = enetc_get_rxfh_key_size, + .get_rxfh_indir_size = enetc_get_rxfh_indir_size, + .get_rxfh = enetc_get_rxfh, + .set_rxfh = enetc_set_rxfh, }; void enetc_set_ethtool_ops(struct net_device *ndev) diff --git a/drivers/net/ethernet/freescale/enetc/enetc_pf.c b/drivers/net/ethernet/freescale/enetc/enetc_pf.c index f050cf039733..59039d087695 100644 --- a/drivers/net/ethernet/freescale/enetc/enetc_pf.c +++ b/drivers/net/ethernet/freescale/enetc/enetc_pf.c @@ -923,6 +923,8 @@ static int enetc_pf_register_with_ierb(struct pci_dev *pdev) static const struct enetc_si_ops enetc_psi_ops = { .setup_cbdr = enetc_setup_cbdr, .teardown_cbdr = enetc_teardown_cbdr, + .get_rss_table = enetc_get_rss_table, + .set_rss_table = enetc_set_rss_table, }; static struct enetc_si *enetc_psi_create(struct pci_dev *pdev) diff --git a/drivers/net/ethernet/freescale/enetc/enetc_pf_common.c b/drivers/net/ethernet/freescale/enetc/enetc_pf_common.c index 3fd9b0727875..c346e0e3ad37 100644 --- a/drivers/net/ethernet/freescale/enetc/enetc_pf_common.c +++ b/drivers/net/ethernet/freescale/enetc/enetc_pf_common.c @@ -128,15 +128,15 @@ void enetc_pf_netdev_setup(struct enetc_si *si, struct net_device *ndev, if (si->hw_features & ENETC_SI_F_LSO) priv->active_offloads |= ENETC_F_LSO; + if (si->num_rss) + ndev->hw_features |= NETIF_F_RXHASH; + /* TODO: currently, i.MX95 ENETC driver does not support advanced features */ if (!is_enetc_rev1(si)) { ndev->hw_features &= ~(NETIF_F_HW_VLAN_CTAG_FILTER | NETIF_F_LOOPBACK); goto end; } - if (si->num_rss) - ndev->hw_features |= NETIF_F_RXHASH; - ndev->xdp_features = NETDEV_XDP_ACT_BASIC | NETDEV_XDP_ACT_REDIRECT | NETDEV_XDP_ACT_NDO_XMIT | NETDEV_XDP_ACT_RX_SG | NETDEV_XDP_ACT_NDO_XMIT_SG; diff --git a/drivers/net/ethernet/freescale/enetc/enetc_vf.c b/drivers/net/ethernet/freescale/enetc/enetc_vf.c index d7d9a720069b..072e5b40a199 100644 --- a/drivers/net/ethernet/freescale/enetc/enetc_vf.c +++ b/drivers/net/ethernet/freescale/enetc/enetc_vf.c @@ -165,6 +165,8 @@ static void enetc_vf_netdev_setup(struct enetc_si *si, struct net_device *ndev, static const struct enetc_si_ops enetc_vsi_ops = { .setup_cbdr = enetc_setup_cbdr, .teardown_cbdr = enetc_teardown_cbdr, + .get_rss_table = enetc_get_rss_table, + .set_rss_table = enetc_set_rss_table, }; static int enetc_vf_probe(struct pci_dev *pdev,
Add Receive side scaling (RSS) support for i.MX95 ENETC PF to improve the network performance and balance the CPU loading. In addition, since both ENETC v1 and ENETC v4 only support the toeplitz algorithm, so a check for hfunc was added. Signed-off-by: Wei Fang <wei.fang@nxp.com> --- drivers/net/ethernet/freescale/enetc/enetc.c | 7 +--- drivers/net/ethernet/freescale/enetc/enetc.h | 4 ++ .../net/ethernet/freescale/enetc/enetc4_pf.c | 37 +++++++++++++++---- .../net/ethernet/freescale/enetc/enetc_cbdr.c | 14 +++++++ .../ethernet/freescale/enetc/enetc_ethtool.c | 33 ++++++++++++++++- .../net/ethernet/freescale/enetc/enetc_pf.c | 2 + .../freescale/enetc/enetc_pf_common.c | 6 +-- .../net/ethernet/freescale/enetc/enetc_vf.c | 2 + 8 files changed, 87 insertions(+), 18 deletions(-)