Re: [PATCH V1] Revert "PCI: tegra194: Enable support for 256 Byte payload"

From: Vidya Sagar <vidyas@nvidia.com>
To: Bjorn Helgaas <helgaas@kernel.org>
Cc: lpieralisi@kernel.org, kw@linux.com, robh@kernel.org,
	bhelgaas@google.com, thierry.reding@gmail.com,
	jonathanh@nvidia.com, Sergey.Semin@baikalelectronics.ru,
	linux-pci@vger.kernel.org, linux-tegra@vger.kernel.org,
	linux-kernel@vger.kernel.org, kthota@nvidia.com,
	mmaddireddy@nvidia.com, sagar.tv@gmail.com
Subject: Re: [PATCH V1] Revert "PCI: tegra194: Enable support for 256 Byte payload"
Date: Fri, 9 Jun 2023 07:53:10 +0530	[thread overview]
Message-ID: <5bf550a9-146e-ad51-bbeb-c55b48478f62@nvidia.com> (raw)
In-Reply-To: <20230608163346.GA1204586@bhelgaas>

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.

-Vidya Sagar

> 
> Bjorn