diff mbox

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

Message ID 1338795894-6292-1-git-send-email-jiang.liu@huawei.com
State Superseded
Headers show

Commit Message

Jiang Liu June 4, 2012, 7:44 a.m. UTC 
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(-)

Comments

Kenji Kaneshige June 4, 2012, 8:23 a.m. UTC | #1 
Reviewed-by: Kenji Kaneshige <kaneshige.kenji@jp.fujitsu.com>
Acked-by: Kenji Kaneshige <kaneshige.kenji@jp.fujitsu.com>

Regards,
Kenji Kaneshige


(2012/06/04 16:44), Jiang Liu 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);
> +	pci_read_config_word(pdev, pos + PCI_EXP_FLAGS,&reg16);
> +	if (!(reg16&  PCI_EXP_FLAGS_SLOT))
> +		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;

--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bjorn Helgaas July 3, 2012, 4:16 a.m. UTC | #2 
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
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bjorn Helgaas July 3, 2012, 3:59 p.m. UTC | #3 
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?
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Don Dutile July 3, 2012, 7:50 p.m. UTC | #4 
On 07/03/2012 11:59 AM, Bjorn Helgaas wrote:
> 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);
>

I like your thinking!

> 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;
>
may want to change read value to 0 just in case callers are doing rtn value
check and just value-read mask & go.  I believe for all the optional/version'd
registers below, non-existent regs are required to be rtn-zero if not implemented.

>        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?

only one is that 'cap' is overused in PCI space, just like 'domain' in
various kernel subsystems.  cap could be 'cap list structure'
or a specific 'capability'.  I wish we had a better TLA for 'cap' and what
it refers to. ... but that's my pet peeve...

> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jiang Liu July 4, 2012, 2:52 a.m. UTC | #5 
Sure, let's try it.

On 2012-7-3 23:59, Bjorn Helgaas wrote:
> 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?
> 
> .
> 


--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bjorn Helgaas July 4, 2012, 6:07 p.m. UTC | #6 
On Tue, Jul 3, 2012 at 1:50 PM, Don Dutile <ddutile@redhat.com> wrote:
> On 07/03/2012 11:59 AM, Bjorn Helgaas wrote:
>>
>> 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);
>>
>
> I like your thinking!
>
>
>> 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;
>>
> may want to change read value to 0 just in case callers are doing rtn value
> check and just value-read mask & go.  I believe for all the
> optional/version'd
> registers below, non-existent regs are required to be rtn-zero if not
> implemented.

Generally I prefer that if a function returns failure, it doesn't
modify the parameters passed by reference, but in this case, I think
you're right that we should set *val to zero to begin with.  It will
simplify the following code somewhat, too.

Note that most non-implemented registers should read as zero, but Slot
Status of Downstream Ports is an exception (spec v3.0, sec 7.8, line
25).

>>        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?
>
>
> only one is that 'cap' is overused in PCI space, just like 'domain' in
> various kernel subsystems.  cap could be 'cap list structure'
> or a specific 'capability'.  I wish we had a better TLA for 'cap' and what
> it refers to. ... but that's my pet peeve...

I agree, 'cap' is overused and confusing.  Even "PCI Express
Capability Structure" as used in the spec seems slightly confusing to
me.  The existing pci_find_capability() interfaces spell out 'cap';
maybe we should, too.  Here are some possibilities, starting with my
current favorite:

  pci_pcie_capability_read_word()
  pci_pcie_cap_read_word()  (extension of existing pci_pcie_cap() idea)
  pci_express_cap_read_word()
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jiang Liu July 9, 2012, 10:05 a.m. UTC | #7 
Hi Bjorn and Yinghai,
	What's the policy to export a symbol by EXPORT_SYMBOL()
or EXPORT_SYMBOL_GPL()? I know the legal difference, but don't
know when I should mark a symbol as GPL.
	Thanks!


--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bjorn Helgaas July 9, 2012, 5:05 p.m. UTC | #8 
On Mon, Jul 9, 2012 at 4:05 AM, Jiang Liu <jiang.liu@huawei.com> wrote:
> Hi Bjorn and Yinghai,
>         What's the policy to export a symbol by EXPORT_SYMBOL()
> or EXPORT_SYMBOL_GPL()? I know the legal difference, but don't
> know when I should mark a symbol as GPL.

From Documentation/DocBook/kernel-hacking.tmpl,

    EXPORT_SYMBOL_GPL implies that the function is considered
    an internal implementation issue, and not really an interface.

