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

[Wei Yang]

On Thu, Oct 29, 2015 at 02:29:19PM +1100, Daniel Axtens wrote:
>Wei Yang <weiyang at 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..."
>

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 majordomo at vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
Richard Yang
Help you, Help me