From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 7A327C48BF6 for ; Mon, 4 Mar 2024 10:51:35 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=og7fJQqzKgzTyZBepuebvwG6PVVr3y0u666mqPT8lGI=; b=OZrt9BvPaJoZx8 91WCHp5WYrMK97onfg/IaGOh7liGgCLcV6zLcL9Vhv18uI8mUzFKzomZ6t4LXOfVV2TRkhmFVh+Ec eVBakyuo0m02Huu0ZCmNur6gEdlJvJag+IMgT5UoMjlt3fWQ8jOLwL8tfBoH8Up3Jq5Z1ajzYptLq fnO6w8MufFPhThhKCoqLI6jNcmHyu1az9eWlI6CFV/XEVb49rqakesWNkiAer2bpSQM0itcuJv1FV 91WZZTRggb7X1Ayf0zYlTHxH0bqSy/4W6NOhg5nwyThaflAmwYAslA60VyQ7o+GbWPszD1xaW1bJn yvZtOGz2tDrtgccyiYNA==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.97.1 #2 (Red Hat Linux)) id 1rh5uk-00000008hmn-0Hcq; Mon, 04 Mar 2024 10:51:22 +0000 Received: from sin.source.kernel.org ([145.40.73.55]) by bombadil.infradead.org with esmtps (Exim 4.97.1 #2 (Red Hat Linux)) id 1rh5uh-00000008hlJ-2po7 for linux-arm-kernel@lists.infradead.org; Mon, 04 Mar 2024 10:51:21 +0000 Received: from smtp.kernel.org (transwarp.subspace.kernel.org [100.75.92.58]) by sin.source.kernel.org (Postfix) with ESMTP id D08D1CE1099; Mon, 4 Mar 2024 10:51:15 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id ABB22C433F1; Mon, 4 Mar 2024 10:51:07 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1709549475; bh=1zd3GVu3Rm2C+KWSEoGXpIwLN5A0GfhS1q9Aw+9W7rM=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=IUdmHlqmjdF9sVcEjisGPLRse7lw64LkirsFmw8ShEDmhnEjzp+/xEafNTJlZcV0Y dZe7GxwvZCcDsFMuNpeI/TBwtkXIQfhyEmx2XSYukzbvJ2wo7p+6JOKdVe9r9toohS NlTeaFLYq74S/3aEiVdhGrcxHA+8HRd3jOHW7ID/kFX181CUuQt3Af87RQlcW0AV8Y 8kdxJIksuaKE5Ts22TYEpnkd0vBxLtG3xh0tytDO0EoHd+LsQIFDr+ctBOLe3l4hdx K6a+Q6vuqbd6IvLegVq8GqYu9Apf58BPfO3BJ5lX9OF7SPNebpBmSvqGTFOLTQKZJX LXYTijmD2A6Xw== Date: Mon, 4 Mar 2024 11:51:04 +0100 From: Niklas Cassel To: Manivannan Sadhasivam Cc: Jingoo Han , Gustavo Pimentel , Lorenzo Pieralisi , Krzysztof =?utf-8?Q?Wilczy=C5=84ski?= , Rob Herring , Bjorn Helgaas , Marek Vasut , Yoshihiro Shimoda , Thierry Reding , Jonathan Hunter , Kishon Vijay Abraham I , Vidya Sagar , Vignesh Raghavendra , Richard Zhu , Lucas Stach , Shawn Guo , Sascha Hauer , Pengutronix Kernel Team , Fabio Estevam , NXP Linux Team , Minghuan Lian , Mingkai Hu , Roy Zang , Kunihiko Hayashi , Masami Hiramatsu , Kishon Vijay Abraham I , linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org, linux-renesas-soc@vger.kernel.org, linux-arm-msm@vger.kernel.org, linux-tegra@vger.kernel.org, linux-omap@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linuxppc-dev@lists.ozlabs.org Subject: Re: [PATCH v8 03/10] PCI: dwc: ep: Introduce dw_pcie_ep_cleanup() API for drivers supporting PERST# Message-ID: References: <20240224-pci-dbi-rework-v8-0-64c7fd0cfe64@linaro.org> <20240224-pci-dbi-rework-v8-3-64c7fd0cfe64@linaro.org> <20240304081713.GH2647@thinkpad> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20240304081713.GH2647@thinkpad> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20240304_025120_124739_36D6D922 X-CRM114-Status: GOOD ( 25.24 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org 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(). 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().) Kind regards, Niklas _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel