linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: "Rafael J. Wysocki" <rafael@kernel.org>
To: Bjorn Helgaas <helgaas@kernel.org>
Cc: "Rafael J. Wysocki" <rafael@kernel.org>,
	"Rafael J. Wysocki" <rjw@rjwysocki.net>,
	linux-nvme <linux-nvme@lists.infradead.org>,
	Keith Busch <kbusch@kernel.org>,
	Mario Limonciello <Mario.Limonciello@dell.com>,
	Kai-Heng Feng <kai.heng.feng@canonical.com>,
	Keith Busch <keith.busch@intel.com>,
	Christoph Hellwig <hch@lst.de>, Sagi Grimberg <sagi@grimberg.me>,
	Linux PM <linux-pm@vger.kernel.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	Rajat Jain <rajatja@google.com>,
	Linux PCI <linux-pci@vger.kernel.org>,
	Heiner Kallweit <hkallweit1@gmail.com>
Subject: Re: [PATCH v3 1/2] PCI: PCIe: ASPM: Introduce pcie_aspm_enabled()
Date: Wed, 9 Oct 2019 00:54:37 +0200	[thread overview]
Message-ID: <CAJZ5v0j-uSM2gheHViommWcrSaLVfzbwV3M7OOWz3GyzhHSwRA@mail.gmail.com> (raw)
In-Reply-To: <20191008211656.GA163302@google.com>

