Re: [v2 PATCH 1/1] tg3: fix meaningless hw_stats reading after tg3_halt memset 0 hw_stats

[Zumeng Chen]

On 05/03/2018 01:04 PM, Michael Chan wrote:
On Wed, May 2, 2018 at 5:30 PM, Zumeng Chen <zumeng.c...@gmail.com> wrote:
On 2018年05月03日 01:32, Michael Chan wrote:
On Wed, May 2, 2018 at 3:27 AM, Zumeng Chen <zumeng.c...@gmail.com> wrote:
On 2018年05月02日 13:12, Michael Chan wrote:
On Tue, May 1, 2018 at 5:42 PM, Zumeng Chen <zumeng.c...@gmail.com>
wrote:
diff --git a/drivers/net/ethernet/broadcom/tg3.h
b/drivers/net/ethernet/broadcom/tg3.h
index 3b5e98e..c61d83c 100644
--- a/drivers/net/ethernet/broadcom/tg3.h
+++ b/drivers/net/ethernet/broadcom/tg3.h
@@ -3102,6 +3102,7 @@ enum TG3_FLAGS {
           TG3_FLAG_ROBOSWITCH,
           TG3_FLAG_ONE_DMA_AT_ONCE,
           TG3_FLAG_RGMII_MODE,
+       TG3_FLAG_HALT,
I think you should be able to use the existing INIT_COMPLETE flag

No,  it will bring the uncertain factors into the existed complicate
logic
of INIT_COMPLETE.
And I think it's very simple logic here to fix the meaningless hw_stats
reading and the problem
of commit f5992b72. I even suspect if you have read INIT_COMPLETE related
codes carefully.

We should use an existing flag whenever appropriate

I disagree. This is sort of blahblah...
I don't want to see another flag added that is practically the same as
!INIT_COMPLETE.  The driver already has close to one hundred flags.
Adding a new flag that is similar to an existing flag will just make
the code more difficult to understand and maintain.

If you don't want to fix it the cleaner way, Siva or I will fix it.

Feel free to go, I just take a double look, INIT_COMPLETE can directly 
be used as follows:

diff --git a/drivers/net/ethernet/broadcom/tg3.c 
b/drivers/net/ethernet/broadcom/tg3.c
index 08bbb63..0e04fd7 100644
--- a/drivers/net/ethernet/broadcom/tg3.c
+++ b/drivers/net/ethernet/broadcom/tg3.c
@@ -8733,14 +8733,11 @@ static void tg3_free_consistent(struct tg3 *tp)
        tg3_mem_rx_release(tp);
        tg3_mem_tx_release(tp);

-       /* Protect tg3_get_stats64() from reading freed tp->hw_stats. */
-       tg3_full_lock(tp, 0);
        if (tp->hw_stats) {
                dma_free_coherent(&tp->pdev->dev, sizeof(struct 
tg3_hw_stats),
                                  tp->hw_stats, tp->stats_mapping);
                tp->hw_stats = NULL;
        }
-       tg3_full_unlock(tp);
 }

 /*
@@ -14178,7 +14175,7 @@ static void tg3_get_stats64(struct net_device *dev,
        struct tg3 *tp = netdev_priv(dev);

        spin_lock_bh(&tp->lock);
-       if (!tp->hw_stats) {
+       if (!tp->hw_stats || tg3_flag(tp, INIT_COMPLETE)) {
                *stats = tp->net_stats_prev;
                spin_unlock_bh(&tp->lock);
                return;

Cheers,
Zumeng