Patchwork [0/7] disable bridge ari forwarding after connected ari device hot removed

login
register
mail settings
Submitter Jiang Liu
Date Oct. 9, 2012, 3:42 a.m.
Message ID <50739D40.8040005@huawei.com>
Download mbox | patch
Permalink /patch/190185/
State Superseded
Headers show

Comments

Jiang Liu - Oct. 9, 2012, 3:42 a.m.
How about this patch, which disables ARI when adding new PCI devices?

On 2012-10-9 11:03, Yijing Wang wrote:
> pci_enable_ari will be called if an ARI pci device found, then set its bridge ARI Forwarding Enable
> bit in Device Control 2 Register. But the bridge ARI Forwarding Enable bit will never be cleared
> when an ARI device hot removed.
> 
> my steps:
> 1. Hot add an ARI pci device;
> 2. Hot remove the ARI pci device;
> 3. Hot add an non ARI pci device;
> 
> In this case, after setp 3, we could only find fun 0 of non ARI pci device because of its bridge ARI Forwarding Enable
> bit set.
> 
> As PCIe Spec 2.0(6.13/441) recommends:
> "Following a hot-plug event below a Downstream Port, it is strongly recommended that software
> Clear the ARI Forwarding Enable bit in the Downstream Port until software determines that a
> newly added component is in fact an ARI Device"
> 
> This series of patches fix this problem.
> 
> Yijing Wang (7):
>   PCI: rework pci_enable_ari for support disable ari forwarding
>   PCI, acpiphp: disable ARI forwarding for acpiphp
>   PCI, pciehp: disable ARI forwarding for pciehp
>   PCI, cpqphp: disable ARI forwarding for cpqphp
>   PCI, shpchp: disable ARI forwarding for shpchp
>   PCI, sgi: disable ARI forwarding for sgiphp
>   PCI, ibmphp: disable ARI forwarding for ibmphp
> 
>  drivers/pci/hotplug/acpiphp_glue.c     |    1 +
>  drivers/pci/hotplug/cpci_hotplug_pci.c |    1 +
>  drivers/pci/hotplug/cpqphp_pci.c       |    1 +
>  drivers/pci/hotplug/ibmphp_core.c      |    1 +
>  drivers/pci/hotplug/pciehp_pci.c       |    1 +
>  drivers/pci/hotplug/sgi_hotplug.c      |    1 +
>  drivers/pci/hotplug/shpchp_pci.c       |    1 +
>  drivers/pci/pci.c                      |   16 +++++++++++-----
>  drivers/pci/pci.h                      |    2 +-
>  drivers/pci/probe.c                    |    2 +-
>  10 files changed, 20 insertions(+), 7 deletions(-)
> 
> 
> 
> .
> 


