From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932091AbdHWNvo (ORCPT ); Wed, 23 Aug 2017 09:51:44 -0400 Received: from mail.kernel.org ([198.145.29.99]:54046 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754001AbdHWNvm (ORCPT ); Wed, 23 Aug 2017 09:51:42 -0400 DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org E1A1520C48 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=kernel.org Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=helgaas@kernel.org Date: Wed, 23 Aug 2017 08:51:40 -0500 From: Bjorn Helgaas To: Oza Oza Cc: Bjorn Helgaas , Rob Herring , Mark Rutland , Ray Jui , Scott Branden , Jon Mason , BCM Kernel Feedback , Andy Gospodarek , linux-pci , linux-kernel@vger.kernel.org, Oza Pawandeep , Sinan Kaya , Timur Tabi , Alex Williamson Subject: Re: [PATCH v7 1/2] PCI: iproc: Retry request when CRS returned from EP Message-ID: <20170823135140.GA8498@bhelgaas-glaptop.roam.corp.google.com> References: <1503331124-25029-1-git-send-email-oza.oza@broadcom.com> <1503331124-25029-2-git-send-email-oza.oza@broadcom.com> <20170822203347.GE6948@bhelgaas-glaptop.roam.corp.google.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, Aug 23, 2017 at 03:57:02PM +0530, Oza Oza wrote: > On Wed, Aug 23, 2017 at 2:03 AM, Bjorn Helgaas wrote: > > [+cc Sinan, Timur, Alex] > > > > Hi Oza, > > > > On Mon, Aug 21, 2017 at 09:28:43PM +0530, Oza Pawandeep wrote: > >> PCIe spec r3.1, sec 2.3.2 > >> If CRS software visibility is not enabled, the RC must reissue the > >> config request as a new request. > >> > >> - If CRS software visibility is enabled, > >> - for a config read of Vendor ID, the RC must return 0x0001 data > >> - for all other config reads/writes, the RC must reissue the > >> request > > > > You mentioned early on that this fixes an issue with NVMe after a > > reset. Is that reset an FLR? I'm wondering if this overlaps with the > > work Sinan is doing: > > > > http://lkml.kernel.org/r/20170818212310.15145.21732.stgit@bhelgaas-glaptop.roam.corp.google.com > > > > With the current code in the tree, pci_flr_wait() probably waits only > > 100ms on your system. It currently reads PCI_COMMAND and probably > > sees 0xffff0001 because the NVMe device returns a CRS completion > > (which this iproc RC incorrectly turns into 0xffff0001 data), so we > > exit the loop after a single msleep(100). > > > > Sinan is extending pci_flr_wait() to read the Vendor ID and look for > > the CRS SV indication. If this is where you're seeing the NVMe issue, > > I bet we can fix this by simply changing iproc_pcie_config_read() to > > return the correct data when it sees a CRS completion. Then the > > retry loop in pci_flr_wait() will work as intended. > > > > The issue with User Space poll mode drivers resulting into CRS. > > > root@bcm958742k:~# /usr/share/spdk/examples/nvme/perf -r 'trtype:PCIe > traddr:0000:01:00.0' -q 128 -s 4096 -w read -d 2048 -t 30 -c 0x1 > Starting DPDK 16.11.1 initialization... > [ DPDK EAL parameters: perf -c 1 -m 2048 --file-prefix=spdk_pid2202 ] > EAL: Detected 8 lcore(s) > EAL: Probing VFIO support... > EAL: VFIO support initialized > EAL: cannot open /proc/self/numa_maps, consider that all memory is in > socket_id 0 > Initializing NVMe Controllers > EAL: PCI device 0000:01:00.0 on NUMA socket 0 > EAL: probe driver: 8086:953 spdk_nvme > EAL: using IOMMU type 1 (Type 1) > [ 45.811353] vfio_ecap_init: 0000:01:00.0 hiding ecap 0x19@0x2a0 > Attaching to NVMe Controller at 0000:01:00.0 [8086:0953] > [ 46.486601] vfio_bar_restore: 0000:01:00.0 reset recovery - restoring bars > nvme_ctrlr.c:1237:nvme_ctrlr_process_init: ***ERROR*** Initialization > timed out in state 3 > nvme_ctrlr.c: 414:nvme_ctrlr_shutdown: ***ERROR*** did not shutdown > within 5 seconds > EAL: Hotplug doesn't support vfio yet > spdk_nvme_probe() failed for transport address '0000:01:00.0' > /usr/share/spdk/examples/nvme/perf: errors occured > > ok, so this is in the context of > [ 52.500579] [] dump_backtrace+0x0/0x208 > [ 52.506147] [] show_stack+0x14/0x20 > [ 52.511357] [] dump_stack+0x8c/0xb0 > [ 52.516567] [] pci_flr_wait+0x18/0xc8 > [ 52.521955] [] pcie_flr+0x34/0x68 > [ 52.526986] [] __pci_dev_reset+0x100/0x2c0 > [ 52.532823] [] pci_try_reset_function+0x64/0x80 > [ 52.539108] [] vfio_pci_ioctl+0x398/0xb78 > [ 52.544857] [] vfio_device_fops_unl_ioctl+0x20/0x30 > [ 52.551501] [] do_vfs_ioctl+0xa4/0x738 > [ 52.556980] [] SyS_ioctl+0x8c/0xa0 > [ 52.562100] [] __sys_trace_return+0x0/0x4 > > > So, I have two things to do here. > > 1) remove retry funciton and just do following. > data = readl(cfg_data_p); > if (data == CFG_RETRY_STATUS) /* 0xffff0001 */ > data = 0xffffffff; > > 2) refactor the code with separate patch. > > but for that Sinan's patch is required. > then with both the patches I think, things will work out correctly for > iproc SOC. It looks like you are using the pci_flr_wait() path as I suspected. If you do the check as you mentioned in 1), this should work even without Sinan's patch. It's possible you'd have to increase the pci_flr_wait() timeout (currently 1sec total). I think we'll end up doing that as part of Sinan's work, so we use the same timeout whether or not the RC supports CRS SV. > > Let me know how this sounds and if you agree with above two points, I > will be posting final set of patches. > > let me know if you want me to change anything in the commit description. > in my opinion I should remove config write description which is irrelevant here. I agree. I don't think you need to say anything special about write, since I think an RC is probably allowed to drop (with no retries) a config write that receives a CRS completion. In this case, I expect the RC would log an error, so you should see something in PCI_STATUS.