Re: [Resend with Ack][PATCH v1] PCI: allow acpiphp to handle PCIe ports without native PCIe hotplug capability

[Bjorn Helgaas <bhelgaas@google.com>]

On Mon, Jul 2, 2012 at 10:16 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> 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.

Thinking about this some more, this still leaves the callers
responsible for using pcie_cap_has_*(), which feels pretty
error-prone.

I wonder if it'd be worth adding interfaces like:

  pcie_cap_read_word(const struct pci_dev *, int where, u16 *val);
  pcie_cap_read_dword(const struct pci_dev *, int where, u32 *val);
  pcie_cap_write_word(const struct pci_dev *, int where, u16 val);
  pcie_cap_write_dword(const struct pci_dev *, int where, u32 val);

We might be able to encapsulate the v1/v2 differences inside these, e.g.,

  int pcie_cap_read_word(const struct pci_dev *dev, int where, u16 *val)
  {
      int pos;

      pos = pci_pcie_cap(dev);
      if (!pos)
          return -EINVAL;

      switch (where) {
      case PCI_EXP_FLAGS:
      case PCI_EXP_DEVCTL:
      case PCI_EXP_DEVSTA:
          return pci_read_config_word(dev, pos + where, val);
      case PCI_EXP_LNKCTL:
      case PCI_EXP_LNKSTA:
          if (pcie_cap_has_lnkctl(dev))
              return pci_read_config_word(dev, pos + where, val);
          else {
              *val = 0;
              return 0;
          }
      case PCI_EXP_SLTCTL:
      case PCI_EXP_SLTSTA:
          if (pcie_cap_has_sltctl(dev))
              return pci_read_config_word(dev, pos + where, val);
          else {
              *val = 0;
              if (where == PCI_EXP_SLTSTA && dev->pcie_type ==
PCI_EXP_TYPE_DOWNSTREAM)
                  *val = PCI_EXP_SLTSTA_PDS;
              return 0;
      ...
      };
      return -EINVAL;
  }

Any thoughts?