So I use EXPORT_SYMBOL_GPL unless I'm willing to support the symbol as
an interface indefinitely.

Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jiang Liu July 10, 2012, 3:54 p.m. UTC | #9 
From: Jiang Liu <liuj97@gmail.com>

As suggested by Bjorn Helgaas and Don Dutile in threads
http://www.spinics.net/lists/linux-pci/msg15663.html, we could improve access
to PCIe capabilities register in to way:
1) cache content of PCIe Capabilities Register into struct pce_dev to avoid
   repeatedly reading this register because it's read only.
2) provide access functions for PCIe Capabilities registers to hide differences
   among PCIe base specifications, so the caller don't need to handle those
   differences.

This patch set applies to
git://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git pci-next

These patch set is still RFC. It provides the new interfaces and has made the
major changes to adopt those new interfaces. But there are still several device
drivers left untouched. Any comments about the new interfaces are welcomed,
especially about function names:). Once we reach an agreement, I will send out
a formal version with all needed work done.

Jiang Liu (11):
  PCI: refine and move pcie_cap_has_*() macros to include/linux/pci.h
  PCI: add access functions for PCIe capabilities to hide PCIe spec
    differences
  PCI: use PCIe cap access functions to simplify PCI core
    implementation
  hotplug/PCI: use PCIe cap access functions to simplify implementation
  portdrv/PCI: use PCIe cap access functions to simplify implementation
  pciehp/PCI: use PCIe cap access functions to simplify implementation
  PME/PCI: use PCIe cap access functions to simplify implementation
  AER/PCI: use PCIe cap access functions to simplify implementation
  ASPM/PCI: use PCIe cap access functions to simplify implementation
  r8169/PCI: use PCIe cap access functions to simplify implementation
  qib/PCI: use PCIe cap access functions to simplify implementation

Yijing Wang (1):
  PCI: add pcie_flags into struct pci_dev to cache PCIe capabilities
    register
  PCI: introduce pci_pcie_type(dev) to replace pci_dev->pcie_type
  PCI: remove unused field pcie_type from struct pci_dev

 arch/powerpc/platforms/powernv/pci-ioda.c          |    2 +-
 drivers/infiniband/hw/qib/qib_pcie.c               |   34 ++-
 drivers/iommu/intel-iommu.c                        |    6 +-
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c      |    2 +-
 .../net/ethernet/qlogic/netxen/netxen_nic_main.c   |    2 +-
 drivers/net/ethernet/realtek/r8169.c               |   38 +--
 drivers/pci/access.c                               |   88 +++++++
 drivers/pci/hotplug/pciehp_acpi.c                  |    5 +-
 drivers/pci/hotplug/pciehp_hpc.c                   |    8 +-
 drivers/pci/hotplug/pcihp_slot.c                   |   17 +-
 drivers/pci/iov.c                                  |    6 +-
 drivers/pci/pci.c                                  |  265 +++++---------------
 drivers/pci/pcie/aer/aer_inject.c                  |    2 +-
 drivers/pci/pcie/aer/aerdrv.c                      |   23 +-
 drivers/pci/pcie/aer/aerdrv_acpi.c                 |    2 +-
 drivers/pci/pcie/aer/aerdrv_core.c                 |   47 ++--
 drivers/pci/pcie/aspm.c                            |  110 ++++----
 drivers/pci/pcie/pme.c                             |   23 +-
 drivers/pci/pcie/portdrv_bus.c                     |    2 +-
 drivers/pci/pcie/portdrv_core.c                    |   24 +-
 drivers/pci/pcie/portdrv_pci.c                     |   15 +-
 drivers/pci/probe.c                                |   30 +--
 drivers/pci/quirks.c                               |    8 +-
 drivers/pci/search.c                               |    2 +-
 include/linux/pci.h                                |   65 ++++-
 include/linux/pci_regs.h                           |   19 +-
 26 files changed, 401 insertions(+), 444 deletions(-)
