All of lore.kernel.org
 help / color / mirror / Atom feed
From: Bjorn Helgaas <helgaas@kernel.org>
To: linux-pci@vger.kernel.org
Cc: "Rafael J . Wysocki" <rjw@rjwysocki.net>,
	Sagi Grimberg <sagi@grimberg.me>,
	Mario Limonciello <Mario.Limonciello@dell.com>,
	Keith Busch <keith.busch@intel.com>,
	Kai-Heng Feng <kai.heng.feng@canonical.com>,
	Rajat Jain <rajatja@google.com>, Christoph Hellwig <hch@lst.de>,
	Heiner Kallweit <hkallweit1@gmail.com>,
	linux-pm@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-nvme@lists.infradead.org,
	Bjorn Helgaas <bhelgaas@google.com>
Subject: [PATCH 1/1] PCI/ASPM: Remove pcie_aspm_enabled() unnecessary locking
Date: Thu, 10 Oct 2019 07:47:46 -0500	[thread overview]
Message-ID: <20191010124746.2882-2-helgaas@kernel.org> (raw)
In-Reply-To: <20191010124746.2882-1-helgaas@kernel.org>

From: Bjorn Helgaas <bhelgaas@google.com>

The lifetime of the link_state structure (bridge->link_state) is not the
same as the lifetime of "bridge" itself.  The link_state is allocated by
pcie_aspm_init_link_state() after children of the bridge have been
enumerated, and it is deallocated by pcie_aspm_exit_link_state() after all
children of the bridge (but not the bridge itself) have been removed.

Previously pcie_aspm_enabled() acquired aspm_lock to ensure that
link_state was not deallocated while we're looking at it.  But the fact
that the caller of pcie_aspm_enabled() holds a reference to @pdev means
there's always at least one child of the bridge, which means link_state
can't be deallocated.

