| Message ID | 20240416062952.14196-3-jiawenwu@trustnetic.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Wangxun fixes | expand |
Jiawen Wu <jiawenwu@trustnetic.com> : > Add flag for reset state to avoid reading statistics when hardware > is reset. > > Fixes: 883b5984a5d2 ("net: wangxun: add ethtool_ops for ring parameters") > Signed-off-by: Jiawen Wu <jiawenwu@trustnetic.com> > --- > drivers/net/ethernet/wangxun/libwx/wx_hw.c | 3 +++ > drivers/net/ethernet/wangxun/libwx/wx_type.h | 6 +++++ > .../net/ethernet/wangxun/ngbe/ngbe_ethtool.c | 24 +++++++++++++++---- > .../ethernet/wangxun/txgbe/txgbe_ethtool.c | 24 +++++++++++++++---- > 4 files changed, 47 insertions(+), 10 deletions(-) > [...] > diff --git a/drivers/net/ethernet/wangxun/ngbe/ngbe_ethtool.c b/drivers/net/ethernet/wangxun/ngbe/ngbe_ethtool.c > index 786a652ae64f..0e85c5a6633e 100644 > --- a/drivers/net/ethernet/wangxun/ngbe/ngbe_ethtool.c > +++ b/drivers/net/ethernet/wangxun/ngbe/ngbe_ethtool.c > @@ -52,7 +52,8 @@ static int ngbe_set_ringparam(struct net_device *netdev, > struct wx *wx = netdev_priv(netdev); > u32 new_rx_count, new_tx_count; > struct wx_ring *temp_ring; > - int i; > + u8 timeout = 50; > + int i, err = 0; > > new_tx_count = clamp_t(u32, ring->tx_pending, WX_MIN_TXD, WX_MAX_TXD); > new_tx_count = ALIGN(new_tx_count, WX_REQ_TX_DESCRIPTOR_MULTIPLE); > @@ -64,6 +65,15 @@ static int ngbe_set_ringparam(struct net_device *netdev, > new_rx_count == wx->rx_ring_count) > return 0; > > + while (test_and_set_bit(WX_STATE_RESETTING, wx->state)) { > + timeout--; > + if (!timeout) { > + err = -EBUSY; > + goto clear_reset; > + } > + usleep_range(1000, 2000); > + } > + This code appears twice. It may be factored out. [...] > @@ -89,7 +101,9 @@ static int ngbe_set_ringparam(struct net_device *netdev, > wx_configure(wx); > ngbe_up(wx); > > - return 0; > +clear_reset: > + clear_bit(WX_STATE_RESETTING, wx->state); > + return err; > } This function always clears the bit but it does not necessarily owns it.
On Tue, Apr 16, 2024 at 02:29:49PM +0800, Jiawen Wu wrote: > Add flag for reset state to avoid reading statistics when hardware > is reset. This explains the what, which you can also get by reading the code change. The commit message should also explain the why? What goes wrong if you read the statistics when the hardware is in reset? Do you get 42 for every statistic? Does the hardware lockup and the reset never completes? Andrew
On Tue, April 16, 2024 10:56 PM, Andrew Lunn wrote: > On Tue, Apr 16, 2024 at 02:29:49PM +0800, Jiawen Wu wrote: > > Add flag for reset state to avoid reading statistics when hardware > > is reset. > > This explains the what, which you can also get by reading the code > change. The commit message should also explain the why? What goes > wrong if you read the statistics when the hardware is in reset? Do you > get 42 for every statistic? Does the hardware lockup and the reset > never completes? I think I should discard this patch, and add the resetting flag to the patch 5/5 to avoid device reset collision. Since wx_update_stats() is called in txgbe_disable_device() while device is resetting. And I haven't found a situation that causes statistics confusion.
diff --git a/drivers/net/ethernet/wangxun/libwx/wx_hw.c b/drivers/net/ethernet/wangxun/libwx/wx_hw.c index 945c13d1a982..e4e6a14c4efc 100644 --- a/drivers/net/ethernet/wangxun/libwx/wx_hw.c +++ b/drivers/net/ethernet/wangxun/libwx/wx_hw.c @@ -2285,6 +2285,9 @@ void wx_update_stats(struct wx *wx) u64 restart_queue = 0, tx_busy = 0; u32 i; + if (test_bit(WX_STATE_RESETTING, wx->state)) + return; + /* gather some stats to the wx struct that are per queue */ for (i = 0; i < wx->num_rx_queues; i++) { struct wx_ring *rx_ring = wx->rx_ring[i]; diff --git a/drivers/net/ethernet/wangxun/libwx/wx_type.h b/drivers/net/ethernet/wangxun/libwx/wx_type.h index 1fdeb464d5f4..3726ec2fec06 100644 --- a/drivers/net/ethernet/wangxun/libwx/wx_type.h +++ b/drivers/net/ethernet/wangxun/libwx/wx_type.h @@ -982,8 +982,14 @@ struct wx_hw_stats { u64 qmprc; }; +enum wx_state { + WX_STATE_RESETTING, + WX_STATE_NBITS, /* must be last */ +}; + struct wx { unsigned long active_vlans[BITS_TO_LONGS(VLAN_N_VID)]; + DECLARE_BITMAP(state, WX_STATE_NBITS); void *priv; u8 __iomem *hw_addr; diff --git a/drivers/net/ethernet/wangxun/ngbe/ngbe_ethtool.c b/drivers/net/ethernet/wangxun/ngbe/ngbe_ethtool.c index 786a652ae64f..0e85c5a6633e 100644 --- a/drivers/net/ethernet/wangxun/ngbe/ngbe_ethtool.c +++ b/drivers/net/ethernet/wangxun/ngbe/ngbe_ethtool.c @@ -52,7 +52,8 @@ static int ngbe_set_ringparam(struct net_device *netdev, struct wx *wx = netdev_priv(netdev); u32 new_rx_count, new_tx_count; struct wx_ring *temp_ring; - int i; + u8 timeout = 50; + int i, err = 0; new_tx_count = clamp_t(u32, ring->tx_pending, WX_MIN_TXD, WX_MAX_TXD); new_tx_count = ALIGN(new_tx_count, WX_REQ_TX_DESCRIPTOR_MULTIPLE); @@ -64,6 +65,15 @@ static int ngbe_set_ringparam(struct net_device *netdev, new_rx_count == wx->rx_ring_count) return 0; + while (test_and_set_bit(WX_STATE_RESETTING, wx->state)) { + timeout--; + if (!timeout) { + err = -EBUSY; + goto clear_reset; + } + usleep_range(1000, 2000); + } + if (!netif_running(wx->netdev)) { for (i = 0; i < wx->num_tx_queues; i++) wx->tx_ring[i]->count = new_tx_count; @@ -72,14 +82,16 @@ static int ngbe_set_ringparam(struct net_device *netdev, wx->tx_ring_count = new_tx_count; wx->rx_ring_count = new_rx_count; - return 0; + goto clear_reset; } /* allocate temporary buffer to store rings in */ i = max_t(int, wx->num_tx_queues, wx->num_rx_queues); temp_ring = kvmalloc_array(i, sizeof(struct wx_ring), GFP_KERNEL); - if (!temp_ring) - return -ENOMEM; + if (!temp_ring) { + err = -ENOMEM; + goto clear_reset; + } ngbe_down(wx); @@ -89,7 +101,9 @@ static int ngbe_set_ringparam(struct net_device *netdev, wx_configure(wx); ngbe_up(wx); - return 0; +clear_reset: + clear_bit(WX_STATE_RESETTING, wx->state); + return err; } static int ngbe_set_channels(struct net_device *dev, diff --git a/drivers/net/ethernet/wangxun/txgbe/txgbe_ethtool.c b/drivers/net/ethernet/wangxun/txgbe/txgbe_ethtool.c index db675512ce4d..216599bbf9de 100644 --- a/drivers/net/ethernet/wangxun/txgbe/txgbe_ethtool.c +++ b/drivers/net/ethernet/wangxun/txgbe/txgbe_ethtool.c @@ -19,7 +19,8 @@ static int txgbe_set_ringparam(struct net_device *netdev, struct wx *wx = netdev_priv(netdev); u32 new_rx_count, new_tx_count; struct wx_ring *temp_ring; - int i; + u8 timeout = 50; + int i, err = 0; new_tx_count = clamp_t(u32, ring->tx_pending, WX_MIN_TXD, WX_MAX_TXD); new_tx_count = ALIGN(new_tx_count, WX_REQ_TX_DESCRIPTOR_MULTIPLE); @@ -31,6 +32,15 @@ static int txgbe_set_ringparam(struct net_device *netdev, new_rx_count == wx->rx_ring_count) return 0; + while (test_and_set_bit(WX_STATE_RESETTING, wx->state)) { + timeout--; + if (!timeout) { + err = -EBUSY; + goto clear_reset; + } + usleep_range(1000, 2000); + } + if (!netif_running(wx->netdev)) { for (i = 0; i < wx->num_tx_queues; i++) wx->tx_ring[i]->count = new_tx_count; @@ -39,14 +49,16 @@ static int txgbe_set_ringparam(struct net_device *netdev, wx->tx_ring_count = new_tx_count; wx->rx_ring_count = new_rx_count; - return 0; + goto clear_reset; } /* allocate temporary buffer to store rings in */ i = max_t(int, wx->num_tx_queues, wx->num_rx_queues); temp_ring = kvmalloc_array(i, sizeof(struct wx_ring), GFP_KERNEL); - if (!temp_ring) - return -ENOMEM; + if (!temp_ring) { + err = -ENOMEM; + goto clear_reset; + } txgbe_down(wx); @@ -55,7 +67,9 @@ static int txgbe_set_ringparam(struct net_device *netdev, txgbe_up(wx); - return 0; +clear_reset: + clear_bit(WX_STATE_RESETTING, wx->state); + return err; } static int txgbe_set_channels(struct net_device *dev,
Add flag for reset state to avoid reading statistics when hardware is reset. Fixes: 883b5984a5d2 ("net: wangxun: add ethtool_ops for ring parameters") Signed-off-by: Jiawen Wu <jiawenwu@trustnetic.com> --- drivers/net/ethernet/wangxun/libwx/wx_hw.c | 3 +++ drivers/net/ethernet/wangxun/libwx/wx_type.h | 6 +++++ .../net/ethernet/wangxun/ngbe/ngbe_ethtool.c | 24 +++++++++++++++---- .../ethernet/wangxun/txgbe/txgbe_ethtool.c | 24 +++++++++++++++---- 4 files changed, 47 insertions(+), 10 deletions(-)