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 EC3F7C4725D for ; Fri, 19 Jan 2024 23:21:04 +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: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:References: List-Owner; bh=aYtlgVAeBvh3udsTH3lLNUeqwfL46QRdqi8aQnS22vg=; b=kdi8PqNuljG8ks CBNDJrNpGBW7cmulT61sjP9NgHCeFfJU+IKekMqmj72VQm9M8J0YQeS80ZMNzU5g+LsfUNDjBgFr/ pgvORrHiw4e59qTQyn6zpEps2fOTm61T2JzACEa7qzOZZjHL8Z8AGI4RqAjz9aPBPaea0IfP5XKar kxI3saXjE7gPV3bL2G31YZT8uTCFQkDBnYd1vACS0pFsE94dJl73s8faJdtmbqMrCgNrxkfoV06qy G+gWJZvLiDnwifytG0QF5QRaZrM0Y2pV+Kc2hhIOavr+yD59aJZlfsX9eZhmcHI7u1kVQaChzGo4q uwhDK/ABcvydJjmi3Xow==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.96 #2 (Red Hat Linux)) id 1rQyAB-006sSC-1t; Fri, 19 Jan 2024 23:20:39 +0000 Received: from dfw.source.kernel.org ([139.178.84.217]) by bombadil.infradead.org with esmtps (Exim 4.96 #2 (Red Hat Linux)) id 1rQyA8-006sRU-11 for linux-arm-kernel@lists.infradead.org; Fri, 19 Jan 2024 23:20:37 +0000 Received: from smtp.kernel.org (transwarp.subspace.kernel.org [100.75.92.58]) by dfw.source.kernel.org (Postfix) with ESMTP id D182C616AA; Fri, 19 Jan 2024 23:20:34 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 3CD33C433F1; Fri, 19 Jan 2024 23:20:34 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1705706434; bh=UdNTVVg961CwiMd5mYYDaAcvaTXvq85umlFhv66v4j0=; h=Date:From:To:Cc:Subject:In-Reply-To:From; b=iUPxmlZW3cYxAfpppMdb/zgUTSxrT57hkfXdY1Bqqne2bP3HJAVRqJOiMFAnyL2v2 mtgKFYVLsOyKU/dalbHacAlxKDtpxR9N8Q5kfXgSHOK2zKrnnMZavxopK0hx59O2fJ X79zrj5r727bHZ5rh0hl4IM5p/N/KLBpODpDJWnac/BKfnvxgQsad4qrPxEbY1gMaw uINgLmt6f+TwIEPaZHqUcIjgp5v18nnUVe+DYIby41w1eyaksUMIiE6xEnPhgv0Dpk ELmGDni7xmhgYqefbWiO4MGBmT3omsQ5FZeqEW6wklkawAeNcbY62aTyfIL8TZElYi G5eIbB/1TTkgg== Date: Fri, 19 Jan 2024 17:20:32 -0600 From: Bjorn Helgaas To: Siddharth Vadapalli Cc: lpieralisi@kernel.org, robh@kernel.org, kw@linux.com, bhelgaas@google.com, linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, ilpo.jarvinen@linux.intel.com, vigneshr@ti.com, r-gunasekaran@ti.com, srk@ti.com Subject: Re: [PATCH v3] PCI: keystone: Fix race condition when initializing PHYs Message-ID: <20240119232032.GA192245@bhelgaas> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20240119_152036_445517_D771ADE9 X-CRM114-Status: GOOD ( 26.62 ) 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 Wed, Jan 10, 2024 at 11:35:24AM +0530, Siddharth Vadapalli wrote: > Hello Bjorn, > > On 10/01/24 02:53, Bjorn Helgaas wrote: > > On Wed, Sep 27, 2023 at 09:48:45AM +0530, Siddharth Vadapalli wrote: > >> The PCI driver invokes the PHY APIs using the ks_pcie_enable_phy() > >> function. The PHY in this case is the Serdes. It is possible that the > >> PCI instance is configured for 2 lane operation across two different > > ... > > >> > >> + /* Obtain reference(s) to the phy(s) */ > >> + for (i = 0; i < num_lanes; i++) > >> + phy_pm_runtime_get_sync(ks_pcie->phy[i]); > >> + > >> ret = ks_pcie_enable_phy(ks_pcie); > >> + > >> + /* Release reference(s) to the phy(s) */ > >> + for (i = 0; i < num_lanes; i++) > >> + phy_pm_runtime_put_sync(ks_pcie->phy[i]); > > > > This looks good and has already been applied, so no immediate action > > required. > > > > This is the only call to ks_pcie_enable_phy(), and these loops get and > > put the PM references for the same PHYs initialized in > > ks_pcie_enable_phy(), so it seems like maybe these loops could be > > moved *into* ks_pcie_enable_phy(). > > Does the following look fine? > =============================================================================== > diff --git a/drivers/pci/controller/dwc/pci-keystone.c > b/drivers/pci/controller/dwc/pci-keystone.c > index e02236003b46..6e9f9589d26c 100644 > --- a/drivers/pci/controller/dwc/pci-keystone.c > +++ b/drivers/pci/controller/dwc/pci-keystone.c > @@ -962,6 +962,9 @@ static int ks_pcie_enable_phy(struct keystone_pcie *ks_pcie) > int num_lanes = ks_pcie->num_lanes; > > for (i = 0; i < num_lanes; i++) { > + /* Obtain reference to the phy */ > + phy_pm_runtime_get_sync(ks_pcie->phy[i]); I thought the point was that you needed to guarantee that all PHYs are powered on and stay that way before initializing any of them. If so, you would need a separate loop, e.g., for (i = 0; i < num_lanes; i++) phy_pm_runtime_get_sync(ks_pcie->phy[i]); for (i = 0; i < num_lanes; i++) { ret = phy_reset(ks_pcie->phy[i]); ... > ret = phy_reset(ks_pcie->phy[i]); > if (ret < 0) > goto err_phy; > @@ -977,12 +980,18 @@ static int ks_pcie_enable_phy(struct keystone_pcie *ks_pcie) > } > } > > + /* Release reference(s) to the phy(s) */ > + for (i = 0; i < num_lanes; i++) > + phy_pm_runtime_put_sync(ks_pcie->phy[i]); > + > return 0; > > err_phy: > while (--i >= 0) { > phy_power_off(ks_pcie->phy[i]); > phy_exit(ks_pcie->phy[i]); > + /* Release reference to the phy */ > + phy_pm_runtime_put_sync(ks_pcie->phy[i]); > } > > return ret; _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel