Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933499AbcKINZ1 (ORCPT ); Wed, 9 Nov 2016 08:25:27 -0500 Received: from cn.fujitsu.com ([59.151.112.132]:6299 "EHLO heian.cn.fujitsu.com" rhost-flags-OK-FAIL-OK-FAIL) by vger.kernel.org with ESMTP id S932676AbcKINZW (ORCPT ); Wed, 9 Nov 2016 08:25:22 -0500 X-IronPort-AV: E=Sophos;i="5.22,518,1449504000"; d="scan'208";a="12818395" Subject: Re: [Intel-wired-lan] [PATCH] igb: use igb_adapter->io_addr instead of e1000_hw->hw_addr To: Alexander Duyck , Hisashi T Fujinaka , Netdev , intel-wired-lan , "linux-kernel@vger.kernel.org" , =?UTF-8?B?SXp1bWksIFRha3Uv5rOJIOaLkw==?= , References: <1478588780-24480-1-git-send-email-caoj.fnst@cn.fujitsu.com> <20161108164214.GF31855@calimero.vinschen.de> <20161108183739.GA3744@calimero.vinschen.de> From: Cao jin Message-ID: <58232463.7030107@cn.fujitsu.com> Date: Wed, 9 Nov 2016 21:28:03 +0800 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.1.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 7bit X-Originating-IP: [10.167.226.69] X-yoursite-MailScanner-ID: C932B47A8660.A9656 X-yoursite-MailScanner: Found to be clean X-yoursite-MailScanner-From: caoj.fnst@cn.fujitsu.com Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4126 Lines: 98 Thanks Corrina for your info. I tested my patch, it works for me on kernel 4.9-rc4. "surprise removal" maybe another issue to solve. This one is enough to solve my issue and other one's, could it be accept first? Cao jin On 11/09/2016 03:33 AM, Alexander Duyck wrote: > On Tue, Nov 8, 2016 at 10:37 AM, Corinna Vinschen wrote: >> On Nov 8 09:16, Hisashi T Fujinaka wrote: >>> On Tue, 8 Nov 2016, Corinna Vinschen wrote: >>>> On Nov 8 15:06, Cao jin wrote: >>>>> When running as guest, under certain condition, it will oops as following. >>>>> writel() in igb_configure_tx_ring() results in oops, because hw->hw_addr >>>>> is NULL. While other register access won't oops kernel because they use >>>>> wr32/rd32 which have a defense against NULL pointer. >>>>> [...] >>>> >>>> Incidentally we're just looking for a solution to that problem too. >>>> Do three patches to fix the same problem at rougly the same time already >>>> qualify as freak accident? >>>> >>>> FTR, I attached my current patch, which I was planning to submit after >>>> some external testing. >>>> >>>> However, all three patches have one thing in common: They workaround >>>> a somewhat dubious resetting of the hardware address to NULL in case >>>> reading from a register failed. >>>> >>>> That makes me wonder if setting the hardware address to NULL in >>>> rd32/igb_rd32 is really such a good idea. It's performed in a function >>>> which return value is *never* tested for validity in the calling >>>> functions and leads to subsequent crashes since no tests for hw_addr == >>>> NULL are performed. >>>> >>>> Maybe commit 22a8b2915 should be reconsidered? Isn't there some more >>>> graceful way to handle the "surprise removal"? >>> >>> Answering this from my home account because, well, work is Outlook. >>> >>> "Reconsidering" would be great. In fact, revert if if you'd like. I'm >>> uncertain that the surprise removal code actually works the way I >>> thought previously and I think I took a lot of it out of my local code. >>> >>> Unfortuantely I don't have any equipment that I can use to reproduce >>> surprise removal any longer so that means I wouldn't be able to test >>> anything. I have to defer to you or Cao Jin. >> >> I'm not too keen to rip out a PCIe NIC under power from my locale >> desktop machine, but I think an actual surprise removal is not the >> problem. >> >> As described in my git log entry, the error condition in igb_rd32 can be >> triggered during a suspend. The HW has been put into a sleep state but >> some register read requests are apparently not guarded against that >> situation. Reading a register in this state returns -1, thus a suspend >> is erroneously triggering the "surprise removal" sequence. > > The question I would have is what is reading the device when it is in > this state. The watchdog and any other functions that would read the > device should be disabled. > > One possibility could be a race between a call to igb_close and the > igb_suspend function. We have seen some of those pop up recently on > ixgbe and it looks like igb has the same bug. We should probably be > using the rtnl_lock to guarantee that netif_device_detach and the call > to __igb_close are completed before igb_close could possibly be called > by the network stack. > >> Here's a raw idea: >> >> - Note that device is suspended in e1000_hw struct. Don't trigger >> error sequence in igb_rd32 if so (...and return a 0 value???) > > The thing is that a suspended device should not be accessed at all. > If we are accessing it while it is suspended then that is a bug. If > you could throw a WARN_ON call in igb_rd32 to capture where this is > being triggered that might be useful. > >> - Otherwise assume it's actually a surprise removal. In theory that >> should somehow trigger a device removal sequence, kind of like >> calling igb_remove, no? > > Well a read of the MMIO region while suspended is more of a surprise > read since there shouldn't be anything going on. We need to isolate > where that read is coming from and fix it. > > Thanks. > > - Alex > > > . >