* [PATCH 1/3] PCI: set correct value for iov device before device @ 2013-05-31 4:21 Jiang Liu 2013-05-31 4:21 ` [PATCH 2/3] PCI, ACPI: Don't glue ACPI dev with pci VFs Jiang Liu ` (2 more replies) 0 siblings, 3 replies; 16+ messages in thread From: Jiang Liu @ 2013-05-31 4:21 UTC (permalink / raw) To: Bjorn Helgaas, Yinghai Lu, Xudong Hao; +Cc: Yijing Wang, linux-pci, Jiang Liu From: Xudong Hao <xudong.hao@intel.com> Commit 4f535093cf8f6da8 "PCI: Put pci_dev in device tree as early as possible" moves device registering from pci_bus_add_devices() to pci_device_add(). That change causes troubles to PCI virtual functions because device_add(&virtfn->dev) is called before setting virtfn->is_virtfn flag, which then causes Xen to report PCI virtual functions as PCI physical functions. So fix it by setting virtfn->is_virtfn before calling pci_device_add(). [Jiang Liu]: move the setting of virtfn->is_virtfn ahead further for better readability and modify changelog. Signed-off-by: Xudong Hao <xudong.hao@intel.com> Signed-off-by: Jiang Liu <jiang.liu@huawei.com> Cc: <stable@vger.kernel.org> # 3.9+ --- Hi Bjorn and Yinghai, How about this? I split out the bugfix patch as separate patch so it could be easily back ported to stable branches. Regards! Gerry --- drivers/pci/iov.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c index 5fffca9..de8ffac 100644 --- a/drivers/pci/iov.c +++ b/drivers/pci/iov.c @@ -82,6 +82,8 @@ static int virtfn_add(struct pci_dev *dev, int id, int reset) pci_read_config_word(dev, iov->pos + PCI_SRIOV_VF_DID, &virtfn->device); pci_setup_device(virtfn); virtfn->dev.parent = dev->dev.parent; + virtfn->physfn = pci_dev_get(dev); + virtfn->is_virtfn = 1; for (i = 0; i < PCI_SRIOV_NUM_BARS; i++) { res = dev->resource + PCI_IOV_RESOURCES + i; @@ -103,9 +105,6 @@ static int virtfn_add(struct pci_dev *dev, int id, int reset) pci_device_add(virtfn, virtfn->bus); mutex_unlock(&iov->dev->sriov->lock); - virtfn->physfn = pci_dev_get(dev); - virtfn->is_virtfn = 1; - rc = pci_bus_add_device(virtfn); sprintf(buf, "virtfn%u", id); rc = sysfs_create_link(&dev->dev.kobj, &virtfn->dev.kobj, buf); -- 1.8.1.2 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH 2/3] PCI, ACPI: Don't glue ACPI dev with pci VFs 2013-05-31 4:21 [PATCH 1/3] PCI: set correct value for iov device before device Jiang Liu @ 2013-05-31 4:21 ` Jiang Liu 2013-05-31 21:40 ` Bjorn Helgaas 2013-06-04 22:44 ` Rafael J. Wysocki 2013-05-31 4:21 ` [PATCH 3/3] PCI: Hide remove and rescan sysfs interfaces for SR-IOV virtual functions Jiang Liu 2013-06-05 18:31 ` [PATCH 1/3] PCI: set correct value for iov device before device Bjorn Helgaas 2 siblings, 2 replies; 16+ messages in thread From: Jiang Liu @ 2013-05-31 4:21 UTC (permalink / raw) To: Bjorn Helgaas, Yinghai Lu, Xudong Hao; +Cc: Yijing Wang, linux-pci, Jiang Liu From: Yinghai Lu <yinghai@kernel.org> When sriov is enabled, VF could just start after PF in pci tree. like c1:00.0 will be PF, and c1:00.1 and after will be VF. acpi do have dev with same ADR. that will make them get glued wrongly. Skip that if it is virtfn. Signed-off-by: Yinghai Lu <yinghai@kernel.org> Signed-off-by: Jiang Liu <jiang.liu@huawei.com> --- drivers/pci/pci-acpi.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c index e4b1fb2..720f3a2 100644 --- a/drivers/pci/pci-acpi.c +++ b/drivers/pci/pci-acpi.c @@ -321,6 +321,10 @@ static int acpi_pci_find_device(struct device *dev, acpi_handle *handle) u64 addr; pci_dev = to_pci_dev(dev); + /* don't mix vf with real pci device */ + if (pci_dev->is_virtfn) + return -ENODEV; + /* Please ref to ACPI spec for the syntax of _ADR */ addr = (PCI_SLOT(pci_dev->devfn) << 16) | PCI_FUNC(pci_dev->devfn); *handle = acpi_get_child(DEVICE_ACPI_HANDLE(dev->parent), addr); -- 1.8.1.2 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH 2/3] PCI, ACPI: Don't glue ACPI dev with pci VFs 2013-05-31 4:21 ` [PATCH 2/3] PCI, ACPI: Don't glue ACPI dev with pci VFs Jiang Liu @ 2013-05-31 21:40 ` Bjorn Helgaas 2013-06-04 21:48 ` Bjorn Helgaas 2013-06-04 22:44 ` Rafael J. Wysocki 1 sibling, 1 reply; 16+ messages in thread From: Bjorn Helgaas @ 2013-05-31 21:40 UTC (permalink / raw) To: Jiang Liu, Rafael J. Wysocki Cc: Yinghai Lu, Xudong Hao, Yijing Wang, linux-pci, Jiang Liu [+cc Rafael] On Thu, May 30, 2013 at 10:21 PM, Jiang Liu <liuj97@gmail.com> wrote: > From: Yinghai Lu <yinghai@kernel.org> > > When sriov is enabled, VF could just start after PF in pci tree. > like c1:00.0 will be PF, and c1:00.1 and after will be VF. > > acpi do have dev with same ADR. that will make them get glued > wrongly. > > Skip that if it is virtfn. > > Signed-off-by: Yinghai Lu <yinghai@kernel.org> > Signed-off-by: Jiang Liu <jiang.liu@huawei.com> > --- > drivers/pci/pci-acpi.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c > index e4b1fb2..720f3a2 100644 > --- a/drivers/pci/pci-acpi.c > +++ b/drivers/pci/pci-acpi.c > @@ -321,6 +321,10 @@ static int acpi_pci_find_device(struct device *dev, acpi_handle *handle) > u64 addr; > > pci_dev = to_pci_dev(dev); > + /* don't mix vf with real pci device */ > + if (pci_dev->is_virtfn) > + return -ENODEV; Rafael, can you review this? I don't understand the implications of this change. And I don't know exactly what problem this would fix, so I don't know if it's stable material or not. Yinghai did propose it as v3.10 material in https://lkml.kernel.org/r/1368498506-25857-1-git-send-email-yinghai@kernel.org, but I don't know why. Bjorn > /* Please ref to ACPI spec for the syntax of _ADR */ > addr = (PCI_SLOT(pci_dev->devfn) << 16) | PCI_FUNC(pci_dev->devfn); > *handle = acpi_get_child(DEVICE_ACPI_HANDLE(dev->parent), addr); > -- > 1.8.1.2 > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/3] PCI, ACPI: Don't glue ACPI dev with pci VFs 2013-05-31 21:40 ` Bjorn Helgaas @ 2013-06-04 21:48 ` Bjorn Helgaas 2013-06-04 21:57 ` Yinghai Lu 0 siblings, 1 reply; 16+ messages in thread From: Bjorn Helgaas @ 2013-06-04 21:48 UTC (permalink / raw) To: Jiang Liu, Rafael J. Wysocki Cc: Yinghai Lu, Xudong Hao, Yijing Wang, linux-pci, Jiang Liu On Fri, May 31, 2013 at 3:40 PM, Bjorn Helgaas <bhelgaas@google.com> wrote: > [+cc Rafael] > > On Thu, May 30, 2013 at 10:21 PM, Jiang Liu <liuj97@gmail.com> wrote: >> From: Yinghai Lu <yinghai@kernel.org> >> >> When sriov is enabled, VF could just start after PF in pci tree. >> like c1:00.0 will be PF, and c1:00.1 and after will be VF. >> >> acpi do have dev with same ADR. that will make them get glued >> wrongly. >> >> Skip that if it is virtfn. >> >> Signed-off-by: Yinghai Lu <yinghai@kernel.org> >> Signed-off-by: Jiang Liu <jiang.liu@huawei.com> >> --- >> drivers/pci/pci-acpi.c | 4 ++++ >> 1 file changed, 4 insertions(+) >> >> diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c >> index e4b1fb2..720f3a2 100644 >> --- a/drivers/pci/pci-acpi.c >> +++ b/drivers/pci/pci-acpi.c >> @@ -321,6 +321,10 @@ static int acpi_pci_find_device(struct device *dev, acpi_handle *handle) >> u64 addr; >> >> pci_dev = to_pci_dev(dev); >> + /* don't mix vf with real pci device */ >> + if (pci_dev->is_virtfn) >> + return -ENODEV; > > Rafael, can you review this? I don't understand the implications of > this change. > > And I don't know exactly what problem this would fix, so I don't know > if it's stable material or not. Yinghai did propose it as v3.10 > material in https://lkml.kernel.org/r/1368498506-25857-1-git-send-email-yinghai@kernel.org, > but I don't know why. Ping? Jiang or Yinghai, what problem does this fix? I'm guessing maybe all three of these should be marked for stable, but I'd like confirmation of that. >> /* Please ref to ACPI spec for the syntax of _ADR */ >> addr = (PCI_SLOT(pci_dev->devfn) << 16) | PCI_FUNC(pci_dev->devfn); >> *handle = acpi_get_child(DEVICE_ACPI_HANDLE(dev->parent), addr); >> -- >> 1.8.1.2 >> ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/3] PCI, ACPI: Don't glue ACPI dev with pci VFs 2013-06-04 21:48 ` Bjorn Helgaas @ 2013-06-04 21:57 ` Yinghai Lu 2013-06-04 22:00 ` Bjorn Helgaas 0 siblings, 1 reply; 16+ messages in thread From: Yinghai Lu @ 2013-06-04 21:57 UTC (permalink / raw) To: Bjorn Helgaas Cc: Jiang Liu, Rafael J. Wysocki, Xudong Hao, Yijing Wang, linux-pci, Jiang Liu On Tue, Jun 4, 2013 at 2:48 PM, Bjorn Helgaas <bhelgaas@google.com> wrote: > On Fri, May 31, 2013 at 3:40 PM, Bjorn Helgaas <bhelgaas@google.com> wrote: >> [+cc Rafael] >> >> On Thu, May 30, 2013 at 10:21 PM, Jiang Liu <liuj97@gmail.com> wrote: >>> From: Yinghai Lu <yinghai@kernel.org> >>> >>> When sriov is enabled, VF could just start after PF in pci tree. >>> like c1:00.0 will be PF, and c1:00.1 and after will be VF. >>> >>> acpi do have dev with same ADR. that will make them get glued >>> wrongly. >>> >>> Skip that if it is virtfn. >>> >>> Signed-off-by: Yinghai Lu <yinghai@kernel.org> >>> Signed-off-by: Jiang Liu <jiang.liu@huawei.com> >>> --- >>> drivers/pci/pci-acpi.c | 4 ++++ >>> 1 file changed, 4 insertions(+) >>> >>> diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c >>> index e4b1fb2..720f3a2 100644 >>> --- a/drivers/pci/pci-acpi.c >>> +++ b/drivers/pci/pci-acpi.c >>> @@ -321,6 +321,10 @@ static int acpi_pci_find_device(struct device *dev, acpi_handle *handle) >>> u64 addr; >>> >>> pci_dev = to_pci_dev(dev); >>> + /* don't mix vf with real pci device */ >>> + if (pci_dev->is_virtfn) >>> + return -ENODEV; >> >> Rafael, can you review this? I don't understand the implications of >> this change. >> >> And I don't know exactly what problem this would fix, so I don't know >> if it's stable material or not. Yinghai did propose it as v3.10 >> material in https://lkml.kernel.org/r/1368498506-25857-1-git-send-email-yinghai@kernel.org, >> but I don't know why. > > Ping? > > Jiang or Yinghai, what problem does this fix? fix the wrong binding between acpi dev and VFs. Found that in my recent sriov test. > > I'm guessing maybe all three of these should be marked for stable, but > I'd like confirmation of that. Yes Yinghai ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/3] PCI, ACPI: Don't glue ACPI dev with pci VFs 2013-06-04 21:57 ` Yinghai Lu @ 2013-06-04 22:00 ` Bjorn Helgaas 2013-06-04 22:08 ` Yinghai Lu 0 siblings, 1 reply; 16+ messages in thread From: Bjorn Helgaas @ 2013-06-04 22:00 UTC (permalink / raw) To: Yinghai Lu Cc: Jiang Liu, Rafael J. Wysocki, Xudong Hao, Yijing Wang, linux-pci, Jiang Liu On Tue, Jun 4, 2013 at 3:57 PM, Yinghai Lu <yinghai@kernel.org> wrote: > On Tue, Jun 4, 2013 at 2:48 PM, Bjorn Helgaas <bhelgaas@google.com> wrote: >> On Fri, May 31, 2013 at 3:40 PM, Bjorn Helgaas <bhelgaas@google.com> wrote: >>> [+cc Rafael] >>> >>> On Thu, May 30, 2013 at 10:21 PM, Jiang Liu <liuj97@gmail.com> wrote: >>>> From: Yinghai Lu <yinghai@kernel.org> >>>> >>>> When sriov is enabled, VF could just start after PF in pci tree. >>>> like c1:00.0 will be PF, and c1:00.1 and after will be VF. >>>> >>>> acpi do have dev with same ADR. that will make them get glued >>>> wrongly. >>>> >>>> Skip that if it is virtfn. >>>> >>>> Signed-off-by: Yinghai Lu <yinghai@kernel.org> >>>> Signed-off-by: Jiang Liu <jiang.liu@huawei.com> >>>> --- >>>> drivers/pci/pci-acpi.c | 4 ++++ >>>> 1 file changed, 4 insertions(+) >>>> >>>> diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c >>>> index e4b1fb2..720f3a2 100644 >>>> --- a/drivers/pci/pci-acpi.c >>>> +++ b/drivers/pci/pci-acpi.c >>>> @@ -321,6 +321,10 @@ static int acpi_pci_find_device(struct device *dev, acpi_handle *handle) >>>> u64 addr; >>>> >>>> pci_dev = to_pci_dev(dev); >>>> + /* don't mix vf with real pci device */ >>>> + if (pci_dev->is_virtfn) >>>> + return -ENODEV; >>> >>> Rafael, can you review this? I don't understand the implications of >>> this change. >>> >>> And I don't know exactly what problem this would fix, so I don't know >>> if it's stable material or not. Yinghai did propose it as v3.10 >>> material in https://lkml.kernel.org/r/1368498506-25857-1-git-send-email-yinghai@kernel.org, >>> but I don't know why. >> >> Ping? >> >> Jiang or Yinghai, what problem does this fix? > > fix the wrong binding between acpi dev and VFs. Well, I read that in the changelog, but that doesn't tell me what bad things happen as a result. Can you elaborate a little bit? Does it mean PM doesn't work, hotplug doesn't work, drivers can't bind to the VFs correctly, the magic smoke comes out of the PF, or what? > Found that in my recent sriov test. > >> >> I'm guessing maybe all three of these should be marked for stable, but >> I'd like confirmation of that. > > Yes > > Yinghai ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/3] PCI, ACPI: Don't glue ACPI dev with pci VFs 2013-06-04 22:00 ` Bjorn Helgaas @ 2013-06-04 22:08 ` Yinghai Lu 0 siblings, 0 replies; 16+ messages in thread From: Yinghai Lu @ 2013-06-04 22:08 UTC (permalink / raw) To: Bjorn Helgaas Cc: Jiang Liu, Rafael J. Wysocki, Xudong Hao, Yijing Wang, linux-pci, Jiang Liu On Tue, Jun 4, 2013 at 3:00 PM, Bjorn Helgaas <bhelgaas@google.com> wrote: > On Tue, Jun 4, 2013 at 3:57 PM, Yinghai Lu <yinghai@kernel.org> wrote: > Well, I read that in the changelog, but that doesn't tell me what bad > things happen as a result. Can you elaborate a little bit? Does it > mean PM doesn't work, hotplug doesn't work, drivers can't bind to the > VFs correctly, the magic smoke comes out of the PF, or what? no, I did not notice anything unusual except the binding message. as my platform have those PM disabled. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/3] PCI, ACPI: Don't glue ACPI dev with pci VFs 2013-05-31 4:21 ` [PATCH 2/3] PCI, ACPI: Don't glue ACPI dev with pci VFs Jiang Liu 2013-05-31 21:40 ` Bjorn Helgaas @ 2013-06-04 22:44 ` Rafael J. Wysocki 2013-06-04 22:49 ` Rafael J. Wysocki 1 sibling, 1 reply; 16+ messages in thread From: Rafael J. Wysocki @ 2013-06-04 22:44 UTC (permalink / raw) To: Jiang Liu Cc: Bjorn Helgaas, Yinghai Lu, Xudong Hao, Yijing Wang, linux-pci, Jiang Liu On Friday, May 31, 2013 12:21:30 PM Jiang Liu wrote: > From: Yinghai Lu <yinghai@kernel.org> > > When sriov is enabled, VF could just start after PF in pci tree. > like c1:00.0 will be PF, and c1:00.1 and after will be VF. > > acpi do have dev with same ADR. that will make them get glued > wrongly. How exactly are they glued in that case? > Skip that if it is virtfn. That should be a bit more specific as far as I can say. I don't see why a VF would not have a valid ACPI device object corresponding to it. Is there any particular reason? Rafael > Signed-off-by: Yinghai Lu <yinghai@kernel.org> > Signed-off-by: Jiang Liu <jiang.liu@huawei.com> > --- > drivers/pci/pci-acpi.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c > index e4b1fb2..720f3a2 100644 > --- a/drivers/pci/pci-acpi.c > +++ b/drivers/pci/pci-acpi.c > @@ -321,6 +321,10 @@ static int acpi_pci_find_device(struct device *dev, acpi_handle *handle) > u64 addr; > > pci_dev = to_pci_dev(dev); > + /* don't mix vf with real pci device */ > + if (pci_dev->is_virtfn) > + return -ENODEV; > + > /* Please ref to ACPI spec for the syntax of _ADR */ > addr = (PCI_SLOT(pci_dev->devfn) << 16) | PCI_FUNC(pci_dev->devfn); > *handle = acpi_get_child(DEVICE_ACPI_HANDLE(dev->parent), addr); > -- I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/3] PCI, ACPI: Don't glue ACPI dev with pci VFs 2013-06-04 22:44 ` Rafael J. Wysocki @ 2013-06-04 22:49 ` Rafael J. Wysocki 2013-06-04 22:57 ` Yinghai Lu 0 siblings, 1 reply; 16+ messages in thread From: Rafael J. Wysocki @ 2013-06-04 22:49 UTC (permalink / raw) To: Jiang Liu Cc: Bjorn Helgaas, Yinghai Lu, Xudong Hao, Yijing Wang, linux-pci, Jiang Liu On Wednesday, June 05, 2013 12:44:27 AM Rafael J. Wysocki wrote: > On Friday, May 31, 2013 12:21:30 PM Jiang Liu wrote: > > From: Yinghai Lu <yinghai@kernel.org> > > > > When sriov is enabled, VF could just start after PF in pci tree. > > like c1:00.0 will be PF, and c1:00.1 and after will be VF. > > > > acpi do have dev with same ADR. that will make them get glued > > wrongly. > > How exactly are they glued in that case? > > > Skip that if it is virtfn. > > That should be a bit more specific as far as I can say. I don't see why a VF > would not have a valid ACPI device object corresponding to it. Is there any > particular reason? To be precise, I don't quite see why it is impossible or invalid for a VF to have a corresponding ACPI device object. It may not be the case on this particular system, but why not in general? Rafael > > Signed-off-by: Yinghai Lu <yinghai@kernel.org> > > Signed-off-by: Jiang Liu <jiang.liu@huawei.com> > > --- > > drivers/pci/pci-acpi.c | 4 ++++ > > 1 file changed, 4 insertions(+) > > > > diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c > > index e4b1fb2..720f3a2 100644 > > --- a/drivers/pci/pci-acpi.c > > +++ b/drivers/pci/pci-acpi.c > > @@ -321,6 +321,10 @@ static int acpi_pci_find_device(struct device *dev, acpi_handle *handle) > > u64 addr; > > > > pci_dev = to_pci_dev(dev); > > + /* don't mix vf with real pci device */ > > + if (pci_dev->is_virtfn) > > + return -ENODEV; > > + > > /* Please ref to ACPI spec for the syntax of _ADR */ > > addr = (PCI_SLOT(pci_dev->devfn) << 16) | PCI_FUNC(pci_dev->devfn); > > *handle = acpi_get_child(DEVICE_ACPI_HANDLE(dev->parent), addr); > > > -- I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/3] PCI, ACPI: Don't glue ACPI dev with pci VFs 2013-06-04 22:49 ` Rafael J. Wysocki @ 2013-06-04 22:57 ` Yinghai Lu 2013-06-04 23:51 ` Rafael J. Wysocki 0 siblings, 1 reply; 16+ messages in thread From: Yinghai Lu @ 2013-06-04 22:57 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Jiang Liu, Bjorn Helgaas, Xudong Hao, Yijing Wang, linux-pci, Jiang Liu On Tue, Jun 4, 2013 at 3:49 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: > On Wednesday, June 05, 2013 12:44:27 AM Rafael J. Wysocki wrote: >> On Friday, May 31, 2013 12:21:30 PM Jiang Liu wrote: >> > From: Yinghai Lu <yinghai@kernel.org> >> > >> > When sriov is enabled, VF could just start after PF in pci tree. >> > like c1:00.0 will be PF, and c1:00.1 and after will be VF. >> > >> > acpi do have dev with same ADR. that will make them get glued >> > wrongly. >> >> How exactly are they glued in that case? >> >> > Skip that if it is virtfn. >> >> That should be a bit more specific as far as I can say. I don't see why a VF >> would not have a valid ACPI device object corresponding to it. Is there any >> particular reason? > > To be precise, I don't quite see why it is impossible or invalid for a VF to > have a corresponding ACPI device object. It may not be the case on this > particular system, but why not in general? at least for ioapic routing GSI, we should not mix VF to use other PF's setting. Yinghai ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/3] PCI, ACPI: Don't glue ACPI dev with pci VFs 2013-06-04 22:57 ` Yinghai Lu @ 2013-06-04 23:51 ` Rafael J. Wysocki 2013-06-05 15:55 ` Yinghai Lu 0 siblings, 1 reply; 16+ messages in thread From: Rafael J. Wysocki @ 2013-06-04 23:51 UTC (permalink / raw) To: Yinghai Lu Cc: Jiang Liu, Bjorn Helgaas, Xudong Hao, Yijing Wang, linux-pci, Jiang Liu On Tuesday, June 04, 2013 03:57:28 PM Yinghai Lu wrote: > On Tue, Jun 4, 2013 at 3:49 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: > > On Wednesday, June 05, 2013 12:44:27 AM Rafael J. Wysocki wrote: > >> On Friday, May 31, 2013 12:21:30 PM Jiang Liu wrote: > >> > From: Yinghai Lu <yinghai@kernel.org> > >> > > >> > When sriov is enabled, VF could just start after PF in pci tree. > >> > like c1:00.0 will be PF, and c1:00.1 and after will be VF. > >> > > >> > acpi do have dev with same ADR. that will make them get glued > >> > wrongly. > >> > >> How exactly are they glued in that case? > >> > >> > Skip that if it is virtfn. > >> > >> That should be a bit more specific as far as I can say. I don't see why a VF > >> would not have a valid ACPI device object corresponding to it. Is there any > >> particular reason? > > > > To be precise, I don't quite see why it is impossible or invalid for a VF to > > have a corresponding ACPI device object. It may not be the case on this > > particular system, but why not in general? > > at least for ioapic routing GSI, we should not mix VF to use other PF's > setting. I can agree with that, but your patch is far more general than this. It won't allow any VF on any system to be "glued" to any ACPI device object and I'm thinking that that may just go too far. May that be addressed in a more specific way? Also, I don't quite understand what the problem *exactly* is, because you haven't given any details so far. Can you possibly attach the specific piece of AML causing the problem to happen on your system? Rafael -- I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/3] PCI, ACPI: Don't glue ACPI dev with pci VFs 2013-06-04 23:51 ` Rafael J. Wysocki @ 2013-06-05 15:55 ` Yinghai Lu 2013-06-05 16:21 ` Bjorn Helgaas 0 siblings, 1 reply; 16+ messages in thread From: Yinghai Lu @ 2013-06-05 15:55 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Jiang Liu, Bjorn Helgaas, Xudong Hao, Yijing Wang, linux-pci, Jiang Liu On Tue, Jun 4, 2013 at 4:51 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: > On Tuesday, June 04, 2013 03:57:28 PM Yinghai Lu wrote: >> > >> > To be precise, I don't quite see why it is impossible or invalid for a VF to >> > have a corresponding ACPI device object. It may not be the case on this >> > particular system, but why not in general? >> >> at least for ioapic routing GSI, we should not mix VF to use other PF's >> setting. > > I can agree with that, but your patch is far more general than this. It won't > allow any VF on any system to be "glued" to any ACPI device object and I'm > thinking that that may just go too far. I think that we should look reversely: Is there any reason or use case that we need to bind PCI VF to acpi device? PCI vf is only showing up after PF driver call pci_enable_siov. Yinghai ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/3] PCI, ACPI: Don't glue ACPI dev with pci VFs 2013-06-05 15:55 ` Yinghai Lu @ 2013-06-05 16:21 ` Bjorn Helgaas 0 siblings, 0 replies; 16+ messages in thread From: Bjorn Helgaas @ 2013-06-05 16:21 UTC (permalink / raw) To: Yinghai Lu Cc: Rafael J. Wysocki, Jiang Liu, Xudong Hao, Yijing Wang, linux-pci, Jiang Liu On Wed, Jun 5, 2013 at 9:55 AM, Yinghai Lu <yinghai@kernel.org> wrote: > On Tue, Jun 4, 2013 at 4:51 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: >> On Tuesday, June 04, 2013 03:57:28 PM Yinghai Lu wrote: >>> > >>> > To be precise, I don't quite see why it is impossible or invalid for a VF to >>> > have a corresponding ACPI device object. It may not be the case on this >>> > particular system, but why not in general? >>> >>> at least for ioapic routing GSI, we should not mix VF to use other PF's >>> setting. >> >> I can agree with that, but your patch is far more general than this. It won't >> allow any VF on any system to be "glued" to any ACPI device object and I'm >> thinking that that may just go too far. > > I think that we should look reversely: > Is there any reason or use case that we need to bind PCI VF to acpi device? > PCI vf is only showing up after PF driver call pci_enable_siov. I disagree with this sentiment. We should handle VFs the same as PFs except when that's impossible. You're proposing a special case of treating VFs differently, so I think the burden is on you to explain why we need to do that. It would be helpful if you answered the questions people ask you, for example, if you could supply the AML Rafael asked about. If it's secret, just say so instead of leaving us with the impression that you're ignoring the question. Bjorn ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 3/3] PCI: Hide remove and rescan sysfs interfaces for SR-IOV virtual functions 2013-05-31 4:21 [PATCH 1/3] PCI: set correct value for iov device before device Jiang Liu 2013-05-31 4:21 ` [PATCH 2/3] PCI, ACPI: Don't glue ACPI dev with pci VFs Jiang Liu @ 2013-05-31 4:21 ` Jiang Liu 2013-06-05 18:32 ` Bjorn Helgaas 2013-06-05 18:31 ` [PATCH 1/3] PCI: set correct value for iov device before device Bjorn Helgaas 2 siblings, 1 reply; 16+ messages in thread From: Jiang Liu @ 2013-05-31 4:21 UTC (permalink / raw) To: Bjorn Helgaas, Yinghai Lu, Xudong Hao Cc: Jiang Liu, Yijing Wang, linux-pci, Jiang Liu From: Jiang Liu <liuj97@gmail.com> PCI devices for SR-IOV virtual functions should only be created/ destroyed by pci_enable_sriov()/pci_disable_sriov() because special data structures are associated with SR-IOV virtual functions. So hide hotplug related sysfs interfaces "remove" and "rescan" for SR-IOV virtual functions, otherwise it may causes memory leakage and other issues. Signed-off-by: Jiang Liu <jiang.liu@huawei.com> Signed-off-by: Yijing Wang <wangyijing@huawei.com> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> Cc: Donald Dutile <ddutile@redhat.com> Cc: Yinghai Lu <yinghai@kernel.org> Cc: Ram Pai <linuxram@us.ibm.com> --- drivers/pci/pci-sysfs.c | 30 ++++++++++++++++++++++++++++-- 1 file changed, 28 insertions(+), 2 deletions(-) diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c index 5b4a9d9..403da60 100644 --- a/drivers/pci/pci-sysfs.c +++ b/drivers/pci/pci-sysfs.c @@ -325,6 +325,8 @@ dev_rescan_store(struct device *dev, struct device_attribute *attr, } return count; } +struct device_attribute dev_rescan_attr = __ATTR(rescan, (S_IWUSR|S_IWGRP), + NULL, dev_rescan_store); static void remove_callback(struct device *dev) { @@ -354,6 +356,8 @@ remove_store(struct device *dev, struct device_attribute *dummy, count = ret; return count; } +struct device_attribute dev_remove_attr = __ATTR(remove, (S_IWUSR|S_IWGRP), + NULL, remove_store); static ssize_t dev_bus_rescan_store(struct device *dev, struct device_attribute *attr, @@ -504,8 +508,6 @@ struct device_attribute pci_dev_attrs[] = { __ATTR(broken_parity_status,(S_IRUGO|S_IWUSR), broken_parity_status_show,broken_parity_status_store), __ATTR(msi_bus, 0644, msi_bus_show, msi_bus_store), - __ATTR(remove, (S_IWUSR|S_IWGRP), NULL, remove_store), - __ATTR(rescan, (S_IWUSR|S_IWGRP), NULL, dev_rescan_store), #if defined(CONFIG_PM_RUNTIME) && defined(CONFIG_ACPI) __ATTR(d3cold_allowed, 0644, d3cold_allowed_show, d3cold_allowed_store), #endif @@ -1463,6 +1465,29 @@ static umode_t pci_dev_attrs_are_visible(struct kobject *kobj, return a->mode; } +static struct attribute *pci_dev_hp_attrs[] = { + &dev_remove_attr.attr, + &dev_rescan_attr.attr, + NULL, +}; + +static umode_t pci_dev_hp_attrs_are_visible(struct kobject *kobj, + struct attribute *a, int n) +{ + struct device *dev = container_of(kobj, struct device, kobj); + struct pci_dev *pdev = to_pci_dev(dev); + + if (pdev->is_virtfn) + return 0; + + return a->mode; +} + +static struct attribute_group pci_dev_hp_attr_group = { + .attrs = pci_dev_hp_attrs, + .is_visible = pci_dev_hp_attrs_are_visible, +}; + #ifdef CONFIG_PCI_IOV static struct attribute *sriov_dev_attrs[] = { &sriov_totalvfs_attr.attr, @@ -1494,6 +1519,7 @@ static struct attribute_group pci_dev_attr_group = { static const struct attribute_group *pci_dev_attr_groups[] = { &pci_dev_attr_group, + &pci_dev_hp_attr_group, #ifdef CONFIG_PCI_IOV &sriov_dev_attr_group, #endif -- 1.8.1.2 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH 3/3] PCI: Hide remove and rescan sysfs interfaces for SR-IOV virtual functions 2013-05-31 4:21 ` [PATCH 3/3] PCI: Hide remove and rescan sysfs interfaces for SR-IOV virtual functions Jiang Liu @ 2013-06-05 18:32 ` Bjorn Helgaas 0 siblings, 0 replies; 16+ messages in thread From: Bjorn Helgaas @ 2013-06-05 18:32 UTC (permalink / raw) To: Jiang Liu; +Cc: Yinghai Lu, Xudong Hao, Yijing Wang, linux-pci, Jiang Liu On Thu, May 30, 2013 at 10:21 PM, Jiang Liu <liuj97@gmail.com> wrote: > From: Jiang Liu <liuj97@gmail.com> > > PCI devices for SR-IOV virtual functions should only be created/ > destroyed by pci_enable_sriov()/pci_disable_sriov() because special > data structures are associated with SR-IOV virtual functions. > So hide hotplug related sysfs interfaces "remove" and "rescan" for > SR-IOV virtual functions, otherwise it may causes memory leakage > and other issues. > > Signed-off-by: Jiang Liu <jiang.liu@huawei.com> > Signed-off-by: Yijing Wang <wangyijing@huawei.com> > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> > Cc: Donald Dutile <ddutile@redhat.com> > Cc: Yinghai Lu <yinghai@kernel.org> > Cc: Ram Pai <linuxram@us.ibm.com> Applied to my -next branch for v3.11, thanks! > --- > drivers/pci/pci-sysfs.c | 30 ++++++++++++++++++++++++++++-- > 1 file changed, 28 insertions(+), 2 deletions(-) > > diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c > index 5b4a9d9..403da60 100644 > --- a/drivers/pci/pci-sysfs.c > +++ b/drivers/pci/pci-sysfs.c > @@ -325,6 +325,8 @@ dev_rescan_store(struct device *dev, struct device_attribute *attr, > } > return count; > } > +struct device_attribute dev_rescan_attr = __ATTR(rescan, (S_IWUSR|S_IWGRP), > + NULL, dev_rescan_store); > > static void remove_callback(struct device *dev) > { > @@ -354,6 +356,8 @@ remove_store(struct device *dev, struct device_attribute *dummy, > count = ret; > return count; > } > +struct device_attribute dev_remove_attr = __ATTR(remove, (S_IWUSR|S_IWGRP), > + NULL, remove_store); > > static ssize_t > dev_bus_rescan_store(struct device *dev, struct device_attribute *attr, > @@ -504,8 +508,6 @@ struct device_attribute pci_dev_attrs[] = { > __ATTR(broken_parity_status,(S_IRUGO|S_IWUSR), > broken_parity_status_show,broken_parity_status_store), > __ATTR(msi_bus, 0644, msi_bus_show, msi_bus_store), > - __ATTR(remove, (S_IWUSR|S_IWGRP), NULL, remove_store), > - __ATTR(rescan, (S_IWUSR|S_IWGRP), NULL, dev_rescan_store), > #if defined(CONFIG_PM_RUNTIME) && defined(CONFIG_ACPI) > __ATTR(d3cold_allowed, 0644, d3cold_allowed_show, d3cold_allowed_store), > #endif > @@ -1463,6 +1465,29 @@ static umode_t pci_dev_attrs_are_visible(struct kobject *kobj, > return a->mode; > } > > +static struct attribute *pci_dev_hp_attrs[] = { > + &dev_remove_attr.attr, > + &dev_rescan_attr.attr, > + NULL, > +}; > + > +static umode_t pci_dev_hp_attrs_are_visible(struct kobject *kobj, > + struct attribute *a, int n) > +{ > + struct device *dev = container_of(kobj, struct device, kobj); > + struct pci_dev *pdev = to_pci_dev(dev); > + > + if (pdev->is_virtfn) > + return 0; > + > + return a->mode; > +} > + > +static struct attribute_group pci_dev_hp_attr_group = { > + .attrs = pci_dev_hp_attrs, > + .is_visible = pci_dev_hp_attrs_are_visible, > +}; > + > #ifdef CONFIG_PCI_IOV > static struct attribute *sriov_dev_attrs[] = { > &sriov_totalvfs_attr.attr, > @@ -1494,6 +1519,7 @@ static struct attribute_group pci_dev_attr_group = { > > static const struct attribute_group *pci_dev_attr_groups[] = { > &pci_dev_attr_group, > + &pci_dev_hp_attr_group, > #ifdef CONFIG_PCI_IOV > &sriov_dev_attr_group, > #endif > -- > 1.8.1.2 > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/3] PCI: set correct value for iov device before device 2013-05-31 4:21 [PATCH 1/3] PCI: set correct value for iov device before device Jiang Liu 2013-05-31 4:21 ` [PATCH 2/3] PCI, ACPI: Don't glue ACPI dev with pci VFs Jiang Liu 2013-05-31 4:21 ` [PATCH 3/3] PCI: Hide remove and rescan sysfs interfaces for SR-IOV virtual functions Jiang Liu @ 2013-06-05 18:31 ` Bjorn Helgaas 2 siblings, 0 replies; 16+ messages in thread From: Bjorn Helgaas @ 2013-06-05 18:31 UTC (permalink / raw) To: Jiang Liu; +Cc: Yinghai Lu, Xudong Hao, Yijing Wang, linux-pci, Jiang Liu On Thu, May 30, 2013 at 10:21 PM, Jiang Liu <liuj97@gmail.com> wrote: > From: Xudong Hao <xudong.hao@intel.com> > > Commit 4f535093cf8f6da8 "PCI: Put pci_dev in device tree as early as > possible" moves device registering from pci_bus_add_devices() to > pci_device_add(). That change causes troubles to PCI virtual functions > because device_add(&virtfn->dev) is called before setting > virtfn->is_virtfn flag, which then causes Xen to report PCI virtual > functions as PCI physical functions. > > So fix it by setting virtfn->is_virtfn before calling pci_device_add(). > > [Jiang Liu]: > move the setting of virtfn->is_virtfn ahead further for better readability > and modify changelog. > > Signed-off-by: Xudong Hao <xudong.hao@intel.com> > Signed-off-by: Jiang Liu <jiang.liu@huawei.com> > Cc: <stable@vger.kernel.org> # 3.9+ > --- > Hi Bjorn and Yinghai, > How about this? I split out the bugfix patch as separate patch > so it could be easily back ported to stable branches. Applied to my -next branch for v3.11, thanks! > --- > drivers/pci/iov.c | 5 ++--- > 1 file changed, 2 insertions(+), 3 deletions(-) > > diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c > index 5fffca9..de8ffac 100644 > --- a/drivers/pci/iov.c > +++ b/drivers/pci/iov.c > @@ -82,6 +82,8 @@ static int virtfn_add(struct pci_dev *dev, int id, int reset) > pci_read_config_word(dev, iov->pos + PCI_SRIOV_VF_DID, &virtfn->device); > pci_setup_device(virtfn); > virtfn->dev.parent = dev->dev.parent; > + virtfn->physfn = pci_dev_get(dev); > + virtfn->is_virtfn = 1; > > for (i = 0; i < PCI_SRIOV_NUM_BARS; i++) { > res = dev->resource + PCI_IOV_RESOURCES + i; > @@ -103,9 +105,6 @@ static int virtfn_add(struct pci_dev *dev, int id, int reset) > pci_device_add(virtfn, virtfn->bus); > mutex_unlock(&iov->dev->sriov->lock); > > - virtfn->physfn = pci_dev_get(dev); > - virtfn->is_virtfn = 1; > - > rc = pci_bus_add_device(virtfn); > sprintf(buf, "virtfn%u", id); > rc = sysfs_create_link(&dev->dev.kobj, &virtfn->dev.kobj, buf); > -- > 1.8.1.2 > ^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2013-06-05 18:32 UTC | newest] Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2013-05-31 4:21 [PATCH 1/3] PCI: set correct value for iov device before device Jiang Liu 2013-05-31 4:21 ` [PATCH 2/3] PCI, ACPI: Don't glue ACPI dev with pci VFs Jiang Liu 2013-05-31 21:40 ` Bjorn Helgaas 2013-06-04 21:48 ` Bjorn Helgaas 2013-06-04 21:57 ` Yinghai Lu 2013-06-04 22:00 ` Bjorn Helgaas 2013-06-04 22:08 ` Yinghai Lu 2013-06-04 22:44 ` Rafael J. Wysocki 2013-06-04 22:49 ` Rafael J. Wysocki 2013-06-04 22:57 ` Yinghai Lu 2013-06-04 23:51 ` Rafael J. Wysocki 2013-06-05 15:55 ` Yinghai Lu 2013-06-05 16:21 ` Bjorn Helgaas 2013-05-31 4:21 ` [PATCH 3/3] PCI: Hide remove and rescan sysfs interfaces for SR-IOV virtual functions Jiang Liu 2013-06-05 18:32 ` Bjorn Helgaas 2013-06-05 18:31 ` [PATCH 1/3] PCI: set correct value for iov device before device Bjorn Helgaas
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.