Messages in this thread | | | Date | Wed, 14 Jun 2023 11:39:20 +0100 | Subject | Re: [PATCH V1] Revert "PCI: tegra194: Enable support for 256 Byte payload" | From | Jon Hunter <> |
| |
Hi Bjorn,
On 09/06/2023 03:23, Vidya Sagar wrote: > > > On 6/8/2023 10:03 PM, Bjorn Helgaas wrote: >> External email: Use caution opening links or attachments >> >> >> On Thu, Jun 08, 2023 at 03:06:52PM +0530, Vidya Sagar wrote: >>> This reverts commit 4fb8e46c1bc4 ("PCI: tegra194: Enable >>> support for 256 Byte payload") >>> >>> Consider a PCIe hierarchy with a PCIe switch and a device connected >>> downstream of the switch that has support for MPS which is the minimum >>> in the hierarchy, and root port programmed with an MPS in its DevCtl >>> register that is greater than the minimum. In this scenario, the default >>> bus configuration of the kernel i.e. "PCIE_BUS_DEFAULT" doesn't >>> configure the MPS settings in the hierarchy correctly resulting in the >>> device with support for minimum MPS in the hierarchy receiving the TLPs >>> of size more than that. Although this can be addresed by appending >>> "pci=pcie_bus_safe" to the kernel command line, it doesn't seem to be a >>> good idea to always have this commandline argument even for the basic >>> functionality to work. >>> Reverting commit 4fb8e46c1bc4 ("PCI: tegra194: Enable support for 256 >>> Byte payload") avoids this requirement and ensures that the basic >>> functionality of the devices irrespective of the hierarchy and the >>> MPS of >>> the devices in the hierarchy. >>> To reap the benefits of having support for higher MPS, optionally, >>> one can >>> always append the kernel command line with "pci=pcie_bus_perf". >> >> Please add blank lines between paragraphs and wrap to fill 75 columns. >> Also add a period at the end of the very first sentence. >> >> s/addresed/addressed/ >> > I'll address your comments in the next patch. > >> I guess that without 4fb8e46c1bc4, Linux configured everything with >> 128 byte MPS, and 4fb8e46c1bc4 was intended as an optimization to >> allow 256 byte MPS. > Correct. > >> >> If the Root Port advertises Max_Payload_Size Supported as 256 bytes in >> DevCap, and the PCI core doesn't configure MPS=256 when possible, I'd >> argue that should be fixed in the PCI core without a driver change >> like this. > Well, kernel does configure MPS=256 but only if the 'perf' configuration > option is selected. 'perf' configuration option also changes the MRRS, > to extract the maximum performance. I'm not sure about the reasons for > not making 'perf' configuration as the default configuration though. > (IIUC, this is what you are coming to, right?) > > The current patch which is a revert of an earlier patch is to keep > things working for different PCIe hierarchies given the default > configuration that kernel is using at the moment.
Any more feedback on this? If not, would be great to queue for v6.5. Feel free to add my ...
Acked-by: Jon Hunter <jonathanh@nvidia.com>
Cheers Jon
-- nvpublic
| |