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

[Daniel Axtens <dja@axtens.net>]

[-- Attachment #1: Type: text/plain, Size: 2456 bytes --]

Wei Yang <weiyang@linux.vnet.ibm.com> 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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 859 bytes --]