[PATCH 1/3] PCI: set correct value for iov device before device

[PATCH 1/3] PCI: set correct value for iov device before device

[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

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

[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

[PATCH 3/3] PCI: Hide remove and rescan sysfs interfaces for SR-IOV virtual functions

[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

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

[Bjorn Helgaas]

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

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

[Bjorn Helgaas]

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

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

[Yinghai Lu]

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

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

[Bjorn Helgaas]

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

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

[Yinghai Lu]

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.

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

[Rafael J. Wysocki]

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.

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

[Rafael J. Wysocki]

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.

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

[Yinghai Lu]

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

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

[Rafael J. Wysocki]

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.

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

[Yinghai Lu]

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

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

[Bjorn Helgaas]

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

Re: [PATCH 1/3] PCI: set correct value for iov device before device

[Bjorn Helgaas]

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
>

Re: [PATCH 3/3] PCI: Hide remove and rescan sysfs interfaces for SR-IOV virtual functions

[Bjorn Helgaas]

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
>