[V10,05/12] powerpc/eeh: Cache only BARs, not windows or IOV BARs

Commit Message

Wei Yang Oct. 26, 2015, 3:15 a.m. UTC

EEH address cache, which helps to locate the PCI device according to
the given (physical) MMIO address, didn't cover PCI bridges. Also, it
shouldn't return PF with address in PF's IOV BARs. Instead, the VFs
should be returned.

Also, by doing so, it removes the type check in
eeh_addr_cache_insert_dev(), since bridge's window would not be cached.

The patch restricts the address cache to cover first 7 BARs for the
above purposes.

[gwshan: changelog]
Signed-off-by: Wei Yang <[e-mail]>
Acked-by: Gavin Shan <[e-mail]>
---
 arch/powerpc/kernel/eeh_cache.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

Comments

Daniel Axtens Oct. 29, 2015, 3:29 a.m. UTC | #1

Wei Yang <[e-mail]> writes:

> EEH address cache, which helps to locate the PCI device according to
> the given (physical) MMIO address, didn't cover PCI bridges. Also, it
> shouldn't return PF with address in PF's IOV BARs. Instead, the VFs
> should be returned.
>
> Also, by doing so, it removes the type check in
> eeh_addr_cache_insert_dev(), since bridge's window would not be cached.
>
> The patch restricts the address cache to cover first 7 BARs for the
> above purposes.
If I've understoond the patch correctly, I think you want to swap the
last two paragraphs in the commit message:

"Restrict the address cache to cover the first 7 BARs...

Since the window of a bridge will now not be cached, remove the type
check..."

With regards to the actual patch, I have now got access to the PCI and
SR-IOV specs, but I'm still getting to grips with it all so let me know
if something I say doesn't make sense.

Here, you restrict the enumeration of resources to the standard and
extension ROM resources (the first 7), which excludes enumeration of
VF resources. That much I understand.

I'm having more trouble convincing myself that it's safe or desirable to
drop the test for bridges. I think I understand that the change to the
for loop means it _should_ be safe, but is there any motivation for the
change other than making the code more straightforward?

>  	/* Walk resources on this device, poke them into the tree *
This comment probably needs to be made more descriptive given the change.
> -	for (i = 0; i < DEVICE_COUNT_RESOURCE; i++) {
> +	for (i = 0; i <= PCI_ROM_RESOURCE; i++) {
>  		resource_size_t start = pci_resource_start(dev,i);
>  		resource_size_t end = pci_resource_end(dev,i);
>  		unsigned long flags = pci_resource_flags(dev,i);
> @@ -222,10 +222,6 @@ void eeh_addr_cache_insert_dev(struct pci_dev *dev)
>  {

Regards,
Daniel

>  	unsigned long flags;
>  
> -	/* Ignore PCI bridges */
> -	if ((dev->class >> 16) == PCI_BASE_CLASS_BRIDGE)
> -		return;
> -
>  	spin_lock_irqsave(&pci_io_addr_cache_root.piar_lock, flags);
>  	__eeh_addr_cache_insert_dev(dev);
>  	spin_unlock_irqrestore(&pci_io_addr_cache_root.piar_lock, flags);
> -- 
> 2.5.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to [e-mail]
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Wei Yang Oct. 29, 2015, 8:57 a.m. UTC | #2

On Thu, Oct 29, 2015 at 02:29:19PM +1100, Daniel Axtens wrote:
>Wei Yang <[e-mail]> writes:
>
>> EEH address cache, which helps to locate the PCI device according to
>> the given (physical) MMIO address, didn't cover PCI bridges. Also, it
>> shouldn't return PF with address in PF's IOV BARs. Instead, the VFs
>> should be returned.
>>
>> Also, by doing so, it removes the type check in
>> eeh_addr_cache_insert_dev(), since bridge's window would not be cached.
>>
>> The patch restricts the address cache to cover first 7 BARs for the
>> above purposes.
>If I've understoond the patch correctly, I think you want to swap the
>last two paragraphs in the commit message:
>
>"Restrict the address cache to cover the first 7 BARs...
>
>Since the window of a bridge will now not be cached, remove the type
>check..."
>

Hmm... my purpose in the last paragraphs is to state what the patch does and
the 2nd one is to mention another change in the log.

The order is both fine to me.

>With regards to the actual patch, I have now got access to the PCI and
>SR-IOV specs, but I'm still getting to grips with it all so let me know
>if something I say doesn't make sense.
>
>Here, you restrict the enumeration of resources to the standard and
>extension ROM resources (the first 7), which excludes enumeration of
>VF resources. That much I understand.
>
>I'm having more trouble convincing myself that it's safe or desirable to
>drop the test for bridges. I think I understand that the change to the
>for loop means it _should_ be safe, but is there any motivation for the
>change other than making the code more straightforward?
>

