Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Fri, 18 Sep 2020 07:47:10 -0500
From:   Bjorn Helgaas <helgaas@kernel.org>
To:     "Z.q. Hou" <zhiqiang.hou@nxp.com>
Cc:     Rob Herring <robh@kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        PCI <linux-pci@vger.kernel.org>,
        Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>,
        Bjorn Helgaas <bhelgaas@google.com>,
        Gustavo Pimentel <gustavo.pimentel@synopsys.com>,
        Michael Walle <michael@walle.cc>,
        Ard Biesheuvel <ardb@kernel.org>
Subject: Re: [PATCH] PCI: dwc: Added link up check in map_bus of
 dw_child_pcie_ops
Message-ID: <20200918124710.GA1800067@bjorn-Precision-5520>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <HE1PR0402MB3371F8191538F47E8249F048843F0@HE1PR0402MB3371.eurprd04.prod.outlook.com>
Precedence: bulk

On Fri, Sep 18, 2020 at 11:02:07AM +0000, Z.q. Hou wrote:

> But now the SError is exactly caused by the first access of the
> non-existent function, I dug into the kernel enumeration code and
> found it will fire a 4Byte CFG read transaction to read the Vendor
> ID and Device ID together, so I suspect the root cause is access the
> Device ID of a non-existent function triggers SError.
> 
> So the alternative solution seems to correct the PCIe enumeration, I
> will submit a patch to let the first access only read the Vendor ID.

If it is incorrect for the first access to be a 32-bit read of both
the Vendor and the Device ID, please cite the relevant section of the
spec in your patch.

I don't like to make changes to generic code to accommodate specific
pieces of hardware because then we restrict future changes based on
some device that will soon be obsolete and forgotten.

I'm pretty sure the spec language about CRS handling is careful to
talk about "reads that *include* Vendor ID", not just "reads of Vendor
ID", so the implication is that it covers 32-bit reads as well as
16-bit reads.

Bjorn