Linux-NVME Archive on lore.kernel.org
 help / color / Atom feed
From: "Liang, Prike" <Prike.Liang@amd.com>
To: Keith Busch <kbusch@kernel.org>
Cc: Christoph Hellwig <hch@infradead.org>,
	"linux-nvme@lists.infradead.org" <linux-nvme@lists.infradead.org>,
	"Chaitanya.Kulkarni@wdc.com" <Chaitanya.Kulkarni@wdc.com>,
	"gregkh@linuxfoundation.org" <gregkh@linuxfoundation.org>,
	"stable@vger.kernel.org" <stable@vger.kernel.org>,
	"Deucher, Alexander" <Alexander.Deucher@amd.com>,
	"S-k, Shyam-sundar" <Shyam-sundar.S-k@amd.com>
Subject: RE: [PATCH v4 1/2] PCI: add AMD PCIe quirk for nvme shutdown opt
Date: Fri, 16 Apr 2021 06:51:06 +0000
Message-ID: <BYAPR12MB3238528F1D91A921FF1201EAFB4C9@BYAPR12MB3238.namprd12.prod.outlook.com> (raw)
In-Reply-To: <20210415222544.GA2760247@dhcp-10-100-145-180.wdc.com>

[AMD Public Use]

> From: Keith Busch <kbusch@kernel.org>
> Sent: Friday, April 16, 2021 6:26 AM
> To: Liang, Prike <Prike.Liang@amd.com>
> Cc: Christoph Hellwig <hch@infradead.org>; linux-nvme@lists.infradead.org;
> Chaitanya.Kulkarni@wdc.com; gregkh@linuxfoundation.org;
> stable@vger.kernel.org; Deucher, Alexander
> <Alexander.Deucher@amd.com>; S-k, Shyam-sundar <Shyam-sundar.S-
> k@amd.com>
> Subject: Re: [PATCH v4 1/2] PCI: add AMD PCIe quirk for nvme shutdown opt
>
> On Thu, Apr 15, 2021 at 09:41:53AM +0000, Liang, Prike wrote:
> > > From: Christoph Hellwig <hch@infradead.org>
> >
> > > I'd also much prefer if the flag is used on every pci_dev that is
> > > affected by the broken behavior rather than requiring another lookup in
> the driver.
> > Sorry can't get the meaning, could you give more detail how to implement
> this?
>
> The suggestion is child devices of the pci bus inherit the quirk so drivers don't
> need to look up the parent device that requires it.
>
> That makes sense for a couple reasons. For one, your hard-coded 0:0.0
> probably aligns to actual implementations, but I did't find a spec requirement
> that the host bridge occupy that BDf, so not having to look up a fixed location
> is more flexible.
>
> If I understand the suggestion correctly, I think it's probably easier to thread
> the quirk through the pci_bus->bus_flags. Does the below
> (untested) make sense?
>
Thanks Busch for elaborate clarification. The PCIe RC bus flag can pass to NVMe device successfully and it works well for this case. I will update the patch accordingly.
> ---
> diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c index
> d47bb18b976a..022ff6cf202f 100644
> --- a/drivers/nvme/host/pci.c
> +++ b/drivers/nvme/host/pci.c
> @@ -2834,6 +2834,9 @@ static bool nvme_acpi_storage_d3(struct pci_dev
> *dev)
>  acpi_status status;
>  u8 val;
>
> +if (dev->bus->bus_flags & PCI_BUS_FLAGS_DISABLE_ON_S2I)
> +return true;
> +
>  /*
>   * Look for _DSD property specifying that the storage device on the
> port
>   * must use D3 to support deep platform power savings during diff --
> git a/drivers/pci/probe.c b/drivers/pci/probe.c index
> 953f15abc850..34ba691ec545 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -558,10 +558,13 @@ static struct pci_bus *pci_alloc_bus(struct pci_bus
> *parent)
>  INIT_LIST_HEAD(&b->resources);
>  b->max_bus_speed = PCI_SPEED_UNKNOWN;
>  b->cur_bus_speed = PCI_SPEED_UNKNOWN;
> +if (parent) {
>  #ifdef CONFIG_PCI_DOMAINS_GENERIC
> -if (parent)
>  b->domain_nr = parent->domain_nr;
>  #endif
> +if (parent->bus_flags & PCI_BUS_FLAGS_DISABLE_ON_S2I)
> +b->bus_flags |= PCI_BUS_FLAGS_DISABLE_ON_S2I;
> +}
>  return b;
>  }
>
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c index
> 653660e3ba9e..e8f74661138a 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -312,6 +312,14 @@ static void quirk_nopciamd(struct pci_dev *dev)  }
>  DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_AMD,
> PCI_DEVICE_ID_AMD_8151_0,quirk_nopciamd);
>
> +static void quirk_amd_s2i_fixup(struct pci_dev *dev) {
> +dev->bus->bus_flags |= PCI_BUS_FLAGS_DISABLE_ON_S2I;
> +pci_info(dev, "AMD simple suspend opt enabled\n");
> +
> +}
> +DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_AMD, 0x1630,
> +quirk_amd_s2i_fixup);
> +
>  /* Triton requires workarounds to be used by the drivers */  static void
> quirk_triton(struct pci_dev *dev)  { diff --git a/include/linux/pci.h
> b/include/linux/pci.h index 86c799c97b77..7072e2ec88a2 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -240,6 +240,8 @@ enum pci_bus_flags {
>  PCI_BUS_FLAGS_NO_MMRBC= (__force pci_bus_flags_t) 2,
>  PCI_BUS_FLAGS_NO_AERSID= (__force pci_bus_flags_t) 4,
>  PCI_BUS_FLAGS_NO_EXTCFG= (__force pci_bus_flags_t) 8,
> +/* Driver must pci_disable_device() for suspend-to-idle */
> +PCI_BUS_FLAGS_DISABLE_ON_S2I= (__force pci_bus_flags_t) 16,
>  };
>
>  /* Values from Link Status register, PCIe r3.1, sec 7.8.8 */
> --

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

  reply index

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-04-15  3:52 Prike Liang
2021-04-15  3:52 ` [PATCH v4 2/2] nvme-pci: add AMD PCIe quirk for suspend/resume Prike Liang
2021-04-15  6:29   ` Greg KH
2021-04-15  7:39     ` Liang, Prike
2021-04-15  8:20 ` [PATCH v4 1/2] PCI: add AMD PCIe quirk for nvme shutdown opt Christoph Hellwig
2021-04-15  9:41   ` Liang, Prike
2021-04-15 22:25     ` Keith Busch
2021-04-16  6:51       ` Liang, Prike [this message]
2021-04-30 17:50 ` Bjorn Helgaas
2021-05-03  7:14   ` Christoph Hellwig
2021-05-03 14:57     ` Keith Busch
2021-05-03 15:50       ` Bjorn Helgaas
2021-05-06  3:22         ` Liang, Prike

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=BYAPR12MB3238528F1D91A921FF1201EAFB4C9@BYAPR12MB3238.namprd12.prod.outlook.com \
    --to=prike.liang@amd.com \
    --cc=Alexander.Deucher@amd.com \
    --cc=Chaitanya.Kulkarni@wdc.com \
    --cc=Shyam-sundar.S-k@amd.com \
    --cc=gregkh@linuxfoundation.org \
    --cc=hch@infradead.org \
    --cc=kbusch@kernel.org \
    --cc=linux-nvme@lists.infradead.org \
    --cc=stable@vger.kernel.org \
    /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

Linux-NVME Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-nvme/0 linux-nvme/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-nvme linux-nvme/ https://lore.kernel.org/linux-nvme \
		linux-nvme@lists.infradead.org
	public-inbox-index linux-nvme

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.infradead.lists.linux-nvme


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git