Received-SPF: pass (google.com: domain of linux-kernel+bounces-128315-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Tue, 2 Apr 2024 12:05:57 -0400
From: Frank Li <Frank.li@nxp.com>
To: Niklas Cassel <cassel@kernel.org>
Cc: Manivannan Sadhasivam <mani@kernel.org>,
	Bjorn Helgaas <helgaas@kernel.org>, niklas.cassel@wdc.com,
	bhelgaas@google.com, gustavo.pimentel@synopsys.com,
	imx@lists.linux.dev, jdmason@kudzu.us, jingoohan1@gmail.com,
	kw@linux.com, linux-kernel@vger.kernel.org,
	linux-pci@vger.kernel.org, lpieralisi@kernel.org, robh@kernel.org
Subject: Re: [PATCH v2 1/1] PCI: dwc: Fix index 0 incorrectly being
 interpreted as a free ATU slot
Message-ID: <Zgws5Z850iM4XtdD@lizhi-Precision-Tower-5810>
References: <20240321171345.GA2385@thinkpad>
 <20240321180732.GA1329092@bhelgaas>
 <20240322052623.GA4092@thinkpad>
 <Zf0i1X5fg-E59NWx@ryzen>
 <20240402051228.GH2933@thinkpad>
 <ZgvOpTcNKPantiHg@ryzen>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <ZgvOpTcNKPantiHg@ryzen>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0:
	=?us-ascii?Q?yN1R9YVYHGeEw6NH4y6UvCICXvOJ2pWl8Il3wv49PXVheAi4AG2GP1mMenCN?=
 =?us-ascii?Q?1uEU1dkqqad11kTaRbfb+f5Cj65WdHwjVSsh4Tw9rRPrleW3MlxygQTeqxn+?=
 =?us-ascii?Q?ZEPSyQbIzhePO+ynugCIvKjnR+xzMZcZvsig0Yh//6LG8lTBVOD8l8CRIr9b?=
 =?us-ascii?Q?/JokRnxSy+jIRIxgMJVXvc31jcGLBU4a2Yqq7hQ6PsPmoSVVA5lXbUM3OB6J?=
 =?us-ascii?Q?GJym0HCwnxgjLFJIFqOLqhlUE7Z56Dkesosm27p8wM4a554+XubmJuk2aU+a?=
 =?us-ascii?Q?5hJlDKch+H4YK7o7K2qSH8iBtVVeZkYgjRcCGM+8km+72dkeQ1WTtpzjvs4P?=
 =?us-ascii?Q?kbfr68tLLbC7vOPPud4nwezNfLVeyTQ1Ap3DCvlZpi3LdQ20dZGVKA3OgNhr?=
 =?us-ascii?Q?VkefaDYDJpwJwm8jRBbTQ4jkqH44JPN9rnGPLB55alMDCIlWTWefYcQjcR4T?=
 =?us-ascii?Q?wCLYyBwbahdQgix0oOZO09rlDwPMdP5ByZ/BsUph8UfUgmYmH6PdmGkoKva1?=
 =?us-ascii?Q?hBXu/+qvj1kI8g9+6oNIvcF12+wwgKiTW7ry55ozAN937H0HkL05LF8rb8if?=
 =?us-ascii?Q?vo/un3itQsuo9Cx2wY1iorHBPsFa2Az3eQGVH8LYq9SQYUsVekocuuyDo2mm?=
 =?us-ascii?Q?5NFQMfnrfwavDFX1GFvfMjj7HwXR/eRWcfjmOWeWNMXwM2eeJQmyF9NoadkQ?=
 =?us-ascii?Q?59xbDISHSQqE9PpwMzuAk9Ks61nNNXil2BFW0Ys+wp03u+4HDl5c3/nHhuA4?=
 =?us-ascii?Q?/UxOY+8JdNWSX0IAX7YkwxxoXlNeewvMc1TCaTQ1FF71ud3v24TlK3mRNS5T?=
 =?us-ascii?Q?Scwjotvx/PIXWM5B8jy/EcJ0/ZH7BCgsVzPDBCOMFjzL6TAE6CVCEY69Eo2r?=
 =?us-ascii?Q?juI7xkuU4CslzWm33YPqN0UMmKbKXxW5VguOscXn+/y66mf+WK1jjhHWjbzr?=
 =?us-ascii?Q?LoY1O9UrmdH/eJohcHaQVQy22dazL90tDzQysGcyOn44yYDGLMAqUorESZSp?=
 =?us-ascii?Q?5o20jOWjWNSfPZpYQpIkokTbcjtoTma576D3/m2BJv7+p7HjIlE8WeqV9YQ6?=
 =?us-ascii?Q?BQFRxmbE7T3Lsax2j5n0uxxwIfTOjFFb8HnzEtr34cj1y+usmiP9vomi7Gce?=
 =?us-ascii?Q?7PakJXipZPR0blJsaOwqcZt93kmhnWC9NOlzjFPpckJDNThe+o8m4DMPBaZz?=
 =?us-ascii?Q?GzZYr/8+EiSJaRhq5HE21M2FRCL4OnQ1nK3LwoyRI5bZA065iuWFnrTlJO2j?=
 =?us-ascii?Q?rRcjCqfcGsG49UpEfPD5gN7JzojJ/JSWmj5z04+x7YQhBiy4YhfYOhtMf1fm?=
 =?us-ascii?Q?mEqIJym+YoQekx6fbUNHun7Xm2ziGO6EY01rkqQwOePNVZbDRoAsa2RjvWG7?=
 =?us-ascii?Q?nASpAIxjGczJOuHZzSdo3msGdvr8QYh5T7x5hdQIkhkumfJtbgbq6hLvPTS7?=
 =?us-ascii?Q?XAdbuIxemre9nHCO9eyYNZW3LcwObbxrfxx+p1uziLGBPDYjBflnElrdQD8E?=
 =?us-ascii?Q?fdvKA2tcIm3mkRcK6TGxYMUR9vSnkf2YJLRlK4VWDvKe5G9bBTWkAfXGDXlT?=
 =?us-ascii?Q?gh0a+6GFz1OKXOi1k7F5LQRxg6n0uz4+0pWoXKRY?=
X-OriginatorOrg: nxp.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 2843157d-480f-407b-121d-08dc532ecd0d
X-MS-Exchange-CrossTenant-AuthSource: PAXPR04MB9642.eurprd04.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 02 Apr 2024 16:06:05.4816
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 686ea1d3-bc2b-4c6f-a92c-d99c5c301635
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: mIwyF5ipaUHuHja4AoaGZNrjwmFdgp7MU6bjvfWsJcmD9bl/qx4cjT16WyucxpvUpLHJLHH/ME1NBDV3ZtU+Sg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PAXPR04MB9256

On Tue, Apr 02, 2024 at 11:23:49AM +0200, Niklas Cassel wrote:
> On Tue, Apr 02, 2024 at 10:42:28AM +0530, Manivannan Sadhasivam wrote:
> > On Fri, Mar 22, 2024 at 07:19:01AM +0100, Niklas Cassel wrote:
> > > On Fri, Mar 22, 2024 at 10:56:23AM +0530, Manivannan Sadhasivam wrote:
> > > > On Thu, Mar 21, 2024 at 01:07:32PM -0500, Bjorn Helgaas wrote:
> > > > > On Thu, Mar 21, 2024 at 10:43:45PM +0530, Manivannan Sadhasivam wrote:
> > > > > > On Mon, Mar 04, 2024 at 05:46:16PM -0500, Frank Li wrote:
> > > > > > > dw_pcie_ep_inbound_atu()
> > > > > > > {
> > > > > > > 	...
> > > > > > > 	if (!ep->bar_to_atu[bar])
> > > > > > > 		free_win = find_first_zero_bit(ep->ib_window_map, pci->num_ib_windows);
> > > > > > > 	else
> > > > > > > 		free_win = ep->bar_to_atu[bar];
> > > > > > > 	...
> > > > > > > }
> > > > > > > 
> > > > > > > The atu index 0 is valid case for atu number. The find_first_zero_bit()
> > > > > > > will return 6 when second time call into this function if atu is 0. Suppose
> > > > > > > it should use branch 'free_win = ep->bar_to_atu[bar]'.
> > > > > > > 
> > > > > > > Change 'bar_to_atu' to free_win + 1. Initialize bar_to_atu as 0 to indicate
> > > > > > > it have not allocate atu to the bar.
> > > > > > 
> > > > > > I'd rewrite the commit message as below:
> > > > > > 
> > > > > > "The mapping between PCI BAR and iATU inbound window are maintained in the
> > > > > > dw_pcie_ep::bar_to_atu[] array. While allocating a new inbound iATU map for a
> > > > > > BAR, dw_pcie_ep_inbound_atu() API will first check for the availability of the
> > > > > > existing mapping in the array and if it is not found (i.e., value in the array
> > > > > > indexed by the BAR is found to be 0), then it will allocate a new map value
> > > > > > using find_first_zero_bit().
> > > > > > 
> > > > > > The issue here is, the existing logic failed to consider the fact that the map
> > > > > > value '0' is a valid value for BAR0. Because, find_first_zero_bit() will return
> > > > > > '0' as the map value for BAR0 (note that it returns the first zero bit
> > > > > > position).
> > > > > > 
> > > > > > Due to this, when PERST# assert + deassert happens on the PERST# supported
> > > > > > platforms, the inbound window allocation restarts from BAR0 and the existing
> > > > > > logic to find the BAR mapping will return '6' for BAR0 instead of '0' due to the
> > > > > > fact that it considers '0' as an invalid map value.
> > > > > > 
> > > > > > So fix this issue by always incrementing the map value before assigning to
> > > > > > bar_to_atu[] array and then decrementing it while fetching. This will make sure
> > > > > > that the map value '0' always represents the invalid mapping."
> > > > > 
> > > > > This translates C code to English in great detail, but still doesn't
> > > > > tell me what's broken from a user's point of view, how urgent the fix
> > > > > is, or how it should be handled.
> > > > > 
> > > > > DMA doesn't work because ATU setup is wrong?  Driver MMIO access to
> > > > > the device doesn't work?  OS crashes?  How?  Incorrectly routed access
> > > > > causes UR response?  Happens on every boot?  Only after a reboot or
> > > > > controller reset?  What platforms are affected?  "PERST# supported
> > > > > platforms" is not actionable without a lot of research or pre-existing
> > > > > knowledge.  Should this be backported to -stable?
> > > > > 
> > > > 
> > > > Severity is less for the bug fixed by this patch. We have 8 inbound iATU windows
> > > > on almost all of the platforms and after PERST# assert + deassert, BAR0 uses map
> > > > '6' instead of '0'.
> > > > 
> > > > This has no user visibility since the mapping will go fine and we have only 6
> > > > BARs. So I'd not mark this as as critical fix that needs special attention.
> > > 
> > > So we will have 6 mappings configured, but only 5 BARs, so two mappings for
> > > BAR0. The iATU looks at them in order, so index 0 will override index 6.
> > > 
> > > We are lucky that the endpoint subsystem does not clean up allocations properly
> > > right now (you have an outstanding series which fixes this).
> > > 
> > > If the endpoint subsystem did clean up resources properly, we would DMA to the
> > > area that was previously allocated for BAR0, instead of the new area for BAR0.
> > > 
> > 
> > How would DMA happen to the previously allocated area? When the BARs are cleared
> > properly and then allocated again, BAR0 will get the map of 0 again and then the
> > iATU will map window 0 with BAR0. Only if we don't clear the BARs properly (as
> > like now), then it will result in BAR0 having 2 identical mappings and even with
> > that there won't be any issue since both map 0 and 6 are valid.
> > 
> > Am I missing anything?
> 
> Like Bjorn summarize it:
> "We dodge the bullet as long as the mappings for BAR 0 are identical,
> which doesn't feel like much comfort."
> 
> Yes, right now we don't have a cleanup of either the backing memory for
> the BAR, or the iATUs mapping the PCI address to backing memory.
> (We allocate the backing memory for the BARs in .bind(), and free it in
> unbind().)
> 
> So the superfluous iATU6 mapping will be the same as the iATU0 mapping.
> 
> After your series, we will still allocate and free the backing memory
> in .bind()/.unbind(), but we will set/clear the iATU mapping in the
> .init()/.deinit() EPF callbacks.
> 
> 
> > 
> > > Perhaps just include this fix in your series that actually cleans up the BARs?
> > > 
> > 
> > Yeah, makes sense. Once we agree on a finalized commit message in this thread,
> > I'll include this patch in my series.
> 
> I think that we have spent too much time on this patch already.
> 
> My suggestion is that you simply apply it to pci/endpoint branch directly and
> fixup the commit message (like Bjorn usually does with [bhelgaas: commit log])
> after Frank's Sign-off.

I combined mani and your suggested message to 

https://lore.kernel.org/imx/20240326193540.3610570-1-Frank.Li@nxp.com/

I am okay to fine tune it by yourself. 

Frank

> 
> 
> Kind regards,
> Niklas