--
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
Yijing Wang - Oct. 9, 2012, 7:42 a.m.
On 2012/10/9 11:42, Jiang Liu wrote:
> How about this patch, which disables ARI when adding new PCI devices?
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 5485883..c841aa6 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -2016,13 +2016,14 @@ void pci_free_cap_save_buffers(struct pci_dev *dev)
>  void pci_enable_ari(struct pci_dev *dev)
>  {

I think it's ok. Delay to disable ARI until next hot add is safe.
Maybe rename as pci_configure_ari is better.

>         u32 cap;
> +       bool enable = true;
>         struct pci_dev *bridge;
> 
>         if (pcie_ari_disabled || !pci_is_pcie(dev) || dev->devfn)
>                 return;
> 
>         if (!pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ARI))
> -               return;
> +               enable = false;
> 
>         bridge = dev->bus->self;
>         if (!bridge)
> @@ -2032,8 +2033,15 @@ void pci_enable_ari(struct pci_dev *dev)
>         if (!(cap & PCI_EXP_DEVCAP2_ARI))
>                 return;
> 
> -       pcie_capability_set_word(bridge, PCI_EXP_DEVCTL2, PCI_EXP_DEVCTL2_ARI);
> -       bridge->ari_enabled = 1;
> +       if (enable) {
> +               pcie_capability_set_word(bridge, PCI_EXP_DEVCTL2,
> +                                        PCI_EXP_DEVCTL2_ARI);
> +               bridge->ari_enabled = 1;
> +       } else {
> +               pcie_capability_clear_word(bridge, PCI_EXP_DEVCTL2,
> +                                        PCI_EXP_DEVCTL2_ARI);
> +               bridge->ari_enabled = 0;
> +       }
>  }
> 
>  /**
>
> On 2012-10-9 11:03, Yijing Wang wrote:
>> pci_enable_ari will be called if an ARI pci device found, then set its bridge ARI Forwarding Enable
>> bit in Device Control 2 Register. But the bridge ARI Forwarding Enable bit will never be cleared
>> when an ARI device hot removed.
>>
>> my steps:
>> 1. Hot add an ARI pci device;
>> 2. Hot remove the ARI pci device;
>> 3. Hot add an non ARI pci device;
>>
>> In this case, after setp 3, we could only find fun 0 of non ARI pci device because of its bridge ARI Forwarding Enable
>> bit set.
>>
>> As PCIe Spec 2.0(6.13/441) recommends:
>> "Following a hot-plug event below a Downstream Port, it is strongly recommended that software
>> Clear the ARI Forwarding Enable bit in the Downstream Port until software determines that a
>> newly added component is in fact an ARI Device"
>>
>> This series of patches fix this problem.
>>
>> Yijing Wang (7):
>>   PCI: rework pci_enable_ari for support disable ari forwarding
>>   PCI, acpiphp: disable ARI forwarding for acpiphp
>>   PCI, pciehp: disable ARI forwarding for pciehp
>>   PCI, cpqphp: disable ARI forwarding for cpqphp
>>   PCI, shpchp: disable ARI forwarding for shpchp
>>   PCI, sgi: disable ARI forwarding for sgiphp
>>   PCI, ibmphp: disable ARI forwarding for ibmphp
>>
>>  drivers/pci/hotplug/acpiphp_glue.c     |    1 +
>>  drivers/pci/hotplug/cpci_hotplug_pci.c |    1 +
>>  drivers/pci/hotplug/cpqphp_pci.c       |    1 +
>>  drivers/pci/hotplug/ibmphp_core.c      |    1 +
>>  drivers/pci/hotplug/pciehp_pci.c       |    1 +
>>  drivers/pci/hotplug/sgi_hotplug.c      |    1 +
>>  drivers/pci/hotplug/shpchp_pci.c       |    1 +
>>  drivers/pci/pci.c                      |   16 +++++++++++-----
>>  drivers/pci/pci.h                      |    2 +-
>>  drivers/pci/probe.c                    |    2 +-
>>  10 files changed, 20 insertions(+), 7 deletions(-)
>>
>>
>>
>> .
>>
> 
> 
> 
> .
>
Jiang Liu - Oct. 9, 2012, 7:51 a.m.
When scanning PCI devices for hot-add, it guarantee function 0 is scanned
at first. And when destroying PCI devices for hot-removal, we also need to
destroy function 0 at last, but currently there's no such guarantees. 

On 2012-10-9 15:42, Yijing Wang wrote:
> On 2012/10/9 11:42, Jiang Liu wrote:
>> How about this patch, which disables ARI when adding new PCI devices?
>> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
>> index 5485883..c841aa6 100644
>> --- a/drivers/pci/pci.c
>> +++ b/drivers/pci/pci.c
>> @@ -2016,13 +2016,14 @@ void pci_free_cap_save_buffers(struct pci_dev *dev)
>>  void pci_enable_ari(struct pci_dev *dev)
>>  {
> 
> I think it's ok. Delay to disable ARI until next hot add is safe.
> Maybe rename as pci_configure_ari is better.
> 
>>         u32 cap;
>> +       bool enable = true;
>>         struct pci_dev *bridge;
>>
>>         if (pcie_ari_disabled || !pci_is_pcie(dev) || dev->devfn)
>>                 return;
>>
>>         if (!pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ARI))
>> -               return;
>> +               enable = false;
>>
>>         bridge = dev->bus->self;
>>         if (!bridge)
>> @@ -2032,8 +2033,15 @@ void pci_enable_ari(struct pci_dev *dev)
>>         if (!(cap & PCI_EXP_DEVCAP2_ARI))
>>                 return;
>>
>> -       pcie_capability_set_word(bridge, PCI_EXP_DEVCTL2, PCI_EXP_DEVCTL2_ARI);
>> -       bridge->ari_enabled = 1;
>> +       if (enable) {
>> +               pcie_capability_set_word(bridge, PCI_EXP_DEVCTL2,
>> +                                        PCI_EXP_DEVCTL2_ARI);
>> +               bridge->ari_enabled = 1;
>> +       } else {
>> +               pcie_capability_clear_word(bridge, PCI_EXP_DEVCTL2,
>> +                                        PCI_EXP_DEVCTL2_ARI);
>> +               bridge->ari_enabled = 0;
>> +       }
>>  }
>>
>>  /**
>>
>> On 2012-10-9 11:03, Yijing Wang wrote:
>>> pci_enable_ari will be called if an ARI pci device found, then set its bridge ARI Forwarding Enable
>>> bit in Device Control 2 Register. But the bridge ARI Forwarding Enable bit will never be cleared
>>> when an ARI device hot removed.
>>>
>>> my steps:
>>> 1. Hot add an ARI pci device;
>>> 2. Hot remove the ARI pci device;
>>> 3. Hot add an non ARI pci device;
>>>
>>> In this case, after setp 3, we could only find fun 0 of non ARI pci device because of its bridge ARI Forwarding Enable
>>> bit set.
>>>
>>> As PCIe Spec 2.0(6.13/441) recommends:
>>> "Following a hot-plug event below a Downstream Port, it is strongly recommended that software
>>> Clear the ARI Forwarding Enable bit in the Downstream Port until software determines that a
>>> newly added component is in fact an ARI Device"
>>>
>>> This series of patches fix this problem.
>>>
>>> Yijing Wang (7):
>>>   PCI: rework pci_enable_ari for support disable ari forwarding
>>>   PCI, acpiphp: disable ARI forwarding for acpiphp
>>>   PCI, pciehp: disable ARI forwarding for pciehp
>>>   PCI, cpqphp: disable ARI forwarding for cpqphp
>>>   PCI, shpchp: disable ARI forwarding for shpchp
>>>   PCI, sgi: disable ARI forwarding for sgiphp
>>>   PCI, ibmphp: disable ARI forwarding for ibmphp
>>>
>>>  drivers/pci/hotplug/acpiphp_glue.c     |    1 +
>>>  drivers/pci/hotplug/cpci_hotplug_pci.c |    1 +
>>>  drivers/pci/hotplug/cpqphp_pci.c       |    1 +
>>>  drivers/pci/hotplug/ibmphp_core.c      |    1 +
>>>  drivers/pci/hotplug/pciehp_pci.c       |    1 +
>>>  drivers/pci/hotplug/sgi_hotplug.c      |    1 +
>>>  drivers/pci/hotplug/shpchp_pci.c       |    1 +
>>>  drivers/pci/pci.c                      |   16 +++++++++++-----
>>>  drivers/pci/pci.h                      |    2 +-
>>>  drivers/pci/probe.c                    |    2 +-
>>>  10 files changed, 20 insertions(+), 7 deletions(-)
>>>
>>>
>>>
>>> .
>>>
>>
>>
>>
>> .
>>
> 
> 


--
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
Yijing Wang - Oct. 9, 2012, 8:14 a.m.
On 2012/10/9 15:51, Jiang Liu wrote:
> When scanning PCI devices for hot-add, it guarantee function 0 is scanned
> at first. And when destroying PCI devices for hot-removal, we also need to
> destroy function 0 at last, but currently there's no such guarantees. 
> 

This is an issue, btw in pciehp now only try to hot remove fun from 0 --> 8,
ARI device supports up to 256 function device.
for (j = 0; j < 8; j++) {
	struct pci_dev *temp = pci_get_slot(parent, PCI_DEVFN(0, j));
	if (!temp)
		continue;


> On 2012-10-9 15:42, Yijing Wang wrote:
>> On 2012/10/9 11:42, Jiang Liu wrote:
>>> How about this patch, which disables ARI when adding new PCI devices?
>>> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
>>> index 5485883..c841aa6 100644
>>> --- a/drivers/pci/pci.c
>>> +++ b/drivers/pci/pci.c
>>> @@ -2016,13 +2016,14 @@ void pci_free_cap_save_buffers(struct pci_dev *dev)
>>>  void pci_enable_ari(struct pci_dev *dev)
>>>  {
>>
>> I think it's ok. Delay to disable ARI until next hot add is safe.
>> Maybe rename as pci_configure_ari is better.
>>
>>>         u32 cap;
>>> +       bool enable = true;
>>>         struct pci_dev *bridge;
>>>
>>>         if (pcie_ari_disabled || !pci_is_pcie(dev) || dev->devfn)
>>>                 return;
>>>
>>>         if (!pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ARI))
>>> -               return;
>>> +               enable = false;
>>>
>>>         bridge = dev->bus->self;
>>>         if (!bridge)
>>> @@ -2032,8 +2033,15 @@ void pci_enable_ari(struct pci_dev *dev)
>>>         if (!(cap & PCI_EXP_DEVCAP2_ARI))
>>>                 return;
>>>
>>> -       pcie_capability_set_word(bridge, PCI_EXP_DEVCTL2, PCI_EXP_DEVCTL2_ARI);
>>> -       bridge->ari_enabled = 1;
>>> +       if (enable) {
>>> +               pcie_capability_set_word(bridge, PCI_EXP_DEVCTL2,
>>> +                                        PCI_EXP_DEVCTL2_ARI);
>>> +               bridge->ari_enabled = 1;
>>> +       } else {
>>> +               pcie_capability_clear_word(bridge, PCI_EXP_DEVCTL2,
>>> +                                        PCI_EXP_DEVCTL2_ARI);
>>> +               bridge->ari_enabled = 0;
>>> +       }
>>>  }
>>>
>>>  /**
>>>
>>> On 2012-10-9 11:03, Yijing Wang wrote:
>>>> pci_enable_ari will be called if an ARI pci device found, then set its bridge ARI Forwarding Enable
>>>> bit in Device Control 2 Register. But the bridge ARI Forwarding Enable bit will never be cleared
>>>> when an ARI device hot removed.
>>>>
>>>> my steps:
>>>> 1. Hot add an ARI pci device;
>>>> 2. Hot remove the ARI pci device;
>>>> 3. Hot add an non ARI pci device;
>>>>
>>>> In this case, after setp 3, we could only find fun 0 of non ARI pci device because of its bridge ARI Forwarding Enable
>>>> bit set.
>>>>
>>>> As PCIe Spec 2.0(6.13/441) recommends:
>>>> "Following a hot-plug event below a Downstream Port, it is strongly recommended that software
>>>> Clear the ARI Forwarding Enable bit in the Downstream Port until software determines that a
>>>> newly added component is in fact an ARI Device"
>>>>
>>>> This series of patches fix this problem.
>>>>
>>>> Yijing Wang (7):
>>>>   PCI: rework pci_enable_ari for support disable ari forwarding
>>>>   PCI, acpiphp: disable ARI forwarding for acpiphp
>>>>   PCI, pciehp: disable ARI forwarding for pciehp
>>>>   PCI, cpqphp: disable ARI forwarding for cpqphp
>>>>   PCI, shpchp: disable ARI forwarding for shpchp
>>>>   PCI, sgi: disable ARI forwarding for sgiphp
>>>>   PCI, ibmphp: disable ARI forwarding for ibmphp
>>>>
>>>>  drivers/pci/hotplug/acpiphp_glue.c     |    1 +
>>>>  drivers/pci/hotplug/cpci_hotplug_pci.c |    1 +
>>>>  drivers/pci/hotplug/cpqphp_pci.c       |    1 +
>>>>  drivers/pci/hotplug/ibmphp_core.c      |    1 +
>>>>  drivers/pci/hotplug/pciehp_pci.c       |    1 +
>>>>  drivers/pci/hotplug/sgi_hotplug.c      |    1 +
>>>>  drivers/pci/hotplug/shpchp_pci.c       |    1 +
>>>>  drivers/pci/pci.c                      |   16 +++++++++++-----
>>>>  drivers/pci/pci.h                      |    2 +-
>>>>  drivers/pci/probe.c                    |    2 +-
>>>>  10 files changed, 20 insertions(+), 7 deletions(-)
>>>>
>>>>
>>>>
>>>> .
>>>>
>>>
>>>
>>>
>>> .
>>>
>>
>>
> 
> 
> 
> .
>
Jiang Liu - Oct. 9, 2012, 8:16 a.m.
On 2012-10-9 16:14, Yijing Wang wrote:
> On 2012/10/9 15:51, Jiang Liu wrote:
>> When scanning PCI devices for hot-add, it guarantee function 0 is scanned
>> at first. And when destroying PCI devices for hot-removal, we also need to
>> destroy function 0 at last, but currently there's no such guarantees. 
>>
> 
> This is an issue, btw in pciehp now only try to hot remove fun from 0 --> 8,
> ARI device supports up to 256 function device.
> for (j = 0; j < 8; j++) {
> 	struct pci_dev *temp = pci_get_slot(parent, PCI_DEVFN(0, j));
> 	if (!temp)
> 		continue;
Another patch to fix.

--
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.c b/drivers/pci/pci.c
index 5485883..c841aa6 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -2016,13 +2016,14 @@  void pci_free_cap_save_buffers(struct pci_dev *dev)
 void pci_enable_ari(struct pci_dev *dev)
 {
        u32 cap;
+       bool enable = true;
        struct pci_dev *bridge;

        if (pcie_ari_disabled || !pci_is_pcie(dev) || dev->devfn)
                return;

        if (!pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ARI))
-               return;
+               enable = false;

        bridge = dev->bus->self;
        if (!bridge)
@@ -2032,8 +2033,15 @@  void pci_enable_ari(struct pci_dev *dev)
        if (!(cap & PCI_EXP_DEVCAP2_ARI))
                return;

-       pcie_capability_set_word(bridge, PCI_EXP_DEVCTL2, PCI_EXP_DEVCTL2_ARI);
-       bridge->ari_enabled = 1;
+       if (enable) {
+               pcie_capability_set_word(bridge, PCI_EXP_DEVCTL2,
+                                        PCI_EXP_DEVCTL2_ARI);
+               bridge->ari_enabled = 1;
+       } else {
+               pcie_capability_clear_word(bridge, PCI_EXP_DEVCTL2,
+                                        PCI_EXP_DEVCTL2_ARI);
+               bridge->ari_enabled = 0;
+       }
 }

 /**