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

[Jon Hunter <jonathanh@nvidia.com>]

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