Patchwork [2/3] PCI, ACPI: Don't glue ACPI dev with pci VFs

login
register
mail settings
Submitter Jiang Liu
Date May 31, 2013, 4:21 a.m.
Message ID <1369974092-11450-2-git-send-email-jiang.liu@huawei.com>
Download mbox | patch
Permalink /patch/247842/
State Rejected
Headers show

Comments

Jiang Liu - May 31, 2013, 4:21 a.m.
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(+)
Bjorn Helgaas - May 31, 2013, 9:40 p.m.
[+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
Bjorn Helgaas - June 4, 2013, 9:48 p.m.
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
Yinghai Lu - June 4, 2013, 9:57 p.m.
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
Bjorn Helgaas - June 4, 2013, 10 p.m.
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
Yinghai Lu - June 4, 2013, 10:08 p.m.
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
Rafael J. Wysocki - June 4, 2013, 10:44 p.m.
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);
>
Rafael J. Wysocki - June 4, 2013, 10:49 p.m.
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);
> > 
>
Yinghai Lu - June 4, 2013, 10:57 p.m.
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
Rafael J. Wysocki - June 4, 2013, 11:51 p.m.
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
Yinghai Lu - June 5, 2013, 3:55 p.m.
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
Bjorn Helgaas - June 5, 2013, 4:21 p.m.
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

Patch

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