| Message ID | 1445829362-2738-6-git-send-email-weiyang@linux.vnet.ibm.com |
|---|---|
| State | Not Applicable |
| Headers | show |
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
On Thu, Oct 29, 2015 at 02:29:19PM +1100, Daniel Axtens wrote: >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..." > 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@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html
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 <weiyang@linux.vnet.ibm.com> > Acked-by: Gavin Shan <gwshan@linux.vnet.ibm.com> > --- > 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); >
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 <weiyang@linux.vnet.ibm.com> >>Acked-by: Gavin Shan <gwshan@linux.vnet.ibm.com> >>--- >> 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
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);