The motivation is just make the code more straightforward.

For a bridge device, the first 7 resources are not used and the last several
are not cached, This is the reason why I remove it in the patch.

>>  	/* Walk resources on this device, poke them into the tree *
>This comment probably needs to be made more descriptive given the change.

Right, will change it.

>> -	for (i = 0; i < DEVICE_COUNT_RESOURCE; i++) {
>> +	for (i = 0; i <= PCI_ROM_RESOURCE; i++) {
>>  		resource_size_t start = pci_resource_start(dev,i);
>>  		resource_size_t end = pci_resource_end(dev,i);
>>  		unsigned long flags = pci_resource_flags(dev,i);
>> @@ -222,10 +222,6 @@ void eeh_addr_cache_insert_dev(struct pci_dev *dev)
>>  {
>
>Regards,
>Daniel
>
>>  	unsigned long flags;
>>  
>> -	/* Ignore PCI bridges */
>> -	if ((dev->class >> 16) == PCI_BASE_CLASS_BRIDGE)
>> -		return;
>> -
>>  	spin_lock_irqsave(&pci_io_addr_cache_root.piar_lock, flags);
>>  	__eeh_addr_cache_insert_dev(dev);
>>  	spin_unlock_irqrestore(&pci_io_addr_cache_root.piar_lock, flags);
>> -- 
>> 2.5.0
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
>> the body of a message to [e-mail]
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Alexey Kardashevskiy Oct. 30, 2015, 3:22 a.m. UTC | #3

On 10/26/2015 02:15 PM, Wei Yang wrote:
> EEH address cache, which helps to locate the PCI device according to
> the given (physical) MMIO address, didn't cover PCI bridges. Also, it
> shouldn't return PF

"it shouldn't return" is about the cache, right? eeh_addr_cache_get_dev() - 
this guy can "return", the cache cannot.

> with address in PF's IOV BARs. Instead, the VFs
> should be returned.
>
> Also, by doing so, it removes the type check in
> eeh_addr_cache_insert_dev(), since bridge's window would not be cached.
>
> The patch restricts the address cache to cover first 7 BARs for the
> above purposes.


I'd better understand something like this :)

This restricts the EEH address cache to use only first 7 BARs. This makes 
__eeh_addr_cache_insert_dev() ignore PCI bridge windows and IOV BARs. As 
the result of this change, eeh_addr_cache_get_dev() will return VFs from 
VF's resource addresses instead of parent PFs.

This removes extra check for a PCI bridge as we limit 
__eeh_addr_cache_insert_dev() to 7 BARs and this effectively excludes PCI 
bridges from being cached.