On Tue, Oct 8, 2019 at 11:16 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> On Tue, Oct 08, 2019 at 11:27:51AM +0200, Rafael J. Wysocki wrote:
> > On Tue, Oct 8, 2019 at 12:34 AM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > > On Thu, Aug 08, 2019 at 11:55:07PM +0200, Rafael J. Wysocki wrote:
> > > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > >
> > > > Add a function checking whether or not PCIe ASPM has been enabled for
> > > > a given device.
> > > >
> > > > It will be used by the NVMe driver to decide how to handle the
> > > > device during system suspend.
> > > >
> > > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > > ---
> > > >
> > > > v2 -> v3:
> > > >   * Make the new function return bool.
> > > >   * Change its name back to pcie_aspm_enabled().
> > > >   * Fix kerneldoc comment formatting.
> > > >
> > > > -> v2:
> > > >   * Move the PCI/PCIe ASPM changes to a separate patch.
> > > >   * Add the _mask suffix to the new function name.
> > > >   * Add EXPORT_SYMBOL_GPL() to the new function.
> > > >   * Avoid adding an unnecessary blank line.
> > > >
> > > > ---
> > > >  drivers/pci/pcie/aspm.c |   20 ++++++++++++++++++++
> > > >  include/linux/pci.h     |    3 +++
> > > >  2 files changed, 23 insertions(+)
> > > >
> > > > Index: linux-pm/drivers/pci/pcie/aspm.c
> > > > ===================================================================
> > > > --- linux-pm.orig/drivers/pci/pcie/aspm.c
> > > > +++ linux-pm/drivers/pci/pcie/aspm.c
> > > > @@ -1170,6 +1170,26 @@ static int pcie_aspm_get_policy(char *bu
> > > >  module_param_call(policy, pcie_aspm_set_policy, pcie_aspm_get_policy,
> > > >       NULL, 0644);
> > > >
> > > > +/**
> > > > + * pcie_aspm_enabled - Check if PCIe ASPM has been enabled for a device.
> > > > + * @pci_device: Target device.
> > > > + */
> > > > +bool pcie_aspm_enabled(struct pci_dev *pci_device)
> > > > +{
> > > > +     struct pci_dev *bridge = pci_upstream_bridge(pci_device);
> > > > +     bool ret;
> > > > +
> > > > +     if (!bridge)
> > > > +             return false;
> > > > +
> > > > +     mutex_lock(&aspm_lock);
> > > > +     ret = bridge->link_state ? !!bridge->link_state->aspm_enabled : false;
> > > > +     mutex_unlock(&aspm_lock);
> > >
> > > Why do we need to acquire aspm_lock here?  We aren't modifying
> > > anything, and I don't think we're preventing a race.  If this races
> > > with another thread that changes aspm_enabled, we'll return either the
> > > old state or the new one, and I think that's still the case even if we
> > > don't acquire aspm_lock.
> >
> > Well, if we can guarantee that pci_remove_bus_device() will never be
> > called in parallel with this helper, then I agree, but can we
> > guarantee that?
>
> Hmm, yeah, I guess that's the question.  It's not a race with another
> thread changing aspm_enabled; the potential race is with another
> thread removing the last child of "bridge", which will free the
> link_state and set bridge->link_state = NULL.
>
> I think it should be safe to call device-related PCI interfaces if
> you're holding a reference to the device, e.g., from a driver bound to
> the device or a sysfs accessor.  Since we call pcie_aspm_enabled(dev)
> from a driver bound to "dev", another thread should not be able to
> remove "dev" while we're using it.
>
> I know that's a little hand-wavey, but if it weren't true, I think
> we'd have a lot more locking sprinkled everywhere in the PCI core than
> we do.
>
> This has implications for Heiner's ASPM sysfs patches because we're
> currently doing this in sysfs accessors:
>
>   static ssize_t aspm_attr_show_common(struct device *dev, ...)
>   {
>     ...
>     link = pcie_aspm_get_link(pdev);
>
>     mutex_lock(&aspm_lock);
>     enabled = link->aspm_enabled & state;
>     mutex_unlock(&aspm_lock);
>     ...
>   }
>
> I assume sysfs must be holding a reference that guarantees "dev" is
> valid througout this code, and therefore we should not need to hold
> aspm_lock.

In principle, pcie_aspm_enabled() need not be called via sysfs.

In the particular NVMe use case, it is called from the driver's own PM
callback, so it would be safe without the locking AFAICS.

I guess it is safe to drop the locking from there, but then it would
be good to mention in the kerneldoc that calling it is only safe under
the assumption that the link_state object cannot go away while it is
running.

  reply	other threads:[~2019-10-08 22:54 UTC|newest]

Thread overview: 75+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-07-25  9:51 [Regression] Commit "nvme/pci: Use host managed power state for suspend" has problems Rafael J. Wysocki
2019-07-25 14:02 ` Kai-Heng Feng
2019-07-25 16:23   ` Mario.Limonciello
2019-07-25 17:03     ` Rafael J. Wysocki
2019-07-25 17:23       ` Mario.Limonciello
2019-07-25 18:20       ` Kai-Heng Feng
2019-07-25 19:09         ` Mario.Limonciello
2019-07-30 10:45       ` Rafael J. Wysocki
2019-07-30 14:41         ` Keith Busch
2019-07-30 17:14           ` Mario.Limonciello
2019-07-30 18:50             ` Kai-Heng Feng
2019-07-30 19:19               ` Keith Busch
2019-07-30 21:05                 ` Mario.Limonciello
2019-07-30 21:31                   ` Keith Busch
2019-07-31 21:25                     ` Rafael J. Wysocki
2019-07-31 22:19                       ` Keith Busch
2019-07-31 22:33                         ` Rafael J. Wysocki
2019-08-01  9:05                           ` Kai-Heng Feng
2019-08-01 17:29                             ` Rafael J. Wysocki
2019-08-01 19:05                               ` Mario.Limonciello
2019-08-01 22:26                                 ` Rafael J. Wysocki
2019-08-02 10:55                                   ` Kai-Heng Feng
2019-08-02 11:04                                     ` Rafael J. Wysocki
2019-08-05 19:13                                       ` Kai-Heng Feng
2019-08-05 21:28                                         ` Rafael J. Wysocki
2019-08-06 14:02                                           ` Mario.Limonciello
2019-08-06 15:00                                             ` Rafael J. Wysocki
2019-08-07 10:29                                               ` Rafael J. Wysocki
2019-08-01 20:22                             ` Keith Busch
2019-08-07  9:48                         ` Rafael J. Wysocki
2019-08-07 10:45                           ` Christoph Hellwig
2019-08-07 10:54                             ` Rafael J. Wysocki
2019-08-07  9:53                         ` [PATCH] nvme-pci: Do not prevent PCI bus-level PM from being used Rafael J. Wysocki
2019-08-07 10:14                           ` Rafael J. Wysocki
2019-08-07 10:43                           ` Christoph Hellwig
2019-08-07 14:37                           ` Keith Busch
2019-08-08  8:36                         ` [PATCH] nvme-pci: Allow PCI bus-level PM to be used if ASPM is disabled Rafael J. Wysocki
2019-08-08  8:48                           ` Christoph Hellwig
2019-08-08  9:06                             ` Rafael J. Wysocki
2019-08-08 10:03                         ` [PATCH v2 0/2] " Rafael J. Wysocki
2019-08-08 10:06                           ` [PATCH v2 1/2] PCI: PCIe: ASPM: Introduce pcie_aspm_enabled_mask() Rafael J. Wysocki
2019-08-08 13:15                             ` Bjorn Helgaas
2019-08-08 14:48                               ` Rafael J. Wysocki
2019-08-08 10:10                           ` [PATCH v2 2/2] nvme-pci: Allow PCI bus-level PM to be used if ASPM is disabled Rafael J. Wysocki
2019-08-08 13:43                             ` Bjorn Helgaas
2019-08-08 14:47                               ` Rafael J. Wysocki
2019-08-08 17:06                                 ` Rafael J. Wysocki
2019-08-08 18:39                                 ` Bjorn Helgaas
2019-08-08 20:01                                   ` Keith Busch
2019-08-08 20:05                                   ` Mario.Limonciello
2019-08-08 20:41                                   ` Rafael J. Wysocki
2019-08-09  4:47                                     ` Bjorn Helgaas
2019-08-09  8:04                                       ` Rafael J. Wysocki
2019-08-08 21:51                         ` [PATCH v3 0/2] " Rafael J. Wysocki
2019-08-08 21:55                           ` [PATCH v3 1/2] PCI: PCIe: ASPM: Introduce pcie_aspm_enabled() Rafael J. Wysocki
2019-08-09  4:50                             ` Bjorn Helgaas
2019-08-09  8:00                               ` Rafael J. Wysocki
2019-10-07 22:34                             ` Bjorn Helgaas
2019-10-08  9:27                               ` Rafael J. Wysocki
2019-10-08 21:16                                 ` Bjorn Helgaas
2019-10-08 22:54                                   ` Rafael J. Wysocki [this message]
2019-10-09 12:49                                     ` Bjorn Helgaas
2019-08-08 21:58                           ` [PATCH v3 2/2] nvme-pci: Allow PCI bus-level PM to be used if ASPM is disabled Rafael J. Wysocki
2019-08-08 22:13                           ` [PATCH v3 0/2] " Keith Busch
2019-08-09  8:05                             ` Rafael J. Wysocki
2019-08-09 14:52                               ` Keith Busch
2019-07-25 16:59   ` [Regression] Commit "nvme/pci: Use host managed power state for suspend" has problems Rafael J. Wysocki
2019-07-25 14:52 ` Keith Busch
2019-07-25 19:48   ` Rafael J. Wysocki
2019-07-25 19:52     ` Keith Busch
2019-07-25 20:02       ` Rafael J. Wysocki
2019-07-26 14:02         ` Kai-Heng Feng
2019-07-27 12:55           ` Rafael J. Wysocki
2019-07-29 15:51             ` Mario.Limonciello
2019-07-29 22:05               ` 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=CAJZ5v0j-uSM2gheHViommWcrSaLVfzbwV3M7OOWz3GyzhHSwRA@mail.gmail.com \
    --to=rafael@kernel.org \
    --cc=Mario.Limonciello@dell.com \
    --cc=hch@lst.de \
    --cc=helgaas@kernel.org \
    --cc=hkallweit1@gmail.com \
    --cc=kai.heng.feng@canonical.com \
    --cc=kbusch@kernel.org \
    --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 \
    --subject='Re: [PATCH v3 1/2] PCI: PCIe: ASPM: Introduce pcie_aspm_enabled()' \
    /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

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).