Message ID | CA+sq2Ccok5wtKjCZUkBhhj3WsiqFAoMgHK7LY210aBtMku+8SA@mail.gmail.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Hi Sunil, On Fri, May 16, 2014 at 11:33:04AM +0100, Sunil Kovvuri wrote: > Hi Liviu, > > I am using your ARM64 PCIe patches to write a PCIe host controller > driver for our SOC. I am facing an issue with SR-IOV capable device. > > Consider an PCI Express endpoint connected to a PCI Express > Root Port. The PCI Express endpoint provides PCI-SIG SR-IOV > capabilities with a single physical function and a large number > of virtual functions. The Root Port contains a pci-pci bridge in > between it and the SR-IOV device. > > When the SR-IOV capable device's driver tries to enable sriov > (pci_enable_sriov()) it fails to create/add PCI device for each > virtual function reporting "not enough MMIO resources for SR-IOV". > > In sriov_enable() (drivers/pci/iov.c) > > 296 for (i = 0; i < PCI_SRIOV_NUM_BARS; i++) { > 297 bars |= (1 << (i + PCI_IOV_RESOURCES)); > 298 res = dev->resource + PCI_IOV_RESOURCES + i; > 299 if (res->parent) > 300 nres++; > 301 } > 302 if (nres != iov->nres) { > 303 dev_err(&dev->dev, "not enough MMIO resources for > SR-IOV\n"); > 304 return -ENOMEM; > 305 } > > Here its checking if physical function's IOV resource has a parent or not. > Which is pci-pci bridge in this case. Otherwise it doesn't consider > that resource. > > Added below api to your patch. > This will try to claim a resource while creating a PCI device which > inturn sets 'res->parent'. > > Let me know if this is okay. > > diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c > index 9f29c9a..fbfb48f 100644 > --- a/arch/arm64/kernel/pci.c > +++ b/arch/arm64/kernel/pci.c > @@ -125,6 +125,21 @@ resource_size_t pcibios_align_resource(void > *data, const struct resource *res, > return res->start; > } > > +int pcibios_add_device(struct pci_dev *pdev) > +{ > + unsigned int i, type_mask = IORESOURCE_IO | IORESOURCE_MEM; > + struct resource *res; > + > + for (i = 0; i < PCI_NUM_RESOURCES; i++) { > + res = &pdev->resource[i]; > + if (res->parent || !(res->flags & type_mask)) > + continue; > + pci_claim_resource(pdev, i); > + } > + > I would like not to have to add your patch in this file as I am trying to remove it entirely. I don't have an SR-IOV capable device in my setup so I am not able to debug your problem, but could you have a go and try to figure out why the SR-IOV resources do not get a parent associated with when they get created? Many thanks, Liviu
Hi Liviu, Issue may not be only with SR-IOV resources. I am not an expert with Linux PCI core. But i am trying to understand how even for a non SR-IOV capable device's resource, gets a parent associated with it. When a PCI device driver calls pci_enable_device() which inturn calls 'arch/arm64/kernel/pci.c' pcibios_enable_device() which uses Linux PCI core API 'pci_enable_resources'. This API checks 'res->parent' for all valid resources before enabling them, otherwise it fails. if (!r->parent) { dev_err(&dev->dev, "device not available " "(can't reserve %pR)\n", r); return -EINVAL; } Can you please let me know where this hierarchy is being set for 'pcibios_enable_resources' to work. I could only find request_resource() API which sets the parent and pci_claim_resource() uses this. Thanks, Sunil. On Fri, May 16, 2014 at 6:54 PM, Liviu Dudau <Liviu.Dudau@arm.com> wrote: > Hi Sunil, > > On Fri, May 16, 2014 at 11:33:04AM +0100, Sunil Kovvuri wrote: >> Hi Liviu, >> >> I am using your ARM64 PCIe patches to write a PCIe host controller >> driver for our SOC. I am facing an issue with SR-IOV capable device. >> >> Consider an PCI Express endpoint connected to a PCI Express >> Root Port. The PCI Express endpoint provides PCI-SIG SR-IOV >> capabilities with a single physical function and a large number >> of virtual functions. The Root Port contains a pci-pci bridge in >> between it and the SR-IOV device. >> >> When the SR-IOV capable device's driver tries to enable sriov >> (pci_enable_sriov()) it fails to create/add PCI device for each >> virtual function reporting "not enough MMIO resources for SR-IOV". >> >> In sriov_enable() (drivers/pci/iov.c) >> >> 296 for (i = 0; i < PCI_SRIOV_NUM_BARS; i++) { >> 297 bars |= (1 << (i + PCI_IOV_RESOURCES)); >> 298 res = dev->resource + PCI_IOV_RESOURCES + i; >> 299 if (res->parent) >> 300 nres++; >> 301 } >> 302 if (nres != iov->nres) { >> 303 dev_err(&dev->dev, "not enough MMIO resources for >> SR-IOV\n"); >> 304 return -ENOMEM; >> 305 } >> >> Here its checking if physical function's IOV resource has a parent or not. >> Which is pci-pci bridge in this case. Otherwise it doesn't consider >> that resource. >> >> Added below api to your patch. >> This will try to claim a resource while creating a PCI device which >> inturn sets 'res->parent'. >> >> Let me know if this is okay. >> >> diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c >> index 9f29c9a..fbfb48f 100644 >> --- a/arch/arm64/kernel/pci.c >> +++ b/arch/arm64/kernel/pci.c >> @@ -125,6 +125,21 @@ resource_size_t pcibios_align_resource(void >> *data, const struct resource *res, >> return res->start; >> } >> >> +int pcibios_add_device(struct pci_dev *pdev) >> +{ >> + unsigned int i, type_mask = IORESOURCE_IO | IORESOURCE_MEM; >> + struct resource *res; >> + >> + for (i = 0; i < PCI_NUM_RESOURCES; i++) { >> + res = &pdev->resource[i]; >> + if (res->parent || !(res->flags & type_mask)) >> + continue; >> + pci_claim_resource(pdev, i); >> + } >> + >> > > I would like not to have to add your patch in this file as I am trying to > remove it entirely. I don't have an SR-IOV capable device in my setup so > I am not able to debug your problem, but could you have a go and try to > figure out why the SR-IOV resources do not get a parent associated with > when they get created? > > Many thanks, > Liviu > > > -- > ==================== > | I would like to | > | fix the world, | > | but they're not | > | giving me the | > \ source code! / > --------------- > ¯\_(?)_/¯ >
On Friday 16 May 2014 16:03:04 Sunil Kovvuri wrote: > When the SR-IOV capable device's driver tries to enable sriov > (pci_enable_sriov()) it fails to create/add PCI device for each > virtual function reporting "not enough MMIO resources for SR-IOV". I assume you have checked that there is indeed enough MMIO space available, right? > In sriov_enable() (drivers/pci/iov.c) > > 296 for (i = 0; i < PCI_SRIOV_NUM_BARS; i++) { > 297 bars |= (1 << (i + PCI_IOV_RESOURCES)); > 298 res = dev->resource + PCI_IOV_RESOURCES + i; > 299 if (res->parent) > 300 nres++; > 301 } > 302 if (nres != iov->nres) { > 303 dev_err(&dev->dev, "not enough MMIO resources for > SR-IOV\n"); > 304 return -ENOMEM; > 305 } > > Here its checking if physical function's IOV resource has a parent or not. > Which is pci-pci bridge in this case. Otherwise it doesn't consider > that resource. > > Added below api to your patch. > This will try to claim a resource while creating a PCI device which > inturn sets 'res->parent'. This looks like the wrong approach. The PCI host controller should really have been registered with the root 'iomem_resource' during the probe of the host controller. Arnd
On Mon, May 19, 2014 at 6:31 PM, Arnd Bergmann <arnd@arndb.de> wrote: > On Friday 16 May 2014 16:03:04 Sunil Kovvuri wrote: >> When the SR-IOV capable device's driver tries to enable sriov >> (pci_enable_sriov()) it fails to create/add PCI device for each >> virtual function reporting "not enough MMIO resources for SR-IOV". > > I assume you have checked that there is indeed enough MMIO space > available, right? > Yes, i have checked for enough MMIO space multiple times. >> In sriov_enable() (drivers/pci/iov.c) >> >> 296 for (i = 0; i < PCI_SRIOV_NUM_BARS; i++) { >> 297 bars |= (1 << (i + PCI_IOV_RESOURCES)); >> 298 res = dev->resource + PCI_IOV_RESOURCES + i; >> 299 if (res->parent) >> 300 nres++; >> 301 } >> 302 if (nres != iov->nres) { >> 303 dev_err(&dev->dev, "not enough MMIO resources for >> SR-IOV\n"); >> 304 return -ENOMEM; >> 305 } >> >> Here its checking if physical function's IOV resource has a parent or not. >> Which is pci-pci bridge in this case. Otherwise it doesn't consider >> that resource. >> >> Added below api to your patch. >> This will try to claim a resource while creating a PCI device which >> inturn sets 'res->parent'. > > This looks like the wrong approach. The PCI host controller should > really have been registered with the root 'iomem_resource' during > the probe of the host controller. > > Arnd I didn't get this, if a SR-IOV device is connected to a PCI-PCI bridge and inturn bridge connected to root port. Then the parent bus is not root, but the bridge. The issue is either hierarchy should not be checked for SR-IOV resources or someone should set the hierarchy (i.e parent resources). Regards, Sunil.
On Tuesday 20 May 2014 09:52:33 Sunil Kovvuri wrote: > >> In sriov_enable() (drivers/pci/iov.c) > >> > >> 296 for (i = 0; i < PCI_SRIOV_NUM_BARS; i++) { > >> 297 bars |= (1 << (i + PCI_IOV_RESOURCES)); > >> 298 res = dev->resource + PCI_IOV_RESOURCES + i; > >> 299 if (res->parent) > >> 300 nres++; > >> 301 } > >> 302 if (nres != iov->nres) { > >> 303 dev_err(&dev->dev, "not enough MMIO resources for > >> SR-IOV\n"); > >> 304 return -ENOMEM; > >> 305 } > >> > >> Here its checking if physical function's IOV resource has a parent or not. > >> Which is pci-pci bridge in this case. Otherwise it doesn't consider > >> that resource. > >> > >> Added below api to your patch. > >> This will try to claim a resource while creating a PCI device which > >> inturn sets 'res->parent'. > > > > This looks like the wrong approach. The PCI host controller should > > really have been registered with the root 'iomem_resource' during > > the probe of the host controller. > > > I didn't get this, if a SR-IOV device is connected to a PCI-PCI bridge > and inturn bridge connected to root port. Then the parent bus is not root, > but the bridge. The issue is either hierarchy should not be checked for > SR-IOV resources or someone should set the hierarchy (i.e parent resources). Ah, I misunderstood the problem, I thought the PCI core was complaining about lack of space in the resources, not about a lack of BARs. It seems there is code like yours in a couple of architectures, but they only claim the resources of bridges, not the actual devices as you seem to be doing. Can you check if the x86 version of pcibios_allocate_bus_resources() does the trick for you? Maybe we can turn that into a generic helper. Arnd
On Tue, May 20, 2014 at 2:14 PM, Arnd Bergmann <arnd@arndb.de> wrote: > On Tuesday 20 May 2014 09:52:33 Sunil Kovvuri wrote: >> >> In sriov_enable() (drivers/pci/iov.c) >> >> >> >> 296 for (i = 0; i < PCI_SRIOV_NUM_BARS; i++) { >> >> 297 bars |= (1 << (i + PCI_IOV_RESOURCES)); >> >> 298 res = dev->resource + PCI_IOV_RESOURCES + i; >> >> 299 if (res->parent) >> >> 300 nres++; >> >> 301 } >> >> 302 if (nres != iov->nres) { >> >> 303 dev_err(&dev->dev, "not enough MMIO resources for >> >> SR-IOV\n"); >> >> 304 return -ENOMEM; >> >> 305 } >> >> >> >> Here its checking if physical function's IOV resource has a parent or not. >> >> Which is pci-pci bridge in this case. Otherwise it doesn't consider >> >> that resource. >> >> >> >> Added below api to your patch. >> >> This will try to claim a resource while creating a PCI device which >> >> inturn sets 'res->parent'. >> > >> > This looks like the wrong approach. The PCI host controller should >> > really have been registered with the root 'iomem_resource' during >> > the probe of the host controller. >> > >> I didn't get this, if a SR-IOV device is connected to a PCI-PCI bridge >> and inturn bridge connected to root port. Then the parent bus is not root, >> but the bridge. The issue is either hierarchy should not be checked for >> SR-IOV resources or someone should set the hierarchy (i.e parent resources). > > Ah, I misunderstood the problem, I thought the PCI core was complaining > about lack of space in the resources, not about a lack of BARs. > > It seems there is code like yours in a couple of architectures, but they > only claim the resources of bridges, not the actual devices as you > seem to be doing. Can you check if the x86 version of > pcibios_allocate_bus_resources() does the trick for you? Maybe we can > turn that into a generic helper. > Thanks for the suggestion. Will try that once. FYI, IA64 architecture does claim resources for devices as well. arch/ia64/pci/pci.c 'pcibios_fixup_device_resources()' Thanks, Sunil.
Hi Liviu, Sorry for the trouble. I got why 'res->parent' is not set in my case. Basically my SR-IOV device has fixed resources, so resources will not be allocated/assigned and hence parent resource is not set. I will move the resource claiming to host controller driver as a fixup so that parent resource hierarchy is set. Thanks for the support. Regards, Sunil. On Fri, May 16, 2014 at 11:12 PM, Sunil Kovvuri <sunil.kovvuri@gmail.com> wrote: > Hi Liviu, > > Issue may not be only with SR-IOV resources. > > I am not an expert with Linux PCI core. But i am trying to understand > how even for a non SR-IOV capable device's resource, gets a parent > associated with it. > > When a PCI device driver calls pci_enable_device() which inturn calls > 'arch/arm64/kernel/pci.c' pcibios_enable_device() which uses Linux PCI core > API 'pci_enable_resources'. This API checks 'res->parent' for all > valid resources > before enabling them, otherwise it fails. > if (!r->parent) { > dev_err(&dev->dev, "device not available " > "(can't reserve %pR)\n", r); > return -EINVAL; > } > > Can you please let me know where this hierarchy is being set for > 'pcibios_enable_resources' to work. > > I could only find request_resource() API which sets the parent and > pci_claim_resource() uses this. > > Thanks, > Sunil. > > On Fri, May 16, 2014 at 6:54 PM, Liviu Dudau <Liviu.Dudau@arm.com> wrote: >> Hi Sunil, >> >> On Fri, May 16, 2014 at 11:33:04AM +0100, Sunil Kovvuri wrote: >>> Hi Liviu, >>> >>> I am using your ARM64 PCIe patches to write a PCIe host controller >>> driver for our SOC. I am facing an issue with SR-IOV capable device. >>> >>> Consider an PCI Express endpoint connected to a PCI Express >>> Root Port. The PCI Express endpoint provides PCI-SIG SR-IOV >>> capabilities with a single physical function and a large number >>> of virtual functions. The Root Port contains a pci-pci bridge in >>> between it and the SR-IOV device. >>> >>> When the SR-IOV capable device's driver tries to enable sriov >>> (pci_enable_sriov()) it fails to create/add PCI device for each >>> virtual function reporting "not enough MMIO resources for SR-IOV". >>> >>> In sriov_enable() (drivers/pci/iov.c) >>> >>> 296 for (i = 0; i < PCI_SRIOV_NUM_BARS; i++) { >>> 297 bars |= (1 << (i + PCI_IOV_RESOURCES)); >>> 298 res = dev->resource + PCI_IOV_RESOURCES + i; >>> 299 if (res->parent) >>> 300 nres++; >>> 301 } >>> 302 if (nres != iov->nres) { >>> 303 dev_err(&dev->dev, "not enough MMIO resources for >>> SR-IOV\n"); >>> 304 return -ENOMEM; >>> 305 } >>> >>> Here its checking if physical function's IOV resource has a parent or not. >>> Which is pci-pci bridge in this case. Otherwise it doesn't consider >>> that resource. >>> >>> Added below api to your patch. >>> This will try to claim a resource while creating a PCI device which >>> inturn sets 'res->parent'. >>> >>> Let me know if this is okay. >>> >>> diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c >>> index 9f29c9a..fbfb48f 100644 >>> --- a/arch/arm64/kernel/pci.c >>> +++ b/arch/arm64/kernel/pci.c >>> @@ -125,6 +125,21 @@ resource_size_t pcibios_align_resource(void >>> *data, const struct resource *res, >>> return res->start; >>> } >>> >>> +int pcibios_add_device(struct pci_dev *pdev) >>> +{ >>> + unsigned int i, type_mask = IORESOURCE_IO | IORESOURCE_MEM; >>> + struct resource *res; >>> + >>> + for (i = 0; i < PCI_NUM_RESOURCES; i++) { >>> + res = &pdev->resource[i]; >>> + if (res->parent || !(res->flags & type_mask)) >>> + continue; >>> + pci_claim_resource(pdev, i); >>> + } >>> + >>> >> >> I would like not to have to add your patch in this file as I am trying to >> remove it entirely. I don't have an SR-IOV capable device in my setup so >> I am not able to debug your problem, but could you have a go and try to >> figure out why the SR-IOV resources do not get a parent associated with >> when they get created? >> >> Many thanks, >> Liviu >> >> >> -- >> ==================== >> | I would like to | >> | fix the world, | >> | but they're not | >> | giving me the | >> \ source code! / >> --------------- >> ¯\_(?)_/¯ >>
On Wed, May 21, 2014 at 04:45:29PM +0530, Sunil Kovvuri wrote: > Hi Liviu, > > Sorry for the trouble. > I got why 'res->parent' is not set in my case. > Basically my SR-IOV device has fixed resources, so resources will not > be allocated/assigned and hence parent resource is not set. > I will move the resource claiming to host controller driver as a fixup > so that parent resource hierarchy is set. > > Thanks for the support. Glad you worked out the cause for the problem. I will still at to my list of ToDo things to investigate resource parenting with my patchset. Best regards, Liviu > > Regards, > Sunil. > > > On Fri, May 16, 2014 at 11:12 PM, Sunil Kovvuri <sunil.kovvuri@gmail.com> wrote: > > Hi Liviu, > > > > Issue may not be only with SR-IOV resources. > > > > I am not an expert with Linux PCI core. But i am trying to understand > > how even for a non SR-IOV capable device's resource, gets a parent > > associated with it. > > > > When a PCI device driver calls pci_enable_device() which inturn calls > > 'arch/arm64/kernel/pci.c' pcibios_enable_device() which uses Linux PCI core > > API 'pci_enable_resources'. This API checks 'res->parent' for all > > valid resources > > before enabling them, otherwise it fails. > > if (!r->parent) { > > dev_err(&dev->dev, "device not available " > > "(can't reserve %pR)\n", r); > > return -EINVAL; > > } > > > > Can you please let me know where this hierarchy is being set for > > 'pcibios_enable_resources' to work. > > > > I could only find request_resource() API which sets the parent and > > pci_claim_resource() uses this. > > > > Thanks, > > Sunil. > > > > On Fri, May 16, 2014 at 6:54 PM, Liviu Dudau <Liviu.Dudau@arm.com> wrote: > >> Hi Sunil, > >> > >> On Fri, May 16, 2014 at 11:33:04AM +0100, Sunil Kovvuri wrote: > >>> Hi Liviu, > >>> > >>> I am using your ARM64 PCIe patches to write a PCIe host controller > >>> driver for our SOC. I am facing an issue with SR-IOV capable device. > >>> > >>> Consider an PCI Express endpoint connected to a PCI Express > >>> Root Port. The PCI Express endpoint provides PCI-SIG SR-IOV > >>> capabilities with a single physical function and a large number > >>> of virtual functions. The Root Port contains a pci-pci bridge in > >>> between it and the SR-IOV device. > >>> > >>> When the SR-IOV capable device's driver tries to enable sriov > >>> (pci_enable_sriov()) it fails to create/add PCI device for each > >>> virtual function reporting "not enough MMIO resources for SR-IOV". > >>> > >>> In sriov_enable() (drivers/pci/iov.c) > >>> > >>> 296 for (i = 0; i < PCI_SRIOV_NUM_BARS; i++) { > >>> 297 bars |= (1 << (i + PCI_IOV_RESOURCES)); > >>> 298 res = dev->resource + PCI_IOV_RESOURCES + i; > >>> 299 if (res->parent) > >>> 300 nres++; > >>> 301 } > >>> 302 if (nres != iov->nres) { > >>> 303 dev_err(&dev->dev, "not enough MMIO resources for > >>> SR-IOV\n"); > >>> 304 return -ENOMEM; > >>> 305 } > >>> > >>> Here its checking if physical function's IOV resource has a parent or not. > >>> Which is pci-pci bridge in this case. Otherwise it doesn't consider > >>> that resource. > >>> > >>> Added below api to your patch. > >>> This will try to claim a resource while creating a PCI device which > >>> inturn sets 'res->parent'. > >>> > >>> Let me know if this is okay. > >>> > >>> diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c > >>> index 9f29c9a..fbfb48f 100644 > >>> --- a/arch/arm64/kernel/pci.c > >>> +++ b/arch/arm64/kernel/pci.c > >>> @@ -125,6 +125,21 @@ resource_size_t pcibios_align_resource(void > >>> *data, const struct resource *res, > >>> return res->start; > >>> } > >>> > >>> +int pcibios_add_device(struct pci_dev *pdev) > >>> +{ > >>> + unsigned int i, type_mask = IORESOURCE_IO | IORESOURCE_MEM; > >>> + struct resource *res; > >>> + > >>> + for (i = 0; i < PCI_NUM_RESOURCES; i++) { > >>> + res = &pdev->resource[i]; > >>> + if (res->parent || !(res->flags & type_mask)) > >>> + continue; > >>> + pci_claim_resource(pdev, i); > >>> + } > >>> + > >>> > >> > >> I would like not to have to add your patch in this file as I am trying to > >> remove it entirely. I don't have an SR-IOV capable device in my setup so > >> I am not able to debug your problem, but could you have a go and try to > >> figure out why the SR-IOV resources do not get a parent associated with > >> when they get created? > >> > >> Many thanks, > >> Liviu > >> > >> > >> -- > >> ==================== > >> | I would like to | > >> | fix the world, | > >> | but they're not | > >> | giving me the | > >> \ source code! / > >> --------------- > >> ¯\_(?)_/¯ > >> > -- > To unsubscribe from this list: send the line "unsubscribe linux-pci" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html >
diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c index 9f29c9a..fbfb48f 100644 --- a/arch/arm64/kernel/pci.c +++ b/arch/arm64/kernel/pci.c @@ -125,6 +125,21 @@ resource_size_t pcibios_align_resource(void *data, const struct resource *res, return res->start; } +int pcibios_add_device(struct pci_dev *pdev) +{ + unsigned int i, type_mask = IORESOURCE_IO | IORESOURCE_MEM; + struct resource *res; + + for (i = 0; i < PCI_NUM_RESOURCES; i++) { + res = &pdev->resource[i]; + if (res->parent || !(res->flags & type_mask)) + continue; + pci_claim_resource(pdev, i); + } + On Fri, Mar 14, 2014 at 9:04 PM, Liviu Dudau <Liviu.Dudau@arm.com> wrote: > Hi, >