Remove the unnecessary locking in pcie_aspm_enabled().

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/pcie/aspm.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index 652ef23bba35..f5c7138a34aa 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -1172,20 +1172,20 @@ module_param_call(policy, pcie_aspm_set_policy, pcie_aspm_get_policy,
 /**
  * pcie_aspm_enabled - Check if PCIe ASPM has been enabled for a device.
  * @pdev: Target device.
+ *
+ * Relies on the upstream bridge's link_state being valid.  The link_state
+ * is deallocated only when the last child of the bridge (i.e., @pdev or a
+ * sibling) is removed, and the caller should be holding a reference to
+ * @pdev, so this should be safe.
  */
 bool pcie_aspm_enabled(struct pci_dev *pdev)
 {
 	struct pci_dev *bridge = pci_upstream_bridge(pdev);
-	bool ret;
 
 	if (!bridge)
 		return false;
 
-	mutex_lock(&aspm_lock);
-	ret = bridge->link_state ? !!bridge->link_state->aspm_enabled : false;
-	mutex_unlock(&aspm_lock);
-
-	return ret;
+	return bridge->link_state ? !!bridge->link_state->aspm_enabled : false;
 }
 EXPORT_SYMBOL_GPL(pcie_aspm_enabled);
 
-- 
2.23.0.581.g78d2f28ef7-goog


WARNING: multiple messages have this Message-ID (diff)
From: Bjorn Helgaas <helgaas@kernel.org>
To: linux-pci@vger.kernel.org
Cc: Sagi Grimberg <sagi@grimberg.me>,
	Mario Limonciello <Mario.Limonciello@dell.com>,
	linux-pm@vger.kernel.org,
	"Rafael J . Wysocki" <rjw@rjwysocki.net>,
	linux-kernel@vger.kernel.org, linux-nvme@lists.infradead.org,
	Keith Busch <keith.busch@intel.com>,
	Kai-Heng Feng <kai.heng.feng@canonical.com>,
	Bjorn Helgaas <bhelgaas@google.com>,
	Rajat Jain <rajatja@google.com>, Christoph Hellwig <hch@lst.de>,
	Heiner Kallweit <hkallweit1@gmail.com>
Subject: [PATCH 1/1] PCI/ASPM: Remove pcie_aspm_enabled() unnecessary locking
Date: Thu, 10 Oct 2019 07:47:46 -0500	[thread overview]
Message-ID: <20191010124746.2882-2-helgaas@kernel.org> (raw)
In-Reply-To: <20191010124746.2882-1-helgaas@kernel.org>

From: Bjorn Helgaas <bhelgaas@google.com>

The lifetime of the link_state structure (bridge->link_state) is not the
same as the lifetime of "bridge" itself.  The link_state is allocated by
pcie_aspm_init_link_state() after children of the bridge have been
enumerated, and it is deallocated by pcie_aspm_exit_link_state() after all
children of the bridge (but not the bridge itself) have been removed.

Previously pcie_aspm_enabled() acquired aspm_lock to ensure that
link_state was not deallocated while we're looking at it.  But the fact
that the caller of pcie_aspm_enabled() holds a reference to @pdev means
there's always at least one child of the bridge, which means link_state
can't be deallocated.

Remove the unnecessary locking in pcie_aspm_enabled().

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/pcie/aspm.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index 652ef23bba35..f5c7138a34aa 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -1172,20 +1172,20 @@ module_param_call(policy, pcie_aspm_set_policy, pcie_aspm_get_policy,
 /**
  * pcie_aspm_enabled - Check if PCIe ASPM has been enabled for a device.
  * @pdev: Target device.
+ *
+ * Relies on the upstream bridge's link_state being valid.  The link_state
+ * is deallocated only when the last child of the bridge (i.e., @pdev or a
+ * sibling) is removed, and the caller should be holding a reference to
+ * @pdev, so this should be safe.
  */
 bool pcie_aspm_enabled(struct pci_dev *pdev)
 {
 	struct pci_dev *bridge = pci_upstream_bridge(pdev);
-	bool ret;
 
 	if (!bridge)
 		return false;
 
-	mutex_lock(&aspm_lock);
-	ret = bridge->link_state ? !!bridge->link_state->aspm_enabled : false;
-	mutex_unlock(&aspm_lock);
-
-	return ret;
+	return bridge->link_state ? !!bridge->link_state->aspm_enabled : false;
 }
 EXPORT_SYMBOL_GPL(pcie_aspm_enabled);
 
-- 
2.23.0.581.g78d2f28ef7-goog


_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

  reply	other threads:[~2019-10-10 12:48 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-10-10 12:47 [PATCH 0/1] PCI/ASPM: Remove locking Bjorn Helgaas
2019-10-10 12:47 ` Bjorn Helgaas
2019-10-10 12:47 ` Bjorn Helgaas [this message]
2019-10-10 12:47   ` [PATCH 1/1] PCI/ASPM: Remove pcie_aspm_enabled() unnecessary locking Bjorn Helgaas
2019-10-10 14:01   ` Christoph Hellwig
2019-10-10 14:01     ` Christoph Hellwig
2019-10-10 16:14     ` Bjorn Helgaas
2019-10-10 16:14       ` Bjorn Helgaas
2019-10-10 16:28   ` Rafael J. Wysocki
2019-10-10 16:28     ` Rafael J. Wysocki

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20191010124746.2882-2-helgaas@kernel.org \
    --to=helgaas@kernel.org \
    --cc=Mario.Limonciello@dell.com \
    --cc=bhelgaas@google.com \
    --cc=hch@lst.de \
    --cc=hkallweit1@gmail.com \
    --cc=kai.heng.feng@canonical.com \
    --cc=keith.busch@intel.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-nvme@lists.infradead.org \
    --cc=linux-pci@vger.kernel.org \
    --cc=linux-pm@vger.kernel.org \
    --cc=rajatja@google.com \
    --cc=rjw@rjwysocki.net \
    --cc=sagi@grimberg.me \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.