Received-SPF: pass (google.com: domain of linux-kernel+bounces-5344-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Tue, 19 Dec 2023 09:20:21 -0500
From: Frank Li <Frank.li@nxp.com>
To: Niklas Cassel <Niklas.Cassel@wdc.com>
Cc: "imx@lists.linux.dev" <imx@lists.linux.dev>,
	Jingoo Han <jingoohan1@gmail.com>,
	Gustavo Pimentel <gustavo.pimentel@synopsys.com>,
	Manivannan Sadhasivam <mani@kernel.org>,
	Lorenzo Pieralisi <lpieralisi@kernel.org>,
	Krzysztof =?utf-8?Q?Wilczy=C5=84ski?= <kw@linux.com>,
	Rob Herring <robh@kernel.org>, Bjorn Helgaas <bhelgaas@google.com>,
	Jon Mason <jdmason@kudzu.us>,
	"open list:PCI DRIVER FOR SYNOPSYS DESIGNWARE" <linux-pci@vger.kernel.org>,
	open list <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 1/1] PCI: dwc: Fix BAR0 wrong map to iATU6 after root
 complex reinit endpoint
Message-ID: <ZYGmpaf18pJgM/qj@lizhi-Precision-Tower-5810>
References: <20231219044844.1195294-1-Frank.Li@nxp.com>
 <ZYFrUWM7JXdv7rtb@x1-carbon>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <ZYFrUWM7JXdv7rtb@x1-carbon>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0:
	=?us-ascii?Q?ZalhVp76npHGe946whZ7FDGjlVEuhJm6VnjV+yDVFLQjHhsTBRDK+gC2E9OE?=
 =?us-ascii?Q?VIhSLy71Ypmo2/LnLrWDHRDV0xuE0wBu6mwEJAVcspyvdMhxNvza7TQvafzU?=
 =?us-ascii?Q?SHBRkI9EbR7oYOl2QzHziRtXgz/yXfPwzle3CThZssUnYTgfo7Hy2AddhBst?=
 =?us-ascii?Q?RPK84OzUl4vULLAdfJSYixsPm3hUhMb7VweXF18P6WE8hrxdFmHXC8nkvsiA?=
 =?us-ascii?Q?CWnRR3nbQDi6wYSMI11wvk8CjIB3VvnIa3Atl0QmEf47slpSZukmdFVi3PFm?=
 =?us-ascii?Q?hUCOdJsgQOYSqzO0eqH0hjdE1QpGeimxRXeid/fe9AMxNYQXc7Q7ViJj7UwR?=
 =?us-ascii?Q?GcMdfMUIehe2F3iZiz1aElZL43p/Yk7eYUYj6JsiLjTjQVUZCn8mTyy2KUFu?=
 =?us-ascii?Q?T3sIGhRXKGZp+LvNE72+VuvhTgLPcq7OfpgaU2HsTVxEV9YHgStnv/AmyYag?=
 =?us-ascii?Q?rGrnvVesdaxHyw2xhejbmtUExRm1jCkKygwTE4wd/9hJdFmpoLf3r/a3jxSs?=
 =?us-ascii?Q?NkDKv+HKWdPLZSunYfqwuqayXljgaq4Mz4FGe+wupijo33uhw17artGIyoJS?=
 =?us-ascii?Q?/SzSDYjbOvBljH3Ngu+xA2+Eh1eWN37VBlNE2yiXUXWeQsKpggAUpvCeGnum?=
 =?us-ascii?Q?DsFfVBangs+sbChjVVuPy3TxYMQe9AFcxAs82x8Zr3y+xvMaGK7J4JXA5NPY?=
 =?us-ascii?Q?f6gwof7Lrd86Tt54oew0sz7BNr4G7uQO3GGETNemKun+gY8W3zDWx4QzfKfC?=
 =?us-ascii?Q?yxs+c8zgfQRz5X0se4VszEFqtsjEs9Aj/A9UJXJsaWFtRLLAkjmBVFE72EeO?=
 =?us-ascii?Q?ducnhFLQfdiz1sDgBMvdrIqxInCmeYH9yqkhqTm175XPe+KuNbjJ0mA0XZ3J?=
 =?us-ascii?Q?VipufVqLysI1wuz2+R6T1wHscecrqnK9rbswM53mT115y6O81rMnVdYelaPg?=
 =?us-ascii?Q?eeeuFwPLPnGSaG8BcJur1KLmst5IGtT7gj3QAaqeqJu80QdeGPrC5Nroy3/S?=
 =?us-ascii?Q?UJ+PmUTwEVib47+LxfANVTa9jW8BhdBfjY1CCT1uOBQSJeu5PQAcOZGUP8qv?=
 =?us-ascii?Q?XIScWoZ2pZkSo1N7fTxUcL7RnNTcxlm4v20cN2gsUrIGulZHOH8K6wUypEwC?=
 =?us-ascii?Q?RzMEAhPi+1ItV3pJ0NJhwhPSakM0x+5tS6Dfc+4dJ1Aejr3ewQLKChduy9Ac?=
 =?us-ascii?Q?ptkyKGG+7dI5GIwsnGHyYL3u/htQJNNB/x5mrxvD8ClH3REGMiotPnzeDlK6?=
 =?us-ascii?Q?a1NGWwQEGeyTUluM9LZaxMoZ+RV6e/tDb29JYjuVJEzs5k6s7W65cTo/Wt8q?=
 =?us-ascii?Q?irGG232ofGinRCOIXOsrXJPCUXy7rCcDoVagb8XkRmSoQVPgYqtduAbFnmsa?=
 =?us-ascii?Q?TPVsAP65mCRJzZEM6qDNjeFJwbSrOGC0LQ28ug6/anD4s6LekKx++ZtBS8nl?=
 =?us-ascii?Q?l8rQa3RZkkm/mc3CWIT2rKZqdybjyR2PVnVs1ven78f91A5vv4CAx8+nVPq2?=
 =?us-ascii?Q?FfcWb8tFWm5Y0HD8axf3I3V71MY0JdUFHVuIWH1dPp6V1YtMzDUeBaeTPWnV?=
 =?us-ascii?Q?LHPVaINfmlw4adAZUDc=3D?=
X-OriginatorOrg: nxp.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 6ade4539-370b-46b6-3514-08dc009da6b7
X-MS-Exchange-CrossTenant-AuthSource: AM6PR04MB4838.eurprd04.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 19 Dec 2023 14:20:28.8278
 (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: SpR2hc7AsXDa8nFhmhbIBYeI9bRuMS8OTlLBkYpM/k9uNbtMe/nY/sXmc32kHuin8Z7W/hfpKMExieN4fH83AA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DU2PR04MB9083

On Tue, Dec 19, 2023 at 10:07:14AM +0000, Niklas Cassel wrote:
> On Mon, Dec 18, 2023 at 11:48:43PM -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 s8. Initialize bar_to_atu as -1 to indicate it have
> > not allocate atu to the bar.
> > 
> > Reported-by: Niklas Cassel <Niklas.Cassel@wdc.com>
> > Close: https://lore.kernel.org/linux-pci/ZXt2A+Fusfz3luQV@x1-carbon/T/#u
> > Fixes: 4284c88fff0e ("PCI: designware-ep: Allow pci_epc_set_bar() update inbound map address")
> > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > ---
> > 
> > Notes:
> >     @Niklas:
> >     	I have not test your case. I should be equal to previous's fix in
> >     mail list.
> 
> Hello Frank,
> 
> Thank you for sending a proper fix for this!
> 
> Personally, I slightly prefer your fix that saves the iatu index + 1, and
> keeps 0 to mean unused. That way, you don't need the memset, and you don't
> need to change the type to signed, but either way is fine by me, so:

index + 1 don't match hardware iATU index. It will be confused because
other parts is 0 based.

So I choose "-1" as free iATU. 

Frank

> 
> Reviewed-by: Niklas Cassel <niklas.cassel@wdc.com>
> 
> > 
> >  drivers/pci/controller/dwc/pcie-designware-ep.c | 11 ++++++++---
> >  drivers/pci/controller/dwc/pcie-designware.h    |  2 +-
> >  2 files changed, 9 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
> > index f6207989fc6ad..0ff5cd64f49b0 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware-ep.c
> > +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
> > @@ -174,7 +174,7 @@ static int dw_pcie_ep_inbound_atu(struct dw_pcie_ep *ep, u8 func_no, int type,
> >  	u32 free_win;
> >  	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> >  
> > -	if (!ep->bar_to_atu[bar])
> > +	if (ep->bar_to_atu[bar] < 0)
> >  		free_win = find_first_zero_bit(ep->ib_window_map, pci->num_ib_windows);
> >  	else
> >  		free_win = ep->bar_to_atu[bar];
> > @@ -228,14 +228,17 @@ static void dw_pcie_ep_clear_bar(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
> >  	struct dw_pcie_ep *ep = epc_get_drvdata(epc);
> >  	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> >  	enum pci_barno bar = epf_bar->barno;
> > -	u32 atu_index = ep->bar_to_atu[bar];
> > +	s8 atu_index = ep->bar_to_atu[bar];
> > +
> > +	if (atu_index < 0)
> > +		return;
> >  
> >  	__dw_pcie_ep_reset_bar(pci, func_no, bar, epf_bar->flags);
> >  
> >  	dw_pcie_disable_atu(pci, PCIE_ATU_REGION_DIR_IB, atu_index);
> >  	clear_bit(atu_index, ep->ib_window_map);
> >  	ep->epf_bar[bar] = NULL;
> > -	ep->bar_to_atu[bar] = 0;
> > +	ep->bar_to_atu[bar] = -1;
> >  }
> >  
> >  static int dw_pcie_ep_set_bar(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
> > @@ -767,6 +770,8 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep)
> >  		return -ENOMEM;
> >  	ep->outbound_addr = addr;
> >  
> > +	memset(ep->bar_to_atu, -1, sizeof(ep->bar_to_atu));
> > +
> >  	epc = devm_pci_epc_create(dev, &epc_ops);
> >  	if (IS_ERR(epc)) {
> >  		dev_err(dev, "Failed to create epc device\n");
> > diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
> > index 55ff76e3d3846..5879907c5cf25 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware.h
> > +++ b/drivers/pci/controller/dwc/pcie-designware.h
> > @@ -362,7 +362,7 @@ struct dw_pcie_ep {
> >  	phys_addr_t		phys_base;
> >  	size_t			addr_size;
> >  	size_t			page_size;
> > -	u8			bar_to_atu[PCI_STD_NUM_BARS];
> > +	s8			bar_to_atu[PCI_STD_NUM_BARS];
> >  	phys_addr_t		*outbound_addr;
> >  	unsigned long		*ib_window_map;
> >  	unsigned long		*ob_window_map;
> > -- 
> > 2.34.1
> >