On Wed, Oct 06, 2021 at 05:29:34PM +0100, Lorenzo Pieralisi wrote:
> On Tue, Oct 05, 2021 at 02:28:26PM -0500, Bjorn Helgaas wrote:
> > On Mon, Oct 04, 2021 at 12:19:38PM +0200, Marek Beh�n wrote:
> > > On Mon, 4 Oct 2021 09:53:35 +0100
> > > Lorenzo Pieralisi <lorenzo.pieralisi@xxxxxxx> wrote:
> > > > On Mon, Oct 04, 2021 at 09:21:48AM +0200, Marek Beh�n wrote:
> > > > > On Sat, 2 Oct 2021 11:35:19 -0500
> > > > > Bjorn Helgaas <helgaas@xxxxxxxxxx> wrote:
> > > > >
> > > > > > On Fri, Oct 01, 2021 at 09:58:52PM +0200, Marek Beh�n wrote:
> > > > > > > From: Pali Roh�r <pali@xxxxxxxxxx>
> > > > > > >
> > > > > > > Commit 43f5c77bcbd2 ("PCI: aardvark: Fix reporting CRS value") fixed
> > > > > > > handling of CRS response and when CRSSVE flag was not enabled it marked CRS
> > > > > > > response as failed transaction (due to simplicity).
> > > > > > >
> > > > > > > But pci-aardvark.c driver is already waiting up to the PIO_RETRY_CNT count
> > > > > > > for PIO config response and so we can with a small change implement
> > > > > > > re-issuing of config requests as described in PCIe base specification.
> > > > > > >
> > > > > > > This change implements re-issuing of config requests when response is CRS.
> > > > > > > Set upper bound of wait cycles to around PIO_RETRY_CNT, afterwards the
> > > > > > > transaction is marked as failed and an all-ones value is returned as
> > > > > > > before.
> > > > > >
> > > > > > Does this fix a problem?
> > > > >
> > > > > Hello Bjorn,
> > > > >
> > > > > Pali has suspisions that certain Marvell WiFi cards may need this [1]
> > > > > to work, but we do not have them so we cannot test this.
> > > > >
> > > > > I guess you think this should be considered a new feature instead of a
> > > > > fix, so that the Fixes tag should be removed, yes? Pali was of the
> > > > > opinion that this patch "fixes" the driver to conform more to the PCIe
> > > > > specification, that is why we added the Fixes tag.
> > > > >
> > > > > Anyway if you think this should be considered a new feature, this patch
> > > > > can be skipped. The following patches apply even without it.
> > > >
> > > > I do not think we should apply to the mainline a fix that can't be
> > > > tested sorry, I will skip this patch.
> > >
> > > Lorenzo,
> > >
> > > my explanation was incorrect.
> > >
> > > The functionality added by this patch _is_ tested and correctly does a
> > > retry: it was done by simulating a CRS reply.
> > >
> > > We just don't know whether there are real cards used by users that
> > > need this functionality (the mentioned Marvell card may be such a card).
> >
> > My claim is that the spec allows root complexes that retry zero times,
> > so we must assume such a root complex exists and we cannot rely on any
> > retries. If such a root complex exists, this patch might fix a
> > problem, but only for aardvark. It would be better to fix the problem
> > in a way that works for all PCIe controllers.
>
> We need a way for those PCI controllers to communicate to SW that
> they actually received a CRS completion (and that they don't retry
> in HW).
AFAICT this would be controller-dependent. I think some controllers
have registers that control the number of retries, but of course
that's completely controller-dependent, too.
> By implementing the logic in the aardvark controller that platform
> information is there so to the best of my knowledge this patch
> is sound.
>
> I assume that the HW retry is in the specs because there is no
> architected way if CRS Software Visibility is not enabled/present to
> report CRS completion in an architected PCI manner but I just
> don't know the entire background behind this.
I assume an error bit would be set in the Status or Secondary Status
register when a PCIe transaction fails. I'm not sure anybody *looks*
at those, though.
This still looks like a PCI controller band-aid for a device or driver
problem that will likely still exist on other platforms.
Bjorn
