All of lore.kernel.org
 help / color / mirror / Atom feed
From: Bjorn Helgaas <bhelgaas@google.com>
To: Jiang Liu <jiang.liu@huawei.com>
Cc: "Rafael J. Wysocki" <rjw@sisk.pl>,
	Yinghai Lu <yinghai@kernel.org>,
	Kenji Kaneshige <kaneshige.kenji@jp.fujitsu.com>,
	Taku Izumi <izumi.taku@jp.fujitsu.com>,
	Don Dutile <ddutile@redhat.com>,
	Yijing Wang <wangyijing@huawei.com>,
	Keping Chen <chenkeping@huawei.com>,
	linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org,
	Jiang Liu <liuj97@gmail.com>
Subject: Re: [Resend with Ack][PATCH v1] PCI: allow acpiphp to handle PCIe ports without native PCIe hotplug capability
Date: Mon, 2 Jul 2012 22:16:58 -0600	[thread overview]
Message-ID: <CAErSpo7NiCsuM+UxveZbYxNsCiStMH5SLZ8EdFzJ7EduB306OQ@mail.gmail.com> (raw)
In-Reply-To: <1338795894-6292-1-git-send-email-jiang.liu@huawei.com>

On Mon, Jun 4, 2012 at 1:44 AM, Jiang Liu <jiang.liu@huawei.com> wrote:
> Commit 0d52f54e2ef64c189dedc332e680b2eb4a34590a (PCI / ACPI: Make acpiphp
> ignore root bridges using PCIe native hotplug) added code that made the
> acpiphp driver completely ignore PCIe root complexes for which the kernel
> had been granted control of the native PCIe hotplug feature by the BIOS
> through _OSC. Later commit 619a5182d1f38a3d629ee48e04fa182ef9170052
> "PCI hotplug: Always allow acpiphp to handle non-PCIe bridges" relaxed
> the constraints to allow acpiphp driver handle non-PCIe bridges under
> such a complex. The constraint needs to be relaxed further to allow
> acpiphp driver to hanlde PCIe ports without native PCIe hotplug capability.
>
> Some MR-IOV switch chipsets, such PLX8696, support multiple virtual PCIe
> switches and may migrate downstream ports among virtual switches.
> To migrate a downstream port from the source virtual switch to the target,
> the port needs to be hot-removed from the source and hot-added into the
> target. pciehp driver can't be used here because there's no slots within
> the virtual PCIe switch. So acpiphp driver is used to support downstream
> port migration. A typical configuration is as below:
> [Root w/o native PCIe HP]
>         [Upstream port of vswitch w/o native PCIe HP]
>                 [Downstream port of vswitch w/ native PCIe HP]
>                         [PCIe enpoint]
>
> Here acpiphp driver will be used to handle root ports and upstream port
> in the virtual switch, and pciehp driver will be used to handle downstream
> ports in the virtual switch.
>
> Acked-by: Rafael J. Wysocki <rjw@sisk.pl>
> Signed-off-by: Jiang Liu <liuj97@gmail.com>
>
> ---
>  drivers/pci/hotplug/acpiphp_glue.c |   49 ++++++++++++++++++++++++++++-------
>  1 files changed, 39 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
> index 806c44f..4889448 100644
> --- a/drivers/pci/hotplug/acpiphp_glue.c
> +++ b/drivers/pci/hotplug/acpiphp_glue.c
> @@ -115,6 +115,43 @@ static const struct acpi_dock_ops acpiphp_dock_ops = {
>         .handler = handle_hotplug_event_func,
>  };
>
> +/* Check whether device is managed by native PCIe hotplug driver */
> +static bool device_is_managed_by_native_pciehp(struct pci_dev *pdev)
> +{
> +       int pos;
> +       u16 reg16;
> +       u32 reg32;
> +       acpi_handle tmp;
> +       struct acpi_pci_root *root;
> +
> +       if (!pci_is_pcie(pdev))
> +               return false;
> +
> +       /* Check whether PCIe port supports native PCIe hotplug */
> +       pos = pci_pcie_cap(pdev);

Add "if (!pos) return false;" here and you can drop the "if
(!pci_is_pcie())" test above.

> +       pci_read_config_word(pdev, pos + PCI_EXP_FLAGS, &reg16);
> +       if (!(reg16 & PCI_EXP_FLAGS_SLOT))

I think this is unsafe.  Per the PCIe v3.0 spec, sec 7.8.2 on p648,
the "Slot Implemented" bit is undefined except for Downstream Ports,
so we're using an undefined bit to decide whether to read
PCI_EXP_SLTCAP.

If the device has a v1 PCIe Capability, it is not required to even
implement PCI_EXP_SLTCAP, so we could be reading garbage out of an
unrelated capability.  This is in sec 7.8, p363, of the v1.1 PCIe
spec.  I think v3.0 of the spec is dangerously incomplete because it
doesn't include enough information to handle the v1 PCIe Capability
correctly.

There's a fair amount of work to fix this.  I started doing it, but
decided I didn't have time to complete it.  Here's what I think we
(and by "we," I'm afraid I mean "you" :)) should do:

  - Add a "u16 pcie_flags" field in struct pci_dev and save the "PCI
Express Capabilities Register" there in set_pcie_port_type().  All
fields in that register are read-only, so it should be safe to cache
it.
  - Remove pcie_type from struct pci_dev and replace it with a
pcie_type() inline that extracts it from pcie_flags.
  - Rework the pcie_cap_has_*() macros in drivers/pci/pci.c to take a
struct pci_dev * and use pcie_flags instead of type and flags.  This
will remove the need for callers to read the flags themselves.
  - Move the pcie_cap_has_*() macros to include/linux/pci_reg.h so
they can be shared.
  - Audit all uses of the Link registers (PCI_EXP_LNKCAP,
PCI_EXP_LNKCTL, PCI_EXP_LNKSTA), Slot registers (PCI_EXP_SLTCAP,
PCI_EXP_SLTCTL, PCI_EXP_SLTSTA), and Root registers (PCI_EXP_RTCAP,
PCI_EXP_RTCTL, PCI_EXP_RTSTA) to make sure the register exists, either
by using pcie_cap_has_*() or some other knowledge of the device.

> +               return false;
> +       pci_read_config_dword(pdev, pos + PCI_EXP_SLTCAP, &reg32);
> +       if (!(reg32 & PCI_EXP_SLTCAP_HPC))
> +               return false;
> +
> +       /*
> +        * Check whether native PCIe hotplug has been enabled for
> +        * this PCIe hierarchy.
> +        */
> +       tmp = acpi_find_root_bridge_handle(pdev);
> +       if (!tmp)
> +               return false;
> +       root = acpi_pci_find_root(tmp);
> +       if (!root)
> +               return false;
> +       if (!(root->osc_control_set & OSC_PCI_EXPRESS_NATIVE_HP_CONTROL))
> +               return false;
> +
> +       return true;
> +}
> +
>  /* callback routine to register each ACPI PCI slot object */
>  static acpi_status
>  register_slot(acpi_handle handle, u32 lvl, void *context, void **rv)
> @@ -133,16 +170,8 @@ register_slot(acpi_handle handle, u32 lvl, void *context, void **rv)
>                 return AE_OK;
>
>         pdev = pbus->self;
> -       if (pdev && pci_is_pcie(pdev)) {
> -               tmp = acpi_find_root_bridge_handle(pdev);
> -               if (tmp) {
> -                       struct acpi_pci_root *root = acpi_pci_find_root(tmp);
> -
> -                       if (root && (root->osc_control_set &
> -                                       OSC_PCI_EXPRESS_NATIVE_HP_CONTROL))
> -                               return AE_OK;
> -               }
> -       }
> +       if (pdev && device_is_managed_by_native_pciehp(pdev))
> +               return AE_OK;
>
>         acpi_evaluate_integer(handle, "_ADR", NULL, &adr);
>         device = (adr >> 16) & 0xffff;
> --
> 1.7.1
>
>

  parent reply	other threads:[~2012-07-03  4:17 UTC|newest]

Thread overview: 48+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2012-06-04  7:44 [Resend with Ack][PATCH v1] PCI: allow acpiphp to handle PCIe ports without native PCIe hotplug capability Jiang Liu
2012-06-04  8:23 ` Kenji Kaneshige
2012-07-03  4:16 ` Bjorn Helgaas [this message]
2012-07-03 15:59   ` Bjorn Helgaas
2012-07-03 19:50     ` Don Dutile
2012-07-04 18:07       ` Bjorn Helgaas
2012-07-09 10:05         ` Jiang Liu
2012-07-09 17:05           ` Bjorn Helgaas
2012-07-04  2:52     ` Jiang Liu
2012-07-10 15:54     ` [RFC PATCH 00/14] improve PCIe capabilities registers handling Jiang Liu
2012-07-10 18:44       ` Bjorn Helgaas
2012-07-10 15:54     ` [RFC PATCH 01/14] PCI: add pcie_flags into struct pci_dev to cache PCIe capabilities register Jiang Liu
2012-07-11  9:01       ` Taku Izumi
2012-07-11 14:27         ` Jiang Liu
2012-07-10 15:54     ` [RFC PATCH 02/14] PCI: introduce pci_pcie_type(dev) to replace pci_dev->pcie_type Jiang Liu
2012-07-10 15:54     ` [RFC PATCH 03/14] PCI: remove unused field pcie_type from struct pci_dev Jiang Liu
2012-07-10 15:54     ` [RFC PATCH 04/14] PCI: refine and move pcie_cap_has_*() macros to include/linux/pci.h Jiang Liu
2012-07-10 18:49       ` Bjorn Helgaas
2012-07-10 15:54     ` [RFC PATCH 05/14] PCI: add access functions for PCIe capabilities to hide PCIe spec differences Jiang Liu
2012-07-10 18:35       ` Bjorn Helgaas
2012-07-11  3:07         ` Jiang Liu
2012-07-11  3:40           ` Bjorn Helgaas
2012-07-11  6:40             ` Jiang Liu
2012-07-11 17:52               ` Bjorn Helgaas
2012-07-12  2:56                 ` Jiang Liu
2012-07-12 20:49                   ` Bjorn Helgaas
2012-07-15 16:47                     ` Jiang Liu
2012-07-16 17:29                       ` Bjorn Helgaas
2012-07-16 18:57                         ` Don Dutile
2012-07-17  0:09                         ` Jiang Liu
2012-07-17  0:14                           ` Bjorn Helgaas
2012-07-10 15:54     ` [RFC PATCH 06/14] PCI: use PCIe cap access functions to simplify PCI core implementation Jiang Liu
2012-07-10 18:35       ` Bjorn Helgaas
2012-07-11  2:49         ` Jiang Liu
2012-07-10 15:54     ` [RFC PATCH 07/14] hotplug/PCI: use PCIe cap access functions to simplify implementation Jiang Liu
2012-07-10 18:35       ` Bjorn Helgaas
2012-07-10 15:54     ` [RFC PATCH 08/14] portdrv/PCI: " Jiang Liu
2012-07-10 15:54     ` [RFC PATCH 09/14] pciehp/PCI: " Jiang Liu
2012-07-10 15:54     ` [RFC PATCH 10/14] PME/PCI: " Jiang Liu
2012-07-10 15:54     ` [RFC PATCH 11/14] AER/PCI: " Jiang Liu
2012-07-10 15:54     ` [RFC PATCH 12/14] ASPM/PCI: " Jiang Liu
2012-07-10 15:54     ` [RFC PATCH 13/14] r8169/PCI: " Jiang Liu
2012-07-10 15:54     ` [RFC PATCH 14/14] qib/PCI: " Jiang Liu
2012-08-15 19:12 ` [Resend with Ack][PATCH v1] PCI: allow acpiphp to handle PCIe ports without native PCIe hotplug capability Bjorn Helgaas
2012-08-16 15:15   ` Jiang Liu
2012-08-22 15:16   ` [PATCH v2] PCI: allow acpiphp to handle PCIe ports w/o " Jiang Liu
2012-09-24 22:10     ` Bjorn Helgaas
2012-09-25 15:16       ` Jiang Liu

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=CAErSpo7NiCsuM+UxveZbYxNsCiStMH5SLZ8EdFzJ7EduB306OQ@mail.gmail.com \
    --to=bhelgaas@google.com \
    --cc=chenkeping@huawei.com \
    --cc=ddutile@redhat.com \
    --cc=izumi.taku@jp.fujitsu.com \
    --cc=jiang.liu@huawei.com \
    --cc=kaneshige.kenji@jp.fujitsu.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pci@vger.kernel.org \
    --cc=liuj97@gmail.com \
    --cc=rjw@sisk.pl \
    --cc=wangyijing@huawei.com \
    --cc=yinghai@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
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.