| Message ID | 1369974092-11450-2-git-send-email-jiang.liu@huawei.com |
|---|---|
| State | Rejected |
| Headers | show |
[+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 > -- 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
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 >> -- 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
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 -- 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
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 -- 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
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. -- 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
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); >
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); > > >
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 -- 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
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
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 -- 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
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 -- 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/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);