>
> [gwshan: changelog]
> Signed-off-by: Wei Yang <[e-mail]>
> Acked-by: Gavin Shan <[e-mail]>
> ---
>   arch/powerpc/kernel/eeh_cache.c | 6 +-----
>   1 file changed, 1 insertion(+), 5 deletions(-)
>
> diff --git a/arch/powerpc/kernel/eeh_cache.c b/arch/powerpc/kernel/eeh_cache.c
> index a1e86e1..e6887f0 100644
> --- a/arch/powerpc/kernel/eeh_cache.c
> +++ b/arch/powerpc/kernel/eeh_cache.c
> @@ -196,7 +196,7 @@ static void __eeh_addr_cache_insert_dev(struct pci_dev *dev)
>   	}
>
>   	/* Walk resources on this device, poke them into the tree */
> -	for (i = 0; i < DEVICE_COUNT_RESOURCE; i++) {
> +	for (i = 0; i <= PCI_ROM_RESOURCE; i++) {
>   		resource_size_t start = pci_resource_start(dev,i);
>   		resource_size_t end = pci_resource_end(dev,i);
>   		unsigned long flags = pci_resource_flags(dev,i);
> @@ -222,10 +222,6 @@ void eeh_addr_cache_insert_dev(struct pci_dev *dev)
>   {
>   	unsigned long flags;
>
> -	/* Ignore PCI bridges */
> -	if ((dev->class >> 16) == PCI_BASE_CLASS_BRIDGE)
> -		return;
> -
>   	spin_lock_irqsave(&pci_io_addr_cache_root.piar_lock, flags);
>   	__eeh_addr_cache_insert_dev(dev);
>   	spin_unlock_irqrestore(&pci_io_addr_cache_root.piar_lock, flags);
>

Wei Yang Oct. 30, 2015, 6:37 a.m. UTC | #4

On Fri, Oct 30, 2015 at 02:22:43PM +1100, Alexey Kardashevskiy wrote:
>On 10/26/2015 02:15 PM, Wei Yang wrote:
>>EEH address cache, which helps to locate the PCI device according to
>>the given (physical) MMIO address, didn't cover PCI bridges. Also, it
>>shouldn't return PF
>
>"it shouldn't return" is about the cache, right? eeh_addr_cache_get_dev() -
>this guy can "return", the cache cannot.
>

Here I want to say if we cache the PF's IOV BAR, eeh_addr_cache_get_dev()
would return PF when the address is for VF.

>>with address in PF's IOV BARs. Instead, the VFs
>>should be returned.
>>
>>Also, by doing so, it removes the type check in
>>eeh_addr_cache_insert_dev(), since bridge's window would not be cached.
>>
>>The patch restricts the address cache to cover first 7 BARs for the
>>above purposes.
>
>
>I'd better understand something like this :)
>
>This restricts the EEH address cache to use only first 7 BARs. This makes
>__eeh_addr_cache_insert_dev() ignore PCI bridge windows and IOV BARs. As the
>result of this change, eeh_addr_cache_get_dev() will return VFs from VF's
>resource addresses instead of parent PFs.
>
>This removes extra check for a PCI bridge as we limit
>__eeh_addr_cache_insert_dev() to 7 BARs and this effectively excludes PCI
>bridges from being cached.
>

Yep, I think this one is more clear. Would use this one.

>
>>
>>[gwshan: changelog]
>>Signed-off-by: Wei Yang <[e-mail]>
>>Acked-by: Gavin Shan <[e-mail]>
>>---
>>  arch/powerpc/kernel/eeh_cache.c | 6 +-----
>>  1 file changed, 1 insertion(+), 5 deletions(-)
>>
>>diff --git a/arch/powerpc/kernel/eeh_cache.c b/arch/powerpc/kernel/eeh_cache.c
>>index a1e86e1..e6887f0 100644
>>--- a/arch/powerpc/kernel/eeh_cache.c
>>+++ b/arch/powerpc/kernel/eeh_cache.c
>>@@ -196,7 +196,7 @@ static void __eeh_addr_cache_insert_dev(struct pci_dev *dev)
>>  	}
>>
>>  	/* Walk resources on this device, poke them into the tree */
>>-	for (i = 0; i < DEVICE_COUNT_RESOURCE; i++) {
>>+	for (i = 0; i <= PCI_ROM_RESOURCE; i++) {
>>  		resource_size_t start = pci_resource_start(dev,i);
>>  		resource_size_t end = pci_resource_end(dev,i);
>>  		unsigned long flags = pci_resource_flags(dev,i);
>>@@ -222,10 +222,6 @@ void eeh_addr_cache_insert_dev(struct pci_dev *dev)
>>  {
>>  	unsigned long flags;
>>
>>-	/* Ignore PCI bridges */
>>-	if ((dev->class >> 16) == PCI_BASE_CLASS_BRIDGE)
>>-		return;
>>-
>>  	spin_lock_irqsave(&pci_io_addr_cache_root.piar_lock, flags);
>>  	__eeh_addr_cache_insert_dev(dev);
>>  	spin_unlock_irqrestore(&pci_io_addr_cache_root.piar_lock, flags);
>>
>
>
>-- 
>Alexey

Patch

diff --git a/arch/powerpc/kernel/eeh_cache.c b/arch/powerpc/kernel/eeh_cache.c
index a1e86e1..e6887f0 100644
--- a/arch/powerpc/kernel/eeh_cache.c
+++ b/arch/powerpc/kernel/eeh_cache.c
@@ -196,7 +196,7 @@  static void __eeh_addr_cache_insert_dev(struct pci_dev *dev)
 	}
 
 	/* Walk resources on this device, poke them into the tree */
-	for (i = 0; i < DEVICE_COUNT_RESOURCE; i++) {
+	for (i = 0; i <= PCI_ROM_RESOURCE; i++) {
 		resource_size_t start = pci_resource_start(dev,i);
 		resource_size_t end = pci_resource_end(dev,i);
 		unsigned long flags = pci_resource_flags(dev,i);
@@ -222,10 +222,6 @@  void eeh_addr_cache_insert_dev(struct pci_dev *dev)
 {
 	unsigned long flags;
 
-	/* Ignore PCI bridges */
-	if ((dev->class >> 16) == PCI_BASE_CLASS_BRIDGE)
-		return;
-
 	spin_lock_irqsave(&pci_io_addr_cache_root.piar_lock, flags);
 	__eeh_addr_cache_insert_dev(dev);
 	spin_unlock_irqrestore(&pci_io_addr_cache_root.piar_lock, flags);