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

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

[Jiang Liu]

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;
-- 
1.7.1

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

[Kenji Kaneshige]

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;

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

[Bjorn Helgaas]

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

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

[Bjorn Helgaas]

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?

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

[Don Dutile]

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

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

[Jiang Liu]

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

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

[Bjorn Helgaas]

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()

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

[Jiang Liu]

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!

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

[Bjorn Helgaas]

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

[RFC PATCH 00/14] improve PCIe capabilities registers handling

[Jiang Liu]

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(-)

-- 
1.7.9.5

[RFC PATCH 01/14] PCI: add pcie_flags into struct pci_dev to cache PCIe capabilities register

[Jiang Liu]

From: Yijing Wang <wangyijing@huawei.com>

From: Yijing Wang <wangyijing@huawei.com>

Since PCI Express Capabilities Register is read only, cache its value
into struct pci_dev to avoid repeatedly calling pci_read_config_*().

Signed-off-by: Yijing Wang <wangyijing@huawei.com>
Signed-off-by: Jiang Liu <liuj97@gmail.com>
---
 drivers/pci/probe.c |    1 +
 include/linux/pci.h |    1 +
 2 files changed, 2 insertions(+)

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 6c143b4..65e82e3 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -929,6 +929,7 @@ void set_pcie_port_type(struct pci_dev *pdev)
 	pdev->is_pcie = 1;
 	pdev->pcie_cap = pos;
 	pci_read_config_word(pdev, pos + PCI_EXP_FLAGS, &reg16);