Bjorn Helgaas July 10, 2012, 6:44 p.m. UTC | #10 
On Tue, Jul 10, 2012 at 9:54 AM, Jiang Liu <liuj97@gmail.com> wrote:
> From: Jiang Liu <liuj97@gmail.com>
>
> As suggested by Bjorn Helgaas and Don Dutile in threads
> http://www.spinics.net/lists/linux-pci/msg15663.html, we could improve access
> to PCIe capabilities register in to way:
> 1) cache content of PCIe Capabilities Register into struct pce_dev to avoid
>    repeatedly reading this register because it's read only.
> 2) provide access functions for PCIe Capabilities registers to hide differences
>    among PCIe base specifications, so the caller don't need to handle those
>    differences.
>
> This patch set applies to
> git://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git pci-next
>
> These patch set is still RFC. It provides the new interfaces and has made the
> major changes to adopt those new interfaces. But there are still several device
> drivers left untouched. Any comments about the new interfaces are welcomed,
> especially about function names:). Once we reach an agreement, I will send out
> a formal version with all needed work done.
>
> Jiang Liu (11):
>   PCI: refine and move pcie_cap_has_*() macros to include/linux/pci.h
>   PCI: add access functions for PCIe capabilities to hide PCIe spec
>     differences
>   PCI: use PCIe cap access functions to simplify PCI core
>     implementation
>   hotplug/PCI: use PCIe cap access functions to simplify implementation
>   portdrv/PCI: use PCIe cap access functions to simplify implementation
>   pciehp/PCI: use PCIe cap access functions to simplify implementation
>   PME/PCI: use PCIe cap access functions to simplify implementation
>   AER/PCI: use PCIe cap access functions to simplify implementation
>   ASPM/PCI: use PCIe cap access functions to simplify implementation
>   r8169/PCI: use PCIe cap access functions to simplify implementation
>   qib/PCI: use PCIe cap access functions to simplify implementation
>
> Yijing Wang (1):
>   PCI: add pcie_flags into struct pci_dev to cache PCIe capabilities
>     register
>   PCI: introduce pci_pcie_type(dev) to replace pci_dev->pcie_type
>   PCI: remove unused field pcie_type from struct pci_dev
>
>  arch/powerpc/platforms/powernv/pci-ioda.c          |    2 +-
>  drivers/infiniband/hw/qib/qib_pcie.c               |   34 ++-
>  drivers/iommu/intel-iommu.c                        |    6 +-
>  drivers/net/ethernet/intel/ixgbe/ixgbe_main.c      |    2 +-
>  .../net/ethernet/qlogic/netxen/netxen_nic_main.c   |    2 +-
>  drivers/net/ethernet/realtek/r8169.c               |   38 +--
>  drivers/pci/access.c                               |   88 +++++++
>  drivers/pci/hotplug/pciehp_acpi.c                  |    5 +-
>  drivers/pci/hotplug/pciehp_hpc.c                   |    8 +-
>  drivers/pci/hotplug/pcihp_slot.c                   |   17 +-
>  drivers/pci/iov.c                                  |    6 +-
>  drivers/pci/pci.c                                  |  265 +++++---------------
>  drivers/pci/pcie/aer/aer_inject.c                  |    2 +-
>  drivers/pci/pcie/aer/aerdrv.c                      |   23 +-
>  drivers/pci/pcie/aer/aerdrv_acpi.c                 |    2 +-
>  drivers/pci/pcie/aer/aerdrv_core.c                 |   47 ++--
>  drivers/pci/pcie/aspm.c                            |  110 ++++----
>  drivers/pci/pcie/pme.c                             |   23 +-
>  drivers/pci/pcie/portdrv_bus.c                     |    2 +-
>  drivers/pci/pcie/portdrv_core.c                    |   24 +-
>  drivers/pci/pcie/portdrv_pci.c                     |   15 +-
>  drivers/pci/probe.c                                |   30 +--
>  drivers/pci/quirks.c                               |    8 +-
>  drivers/pci/search.c                               |    2 +-
>  include/linux/pci.h                                |   65 ++++-
>  include/linux/pci_regs.h                           |   19 +-
>  26 files changed, 401 insertions(+), 444 deletions(-)

Very nice; I like this a lot.
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bjorn Helgaas Aug. 15, 2012, 7:12 p.m. UTC | #11 
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.

Gerry, I assume you'll refresh and repost this after we get the PCIe
capability stuff squared away, so I'll ignore this patch for now.

> 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);
> +       pci_read_config_word(pdev, pos + PCI_EXP_FLAGS, &reg16);
> +       if (!(reg16 & PCI_EXP_FLAGS_SLOT))
> +               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
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jiang Liu Aug. 16, 2012, 3:15 p.m. UTC | #12 
On 08/16/2012 03:12 AM, Bjorn Helgaas 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.
> 
> Gerry, I assume you'll refresh and repost this after we get the PCIe
> capability stuff squared away, so I'll ignore this patch for now.
> 
Hi Bjorn,
	Please just ignore it and I will rework it after the PCIe capabilities
patches are done.
	Regards!
	Gerry

--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

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);
+	pci_read_config_word(pdev, pos + PCI_EXP_FLAGS, &reg16);
+	if (!(reg16 & PCI_EXP_FLAGS_SLOT))
+		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;