On Wed, Aug 23, 2017 at 7:21 PM, Bjorn Helgaas <helg...@kernel.org> wrote:
> On Wed, Aug 23, 2017 at 03:57:02PM +0530, Oza Oza wrote:
>> On Wed, Aug 23, 2017 at 2:03 AM, Bjorn Helgaas <helg...@kernel.org> 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.st...@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] [<ffff000008088498>] dump_backtrace+0x0/0x208
>> [ 52.506147] [<ffff0000080886b4>] show_stack+0x14/0x20
>> [ 52.511357] [<ffff0000083ace34>] dump_stack+0x8c/0xb0
>> [ 52.516567] [<ffff00000840a550>] pci_flr_wait+0x18/0xc8
>> [ 52.521955] [<ffff00000840bf9c>] pcie_flr+0x34/0x68
>> [ 52.526986] [<ffff00000840e3d0>] __pci_dev_reset+0x100/0x2c0
>> [ 52.532823] [<ffff00000840e60c>] pci_try_reset_function+0x64/0x80
>> [ 52.539108] [<ffff00000856e4b8>] vfio_pci_ioctl+0x398/0xb78
>> [ 52.544857] [<ffff000008568378>] vfio_device_fops_unl_ioctl+0x20/0x30
>> [ 52.551501] [<ffff0000081d4e24>] do_vfs_ioctl+0xa4/0x738
>> [ 52.556980] [<ffff0000081d5544>] SyS_ioctl+0x8c/0xa0
>> [ 52.562100] [<ffff000008082f8c>] __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.
looks like this can not work.
If I add folowing in iproc_pcie_config_read()
if (data == CFG_RETRY_STATUS) /* 0xffff0001 */
data = 0xffffffff;
because pci_bus_read_dev_vendor_id returns false
/* some broken boards return 0 or ~0 if a slot is empty: */
if (*l == 0xffffffff || *l == 0x00000000 ||
*l == 0x0000ffff || *l == 0xffff0000)
return false;
In working Enumuration case I get following:
[ 9.125976] pci 0000:00:00.0: bridge configuration invalid ([bus
00-00]), re-configuring
[ 9.134267] where=0x0 val=0xffff0001
[ 9.146946] where=0x0 val=0xffff0001
[ 9.158943] where=0x0 val=0xffff0001
[ 9.170945] where=0x0 val=0xffff0001
[ 9.186945] where=0x0 val=0xffff0001
[ 9.210944] where=0x0 val=0xffff0001
[ 9.250943] where=0x0 val=0xffff0001
[ 9.322942] where=0x0 val=0xffff0001
[ 9.458943] where=0x0 val=0xffff0001
[ 9.726942] where=0x0 val=0x9538086 >> actual vendor and device id.
so I think I have to retry in RC driver, so the old code still holds good.
except that I have to do factoring out.
please let me know If I missed anything, or you want me to try anything else.
Regards,
Oza.
>
> 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.