+	pdev->pcie_flags = reg16;
 	pdev->pcie_type = (reg16 & PCI_EXP_FLAGS_TYPE) >> 4;
 	pci_read_config_word(pdev, pos + PCI_EXP_DEVCAP, &reg16);
 	pdev->pcie_mpss = reg16 & PCI_EXP_DEVCAP_PAYLOAD;
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 5faa831..f4a7ad6 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -258,6 +258,7 @@ struct pci_dev {
 	u8		pcie_mpss:3;	/* PCI-E Max Payload Size Supported */
 	u8		rom_base_reg;	/* which config register controls the ROM */
 	u8		pin;  		/* which interrupt pin this device uses */
+	u16		pcie_flags;	/* cached PCI-E Capabilities Register */
 
 	struct pci_driver *driver;	/* which driver has allocated this device */
 	u64		dma_mask;	/* Mask of the bits of bus address this
-- 
1.7.9.5

[RFC PATCH 02/14] PCI: introduce pci_pcie_type(dev) to replace pci_dev->pcie_type

[Jiang Liu]

From: Yijing Wang <wangyijing@huawei.com>

Introduce an inline function pci_pcie_type(dev) to extract PCIe device type
from pci_dev->pcie_flags field, and prepare for removing pci_dev->pcie_type.

Signed-off-by: Yijing Wang <wangyijing@huawei.com>
Signed-off-by: Jiang Liu <liuj97@gmail.com>
---
 arch/powerpc/platforms/powernv/pci-ioda.c          |    2 +-
 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/pci/iov.c                                  |    6 +--
 drivers/pci/pci.c                                  |   26 ++++++------
 drivers/pci/pcie/aer/aer_inject.c                  |    2 +-
 drivers/pci/pcie/aer/aerdrv.c                      |    7 ++--
 drivers/pci/pcie/aer/aerdrv_acpi.c                 |    2 +-
 drivers/pci/pcie/aer/aerdrv_core.c                 |    2 +-
 drivers/pci/pcie/aspm.c                            |   42 ++++++++++----------
 drivers/pci/pcie/pme.c                             |    6 +--
 drivers/pci/pcie/portdrv_bus.c                     |    2 +-
 drivers/pci/pcie/portdrv_core.c                    |    4 +-
 drivers/pci/pcie/portdrv_pci.c                     |    8 ++--
 drivers/pci/probe.c                                |    9 +++--
 drivers/pci/search.c                               |    2 +-
 include/linux/pci.h                                |   10 +++++
 18 files changed, 77 insertions(+), 63 deletions(-)

diff --git a/arch/powerpc/platforms/powernv/pci-ioda.c b/arch/powerpc/platforms/powernv/pci-ioda.c
index 9cda6a1..b46e1da 100644
--- a/arch/powerpc/platforms/powernv/pci-ioda.c
+++ b/arch/powerpc/platforms/powernv/pci-ioda.c
@@ -855,7 +855,7 @@ static void __devinit pnv_ioda_setup_PEs(struct pci_bus *bus)
 		if (pe == NULL)
 			continue;
 		/* Leaving the PCIe domain ... single PE# */
-		if (dev->pcie_type == PCI_EXP_TYPE_PCI_BRIDGE)
+		if (pci_pcie_type(dev) == PCI_EXP_TYPE_PCI_BRIDGE)
 			pnv_ioda_setup_bus_PE(dev, pe);
 		else if (dev->subordinate)
 			pnv_ioda_setup_PEs(dev->subordinate);
diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
index 2fb7d15..8612271 100644
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -2350,7 +2350,7 @@ static int iommu_should_identity_map(struct pci_dev *pdev, int startup)
 			return 0;
 		if (pdev->class >> 8 == PCI_CLASS_BRIDGE_PCI)
 			return 0;
-	} else if (pdev->pcie_type == PCI_EXP_TYPE_PCI_BRIDGE)
+	} else if (pci_pcie_type(pdev) == PCI_EXP_TYPE_PCI_BRIDGE)
 		return 0;
 
 	/* 
@@ -3545,10 +3545,10 @@ found:
 		struct pci_dev *bridge = bus->self;
 
 		if (!bridge || !pci_is_pcie(bridge) ||
-		    bridge->pcie_type == PCI_EXP_TYPE_PCI_BRIDGE)
+		    pci_pcie_type(bridge) == PCI_EXP_TYPE_PCI_BRIDGE)
 			return 0;
 
-		if (bridge->pcie_type == PCI_EXP_TYPE_ROOT_PORT) {
+		if (pci_pcie_type(bridge) == PCI_EXP_TYPE_ROOT_PORT) {
 			for (i = 0; i < atsru->devices_cnt; i++)
 				if (atsru->devices[i] == bridge)
 					return 1;
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index bf20457..565772e 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -7470,7 +7470,7 @@ static pci_ers_result_t ixgbe_io_error_detected(struct pci_dev *pdev,
 		goto skip_bad_vf_detection;
 
 	bdev = pdev->bus->self;
-	while (bdev && (bdev->pcie_type != PCI_EXP_TYPE_ROOT_PORT))
+	while (bdev && (pci_pcie_type(bdev) != PCI_EXP_TYPE_ROOT_PORT))
 		bdev = bdev->bus->self;
 
 	if (!bdev)
diff --git a/drivers/net/ethernet/qlogic/netxen/netxen_nic_main.c b/drivers/net/ethernet/qlogic/netxen/netxen_nic_main.c
index 342b3a7..01d6141 100644
--- a/drivers/net/ethernet/qlogic/netxen/netxen_nic_main.c
+++ b/drivers/net/ethernet/qlogic/netxen/netxen_nic_main.c
@@ -1382,7 +1382,7 @@ static void netxen_mask_aer_correctable(struct netxen_adapter *adapter)
 		adapter->ahw.board_type != NETXEN_BRDTYPE_P3_10G_TP)
 		return;
 
-	if (root->pcie_type != PCI_EXP_TYPE_ROOT_PORT)
+	if (pci_pcie_type(root) != PCI_EXP_TYPE_ROOT_PORT)
 		return;
 
 	aer_pos = pci_find_ext_capability(root, PCI_EXT_CAP_ID_ERR);
diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
index 74bbaf8..aeccc91 100644
--- a/drivers/pci/iov.c
+++ b/drivers/pci/iov.c
@@ -433,8 +433,8 @@ static int sriov_init(struct pci_dev *dev, int pos)
 	struct resource *res;
 	struct pci_dev *pdev;
 
-	if (dev->pcie_type != PCI_EXP_TYPE_RC_END &&
-	    dev->pcie_type != PCI_EXP_TYPE_ENDPOINT)
+	if (pci_pcie_type(dev) != PCI_EXP_TYPE_RC_END &&
+	    pci_pcie_type(dev) != PCI_EXP_TYPE_ENDPOINT)
 		return -ENODEV;
 
 	pci_read_config_word(dev, pos + PCI_SRIOV_CTRL, &ctrl);
@@ -503,7 +503,7 @@ found:
 	iov->self = dev;
 	pci_read_config_dword(dev, pos + PCI_SRIOV_CAP, &iov->cap);
 	pci_read_config_byte(dev, pos + PCI_SRIOV_FUNC_LINK, &iov->link);
-	if (dev->pcie_type == PCI_EXP_TYPE_RC_END)
+	if (pci_pcie_type(dev) == PCI_EXP_TYPE_RC_END)
 		iov->link = PCI_DEVFN(PCI_SLOT(dev->devfn), iov->link);
 
 	if (pdev)
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index f3ea977..28eb55b 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -885,7 +885,7 @@ static struct pci_cap_saved_state *pci_find_saved_cap(
 
 static int pci_save_pcie_state(struct pci_dev *dev)
 {
-	int pos, i = 0;
+	int type, pos, i = 0;
 	struct pci_cap_saved_state *save_state;
 	u16 *cap;
 	u16 flags;
@@ -903,13 +903,14 @@ static int pci_save_pcie_state(struct pci_dev *dev)
 
 	pci_read_config_word(dev, pos + PCI_EXP_FLAGS, &flags);
 
-	if (pcie_cap_has_devctl(dev->pcie_type, flags))
+	type = pci_pcie_type(dev);
+	if (pcie_cap_has_devctl(type, flags))
 		pci_read_config_word(dev, pos + PCI_EXP_DEVCTL, &cap[i++]);
-	if (pcie_cap_has_lnkctl(dev->pcie_type, flags))
+	if (pcie_cap_has_lnkctl(type, flags))
 		pci_read_config_word(dev, pos + PCI_EXP_LNKCTL, &cap[i++]);
-	if (pcie_cap_has_sltctl(dev->pcie_type, flags))
+	if (pcie_cap_has_sltctl(type, flags))
 		pci_read_config_word(dev, pos + PCI_EXP_SLTCTL, &cap[i++]);
-	if (pcie_cap_has_rtctl(dev->pcie_type, flags))
+	if (pcie_cap_has_rtctl(type, flags))
 		pci_read_config_word(dev, pos + PCI_EXP_RTCTL, &cap[i++]);
 
 	pos = pci_pcie_cap2(dev);
@@ -924,7 +925,7 @@ static int pci_save_pcie_state(struct pci_dev *dev)
 
 static void pci_restore_pcie_state(struct pci_dev *dev)
 {
-	int i = 0, pos;
+	int i = 0, pos, type;
 	struct pci_cap_saved_state *save_state;
 	u16 *cap;
 	u16 flags;
@@ -937,13 +938,14 @@ static void pci_restore_pcie_state(struct pci_dev *dev)
 
 	pci_read_config_word(dev, pos + PCI_EXP_FLAGS, &flags);
 
-	if (pcie_cap_has_devctl(dev->pcie_type, flags))
+	type = pci_pcie_type(dev);
+	if (pcie_cap_has_devctl(type, flags))
 		pci_write_config_word(dev, pos + PCI_EXP_DEVCTL, cap[i++]);
-	if (pcie_cap_has_lnkctl(dev->pcie_type, flags))
+	if (pcie_cap_has_lnkctl(type, flags))
 		pci_write_config_word(dev, pos + PCI_EXP_LNKCTL, cap[i++]);
-	if (pcie_cap_has_sltctl(dev->pcie_type, flags))
+	if (pcie_cap_has_sltctl(type, flags))
 		pci_write_config_word(dev, pos + PCI_EXP_SLTCTL, cap[i++]);
-	if (pcie_cap_has_rtctl(dev->pcie_type, flags))
+	if (pcie_cap_has_rtctl(type, flags))
 		pci_write_config_word(dev, pos + PCI_EXP_RTCTL, cap[i++]);
 
 	pos = pci_pcie_cap2(dev);
@@ -2459,8 +2461,8 @@ bool pci_acs_enabled(struct pci_dev *pdev, u16 acs_flags)
 		acs_flags &= (PCI_ACS_RR | PCI_ACS_CR |
 			      PCI_ACS_EC | PCI_ACS_DT);
 
-	if (pdev->pcie_type == PCI_EXP_TYPE_DOWNSTREAM ||
-	    pdev->pcie_type == PCI_EXP_TYPE_ROOT_PORT ||
+	if (pci_pcie_type(pdev) == PCI_EXP_TYPE_DOWNSTREAM ||
+	    pci_pcie_type(pdev) == PCI_EXP_TYPE_ROOT_PORT ||
 	    pdev->multifunction) {
 		pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_ACS);
 		if (!pos)
diff --git a/drivers/pci/pcie/aer/aer_inject.c b/drivers/pci/pcie/aer/aer_inject.c
index 5222986..4e24cb8 100644
--- a/drivers/pci/pcie/aer/aer_inject.c
+++ b/drivers/pci/pcie/aer/aer_inject.c
@@ -288,7 +288,7 @@ static struct pci_dev *pcie_find_root_port(struct pci_dev *dev)
 	while (1) {
 		if (!pci_is_pcie(dev))
 			break;
-		if (dev->pcie_type == PCI_EXP_TYPE_ROOT_PORT)
+		if (pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT)
 			return dev;
 		if (!dev->bus->self)
 			break;
diff --git a/drivers/pci/pcie/aer/aerdrv.c b/drivers/pci/pcie/aer/aerdrv.c
index 58ad791..f7c6245 100644
--- a/drivers/pci/pcie/aer/aerdrv.c
+++ b/drivers/pci/pcie/aer/aerdrv.c
@@ -81,10 +81,11 @@ bool pci_aer_available(void)
 static int set_device_error_reporting(struct pci_dev *dev, void *data)
 {
 	bool enable = *((bool *)data);
+	int type = pci_pcie_type(dev);
 
-	if ((dev->pcie_type == PCI_EXP_TYPE_ROOT_PORT) ||
-	    (dev->pcie_type == PCI_EXP_TYPE_UPSTREAM) ||
-	    (dev->pcie_type == PCI_EXP_TYPE_DOWNSTREAM)) {
+	if ((type == PCI_EXP_TYPE_ROOT_PORT) ||
+	    (type == PCI_EXP_TYPE_UPSTREAM) ||
+	    (type == PCI_EXP_TYPE_DOWNSTREAM)) {
 		if (enable)
 			pci_enable_pcie_error_reporting(dev);
 		else
diff --git a/drivers/pci/pcie/aer/aerdrv_acpi.c b/drivers/pci/pcie/aer/aerdrv_acpi.c
index 124f20f..5194a7d 100644
--- a/drivers/pci/pcie/aer/aerdrv_acpi.c
+++ b/drivers/pci/pcie/aer/aerdrv_acpi.c
@@ -60,7 +60,7 @@ static int aer_hest_parse(struct acpi_hest_header *hest_hdr, void *data)
 	p = (struct acpi_hest_aer_common *)(hest_hdr + 1);
 	if (p->flags & ACPI_HEST_GLOBAL) {
 		if ((pci_is_pcie(info->pci_dev) &&
-		     info->pci_dev->pcie_type == pcie_type) || bridge)
+		     pci_pcie_type(info->pci_dev) == pcie_type) || bridge)
 			ff = !!(p->flags & ACPI_HEST_FIRMWARE_FIRST);
 	} else
 		if (hest_match_pci(p, info->pci_dev))
diff --git a/drivers/pci/pcie/aer/aerdrv_core.c b/drivers/pci/pcie/aer/aerdrv_core.c
index 0ca0535..f551534 100644
--- a/drivers/pci/pcie/aer/aerdrv_core.c
+++ b/drivers/pci/pcie/aer/aerdrv_core.c
@@ -465,7 +465,7 @@ static pci_ers_result_t reset_link(struct pci_dev *dev)
 
 	if (driver && driver->reset_link) {
 		status = driver->reset_link(udev);
-	} else if (udev->pcie_type == PCI_EXP_TYPE_DOWNSTREAM) {
+	} else if (pci_pcie_type(udev) == PCI_EXP_TYPE_DOWNSTREAM) {
 		status = default_downstream_reset_link(udev);
 	} else {
 		dev_printk(KERN_DEBUG, &dev->dev,
diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index b500840..2591603 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -412,7 +412,7 @@ static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
 	 * do ASPM for now.
 	 */
 	list_for_each_entry(child, &linkbus->devices, bus_list) {
-		if (child->pcie_type == PCI_EXP_TYPE_PCI_BRIDGE) {
+		if (pci_pcie_type(child) == PCI_EXP_TYPE_PCI_BRIDGE) {
 			link->aspm_disable = ASPM_STATE_ALL;
 			break;
 		}
@@ -425,8 +425,8 @@ static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
 		struct aspm_latency *acceptable =
 			&link->acceptable[PCI_FUNC(child->devfn)];
 
-		if (child->pcie_type != PCI_EXP_TYPE_ENDPOINT &&
-		    child->pcie_type != PCI_EXP_TYPE_LEG_END)
+		if (pci_pcie_type(child) != PCI_EXP_TYPE_ENDPOINT &&
+		    pci_pcie_type(child) != PCI_EXP_TYPE_LEG_END)
 			continue;
 
 		pos = pci_pcie_cap(child);
@@ -552,7 +552,7 @@ static struct pcie_link_state *alloc_pcie_link_state(struct pci_dev *pdev)
 	INIT_LIST_HEAD(&link->children);
 	INIT_LIST_HEAD(&link->link);
 	link->pdev = pdev;
-	if (pdev->pcie_type == PCI_EXP_TYPE_DOWNSTREAM) {
+	if (pci_pcie_type(pdev) == PCI_EXP_TYPE_DOWNSTREAM) {
 		struct pcie_link_state *parent;
 		parent = pdev->bus->parent->self->link_state;
 		if (!parent) {
@@ -585,12 +585,12 @@ void pcie_aspm_init_link_state(struct pci_dev *pdev)
 
 	if (!pci_is_pcie(pdev) || pdev->link_state)
 		return;
-	if (pdev->pcie_type != PCI_EXP_TYPE_ROOT_PORT &&
-	    pdev->pcie_type != PCI_EXP_TYPE_DOWNSTREAM)
+	if (pci_pcie_type(pdev) != PCI_EXP_TYPE_ROOT_PORT &&
+	    pci_pcie_type(pdev) != PCI_EXP_TYPE_DOWNSTREAM)
 		return;
 
 	/* VIA has a strange chipset, root port is under a bridge */
-	if (pdev->pcie_type == PCI_EXP_TYPE_ROOT_PORT &&
+	if (pci_pcie_type(pdev) == PCI_EXP_TYPE_ROOT_PORT &&
 	    pdev->bus->self)
 		return;
 
@@ -647,8 +647,8 @@ static void pcie_update_aspm_capable(struct pcie_link_state *root)
 		if (link->root != root)
 			continue;
 		list_for_each_entry(child, &linkbus->devices, bus_list) {
-			if ((child->pcie_type != PCI_EXP_TYPE_ENDPOINT) &&
-			    (child->pcie_type != PCI_EXP_TYPE_LEG_END))
+			if ((pci_pcie_type(child) != PCI_EXP_TYPE_ENDPOINT) &&
+			    (pci_pcie_type(child) != PCI_EXP_TYPE_LEG_END))
 				continue;
 			pcie_aspm_check_latency(child);
 		}
@@ -663,8 +663,8 @@ void pcie_aspm_exit_link_state(struct pci_dev *pdev)
 
 	if (!pci_is_pcie(pdev) || !parent || !parent->link_state)
 		return;
-	if ((parent->pcie_type != PCI_EXP_TYPE_ROOT_PORT) &&
-	    (parent->pcie_type != PCI_EXP_TYPE_DOWNSTREAM))
+	if ((pci_pcie_type(parent) != PCI_EXP_TYPE_ROOT_PORT) &&
+	    (pci_pcie_type(parent) != PCI_EXP_TYPE_DOWNSTREAM))
 		return;
 
 	down_read(&pci_bus_sem);
@@ -704,8 +704,8 @@ void pcie_aspm_pm_state_change(struct pci_dev *pdev)
 
 	if (aspm_disabled || !pci_is_pcie(pdev) || !link)
 		return;
-	if ((pdev->pcie_type != PCI_EXP_TYPE_ROOT_PORT) &&
-	    (pdev->pcie_type != PCI_EXP_TYPE_DOWNSTREAM))
+	if ((pci_pcie_type(pdev) != PCI_EXP_TYPE_ROOT_PORT) &&
+	    (pci_pcie_type(pdev) != PCI_EXP_TYPE_DOWNSTREAM))
 		return;
 	/*
 	 * Devices changed PM state, we should recheck if latency
@@ -729,8 +729,8 @@ void pcie_aspm_powersave_config_link(struct pci_dev *pdev)
 	if (aspm_policy != POLICY_POWERSAVE)
 		return;
 
-	if ((pdev->pcie_type != PCI_EXP_TYPE_ROOT_PORT) &&
-	    (pdev->pcie_type != PCI_EXP_TYPE_DOWNSTREAM))
+	if ((pci_pcie_type(pdev) != PCI_EXP_TYPE_ROOT_PORT) &&
+	    (pci_pcie_type(pdev) != PCI_EXP_TYPE_DOWNSTREAM))
 		return;
 
 	down_read(&pci_bus_sem);
@@ -757,8 +757,8 @@ static void __pci_disable_link_state(struct pci_dev *pdev, int state, bool sem,
 	if (!pci_is_pcie(pdev))
 		return;
 
-	if (pdev->pcie_type == PCI_EXP_TYPE_ROOT_PORT ||
-	    pdev->pcie_type == PCI_EXP_TYPE_DOWNSTREAM)
+	if (pci_pcie_type(pdev) == PCI_EXP_TYPE_ROOT_PORT ||
+	    pci_pcie_type(pdev) == PCI_EXP_TYPE_DOWNSTREAM)
 		parent = pdev;
 	if (!parent || !parent->link_state)
 		return;
@@ -933,8 +933,8 @@ void pcie_aspm_create_sysfs_dev_files(struct pci_dev *pdev)
 	struct pcie_link_state *link_state = pdev->link_state;
 
 	if (!pci_is_pcie(pdev) ||
-	    (pdev->pcie_type != PCI_EXP_TYPE_ROOT_PORT &&
-	     pdev->pcie_type != PCI_EXP_TYPE_DOWNSTREAM) || !link_state)
+	    (pci_pcie_type(pdev) != PCI_EXP_TYPE_ROOT_PORT &&
+	     pci_pcie_type(pdev) != PCI_EXP_TYPE_DOWNSTREAM) || !link_state)
 		return;
 
 	if (link_state->aspm_support)
@@ -950,8 +950,8 @@ void pcie_aspm_remove_sysfs_dev_files(struct pci_dev *pdev)
 	struct pcie_link_state *link_state = pdev->link_state;
 
 	if (!pci_is_pcie(pdev) ||
-	    (pdev->pcie_type != PCI_EXP_TYPE_ROOT_PORT &&
-	     pdev->pcie_type != PCI_EXP_TYPE_DOWNSTREAM) || !link_state)
+	    (pci_pcie_type(pdev) != PCI_EXP_TYPE_ROOT_PORT &&
+	     pci_pcie_type(pdev) != PCI_EXP_TYPE_DOWNSTREAM) || !link_state)
 		return;
 
 	if (link_state->aspm_support)
diff --git a/drivers/pci/pcie/pme.c b/drivers/pci/pcie/pme.c
index 001f1b7..30897bf 100644
--- a/drivers/pci/pcie/pme.c
+++ b/drivers/pci/pcie/pme.c
@@ -120,7 +120,7 @@ static bool pcie_pme_from_pci_bridge(struct pci_bus *bus, u8 devfn)
 	if (!dev)
 		return false;
 
-	if (pci_is_pcie(dev) && dev->pcie_type == PCI_EXP_TYPE_PCI_BRIDGE) {
+	if (pci_is_pcie(dev) && pci_pcie_type(dev) == PCI_EXP_TYPE_PCI_BRIDGE) {
 		down_read(&pci_bus_sem);
 		if (pcie_pme_walk_bus(bus))
 			found = true;
@@ -335,13 +335,13 @@ static void pcie_pme_mark_devices(struct pci_dev *port)
 		struct pci_dev *dev;
 
 		/* Check if this is a root port event collector. */
-		if (port->pcie_type != PCI_EXP_TYPE_RC_EC || !bus)
+		if (pci_pcie_type(port) != PCI_EXP_TYPE_RC_EC || !bus)
 			return;
 
 		down_read(&pci_bus_sem);
 		list_for_each_entry(dev, &bus->devices, bus_list)
 			if (pci_is_pcie(dev)
-			    && dev->pcie_type == PCI_EXP_TYPE_RC_END)
+			    && pci_pcie_type(dev) == PCI_EXP_TYPE_RC_END)
 				pcie_pme_set_native(dev, NULL);
 		up_read(&pci_bus_sem);
 	}
diff --git a/drivers/pci/pcie/portdrv_bus.c b/drivers/pci/pcie/portdrv_bus.c
index 18bf90f..67be55a 100644
--- a/drivers/pci/pcie/portdrv_bus.c
+++ b/drivers/pci/pcie/portdrv_bus.c
@@ -38,7 +38,7 @@ static int pcie_port_bus_match(struct device *dev, struct device_driver *drv)
 		return 0;
 
 	if ((driver->port_type != PCIE_ANY_PORT) &&
-	    (driver->port_type != pciedev->port->pcie_type))
+	    (driver->port_type != pci_pcie_type(pciedev->port)))
 		return 0;
 
 	return 1;
diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c
index 75915b3..bf320a9 100644
--- a/drivers/pci/pcie/portdrv_core.c
+++ b/drivers/pci/pcie/portdrv_core.c
@@ -298,7 +298,7 @@ static int get_port_device_capability(struct pci_dev *dev)
 		services |= PCIE_PORT_SERVICE_VC;
 	/* Root ports are capable of generating PME too */
 	if ((cap_mask & PCIE_PORT_SERVICE_PME)
-	    && dev->pcie_type == PCI_EXP_TYPE_ROOT_PORT) {
+	    && pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT) {
 		services |= PCIE_PORT_SERVICE_PME;
 		/*
 		 * Disable PME interrupt on this port in case it's been enabled
@@ -336,7 +336,7 @@ static int pcie_device_init(struct pci_dev *pdev, int service, int irq)
 	device->release = release_pcie_device;	/* callback to free pcie dev */
 	dev_set_name(device, "%s:pcie%02x",
 		     pci_name(pdev),
-		     get_descriptor_id(pdev->pcie_type, service));
+		     get_descriptor_id(pci_pcie_type(pdev), service));
 	device->parent = &pdev->dev;
 	device_enable_async_suspend(device);
 
diff --git a/drivers/pci/pcie/portdrv_pci.c b/drivers/pci/pcie/portdrv_pci.c
index 3a7eefc..24d1463 100644
--- a/drivers/pci/pcie/portdrv_pci.c
+++ b/drivers/pci/pcie/portdrv_pci.c
@@ -95,7 +95,7 @@ static int pcie_port_resume_noirq(struct device *dev)
 	 * which breaks ACPI-based runtime wakeup on PCI Express, so clear those
 	 * bits now just in case (shouldn't hurt).
 	 */
-	if(pdev->pcie_type == PCI_EXP_TYPE_ROOT_PORT)
+	if (pci_pcie_type(pdev) == PCI_EXP_TYPE_ROOT_PORT)
 		pcie_clear_root_pme_status(pdev);
 	return 0;
 }
@@ -186,9 +186,9 @@ static int __devinit pcie_portdrv_probe(struct pci_dev *dev,
 	int status;
 
 	if (!pci_is_pcie(dev) ||
-	    ((dev->pcie_type != PCI_EXP_TYPE_ROOT_PORT) &&
-	     (dev->pcie_type != PCI_EXP_TYPE_UPSTREAM) &&
-	     (dev->pcie_type != PCI_EXP_TYPE_DOWNSTREAM)))
+	    ((pci_pcie_type(dev) != PCI_EXP_TYPE_ROOT_PORT) &&
+	     (pci_pcie_type(dev) != PCI_EXP_TYPE_UPSTREAM) &&
+	     (pci_pcie_type(dev) != PCI_EXP_TYPE_DOWNSTREAM)))
 		return -ENODEV;
 
 	if (!dev->irq && dev->pin) {
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 65e82e3..3d958e6 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -1384,9 +1384,9 @@ static int only_one_child(struct pci_bus *bus)
 
 	if (!parent || !pci_is_pcie(parent))
 		return 0;
-	if (parent->pcie_type == PCI_EXP_TYPE_ROOT_PORT)
+	if (pci_pcie_type(parent) == PCI_EXP_TYPE_ROOT_PORT)
 		return 1;
-	if (parent->pcie_type == PCI_EXP_TYPE_DOWNSTREAM &&
+	if (pci_pcie_type(parent) == PCI_EXP_TYPE_DOWNSTREAM &&
 	    !pci_has_flag(PCI_SCAN_ALL_PCIE_DEVS))
 		return 1;
 	return 0;
@@ -1463,7 +1463,7 @@ static int pcie_find_smpss(struct pci_dev *dev, void *data)
 	 */
 	if (dev->is_hotplug_bridge && (!list_is_singular(&dev->bus->devices) ||
 	     (dev->bus->self &&
-	      dev->bus->self->pcie_type != PCI_EXP_TYPE_ROOT_PORT)))
+	      pci_pcie_type(dev->bus->self) != PCI_EXP_TYPE_ROOT_PORT)))
 		*smpss = 0;
 
 	if (*smpss > dev->pcie_mpss)
@@ -1479,7 +1479,8 @@ static void pcie_write_mps(struct pci_dev *dev, int mps)
 	if (pcie_bus_config == PCIE_BUS_PERFORMANCE) {
 		mps = 128 << dev->pcie_mpss;
 
-		if (dev->pcie_type != PCI_EXP_TYPE_ROOT_PORT && dev->bus->self)
+		if (pci_pcie_type(dev) != PCI_EXP_TYPE_ROOT_PORT &&
+		    dev->bus->self)
 			/* For "Performance", the assumption is made that
 			 * downstream communication will never be larger than
 			 * the MRRS.  So, the MPS only needs to be configured
diff --git a/drivers/pci/search.c b/drivers/pci/search.c
index 993d4a0..621b162 100644
--- a/drivers/pci/search.c
+++ b/drivers/pci/search.c
@@ -41,7 +41,7 @@ pci_find_upstream_pcie_bridge(struct pci_dev *pdev)
 			continue;
 		}
 		/* PCI device should connect to a PCIe bridge */
-		if (pdev->pcie_type != PCI_EXP_TYPE_PCI_BRIDGE) {
+		if (pci_pcie_type(pdev) != PCI_EXP_TYPE_PCI_BRIDGE) {
 			/* Busted hardware? */
 			WARN_ON_ONCE(1);
 			return NULL;
diff --git a/include/linux/pci.h b/include/linux/pci.h
index f4a7ad6..fc2034d 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1651,6 +1651,16 @@ static inline bool pci_is_pcie(struct pci_dev *dev)
 	return !!pci_pcie_cap(dev);
 }
 
+/**
+ * pci_pcie_type - get the PCIe device/port type
+ * @dev: PCI device
+ */
+static inline int pci_pcie_type(const struct pci_dev *dev)
+{
+	return (dev->pcie_flags & PCI_EXP_FLAGS_TYPE) >> 4;
+}
+
+
 void pci_request_acs(void);
 bool pci_acs_enabled(struct pci_dev *pdev, u16 acs_flags);
 bool pci_acs_path_enabled(struct pci_dev *start,
-- 
1.7.9.5

[RFC PATCH 03/14] PCI: remove unused field pcie_type from struct pci_dev

[Jiang Liu]

From: Yijing Wang <wangyijing@huawei.com>

With introduction of pci_pcie_type(), pci_dev->pcie_type field becomes
redundant, so remove it.

Signed-off-by: Yijing Wang <wangyijing@huawei.com>
Signed-off-by: Jiang Liu <liuj97@gmail.com>
---
 drivers/pci/probe.c |    1 -
 include/linux/pci.h |    1 -
 2 files changed, 2 deletions(-)

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 3d958e6..446933d 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -930,7 +930,6 @@ void set_pcie_port_type(struct pci_dev *pdev)
 	pdev->pcie_cap = pos;
 	pci_read_config_word(pdev, pos + PCI_EXP_FLAGS, &reg16);
 	pdev->pcie_flags = reg16;
-	pdev->pcie_type = (reg16 & PCI_EXP_FLAGS_TYPE) >> 4;
 	pci_read_config_word(pdev, pos + PCI_EXP_DEVCAP, &reg16);
 	pdev->pcie_mpss = reg16 & PCI_EXP_DEVCAP_PAYLOAD;
 }
diff --git a/include/linux/pci.h b/include/linux/pci.h
index fc2034d..1837354 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -254,7 +254,6 @@ struct pci_dev {
 	u8		revision;	/* PCI revision, low byte of class word */
 	u8		hdr_type;	/* PCI header type (`multi' flag masked out) */
 	u8		pcie_cap;	/* PCI-E capability offset */
-	u8		pcie_type:4;	/* PCI-E device/port type */
 	u8		pcie_mpss:3;	/* PCI-E Max Payload Size Supported */
 	u8		rom_base_reg;	/* which config register controls the ROM */
 	u8		pin;  		/* which interrupt pin this device uses */
-- 
1.7.9.5

[RFC PATCH 04/14] PCI: refine and move pcie_cap_has_*() macros to include/linux/pci.h

[Jiang Liu]

From: Jiang Liu <jiang.liu@huawei.com>

Move pcie_cap_has_*() macros to include/linux/pci.h, so they can be shared.
Since pcie_flags was introduced, rework these macros to take a struct pci_dev *
and use pcie_flags insead of type and flags.

Signed-off-by: Jiang Liu <liuj97@gmail.com>
Signed-off-by: Yijing Wang <wangyijing@huawei.com>
---
 drivers/pci/pci.c   |   50 ++++++++++++--------------------------------------
 include/linux/pci.h |   43 +++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 55 insertions(+), 38 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 28eb55b..4c6ab70 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -272,15 +272,11 @@ int pci_bus_find_capability(struct pci_bus *bus, unsigned int devfn, int cap)
  */
 static int pci_pcie_cap2(struct pci_dev *dev)
 {
-	u16 flags;
 	int pos;
 
 	pos = pci_pcie_cap(dev);
-	if (pos) {
-		pci_read_config_word(dev, pos + PCI_EXP_FLAGS, &flags);
-		if ((flags & PCI_EXP_FLAGS_VERS) < 2)
-			pos = 0;
-	}
+	if (pos && !pci_pcie_cap_has_cap2(dev))
+		pos = 0;
 
 	return pos;
 }
@@ -854,21 +850,6 @@ EXPORT_SYMBOL(pci_choose_state);
 
 #define PCI_EXP_SAVE_REGS	7
 
-#define pcie_cap_has_devctl(type, flags)	1
-#define pcie_cap_has_lnkctl(type, flags)		\
-		((flags & PCI_EXP_FLAGS_VERS) > 1 ||	\
-		 (type == PCI_EXP_TYPE_ROOT_PORT ||	\
-		  type == PCI_EXP_TYPE_ENDPOINT ||	\
-		  type == PCI_EXP_TYPE_LEG_END))
-#define pcie_cap_has_sltctl(type, flags)		\
-		((flags & PCI_EXP_FLAGS_VERS) > 1 ||	\
-		 ((type == PCI_EXP_TYPE_ROOT_PORT) ||	\
-		  (type == PCI_EXP_TYPE_DOWNSTREAM &&	\
-		   (flags & PCI_EXP_FLAGS_SLOT))))
-#define pcie_cap_has_rtctl(type, flags)			\
-		((flags & PCI_EXP_FLAGS_VERS) > 1 ||	\
-		 (type == PCI_EXP_TYPE_ROOT_PORT ||	\
-		  type == PCI_EXP_TYPE_RC_EC))
 
 static struct pci_cap_saved_state *pci_find_saved_cap(
 	struct pci_dev *pci_dev, char cap)
@@ -885,10 +866,9 @@ static struct pci_cap_saved_state *pci_find_saved_cap(
 
 static int pci_save_pcie_state(struct pci_dev *dev)
 {
-	int type, pos, i = 0;
+	int pos, i = 0;
 	struct pci_cap_saved_state *save_state;
 	u16 *cap;
-	u16 flags;
 
 	pos = pci_pcie_cap(dev);
 	if (!pos)
@@ -901,16 +881,14 @@ static int pci_save_pcie_state(struct pci_dev *dev)
 	}
 	cap = (u16 *)&save_state->cap.data[0];
 
-	pci_read_config_word(dev, pos + PCI_EXP_FLAGS, &flags);
 
-	type = pci_pcie_type(dev);
-	if (pcie_cap_has_devctl(type, flags))
+	if (pci_pcie_cap_has_devctl(dev))
 		pci_read_config_word(dev, pos + PCI_EXP_DEVCTL, &cap[i++]);
-	if (pcie_cap_has_lnkctl(type, flags))
+	if (pci_pcie_cap_has_lnkctl(dev))
 		pci_read_config_word(dev, pos + PCI_EXP_LNKCTL, &cap[i++]);
-	if (pcie_cap_has_sltctl(type, flags))
+	if (pci_pcie_cap_has_sltctl(dev))
 		pci_read_config_word(dev, pos + PCI_EXP_SLTCTL, &cap[i++]);
-	if (pcie_cap_has_rtctl(type, flags))
+	if (pci_pcie_cap_has_rtctl(dev))
 		pci_read_config_word(dev, pos + PCI_EXP_RTCTL, &cap[i++]);
 
 	pos = pci_pcie_cap2(dev);
@@ -925,10 +903,9 @@ static int pci_save_pcie_state(struct pci_dev *dev)
 
 static void pci_restore_pcie_state(struct pci_dev *dev)
 {
-	int i = 0, pos, type;
+	int i = 0, pos;
 	struct pci_cap_saved_state *save_state;
 	u16 *cap;
-	u16 flags;
 
 	save_state = pci_find_saved_cap(dev, PCI_CAP_ID_EXP);
 	pos = pci_find_capability(dev, PCI_CAP_ID_EXP);
@@ -936,16 +913,13 @@ static void pci_restore_pcie_state(struct pci_dev *dev)
 		return;
 	cap = (u16 *)&save_state->cap.data[0];
 
-	pci_read_config_word(dev, pos + PCI_EXP_FLAGS, &flags);
-
-	type = pci_pcie_type(dev);
-	if (pcie_cap_has_devctl(type, flags))
+	if (pci_pcie_cap_has_devctl(dev))
 		pci_write_config_word(dev, pos + PCI_EXP_DEVCTL, cap[i++]);
-	if (pcie_cap_has_lnkctl(type, flags))
+	if (pci_pcie_cap_has_lnkctl(dev))
 		pci_write_config_word(dev, pos + PCI_EXP_LNKCTL, cap[i++]);
-	if (pcie_cap_has_sltctl(type, flags))
+	if (pci_pcie_cap_has_sltctl(dev))
 		pci_write_config_word(dev, pos + PCI_EXP_SLTCTL, cap[i++]);
-	if (pcie_cap_has_rtctl(type, flags))
+	if (pci_pcie_cap_has_rtctl(dev))
 		pci_write_config_word(dev, pos + PCI_EXP_RTCTL, cap[i++]);
 
 	pos = pci_pcie_cap2(dev);
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 1837354..346b2d9 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1659,6 +1659,49 @@ static inline int pci_pcie_type(const struct pci_dev *dev)
 	return (dev->pcie_flags & PCI_EXP_FLAGS_TYPE) >> 4;
 }
 
+static inline int pci_pcie_cap_version(const struct pci_dev *pdev)
+{
+	return pdev->pcie_flags & PCI_EXP_FLAGS_VERS;
+}
+
+static inline int pci_pcie_cap_has_cap2(const struct pci_dev *pdev)
+{
+	return pci_pcie_cap_version(pdev) > 1;
+}
+
+static inline bool pci_pcie_cap_has_devctl(const struct pci_dev *pdev)
+{
+	return true;
+}
+
+static inline bool pci_pcie_cap_has_lnkctl(const struct pci_dev *pdev)
+{
+	int type = pci_pcie_type(pdev);
+
+	return pci_pcie_cap_version(pdev) > 1 ||
+	       type == PCI_EXP_TYPE_ROOT_PORT ||
+	       type == PCI_EXP_TYPE_ENDPOINT ||
+	       type == PCI_EXP_TYPE_LEG_END;
+}
+
+static inline bool pci_pcie_cap_has_sltctl(const struct pci_dev *pdev)
+{
+	int type = pci_pcie_type(pdev);
+
+	return pci_pcie_cap_version(pdev) > 1 ||
+	       type == PCI_EXP_TYPE_ROOT_PORT ||
+	       (type == PCI_EXP_TYPE_DOWNSTREAM &&
+		pdev->pcie_flags & PCI_EXP_FLAGS_SLOT);
+}
+
+static inline bool pci_pcie_cap_has_rtctl(const struct pci_dev *pdev)
+{
+	int type = pci_pcie_type(pdev);
+
+	return pci_pcie_cap_version(pdev) > 1 ||
+	       type == PCI_EXP_TYPE_ROOT_PORT ||
+	       type == PCI_EXP_TYPE_RC_EC;
+}
 
 void pci_request_acs(void);
 bool pci_acs_enabled(struct pci_dev *pdev, u16 acs_flags);
-- 
1.7.9.5

[RFC PATCH 05/14] PCI: add access functions for PCIe capabilities to hide PCIe spec differences

[Jiang Liu]

From: Jiang Liu <jiang.liu@huawei.com>

Introduce four configuration access functions for PCIe capabilities to
hide difference among PCIe Base Spec versions. With these functions,
we can remove callers responsible for using pci_pcie_cap_has_*().

pci_pcie_cap_read_word/dword() functions will store the pcie cap register
value by passed parameter val,if related pcie cap register is not implemented
on the pcie device, the passed parameter val will be set 0 and return -EINVAL.

pci_pcie_capability_write_word/dowrd() functions will write the value to pcie
cap registers,if related pcie cap register is not implemented on the pcie
device, it will return -EINVAL.

Signed-off-by: Jiang Liu <liuj97@gmail.com>
Signed-off-by: Yijing Wang <wangyijing@huawei.com>
---
 drivers/pci/access.c     |   88 ++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/pci.h      |   10 ++++++
 include/linux/pci_regs.h |   19 ++++++++--
 3 files changed, 115 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/access.c b/drivers/pci/access.c
index ba91a7e..80ae022 100644
--- a/drivers/pci/access.c
+++ b/drivers/pci/access.c
@@ -469,3 +469,91 @@ void pci_cfg_access_unlock(struct pci_dev *dev)
 	raw_spin_unlock_irqrestore(&pci_lock, flags);
 }
 EXPORT_SYMBOL_GPL(pci_cfg_access_unlock);
+
+static int
+pci_pcie_cap_get_offset(struct pci_dev *dev, int where, size_t sz)
+{
+	bool valid;
+
+	if (!pci_is_pcie(dev))
+		return -EINVAL;
+	if (where & (sz - 1))
+		return -EINVAL;
+
+	if (where < 0)
+		valid = false;
+	else if (where < PCI_EXP_DEVCAP)
+		valid = true;
+	else if (where < PCI_EXP_LNKCAP)
+		valid = pci_pcie_cap_has_devctl(dev);
+	else if (where < PCI_EXP_SLTCAP)
+		valid = pci_pcie_cap_has_lnkctl(dev);
+	else if (where < PCI_EXP_RTCTL)
+		valid = pci_pcie_cap_has_sltctl(dev);
+	else if (where < PCI_EXP_DEVCAP2)
+		valid = pci_pcie_cap_has_rtctl(dev);
+	else if (where < PCI_EXP_CAP2_SIZE)
+		valid = pci_pcie_cap_has_cap2(dev);
+	else
+		valid = false;
+
+	return valid ? where + pci_pcie_cap(dev) : -EINVAL;
+}
+
+int pci_pcie_cap_read_word(struct pci_dev *dev, int where, u16 *valp)
+{
+	*valp = 0;
+	where = pci_pcie_cap_get_offset(dev, where, sizeof(u16));
+	if (where >= 0)
+		return pci_read_config_word(dev, where, valp);
+
+	if (pci_is_pcie(dev) && where == PCI_EXP_SLTSTA &&
+	    pci_pcie_type(dev) == PCI_EXP_TYPE_DOWNSTREAM)
+		*valp = PCI_EXP_SLTSTA_PDS;
+
+	return -EINVAL;
+}
+EXPORT_SYMBOL(pci_pcie_cap_read_word);
+
+int pci_pcie_cap_read_dword(struct pci_dev *dev, int where, u32 *valp)
+{
+	*valp = 0;
+	where = pci_pcie_cap_get_offset(dev, where, sizeof(u32));
+	if (where >= 0)
+		return pci_read_config_dword(dev, where, valp);
+
+	/*
+	 * Quotation from PCIe Base Spec 3.0:
+	 * For Functions that do not implement the Slot Capabilities,
+	 * Slot Status, and Slot Control registers, these spaces must
+	 * be hardwired to 0b, with the exception of the Presence Detect
+	 * State bit in the Slot Status register of Downstream Ports,
+	 * which must be hardwired to 1b.
+	 */
+	if (pci_is_pcie(dev) && where == PCI_EXP_SLTCTL &&
+	    pci_pcie_type(dev) == PCI_EXP_TYPE_DOWNSTREAM)
+		*valp = PCI_EXP_SLTSTA_PDS << 16;
+
+	return -EINVAL;
+}
+EXPORT_SYMBOL(pci_pcie_cap_read_dword);
+
+int pci_pcie_cap_write_word(struct pci_dev *dev, int where, u16 val)
+{
+	where = pci_pcie_cap_get_offset(dev, where, sizeof(u16));
+	if (where >= 0)
+		return pci_write_config_word(dev, where, val);
+
+	return -EINVAL;
+}
+EXPORT_SYMBOL(pci_pcie_cap_write_word);
+
+int pci_pcie_cap_write_dword(struct pci_dev *dev, int where, u32 val)
+{
+	where = pci_pcie_cap_get_offset(dev, where, sizeof(u32));
+	if (where >= 0)
+		return pci_write_config_dword(dev, where, val);
+
+	return -EINVAL;
+}
+EXPORT_SYMBOL(pci_pcie_cap_write_dword);
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 346b2d9..78767b2 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1703,6 +1703,11 @@ static inline bool pci_pcie_cap_has_rtctl(const struct pci_dev *pdev)
 	       type == PCI_EXP_TYPE_RC_EC;
 }
 
+extern int pci_pcie_cap_read_word(struct pci_dev *dev, int where, u16 *valp);
+extern int pci_pcie_cap_read_dword(struct pci_dev *dev, int where, u32 *valp);
+extern int pci_pcie_cap_write_word(struct pci_dev *dev, int where, u16 val);
+extern int pci_pcie_cap_write_dword(struct pci_dev *dev, int where, u32 val);
+
 void pci_request_acs(void);
 bool pci_acs_enabled(struct pci_dev *pdev, u16 acs_flags);
 bool pci_acs_path_enabled(struct pci_dev *start,
@@ -1843,5 +1848,10 @@ static inline struct eeh_dev *pci_dev_to_eeh_dev(struct pci_dev *pdev)
  */
 struct pci_dev *pci_find_upstream_pcie_bridge(struct pci_dev *pdev);
 
+int pci_pcie_capability_read_word(struct pci_dev *dev, int where, u16 *val);
+int pci_pcie_capability_read_dword(struct pci_dev *dev, int where, u32 *val);
+int pci_pcie_capability_write_word(struct pci_dev *dev, int where, u16 val);
+int pci_pcie_capability_write_dword(struct pci_dev *dev, int where, u32 val);
+
 #endif /* __KERNEL__ */
 #endif /* LINUX_PCI_H */
diff --git a/include/linux/pci_regs.h b/include/linux/pci_regs.h
index 53274bf..ac60e22 100644
--- a/include/linux/pci_regs.h
+++ b/include/linux/pci_regs.h
@@ -542,9 +542,24 @@
 #define  PCI_EXP_OBFF_MSGA_EN	0x2000	/* OBFF enable with Message type A */
 #define  PCI_EXP_OBFF_MSGB_EN	0x4000	/* OBFF enable with Message type B */
 #define  PCI_EXP_OBFF_WAKE_EN	0x6000	/* OBFF using WAKE# signaling */
-#define PCI_CAP_EXP_ENDPOINT_SIZEOF_V2	44	/* v2 endpoints end here */
+#define PCI_EXP_DEVSTA2		42	/* Device Status 2 */
+#define PCI_CAP_EXP_ENDPOINT_SIZEOF_V2	44 /* v2 endpoints end here */
+#define PCI_EXP_LNKCAP2		44	/* Link Capabilities 2 */
 #define PCI_EXP_LNKCTL2		48	/* Link Control 2 */
-#define PCI_EXP_SLTCTL2		56	/* Slot Control 2 */
+#define  PCI_EXP_LNKCTL2_TLS	0x0f	/* Target Link Speed */
+#define  PCI_EXP_LNKCTL2_EC	0x10	/* Enter Compliance */
+#define  PCI_EXP_LNKCTL2_HASD	0x20	/* Hardware Autonomous Speed Disable */
+#define  PCI_EXP_LNKCTL2_SD	0x40	/* Selectable De-emphasis */
+#define  PCI_EXP_LNKCTL2_TM	0x380	/* Transmit Margin */
+#define  PCI_EXP_LNKCTL2_EMC	0x400	/* Enter Modified Compliance */
+#define  PCI_EXP_LNKCTL2_CS	0x800	/* Compliance SOS */
+#define  PCI_EXP_LNKCTL2_CD	0x1000	/* Compliance De-emphasis */
+#define PCI_EXP_LNKSTA2		50	/* Link Status 2 */
+#define  PCI_EXP_LNKSTA2_CDL	0x01	/* Current De-emphasis Level */
+#define PCI_EXP_SLTCAP2		52	/* Slot Capabilities 2 */
+#define PCI_EXP_SLTCTL2		56	/* Slot Control 2*/
+#define PCI_EXP_SLTSTA2		58	/* Slot Status 2*/
+#define	PCI_EXP_CAP2_SIZE	60
 
 /* Extended Capabilities (PCI-X 2.0 and Express) */
 #define PCI_EXT_CAP_ID(header)		(header & 0x0000ffff)
-- 
1.7.9.5

[RFC PATCH 06/14] PCI: use PCIe cap access functions to simplify PCI core implementation

[Jiang Liu]

From: Jiang Liu <jiang.liu@huawei.com>

Use PCIe cap access functions to simplify PCI core implementation.

Signed-off-by: Jiang Liu <liuj97@gmail.com>
---
 drivers/pci/pci.c    |  237 ++++++++++++++------------------------------------
 drivers/pci/probe.c  |   19 ++--
 drivers/pci/quirks.c |    8 +-
 3 files changed, 73 insertions(+), 191 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 4c6ab70..b8142f1 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -254,34 +254,6 @@ int pci_bus_find_capability(struct pci_bus *bus, unsigned int devfn, int cap)
 }
 
 /**
- * pci_pcie_cap2 - query for devices' PCI_CAP_ID_EXP v2 capability structure
- * @dev: PCI device to check
- *
- * Like pci_pcie_cap() but also checks that the PCIe capability version is
- * >= 2.  Note that v1 capability structures could be sparse in that not
- * all register fields were required.  v2 requires the entire structure to
- * be present size wise, while still allowing for non-implemented registers
- * to exist but they must be hardwired to 0.
- *
- * Due to the differences in the versions of capability structures, one
- * must be careful not to try and access non-existant registers that may
- * exist in early versions - v1 - of Express devices.
- *
- * Returns the offset of the PCIe capability structure as long as the
- * capability version is >= 2; otherwise 0 is returned.
- */
-static int pci_pcie_cap2(struct pci_dev *dev)
-{
-	int pos;
-
-	pos = pci_pcie_cap(dev);
-	if (pos && !pci_pcie_cap_has_cap2(dev))
-		pos = 0;
-
-	return pos;
-}
-
-/**
  * pci_find_ext_capability - Find an extended capability
  * @dev: PCI device to query
  * @cap: capability code
@@ -866,12 +838,11 @@ static struct pci_cap_saved_state *pci_find_saved_cap(
 
 static int pci_save_pcie_state(struct pci_dev *dev)
 {
-	int pos, i = 0;
+	int i = 0;
 	struct pci_cap_saved_state *save_state;
 	u16 *cap;
 
-	pos = pci_pcie_cap(dev);
-	if (!pos)
+	if (!pci_is_pcie(dev))
 		return 0;
 
 	save_state = pci_find_saved_cap(dev, PCI_CAP_ID_EXP);
@@ -881,54 +852,35 @@ static int pci_save_pcie_state(struct pci_dev *dev)
 	}
 	cap = (u16 *)&save_state->cap.data[0];
 
+	pci_pcie_cap_read_word(dev, PCI_EXP_DEVCTL, &cap[i++]);
+	pci_pcie_cap_read_word(dev, PCI_EXP_LNKCTL, &cap[i++]);
+	pci_pcie_cap_read_word(dev, PCI_EXP_SLTCTL, &cap[i++]);
+	pci_pcie_cap_read_word(dev, PCI_EXP_RTCTL,  &cap[i++]);
+	pci_pcie_cap_read_word(dev, PCI_EXP_DEVCTL2, &cap[i++]);
+	pci_pcie_cap_read_word(dev, PCI_EXP_LNKCTL2, &cap[i++]);
+	pci_pcie_cap_read_word(dev, PCI_EXP_SLTCTL2, &cap[i++]);
 
-	if (pci_pcie_cap_has_devctl(dev))
-		pci_read_config_word(dev, pos + PCI_EXP_DEVCTL, &cap[i++]);
-	if (pci_pcie_cap_has_lnkctl(dev))
-		pci_read_config_word(dev, pos + PCI_EXP_LNKCTL, &cap[i++]);
-	if (pci_pcie_cap_has_sltctl(dev))
-		pci_read_config_word(dev, pos + PCI_EXP_SLTCTL, &cap[i++]);
-	if (pci_pcie_cap_has_rtctl(dev))
-		pci_read_config_word(dev, pos + PCI_EXP_RTCTL, &cap[i++]);
-
-	pos = pci_pcie_cap2(dev);
-	if (!pos)
-		return 0;
-
-	pci_read_config_word(dev, pos + PCI_EXP_DEVCTL2, &cap[i++]);
-	pci_read_config_word(dev, pos + PCI_EXP_LNKCTL2, &cap[i++]);
-	pci_read_config_word(dev, pos + PCI_EXP_SLTCTL2, &cap[i++]);
 	return 0;
 }
 
 static void pci_restore_pcie_state(struct pci_dev *dev)
 {
-	int i = 0, pos;
+	int i = 0;
 	struct pci_cap_saved_state *save_state;
 	u16 *cap;
 
 	save_state = pci_find_saved_cap(dev, PCI_CAP_ID_EXP);
-	pos = pci_find_capability(dev, PCI_CAP_ID_EXP);
-	if (!save_state || pos <= 0)
+	if (!save_state)
 		return;
 	cap = (u16 *)&save_state->cap.data[0];
 
-	if (pci_pcie_cap_has_devctl(dev))
-		pci_write_config_word(dev, pos + PCI_EXP_DEVCTL, cap[i++]);
-	if (pci_pcie_cap_has_lnkctl(dev))
-		pci_write_config_word(dev, pos + PCI_EXP_LNKCTL, cap[i++]);
-	if (pci_pcie_cap_has_sltctl(dev))
-		pci_write_config_word(dev, pos + PCI_EXP_SLTCTL, cap[i++]);
-	if (pci_pcie_cap_has_rtctl(dev))
-		pci_write_config_word(dev, pos + PCI_EXP_RTCTL, cap[i++]);
-
-	pos = pci_pcie_cap2(dev);
-	if (!pos)
-		return;
-
-	pci_write_config_word(dev, pos + PCI_EXP_DEVCTL2, cap[i++]);
-	pci_write_config_word(dev, pos + PCI_EXP_LNKCTL2, cap[i++]);
-	pci_write_config_word(dev, pos + PCI_EXP_SLTCTL2, cap[i++]);
+	pci_pcie_cap_write_word(dev, PCI_EXP_DEVCTL, cap[i++]);
+	pci_pcie_cap_write_word(dev, PCI_EXP_LNKCTL, cap[i++]);
+	pci_pcie_cap_write_word(dev, PCI_EXP_SLTCTL, cap[i++]);
+	pci_pcie_cap_write_word(dev, PCI_EXP_RTCTL,  cap[i++]);
+	pci_pcie_cap_write_word(dev, PCI_EXP_DEVCTL2, cap[i++]);
+	pci_pcie_cap_write_word(dev, PCI_EXP_LNKCTL2, cap[i++]);
+	pci_pcie_cap_write_word(dev, PCI_EXP_SLTCTL2, cap[i++]);
 }
 
 
@@ -2042,7 +1994,6 @@ void pci_free_cap_save_buffers(struct pci_dev *dev)
  */
 void pci_enable_ari(struct pci_dev *dev)
 {
-	int pos;
 	u32 cap;
 	u16 ctrl;
 	struct pci_dev *bridge;
@@ -2050,8 +2001,7 @@ void pci_enable_ari(struct pci_dev *dev)
 	if (pcie_ari_disabled || !pci_is_pcie(dev) || dev->devfn)
 		return;
 
-	pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ARI);
-	if (!pos)
+	if (pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ARI))
 		return;
 
 	bridge = dev->bus->self;
@@ -2059,17 +2009,14 @@ void pci_enable_ari(struct pci_dev *dev)
 		return;
 
 	/* ARI is a PCIe cap v2 feature */
-	pos = pci_pcie_cap2(bridge);
-	if (!pos)
+	if (pci_pcie_cap_read_dword(bridge, PCI_EXP_DEVCAP2, &cap) ||
+	   !(cap & PCI_EXP_DEVCAP2_ARI))
 		return;
 
-	pci_read_config_dword(bridge, pos + PCI_EXP_DEVCAP2, &cap);
-	if (!(cap & PCI_EXP_DEVCAP2_ARI))
+	if (pci_pcie_cap_read_word(bridge, PCI_EXP_DEVCTL2, &ctrl))
 		return;
-
-	pci_read_config_word(bridge, pos + PCI_EXP_DEVCTL2, &ctrl);
 	ctrl |= PCI_EXP_DEVCTL2_ARI;
-	pci_write_config_word(bridge, pos + PCI_EXP_DEVCTL2, ctrl);
+	pci_pcie_cap_write_word(bridge, PCI_EXP_DEVCTL2, ctrl);
 
 	bridge->ari_enabled = 1;
 }
@@ -2085,20 +2032,16 @@ void pci_enable_ari(struct pci_dev *dev)
  */
 void pci_enable_ido(struct pci_dev *dev, unsigned long type)
 {
-	int pos;
 	u16 ctrl;
 
 	/* ID-based Ordering is a PCIe cap v2 feature */
-	pos = pci_pcie_cap2(dev);
-	if (!pos)
+	if (pci_pcie_cap_read_word(dev, PCI_EXP_DEVCTL2, &ctrl))
 		return;
-
-	pci_read_config_word(dev, pos + PCI_EXP_DEVCTL2, &ctrl);
 	if (type & PCI_EXP_IDO_REQUEST)
 		ctrl |= PCI_EXP_IDO_REQ_EN;
 	if (type & PCI_EXP_IDO_COMPLETION)
 		ctrl |= PCI_EXP_IDO_CMP_EN;
-	pci_write_config_word(dev, pos + PCI_EXP_DEVCTL2, ctrl);
+	pci_pcie_cap_write_word(dev, PCI_EXP_DEVCTL2, ctrl);
 }
 EXPORT_SYMBOL(pci_enable_ido);
 
@@ -2109,20 +2052,16 @@ EXPORT_SYMBOL(pci_enable_ido);
  */
 void pci_disable_ido(struct pci_dev *dev, unsigned long type)
 {
-	int pos;
 	u16 ctrl;
 
 	/* ID-based Ordering is a PCIe cap v2 feature */
-	pos = pci_pcie_cap2(dev);
-	if (!pos)
+	if (pci_pcie_cap_read_word(dev, PCI_EXP_DEVCTL2, &ctrl))
 		return;
-
-	pci_read_config_word(dev, pos + PCI_EXP_DEVCTL2, &ctrl);
 	if (type & PCI_EXP_IDO_REQUEST)
 		ctrl &= ~PCI_EXP_IDO_REQ_EN;
 	if (type & PCI_EXP_IDO_COMPLETION)
 		ctrl &= ~PCI_EXP_IDO_CMP_EN;
-	pci_write_config_word(dev, pos + PCI_EXP_DEVCTL2, ctrl);
+	pci_pcie_cap_write_word(dev, PCI_EXP_DEVCTL2, ctrl);
 }
 EXPORT_SYMBOL(pci_disable_ido);
 
@@ -2147,18 +2086,12 @@ EXPORT_SYMBOL(pci_disable_ido);
  */
 int pci_enable_obff(struct pci_dev *dev, enum pci_obff_signal_type type)
 {
-	int pos;
 	u32 cap;
 	u16 ctrl;
 	int ret;
 
-	/* OBFF is a PCIe cap v2 feature */
-	pos = pci_pcie_cap2(dev);
-	if (!pos)
-		return -ENOTSUPP;
-
-	pci_read_config_dword(dev, pos + PCI_EXP_DEVCAP2, &cap);
-	if (!(cap & PCI_EXP_OBFF_MASK))
+	if (pci_pcie_cap_read_dword(dev, PCI_EXP_DEVCAP2, &cap) ||
+	    !(cap & PCI_EXP_OBFF_MASK))
 		return -ENOTSUPP; /* no OBFF support at all */
 
 	/* Make sure the topology supports OBFF as well */
@@ -2168,7 +2101,7 @@ int pci_enable_obff(struct pci_dev *dev, enum pci_obff_signal_type type)
 			return ret;
 	}
 
-	pci_read_config_word(dev, pos + PCI_EXP_DEVCTL2, &ctrl);
+	pci_pcie_cap_read_word(dev, PCI_EXP_DEVCTL2, &ctrl);
 	if (cap & PCI_EXP_OBFF_WAKE)
 		ctrl |= PCI_EXP_OBFF_WAKE_EN;
 	else {
@@ -2186,7 +2119,7 @@ int pci_enable_obff(struct pci_dev *dev, enum pci_obff_signal_type type)
 			return -ENOTSUPP;
 		}
 	}
-	pci_write_config_word(dev, pos + PCI_EXP_DEVCTL2, ctrl);
+	pci_pcie_cap_write_word(dev, PCI_EXP_DEVCTL2, ctrl);
 
 	return 0;
 }
