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 X-Spam-Level: X-Spam-Status: No, score=-8.6 required=3.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY, SPF_HELO_NONE,SPF_PASS,USER_AGENT_SANE_1 autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 0E4E5C433DF for ; Tue, 30 Jun 2020 12:31:49 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id DF85520672 for ; Tue, 30 Jun 2020 12:31:48 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1593520308; bh=PhXZOiHYFfQIlVAYlW6qFnQSXZojRskUV7lRHqn16QI=; h=Date:From:To:Cc:Subject:References:In-Reply-To:List-ID:From; b=K2oedlJHNcgD4NF5h0W7+1QqYnPtsPpjIpkByGi++JV6z8a4DYTDrDCCplBwHE9ha 4IK+cllwh0nUVyIW/mx1ksAi+xtaGQziDjcyxcfg0iOauXse8+Tr5wu62IwRnaXpzf b4UM6rGkE9iGk+960M96aBAvPjPOidJaCNpFb+xo= Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S2387681AbgF3Mbs (ORCPT ); Tue, 30 Jun 2020 08:31:48 -0400 Received: from mail.kernel.org ([198.145.29.99]:41128 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1730095AbgF3Mbr (ORCPT ); Tue, 30 Jun 2020 08:31:47 -0400 Received: from pali.im (pali.im [31.31.79.79]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPSA id B08BC20780; Tue, 30 Jun 2020 12:31:46 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1593520306; bh=PhXZOiHYFfQIlVAYlW6qFnQSXZojRskUV7lRHqn16QI=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=cqHNnvn1lWCLKzNx58COqIF64R7i6pFEt/4LFtvh1Ej/N2dgN/Q+MWsoWC3ZNu5Fr fJcY48jSvAkZZNEUk3EkwlqRFhITTJKiKqMCke3rrxcBk83U0HF0I1+QBipkFT1oEp Q/L7EyKjSyu+aoYwUk/9qDLLHmTudgKe/qVS3pFo= Received: by pali.im (Postfix) id 944ED81A; Tue, 30 Jun 2020 14:31:44 +0200 (CEST) Date: Tue, 30 Jun 2020 14:31:44 +0200 From: Pali =?utf-8?B?Um9ow6Fy?= To: Bjorn Helgaas Cc: Thomas Petazzoni , Lorenzo Pieralisi , Andrew Murray , Bjorn Helgaas , Marek =?utf-8?B?QmVow7pu?= , Remi Pommarel , Tomasz Maciej Nowak , Xogium , linux-pci@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH] PCI: aardvark: Don't touch PCIe registers if no card connected Message-ID: <20200630123144.vllnun266i6n5q4d@pali> References: <20200528163809.54f5ldvphrjg3zg3@pali> <20200528164938.GA325239@bjorn-Precision-5520> <20200529083013.5cg7tvfemomnmvjd@pali> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20200529083013.5cg7tvfemomnmvjd@pali> User-Agent: NeoMutt/20180716 Sender: linux-pci-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-pci@vger.kernel.org Hello! On Friday 29 May 2020 10:30:13 Pali Rohár wrote: > On Thursday 28 May 2020 11:49:38 Bjorn Helgaas wrote: > > On Thu, May 28, 2020 at 06:38:09PM +0200, Pali Rohár wrote: > > > On Thursday 28 May 2020 11:26:04 Bjorn Helgaas wrote: > > > > On Thu, May 28, 2020 at 04:31:41PM +0200, Pali Rohár wrote: > > > > > When there is no PCIe card connected and advk_pcie_rd_conf() or > > > > > advk_pcie_wr_conf() is called for PCI bus which doesn't belong to emulated > > > > > root bridge, the aardvark driver throws the following error message: > > > > > > > > > > advk-pcie d0070000.pcie: config read/write timed out > > > > > > > > > > Obviously accessing PCIe registers of disconnected card is not possible. > > > > > > > > > > Extend check in advk_pcie_valid_device() function for validating > > > > > availability of PCIe bus. If PCIe link is down, then the device is marked > > > > > as Not Found and the driver does not try to access these registers. > > > > > > > > > > Signed-off-by: Pali Rohár > > > > > --- > > > > > drivers/pci/controller/pci-aardvark.c | 3 +++ > > > > > 1 file changed, 3 insertions(+) > > > > > > > > > > diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c > > > > > index 90ff291c24f0..53a4cfd7d377 100644 > > > > > --- a/drivers/pci/controller/pci-aardvark.c > > > > > +++ b/drivers/pci/controller/pci-aardvark.c > > > > > @@ -644,6 +644,9 @@ static bool advk_pcie_valid_device(struct advk_pcie *pcie, struct pci_bus *bus, > > > > > if ((bus->number == pcie->root_bus_nr) && PCI_SLOT(devfn) != 0) > > > > > return false; > > > > > > > > > > + if (bus->number != pcie->root_bus_nr && !advk_pcie_link_up(pcie)) > > > > > + return false; > > > > > > > > I don't think this is the right fix. This makes it racy because the > > > > link may go down after we call advk_pcie_valid_device() but before we > > > > perform the config read. > > > > > > Yes, it is racy, but I do not think it cause problems. Trying to read > > > PCIe registers when device is not connected cause just those timeouts, > > > printing error message and increased delay in advk_pcie_wait_pio() due > > > to polling loop. This patch reduce unnecessary access to PCIe registers > > > when advk_pcie_wait_pio() polling just fail. > > > > > > I think it is a good idea to not call blocking advk_pcie_wait_pio() when > > > it is not needed. We could have faster enumeration of PCIe buses when > > > card is not connected. > > > > Maybe advk_pcie_check_pio_status() and advk_pcie_wait_pio() could be > > combined so we could get the correct error status as soon as it's > > available, without waiting for a timeout? > > Any idea how to achieve it? > > First call is polling function advk_pcie_wait_pio() and second call is > advk_pcie_check_pio_status() which just reads status register and prints > error message to dmesg. > > So for me it looks like that combining these two functions into one does > not change anything. We always need to call polling code prior to > checking status register. And therefore need to wait for timeout. Unless > something like in this proposed patch is not used (to skip whole > register access if it would fail). So to answer your question, correct status is possible to retrieve only after waiting for timeout. As status would be available only after timeout expires. Therefore my proposed patch in this (or some other) form is needed if we want to prevent trying to read from registers and waiting for answer when card is disconnected. I would really like to see this issue fixed, so booting linux kernel on board without connected PCIe card would not be delayed. Thomas, Lorenzo, Bjorn: do you have any idea how to fix it differently? Or if not, could be my proposed patch accepted in some form? > > In any event, the "return PCIBIOS_SET_FAILED" needs to be fixed. Most > > callers of config read do not check for failure, but most of the ones > > that do, check for "val == ~0". Only a few check for a status of > > other than PCIBIOS_SUCCESSFUL. > > > > > > I have no objection to removing the "config read/write timed out" > > > > message. The "return PCIBIOS_SET_FAILED" in the read case probably > > > > should be augmented by setting "*val = 0xffffffff". > > Now I see, "*val = 0xffffffff" should be really set when function > advk_pcie_rd_conf() fails. I have already sent separate patch which fixes this issue. > > > > > return true; > > > > > } > > > > > > > > > > -- > > > > > 2.20.1 > > > > >