Message-ID: <20240304150417.GK2647@thinkpad>
Date: Mon, 4 Mar 2024 20:34:17 +0530
From: Manivannan Sadhasivam <mani@...nel.org>
To: Niklas Cassel <cassel@...nel.org>
Cc: Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>,
	Jingoo Han <jingoohan1@...il.com>,
	Gustavo Pimentel <gustavo.pimentel@...opsys.com>,
	Lorenzo Pieralisi <lpieralisi@...nel.org>,
	Krzysztof Wilczyński <kw@...ux.com>,
	Rob Herring <robh@...nel.org>, Bjorn Helgaas <bhelgaas@...gle.com>,
	Marek Vasut <marek.vasut+renesas@...il.com>,
	Yoshihiro Shimoda <yoshihiro.shimoda.uh@...esas.com>,
	Thierry Reding <thierry.reding@...il.com>,
	Jonathan Hunter <jonathanh@...dia.com>,
	Kishon Vijay Abraham I <kishon@...com>,
	Vidya Sagar <vidyas@...dia.com>,
	Vignesh Raghavendra <vigneshr@...com>,
	Richard Zhu <hongxing.zhu@....com>,
	Lucas Stach <l.stach@...gutronix.de>,
	Shawn Guo <shawnguo@...nel.org>,
	Sascha Hauer <s.hauer@...gutronix.de>,
	Pengutronix Kernel Team <kernel@...gutronix.de>,
	Fabio Estevam <festevam@...il.com>,
	NXP Linux Team <linux-imx@....com>,
	Minghuan Lian <minghuan.Lian@....com>,
	Mingkai Hu <mingkai.hu@....com>, Roy Zang <roy.zang@....com>,
	Kunihiko Hayashi <hayashi.kunihiko@...ionext.com>,
	Masami Hiramatsu <mhiramat@...nel.org>,
	Kishon Vijay Abraham I <kishon@...nel.org>,
	linux-pci@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-renesas-soc@...r.kernel.org, linux-arm-msm@...r.kernel.org,
	linux-tegra@...r.kernel.org, linux-omap@...r.kernel.org,
	linux-arm-kernel@...ts.infradead.org, linuxppc-dev@...ts.ozlabs.org
Subject: Re: [PATCH v8 03/10] PCI: dwc: ep: Introduce dw_pcie_ep_cleanup()
 API for drivers supporting PERST#

On Mon, Mar 04, 2024 at 11:51:04AM +0100, Niklas Cassel wrote:
> On Mon, Mar 04, 2024 at 01:47:13PM +0530, Manivannan Sadhasivam wrote:
> > On Thu, Feb 29, 2024 at 01:40:29PM +0100, Niklas Cassel wrote:
> > > On Sat, Feb 24, 2024 at 12:24:09PM +0530, Manivannan Sadhasivam wrote:
> > > 
> > > Since e.g. qcom-ep.c does a reset_control_assert() during perst
> > > assert/deassert, which should clear sticky registers, I think that
> > > you should let dw_pcie_ep_cleanup() clean up the BARs using
> > > dw_pcie_ep_clear_bar().
> > > 
> > 
> > As I mentioned earlier, it is the job of the EPF drivers to clear the BARs since
> > they allocate them. I'm trying to reduce the implicit resetting wherever we
> > could.
> > 
> > The proper fix is to add the LINK_DOWN callback to EPF drivers and do cleanup.
> > I'm planning to submit a series for that after this one.
> 
> Currently, pci-epf-test allocates memory for the BARs in .bind().
> Likewise it frees the memory for the BARs in .unbind().
> 
> AFAICT, most iATU registers, and most BAR registers are sticky registers,
> so they will not get reset on link down.
> (The currently selected BAR size, in case of Resizable BAR is an exception.)
> 
> That means that even on link down, we do not need to free the memory,
> or change the iATU settings. (This applies to all drivers.)
> 
> 
> 
> However, on PERST (for the drivers call dw_pcie_ep_cleanup()), they call
> reset_control_assert(), so they will clear sticky registers, which means
> that they need to at least re-write the iATU and BAR registers.
> (I guess they could free + allocate the memory for the BARs again,
> but I don't think that is strictly necessary.)
> That is why I suggested that you call dw_pcie_ep_clear_bar() from
> dw_pcie_ep_cleanup().
> 

Sorry, I keep assuming the flow w.r.t PERST# supported platforms :/

My bad!

> 
> 
> If you free the memory for the BARs in link_down() (this callback exists
> for many drivers, even drivers without a PERST handler), where are you
> supposted to alloc the memory for the BARs again?
> 
> Allocating them at link_up() is too late (because as soon as the link is
> up, the host is allowed to enumerate the EP BARs.) The proper place is to
> allocate them when receiving PERST, but not all drivers have a PERST handler.
> 
> (My understanding is that 1) PERST assert 2) PERST deassert 3) link is up.)
> 
> 
> 
> unbind() undos what was done in bind(), so shouldn't link_down() undo what was
> done in link_up()? With that logic, if you move the alloc to .core_init(),
> should we perhaps have a .core_deinit() callback for EPF drivers?
> (I guess only drivers which perform a reset during PERST would call this.)
> 
> But considering that free+alloc is not strictly needed, why not just keep
> the allocation + free in .bind()/.unbind() ?
> (To avoid the need to create a .core_deinit()), and let dw_pcie_ep_cleanup()
> call dw_pcie_ep_clear_bar() ?
> 
> I guess my point is that it seems a bit pointless for drivers that do not
> clear sticky registers to free+alloc memory on link down, for no good
> reason. (Memory might get fragmented over time, so it might not be possible
> to perform a big allocation after the device has been running for a really
> long time.)
> 
> 
> 
> So I'm thinking that we either
> 1) Keep the alloc/free in bind/unbind, and let dw_pcie_ep_cleanup() call
> dw_pcie_ep_clear_bar(),
> or
> 2) Introduce a .deinit_core() callback which will free the BARs.
> (Because I don't see how you will (re-)allocate memory for all drivers
> if you free the memory in link_down().)
> 

I think option 2 is the better solution. In my view, calling
dw_pcie_ep_clear_bar() from EPC drivers is a layering violation since the
allocation happens from EPF drivers.

So clearing the BARs during the deinit() callback that gets called when PERST#
assert happens is the way to go.

- Mani

-- 
மணிவண்ணன் சதாசிவம்

Powered by blists - more mailing lists