Re: ECRC and Max Read Request Size

[Bjorn Helgaas <helgaas@kernel.org>]

On Fri, Nov 06, 2015 at 12:54:07PM -0500, Sinan Kaya wrote:
> On 11/6/2015 12:22 PM, Bjorn Helgaas wrote:
> >On Wed, Oct 28, 2015 at 01:51:48PM -0400, Sinan Kaya wrote:
> >>I'm seeing two problems with the current PCI framework when it comes
> >>to end-to-end CRC (ECRC) and Max Read Request Size.
> >>
> >>The problem with ECRC is that it blindly enables ECRC generation on
> >>devices without checking if it is supported by the entire bus with
> >>ECRC=on option.
> >>
> >>ECRC check can be enabled on all devices but ECRC generation on the
> >>root complex and switches needs to be set only if all devices
> >>support ECRC checking.
> >>
> >>I'm thinking of making changes in this area to cover this gap with
> >>ECRC=safe option.
> >
> >Sometimes they can't be avoided, but I'm generally not in favor of
> >adding command line parameters because they require too much of our
> >users and they introduce code paths that are rarely exercised.
> >
> >What specific ECRC problem are you seeing?  Do you have devices that
> >don't operate correctly with the current code?  Or do you want to add
> >some new functionality, e.g., to enable ECRC in cases where we don't
> >enable it today?
> 
> ECRC is an optional PCIe feature. Even ECRC support has some flavors
> 
> - A card can support ECRC checking.
> - A card can support ECRC checking and generation.
> 
> Right now, the code is enabling both without checking if they are
> supported at all.
> 
> I have some legacy PCIe cards that don't support ECRC completely
> even though the host bridge supports it. If ECRC checking and
> generation is enabled under this situation, I have problems
> communicating to the endpoint.

That should definitely be fixed.  Do we enable ECRC unconditionally,
or only when we boot with "pci=ecrc=on"?  The doc
(Documentation/kernel-parameters.txt) suggests the latter.

It would be ideal if we could try to turn on ECRC all the time by
default for paths that support it.  I suppose there's some risk that
we'd trip over broken hardware.  If that's an issue, we could consider
turning it on all the time on machines newer than X (that's easy on
x86, where we have a DMI BIOS date, but maybe not so easy on other
arches).

> Another opinion is let the firmware do its magic before the OS
> starts but there is no way to apply the same settings after a
> hot-plug insertion.

The ACPI _HPX method does allow the BIOS to specify Device Control
register values, so it's conceivable that MPS and MRRS could be
configured that way for hot-added devices.  But we currently
explicitly ignore those _HPX fields (see 302328c00341) because I don't
think it's safe to use them blindly, at least for MPS.  If the Linux
MPS configuration stopped making assumptions about MRRS settings, I
think it probably would be safe to use _HPX MRRS values.

Bjorn