@@ -2200,17 +2133,13 @@ EXPORT_SYMBOL(pci_enable_obff);
  */
 void pci_disable_obff(struct pci_dev *dev)
 {
-	int pos;
 	u16 ctrl;
 
 	/* OBFF is a PCIe cap v2 feature */
-	pos = pci_pcie_cap2(dev);
-	if (!pos)
-		return;
-
-	pci_read_config_word(dev, pos + PCI_EXP_DEVCTL2, &ctrl);
-	ctrl &= ~PCI_EXP_OBFF_WAKE_EN;
-	pci_write_config_word(dev, pos + PCI_EXP_DEVCTL2, ctrl);
+	if (!pci_pcie_cap_read_word(dev, PCI_EXP_DEVCTL2, &ctrl)) {
+		ctrl &= ~PCI_EXP_OBFF_WAKE_EN;
+		pci_pcie_cap_write_word(dev, PCI_EXP_DEVCTL2, ctrl);
+	}
 }
 EXPORT_SYMBOL(pci_disable_obff);
 
@@ -2223,17 +2152,14 @@ EXPORT_SYMBOL(pci_disable_obff);
  */
 static bool pci_ltr_supported(struct pci_dev *dev)
 {
-	int pos;
 	u32 cap;
 
 	/* LTR is a PCIe cap v2 feature */
-	pos = pci_pcie_cap2(dev);
-	if (!pos)
+	if (pci_pcie_cap_read_dword(dev, PCI_EXP_DEVCAP2, &cap) ||
+	    !(cap & PCI_EXP_DEVCAP2_LTR))
 		return false;
 
-	pci_read_config_dword(dev, pos + PCI_EXP_DEVCAP2, &cap);
-
-	return cap & PCI_EXP_DEVCAP2_LTR;
+	return true;
 }
 
 /**
@@ -2248,22 +2174,16 @@ static bool pci_ltr_supported(struct pci_dev *dev)
  */
 int pci_enable_ltr(struct pci_dev *dev)
 {
-	int pos;
 	u16 ctrl;
 	int ret;
 
-	if (!pci_ltr_supported(dev))
-		return -ENOTSUPP;
-
-	/* LTR is a PCIe cap v2 feature */
-	pos = pci_pcie_cap2(dev);
-	if (!pos)
-		return -ENOTSUPP;
-
 	/* Only primary function can enable/disable LTR */
 	if (PCI_FUNC(dev->devfn) != 0)
 		return -EINVAL;
 
+	if (!pci_ltr_supported(dev))
+		return -ENOTSUPP;
+
 	/* Enable upstream ports first */
 	if (dev->bus->self) {
 		ret = pci_enable_ltr(dev->bus->self);
@@ -2271,9 +2191,10 @@ int pci_enable_ltr(struct pci_dev *dev)
 			return ret;
 	}
 
-	pci_read_config_word(dev, pos + PCI_EXP_DEVCTL2, &ctrl);
+	if (pci_pcie_cap_read_word(dev, PCI_EXP_DEVCTL2, &ctrl))
+		return -ENOTSUPP;
 	ctrl |= PCI_EXP_LTR_EN;
-	pci_write_config_word(dev, pos + PCI_EXP_DEVCTL2, ctrl);
+	pci_pcie_cap_write_word(dev, PCI_EXP_DEVCTL2, ctrl);
 
 	return 0;
 }
@@ -2285,24 +2206,19 @@ EXPORT_SYMBOL(pci_enable_ltr);
  */
 void pci_disable_ltr(struct pci_dev *dev)
 {
-	int pos;
 	u16 ctrl;
 
-	if (!pci_ltr_supported(dev))
-		return;
-
-	/* LTR is a PCIe cap v2 feature */
-	pos = pci_pcie_cap2(dev);
-	if (!pos)
-		return;
-
 	/* Only primary function can enable/disable LTR */
 	if (PCI_FUNC(dev->devfn) != 0)
 		return;
 
-	pci_read_config_word(dev, pos + PCI_EXP_DEVCTL2, &ctrl);
-	ctrl &= ~PCI_EXP_LTR_EN;
-	pci_write_config_word(dev, pos + PCI_EXP_DEVCTL2, ctrl);
+	if (!pci_ltr_supported(dev))
+		return;
+
+	if (!pci_pcie_cap_read_word(dev, PCI_EXP_DEVCTL2, &ctrl)) {
+		ctrl &= ~PCI_EXP_LTR_EN;
+		pci_pcie_cap_write_word(dev, PCI_EXP_DEVCTL2, ctrl);
+	}
 }
 EXPORT_SYMBOL(pci_disable_ltr);
 
@@ -3152,16 +3068,11 @@ EXPORT_SYMBOL(pci_set_dma_seg_boundary);
 static int pcie_flr(struct pci_dev *dev, int probe)
 {
 	int i;
-	int pos;
 	u32 cap;
 	u16 status, control;
 
-	pos = pci_pcie_cap(dev);
-	if (!pos)
-		return -ENOTTY;
-
-	pci_read_config_dword(dev, pos + PCI_EXP_DEVCAP, &cap);
-	if (!(cap & PCI_EXP_DEVCAP_FLR))
+	if (pci_pcie_cap_read_dword(dev, PCI_EXP_DEVCAP, &cap) ||
+	    !(cap & PCI_EXP_DEVCAP_FLR))
 		return -ENOTTY;
 
 	if (probe)
@@ -3172,7 +3083,7 @@ static int pcie_flr(struct pci_dev *dev, int probe)
 		if (i)
 			msleep((1 << (i - 1)) * 100);
 
-		pci_read_config_word(dev, pos + PCI_EXP_DEVSTA, &status);
+		pci_pcie_cap_read_word(dev, PCI_EXP_DEVSTA, &status);
 		if (!(status & PCI_EXP_DEVSTA_TRPND))
 			goto clear;
 	}
@@ -3181,9 +3092,9 @@ static int pcie_flr(struct pci_dev *dev, int probe)
 			"proceeding with reset anyway\n");
 
 clear:
-	pci_read_config_word(dev, pos + PCI_EXP_DEVCTL, &control);
+	pci_pcie_cap_read_word(dev, PCI_EXP_DEVCTL, &control);
 	control |= PCI_EXP_DEVCTL_BCR_FLR;
-	pci_write_config_word(dev, pos + PCI_EXP_DEVCTL, control);
+	pci_pcie_cap_write_word(dev, PCI_EXP_DEVCTL, control);
 
 	msleep(100);
 
