There are some bugfix for the HNS ethernet driver
---
changeLog:
v2 -> v3:
- Refine the commit msg as Wojciech suggestions
- Reconstruct the "hns_mac_link_anti_shake" function suggested by Wojciech
v2: https://lore.kernel.org/all/[email protected]/
v1 -> v2:
- Fixed the internal function is not decorated with static issue, suggested by Jakub
v1: https://lore.kernel.org/all/[email protected]/
---
Yonglong Liu (2):
net: hns: fix wrong head when modify the tx feature when sending
packets
net: hns: fix fake link up on xge port
.../net/ethernet/hisilicon/hns/hns_dsaf_mac.c | 29 ++++++++++
drivers/net/ethernet/hisilicon/hns/hns_enet.c | 53 +++++++++++--------
drivers/net/ethernet/hisilicon/hns/hns_enet.h | 3 +-
3 files changed, 62 insertions(+), 23 deletions(-)
--
2.30.0
From: Yonglong Liu <[email protected]>
Upon changing the tx feature, the hns driver will modify the
maybe_stop_tx() and fill_desc() functions, if the modify happens
during packet sending, will cause the hardware and software
pointers do not match, and the port can not work anymore.
This patch deletes the maybe_stop_tx() and fill_desc() functions
modification when setting tx feature, and use the skb_is_gro()
to determine which functions to use in the tx path.
Fixes: 38f616da1c28 ("net:hns: Add support of ethtool TSO set option for Hip06 in HNS")
Signed-off-by: Yonglong Liu <[email protected]>
Signed-off-by: Jijie Shao <[email protected]>
Reviewed-by: Wojciech Drewek <[email protected]>
---
drivers/net/ethernet/hisilicon/hns/hns_enet.c | 53 +++++++++++--------
drivers/net/ethernet/hisilicon/hns/hns_enet.h | 3 +-
2 files changed, 33 insertions(+), 23 deletions(-)
diff --git a/drivers/net/ethernet/hisilicon/hns/hns_enet.c b/drivers/net/ethernet/hisilicon/hns/hns_enet.c
index 0900abf5c508..8a713eed4465 100644
--- a/drivers/net/ethernet/hisilicon/hns/hns_enet.c
+++ b/drivers/net/ethernet/hisilicon/hns/hns_enet.c
@@ -142,7 +142,8 @@ MODULE_DEVICE_TABLE(acpi, hns_enet_acpi_match);
static void fill_desc(struct hnae_ring *ring, void *priv,
int size, dma_addr_t dma, int frag_end,
- int buf_num, enum hns_desc_type type, int mtu)
+ int buf_num, enum hns_desc_type type, int mtu,
+ bool is_gso)
{
struct hnae_desc *desc = &ring->desc[ring->next_to_use];
struct hnae_desc_cb *desc_cb = &ring->desc_cb[ring->next_to_use];
@@ -275,6 +276,15 @@ static int hns_nic_maybe_stop_tso(
return 0;
}
+static int hns_nic_maybe_stop_tx_v2(struct sk_buff **out_skb, int *bnum,
+ struct hnae_ring *ring)
+{
+ if (skb_is_gso(*out_skb))
+ return hns_nic_maybe_stop_tso(out_skb, bnum, ring);
+ else
+ return hns_nic_maybe_stop_tx(out_skb, bnum, ring);
+}
+
static void fill_tso_desc(struct hnae_ring *ring, void *priv,
int size, dma_addr_t dma, int frag_end,
int buf_num, enum hns_desc_type type, int mtu)
@@ -300,6 +310,19 @@ static void fill_tso_desc(struct hnae_ring *ring, void *priv,
mtu);
}
+static void fill_desc_v2(struct hnae_ring *ring, void *priv,
+ int size, dma_addr_t dma, int frag_end,
+ int buf_num, enum hns_desc_type type, int mtu,
+ bool is_gso)
+{
+ if (is_gso)
+ fill_tso_desc(ring, priv, size, dma, frag_end, buf_num, type,
+ mtu);
+ else
+ fill_v2_desc(ring, priv, size, dma, frag_end, buf_num, type,
+ mtu);
+}
+
netdev_tx_t hns_nic_net_xmit_hw(struct net_device *ndev,
struct sk_buff *skb,
struct hns_nic_ring_data *ring_data)
@@ -313,6 +336,7 @@ netdev_tx_t hns_nic_net_xmit_hw(struct net_device *ndev,
int seg_num;
dma_addr_t dma;
int size, next_to_use;
+ bool is_gso;
int i;
switch (priv->ops.maybe_stop_tx(&skb, &buf_num, ring)) {
@@ -339,8 +363,9 @@ netdev_tx_t hns_nic_net_xmit_hw(struct net_device *ndev,
ring->stats.sw_err_cnt++;
goto out_err_tx_ok;
}
+ is_gso = skb_is_gso(skb);
priv->ops.fill_desc(ring, skb, size, dma, seg_num == 1 ? 1 : 0,
- buf_num, DESC_TYPE_SKB, ndev->mtu);
+ buf_num, DESC_TYPE_SKB, ndev->mtu, is_gso);
/* fill the fragments */
for (i = 1; i < seg_num; i++) {
@@ -354,7 +379,7 @@ netdev_tx_t hns_nic_net_xmit_hw(struct net_device *ndev,
}
priv->ops.fill_desc(ring, skb_frag_page(frag), size, dma,
seg_num - 1 == i ? 1 : 0, buf_num,
- DESC_TYPE_PAGE, ndev->mtu);
+ DESC_TYPE_PAGE, ndev->mtu, is_gso);
}
/*complete translate all packets*/
@@ -1776,15 +1801,6 @@ static int hns_nic_set_features(struct net_device *netdev,
netdev_info(netdev, "enet v1 do not support tso!\n");
break;
default:
- if (features & (NETIF_F_TSO | NETIF_F_TSO6)) {
- priv->ops.fill_desc = fill_tso_desc;
- priv->ops.maybe_stop_tx = hns_nic_maybe_stop_tso;
- /* The chip only support 7*4096 */
- netif_set_tso_max_size(netdev, 7 * 4096);
- } else {
- priv->ops.fill_desc = fill_v2_desc;
- priv->ops.maybe_stop_tx = hns_nic_maybe_stop_tx;
- }
break;
}
netdev->features = features;
@@ -2159,16 +2175,9 @@ static void hns_nic_set_priv_ops(struct net_device *netdev)
priv->ops.maybe_stop_tx = hns_nic_maybe_stop_tx;
} else {
priv->ops.get_rxd_bnum = get_v2rx_desc_bnum;
- if ((netdev->features & NETIF_F_TSO) ||
- (netdev->features & NETIF_F_TSO6)) {
- priv->ops.fill_desc = fill_tso_desc;
- priv->ops.maybe_stop_tx = hns_nic_maybe_stop_tso;
- /* This chip only support 7*4096 */
- netif_set_tso_max_size(netdev, 7 * 4096);
- } else {
- priv->ops.fill_desc = fill_v2_desc;
- priv->ops.maybe_stop_tx = hns_nic_maybe_stop_tx;
- }
+ priv->ops.fill_desc = fill_desc_v2;
+ priv->ops.maybe_stop_tx = hns_nic_maybe_stop_tx_v2;
+ netif_set_tso_max_size(netdev, 7 * 4096);
/* enable tso when init
* control tso on/off through TSE bit in bd
*/
diff --git a/drivers/net/ethernet/hisilicon/hns/hns_enet.h b/drivers/net/ethernet/hisilicon/hns/hns_enet.h
index ffa9d6573f54..3f3ee032f631 100644
--- a/drivers/net/ethernet/hisilicon/hns/hns_enet.h
+++ b/drivers/net/ethernet/hisilicon/hns/hns_enet.h
@@ -44,7 +44,8 @@ struct hns_nic_ring_data {
struct hns_nic_ops {
void (*fill_desc)(struct hnae_ring *ring, void *priv,
int size, dma_addr_t dma, int frag_end,
- int buf_num, enum hns_desc_type type, int mtu);
+ int buf_num, enum hns_desc_type type, int mtu,
+ bool is_gso);
int (*maybe_stop_tx)(struct sk_buff **out_skb,
int *bnum, struct hnae_ring *ring);
void (*get_rxd_bnum)(u32 bnum_flag, int *out_bnum);
--
2.30.0
On Mon, 2023-12-04 at 22:32 +0800, Jijie Shao wrote:
> @@ -2159,16 +2175,9 @@ static void hns_nic_set_priv_ops(struct net_device *netdev)
> priv->ops.maybe_stop_tx = hns_nic_maybe_stop_tx;
> } else {
> priv->ops.get_rxd_bnum = get_v2rx_desc_bnum;
> - if ((netdev->features & NETIF_F_TSO) ||
> - (netdev->features & NETIF_F_TSO6)) {
> - priv->ops.fill_desc = fill_tso_desc;
> - priv->ops.maybe_stop_tx = hns_nic_maybe_stop_tso;
> - /* This chip only support 7*4096 */
> - netif_set_tso_max_size(netdev, 7 * 4096);
> - } else {
> - priv->ops.fill_desc = fill_v2_desc;
> - priv->ops.maybe_stop_tx = hns_nic_maybe_stop_tx;
> - }
> + priv->ops.fill_desc = fill_desc_v2;
> + priv->ops.maybe_stop_tx = hns_nic_maybe_stop_tx_v2;
> + netif_set_tso_max_size(netdev, 7 * 4096);
> /* enable tso when init
> * control tso on/off through TSE bit in bd
> */
Side note: since both 'fill_desc' and 'maybe_stop_tx' have constant
values, for net-next you should really consider replacing the function
pointers with direct-calls.
You currently have at least 2 indirect calls per wire packet, which
hurt performances a lot in case security issues mitigations are in
place.
Cheers,
Paolo
Hello:
This series was applied to netdev/net.git (main)
by Paolo Abeni <[email protected]>:
On Mon, 4 Dec 2023 22:32:30 +0800 you wrote:
> There are some bugfix for the HNS ethernet driver
>
> ---
> changeLog:
> v2 -> v3:
> - Refine the commit msg as Wojciech suggestions
> - Reconstruct the "hns_mac_link_anti_shake" function suggested by Wojciech
> v2: https://lore.kernel.org/all/[email protected]/
> v1 -> v2:
> - Fixed the internal function is not decorated with static issue, suggested by Jakub
> v1: https://lore.kernel.org/all/[email protected]/
>
> [...]
Here is the summary with links:
- [V3,net,1/2] net: hns: fix wrong head when modify the tx feature when sending packets
https://git.kernel.org/netdev/net/c/84757d083945
- [V3,net,2/2] net: hns: fix fake link up on xge port
https://git.kernel.org/netdev/net/c/f708aba40f9c
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
on 2023/12/6 19:18, Paolo Abeni wrote:
> + priv->ops.fill_desc = fill_desc_v2;
> + priv->ops.maybe_stop_tx = hns_nic_maybe_stop_tx_v2;
> Side note: since both 'fill_desc' and 'maybe_stop_tx' have constant
> values, for net-next you should really consider replacing the function
> pointers with direct-calls.
>
> You currently have at least 2 indirect calls per wire packet, which
> hurt performances a lot in case security issues mitigations are in
> place.
>
> Cheers,
>
> Paolo
Thank you for your advice. Currently, because the hardware behavior is
different, the two versions of ops are retained to unify the subsequent
process. We will try to unify the two version ops, and if that does not
work, we will consider maintaining the status quo. Thanks again! Jijie
On Wed, 2023-12-06 at 20:31 +0800, Jijie Shao wrote:
> on 2023/12/6 19:18, Paolo Abeni wrote:
> > + priv->ops.fill_desc = fill_desc_v2;
> > + priv->ops.maybe_stop_tx = hns_nic_maybe_stop_tx_v2;
> > Side note: since both 'fill_desc' and 'maybe_stop_tx' have constant
> > values, for net-next you should really consider replacing the function
> > pointers with direct-calls.
> >
> > You currently have at least 2 indirect calls per wire packet, which
> > hurt performances a lot in case security issues mitigations are in
> > place.
> >
> > Cheers,
> >
> > Paolo
>
> Thank you for your advice. Currently, because the hardware behavior is
> different, the two versions of ops are retained to unify the subsequent
> process. We will try to unify the two version ops, and if that does not
> work, we will consider maintaining the status quo. Thanks again! Jijie
Note that even if you will have to resort to call different functions
for different H/W revisions, from performance PoV you will be far
better off doing a test at runtime to call the corresponding helper.
Or you can use the indirect call wrappers - have a look into:
include/linux/indirect_call_wrapper.h
Cheers,
Paolo