Patchwork PCI: Make pci_find_upstream_pcie_bridge() handle non PCIE VFs well

login
register
mail settings
Submitter tadeusz.struk@intel.com
Date Jan. 8, 2013, 2:32 p.m.
Message ID <50ec2e19.EUrQfXEnsRJiPCTv%tadeusz.struk@intel.com>
Download mbox | patch
Permalink /patch/210406/
State Changes Requested
Headers show

Comments

tadeusz.struk@intel.com - Jan. 8, 2013, 2:32 p.m.
pci_find_upstream_pcie_bridge() doesn't handle well non PCIE VFs
that are part of a PCIE PF device.

Signed-off-by: Tadeusz Struk <tadeusz.struk@intel.com>
---
 drivers/pci/search.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)
Don Dutile - Jan. 8, 2013, 5:05 p.m.
On 01/08/2013 09:32 AM, tadeusz.struk@intel.com wrote:
> pci_find_upstream_pcie_bridge() doesn't handle well non PCIE VFs
> that are part of a PCIE PF device.
>
> Signed-off-by: Tadeusz Struk<tadeusz.struk@intel.com>
> ---
>   drivers/pci/search.c |    6 ++++++
>   1 files changed, 6 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/pci/search.c b/drivers/pci/search.c
> index bf969ba..8ecdab2 100644
> --- a/drivers/pci/search.c
> +++ b/drivers/pci/search.c
> @@ -23,6 +23,8 @@ EXPORT_SYMBOL_GPL(pci_bus_sem);
>    * if the device isn't connected to a PCIe bridge (that is its parent is a
>    * legacy PCI bridge and the bridge is directly connected to bus 0), return its
>    * parent
> + * if the device is a VF that doesn't have PCIe cap,
> + * but the PF is a PCIE, return NULL
>    */
>   struct pci_dev *
>   pci_find_upstream_pcie_bridge(struct pci_dev *pdev)
> @@ -31,6 +33,10 @@ pci_find_upstream_pcie_bridge(struct pci_dev *pdev)
>
>   	if (pci_is_pcie(pdev))
>   		return NULL;
> +
> +	if (pdev->is_virtfn&&  pci_is_pcie(pdev->physfn))
> +		return NULL;
> +
>   	while (1) {
>   		if (pci_is_root_bus(pdev->bus))
>   			break;

The verbage/description is confusing, and
the comment doesn't match the code:
(a) no such thing as a non-PCIe VF -- all VFs
     are PCIe-based.
(b) code says to return null if VF doesn't have PCIe cap,
     but the code checks if pdev is VF and if PF is PCIe,
     which it must be! ...

nack until a better &/or matching explanation of what
the real problem is, and what the solution is trying to do.

- Don

--
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
tadeusz.struk@intel.com - Jan. 9, 2013, 10:27 a.m.
On 01/08/2013 05:05 PM, Don Dutile wrote:

> (a) no such thing as a non-PCIe VF -- all VFs
>     are PCIe-based.

The sriov spec says that a VF doesn't necessarily has to have PCIE cap:
"3.5 PCI Express Capability:
   ...
   PFs and VFs are required to implement this capability ... subject to
   the exceptions and additional requirements described below"

> (b) code says to return null if VF doesn't have PCIe cap,
>     but the code checks if pdev is VF and if PF is PCIe,
>     which it must be! ...

You are right, I should rather check if the VF is not a pcie.

> nack until a better &/or matching explanation of what
> the real problem is, and what the solution is trying to do.

The problem is that I do have a device where VFs are not PCIE and I get this nasty warning.
For now I have a workaround, but it would be nice it Linux would support non PCIE VFs.

Thanks,
T

--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Don Dutile - Jan. 9, 2013, 4:32 p.m.
On 01/09/2013 05:27 AM, Tadeusz Struk wrote:
> On 01/08/2013 05:05 PM, Don Dutile wrote:
>
>> (a) no such thing as a non-PCIe VF -- all VFs
>>      are PCIe-based.
>
> The sriov spec says that a VF doesn't necessarily has to have PCIE cap:
> "3.5 PCI Express Capability:
>     ...
>     PFs and VFs are required to implement this capability ... subject to
>     the exceptions and additional requirements described below"
>
>> (b) code says to return null if VF doesn't have PCIe cap,
>>      but the code checks if pdev is VF and if PF is PCIe,
>>      which it must be! ...
>
> You are right, I should rather check if the VF is not a pcie.
>
Again -- nack! -- SRIOV devices must be PCIe.
Virtualization support expects VFs to be PCIe-based,
and supports things like:
(a) FLR
(b) dev-id tagged source packetting for DMA/IOMMU-mapping/filtering
(c) MSI/MSI-X only
  etc....

>> nack until a better&/or matching explanation of what
>> the real problem is, and what the solution is trying to do.
>
> The problem is that I do have a device where VFs are not PCIE and I get this nasty warning.
> For now I have a workaround, but it would be nice it Linux would support non PCIE VFs.
>
This is a violation of the SRIOV spec.
I do not see how we'll take a patch for violating a core specification,
and then expecting us to hack the core sriov support for it.
At best, a quirk-like hack would be entertained.

Why is this call needed by your VF (or PF) driver ?

> Thanks,
> T
>

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

Patch

diff --git a/drivers/pci/search.c b/drivers/pci/search.c
index bf969ba..8ecdab2 100644
--- a/drivers/pci/search.c
+++ b/drivers/pci/search.c
@@ -23,6 +23,8 @@  EXPORT_SYMBOL_GPL(pci_bus_sem);
  * if the device isn't connected to a PCIe bridge (that is its parent is a
  * legacy PCI bridge and the bridge is directly connected to bus 0), return its
  * parent
+ * if the device is a VF that doesn't have PCIe cap,
+ * but the PF is a PCIE, return NULL
  */
 struct pci_dev *
 pci_find_upstream_pcie_bridge(struct pci_dev *pdev)
@@ -31,6 +33,10 @@  pci_find_upstream_pcie_bridge(struct pci_dev *pdev)
 
 	if (pci_is_pcie(pdev))
 		return NULL;
+
+	if (pdev->is_virtfn && pci_is_pcie(pdev->physfn))
+		return NULL;
+
 	while (1) {
 		if (pci_is_root_bus(pdev->bus))
 			break;