@@ -3551,14 +3462,10 @@ EXPORT_SYMBOL(pcix_set_mmrbc);
  */
 int pcie_get_readrq(struct pci_dev *dev)
 {
-	int ret, cap;
+	int ret;
 	u16 ctl;
 
-	cap = pci_pcie_cap(dev);
-	if (!cap)
-		return -EINVAL;
-
-	ret = pci_read_config_word(dev, cap + PCI_EXP_DEVCTL, &ctl);
+	ret = pci_pcie_cap_read_word(dev, PCI_EXP_DEVCTL, &ctl);
 	if (!ret)
 		ret = 128 << ((ctl & PCI_EXP_DEVCTL_READRQ) >> 12);
 
@@ -3576,17 +3483,13 @@ EXPORT_SYMBOL(pcie_get_readrq);
  */
 int pcie_set_readrq(struct pci_dev *dev, int rq)
 {
-	int cap, err = -EINVAL;
+	int err = -EINVAL;
 	u16 ctl, v;
 
 	if (rq < 128 || rq > 4096 || !is_power_of_2(rq))
 		goto out;
 
-	cap = pci_pcie_cap(dev);
-	if (!cap)
-		goto out;
-
-	err = pci_read_config_word(dev, cap + PCI_EXP_DEVCTL, &ctl);
+	err = pci_pcie_cap_read_word(dev, PCI_EXP_DEVCTL, &ctl);
 	if (err)
 		goto out;
 	/*
@@ -3609,7 +3512,7 @@ int pcie_set_readrq(struct pci_dev *dev, int rq)
 	if ((ctl & PCI_EXP_DEVCTL_READRQ) != v) {
 		ctl &= ~PCI_EXP_DEVCTL_READRQ;
 		ctl |= v;
-		err = pci_write_config_word(dev, cap + PCI_EXP_DEVCTL, ctl);
+		err = pci_pcie_cap_write_word(dev, PCI_EXP_DEVCTL, ctl);
 	}
 
 out:
@@ -3626,14 +3529,10 @@ EXPORT_SYMBOL(pcie_set_readrq);
  */
 int pcie_get_mps(struct pci_dev *dev)
 {
-	int ret, cap;
+	int ret;
 	u16 ctl;
 
-	cap = pci_pcie_cap(dev);
-	if (!cap)
-		return -EINVAL;
-
-	ret = pci_read_config_word(dev, cap + PCI_EXP_DEVCTL, &ctl);
+	ret = pci_pcie_cap_read_word(dev, PCI_EXP_DEVCTL, &ctl);
 	if (!ret)
 		ret = 128 << ((ctl & PCI_EXP_DEVCTL_PAYLOAD) >> 5);
 
@@ -3650,7 +3549,7 @@ int pcie_get_mps(struct pci_dev *dev)
  */
 int pcie_set_mps(struct pci_dev *dev, int mps)
 {
-	int cap, err = -EINVAL;
+	int err = -EINVAL;
 	u16 ctl, v;
 
 	if (mps < 128 || mps > 4096 || !is_power_of_2(mps))
@@ -3661,18 +3560,14 @@ int pcie_set_mps(struct pci_dev *dev, int mps)
 		goto out;
 	v <<= 5;
 
-	cap = pci_pcie_cap(dev);
-	if (!cap)
-		goto out;
-
-	err = pci_read_config_word(dev, cap + PCI_EXP_DEVCTL, &ctl);
+	err = pci_pcie_cap_read_word(dev, PCI_EXP_DEVCTL, &ctl);
 	if (err)
 		goto out;
 
 	if ((ctl & PCI_EXP_DEVCTL_PAYLOAD) != v) {
 		ctl &= ~PCI_EXP_DEVCTL_PAYLOAD;
 		ctl |= v;
-		err = pci_write_config_word(dev, cap + PCI_EXP_DEVCTL, ctl);
+		err = pci_pcie_cap_write_word(dev, PCI_EXP_DEVCTL, ctl);
 	}
 out:
 	return err;
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 446933d..5112ff5 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -603,10 +603,10 @@ static void pci_set_bus_speed(struct pci_bus *bus)
 		u32 linkcap;
 		u16 linksta;
 
-		pci_read_config_dword(bridge, pos + PCI_EXP_LNKCAP, &linkcap);
+		pci_pcie_cap_read_dword(bridge, PCI_EXP_LNKCAP, &linkcap);
 		bus->max_bus_speed = pcie_link_speed[linkcap & 0xf];
 
-		pci_read_config_word(bridge, pos + PCI_EXP_LNKSTA, &linksta);
+		pci_pcie_cap_read_word(bridge, PCI_EXP_LNKSTA, &linksta);
 		pcie_update_link_speed(bus, linksta);
 	}
 }
@@ -936,18 +936,10 @@ void set_pcie_port_type(struct pci_dev *pdev)
 
 void set_pcie_hotplug_bridge(struct pci_dev *pdev)
 {
-	int pos;
-	u16 reg16;
 	u32 reg32;
 
-	pos = pci_pcie_cap(pdev);
-	if (!pos)
-		return;
-	pci_read_config_word(pdev, pos + PCI_EXP_FLAGS, &reg16);
-	if (!(reg16 & PCI_EXP_FLAGS_SLOT))
-		return;
-	pci_read_config_dword(pdev, pos + PCI_EXP_SLTCAP, &reg32);
-	if (reg32 & PCI_EXP_SLTCAP_HPC)
+	if (!pci_pcie_cap_read_dword(pdev, PCI_EXP_SLTCAP, &reg32) &&
+	    (reg32 & PCI_EXP_SLTCAP_HPC))
 		pdev->is_hotplug_bridge = 1;
 }
 
@@ -1160,8 +1152,7 @@ int pci_cfg_space_size(struct pci_dev *dev)
 	if (class == PCI_CLASS_BRIDGE_HOST)
 		return pci_cfg_space_size_ext(dev);
 
-	pos = pci_pcie_cap(dev);
-	if (!pos) {
+	if (!pci_is_pcie(dev)) {
 		pos = pci_find_capability(dev, PCI_CAP_ID_PCIX);
 		if (!pos)
 			goto fail;
diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index 52f44b5..7586c24 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -3093,17 +3093,13 @@ static int reset_intel_generic_dev(struct pci_dev *dev, int probe)
 
 static int reset_intel_82599_sfp_virtfn(struct pci_dev *dev, int probe)
 {
-	int pos;
-
-	pos = pci_find_capability(dev, PCI_CAP_ID_EXP);
-	if (!pos)
+	if (!pci_is_pcie(dev))
 		return -ENOTTY;
 
 	if (probe)
 		return 0;
 
-	pci_write_config_word(dev, pos + PCI_EXP_DEVCTL,
-				PCI_EXP_DEVCTL_BCR_FLR);
+	pci_pcie_cap_write_word(dev, PCI_EXP_DEVCTL, PCI_EXP_DEVCTL_BCR_FLR);
 	msleep(100);
 
 	return 0;
-- 
1.7.9.5

[RFC PATCH 07/14] hotplug/PCI: use PCIe cap access functions to simplify implementation

[Jiang Liu]

From: Jiang Liu <jiang.liu@huawei.com>

Use PCIe cap access functions to simplify pcihp_slot.c

Signed-off-by: Jiang Liu <liuj97@gmail.com>
---
 drivers/pci/hotplug/pcihp_slot.c |   17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/drivers/pci/hotplug/pcihp_slot.c b/drivers/pci/hotplug/pcihp_slot.c
index 8c05a18..08ca019 100644
--- a/drivers/pci/hotplug/pcihp_slot.c
+++ b/drivers/pci/hotplug/pcihp_slot.c
@@ -103,8 +103,7 @@ static void program_hpp_type2(struct pci_dev *dev, struct hpp_type2 *hpp)
 		return;
 
 	/* Find PCI Express capability */
-	pos = pci_pcie_cap(dev);
-	if (!pos)
+	if (!pci_is_pcie(dev))
 		return;
 
 	if (hpp->revision > 1) {
@@ -114,16 +113,18 @@ static void program_hpp_type2(struct pci_dev *dev, struct hpp_type2 *hpp)
 	}
 
 	/* Initialize Device Control Register */
-	pci_read_config_word(dev, pos + PCI_EXP_DEVCTL, &reg16);
+	if (pci_pcie_cap_read_word(dev, PCI_EXP_DEVCTL, &reg16))
+		return;
 	reg16 = (reg16 & hpp->pci_exp_devctl_and) | hpp->pci_exp_devctl_or;
-	pci_write_config_word(dev, pos + PCI_EXP_DEVCTL, reg16);
+	pci_pcie_cap_write_word(dev, PCI_EXP_DEVCTL, reg16);
 
 	/* Initialize Link Control Register */
 	if (dev->subordinate) {
-		pci_read_config_word(dev, pos + PCI_EXP_LNKCTL, &reg16);
-		reg16 = (reg16 & hpp->pci_exp_lnkctl_and)
-			| hpp->pci_exp_lnkctl_or;
-		pci_write_config_word(dev, pos + PCI_EXP_LNKCTL, reg16);
+		if (pci_pcie_cap_read_word(dev, PCI_EXP_LNKCTL, &reg16)) {
+			reg16 = (reg16 & hpp->pci_exp_lnkctl_and)
+				| hpp->pci_exp_lnkctl_or;
+			pci_pcie_cap_write_word(dev, PCI_EXP_LNKCTL, reg16);
+		}
 	}
 
 	/* Find Advanced Error Reporting Enhanced Capability */
-- 
1.7.9.5

[RFC PATCH 08/14] portdrv/PCI: use PCIe cap access functions to simplify implementation

[Jiang Liu]

From: Jiang Liu <jiang.liu@huawei.com>

Use PCIe cap access functions to simplify PCIe portdrv implementation

Signed-off-by: Jiang Liu <liuj97@gmail.com>
Signed-off-by: Yijing Wang <wangyijing@huawei.com>
---
 drivers/pci/pcie/portdrv_core.c |   20 +++++++-------------
 drivers/pci/pcie/portdrv_pci.c  |    7 ++-----
 2 files changed, 9 insertions(+), 18 deletions(-)

diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c
index bf320a9..a083e81 100644
--- a/drivers/pci/pcie/portdrv_core.c
+++ b/drivers/pci/pcie/portdrv_core.c
@@ -76,7 +76,6 @@ static int pcie_port_enable_msix(struct pci_dev *dev, int *vectors, int mask)
 	struct msix_entry *msix_entries;
 	int idx[PCIE_PORT_DEVICE_MAXSERVICES];
 	int nr_entries, status, pos, i, nvec;
-	u16 reg16;
 	u32 reg32;
 
 	nr_entries = pci_msix_table_size(dev);
@@ -120,9 +119,7 @@ static int pcie_port_enable_msix(struct pci_dev *dev, int *vectors, int mask)
 		 * the value in this field indicates which MSI-X Table entry is
 		 * used to generate the interrupt message."
 		 */
-		pos = pci_pcie_cap(dev);
-		pci_read_config_word(dev, pos + PCI_EXP_FLAGS, &reg16);
-		entry = (reg16 & PCI_EXP_FLAGS_IRQ) >> 9;
+		entry = (dev->pcie_flags & PCI_EXP_FLAGS_IRQ) >> 9;
 		if (entry >= nr_entries)
 			goto Error;
 
@@ -246,7 +243,7 @@ static void cleanup_service_irqs(struct pci_dev *dev)
  */
 static int get_port_device_capability(struct pci_dev *dev)
 {
-	int services = 0, pos;
+	int services = 0;
 	u16 reg16;
 	u32 reg32;
 	int cap_mask = 0;
@@ -265,11 +262,9 @@ static int get_port_device_capability(struct pci_dev *dev)
 			return 0;
 	}
 
-	pos = pci_pcie_cap(dev);
-	pci_read_config_word(dev, pos + PCI_EXP_FLAGS, &reg16);
 	/* Hot-Plug Capable */
-	if ((cap_mask & PCIE_PORT_SERVICE_HP) && (reg16 & PCI_EXP_FLAGS_SLOT)) {
-		pci_read_config_dword(dev, pos + PCI_EXP_SLTCAP, &reg32);
+	if ((cap_mask & PCIE_PORT_SERVICE_HP) &&
+	    !pci_pcie_cap_read_dword(dev, PCI_EXP_SLTCAP, &reg32))
 		if (reg32 & PCI_EXP_SLTCAP_HPC) {
 			services |= PCIE_PORT_SERVICE_HP;
 			/*
@@ -277,12 +272,11 @@ static int get_port_device_capability(struct pci_dev *dev)
 			 * enabled by the BIOS and the hot-plug service driver
 			 * is not loaded.
 			 */
-			pos += PCI_EXP_SLTCTL;
-			pci_read_config_word(dev, pos, &reg16);
+			pci_pcie_cap_read_word(dev, PCI_EXP_SLTCTL, &reg16);
 			reg16 &= ~(PCI_EXP_SLTCTL_CCIE | PCI_EXP_SLTCTL_HPIE);
-			pci_write_config_word(dev, pos, reg16);
+			pci_pcie_cap_write_word(dev, PCI_EXP_SLTCTL, reg16);
 		}
-	}
+
 	/* AER capable */
 	if ((cap_mask & PCIE_PORT_SERVICE_AER)
 	    && pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ERR)) {
diff --git a/drivers/pci/pcie/portdrv_pci.c b/drivers/pci/pcie/portdrv_pci.c
index 24d1463..3b2c92d 100644
--- a/drivers/pci/pcie/portdrv_pci.c
+++ b/drivers/pci/pcie/portdrv_pci.c
@@ -64,14 +64,11 @@ __setup("pcie_ports=", pcie_port_setup);
  */
 void pcie_clear_root_pme_status(struct pci_dev *dev)
 {
-	int rtsta_pos;
 	u32 rtsta;
 
-	rtsta_pos = pci_pcie_cap(dev) + PCI_EXP_RTSTA;
-
-	pci_read_config_dword(dev, rtsta_pos, &rtsta);
+	pci_pcie_cap_read_dword(dev, PCI_EXP_RTSTA, &rtsta);
 	rtsta |= PCI_EXP_RTSTA_PME;
-	pci_write_config_dword(dev, rtsta_pos, rtsta);
+	pci_pcie_cap_write_dword(dev, PCI_EXP_RTSTA, rtsta);
 }
 
 static int pcie_portdrv_restore_config(struct pci_dev *dev)
-- 
1.7.9.5

[RFC PATCH 09/14] pciehp/PCI: use PCIe cap access functions to simplify implementation

[Jiang Liu]

From: Jiang Liu <jiang.liu@huawei.com>

Use PCIe cap access functions to simplify PCIe hotplug implementation

Signed-off-by: Jiang Liu <liuj97@gmail.com>
Signed-off-by: Yijing Wang <wangyijing@huawei.com>
---
 drivers/pci/hotplug/pciehp_acpi.c |    5 +----
 drivers/pci/hotplug/pciehp_hpc.c  |    8 ++++----
 2 files changed, 5 insertions(+), 8 deletions(-)

diff --git a/drivers/pci/hotplug/pciehp_acpi.c b/drivers/pci/hotplug/pciehp_acpi.c
index 376d70d..6b60301 100644
--- a/drivers/pci/hotplug/pciehp_acpi.c
+++ b/drivers/pci/hotplug/pciehp_acpi.c
@@ -81,16 +81,13 @@ static struct list_head __initdata dummy_slots = LIST_HEAD_INIT(dummy_slots);
 /* Dummy driver for dumplicate name detection */
 static int __init dummy_probe(struct pcie_device *dev)
 {
-	int pos;
 	u32 slot_cap;
 	acpi_handle handle;
 	struct dummy_slot *slot, *tmp;
 	struct pci_dev *pdev = dev->port;
 
-	pos = pci_pcie_cap(pdev);
-	if (!pos)
+	if (pci_pcie_cap_read_dword(pdev, PCI_EXP_SLTCAP, &slot_cap))
 		return -ENODEV;
-	pci_read_config_dword(pdev, pos + PCI_EXP_SLTCAP, &slot_cap);
 	slot = kzalloc(sizeof(*slot), GFP_KERNEL);
 	if (!slot)
 		return -ENOMEM;
diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
index a960fae..d184754 100644
--- a/drivers/pci/hotplug/pciehp_hpc.c
+++ b/drivers/pci/hotplug/pciehp_hpc.c
@@ -44,25 +44,25 @@
 static inline int pciehp_readw(struct controller *ctrl, int reg, u16 *value)
 {
 	struct pci_dev *dev = ctrl->pcie->port;
-	return pci_read_config_word(dev, pci_pcie_cap(dev) + reg, value);
+	return pci_pcie_cap_read_word(dev, reg, value);
 }
 
 static inline int pciehp_readl(struct controller *ctrl, int reg, u32 *value)
 {
 	struct pci_dev *dev = ctrl->pcie->port;
-	return pci_read_config_dword(dev, pci_pcie_cap(dev) + reg, value);
+	return pci_pcie_cap_read_dword(dev, reg, value);
 }
 
 static inline int pciehp_writew(struct controller *ctrl, int reg, u16 value)
 {
 	struct pci_dev *dev = ctrl->pcie->port;
-	return pci_write_config_word(dev, pci_pcie_cap(dev) + reg, value);
+	return pci_pcie_cap_write_word(dev, reg, value);
 }
 
 static inline int pciehp_writel(struct controller *ctrl, int reg, u32 value)
 {
 	struct pci_dev *dev = ctrl->pcie->port;
-	return pci_write_config_dword(dev, pci_pcie_cap(dev) + reg, value);
+	return pci_pcie_cap_write_dword(dev, reg, value);
 }
 
 /* Power Control Command */
-- 
1.7.9.5

[RFC PATCH 10/14] PME/PCI: use PCIe cap access functions to simplify implementation

[Jiang Liu]

From: Jiang Liu <jiang.liu@huawei.com>

Use PCIe cap access functions to simplify PCIe PME implementation

Signed-off-by: Jiang Liu <liuj97@gmail.com>
Signed-off-by: Yijing Wang <wangyijing@huawei.com>
---
 drivers/pci/pcie/pme.c |   17 ++++-------------
 1 file changed, 4 insertions(+), 13 deletions(-)

diff --git a/drivers/pci/pcie/pme.c b/drivers/pci/pcie/pme.c
index 30897bf..d561d16 100644
--- a/drivers/pci/pcie/pme.c
+++ b/drivers/pci/pcie/pme.c
@@ -57,17 +57,14 @@ struct pcie_pme_service_data {
  */
 void pcie_pme_interrupt_enable(struct pci_dev *dev, bool enable)
 {
-	int rtctl_pos;
 	u16 rtctl;
 
-	rtctl_pos = pci_pcie_cap(dev) + PCI_EXP_RTCTL;
-
-	pci_read_config_word(dev, rtctl_pos, &rtctl);
+	pci_pcie_cap_read_word(dev, PCI_EXP_RTCTL, &rtctl);
 	if (enable)
 		rtctl |= PCI_EXP_RTCTL_PMEIE;
 	else
 		rtctl &= ~PCI_EXP_RTCTL_PMEIE;
-	pci_write_config_word(dev, rtctl_pos, rtctl);
+	pci_pcie_cap_write_word(dev, PCI_EXP_RTCTL, rtctl);
 }
 
 /**
@@ -226,18 +223,15 @@ static void pcie_pme_work_fn(struct work_struct *work)
 	struct pcie_pme_service_data *data =
 			container_of(work, struct pcie_pme_service_data, work);
 	struct pci_dev *port = data->srv->port;
-	int rtsta_pos;
 	u32 rtsta;
 
-	rtsta_pos = pci_pcie_cap(port) + PCI_EXP_RTSTA;
-
 	spin_lock_irq(&data->lock);
 
 	for (;;) {
 		if (data->noirq)
 			break;
 
-		pci_read_config_dword(port, rtsta_pos, &rtsta);
+		pci_pcie_cap_read_dword(port, PCI_EXP_RTSTA, &rtsta);
 		if (rtsta & PCI_EXP_RTSTA_PME) {
 			/*
 			 * Clear PME status of the port.  If there are other
@@ -276,17 +270,14 @@ static irqreturn_t pcie_pme_irq(int irq, void *context)
 {
 	struct pci_dev *port;
 	struct pcie_pme_service_data *data;
-	int rtsta_pos;
 	u32 rtsta;
 	unsigned long flags;
 
 	port = ((struct pcie_device *)context)->port;
 	data = get_service_data((struct pcie_device *)context);
 
-	rtsta_pos = pci_pcie_cap(port) + PCI_EXP_RTSTA;
-
 	spin_lock_irqsave(&data->lock, flags);
-	pci_read_config_dword(port, rtsta_pos, &rtsta);
+	pci_pcie_cap_read_dword(port, PCI_EXP_RTSTA, &rtsta);
 
 	if (!(rtsta & PCI_EXP_RTSTA_PME)) {
 		spin_unlock_irqrestore(&data->lock, flags);
-- 
1.7.9.5

[RFC PATCH 11/14] AER/PCI: use PCIe cap access functions to simplify implementation

[Jiang Liu]

From: Jiang Liu <jiang.liu@huawei.com>

Use PCIe cap access functions to simplify PCI AER implementation.

Signed-off-by: Jiang Liu <liuj97@gmail.com>
Signed-off-by: Yijing Wang <wangyijing@huawei.com>
---
 drivers/pci/pcie/aer/aerdrv.c      |   16 ++++++-------
 drivers/pci/pcie/aer/aerdrv_core.c |   45 ++++++++++++++----------------------
 2 files changed, 24 insertions(+), 37 deletions(-)

diff --git a/drivers/pci/pcie/aer/aerdrv.c b/drivers/pci/pcie/aer/aerdrv.c
index f7c6245..27ec1bb 100644
--- a/drivers/pci/pcie/aer/aerdrv.c
+++ b/drivers/pci/pcie/aer/aerdrv.c
@@ -122,19 +122,18 @@ static void set_downstream_devices_error_reporting(struct pci_dev *dev,
 static void aer_enable_rootport(struct aer_rpc *rpc)
 {
 	struct pci_dev *pdev = rpc->rpd->port;
-	int pos, aer_pos;
+	int aer_pos;
 	u16 reg16;
 	u32 reg32;
 
-	pos = pci_pcie_cap(pdev);
 	/* Clear PCIe Capability's Device Status */
-	pci_read_config_word(pdev, pos+PCI_EXP_DEVSTA, &reg16);
-	pci_write_config_word(pdev, pos+PCI_EXP_DEVSTA, reg16);
+	pci_pcie_cap_read_word(pdev, PCI_EXP_DEVSTA, &reg16);
+	pci_pcie_cap_write_word(pdev, PCI_EXP_DEVSTA, reg16);
 
 	/* Disable system error generation in response to error messages */
-	pci_read_config_word(pdev, pos + PCI_EXP_RTCTL, &reg16);
+	pci_pcie_cap_read_word(pdev, PCI_EXP_RTCTL, &reg16);
 	reg16 &= ~(SYSTEM_ERROR_INTR_ON_MESG_MASK);
-	pci_write_config_word(pdev, pos + PCI_EXP_RTCTL, reg16);
+	pci_pcie_cap_write_word(pdev, PCI_EXP_RTCTL, reg16);
 
 	aer_pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_ERR);
 	/* Clear error status */
@@ -396,9 +395,8 @@ static void aer_error_resume(struct pci_dev *dev)
 	u16 reg16;
 
 	/* Clean up Root device status */
-	pos = pci_pcie_cap(dev);
-	pci_read_config_word(dev, pos + PCI_EXP_DEVSTA, &reg16);
-	pci_write_config_word(dev, pos + PCI_EXP_DEVSTA, reg16);
+	pci_pcie_cap_read_word(dev, PCI_EXP_DEVSTA, &reg16);
+	pci_pcie_cap_write_word(dev, PCI_EXP_DEVSTA, reg16);
 
 	/* Clean AER Root Error Status */
 	pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ERR);
diff --git a/drivers/pci/pcie/aer/aerdrv_core.c b/drivers/pci/pcie/aer/aerdrv_core.c
index f551534..5671fce 100644
--- a/drivers/pci/pcie/aer/aerdrv_core.c
+++ b/drivers/pci/pcie/aer/aerdrv_core.c
@@ -35,25 +35,20 @@ module_param(nosourceid, bool, 0);
 int pci_enable_pcie_error_reporting(struct pci_dev *dev)
 {
 	u16 reg16 = 0;
-	int pos;
 
 	if (pcie_aer_get_firmware_first(dev))
 		return -EIO;
 
-	pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ERR);
-	if (!pos)
-		return -EIO;
-
-	pos = pci_pcie_cap(dev);
-	if (!pos)
+	if (!pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ERR))
 		return -EIO;
 
-	pci_read_config_word(dev, pos + PCI_EXP_DEVCTL, &reg16);
-	reg16 |= (PCI_EXP_DEVCTL_CERE |
-		PCI_EXP_DEVCTL_NFERE |
-		PCI_EXP_DEVCTL_FERE |
-		PCI_EXP_DEVCTL_URRE);
-	pci_write_config_word(dev, pos + PCI_EXP_DEVCTL, reg16);
+	if (!pci_pcie_cap_read_word(dev, PCI_EXP_DEVCTL, &reg16)) {
+		reg16 |= (PCI_EXP_DEVCTL_CERE |
+			PCI_EXP_DEVCTL_NFERE |
+			PCI_EXP_DEVCTL_FERE |
+			PCI_EXP_DEVCTL_URRE);
+		pci_pcie_cap_write_word(dev, PCI_EXP_DEVCTL, reg16);
+	}
 
 	return 0;
 }
@@ -62,21 +57,17 @@ EXPORT_SYMBOL_GPL(pci_enable_pcie_error_reporting);
 int pci_disable_pcie_error_reporting(struct pci_dev *dev)
 {
 	u16 reg16 = 0;
-	int pos;
 
 	if (pcie_aer_get_firmware_first(dev))
 		return -EIO;
 
-	pos = pci_pcie_cap(dev);
-	if (!pos)
-		return -EIO;
-
-	pci_read_config_word(dev, pos + PCI_EXP_DEVCTL, &reg16);
-	reg16 &= ~(PCI_EXP_DEVCTL_CERE |
-		PCI_EXP_DEVCTL_NFERE |
-		PCI_EXP_DEVCTL_FERE |
-		PCI_EXP_DEVCTL_URRE);
-	pci_write_config_word(dev, pos + PCI_EXP_DEVCTL, reg16);
+	if (!pci_pcie_cap_read_word(dev, PCI_EXP_DEVCTL, &reg16)) {
+		reg16 &= ~(PCI_EXP_DEVCTL_CERE |
+			PCI_EXP_DEVCTL_NFERE |
+			PCI_EXP_DEVCTL_FERE |
+			PCI_EXP_DEVCTL_URRE);
+		pci_pcie_cap_write_word(dev, PCI_EXP_DEVCTL, reg16);
+	}
 
 	return 0;
 }
@@ -151,18 +142,16 @@ static bool is_error_source(struct pci_dev *dev, struct aer_err_info *e_info)
 	 */
 	if (atomic_read(&dev->enable_cnt) == 0)
 		return false;
-	pos = pci_pcie_cap(dev);
-	if (!pos)
-		return false;
 
 	/* Check if AER is enabled */
-	pci_read_config_word(dev, pos + PCI_EXP_DEVCTL, &reg16);
+	pci_pcie_cap_read_word(dev, PCI_EXP_DEVCTL, &reg16);
 	if (!(reg16 & (
 		PCI_EXP_DEVCTL_CERE |
 		PCI_EXP_DEVCTL_NFERE |
 		PCI_EXP_DEVCTL_FERE |
 		PCI_EXP_DEVCTL_URRE)))
 		return false;
+
 	pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ERR);
 	if (!pos)
 		return false;
-- 
1.7.9.5

[RFC PATCH 12/14] ASPM/PCI: use PCIe cap access functions to simplify implementation

[Jiang Liu]

From: Jiang Liu <jiang.liu@huawei.com>

Use PCIe cap access functions to simplify PCIe ASPM implementation

Signed-off-by: Jiang Liu <liuj97@gmail.com>
Signed-off-by: Yijing Wang <wangyijing@huawei.com>
---
 drivers/pci/pcie/aspm.c |   68 ++++++++++++++++++-----------------------------
 1 file changed, 26 insertions(+), 42 deletions(-)

diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index 2591603..ff75bc5 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -125,21 +125,18 @@ static int policy_to_clkpm_state(struct pcie_link_state *link)
 
 static void pcie_set_clkpm_nocheck(struct pcie_link_state *link, int enable)
 {
-	int pos;
 	u16 reg16;
 	struct pci_dev *child;
 	struct pci_bus *linkbus = link->pdev->subordinate;
 
 	list_for_each_entry(child, &linkbus->devices, bus_list) {
-		pos = pci_pcie_cap(child);
-		if (!pos)
+		if (pci_pcie_cap_read_word(child, PCI_EXP_LNKCTL, &reg16))
 			return;
-		pci_read_config_word(child, pos + PCI_EXP_LNKCTL, &reg16);
 		if (enable)
 			reg16 |= PCI_EXP_LNKCTL_CLKREQ_EN;
 		else
 			reg16 &= ~PCI_EXP_LNKCTL_CLKREQ_EN;
-		pci_write_config_word(child, pos + PCI_EXP_LNKCTL, reg16);
+		pci_pcie_cap_write_word(child, PCI_EXP_LNKCTL, reg16);
 	}
 	link->clkpm_enabled = !!enable;
 }
@@ -157,7 +154,7 @@ static void pcie_set_clkpm(struct pcie_link_state *link, int enable)
 
 static void pcie_clkpm_cap_init(struct pcie_link_state *link, int blacklist)
 {
-	int pos, capable = 1, enabled = 1;
+	int capable = 1, enabled = 1;
 	u32 reg32;
 	u16 reg16;
 	struct pci_dev *child;
@@ -165,16 +162,14 @@ static void pcie_clkpm_cap_init(struct pcie_link_state *link, int blacklist)
 
 	/* All functions should have the same cap and state, take the worst */
 	list_for_each_entry(child, &linkbus->devices, bus_list) {
-		pos = pci_pcie_cap(child);
-		if (!pos)
+		if (pci_pcie_cap_read_dword(child, PCI_EXP_LNKCAP, &reg32))
 			return;
-		pci_read_config_dword(child, pos + PCI_EXP_LNKCAP, &reg32);
 		if (!(reg32 & PCI_EXP_LNKCAP_CLKPM)) {
 			capable = 0;
 			enabled = 0;
 			break;
 		}
-		pci_read_config_word(child, pos + PCI_EXP_LNKCTL, &reg16);
+		pci_pcie_cap_read_word(child, PCI_EXP_LNKCTL, &reg16);
 		if (!(reg16 & PCI_EXP_LNKCTL_CLKREQ_EN))
 			enabled = 0;
 	}
@@ -190,7 +185,7 @@ static void pcie_clkpm_cap_init(struct pcie_link_state *link, int blacklist)
  */
 static void pcie_aspm_configure_common_clock(struct pcie_link_state *link)
 {
-	int ppos, cpos, same_clock = 1;
+	int same_clock = 1;
 	u16 reg16, parent_reg, child_reg[8];
 	unsigned long start_jiffies;
 	struct pci_dev *child, *parent = link->pdev;
@@ -203,46 +198,43 @@ static void pcie_aspm_configure_common_clock(struct pcie_link_state *link)
 	BUG_ON(!pci_is_pcie(child));
 
 	/* Check downstream component if bit Slot Clock Configuration is 1 */
-	cpos = pci_pcie_cap(child);
-	pci_read_config_word(child, cpos + PCI_EXP_LNKSTA, &reg16);
-	if (!(reg16 & PCI_EXP_LNKSTA_SLC))
+	if (pci_pcie_cap_read_word(child, PCI_EXP_LNKSTA, &reg16) ||
+	   !(reg16 & PCI_EXP_LNKSTA_SLC))
 		same_clock = 0;
 
 	/* Check upstream component if bit Slot Clock Configuration is 1 */
-	ppos = pci_pcie_cap(parent);
-	pci_read_config_word(parent, ppos + PCI_EXP_LNKSTA, &reg16);
-	if (!(reg16 & PCI_EXP_LNKSTA_SLC))
+	if (pci_pcie_cap_read_word(parent, PCI_EXP_LNKSTA, &reg16) ||
+	    !(reg16 & PCI_EXP_LNKSTA_SLC))
 		same_clock = 0;
 
 	/* Configure downstream component, all functions */
 	list_for_each_entry(child, &linkbus->devices, bus_list) {
-		cpos = pci_pcie_cap(child);
-		pci_read_config_word(child, cpos + PCI_EXP_LNKCTL, &reg16);
+		pci_pcie_cap_read_word(child, PCI_EXP_LNKCTL, &reg16);
 		child_reg[PCI_FUNC(child->devfn)] = reg16;
 		if (same_clock)
 			reg16 |= PCI_EXP_LNKCTL_CCC;
 		else
 			reg16 &= ~PCI_EXP_LNKCTL_CCC;
-		pci_write_config_word(child, cpos + PCI_EXP_LNKCTL, reg16);
+		pci_pcie_cap_write_word(child, PCI_EXP_LNKCTL, reg16);
 	}
 
 	/* Configure upstream component */
-	pci_read_config_word(parent, ppos + PCI_EXP_LNKCTL, &reg16);
+	pci_pcie_cap_read_word(parent, PCI_EXP_LNKCTL, &reg16);
 	parent_reg = reg16;
 	if (same_clock)
 		reg16 |= PCI_EXP_LNKCTL_CCC;
 	else
 		reg16 &= ~PCI_EXP_LNKCTL_CCC;
-	pci_write_config_word(parent, ppos + PCI_EXP_LNKCTL, reg16);
+	pci_pcie_cap_write_word(parent, PCI_EXP_LNKCTL, reg16);
 
 	/* Retrain link */
 	reg16 |= PCI_EXP_LNKCTL_RL;
-	pci_write_config_word(parent, ppos + PCI_EXP_LNKCTL, reg16);
+	pci_pcie_cap_write_word(parent, PCI_EXP_LNKCTL, reg16);
 
 	/* Wait for link training end. Break out after waiting for timeout */
 	start_jiffies = jiffies;
 	for (;;) {
-		pci_read_config_word(parent, ppos + PCI_EXP_LNKSTA, &reg16);
+		pci_pcie_cap_read_word(parent, PCI_EXP_LNKSTA, &reg16);
 		if (!(reg16 & PCI_EXP_LNKSTA_LT))
 			break;
 		if (time_after(jiffies, start_jiffies + LINK_RETRAIN_TIMEOUT))
@@ -256,11 +248,10 @@ static void pcie_aspm_configure_common_clock(struct pcie_link_state *link)
 	dev_printk(KERN_ERR, &parent->dev,
 		   "ASPM: Could not configure common clock\n");
 	list_for_each_entry(child, &linkbus->devices, bus_list) {
-		cpos = pci_pcie_cap(child);
-		pci_write_config_word(child, cpos + PCI_EXP_LNKCTL,
-				      child_reg[PCI_FUNC(child->devfn)]);
+		pci_pcie_cap_write_word(child, PCI_EXP_LNKCTL,
+					child_reg[PCI_FUNC(child->devfn)]);
 	}
-	pci_write_config_word(parent, ppos + PCI_EXP_LNKCTL, parent_reg);
+	pci_pcie_cap_write_word(parent, PCI_EXP_LNKCTL, parent_reg);
 }
 
 /* Convert L0s latency encoding to ns */
@@ -305,16 +296,14 @@ struct aspm_register_info {
 static void pcie_get_aspm_reg(struct pci_dev *pdev,
 			      struct aspm_register_info *info)
 {
-	int pos;
 	u16 reg16;
 	u32 reg32;
 
-	pos = pci_pcie_cap(pdev);
-	pci_read_config_dword(pdev, pos + PCI_EXP_LNKCAP, &reg32);
+	pci_pcie_cap_read_dword(pdev, PCI_EXP_LNKCAP, &reg32);
 	info->support = (reg32 & PCI_EXP_LNKCAP_ASPMS) >> 10;
 	info->latency_encoding_l0s = (reg32 & PCI_EXP_LNKCAP_L0SEL) >> 12;
 	info->latency_encoding_l1  = (reg32 & PCI_EXP_LNKCAP_L1EL) >> 15;
-	pci_read_config_word(pdev, pos + PCI_EXP_LNKCTL, &reg16);
+	pci_pcie_cap_read_word(pdev, PCI_EXP_LNKCTL, &reg16);
 	info->enabled = reg16 & PCI_EXP_LNKCTL_ASPMC;
 }
 
@@ -420,7 +409,6 @@ static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
 
 	/* Get and check endpoint acceptable latencies */
 	list_for_each_entry(child, &linkbus->devices, bus_list) {
-		int pos;
 		u32 reg32, encoding;
 		struct aspm_latency *acceptable =
 			&link->acceptable[PCI_FUNC(child->devfn)];
@@ -429,8 +417,7 @@ static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
 		    pci_pcie_type(child) != PCI_EXP_TYPE_LEG_END)
 			continue;
 
-		pos = pci_pcie_cap(child);
-		pci_read_config_dword(child, pos + PCI_EXP_DEVCAP, &reg32);
+		pci_pcie_cap_read_dword(child, PCI_EXP_DEVCAP, &reg32);
 		/* Calculate endpoint L0s acceptable latency */
 		encoding = (reg32 & PCI_EXP_DEVCAP_L0S) >> 6;
 		acceptable->l0s = calc_l0s_acceptable(encoding);
@@ -445,12 +432,11 @@ static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
 static void pcie_config_aspm_dev(struct pci_dev *pdev, u32 val)
 {
 	u16 reg16;
-	int pos = pci_pcie_cap(pdev);
 
-	pci_read_config_word(pdev, pos + PCI_EXP_LNKCTL, &reg16);
+	pci_pcie_cap_read_word(pdev, PCI_EXP_LNKCTL, &reg16);
 	reg16 &= ~0x3;
 	reg16 |= val;
-	pci_write_config_word(pdev, pos + PCI_EXP_LNKCTL, reg16);
+	pci_pcie_cap_write_word(pdev, PCI_EXP_LNKCTL, reg16);
 }
 
 static void pcie_config_aspm_link(struct pcie_link_state *link, u32 state)
@@ -505,7 +491,6 @@ static void free_link_state(struct pcie_link_state *link)
 static int pcie_aspm_sanity_check(struct pci_dev *pdev)
 {
 	struct pci_dev *child;
-	int pos;
 	u32 reg32;
 
 	/*
@@ -513,8 +498,7 @@ static int pcie_aspm_sanity_check(struct pci_dev *pdev)
 	 * very strange. Disable ASPM for the whole slot
 	 */
 	list_for_each_entry(child, &pdev->subordinate->devices, bus_list) {
-		pos = pci_pcie_cap(child);
-		if (!pos)
+		if (!pci_is_pcie(child))
 			return -EINVAL;
 
 		/*
@@ -530,7 +514,7 @@ static int pcie_aspm_sanity_check(struct pci_dev *pdev)
 		 * Disable ASPM for pre-1.1 PCIe device, we follow MS to use
 		 * RBER bit to determine if a function is 1.1 version device
 		 */
-		pci_read_config_dword(child, pos + PCI_EXP_DEVCAP, &reg32);
+		pci_pcie_cap_read_dword(child, PCI_EXP_DEVCAP, &reg32);
 		if (!(reg32 & PCI_EXP_DEVCAP_RBER) && !aspm_force) {
 			dev_printk(KERN_INFO, &child->dev, "disabling ASPM"
 				" on pre-1.1 PCIe device.  You can enable it"
-- 
1.7.9.5

[RFC PATCH 13/14] r8169/PCI: use PCIe cap access functions to simplify implementation

[Jiang Liu]

From: Jiang Liu <jiang.liu@huawei.com>

Use PCIe cap access functions to simplify r8169 driver implementation

Signed-off-by: Jiang Liu <liuj97@gmail.com>
Signed-off-by: Yijing Wang <wangyijing@huawei.com>
---
 drivers/net/ethernet/realtek/r8169.c |   38 +++++++++++-----------------------
 1 file changed, 12 insertions(+), 26 deletions(-)

diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
index 9757ce3..c9a7146 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -807,14 +807,11 @@ static void rtl_unlock_work(struct rtl8169_private *tp)
 
 static void rtl_tx_performance_tweak(struct pci_dev *pdev, u16 force)
 {
-	int cap = pci_pcie_cap(pdev);
+	u16 ctl;
 
-	if (cap) {
-		u16 ctl;
-
-		pci_read_config_word(pdev, cap + PCI_EXP_DEVCTL, &ctl);
+	if (!pci_pcie_cap_read_word(pdev, PCI_EXP_DEVCTL, &ctl)) {
 		ctl = (ctl & ~PCI_EXP_DEVCTL_READRQ) | force;
-		pci_write_config_word(pdev, cap + PCI_EXP_DEVCTL, ctl);
+		pci_pcie_cap_write_word(pdev, PCI_EXP_DEVCTL, ctl);
 	}
 }
 
@@ -4504,27 +4501,21 @@ static void rtl_ephy_init(void __iomem *ioaddr, const struct ephy_info *e, int l
 
 static void rtl_disable_clock_request(struct pci_dev *pdev)
 {
-	int cap = pci_pcie_cap(pdev);
-
-	if (cap) {
-		u16 ctl;
+	u16 ctl;
 
-		pci_read_config_word(pdev, cap + PCI_EXP_LNKCTL, &ctl);
+	if (!pci_pcie_cap_read_word(pdev, PCI_EXP_LNKCTL, &ctl)) {
 		ctl &= ~PCI_EXP_LNKCTL_CLKREQ_EN;
-		pci_write_config_word(pdev, cap + PCI_EXP_LNKCTL, ctl);
+		pci_pcie_cap_write_word(pdev, PCI_EXP_LNKCTL, ctl);
 	}
 }
 
 static void rtl_enable_clock_request(struct pci_dev *pdev)
 {
-	int cap = pci_pcie_cap(pdev);
-
-	if (cap) {
-		u16 ctl;
+	u16 ctl;
 
-		pci_read_config_word(pdev, cap + PCI_EXP_LNKCTL, &ctl);
+	if (!pci_pcie_cap_read_word(pdev, PCI_EXP_LNKCTL, &ctl)) {
 		ctl |= PCI_EXP_LNKCTL_CLKREQ_EN;
-		pci_write_config_word(pdev, cap + PCI_EXP_LNKCTL, ctl);
+		pci_pcie_cap_write_word(pdev, PCI_EXP_LNKCTL, ctl);
 	}
 }
 
@@ -5132,14 +5123,9 @@ static void rtl_hw_start_8101(struct net_device *dev)
 		tp->event_slow &= ~RxFIFOOver;
 
 	if (tp->mac_version == RTL_GIGA_MAC_VER_13 ||
-	    tp->mac_version == RTL_GIGA_MAC_VER_16) {
-		int cap = pci_pcie_cap(pdev);
-
-		if (cap) {
-			pci_write_config_word(pdev, cap + PCI_EXP_DEVCTL,
-					      PCI_EXP_DEVCTL_NOSNOOP_EN);
-		}
-	}
+	    tp->mac_version == RTL_GIGA_MAC_VER_16)
+		pci_pcie_cap_write_word(pdev, PCI_EXP_DEVCTL,
+					PCI_EXP_DEVCTL_NOSNOOP_EN);
 
 	RTL_W8(Cfg9346, Cfg9346_Unlock);
 
-- 
1.7.9.5

[RFC PATCH 14/14] qib/PCI: use PCIe cap access functions to simplify implementation

[Jiang Liu]

From: Jiang Liu <jiang.liu@huawei.com>

Use PCIe cap access functions to simplify qib implementation.

Signed-off-by: Jiang Liu <liuj97@gmail.com>
Signed-off-by: Yijing Wang <wangyijing@huawei.com>
---
 drivers/infiniband/hw/qib/qib_pcie.c |   34 ++++++++++++++--------------------
 1 file changed, 14 insertions(+), 20 deletions(-)

diff --git a/drivers/infiniband/hw/qib/qib_pcie.c b/drivers/infiniband/hw/qib/qib_pcie.c
index 790646e..3c30249 100644
--- a/drivers/infiniband/hw/qib/qib_pcie.c
+++ b/drivers/infiniband/hw/qib/qib_pcie.c
@@ -271,10 +271,9 @@ int qib_pcie_params(struct qib_devdata *dd, u32 minw, u32 *nent,
 		    struct qib_msix_entry *entry)
 {
 	u16 linkstat, speed;
-	int pos = 0, pose, ret = 1;
+	int pos = 0, ret = 1;
 
-	pose = pci_pcie_cap(dd->pcidev);
-	if (!pose) {
+	if (!pci_is_pcie(dd->pcidev)) {
 		qib_dev_err(dd, "Can't find PCI Express capability!\n");
 		/* set up something... */
 		dd->lbus_width = 1;
@@ -296,7 +295,7 @@ int qib_pcie_params(struct qib_devdata *dd, u32 minw, u32 *nent,
 	if (!pos)
 		qib_enable_intx(dd->pcidev);
 
-	pci_read_config_word(dd->pcidev, pose + PCI_EXP_LNKSTA, &linkstat);
+	pci_pcie_cap_read_word(dd->pcidev, PCI_EXP_LNKSTA, &linkstat);
 	/*
 	 * speed is bits 0-3, linkwidth is bits 4-8
 	 * no defines for them in headers
@@ -514,7 +513,6 @@ static int qib_tune_pcie_coalesce(struct qib_devdata *dd)
 {
 	int r;
 	struct pci_dev *parent;
-	int ppos;
 	u16 devid;
 	u32 mask, bits, val;
 
@@ -527,8 +525,7 @@ static int qib_tune_pcie_coalesce(struct qib_devdata *dd)
 		qib_devinfo(dd->pcidev, "Parent not root\n");
 		return 1;
 	}
-	ppos = pci_pcie_cap(parent);
-	if (!ppos)
+	if (!pci_is_pcie(parent))
 		return 1;
 	if (parent->vendor != 0x8086)
 		return 1;
@@ -585,7 +582,6 @@ static int qib_tune_pcie_caps(struct qib_devdata *dd)
 {
 	int ret = 1; /* Assume the worst */
 	struct pci_dev *parent;
-	int ppos, epos;
 	u16 pcaps, pctl, ecaps, ectl;
 	int rc_sup, ep_sup;
 	int rc_cur, ep_cur;
@@ -596,17 +592,15 @@ static int qib_tune_pcie_caps(struct qib_devdata *dd)
 		qib_devinfo(dd->pcidev, "Parent not root\n");
 		goto bail;
 	}
-	ppos = pci_pcie_cap(parent);
-	if (ppos) {
-		pci_read_config_word(parent, ppos + PCI_EXP_DEVCAP, &pcaps);
-		pci_read_config_word(parent, ppos + PCI_EXP_DEVCTL, &pctl);
+	if (pci_is_pcie(parent)) {
+		pci_pcie_cap_read_word(parent, PCI_EXP_DEVCAP, &pcaps);
+		pci_pcie_cap_read_word(parent, PCI_EXP_DEVCTL, &pctl);
 	} else
 		goto bail;
 	/* Find out supported and configured values for endpoint (us) */
-	epos = pci_pcie_cap(dd->pcidev);
-	if (epos) {
-		pci_read_config_word(dd->pcidev, epos + PCI_EXP_DEVCAP, &ecaps);
-		pci_read_config_word(dd->pcidev, epos + PCI_EXP_DEVCTL, &ectl);
+	if (pci_is_pcie(dd->pcidev)) {
+		pci_pcie_cap_read_word(dd->pcidev, PCI_EXP_DEVCAP, &ecaps);
+		pci_pcie_cap_read_word(dd->pcidev, PCI_EXP_DEVCTL, &ectl);
 	} else
 		goto bail;
 	ret = 0;
@@ -627,14 +621,14 @@ static int qib_tune_pcie_caps(struct qib_devdata *dd)
 		rc_cur = rc_sup;
 		pctl = (pctl & ~PCI_EXP_DEVCTL_PAYLOAD) |
 			val2fld(rc_cur, PCI_EXP_DEVCTL_PAYLOAD);
-		pci_write_config_word(parent, ppos + PCI_EXP_DEVCTL, pctl);
+		pci_pcie_cap_write_word(parent, PCI_EXP_DEVCTL, pctl);
 	}
 	/* If less than (allowed, supported), bump endpoint payload */
 	if (rc_sup > ep_cur) {
 		ep_cur = rc_sup;
 		ectl = (ectl & ~PCI_EXP_DEVCTL_PAYLOAD) |
 			val2fld(ep_cur, PCI_EXP_DEVCTL_PAYLOAD);
-		pci_write_config_word(dd->pcidev, epos + PCI_EXP_DEVCTL, ectl);
+		pci_pcie_cap_write_word(dd->pcidev, PCI_EXP_DEVCTL, ectl);
 	}
 
 	/*
@@ -652,13 +646,13 @@ static int qib_tune_pcie_caps(struct qib_devdata *dd)
 		rc_cur = rc_sup;
 		pctl = (pctl & ~PCI_EXP_DEVCTL_READRQ) |
 			val2fld(rc_cur, PCI_EXP_DEVCTL_READRQ);
-		pci_write_config_word(parent, ppos + PCI_EXP_DEVCTL, pctl);
+		pci_pcie_cap_write_word(parent, PCI_EXP_DEVCTL, pctl);
 	}
 	if (rc_sup > ep_cur) {
 		ep_cur = rc_sup;
 		ectl = (ectl & ~PCI_EXP_DEVCTL_READRQ) |
 			val2fld(ep_cur, PCI_EXP_DEVCTL_READRQ);
-		pci_write_config_word(dd->pcidev, epos + PCI_EXP_DEVCTL, ectl);
+		pci_pcie_cap_write_word(dd->pcidev, PCI_EXP_DEVCTL, ectl);
 	}
 bail:
 	return ret;
-- 
1.7.9.5

Re: [RFC PATCH 05/14] PCI: add access functions for PCIe capabilities to hide PCIe spec differences

[Bjorn Helgaas]

On Tue, Jul 10, 2012 at 9:54 AM, Jiang Liu <liuj97@gmail.com> wrote:
> From: Jiang Liu <jiang.liu@huawei.com>
>
> Introduce four configuration access functions for PCIe capabilities to
> hide difference among PCIe Base Spec versions. With these functions,
> we can remove callers responsible for using pci_pcie_cap_has_*().
>
> pci_pcie_cap_read_word/dword() functions will store the pcie cap register
> value by passed parameter val,if related pcie cap register is not implemented
> on the pcie device, the passed parameter val will be set 0 and return -EINVAL.
>
> pci_pcie_capability_write_word/dowrd() functions will write the value to pcie
> cap registers,if related pcie cap register is not implemented on the pcie
> device, it will return -EINVAL.
>
> Signed-off-by: Jiang Liu <liuj97@gmail.com>
> Signed-off-by: Yijing Wang <wangyijing@huawei.com>
> ---
>  drivers/pci/access.c     |   88 ++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/pci.h      |   10 ++++++
>  include/linux/pci_regs.h |   19 ++++++++--
>  3 files changed, 115 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/pci/access.c b/drivers/pci/access.c
> index ba91a7e..80ae022 100644
> --- a/drivers/pci/access.c
> +++ b/drivers/pci/access.c
> @@ -469,3 +469,91 @@ void pci_cfg_access_unlock(struct pci_dev *dev)
>         raw_spin_unlock_irqrestore(&pci_lock, flags);
>  }
>  EXPORT_SYMBOL_GPL(pci_cfg_access_unlock);
> +
> +static int
> +pci_pcie_cap_get_offset(struct pci_dev *dev, int where, size_t sz)
> +{
> +       bool valid;
> +
> +       if (!pci_is_pcie(dev))
> +               return -EINVAL;
> +       if (where & (sz - 1))
> +               return -EINVAL;
> +
> +       if (where < 0)
> +               valid = false;
> +       else if (where < PCI_EXP_DEVCAP)
> +               valid = true;
> +       else if (where < PCI_EXP_LNKCAP)
> +               valid = pci_pcie_cap_has_devctl(dev);
> +       else if (where < PCI_EXP_SLTCAP)
> +               valid = pci_pcie_cap_has_lnkctl(dev);
> +       else if (where < PCI_EXP_RTCTL)
> +               valid = pci_pcie_cap_has_sltctl(dev);
> +       else if (where < PCI_EXP_DEVCAP2)
> +               valid = pci_pcie_cap_has_rtctl(dev);
> +       else if (where < PCI_EXP_CAP2_SIZE)
> +               valid = pci_pcie_cap_has_cap2(dev);
> +       else
> +               valid = false;
> +
> +       return valid ? where + pci_pcie_cap(dev) : -EINVAL;
> +}
> +
> +int pci_pcie_cap_read_word(struct pci_dev *dev, int where, u16 *valp)
> +{
> +       *valp = 0;
> +       where = pci_pcie_cap_get_offset(dev, where, sizeof(u16));

This is a really slick factorization; I like it much better than my
proposal.  I would like it even *better* if it read something like
this:

    bool implemented;

    *valp = 0;
    if (!pci_is_pcie(dev) || where & 1)
        return -EINVAL;

    implemented = pci_pcie_cap_implemented(dev, where);
    if (implemented)
        return pci_read_config_word(dev, pci_pcie_cap(dev) + where, valp);

    if (pci_is_pcie(dev) && where == PCI_EXP_SLTSTA ...

because I think it's useful to have the "pos + where" visual pattern
in the pci_read_config_word() arguments.

> +       if (where >= 0)
> +               return pci_read_config_word(dev, where, valp);
> +
> +       if (pci_is_pcie(dev) && where == PCI_EXP_SLTSTA &&
> +           pci_pcie_type(dev) == PCI_EXP_TYPE_DOWNSTREAM)
> +               *valp = PCI_EXP_SLTSTA_PDS;

I think we should be returning success in this case (SLTSTA for
downstream port).  In fact, I think we should return success even when
we're emulating the read of an unimplemented register from a v1
capability.  The caller should not be aware at all that there is a
difference between v1 and v2 capabilities.

I'd put the spec reference here rather than in read_dword(), since
SLTSTA is a u16 and this is the natural way to read it.  Then maybe a
short comment in read_dword() below.

> +       return -EINVAL;
> +}
> +EXPORT_SYMBOL(pci_pcie_cap_read_word);
> +
> +int pci_pcie_cap_read_dword(struct pci_dev *dev, int where, u32 *valp)
> +{
> +       *valp = 0;
> +       where = pci_pcie_cap_get_offset(dev, where, sizeof(u32));
> +       if (where >= 0)
> +               return pci_read_config_dword(dev, where, valp);
> +
> +       /*
> +        * Quotation from PCIe Base Spec 3.0:
> +        * For Functions that do not implement the Slot Capabilities,
> +        * Slot Status, and Slot Control registers, these spaces must
> +        * be hardwired to 0b, with the exception of the Presence Detect
> +        * State bit in the Slot Status register of Downstream Ports,
> +        * which must be hardwired to 1b.
> +        */
> +       if (pci_is_pcie(dev) && where == PCI_EXP_SLTCTL &&
> +           pci_pcie_type(dev) == PCI_EXP_TYPE_DOWNSTREAM)
> +               *valp = PCI_EXP_SLTSTA_PDS << 16;

Return success here, too.

> +
> +       return -EINVAL;
> +}
> +EXPORT_SYMBOL(pci_pcie_cap_read_dword);
> +
> +int pci_pcie_cap_write_word(struct pci_dev *dev, int where, u16 val)
> +{
> +       where = pci_pcie_cap_get_offset(dev, where, sizeof(u16));
> +       if (where >= 0)
> +               return pci_write_config_word(dev, where, val);
> +
> +       return -EINVAL;
> +}
> +EXPORT_SYMBOL(pci_pcie_cap_write_word);
> +
> +int pci_pcie_cap_write_dword(struct pci_dev *dev, int where, u32 val)
> +{
> +       where = pci_pcie_cap_get_offset(dev, where, sizeof(u32));
> +       if (where >= 0)
> +               return pci_write_config_dword(dev, where, val);
> +
> +       return -EINVAL;
> +}
> +EXPORT_SYMBOL(pci_pcie_cap_write_dword);
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 346b2d9..78767b2 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -1703,6 +1703,11 @@ static inline bool pci_pcie_cap_has_rtctl(const struct pci_dev *pdev)
>                type == PCI_EXP_TYPE_RC_EC;
>  }
>
> +extern int pci_pcie_cap_read_word(struct pci_dev *dev, int where, u16 *valp);
> +extern int pci_pcie_cap_read_dword(struct pci_dev *dev, int where, u32 *valp);
> +extern int pci_pcie_cap_write_word(struct pci_dev *dev, int where, u16 val);
> +extern int pci_pcie_cap_write_dword(struct pci_dev *dev, int where, u32 val);

You don't need the "extern" here (and I think you'll probably remove
these altogether, see below).

> +
>  void pci_request_acs(void);
>  bool pci_acs_enabled(struct pci_dev *pdev, u16 acs_flags);
>  bool pci_acs_path_enabled(struct pci_dev *start,
> @@ -1843,5 +1848,10 @@ static inline struct eeh_dev *pci_dev_to_eeh_dev(struct pci_dev *pdev)
>   */
>  struct pci_dev *pci_find_upstream_pcie_bridge(struct pci_dev *pdev);
>
> +int pci_pcie_capability_read_word(struct pci_dev *dev, int where, u16 *val);
> +int pci_pcie_capability_read_dword(struct pci_dev *dev, int where, u32 *val);
> +int pci_pcie_capability_write_word(struct pci_dev *dev, int where, u16 val);
> +int pci_pcie_capability_write_dword(struct pci_dev *dev, int where, u32 val);

There's some confusion here: pci_pcie_cap_* versus
pci_pcie_capability_*.  I think you only need one set, and I prefer
pci_pcie_capability_* to follow the example of
pci_bus_find_capability().

> +
>  #endif /* __KERNEL__ */
>  #endif /* LINUX_PCI_H */
> diff --git a/include/linux/pci_regs.h b/include/linux/pci_regs.h
> index 53274bf..ac60e22 100644
> --- a/include/linux/pci_regs.h
> +++ b/include/linux/pci_regs.h
> @@ -542,9 +542,24 @@
>  #define  PCI_EXP_OBFF_MSGA_EN  0x2000  /* OBFF enable with Message type A */
>  #define  PCI_EXP_OBFF_MSGB_EN  0x4000  /* OBFF enable with Message type B */
>  #define  PCI_EXP_OBFF_WAKE_EN  0x6000  /* OBFF using WAKE# signaling */
> -#define PCI_CAP_EXP_ENDPOINT_SIZEOF_V2 44      /* v2 endpoints end here */
> +#define PCI_EXP_DEVSTA2                42      /* Device Status 2 */
> +#define PCI_CAP_EXP_ENDPOINT_SIZEOF_V2 44 /* v2 endpoints end here */
> +#define PCI_EXP_LNKCAP2                44      /* Link Capabilities 2 */
>  #define PCI_EXP_LNKCTL2                48      /* Link Control 2 */
> -#define PCI_EXP_SLTCTL2                56      /* Slot Control 2 */
> +#define  PCI_EXP_LNKCTL2_TLS   0x0f    /* Target Link Speed */
> +#define  PCI_EXP_LNKCTL2_EC    0x10    /* Enter Compliance */
> +#define  PCI_EXP_LNKCTL2_HASD  0x20    /* Hardware Autonomous Speed Disable */
> +#define  PCI_EXP_LNKCTL2_SD    0x40    /* Selectable De-emphasis */
> +#define  PCI_EXP_LNKCTL2_TM    0x380   /* Transmit Margin */
> +#define  PCI_EXP_LNKCTL2_EMC   0x400   /* Enter Modified Compliance */
> +#define  PCI_EXP_LNKCTL2_CS    0x800   /* Compliance SOS */
> +#define  PCI_EXP_LNKCTL2_CD    0x1000  /* Compliance De-emphasis */
> +#define PCI_EXP_LNKSTA2                50      /* Link Status 2 */
> +#define  PCI_EXP_LNKSTA2_CDL   0x01    /* Current De-emphasis Level */
> +#define PCI_EXP_SLTCAP2                52      /* Slot Capabilities 2 */
> +#define PCI_EXP_SLTCTL2                56      /* Slot Control 2*/
> +#define PCI_EXP_SLTSTA2                58      /* Slot Status 2*/
> +#define        PCI_EXP_CAP2_SIZE       60

Most of these changes look unrelated to the current patch.  They
should be moved to a patch that uses the symbols you're adding.

>
>  /* Extended Capabilities (PCI-X 2.0 and Express) */
>  #define PCI_EXT_CAP_ID(header)         (header & 0x0000ffff)
> --
> 1.7.9.5
>

Re: [RFC PATCH 06/14] PCI: use PCIe cap access functions to simplify PCI core implementation

[Bjorn Helgaas]

On Tue, Jul 10, 2012 at 9:54 AM, Jiang Liu <liuj97@gmail.com> wrote:
> From: Jiang Liu <jiang.liu@huawei.com>
>
> Use PCIe cap access functions to simplify PCI core implementation.
>
> Signed-off-by: Jiang Liu <liuj97@gmail.com>
> ---
>  drivers/pci/pci.c    |  237 ++++++++++++++------------------------------------
>  drivers/pci/probe.c  |   19 ++--
>  drivers/pci/quirks.c |    8 +-
>  3 files changed, 73 insertions(+), 191 deletions(-)

Nice!

> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 4c6ab70..b8142f1 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -254,34 +254,6 @@ int pci_bus_find_capability(struct pci_bus *bus, unsigned int devfn, int cap)
>  }
>
>  /**
> - * pci_pcie_cap2 - query for devices' PCI_CAP_ID_EXP v2 capability structure
> - * @dev: PCI device to check
> - *
> - * Like pci_pcie_cap() but also checks that the PCIe capability version is
> - * >= 2.  Note that v1 capability structures could be sparse in that not
> - * all register fields were required.  v2 requires the entire structure to
> - * be present size wise, while still allowing for non-implemented registers
> - * to exist but they must be hardwired to 0.
> - *
> - * Due to the differences in the versions of capability structures, one
> - * must be careful not to try and access non-existant registers that may
> - * exist in early versions - v1 - of Express devices.
> - *
> - * Returns the offset of the PCIe capability structure as long as the
> - * capability version is >= 2; otherwise 0 is returned.
> - */
> -static int pci_pcie_cap2(struct pci_dev *dev)
> -{
> -       int pos;
> -
> -       pos = pci_pcie_cap(dev);
> -       if (pos && !pci_pcie_cap_has_cap2(dev))
> -               pos = 0;
> -
> -       return pos;
> -}
> -
> -/**
>   * pci_find_ext_capability - Find an extended capability
>   * @dev: PCI device to query
>   * @cap: capability code
> @@ -866,12 +838,11 @@ static struct pci_cap_saved_state *pci_find_saved_cap(
>
>  static int pci_save_pcie_state(struct pci_dev *dev)
>  {
> -       int pos, i = 0;
> +       int i = 0;
>         struct pci_cap_saved_state *save_state;
>         u16 *cap;
>
> -       pos = pci_pcie_cap(dev);
> -       if (!pos)
> +       if (!pci_is_pcie(dev))
>                 return 0;
>
>         save_state = pci_find_saved_cap(dev, PCI_CAP_ID_EXP);
> @@ -881,54 +852,35 @@ static int pci_save_pcie_state(struct pci_dev *dev)
>         }
>         cap = (u16 *)&save_state->cap.data[0];
>
> +       pci_pcie_cap_read_word(dev, PCI_EXP_DEVCTL, &cap[i++]);
> +       pci_pcie_cap_read_word(dev, PCI_EXP_LNKCTL, &cap[i++]);
> +       pci_pcie_cap_read_word(dev, PCI_EXP_SLTCTL, &cap[i++]);
> +       pci_pcie_cap_read_word(dev, PCI_EXP_RTCTL,  &cap[i++]);
> +       pci_pcie_cap_read_word(dev, PCI_EXP_DEVCTL2, &cap[i++]);
> +       pci_pcie_cap_read_word(dev, PCI_EXP_LNKCTL2, &cap[i++]);
> +       pci_pcie_cap_read_word(dev, PCI_EXP_SLTCTL2, &cap[i++]);
>
> -       if (pci_pcie_cap_has_devctl(dev))
> -               pci_read_config_word(dev, pos + PCI_EXP_DEVCTL, &cap[i++]);
> -       if (pci_pcie_cap_has_lnkctl(dev))
> -               pci_read_config_word(dev, pos + PCI_EXP_LNKCTL, &cap[i++]);
> -       if (pci_pcie_cap_has_sltctl(dev))
> -               pci_read_config_word(dev, pos + PCI_EXP_SLTCTL, &cap[i++]);
> -       if (pci_pcie_cap_has_rtctl(dev))
> -               pci_read_config_word(dev, pos + PCI_EXP_RTCTL, &cap[i++]);
> -
> -       pos = pci_pcie_cap2(dev);
> -       if (!pos)
> -               return 0;
> -
> -       pci_read_config_word(dev, pos + PCI_EXP_DEVCTL2, &cap[i++]);
> -       pci_read_config_word(dev, pos + PCI_EXP_LNKCTL2, &cap[i++]);
> -       pci_read_config_word(dev, pos + PCI_EXP_SLTCTL2, &cap[i++]);

Very nice!  I love this simplification.

>         return 0;
>  }
>
>  static void pci_restore_pcie_state(struct pci_dev *dev)
>  {
> -       int i = 0, pos;
> +       int i = 0;
>         struct pci_cap_saved_state *save_state;
>         u16 *cap;
>
>         save_state = pci_find_saved_cap(dev, PCI_CAP_ID_EXP);
> -       pos = pci_find_capability(dev, PCI_CAP_ID_EXP);
> -       if (!save_state || pos <= 0)
> +       if (!save_state)
>                 return;
>         cap = (u16 *)&save_state->cap.data[0];
>
> -       if (pci_pcie_cap_has_devctl(dev))
> -               pci_write_config_word(dev, pos + PCI_EXP_DEVCTL, cap[i++]);
> -       if (pci_pcie_cap_has_lnkctl(dev))
> -               pci_write_config_word(dev, pos + PCI_EXP_LNKCTL, cap[i++]);
> -       if (pci_pcie_cap_has_sltctl(dev))
> -               pci_write_config_word(dev, pos + PCI_EXP_SLTCTL, cap[i++]);
> -       if (pci_pcie_cap_has_rtctl(dev))
> -               pci_write_config_word(dev, pos + PCI_EXP_RTCTL, cap[i++]);
> -
> -       pos = pci_pcie_cap2(dev);
> -       if (!pos)
> -               return;
> -
> -       pci_write_config_word(dev, pos + PCI_EXP_DEVCTL2, cap[i++]);
> -       pci_write_config_word(dev, pos + PCI_EXP_LNKCTL2, cap[i++]);
> -       pci_write_config_word(dev, pos + PCI_EXP_SLTCTL2, cap[i++]);
> +       pci_pcie_cap_write_word(dev, PCI_EXP_DEVCTL, cap[i++]);
> +       pci_pcie_cap_write_word(dev, PCI_EXP_LNKCTL, cap[i++]);
> +       pci_pcie_cap_write_word(dev, PCI_EXP_SLTCTL, cap[i++]);
> +       pci_pcie_cap_write_word(dev, PCI_EXP_RTCTL,  cap[i++]);
> +       pci_pcie_cap_write_word(dev, PCI_EXP_DEVCTL2, cap[i++]);
> +       pci_pcie_cap_write_word(dev, PCI_EXP_LNKCTL2, cap[i++]);
> +       pci_pcie_cap_write_word(dev, PCI_EXP_SLTCTL2, cap[i++]);
>  }
>
>
> @@ -2042,7 +1994,6 @@ void pci_free_cap_save_buffers(struct pci_dev *dev)
>   */
>  void pci_enable_ari(struct pci_dev *dev)
>  {
> -       int pos;
>         u32 cap;
>         u16 ctrl;
>         struct pci_dev *bridge;
> @@ -2050,8 +2001,7 @@ void pci_enable_ari(struct pci_dev *dev)
>         if (pcie_ari_disabled || !pci_is_pcie(dev) || dev->devfn)
>                 return;
>
> -       pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ARI);
> -       if (!pos)
> +       if (pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ARI))

What's going on here?  This looks wrong, and unrelated to the rest of the patch.

>                 return;
>
>         bridge = dev->bus->self;
> @@ -2059,17 +2009,14 @@ void pci_enable_ari(struct pci_dev *dev)
>                 return;
>
>         /* ARI is a PCIe cap v2 feature */

If we're doing this right, we should be able to remove this comment
(and similar ones below).

> -       pos = pci_pcie_cap2(bridge);
> -       if (!pos)
> +       if (pci_pcie_cap_read_dword(bridge, PCI_EXP_DEVCAP2, &cap) ||
> +          !(cap & PCI_EXP_DEVCAP2_ARI))

I don't think there's any point in checking every return from
pci_pcie_cap_read_dword().  We've already checked pci_is_pcie() above,
and checking here just clutters the code.  In cases like this, my
opinion is that clean code leads to fewer bugs, and that benefit
outweighs whatever technical "every return must be checked" benefit
there might be.

>                 return;
>
> -       pci_read_config_dword(bridge, pos + PCI_EXP_DEVCAP2, &cap);
> -       if (!(cap & PCI_EXP_DEVCAP2_ARI))
> +       if (pci_pcie_cap_read_word(bridge, PCI_EXP_DEVCTL2, &ctrl))
>                 return;
> -
> -       pci_read_config_word(bridge, pos + PCI_EXP_DEVCTL2, &ctrl);
>         ctrl |= PCI_EXP_DEVCTL2_ARI;
> -       pci_write_config_word(bridge, pos + PCI_EXP_DEVCTL2, ctrl);
> +       pci_pcie_cap_write_word(bridge, PCI_EXP_DEVCTL2, ctrl);
>
>         bridge->ari_enabled = 1;
>  }
> @@ -2085,20 +2032,16 @@ void pci_enable_ari(struct pci_dev *dev)
>   */
>  void pci_enable_ido(struct pci_dev *dev, unsigned long type)
>  {
> -       int pos;
>         u16 ctrl;
>
>         /* ID-based Ordering is a PCIe cap v2 feature */
> -       pos = pci_pcie_cap2(dev);
> -       if (!pos)
> +       if (pci_pcie_cap_read_word(dev, PCI_EXP_DEVCTL2, &ctrl))
>                 return;
> -
> -       pci_read_config_word(dev, pos + PCI_EXP_DEVCTL2, &ctrl);
>         if (type & PCI_EXP_IDO_REQUEST)
>                 ctrl |= PCI_EXP_IDO_REQ_EN;
>         if (type & PCI_EXP_IDO_COMPLETION)
>                 ctrl |= PCI_EXP_IDO_CMP_EN;
> -       pci_write_config_word(dev, pos + PCI_EXP_DEVCTL2, ctrl);
> +       pci_pcie_cap_write_word(dev, PCI_EXP_DEVCTL2, ctrl);
>  }
>  EXPORT_SYMBOL(pci_enable_ido);
>
> @@ -2109,20 +2052,16 @@ EXPORT_SYMBOL(pci_enable_ido);
>   */
>  void pci_disable_ido(struct pci_dev *dev, unsigned long type)
>  {
> -       int pos;
>         u16 ctrl;
>
>         /* ID-based Ordering is a PCIe cap v2 feature */
> -       pos = pci_pcie_cap2(dev);
> -       if (!pos)
> +       if (pci_pcie_cap_read_word(dev, PCI_EXP_DEVCTL2, &ctrl))
>                 return;
> -
> -       pci_read_config_word(dev, pos + PCI_EXP_DEVCTL2, &ctrl);
>         if (type & PCI_EXP_IDO_REQUEST)
>                 ctrl &= ~PCI_EXP_IDO_REQ_EN;
>         if (type & PCI_EXP_IDO_COMPLETION)
>                 ctrl &= ~PCI_EXP_IDO_CMP_EN;
> -       pci_write_config_word(dev, pos + PCI_EXP_DEVCTL2, ctrl);
> +       pci_pcie_cap_write_word(dev, PCI_EXP_DEVCTL2, ctrl);
>  }
>  EXPORT_SYMBOL(pci_disable_ido);
>
> @@ -2147,18 +2086,12 @@ EXPORT_SYMBOL(pci_disable_ido);
>   */
>  int pci_enable_obff(struct pci_dev *dev, enum pci_obff_signal_type type)
>  {
> -       int pos;
>         u32 cap;
>         u16 ctrl;
>         int ret;
>
> -       /* OBFF is a PCIe cap v2 feature */
> -       pos = pci_pcie_cap2(dev);
> -       if (!pos)
> -               return -ENOTSUPP;
> -
> -       pci_read_config_dword(dev, pos + PCI_EXP_DEVCAP2, &cap);
> -       if (!(cap & PCI_EXP_OBFF_MASK))
> +       if (pci_pcie_cap_read_dword(dev, PCI_EXP_DEVCAP2, &cap) ||
> +           !(cap & PCI_EXP_OBFF_MASK))
>                 return -ENOTSUPP; /* no OBFF support at all */
>
>         /* Make sure the topology supports OBFF as well */
> @@ -2168,7 +2101,7 @@ int pci_enable_obff(struct pci_dev *dev, enum pci_obff_signal_type type)
>                         return ret;
>         }
>
> -       pci_read_config_word(dev, pos + PCI_EXP_DEVCTL2, &ctrl);
> +       pci_pcie_cap_read_word(dev, PCI_EXP_DEVCTL2, &ctrl);
>         if (cap & PCI_EXP_OBFF_WAKE)
>                 ctrl |= PCI_EXP_OBFF_WAKE_EN;
>         else {
> @@ -2186,7 +2119,7 @@ int pci_enable_obff(struct pci_dev *dev, enum pci_obff_signal_type type)
>                         return -ENOTSUPP;
>                 }
>         }
> -       pci_write_config_word(dev, pos + PCI_EXP_DEVCTL2, ctrl);
> +       pci_pcie_cap_write_word(dev, PCI_EXP_DEVCTL2, ctrl);
>
>         return 0;
>  }
> @@ -2200,17 +2133,13 @@ EXPORT_SYMBOL(pci_enable_obff);
>   */
>  void pci_disable_obff(struct pci_dev *dev)
>  {
> -       int pos;
>         u16 ctrl;
>
>         /* OBFF is a PCIe cap v2 feature */
> -       pos = pci_pcie_cap2(dev);
> -       if (!pos)
> -               return;
> -
> -       pci_read_config_word(dev, pos + PCI_EXP_DEVCTL2, &ctrl);
> -       ctrl &= ~PCI_EXP_OBFF_WAKE_EN;
> -       pci_write_config_word(dev, pos + PCI_EXP_DEVCTL2, ctrl);
> +       if (!pci_pcie_cap_read_word(dev, PCI_EXP_DEVCTL2, &ctrl)) {
> +               ctrl &= ~PCI_EXP_OBFF_WAKE_EN;
> +               pci_pcie_cap_write_word(dev, PCI_EXP_DEVCTL2, ctrl);
> +       }
>  }
>  EXPORT_SYMBOL(pci_disable_obff);
>
> @@ -2223,17 +2152,14 @@ EXPORT_SYMBOL(pci_disable_obff);
>   */
>  static bool pci_ltr_supported(struct pci_dev *dev)
>  {
> -       int pos;
>         u32 cap;
>
>         /* LTR is a PCIe cap v2 feature */
> -       pos = pci_pcie_cap2(dev);
> -       if (!pos)
> +       if (pci_pcie_cap_read_dword(dev, PCI_EXP_DEVCAP2, &cap) ||
> +           !(cap & PCI_EXP_DEVCAP2_LTR))
>                 return false;

How about if you restructure this to avoid the double negatives?  E.g.,

    pci_pcie_cap_read_dword(dev, PCI_EXP_DEVCAP2, &cap);
    if (cap & PCI_EXP_DEVCAP2_LTR)
            return true;

    return false;

>
> -       pci_read_config_dword(dev, pos + PCI_EXP_DEVCAP2, &cap);
> -
> -       return cap & PCI_EXP_DEVCAP2_LTR;
> +       return true;
>  }
>
>  /**
> @@ -2248,22 +2174,16 @@ static bool pci_ltr_supported(struct pci_dev *dev)
>   */
>  int pci_enable_ltr(struct pci_dev *dev)
>  {
> -       int pos;
>         u16 ctrl;
>         int ret;
>
> -       if (!pci_ltr_supported(dev))
> -               return -ENOTSUPP;
> -
> -       /* LTR is a PCIe cap v2 feature */
> -       pos = pci_pcie_cap2(dev);
> -       if (!pos)
> -               return -ENOTSUPP;
> -
>         /* Only primary function can enable/disable LTR */
>         if (PCI_FUNC(dev->devfn) != 0)
>                 return -EINVAL;
>
> +       if (!pci_ltr_supported(dev))
> +               return -ENOTSUPP;
> +
>         /* Enable upstream ports first */
>         if (dev->bus->self) {
>                 ret = pci_enable_ltr(dev->bus->self);
> @@ -2271,9 +2191,10 @@ int pci_enable_ltr(struct pci_dev *dev)
>                         return ret;
>         }
>
> -       pci_read_config_word(dev, pos + PCI_EXP_DEVCTL2, &ctrl);
> +       if (pci_pcie_cap_read_word(dev, PCI_EXP_DEVCTL2, &ctrl))
> +               return -ENOTSUPP;
>         ctrl |= PCI_EXP_LTR_EN;
> -       pci_write_config_word(dev, pos + PCI_EXP_DEVCTL2, ctrl);
> +       pci_pcie_cap_write_word(dev, PCI_EXP_DEVCTL2, ctrl);
>
>         return 0;
>  }
> @@ -2285,24 +2206,19 @@ EXPORT_SYMBOL(pci_enable_ltr);
>   */
>  void pci_disable_ltr(struct pci_dev *dev)
>  {
> -       int pos;
>         u16 ctrl;
>
> -       if (!pci_ltr_supported(dev))
> -               return;
> -
> -       /* LTR is a PCIe cap v2 feature */
> -       pos = pci_pcie_cap2(dev);
> -       if (!pos)
> -               return;
> -
>         /* Only primary function can enable/disable LTR */
>         if (PCI_FUNC(dev->devfn) != 0)
>                 return;
>
> -       pci_read_config_word(dev, pos + PCI_EXP_DEVCTL2, &ctrl);
> -       ctrl &= ~PCI_EXP_LTR_EN;
> -       pci_write_config_word(dev, pos + PCI_EXP_DEVCTL2, ctrl);
> +       if (!pci_ltr_supported(dev))
> +               return;
> +
> +       if (!pci_pcie_cap_read_word(dev, PCI_EXP_DEVCTL2, &ctrl)) {
> +               ctrl &= ~PCI_EXP_LTR_EN;
> +               pci_pcie_cap_write_word(dev, PCI_EXP_DEVCTL2, ctrl);
> +       }
>  }
>  EXPORT_SYMBOL(pci_disable_ltr);
>
> @@ -3152,16 +3068,11 @@ EXPORT_SYMBOL(pci_set_dma_seg_boundary);
>  static int pcie_flr(struct pci_dev *dev, int probe)
>  {
>         int i;
> -       int pos;
>         u32 cap;
>         u16 status, control;
>
> -       pos = pci_pcie_cap(dev);
> -       if (!pos)
> -               return -ENOTTY;
> -
> -       pci_read_config_dword(dev, pos + PCI_EXP_DEVCAP, &cap);
> -       if (!(cap & PCI_EXP_DEVCAP_FLR))
> +       if (pci_pcie_cap_read_dword(dev, PCI_EXP_DEVCAP, &cap) ||
> +           !(cap & PCI_EXP_DEVCAP_FLR))
>                 return -ENOTTY;
>
>         if (probe)
> @@ -3172,7 +3083,7 @@ static int pcie_flr(struct pci_dev *dev, int probe)
>                 if (i)
>                         msleep((1 << (i - 1)) * 100);
>
> -               pci_read_config_word(dev, pos + PCI_EXP_DEVSTA, &status);
> +               pci_pcie_cap_read_word(dev, PCI_EXP_DEVSTA, &status);
>                 if (!(status & PCI_EXP_DEVSTA_TRPND))
>                         goto clear;
>         }
> @@ -3181,9 +3092,9 @@ static int pcie_flr(struct pci_dev *dev, int probe)
>                         "proceeding with reset anyway\n");
>
>  clear:
> -       pci_read_config_word(dev, pos + PCI_EXP_DEVCTL, &control);
> +       pci_pcie_cap_read_word(dev, PCI_EXP_DEVCTL, &control);
>         control |= PCI_EXP_DEVCTL_BCR_FLR;
> -       pci_write_config_word(dev, pos + PCI_EXP_DEVCTL, control);
> +       pci_pcie_cap_write_word(dev, PCI_EXP_DEVCTL, control);
>
>         msleep(100);
>
> @@ -3551,14 +3462,10 @@ EXPORT_SYMBOL(pcix_set_mmrbc);
>   */
>  int pcie_get_readrq(struct pci_dev *dev)
>  {
> -       int ret, cap;
> +       int ret;
>         u16 ctl;
>
> -       cap = pci_pcie_cap(dev);
> -       if (!cap)
> -               return -EINVAL;
> -
> -       ret = pci_read_config_word(dev, cap + PCI_EXP_DEVCTL, &ctl);
> +       ret = pci_pcie_cap_read_word(dev, PCI_EXP_DEVCTL, &ctl);
>         if (!ret)
>                 ret = 128 << ((ctl & PCI_EXP_DEVCTL_READRQ) >> 12);
>
> @@ -3576,17 +3483,13 @@ EXPORT_SYMBOL(pcie_get_readrq);
>   */
>  int pcie_set_readrq(struct pci_dev *dev, int rq)
>  {
> -       int cap, err = -EINVAL;
> +       int err = -EINVAL;
>         u16 ctl, v;
>
>         if (rq < 128 || rq > 4096 || !is_power_of_2(rq))
>                 goto out;
>
> -       cap = pci_pcie_cap(dev);
> -       if (!cap)
> -               goto out;
> -
> -       err = pci_read_config_word(dev, cap + PCI_EXP_DEVCTL, &ctl);
> +       err = pci_pcie_cap_read_word(dev, PCI_EXP_DEVCTL, &ctl);
>         if (err)
>                 goto out;
>         /*
> @@ -3609,7 +3512,7 @@ int pcie_set_readrq(struct pci_dev *dev, int rq)
>         if ((ctl & PCI_EXP_DEVCTL_READRQ) != v) {
>                 ctl &= ~PCI_EXP_DEVCTL_READRQ;
>                 ctl |= v;
> -               err = pci_write_config_word(dev, cap + PCI_EXP_DEVCTL, ctl);
> +               err = pci_pcie_cap_write_word(dev, PCI_EXP_DEVCTL, ctl);
>         }
>
>  out:
> @@ -3626,14 +3529,10 @@ EXPORT_SYMBOL(pcie_set_readrq);
>   */
>  int pcie_get_mps(struct pci_dev *dev)
>  {
> -       int ret, cap;
> +       int ret;
>         u16 ctl;
>
> -       cap = pci_pcie_cap(dev);
> -       if (!cap)
> -               return -EINVAL;
> -
> -       ret = pci_read_config_word(dev, cap + PCI_EXP_DEVCTL, &ctl);
> +       ret = pci_pcie_cap_read_word(dev, PCI_EXP_DEVCTL, &ctl);
>         if (!ret)
>                 ret = 128 << ((ctl & PCI_EXP_DEVCTL_PAYLOAD) >> 5);
>
> @@ -3650,7 +3549,7 @@ int pcie_get_mps(struct pci_dev *dev)
>   */
>  int pcie_set_mps(struct pci_dev *dev, int mps)
>  {
> -       int cap, err = -EINVAL;
> +       int err = -EINVAL;
>         u16 ctl, v;
>
>         if (mps < 128 || mps > 4096 || !is_power_of_2(mps))
> @@ -3661,18 +3560,14 @@ int pcie_set_mps(struct pci_dev *dev, int mps)
>                 goto out;
>         v <<= 5;
>
> -       cap = pci_pcie_cap(dev);
> -       if (!cap)
> -               goto out;
> -
> -       err = pci_read_config_word(dev, cap + PCI_EXP_DEVCTL, &ctl);
> +       err = pci_pcie_cap_read_word(dev, PCI_EXP_DEVCTL, &ctl);
>         if (err)
>                 goto out;
>
>         if ((ctl & PCI_EXP_DEVCTL_PAYLOAD) != v) {
>                 ctl &= ~PCI_EXP_DEVCTL_PAYLOAD;
>                 ctl |= v;
> -               err = pci_write_config_word(dev, cap + PCI_EXP_DEVCTL, ctl);
> +               err = pci_pcie_cap_write_word(dev, PCI_EXP_DEVCTL, ctl);
>         }
>  out:
>         return err;
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 446933d..5112ff5 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -603,10 +603,10 @@ static void pci_set_bus_speed(struct pci_bus *bus)
>                 u32 linkcap;
>                 u16 linksta;
>
> -               pci_read_config_dword(bridge, pos + PCI_EXP_LNKCAP, &linkcap);
> +               pci_pcie_cap_read_dword(bridge, PCI_EXP_LNKCAP, &linkcap);
>                 bus->max_bus_speed = pcie_link_speed[linkcap & 0xf];
>
> -               pci_read_config_word(bridge, pos + PCI_EXP_LNKSTA, &linksta);
> +               pci_pcie_cap_read_word(bridge, PCI_EXP_LNKSTA, &linksta);
>                 pcie_update_link_speed(bus, linksta);
>         }
>  }
> @@ -936,18 +936,10 @@ void set_pcie_port_type(struct pci_dev *pdev)
>
>  void set_pcie_hotplug_bridge(struct pci_dev *pdev)
>  {
> -       int pos;
> -       u16 reg16;
>         u32 reg32;
>
> -       pos = pci_pcie_cap(pdev);
> -       if (!pos)
> -               return;
> -       pci_read_config_word(pdev, pos + PCI_EXP_FLAGS, &reg16);
> -       if (!(reg16 & PCI_EXP_FLAGS_SLOT))
> -               return;
> -       pci_read_config_dword(pdev, pos + PCI_EXP_SLTCAP, &reg32);
> -       if (reg32 & PCI_EXP_SLTCAP_HPC)
> +       if (!pci_pcie_cap_read_dword(pdev, PCI_EXP_SLTCAP, &reg32) &&
> +           (reg32 & PCI_EXP_SLTCAP_HPC))
>                 pdev->is_hotplug_bridge = 1;
>  }
>
> @@ -1160,8 +1152,7 @@ int pci_cfg_space_size(struct pci_dev *dev)
>         if (class == PCI_CLASS_BRIDGE_HOST)
>                 return pci_cfg_space_size_ext(dev);
>
> -       pos = pci_pcie_cap(dev);
> -       if (!pos) {
> +       if (!pci_is_pcie(dev)) {
>                 pos = pci_find_capability(dev, PCI_CAP_ID_PCIX);
>                 if (!pos)
>                         goto fail;
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index 52f44b5..7586c24 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -3093,17 +3093,13 @@ static int reset_intel_generic_dev(struct pci_dev *dev, int probe)
>
>  static int reset_intel_82599_sfp_virtfn(struct pci_dev *dev, int probe)
>  {
> -       int pos;
> -
> -       pos = pci_find_capability(dev, PCI_CAP_ID_EXP);
> -       if (!pos)
> +       if (!pci_is_pcie(dev))
>                 return -ENOTTY;
>
>         if (probe)
>                 return 0;
>
> -       pci_write_config_word(dev, pos + PCI_EXP_DEVCTL,
> -                               PCI_EXP_DEVCTL_BCR_FLR);
> +       pci_pcie_cap_write_word(dev, PCI_EXP_DEVCTL, PCI_EXP_DEVCTL_BCR_FLR);
>         msleep(100);
>
>         return 0;
> --
> 1.7.9.5
>

Re: [RFC PATCH 07/14] hotplug/PCI: use PCIe cap access functions to simplify implementation

[Bjorn Helgaas]

On Tue, Jul 10, 2012 at 9:54 AM, Jiang Liu <liuj97@gmail.com> wrote:
> From: Jiang Liu <jiang.liu@huawei.com>
>
> Use PCIe cap access functions to simplify pcihp_slot.c
>
> Signed-off-by: Jiang Liu <liuj97@gmail.com>
> ---
>  drivers/pci/hotplug/pcihp_slot.c |   17 +++++++++--------
>  1 file changed, 9 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/pci/hotplug/pcihp_slot.c b/drivers/pci/hotplug/pcihp_slot.c
> index 8c05a18..08ca019 100644
> --- a/drivers/pci/hotplug/pcihp_slot.c
> +++ b/drivers/pci/hotplug/pcihp_slot.c
> @@ -103,8 +103,7 @@ static void program_hpp_type2(struct pci_dev *dev, struct hpp_type2 *hpp)
>                 return;
>
>         /* Find PCI Express capability */

Drop the comment, too.

> -       pos = pci_pcie_cap(dev);
> -       if (!pos)
> +       if (!pci_is_pcie(dev))
>                 return;
>
>         if (hpp->revision > 1) {
> @@ -114,16 +113,18 @@ static void program_hpp_type2(struct pci_dev *dev, struct hpp_type2 *hpp)
>         }
>
>         /* Initialize Device Control Register */
> -       pci_read_config_word(dev, pos + PCI_EXP_DEVCTL, &reg16);
> +       if (pci_pcie_cap_read_word(dev, PCI_EXP_DEVCTL, &reg16))
> +               return;
>         reg16 = (reg16 & hpp->pci_exp_devctl_and) | hpp->pci_exp_devctl_or;
> -       pci_write_config_word(dev, pos + PCI_EXP_DEVCTL, reg16);
> +       pci_pcie_cap_write_word(dev, PCI_EXP_DEVCTL, reg16);
>
>         /* Initialize Link Control Register */
>         if (dev->subordinate) {
> -               pci_read_config_word(dev, pos + PCI_EXP_LNKCTL, &reg16);
> -               reg16 = (reg16 & hpp->pci_exp_lnkctl_and)
> -                       | hpp->pci_exp_lnkctl_or;
> -               pci_write_config_word(dev, pos + PCI_EXP_LNKCTL, reg16);
> +               if (pci_pcie_cap_read_word(dev, PCI_EXP_LNKCTL, &reg16)) {
> +                       reg16 = (reg16 & hpp->pci_exp_lnkctl_and)
> +                               | hpp->pci_exp_lnkctl_or;
> +                       pci_pcie_cap_write_word(dev, PCI_EXP_LNKCTL, reg16);
> +               }
>         }
>
>         /* Find Advanced Error Reporting Enhanced Capability */
> --
> 1.7.9.5
>

Re: [RFC PATCH 00/14] improve PCIe capabilities registers handling

[Bjorn Helgaas]

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.

Re: [RFC PATCH 04/14] PCI: refine and move pcie_cap_has_*() macros to include/linux/pci.h

[Bjorn Helgaas]

On Tue, Jul 10, 2012 at 9:54 AM, Jiang Liu <liuj97@gmail.com> wrote:
> From: Jiang Liu <jiang.liu@huawei.com>
>
> Move pcie_cap_has_*() macros to include/linux/pci.h, so they can be shared.
> Since pcie_flags was introduced, rework these macros to take a struct pci_dev *
> and use pcie_flags insead of type and flags.
>
> Signed-off-by: Jiang Liu <liuj97@gmail.com>
> Signed-off-by: Yijing Wang <wangyijing@huawei.com>
> ---
>  drivers/pci/pci.c   |   50 ++++++++++++--------------------------------------
>  include/linux/pci.h |   43 +++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 55 insertions(+), 38 deletions(-)
>
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 28eb55b..4c6ab70 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -272,15 +272,11 @@ int pci_bus_find_capability(struct pci_bus *bus, unsigned int devfn, int cap)
>   */
>  static int pci_pcie_cap2(struct pci_dev *dev)
>  {
> -       u16 flags;
>         int pos;
>
>         pos = pci_pcie_cap(dev);
> -       if (pos) {
> -               pci_read_config_word(dev, pos + PCI_EXP_FLAGS, &flags);
> -               if ((flags & PCI_EXP_FLAGS_VERS) < 2)
> -                       pos = 0;
> -       }
> +       if (pos && !pci_pcie_cap_has_cap2(dev))
> +               pos = 0;
>
>         return pos;
>  }
> @@ -854,21 +850,6 @@ EXPORT_SYMBOL(pci_choose_state);
>
>  #define PCI_EXP_SAVE_REGS      7
>
> -#define pcie_cap_has_devctl(type, flags)       1
> -#define pcie_cap_has_lnkctl(type, flags)               \
> -               ((flags & PCI_EXP_FLAGS_VERS) > 1 ||    \
> -                (type == PCI_EXP_TYPE_ROOT_PORT ||     \
> -                 type == PCI_EXP_TYPE_ENDPOINT ||      \
> -                 type == PCI_EXP_TYPE_LEG_END))
> -#define pcie_cap_has_sltctl(type, flags)               \
> -               ((flags & PCI_EXP_FLAGS_VERS) > 1 ||    \
> -                ((type == PCI_EXP_TYPE_ROOT_PORT) ||   \
> -                 (type == PCI_EXP_TYPE_DOWNSTREAM &&   \
> -                  (flags & PCI_EXP_FLAGS_SLOT))))
> -#define pcie_cap_has_rtctl(type, flags)                        \
> -               ((flags & PCI_EXP_FLAGS_VERS) > 1 ||    \
> -                (type == PCI_EXP_TYPE_ROOT_PORT ||     \
> -                 type == PCI_EXP_TYPE_RC_EC))
>
>  static struct pci_cap_saved_state *pci_find_saved_cap(
>         struct pci_dev *pci_dev, char cap)
> @@ -885,10 +866,9 @@ static struct pci_cap_saved_state *pci_find_saved_cap(
>
>  static int pci_save_pcie_state(struct pci_dev *dev)
>  {
> -       int type, pos, i = 0;
> +       int pos, i = 0;
>         struct pci_cap_saved_state *save_state;
>         u16 *cap;
> -       u16 flags;
>
>         pos = pci_pcie_cap(dev);
>         if (!pos)
> @@ -901,16 +881,14 @@ static int pci_save_pcie_state(struct pci_dev *dev)
>         }
>         cap = (u16 *)&save_state->cap.data[0];
>
> -       pci_read_config_word(dev, pos + PCI_EXP_FLAGS, &flags);
>
> -       type = pci_pcie_type(dev);
> -       if (pcie_cap_has_devctl(type, flags))
> +       if (pci_pcie_cap_has_devctl(dev))
>                 pci_read_config_word(dev, pos + PCI_EXP_DEVCTL, &cap[i++]);
> -       if (pcie_cap_has_lnkctl(type, flags))
> +       if (pci_pcie_cap_has_lnkctl(dev))
>                 pci_read_config_word(dev, pos + PCI_EXP_LNKCTL, &cap[i++]);
> -       if (pcie_cap_has_sltctl(type, flags))
> +       if (pci_pcie_cap_has_sltctl(dev))
>                 pci_read_config_word(dev, pos + PCI_EXP_SLTCTL, &cap[i++]);
> -       if (pcie_cap_has_rtctl(type, flags))
> +       if (pci_pcie_cap_has_rtctl(dev))
>                 pci_read_config_word(dev, pos + PCI_EXP_RTCTL, &cap[i++]);
>
>         pos = pci_pcie_cap2(dev);
> @@ -925,10 +903,9 @@ static int pci_save_pcie_state(struct pci_dev *dev)
>
>  static void pci_restore_pcie_state(struct pci_dev *dev)
>  {
> -       int i = 0, pos, type;
> +       int i = 0, pos;
>         struct pci_cap_saved_state *save_state;
>         u16 *cap;
> -       u16 flags;
>
>         save_state = pci_find_saved_cap(dev, PCI_CAP_ID_EXP);
>         pos = pci_find_capability(dev, PCI_CAP_ID_EXP);
> @@ -936,16 +913,13 @@ static void pci_restore_pcie_state(struct pci_dev *dev)
>                 return;
>         cap = (u16 *)&save_state->cap.data[0];
>
> -       pci_read_config_word(dev, pos + PCI_EXP_FLAGS, &flags);
> -
> -       type = pci_pcie_type(dev);
> -       if (pcie_cap_has_devctl(type, flags))
> +       if (pci_pcie_cap_has_devctl(dev))
>                 pci_write_config_word(dev, pos + PCI_EXP_DEVCTL, cap[i++]);
> -       if (pcie_cap_has_lnkctl(type, flags))
> +       if (pci_pcie_cap_has_lnkctl(dev))
>                 pci_write_config_word(dev, pos + PCI_EXP_LNKCTL, cap[i++]);
> -       if (pcie_cap_has_sltctl(type, flags))
> +       if (pci_pcie_cap_has_sltctl(dev))
>                 pci_write_config_word(dev, pos + PCI_EXP_SLTCTL, cap[i++]);
> -       if (pcie_cap_has_rtctl(type, flags))
> +       if (pci_pcie_cap_has_rtctl(dev))
>                 pci_write_config_word(dev, pos + PCI_EXP_RTCTL, cap[i++]);
>
>         pos = pci_pcie_cap2(dev);
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 1837354..346b2d9 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -1659,6 +1659,49 @@ static inline int pci_pcie_type(const struct pci_dev *dev)
>         return (dev->pcie_flags & PCI_EXP_FLAGS_TYPE) >> 4;
>  }
>
> +static inline int pci_pcie_cap_version(const struct pci_dev *pdev)
> +{
> +       return pdev->pcie_flags & PCI_EXP_FLAGS_VERS;
> +}
> +
> +static inline int pci_pcie_cap_has_cap2(const struct pci_dev *pdev)
> +{
> +       return pci_pcie_cap_version(pdev) > 1;
> +}

Do we need these (pci_pcie_cap_version() and pci_pcie_cap_has_cap2())
anywhere other than maybe in pci/access.c?  I hope not.

> +
> +static inline bool pci_pcie_cap_has_devctl(const struct pci_dev *pdev)
> +{
> +       return true;
> +}
> +
> +static inline bool pci_pcie_cap_has_lnkctl(const struct pci_dev *pdev)
> +{
> +       int type = pci_pcie_type(pdev);
> +
> +       return pci_pcie_cap_version(pdev) > 1 ||
> +              type == PCI_EXP_TYPE_ROOT_PORT ||
> +              type == PCI_EXP_TYPE_ENDPOINT ||
> +              type == PCI_EXP_TYPE_LEG_END;
> +}
> +
> +static inline bool pci_pcie_cap_has_sltctl(const struct pci_dev *pdev)
> +{
> +       int type = pci_pcie_type(pdev);
> +
> +       return pci_pcie_cap_version(pdev) > 1 ||
> +              type == PCI_EXP_TYPE_ROOT_PORT ||
> +              (type == PCI_EXP_TYPE_DOWNSTREAM &&
> +               pdev->pcie_flags & PCI_EXP_FLAGS_SLOT);
> +}
> +
> +static inline bool pci_pcie_cap_has_rtctl(const struct pci_dev *pdev)
> +{
> +       int type = pci_pcie_type(pdev);
> +
> +       return pci_pcie_cap_version(pdev) > 1 ||
> +              type == PCI_EXP_TYPE_ROOT_PORT ||
> +              type == PCI_EXP_TYPE_RC_EC;
> +}

I hope these are used only in pci_pcie_cap_read/write(), so maybe they
could just be moved to access.c along with them.  Maybe you can just
*add* definitions to pci/access.c (leaving pci/pci.c alone).  Then
you'd only have to touch the save/restore_pcie_state() functions once.

>  void pci_request_acs(void);
>  bool pci_acs_enabled(struct pci_dev *pdev, u16 acs_flags);
> --
> 1.7.9.5
>

Re: [RFC PATCH 06/14] PCI: use PCIe cap access functions to simplify PCI core implementation

[Jiang Liu]

On 2012-7-11 2:35, Bjorn Helgaas wrote:
>> @@ -2042,7 +1994,6 @@ void pci_free_cap_save_buffers(struct pci_dev *dev)
>>   */
>>  void pci_enable_ari(struct pci_dev *dev)
>>  {
>> -       int pos;
>>         u32 cap;
>>         u16 ctrl;
>>         struct pci_dev *bridge;
>> @@ -2050,8 +2001,7 @@ void pci_enable_ari(struct pci_dev *dev)
>>         if (pcie_ari_disabled || !pci_is_pcie(dev) || dev->devfn)
>>                 return;
>>
>> -       pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ARI);
>> -       if (!pos)
>> +       if (pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ARI))
> 
> What's going on here?  This looks wrong, and unrelated to the rest of the patch.
Yeah, it's a bug. My original idea is to get rid of "int pos", and it should be
if (!pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ARI))

> 
>>                 return;
>>
>>         bridge = dev->bus->self;
>> @@ -2059,17 +2009,14 @@ void pci_enable_ari(struct pci_dev *dev)
>>                 return;
>>
>>         /* ARI is a PCIe cap v2 feature */
> 
> If we're doing this right, we should be able to remove this comment
> (and similar ones below).
Will remove them.

> 
>> -       pos = pci_pcie_cap2(bridge);
>> -       if (!pos)
>> +       if (pci_pcie_cap_read_dword(bridge, PCI_EXP_DEVCAP2, &cap) ||
>> +          !(cap & PCI_EXP_DEVCAP2_ARI))
> 
> I don't think there's any point in checking every return from
> pci_pcie_cap_read_dword().  We've already checked pci_is_pcie() above,
> and checking here just clutters the code.  In cases like this, my
> opinion is that clean code leads to fewer bugs, and that benefit
> outweighs whatever technical "every return must be checked" benefit
> there might be.
Good point!

>> @@ -2223,17 +2152,14 @@ EXPORT_SYMBOL(pci_disable_obff);
>>   */
>>  static bool pci_ltr_supported(struct pci_dev *dev)
>>  {
>> -       int pos;
>>         u32 cap;
>>
>>         /* LTR is a PCIe cap v2 feature */
>> -       pos = pci_pcie_cap2(dev);
>> -       if (!pos)
>> +       if (pci_pcie_cap_read_dword(dev, PCI_EXP_DEVCAP2, &cap) ||
>> +           !(cap & PCI_EXP_DEVCAP2_LTR))
>>                 return false;
> 
> How about if you restructure this to avoid the double negatives?  E.g.,
> 
>     pci_pcie_cap_read_dword(dev, PCI_EXP_DEVCAP2, &cap);
>     if (cap & PCI_EXP_DEVCAP2_LTR)
>             return true;
> 
>     return false;
> 
Good point.

Thanks!
Gerry

Re: [RFC PATCH 05/14] PCI: add access functions for PCIe capabilities to hide PCIe spec differences

[Jiang Liu]

On 2012-7-11 2:35, Bjorn Helgaas wrote:
>> diff --git a/drivers/pci/access.c b/drivers/pci/access.c
>> index ba91a7e..80ae022 100644
>> --- a/drivers/pci/access.c
>> +++ b/drivers/pci/access.c
>> @@ -469,3 +469,91 @@ void pci_cfg_access_unlock(struct pci_dev *dev)
>>         raw_spin_unlock_irqrestore(&pci_lock, flags);
>>  }
>>  EXPORT_SYMBOL_GPL(pci_cfg_access_unlock);
>> +
>> +static int
>> +pci_pcie_cap_get_offset(struct pci_dev *dev, int where, size_t sz)
>> +{
>> +       bool valid;
>> +
>> +       if (!pci_is_pcie(dev))
>> +               return -EINVAL;
>> +       if (where & (sz - 1))
>> +               return -EINVAL;
>> +
>> +       if (where < 0)
>> +               valid = false;
>> +       else if (where < PCI_EXP_DEVCAP)
>> +               valid = true;
>> +       else if (where < PCI_EXP_LNKCAP)
>> +               valid = pci_pcie_cap_has_devctl(dev);
>> +       else if (where < PCI_EXP_SLTCAP)
>> +               valid = pci_pcie_cap_has_lnkctl(dev);
>> +       else if (where < PCI_EXP_RTCTL)
>> +               valid = pci_pcie_cap_has_sltctl(dev);
>> +       else if (where < PCI_EXP_DEVCAP2)
>> +               valid = pci_pcie_cap_has_rtctl(dev);
>> +       else if (where < PCI_EXP_CAP2_SIZE)
>> +               valid = pci_pcie_cap_has_cap2(dev);
>> +       else
>> +               valid = false;
>> +
>> +       return valid ? where + pci_pcie_cap(dev) : -EINVAL;
>> +}
>> +
>> +int pci_pcie_cap_read_word(struct pci_dev *dev, int where, u16 *valp)
>> +{
>> +       *valp = 0;
>> +       where = pci_pcie_cap_get_offset(dev, where, sizeof(u16));
> 
> This is a really slick factorization; I like it much better than my
> proposal.  I would like it even *better* if it read something like
> this:
> 
>     bool implemented;
> 
>     *valp = 0;
>     if (!pci_is_pcie(dev) || where & 1)
>         return -EINVAL;
> 
>     implemented = pci_pcie_cap_implemented(dev, where);
>     if (implemented)
>         return pci_read_config_word(dev, pci_pcie_cap(dev) + where, valp);
> 
>     if (pci_is_pcie(dev) && where == PCI_EXP_SLTSTA ...
> 
> because I think it's useful to have the "pos + where" visual pattern
> in the pci_read_config_word() arguments.
Sure, for better readability.

> 
>> +       if (where >= 0)
>> +               return pci_read_config_word(dev, where, valp);
>> +
>> +       if (pci_is_pcie(dev) && where == PCI_EXP_SLTSTA &&
>> +           pci_pcie_type(dev) == PCI_EXP_TYPE_DOWNSTREAM)
>> +               *valp = PCI_EXP_SLTSTA_PDS;
> 
> I think we should be returning success in this case (SLTSTA for
> downstream port).  In fact, I think we should return success even when
> we're emulating the read of an unimplemented register from a v1
> capability.  The caller should not be aware at all that there is a
> difference between v1 and v2 capabilities.
> 
> I'd put the spec reference here rather than in read_dword(), since
> SLTSTA is a u16 and this is the natural way to read it.  Then maybe a
> short comment in read_dword() below.
Good point. Return success when reading unimplemented registeres, that
may simplify code. For we still should return -EINVAL when writing
unimplemented registers, right?

>> +EXPORT_SYMBOL(pci_pcie_cap_write_dword);
>> diff --git a/include/linux/pci.h b/include/linux/pci.h
>> index 346b2d9..78767b2 100644
>> --- a/include/linux/pci.h
>> +++ b/include/linux/pci.h
>> @@ -1703,6 +1703,11 @@ static inline bool pci_pcie_cap_has_rtctl(const struct pci_dev *pdev)
>>                type == PCI_EXP_TYPE_RC_EC;
>>  }
>>
>> +extern int pci_pcie_cap_read_word(struct pci_dev *dev, int where, u16 *valp);
>> +extern int pci_pcie_cap_read_dword(struct pci_dev *dev, int where, u32 *valp);
>> +extern int pci_pcie_cap_write_word(struct pci_dev *dev, int where, u16 val);
>> +extern int pci_pcie_cap_write_dword(struct pci_dev *dev, int where, u32 val);
> 
> You don't need the "extern" here (and I think you'll probably remove
> these altogether, see below).
> 
>> +
>>  void pci_request_acs(void);
>>  bool pci_acs_enabled(struct pci_dev *pdev, u16 acs_flags);
>>  bool pci_acs_path_enabled(struct pci_dev *start,
>> @@ -1843,5 +1848,10 @@ static inline struct eeh_dev *pci_dev_to_eeh_dev(struct pci_dev *pdev)
>>   */
>>  struct pci_dev *pci_find_upstream_pcie_bridge(struct pci_dev *pdev);
>>
>> +int pci_pcie_capability_read_word(struct pci_dev *dev, int where, u16 *val);
>> +int pci_pcie_capability_read_dword(struct pci_dev *dev, int where, u32 *val);
>> +int pci_pcie_capability_write_word(struct pci_dev *dev, int where, u16 val);
>> +int pci_pcie_capability_write_dword(struct pci_dev *dev, int where, u32 val);
> 
> There's some confusion here: pci_pcie_cap_* versus
> pci_pcie_capability_*.  I think you only need one set, and I prefer
> pci_pcie_capability_* to follow the example of
> pci_bus_find_capability().
The above confusion was caused by a dirty merge.

> 
>> +
>>  #endif /* __KERNEL__ */
>>  #endif /* LINUX_PCI_H */
>> diff --git a/include/linux/pci_regs.h b/include/linux/pci_regs.h
>> index 53274bf..ac60e22 100644
>> --- a/include/linux/pci_regs.h
>> +++ b/include/linux/pci_regs.h
>> @@ -542,9 +542,24 @@
>>  #define  PCI_EXP_OBFF_MSGA_EN  0x2000  /* OBFF enable with Message type A */
>>  #define  PCI_EXP_OBFF_MSGB_EN  0x4000  /* OBFF enable with Message type B */
>>  #define  PCI_EXP_OBFF_WAKE_EN  0x6000  /* OBFF using WAKE# signaling */
>> -#define PCI_CAP_EXP_ENDPOINT_SIZEOF_V2 44      /* v2 endpoints end here */
>> +#define PCI_EXP_DEVSTA2                42      /* Device Status 2 */
>> +#define PCI_CAP_EXP_ENDPOINT_SIZEOF_V2 44 /* v2 endpoints end here */
>> +#define PCI_EXP_LNKCAP2                44      /* Link Capabilities 2 */
>>  #define PCI_EXP_LNKCTL2                48      /* Link Control 2 */
>> -#define PCI_EXP_SLTCTL2                56      /* Slot Control 2 */
>> +#define  PCI_EXP_LNKCTL2_TLS   0x0f    /* Target Link Speed */
>> +#define  PCI_EXP_LNKCTL2_EC    0x10    /* Enter Compliance */
>> +#define  PCI_EXP_LNKCTL2_HASD  0x20    /* Hardware Autonomous Speed Disable */
>> +#define  PCI_EXP_LNKCTL2_SD    0x40    /* Selectable De-emphasis */
>> +#define  PCI_EXP_LNKCTL2_TM    0x380   /* Transmit Margin */
>> +#define  PCI_EXP_LNKCTL2_EMC   0x400   /* Enter Modified Compliance */
>> +#define  PCI_EXP_LNKCTL2_CS    0x800   /* Compliance SOS */
>> +#define  PCI_EXP_LNKCTL2_CD    0x1000  /* Compliance De-emphasis */
>> +#define PCI_EXP_LNKSTA2                50      /* Link Status 2 */
>> +#define  PCI_EXP_LNKSTA2_CDL   0x01    /* Current De-emphasis Level */
>> +#define PCI_EXP_SLTCAP2                52      /* Slot Capabilities 2 */
>> +#define PCI_EXP_SLTCTL2                56      /* Slot Control 2*/
>> +#define PCI_EXP_SLTSTA2                58      /* Slot Status 2*/
>> +#define        PCI_EXP_CAP2_SIZE       60
> 
> Most of these changes look unrelated to the current patch.  They
> should be moved to a patch that uses the symbols you're adding.
Good point, create on demand:)

Thanks!
Gerry

Re: [RFC PATCH 05/14] PCI: add access functions for PCIe capabilities to hide PCIe spec differences

[Bjorn Helgaas]

On Tue, Jul 10, 2012 at 9:07 PM, Jiang Liu <jiang.liu@huawei.com> wrote:
> On 2012-7-11 2:35, Bjorn Helgaas wrote:
>>> diff --git a/drivers/pci/access.c b/drivers/pci/access.c
>>> index ba91a7e..80ae022 100644
>>> --- a/drivers/pci/access.c
>>> +++ b/drivers/pci/access.c
>>> @@ -469,3 +469,91 @@ void pci_cfg_access_unlock(struct pci_dev *dev)
>>>         raw_spin_unlock_irqrestore(&pci_lock, flags);
>>>  }
>>>  EXPORT_SYMBOL_GPL(pci_cfg_access_unlock);
>>> +
>>> +static int
>>> +pci_pcie_cap_get_offset(struct pci_dev *dev, int where, size_t sz)
>>> +{
>>> +       bool valid;
>>> +
>>> +       if (!pci_is_pcie(dev))
>>> +               return -EINVAL;
>>> +       if (where & (sz - 1))
>>> +               return -EINVAL;
>>> +
>>> +       if (where < 0)
>>> +               valid = false;
>>> +       else if (where < PCI_EXP_DEVCAP)
>>> +               valid = true;
>>> +       else if (where < PCI_EXP_LNKCAP)
>>> +               valid = pci_pcie_cap_has_devctl(dev);
>>> +       else if (where < PCI_EXP_SLTCAP)
>>> +               valid = pci_pcie_cap_has_lnkctl(dev);
>>> +       else if (where < PCI_EXP_RTCTL)
>>> +               valid = pci_pcie_cap_has_sltctl(dev);
>>> +       else if (where < PCI_EXP_DEVCAP2)
>>> +               valid = pci_pcie_cap_has_rtctl(dev);
>>> +       else if (where < PCI_EXP_CAP2_SIZE)
>>> +               valid = pci_pcie_cap_has_cap2(dev);
>>> +       else
>>> +               valid = false;
>>> +
>>> +       return valid ? where + pci_pcie_cap(dev) : -EINVAL;
>>> +}
>>> +
>>> +int pci_pcie_cap_read_word(struct pci_dev *dev, int where, u16 *valp)
>>> +{
>>> +       *valp = 0;
>>> +       where = pci_pcie_cap_get_offset(dev, where, sizeof(u16));
>>
>> This is a really slick factorization; I like it much better than my
>> proposal.  I would like it even *better* if it read something like
>> this:
>>
>>     bool implemented;
>>
>>     *valp = 0;
>>     if (!pci_is_pcie(dev) || where & 1)
>>         return -EINVAL;
>>
>>     implemented = pci_pcie_cap_implemented(dev, where);
>>     if (implemented)
>>         return pci_read_config_word(dev, pci_pcie_cap(dev) + where, valp);
>>
>>     if (pci_is_pcie(dev) && where == PCI_EXP_SLTSTA ...
>>
>> because I think it's useful to have the "pos + where" visual pattern
>> in the pci_read_config_word() arguments.
> Sure, for better readability.
>
>>
>>> +       if (where >= 0)
>>> +               return pci_read_config_word(dev, where, valp);
>>> +
>>> +       if (pci_is_pcie(dev) && where == PCI_EXP_SLTSTA &&
>>> +           pci_pcie_type(dev) == PCI_EXP_TYPE_DOWNSTREAM)
>>> +               *valp = PCI_EXP_SLTSTA_PDS;
>>
>> I think we should be returning success in this case (SLTSTA for
>> downstream port).  In fact, I think we should return success even when
>> we're emulating the read of an unimplemented register from a v1
>> capability.  The caller should not be aware at all that there is a
>> difference between v1 and v2 capabilities.
>>
>> I'd put the spec reference here rather than in read_dword(), since
>> SLTSTA is a u16 and this is the natural way to read it.  Then maybe a
>> short comment in read_dword() below.
> Good point. Return success when reading unimplemented registeres, that
> may simplify code. For we still should return -EINVAL when writing
> unimplemented registers, right?

Yeah, I guess it's OK to return -EINVAL when *writing* to an
unimplemented register.  Hopefully the caller is structured such that
we don't even try to write in that case.  It'd be interesting to audit
the callers and explore that, but I haven't done that.

>>> +EXPORT_SYMBOL(pci_pcie_cap_write_dword);
>>> diff --git a/include/linux/pci.h b/include/linux/pci.h
>>> index 346b2d9..78767b2 100644
>>> --- a/include/linux/pci.h
>>> +++ b/include/linux/pci.h
>>> @@ -1703,6 +1703,11 @@ static inline bool pci_pcie_cap_has_rtctl(const struct pci_dev *pdev)
>>>                type == PCI_EXP_TYPE_RC_EC;
>>>  }
>>>
>>> +extern int pci_pcie_cap_read_word(struct pci_dev *dev, int where, u16 *valp);
>>> +extern int pci_pcie_cap_read_dword(struct pci_dev *dev, int where, u32 *valp);
>>> +extern int pci_pcie_cap_write_word(struct pci_dev *dev, int where, u16 val);
>>> +extern int pci_pcie_cap_write_dword(struct pci_dev *dev, int where, u32 val);
>>
>> You don't need the "extern" here (and I think you'll probably remove
>> these altogether, see below).
>>
>>> +
>>>  void pci_request_acs(void);
>>>  bool pci_acs_enabled(struct pci_dev *pdev, u16 acs_flags);
>>>  bool pci_acs_path_enabled(struct pci_dev *start,
>>> @@ -1843,5 +1848,10 @@ static inline struct eeh_dev *pci_dev_to_eeh_dev(struct pci_dev *pdev)
>>>   */
>>>  struct pci_dev *pci_find_upstream_pcie_bridge(struct pci_dev *pdev);
>>>
>>> +int pci_pcie_capability_read_word(struct pci_dev *dev, int where, u16 *val);
>>> +int pci_pcie_capability_read_dword(struct pci_dev *dev, int where, u32 *val);
>>> +int pci_pcie_capability_write_word(struct pci_dev *dev, int where, u16 val);
>>> +int pci_pcie_capability_write_dword(struct pci_dev *dev, int where, u32 val);
>>
>> There's some confusion here: pci_pcie_cap_* versus
>> pci_pcie_capability_*.  I think you only need one set, and I prefer
>> pci_pcie_capability_* to follow the example of
>> pci_bus_find_capability().
> The above confusion was caused by a dirty merge.
>
>>
>>> +
>>>  #endif /* __KERNEL__ */
>>>  #endif /* LINUX_PCI_H */
>>> diff --git a/include/linux/pci_regs.h b/include/linux/pci_regs.h
>>> index 53274bf..ac60e22 100644
>>> --- a/include/linux/pci_regs.h
>>> +++ b/include/linux/pci_regs.h
>>> @@ -542,9 +542,24 @@
>>>  #define  PCI_EXP_OBFF_MSGA_EN  0x2000  /* OBFF enable with Message type A */
>>>  #define  PCI_EXP_OBFF_MSGB_EN  0x4000  /* OBFF enable with Message type B */
>>>  #define  PCI_EXP_OBFF_WAKE_EN  0x6000  /* OBFF using WAKE# signaling */
>>> -#define PCI_CAP_EXP_ENDPOINT_SIZEOF_V2 44      /* v2 endpoints end here */
>>> +#define PCI_EXP_DEVSTA2                42      /* Device Status 2 */
>>> +#define PCI_CAP_EXP_ENDPOINT_SIZEOF_V2 44 /* v2 endpoints end here */
>>> +#define PCI_EXP_LNKCAP2                44      /* Link Capabilities 2 */
>>>  #define PCI_EXP_LNKCTL2                48      /* Link Control 2 */
>>> -#define PCI_EXP_SLTCTL2                56      /* Slot Control 2 */
>>> +#define  PCI_EXP_LNKCTL2_TLS   0x0f    /* Target Link Speed */
>>> +#define  PCI_EXP_LNKCTL2_EC    0x10    /* Enter Compliance */
>>> +#define  PCI_EXP_LNKCTL2_HASD  0x20    /* Hardware Autonomous Speed Disable */
>>> +#define  PCI_EXP_LNKCTL2_SD    0x40    /* Selectable De-emphasis */
>>> +#define  PCI_EXP_LNKCTL2_TM    0x380   /* Transmit Margin */
>>> +#define  PCI_EXP_LNKCTL2_EMC   0x400   /* Enter Modified Compliance */
>>> +#define  PCI_EXP_LNKCTL2_CS    0x800   /* Compliance SOS */
>>> +#define  PCI_EXP_LNKCTL2_CD    0x1000  /* Compliance De-emphasis */
>>> +#define PCI_EXP_LNKSTA2                50      /* Link Status 2 */
>>> +#define  PCI_EXP_LNKSTA2_CDL   0x01    /* Current De-emphasis Level */
>>> +#define PCI_EXP_SLTCAP2                52      /* Slot Capabilities 2 */
>>> +#define PCI_EXP_SLTCTL2                56      /* Slot Control 2*/
>>> +#define PCI_EXP_SLTSTA2                58      /* Slot Status 2*/
>>> +#define        PCI_EXP_CAP2_SIZE       60
>>
>> Most of these changes look unrelated to the current patch.  They
>> should be moved to a patch that uses the symbols you're adding.
> Good point, create on demand:)
>
> Thanks!
> Gerry
>

Re: [RFC PATCH 05/14] PCI: add access functions for PCIe capabilities to hide PCIe spec differences

[Jiang Liu]

On 2012-7-11 11:40, Bjorn Helgaas wrote:

>> Good point. Return success when reading unimplemented registeres, that
>> may simplify code. For we still should return -EINVAL when writing
>> unimplemented registers, right?
> 
> Yeah, I guess it's OK to return -EINVAL when *writing* to an
> unimplemented register.  Hopefully the caller is structured such that
> we don't even try to write in that case.  It'd be interesting to audit
> the callers and explore that, but I haven't done that.
Hi Bjorn,
	Seems it would be better to return error code for unimplemented
registers, otherwise following code will becomes more complex. A special
error code for unimplemented registers, such as -EIO?

static void rtl_disable_clock_request(struct pci_dev *pdev)
{
        u16 ctl;

        if (!pci_pcie_capability_read_word(pdev, PCI_EXP_LNKCTL, &ctl)) {
                ctl &= ~PCI_EXP_LNKCTL_CLKREQ_EN;
                pci_pcie_capability_write_word(pdev, PCI_EXP_LNKCTL, ctl);
        }
}

Re: [RFC PATCH 01/14] PCI: add pcie_flags into struct pci_dev to cache PCIe capabilities register

[Taku Izumi]

On Tue, 10 Jul 2012 23:54:02 +0800
Jiang Liu <liuj97@gmail.com> wrote:

> From: Yijing Wang <wangyijing@huawei.com>
> 
> From: Yijing Wang <wangyijing@huawei.com>
> 
> Since PCI Express Capabilities Register is read only, cache its value
> into struct pci_dev to avoid repeatedly calling pci_read_config_*().
> 
> Signed-off-by: Yijing Wang <wangyijing@huawei.com>
> Signed-off-by: Jiang Liu <liuj97@gmail.com>
> ---
>  drivers/pci/probe.c |    1 +
>  include/linux/pci.h |    1 +
>  2 files changed, 2 insertions(+)
> 
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 6c143b4..65e82e3 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -929,6 +929,7 @@ void set_pcie_port_type(struct pci_dev *pdev)
>  	pdev->is_pcie = 1;
>  	pdev->pcie_cap = pos;
>  	pci_read_config_word(pdev, pos + PCI_EXP_FLAGS, &reg16);
> +	pdev->pcie_flags = reg16;
>  	pdev->pcie_type = (reg16 & PCI_EXP_FLAGS_TYPE) >> 4;
>  	pci_read_config_word(pdev, pos + PCI_EXP_DEVCAP, &reg16);
>  	pdev->pcie_mpss = reg16 & PCI_EXP_DEVCAP_PAYLOAD;
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 5faa831..f4a7ad6 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -258,6 +258,7 @@ struct pci_dev {
>  	u8		pcie_mpss:3;	/* PCI-E Max Payload Size Supported */
>  	u8		rom_base_reg;	/* which config register controls the ROM */
>  	u8		pin;  		/* which interrupt pin this device uses */
> +	u16		pcie_flags;	/* cached PCI-E Capabilities Register */

 "xxx_flags" sounds like a bit flag. This variable stores a value of PCIe capability 
 register, doesn't it?   How about "pcie_cap_reg" ?

>  
>  	struct pci_driver *driver;	/* which driver has allocated this device */
>  	u64		dma_mask;	/* Mask of the bits of bus address this
> -- 
> 1.7.9.5
> 
> --
> 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
> 


-- 
Taku Izumi <izumi.taku@jp.fujitsu.com>

Re: [RFC PATCH 01/14] PCI: add pcie_flags into struct pci_dev to cache PCIe capabilities register

[Jiang Liu]

On 07/11/2012 05:01 PM, Taku Izumi wrote:
>> +++ b/include/linux/pci.h
>> @@ -258,6 +258,7 @@ struct pci_dev {
>>  	u8		pcie_mpss:3;	/* PCI-E Max Payload Size Supported */
>>  	u8		rom_base_reg;	/* which config register controls the ROM */
>>  	u8		pin;  		/* which interrupt pin this device uses */
>> +	u16		pcie_flags;	/* cached PCI-E Capabilities Register */
> 
>  "xxx_flags" sounds like a bit flag. This variable stores a value of PCIe capability 
>  register, doesn't it?   How about "pcie_cap_reg" ?
Hi Taku,
	Good suggestion, will change it.

	Thanks!
	Gerry

Re: [RFC PATCH 05/14] PCI: add access functions for PCIe capabilities to hide PCIe spec differences

[Bjorn Helgaas]

On Wed, Jul 11, 2012 at 12:40 AM, Jiang Liu <jiang.liu@huawei.com> wrote:
> On 2012-7-11 11:40, Bjorn Helgaas wrote:
>
>>> Good point. Return success when reading unimplemented registeres, that
>>> may simplify code. For we still should return -EINVAL when writing
>>> unimplemented registers, right?
>>
>> Yeah, I guess it's OK to return -EINVAL when *writing* to an
>> unimplemented register.  Hopefully the caller is structured such that
>> we don't even try to write in that case.  It'd be interesting to audit
>> the callers and explore that, but I haven't done that.
> Hi Bjorn,
>         Seems it would be better to return error code for unimplemented
> registers, otherwise following code will becomes more complex. A special
> error code for unimplemented registers, such as -EIO?

I think you're asking about returning error for *reads* of
unimplemented registers?  I guess I still think it's OK to completely
hide the v1 nastiness inside these accessors, and return success with
a zero value when reading.  Having several different error returns
seems like overkill for this case.  Nobody wants to distinguish
between different reasons for failure.

I'm actually not sure that it's worth returning an error even when
*writing* an unimplemented register.  What if we return success and
just drop the write?

Maybe these should even be void functions.  It feels like the only
real use of the return value is to detect programmer error, and I
don't think that's very effective.  If we remove the return values,
people will have to focus on the *data*, which seems more important
anyway.

> static void rtl_disable_clock_request(struct pci_dev *pdev)
> {
>         u16 ctl;
>
>         if (!pci_pcie_capability_read_word(pdev, PCI_EXP_LNKCTL, &ctl)) {
>                 ctl &= ~PCI_EXP_LNKCTL_CLKREQ_EN;
>                 pci_pcie_capability_write_word(pdev, PCI_EXP_LNKCTL, ctl);
>         }
> }

I would write that as:

    if (!pci_is_pcie(pdev)
        return;

    pci_pcie_capability_read_word(pdev, PCI_EXP_LNKCTL, &ctl);
    if (ctl & PCI_EXP_LNKCTL_CLKREQ_EN)
        pci_pcie_capability_write_word(pdev, PCI_EXP_LNKCTL, ctl &
~PCI_EXP_LNKCTL_CLKREQ_EN);

which does the right thing regardless of what we do for return values,
and saves a config write in the case where LNKCTL is implemented and
CLKREQ_EN is already cleared.

Bjorn

Re: [RFC PATCH 05/14] PCI: add access functions for PCIe capabilities to hide PCIe spec differences

[Jiang Liu]

On 2012-7-12 1:52, Bjorn Helgaas wrote:
>> Hi Bjorn,
>>         Seems it would be better to return error code for unimplemented
>> registers, otherwise following code will becomes more complex. A special
>> error code for unimplemented registers, such as -EIO?
> 
> I think you're asking about returning error for *reads* of
> unimplemented registers?  I guess I still think it's OK to completely
> hide the v1 nastiness inside these accessors, and return success with
> a zero value when reading.  Having several different error returns
> seems like overkill for this case.  Nobody wants to distinguish
> between different reasons for failure.
> 
> I'm actually not sure that it's worth returning an error even when
> *writing* an unimplemented register.  What if we return success and
> just drop the write?
> 
> Maybe these should even be void functions.  It feels like the only
> real use of the return value is to detect programmer error, and I
> don't think that's very effective.  If we remove the return values,
> people will have to focus on the *data*, which seems more important
> anyway.
Hi Bjorn,
	It's a little risk to change these PCIe capabilities access
functions as void. On some platform with hardware error detecting/correcting
capabilities, such as EEH on Power, it would be better to return
error code if hardware error happens during accessing configuration registers.
	As I know, coming Intel Xeon processor may provide PCIe hardware
error detecting capability similar to EEH on power.

>> static void rtl_disable_clock_request(struct pci_dev *pdev)
>> {
>>         u16 ctl;
>>
>>         if (!pci_pcie_capability_read_word(pdev, PCI_EXP_LNKCTL, &ctl)) {
>>                 ctl &= ~PCI_EXP_LNKCTL_CLKREQ_EN;
>>                 pci_pcie_capability_write_word(pdev, PCI_EXP_LNKCTL, ctl);
>>         }
>> }
> 
> I would write that as:
> 
>     if (!pci_is_pcie(pdev)
>         return;
> 
>     pci_pcie_capability_read_word(pdev, PCI_EXP_LNKCTL, &ctl);
>     if (ctl & PCI_EXP_LNKCTL_CLKREQ_EN)
>         pci_pcie_capability_write_word(pdev, PCI_EXP_LNKCTL, ctl &
> ~PCI_EXP_LNKCTL_CLKREQ_EN);
> 
> which does the right thing regardless of what we do for return values,
> and saves a config write in the case where LNKCTL is implemented and
> CLKREQ_EN is already cleared.
When clearing a flag, we could do that. But if we are trying to set a
flag, it would be better to make sure the target register does exist.

Re: [RFC PATCH 05/14] PCI: add access functions for PCIe capabilities to hide PCIe spec differences

[Bjorn Helgaas]

On Wed, Jul 11, 2012 at 8:56 PM, Jiang Liu <jiang.liu@huawei.com> wrote:
> On 2012-7-12 1:52, Bjorn Helgaas wrote:
>>> Hi Bjorn,
>>>         Seems it would be better to return error code for unimplemented
>>> registers, otherwise following code will becomes more complex. A special
>>> error code for unimplemented registers, such as -EIO?
>>
>> I think you're asking about returning error for *reads* of
>> unimplemented registers?  I guess I still think it's OK to completely
>> hide the v1 nastiness inside these accessors, and return success with
>> a zero value when reading.  Having several different error returns
>> seems like overkill for this case.  Nobody wants to distinguish
>> between different reasons for failure.
>>
>> I'm actually not sure that it's worth returning an error even when
>> *writing* an unimplemented register.  What if we return success and
>> just drop the write?
>>
>> Maybe these should even be void functions.  It feels like the only
>> real use of the return value is to detect programmer error, and I
>> don't think that's very effective.  If we remove the return values,
>> people will have to focus on the *data*, which seems more important
>> anyway.
> Hi Bjorn,
>         It's a little risk to change these PCIe capabilities access
> functions as void. On some platform with hardware error detecting/correcting
> capabilities, such as EEH on Power, it would be better to return
> error code if hardware error happens during accessing configuration registers.
>         As I know, coming Intel Xeon processor may provide PCIe hardware
> error detecting capability similar to EEH on power.

I guess I'm playing devil's advocate here.  As a general rule, people
don't check the return value of pci_read_config_*() or
pci_write_config_*().  Unless you change them all, most callers of
pci_pcie_capability_read_*() and _write_*() won't check the returns
either.  So I'm not sure return values are an effective way to detect
those hardware errors.

How do these EEH errors get detected or reported today?  Do the
drivers check every config access for success?  Adding those checks
and figuring out how to handle errors at every possible point doesn't
seem like a recipe for success.

>>> static void rtl_disable_clock_request(struct pci_dev *pdev)
>>> {
>>>         u16 ctl;
>>>
>>>         if (!pci_pcie_capability_read_word(pdev, PCI_EXP_LNKCTL, &ctl)) {
>>>                 ctl &= ~PCI_EXP_LNKCTL_CLKREQ_EN;
>>>                 pci_pcie_capability_write_word(pdev, PCI_EXP_LNKCTL, ctl);
>>>         }
>>> }
>>
>> I would write that as:
>>
>>     if (!pci_is_pcie(pdev)
>>         return;
>>
>>     pci_pcie_capability_read_word(pdev, PCI_EXP_LNKCTL, &ctl);
>>     if (ctl & PCI_EXP_LNKCTL_CLKREQ_EN)
>>         pci_pcie_capability_write_word(pdev, PCI_EXP_LNKCTL, ctl &
>> ~PCI_EXP_LNKCTL_CLKREQ_EN);
>>
>> which does the right thing regardless of what we do for return values,
>> and saves a config write in the case where LNKCTL is implemented and
>> CLKREQ_EN is already cleared.
> When clearing a flag, we could do that. But if we are trying to set a
> flag, it would be better to make sure the target register does exist.
>

Re: [RFC PATCH 05/14] PCI: add access functions for PCIe capabilities to hide PCIe spec differences

[Jiang Liu]

On 07/13/2012 04:49 AM, Bjorn Helgaas wrote:
>> Hi Bjorn,
>>         It's a little risk to change these PCIe capabilities access
>> functions as void. On some platform with hardware error detecting/correcting
>> capabilities, such as EEH on Power, it would be better to return
>> error code if hardware error happens during accessing configuration registers.
>>         As I know, coming Intel Xeon processor may provide PCIe hardware
>> error detecting capability similar to EEH on power.
> 
> I guess I'm playing devil's advocate here.  As a general rule, people
> don't check the return value of pci_read_config_*() or
> pci_write_config_*().  Unless you change them all, most callers of
> pci_pcie_capability_read_*() and _write_*() won't check the returns
> either.  So I'm not sure return values are an effective way to detect
> those hardware errors.
> 
> How do these EEH errors get detected or reported today?  Do the
> drivers check every config access for success?  Adding those checks
> and figuring out how to handle errors at every possible point doesn't
> seem like a recipe for success.

Hi Bjorn,
	Sorry for later reply, on travel these days.
	Yeah, it's true that most driver doesn't check return values of configuration
access functions, but there are still some drivers which do check return value of
pci_read_config_xxx(). For example, pciehp driver checks return value of CFG access
functions.

	It's not realistic to enhance all drivers, but we may focus on a small set of
drivers for hardwares on specific high-end servers. For RAS features, we can never provide
perfect solutions, so we prefer some improvements. After all a small improvement is still
an improvement:)

	I'm only familiar with PCI on IA64 and x86. For PowerPC, I just know that the OS
may query firmware whether there's some hardware faults if pci_cfg_read_xxx() returns
all 1s. For PCI on IA64, SAL may handle PCI hardware errors and return error code to
pci_cfg_read_xxx(). For x86, I think it will have some mechanisms to report hardware faults
like SAL on IA64.

	So how about keeping consistence with pci_cfg_read_xxx() and pci_user_cfg_read_xxx()?
	Thanks!
	Gerry

Re: [RFC PATCH 05/14] PCI: add access functions for PCIe capabilities to hide PCIe spec differences

[Bjorn Helgaas]

On Sun, Jul 15, 2012 at 10:47 AM, Jiang Liu <liuj97@gmail.com> wrote:
> On 07/13/2012 04:49 AM, Bjorn Helgaas wrote:
>>> Hi Bjorn,
>>>         It's a little risk to change these PCIe capabilities access
>>> functions as void. On some platform with hardware error detecting/correcting
>>> capabilities, such as EEH on Power, it would be better to return
>>> error code if hardware error happens during accessing configuration registers.
>>>         As I know, coming Intel Xeon processor may provide PCIe hardware
>>> error detecting capability similar to EEH on power.
>>
>> I guess I'm playing devil's advocate here.  As a general rule, people
>> don't check the return value of pci_read_config_*() or
>> pci_write_config_*().  Unless you change them all, most callers of
>> pci_pcie_capability_read_*() and _write_*() won't check the returns
>> either.  So I'm not sure return values are an effective way to detect
>> those hardware errors.
>>
>> How do these EEH errors get detected or reported today?  Do the
>> drivers check every config access for success?  Adding those checks
>> and figuring out how to handle errors at every possible point doesn't
>> seem like a recipe for success.
>
> Hi Bjorn,
>         Sorry for later reply, on travel these days.
>         Yeah, it's true that most driver doesn't check return values of configuration
> access functions, but there are still some drivers which do check return value of
> pci_read_config_xxx(). For example, pciehp driver checks return value of CFG access
> functions.
>
>         It's not realistic to enhance all drivers, but we may focus on a small set of
> drivers for hardwares on specific high-end servers. For RAS features, we can never provide
> perfect solutions, so we prefer some improvements. After all a small improvement is still
> an improvement:)
>
>         I'm only familiar with PCI on IA64 and x86. For PowerPC, I just know that the OS
> may query firmware whether there's some hardware faults if pci_cfg_read_xxx() returns
> all 1s. For PCI on IA64, SAL may handle PCI hardware errors and return error code to
> pci_cfg_read_xxx(). For x86, I think it will have some mechanisms to report hardware faults
> like SAL on IA64.
>
>         So how about keeping consistence with pci_cfg_read_xxx() and pci_user_cfg_read_xxx()?

My goal is "the caller should never have to know whether this is a v1
or v2 capability."  Returning any error other than one passed along
from pci_read/write_config_xxx() means we miss that goal.  Perhaps the
goal is unattainable, but I haven't been convinced yet.

I think hardware error detection is irrelevant to this discussion.
After reading Documentation/PCI/pci-error-recovery.txt, I'm even less
convinced that checking return values from pci_read/write_config_xxx()
or pci_pcie_capability_read/write_xxx() is a useful way to detect
hardware errors.

Having drivers detect hardware failures by checking for config access
errors is neither necessary nor sufficient.  It's not necessary
because a platform can implement a config accessor that checks *every*
access and reports failures to the driver via the pci_error_handler
framework.  It's not sufficient because config accesses are rare
(usually only at init-time), and hardware failures may happen at
arbitrary other times.

In my opinion, the only relevant question is whether a caller of
pci_pcie_capability_read/write_xxx() needs to know whether a register
is implemented (i.e., we have a v2 capability) or not.  For reads, I
don't think there's a case where fabricating a value of zero when
reading an unimplemented register is a problem.

Writes are obviously more interesting, but I'm still not sure there's
a case where silently dropping a write to an unimplemented register is
a problem.  The "capability" registers are read-only, so there's no
problem if we drop writes to them.  The "status" registers are
generally RO or RW1C, where it's only meaningful to write a non-zero
value if you're previously *read* a non-zero value.  The "control"
registers are often RW, of course, but generally it's only meaningful
to write a non-zero value when a non-zero bit in the "capability"
register has previously told you that something is supported.

Bjorn

Re: [RFC PATCH 05/14] PCI: add access functions for PCIe capabilities to hide PCIe spec differences

[Don Dutile]

On 07/16/2012 01:29 PM, Bjorn Helgaas wrote:
> On Sun, Jul 15, 2012 at 10:47 AM, Jiang Liu<liuj97@gmail.com>  wrote:
>> On 07/13/2012 04:49 AM, Bjorn Helgaas wrote:
>>>> Hi Bjorn,
>>>>          It's a little risk to change these PCIe capabilities access
>>>> functions as void. On some platform with hardware error detecting/correcting
>>>> capabilities, such as EEH on Power, it would be better to return
>>>> error code if hardware error happens during accessing configuration registers.
>>>>          As I know, coming Intel Xeon processor may provide PCIe hardware
>>>> error detecting capability similar to EEH on power.
>>>
>>> I guess I'm playing devil's advocate here.  As a general rule, people
>>> don't check the return value of pci_read_config_*() or
>>> pci_write_config_*().  Unless you change them all, most callers of
>>> pci_pcie_capability_read_*() and _write_*() won't check the returns
>>> either.  So I'm not sure return values are an effective way to detect
>>> those hardware errors.
>>>
>>> How do these EEH errors get detected or reported today?  Do the
>>> drivers check every config access for success?  Adding those checks
>>> and figuring out how to handle errors at every possible point doesn't
>>> seem like a recipe for success.
>>
>> Hi Bjorn,
>>          Sorry for later reply, on travel these days.
>>          Yeah, it's true that most driver doesn't check return values of configuration
>> access functions, but there are still some drivers which do check return value of
>> pci_read_config_xxx(). For example, pciehp driver checks return value of CFG access
>> functions.
>>
>>          It's not realistic to enhance all drivers, but we may focus on a small set of
>> drivers for hardwares on specific high-end servers. For RAS features, we can never provide
>> perfect solutions, so we prefer some improvements. After all a small improvement is still
>> an improvement:)
>>
>>          I'm only familiar with PCI on IA64 and x86. For PowerPC, I just know that the OS
>> may query firmware whether there's some hardware faults if pci_cfg_read_xxx() returns
>> all 1s. For PCI on IA64, SAL may handle PCI hardware errors and return error code to
>> pci_cfg_read_xxx(). For x86, I think it will have some mechanisms to report hardware faults
>> like SAL on IA64.
>>
>>          So how about keeping consistence with pci_cfg_read_xxx() and pci_user_cfg_read_xxx()?
>
> My goal is "the caller should never have to know whether this is a v1
> or v2 capability."  Returning any error other than one passed along
> from pci_read/write_config_xxx() means we miss that goal.  Perhaps the
> goal is unattainable, but I haven't been convinced yet.
>
> I think hardware error detection is irrelevant to this discussion.
> After reading Documentation/PCI/pci-error-recovery.txt, I'm even less
> convinced that checking return values from pci_read/write_config_xxx()
> or pci_pcie_capability_read/write_xxx() is a useful way to detect
> hardware errors.
>
> Having drivers detect hardware failures by checking for config access
> errors is neither necessary nor sufficient.  It's not necessary
> because a platform can implement a config accessor that checks *every*
> access and reports failures to the driver via the pci_error_handler
> framework.  It's not sufficient because config accesses are rare
> (usually only at init-time), and hardware failures may happen at
> arbitrary other times.
>
> In my opinion, the only relevant question is whether a caller of
> pci_pcie_capability_read/write_xxx() needs to know whether a register
> is implemented (i.e., we have a v2 capability) or not.  For reads, I
> don't think there's a case where fabricating a value of zero when
> reading an unimplemented register is a problem.
>
> Writes are obviously more interesting, but I'm still not sure there's
> a case where silently dropping a write to an unimplemented register is
> a problem.  The "capability" registers are read-only, so there's no
> problem if we drop writes to them.  The "status" registers are
> generally RO or RW1C, where it's only meaningful to write a non-zero
> value if you're previously *read* a non-zero value.  The "control"
> registers are often RW, of course, but generally it's only meaningful
> to write a non-zero value when a non-zero bit in the "capability"
> register has previously told you that something is supported.
>
> Bjorn
+1
Returning 0 on capability reads -- due to unimplemented
features/register or due to failures,
should translate into the (core) code doing no writes.
Thus, the reason I suggested returning 0 on failure in original posting.

Re: [RFC PATCH 05/14] PCI: add access functions for PCIe capabilities to hide PCIe spec differences

[Jiang Liu]

On 07/17/2012 01:29 AM, Bjorn Helgaas wrote:
> On Sun, Jul 15, 2012 at 10:47 AM, Jiang Liu <liuj97@gmail.com> wrote:
>> On 07/13/2012 04:49 AM, Bjorn Helgaas wrote:
>>>> Hi Bjorn,
>>>>         It's a little risk to change these PCIe capabilities access
>>>> functions as void. On some platform with hardware error detecting/correcting
>>>> capabilities, such as EEH on Power, it would be better to return
>>>> error code if hardware error happens during accessing configuration registers.
>>>>         As I know, coming Intel Xeon processor may provide PCIe hardware
>>>> error detecting capability similar to EEH on power.
>>>
>>> I guess I'm playing devil's advocate here.  As a general rule, people
>>> don't check the return value of pci_read_config_*() or
>>> pci_write_config_*().  Unless you change them all, most callers of
>>> pci_pcie_capability_read_*() and _write_*() won't check the returns
>>> either.  So I'm not sure return values are an effective way to detect
>>> those hardware errors.
>>>
>>> How do these EEH errors get detected or reported today?  Do the
>>> drivers check every config access for success?  Adding those checks
>>> and figuring out how to handle errors at every possible point doesn't
>>> seem like a recipe for success.
>>
>> Hi Bjorn,
>>         Sorry for later reply, on travel these days.
>>         Yeah, it's true that most driver doesn't check return values of configuration
>> access functions, but there are still some drivers which do check return value of
>> pci_read_config_xxx(). For example, pciehp driver checks return value of CFG access
>> functions.
>>
>>         It's not realistic to enhance all drivers, but we may focus on a small set of
>> drivers for hardwares on specific high-end servers. For RAS features, we can never provide
>> perfect solutions, so we prefer some improvements. After all a small improvement is still
>> an improvement:)
>>
>>         I'm only familiar with PCI on IA64 and x86. For PowerPC, I just know that the OS
>> may query firmware whether there's some hardware faults if pci_cfg_read_xxx() returns
>> all 1s. For PCI on IA64, SAL may handle PCI hardware errors and return error code to
>> pci_cfg_read_xxx(). For x86, I think it will have some mechanisms to report hardware faults
>> like SAL on IA64.
>>
>>         So how about keeping consistence with pci_cfg_read_xxx() and pci_user_cfg_read_xxx()?
> 
> My goal is "the caller should never have to know whether this is a v1
> or v2 capability."  Returning any error other than one passed along
> from pci_read/write_config_xxx() means we miss that goal.  Perhaps the
> goal is unattainable, but I haven't been convinced yet.
> 
> I think hardware error detection is irrelevant to this discussion.
> After reading Documentation/PCI/pci-error-recovery.txt, I'm even less
> convinced that checking return values from pci_read/write_config_xxx()
> or pci_pcie_capability_read/write_xxx() is a useful way to detect
> hardware errors.
> 
> Having drivers detect hardware failures by checking for config access
> errors is neither necessary nor sufficient.  It's not necessary
> because a platform can implement a config accessor that checks *every*
> access and reports failures to the driver via the pci_error_handler
> framework.  It's not sufficient because config accesses are rare
> (usually only at init-time), and hardware failures may happen at
> arbitrary other times.
> 
> In my opinion, the only relevant question is whether a caller of
> pci_pcie_capability_read/write_xxx() needs to know whether a register
> is implemented (i.e., we have a v2 capability) or not.  For reads, I
> don't think there's a case where fabricating a value of zero when
> reading an unimplemented register is a problem.
> 
> Writes are obviously more interesting, but I'm still not sure there's
> a case where silently dropping a write to an unimplemented register is
> a problem.  The "capability" registers are read-only, so there's no
> problem if we drop writes to them.  The "status" registers are
> generally RO or RW1C, where it's only meaningful to write a non-zero
> value if you're previously *read* a non-zero value.  The "control"
> registers are often RW, of course, but generally it's only meaningful
> to write a non-zero value when a non-zero bit in the "capability"
> register has previously told you that something is supported.
Hi Bjorn,
	I'm convinced by you that we shouldn't return error when accessing
an unimplemented PCIe capabilities register and just hide the differences 
among V1/V2 specification. Then how about returning error from
"pci_read/write_config_xxx()" to callers of pci_pcie_capabilitiy_read/write_xxx()?
I still prefer to return error code to keep consistence with other configuration
space access interfaces:)
	Thanks!
	Gerry

> 
> Bjorn
> 

Re: [RFC PATCH 05/14] PCI: add access functions for PCIe capabilities to hide PCIe spec differences

[Bjorn Helgaas]

On Mon, Jul 16, 2012 at 6:09 PM, Jiang Liu <liuj97@gmail.com> wrote:
> On 07/17/2012 01:29 AM, Bjorn Helgaas wrote:
>> On Sun, Jul 15, 2012 at 10:47 AM, Jiang Liu <liuj97@gmail.com> wrote:
>>> On 07/13/2012 04:49 AM, Bjorn Helgaas wrote:
>>>>> Hi Bjorn,
>>>>>         It's a little risk to change these PCIe capabilities access
>>>>> functions as void. On some platform with hardware error detecting/correcting
>>>>> capabilities, such as EEH on Power, it would be better to return
>>>>> error code if hardware error happens during accessing configuration registers.
>>>>>         As I know, coming Intel Xeon processor may provide PCIe hardware
>>>>> error detecting capability similar to EEH on power.
>>>>
>>>> I guess I'm playing devil's advocate here.  As a general rule, people
>>>> don't check the return value of pci_read_config_*() or
>>>> pci_write_config_*().  Unless you change them all, most callers of
>>>> pci_pcie_capability_read_*() and _write_*() won't check the returns
>>>> either.  So I'm not sure return values are an effective way to detect
>>>> those hardware errors.
>>>>
>>>> How do these EEH errors get detected or reported today?  Do the
>>>> drivers check every config access for success?  Adding those checks
>>>> and figuring out how to handle errors at every possible point doesn't
>>>> seem like a recipe for success.
>>>
>>> Hi Bjorn,
>>>         Sorry for later reply, on travel these days.
>>>         Yeah, it's true that most driver doesn't check return values of configuration
>>> access functions, but there are still some drivers which do check return value of
>>> pci_read_config_xxx(). For example, pciehp driver checks return value of CFG access
>>> functions.
>>>
>>>         It's not realistic to enhance all drivers, but we may focus on a small set of
>>> drivers for hardwares on specific high-end servers. For RAS features, we can never provide
>>> perfect solutions, so we prefer some improvements. After all a small improvement is still
>>> an improvement:)
>>>
>>>         I'm only familiar with PCI on IA64 and x86. For PowerPC, I just know that the OS
>>> may query firmware whether there's some hardware faults if pci_cfg_read_xxx() returns
>>> all 1s. For PCI on IA64, SAL may handle PCI hardware errors and return error code to
>>> pci_cfg_read_xxx(). For x86, I think it will have some mechanisms to report hardware faults
>>> like SAL on IA64.
>>>
>>>         So how about keeping consistence with pci_cfg_read_xxx() and pci_user_cfg_read_xxx()?
>>
>> My goal is "the caller should never have to know whether this is a v1
>> or v2 capability."  Returning any error other than one passed along
>> from pci_read/write_config_xxx() means we miss that goal.  Perhaps the
>> goal is unattainable, but I haven't been convinced yet.
>>
>> I think hardware error detection is irrelevant to this discussion.
>> After reading Documentation/PCI/pci-error-recovery.txt, I'm even less
>> convinced that checking return values from pci_read/write_config_xxx()
>> or pci_pcie_capability_read/write_xxx() is a useful way to detect
>> hardware errors.
>>
>> Having drivers detect hardware failures by checking for config access
>> errors is neither necessary nor sufficient.  It's not necessary
>> because a platform can implement a config accessor that checks *every*
>> access and reports failures to the driver via the pci_error_handler
>> framework.  It's not sufficient because config accesses are rare
>> (usually only at init-time), and hardware failures may happen at
>> arbitrary other times.
>>
>> In my opinion, the only relevant question is whether a caller of
>> pci_pcie_capability_read/write_xxx() needs to know whether a register
>> is implemented (i.e., we have a v2 capability) or not.  For reads, I
>> don't think there's a case where fabricating a value of zero when
>> reading an unimplemented register is a problem.
>>
>> Writes are obviously more interesting, but I'm still not sure there's
>> a case where silently dropping a write to an unimplemented register is
>> a problem.  The "capability" registers are read-only, so there's no
>> problem if we drop writes to them.  The "status" registers are
>> generally RO or RW1C, where it's only meaningful to write a non-zero
>> value if you're previously *read* a non-zero value.  The "control"
>> registers are often RW, of course, but generally it's only meaningful
>> to write a non-zero value when a non-zero bit in the "capability"
>> register has previously told you that something is supported.
> Hi Bjorn,
>         I'm convinced by you that we shouldn't return error when accessing
> an unimplemented PCIe capabilities register and just hide the differences
> among V1/V2 specification. Then how about returning error from
> "pci_read/write_config_xxx()" to callers of pci_pcie_capabilitiy_read/write_xxx()?
> I still prefer to return error code to keep consistence with other configuration
> space access interfaces:)

I think it's fine to return the status of pci_read/write_config_xxx(), e.g.,

    int pci_pcie_cap_read_word(...)
    {
        ...
        if (<implemented>)
            return pci_read_config_word(...);

        ...
        return 0;
    }

Bjorn

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

[Bjorn Helgaas]

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

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

[Jiang Liu]

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

[PATCH v2] PCI: allow acpiphp to handle PCIe ports w/o native PCIe hotplug capability

[Jiang Liu]

From: Jiang Liu <jiang.liu@huawei.com>

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.

v1->v2: use PCIe capability accessors to read PCI_EXP_SLTCAP register

Acked-by: Rafael J. Wysocki <rjw@sisk.pl>
Signed-off-by: Jiang Liu <liuj97@gmail.com>
---
 drivers/pci/hotplug/acpiphp_glue.c |   41 +++++++++++++++++++++++++++---------
 1 file changed, 31 insertions(+), 10 deletions(-)

diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
index ad6fd66..e6da392 100644
--- a/drivers/pci/hotplug/acpiphp_glue.c
+++ b/drivers/pci/hotplug/acpiphp_glue.c
@@ -115,6 +115,35 @@ static const struct acpi_dock_ops acpiphp_dock_ops = {
 	.handler = handle_hotplug_event_func,
 };
 
+/* Check whether the PCI device is managed by native PCIe hotplug driver */
+static bool device_is_managed_by_native_pciehp(struct pci_dev *pdev)
+{
+	u32 reg32;
+	acpi_handle tmp;
+	struct acpi_pci_root *root;
+
+	/* Check whether the PCIe port supports native PCIe hotplug */
+	if (pcie_capability_read_dword(pdev, PCI_EXP_SLTCAP, &reg32))
+		return false;
+	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)
@@ -142,16 +171,8 @@ register_slot(acpi_handle handle, u32 lvl, void *context, void **rv)
 	function = adr & 0xffff;
 
 	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;
 
 	newfunc = kzalloc(sizeof(struct acpiphp_func), GFP_KERNEL);
 	if (!newfunc)
-- 
1.7.9.5

Re: [PATCH v2] PCI: allow acpiphp to handle PCIe ports w/o native PCIe hotplug capability

[Bjorn Helgaas]

On Wed, Aug 22, 2012 at 9:16 AM, Jiang Liu <liuj97@gmail.com> wrote:
> From: Jiang Liu <jiang.liu@huawei.com>
>
> 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.
>
> v1->v2: use PCIe capability accessors to read PCI_EXP_SLTCAP register
>
> Acked-by: Rafael J. Wysocki <rjw@sisk.pl>
> Signed-off-by: Jiang Liu <liuj97@gmail.com>

I tweaked the changelog (typos, indentation) and applied this to my
"pci/jiang-acpiphp" branch.  Kenji acked a previous version, but I
didn't add it here because this is slightly updated.

If the PLX8696 supports moving downstream ports between upstream
ports, it seems like a deficiency if the upstream port doesn't support
native hotplug.  If I understand correctly, this patch allows the use
of acpiphp for "hotplug" of the downstream ports, but that is only an
option if the PLX8696 is in the ACPI namespace.

It seems like the lack of PLX8696 native hotplug for upstream ports
mean this virtual switch migration can only be done if the PLX8696 is
soldered into the system -- for instance, we don't have any way to
deal with an 8696 on a plug-in card because ACPI won't know anything
about that device.

I'm not objecting to this patch; I'm just trying to understand the
situation better.

> ---
>  drivers/pci/hotplug/acpiphp_glue.c |   41 +++++++++++++++++++++++++++---------
>  1 file changed, 31 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
> index ad6fd66..e6da392 100644
> --- a/drivers/pci/hotplug/acpiphp_glue.c
> +++ b/drivers/pci/hotplug/acpiphp_glue.c
> @@ -115,6 +115,35 @@ static const struct acpi_dock_ops acpiphp_dock_ops = {
>         .handler = handle_hotplug_event_func,
>  };
>
> +/* Check whether the PCI device is managed by native PCIe hotplug driver */
> +static bool device_is_managed_by_native_pciehp(struct pci_dev *pdev)
> +{
> +       u32 reg32;
> +       acpi_handle tmp;
> +       struct acpi_pci_root *root;
> +
> +       /* Check whether the PCIe port supports native PCIe hotplug */
> +       if (pcie_capability_read_dword(pdev, PCI_EXP_SLTCAP, &reg32))
> +               return false;
> +       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)
> @@ -142,16 +171,8 @@ register_slot(acpi_handle handle, u32 lvl, void *context, void **rv)
>         function = adr & 0xffff;
>
>         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;
>
>         newfunc = kzalloc(sizeof(struct acpiphp_func), GFP_KERNEL);
>         if (!newfunc)
> --
> 1.7.9.5
>

Re: [PATCH v2] PCI: allow acpiphp to handle PCIe ports w/o native PCIe hotplug capability

[Jiang Liu]

On 09/25/2012 06:10 AM, Bjorn Helgaas wrote:
> On Wed, Aug 22, 2012 at 9:16 AM, Jiang Liu <liuj97@gmail.com> wrote:
>> From: Jiang Liu <jiang.liu@huawei.com>
>>
>> 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.
>>
>> v1->v2: use PCIe capability accessors to read PCI_EXP_SLTCAP register
>>
>> Acked-by: Rafael J. Wysocki <rjw@sisk.pl>
>> Signed-off-by: Jiang Liu <liuj97@gmail.com>
> 
> I tweaked the changelog (typos, indentation) and applied this to my
> "pci/jiang-acpiphp" branch.  Kenji acked a previous version, but I
> didn't add it here because this is slightly updated.
> 
> If the PLX8696 supports moving downstream ports between upstream
> ports, it seems like a deficiency if the upstream port doesn't support
> native hotplug.  If I understand correctly, this patch allows the use
> of acpiphp for "hotplug" of the downstream ports, but that is only an
> option if the PLX8696 is in the ACPI namespace.
> 
> It seems like the lack of PLX8696 native hotplug for upstream ports
> mean this virtual switch migration can only be done if the PLX8696 is
> soldered into the system -- for instance, we don't have any way to
> deal with an 8696 on a plug-in card because ACPI won't know anything
> about that device.
Hi Bjorn,
	That's true, we can't deal with 8696 on add-in cards yet. The platform
we are working with is:
	The motherboard has a specially designed slot for a root port, which
could only be used to connect an IO-extension box with 8696. So the BIOS could
build ACPI objects for the 8696 chipset.
	Thanks!
	Gerry