Re: [v2 PATCH 1/1] tg3: fix meaningless hw_stats reading after tg3_halt memset 0 hw_stats
On 05/03/2018 01:04 PM, Michael Chan wrote: On Wed, May 2, 2018 at 5:30 PM, Zumeng Chen wrote: On 2018年05月03日 01:32, Michael Chan wrote: On Wed, May 2, 2018 at 3:27 AM, Zumeng Chen wrote: On 2018年05月02日 13:12, Michael Chan wrote: On Tue, May 1, 2018 at 5:42 PM, Zumeng Chen 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
Re: [v2 PATCH 1/1] tg3: fix meaningless hw_stats reading after tg3_halt memset 0 hw_stats
On Wed, May 2, 2018 at 5:30 PM, Zumeng Chen wrote: > On 2018年05月03日 01:32, Michael Chan wrote: >> >> On Wed, May 2, 2018 at 3:27 AM, Zumeng Chen wrote: >>> >>> On 2018年05月02日 13:12, Michael Chan wrote: On Tue, May 1, 2018 at 5:42 PM, Zumeng Chen 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.
Re: [v2 PATCH 1/1] tg3: fix meaningless hw_stats reading after tg3_halt memset 0 hw_stats
On 2018年05月03日 01:32, Michael Chan wrote: On Wed, May 2, 2018 at 3:27 AM, Zumeng Chen wrote: On 2018年05月02日 13:12, Michael Chan wrote: On Tue, May 1, 2018 at 5:42 PM, Zumeng Chen 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... , instead of adding yet another flag to do similar things. I've looked at the code briefly and believe that INIT_COMPLETE will work. When we fix a problem, we'd better think if we introduce a new one. If you think it won't work, please be specific and point out why it won't work. Thanks. I don't care if it work or not, I directly feel it's a bad idea. INIT_COMPLETE include a lot of network stuffs, it's not simple hardware reset related. Here again, My fix logic is very simple to fix the problem I met, I think this is how Linux works together with such a lot of thing, which means clear, simple, and robust for every unit, we re-unite them eco-systematically. Finally, it's yours, so be it. Cheers, Zumeng
Re: [v2 PATCH 1/1] tg3: fix meaningless hw_stats reading after tg3_halt memset 0 hw_stats
On Wed, May 2, 2018 at 3:27 AM, Zumeng Chen wrote: > On 2018年05月02日 13:12, Michael Chan wrote: >> >> On Tue, May 1, 2018 at 5:42 PM, Zumeng Chen 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, instead of adding yet another flag to do similar things. I've looked at the code briefly and believe that INIT_COMPLETE will work. If you think it won't work, please be specific and point out why it won't work. Thanks.
Re: [v2 PATCH 1/1] tg3: fix meaningless hw_stats reading after tg3_halt memset 0 hw_stats
On 2018年05月02日 13:12, Michael Chan wrote: On Tue, May 1, 2018 at 5:42 PM, Zumeng Chen 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. Cheers, Zumeng and not have to add a new flag. /* Add new flags before this comment and TG3_FLAG_NUMBER_OF_FLAGS */ TG3_FLAG_NUMBER_OF_FLAGS, /* Last entry in enum TG3_FLAGS */ -- 2.9.3
Re: [v2 PATCH 1/1] tg3: fix meaningless hw_stats reading after tg3_halt memset 0 hw_stats
On Tue, May 1, 2018 at 5:42 PM, Zumeng Chen 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 and not have to add a new flag. > > /* Add new flags before this comment and TG3_FLAG_NUMBER_OF_FLAGS */ > TG3_FLAG_NUMBER_OF_FLAGS, /* Last entry in enum TG3_FLAGS */ > -- > 2.9.3 >
[v2 PATCH 1/1] tg3: fix meaningless hw_stats reading after tg3_halt memset 0 hw_stats
From: Zumeng Chen Reading hw_stats will get the actual data after a sucessfull tg3_reset_hw, which actually after tg3_timer_start, so TG3_FLAG_HALT is introduced to tell tg3_get_stats64 when hw_stats is ready to read. It will be set after having done memset(tp->hw_stats, 0) in tg3_halt and be clear when hw_init done. Both tg3_get_stats64 and tg3_halt are protected by tp->lock in all scope. Meanwhile, this patch is also to fix a kernel BUG_ON(in_interrupt) crash when tg3_free_consistent is stuck in tp->lock, which might get a lot of in_softirq counts(512 or so), and BUG_ON when vunmap to unmap hw->stats. [ cut here ] kernel BUG at /kernel-source//mm/vmalloc.c:1621! Internal error: Oops - BUG: 0 [#1] PREEMPT SMP task: ffc87431 task.stack: ffc8742bc000 PC is at vunmap+0x48/0x50 LR is at __dma_free+0x98/0xa0 pc : [] lr : [] pstate: 0145 sp : ffc8742bfad0 x29: ffc8742bfad0 x28: ffc87431 x27: ffc878931200 x26: ffc87453 x25: 0003 x24: ff800b3aa000 x23: 700bb000 x22: x21: x20: ffc87aafd0a0 x19: ff800b3aa000 x18: 0020 x17: 007f9e191e10 x16: ff8008eb0d28 x15: 000a x14: 00070cc8 x13: ff8008c65000 x12: x11: 000a x10: ffbf21d0e220 x9 : 0004 x8 : ff8008c65000 x7 : 3ff0 x6 : x5 : ff8008097f20 x4 : x3 : ff8008fd4fff x2 : ffc87b361788 x1 : ff800b3aafff x0 : 0201 Process connmand (pid: 785, stack limit = 0xffc8742bc000) Stack: (0xffc8742bfad0 to 0xffc8742c) fac0: ffc8742bfaf0 ff8008097fb8 fae0: 1000 ff80 ffc8742bfb30 ff8000b717d4 fb00: ffc87aafd0a0 ff8008a38000 ff800b3aa000 ffc874530904 fb20: ffc874530900 700bb000 ffc8742bfb80 ff8000b80324 fb40: 0001 ffc874530900 0100 0200 fb60: 9003 ffc87453 0003 ffc87453 fb80: ffc8742bfbd0 ff8000b8aa5c ffc874530900 ffc87453 fba0: 0001 9003 ffc878931210 fbc0: 9002 ffc87453 ffc8742bfc00 ff80088bf44c fbe0: ffc87453 ffc8742bfc50 0001 ffc87431 fc00: ffc8742bfc30 ff80088bf5e4 ffc87453 9002 fc20: ffc8742bfc40 ffc87453 ffc8742bfc60 ff80088c9d58 fc40: ffc87453 ff80088c9d34 ffc874530080 ffc874530080 fc60: ffc8742bfca0 ff80088c9e4c ffc87453 9003 fc80: 8914 007ffd94ba10 ffc8742bfd38 fca0: ffc8742bfcd0 ff80089509f8 ff9d fcc0: 8914 ffc8742bfd60 ff8008953088 fce0: 8914 ffc874b49b80 007ffd94ba10 ff8008e9b400 fd00: 0004 007ffd94ba10 0124 001d fd20: ff8008a32000 ff8008e9b400 0004 34687465 fd40: 9002 fd60: ffc8742bfd90 ff80088a1720 ffc874b49b80 8914 fd80: 007ffd94ba10 ffc8742bfdc0 ff80088a2648 fda0: 8914 007ffd94ba10 ff8008e9b400 ffc878a73c00 fdc0: ffc8742bfe00 ff800822e9e0 8914 007ffd94ba10 fde0: ffc874b49bb0 ffc8747e5800 ffc8742bfe50 ff800823cd58 fe00: ffc8742bfe80 ff800822f0ec ffc878a73c00 fe20: ffc878a73c00 0004 8914 0008 fe40: ffc8742bfe80 ff800822f0b0 ffc878a73c00 fe60: ffc878a73c00 0004 8914 ff8008083730 fe80: ff8008083730 0048771fb000 fea0: 007f9e191e1c 0015 fec0: 0004 8914 007ffd94ba10 fee0: 002f 0004 0010 ff00: 001d 000f 0101010101010101 ff20: 6532336338646634 00656c6261635f38 007f9e46a220 007f9e45f318 ff40: 004c1a58 007f9e191e10 06df ff60: 0004 004c6470 004c3c40 00512d20 ff80: 0001 ffa0: 007ffd94b9f0 00463dec 007ffd94b9f0 ffc0: 007f9e191e1c 0004 001d ffe0: Call trace: Exception stack(0xffc8742bf900 to 0xffc8742bfa30) f900: ff800b3aa000 0080 ffc8742bfad0 ff80081eb420 f920: ff80 ff80081a58ec ffc8742bf940 ff8