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

From: Bjorn Helgaas <helgaas@kernel.org>
To: Vidya Sagar <vidyas@nvidia.com>
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 V3] Revert "PCI: tegra194: Enable support for 256 Byte payload"
Date: Wed, 19 Jul 2023 09:57:22 -0500	[thread overview]
Message-ID: <20230719145722.GA501156@bhelgaas> (raw)
In-Reply-To: <43a9ab32-c030-1906-7e7d-c529e6c9c70d@nvidia.com>

On Wed, Jul 19, 2023 at 04:31:13PM +0530, Vidya Sagar wrote:
> On 7/18/2023 4:39 PM, Bjorn Helgaas wrote:
> > On Tue, Jul 18, 2023 at 08:03:47AM +0530, Vidya Sagar wrote:
> > > On 7/14/2023 3:09 AM, Bjorn Helgaas wrote:
> > > > On Mon, Jun 19, 2023 at 03:56:04PM +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 addressed 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.
> > > > 
> > > > I think this has some irrelevant detail (IIUC the problem should
> > > > happen even without a switch) and could be more specific (I think the
> > > > problem case is RP MPS=256, EP only supports MPS=128).
> > > 
> > > The issue is present only if there is a switch.
> > 
> > So if there's no switch, and an EP that only supports MPS=128, the PCI
> > core changes the RP MPS setting to 128?  Just based on reading the
>
> Yes. The code after the if condition here takes care of that.
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pci/probe.c?h=v6.5-rc2#n2049

Oh, right, thanks.  I vaguely remember the logic that if the immediate
parent is a Root Port, there are no other branches in the hierarchy to
worry about, so we can just configure the Root Port MPS to match the
device.

> > code, I thought we would leave RP MPS=256 and EP MPS=128, which would
> > be a problem.  But maybe the PCI core changes the RP down to MPS=128?
> > 
> > Bjorn