| Message ID | 1485834216-25191-1-git-send-email-gwshan@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
On 31/01/17 14:43, Gavin Shan wrote: > The following error message was observed. It's complaining M32 > memory window is missed on virtual PHB, which is a bit confusing. > The problem is the memory windows are never updated from its > device-tree node. > > PCI: Memory resource 0 not set for host bridge \ > /pciex@3fffe40000000/pci@0/device@0 > > This avoids the unnecessary error message by updating the PHB's > memory windows with pci_process_bridge_OF_ranges(). The function > is exported as well. > > Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com> I talked this over with Gavin in person today. We don't set a memory window on the vPHB or its devices because it's not necessary. The effect of this patch is to copy the memory resources from the *real* PHB to the vPHB, as given through the device tree. It shouldn't have any practical effect other than squashing this message. > --- > arch/powerpc/kernel/pci-common.c | 1 + > drivers/misc/cxl/vphb.c | 9 +++++++++ > 2 files changed, 10 insertions(+) > > diff --git a/arch/powerpc/kernel/pci-common.c b/arch/powerpc/kernel/pci-common.c > index 74bec54..b5ffd8a 100644 > --- a/arch/powerpc/kernel/pci-common.c > +++ b/arch/powerpc/kernel/pci-common.c > @@ -824,6 +824,7 @@ void pci_process_bridge_OF_ranges(struct pci_controller *hose, > } > } > } > +EXPORT_SYMBOL_GPL(pci_process_bridge_OF_ranges); > > /* Decide whether to display the domain number in /proc */ > int pci_proc_domain(struct pci_bus *bus) > diff --git a/drivers/misc/cxl/vphb.c b/drivers/misc/cxl/vphb.c > index 3519ace..8382761 100644 > --- a/drivers/misc/cxl/vphb.c > +++ b/drivers/misc/cxl/vphb.c > @@ -215,6 +215,15 @@ int cxl_pci_vphb_add(struct cxl_afu *afu) > if (!phb) > return -ENODEV; > > + /* Parse IO and memory ranges */ > + if (dev_is_pci(parent)) { Is this ever going to be false? > + struct pci_dev *pdev; I think we tend to keep declarations up at the top of the function? > + > + pdev = to_pci_dev(parent); > + vphb_dn = pnv_pci_get_phb_node(pdev); > + pci_process_bridge_OF_ranges(phb, vphb_dn, false); > + } > + > /* Setup parent in sysfs */ > phb->parent = parent; > >
Andrew Donnellan <andrew.donnellan@au1.ibm.com> writes: > On 31/01/17 14:43, Gavin Shan wrote: >> The following error message was observed. It's complaining M32 >> memory window is missed on virtual PHB, which is a bit confusing. >> The problem is the memory windows are never updated from its >> device-tree node. >> >> PCI: Memory resource 0 not set for host bridge \ >> /pciex@3fffe40000000/pci@0/device@0 >> >> This avoids the unnecessary error message by updating the PHB's >> memory windows with pci_process_bridge_OF_ranges(). The function >> is exported as well. >> >> Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com> > > I talked this over with Gavin in person today. > > We don't set a memory window on the vPHB or its devices because it's not > necessary. > > The effect of this patch is to copy the memory resources from the *real* > PHB to the vPHB, as given through the device tree. It shouldn't have any > practical effect other than squashing this message. It sounds a bit backward to me. If we don't need the resources then why have them? If we have code that thinks that's an error, than maybe that's what needs fixing, or special casing for the vPHB? cheers
On Tue, Feb 07, 2017 at 07:14:33PM +1100, Andrew Donnellan wrote: >On 31/01/17 14:43, Gavin Shan wrote: >>The following error message was observed. It's complaining M32 >>memory window is missed on virtual PHB, which is a bit confusing. >>The problem is the memory windows are never updated from its >>device-tree node. >> >> PCI: Memory resource 0 not set for host bridge \ >> /pciex@3fffe40000000/pci@0/device@0 >> >>This avoids the unnecessary error message by updating the PHB's >>memory windows with pci_process_bridge_OF_ranges(). The function >>is exported as well. >> >>Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com> > >I talked this over with Gavin in person today. > >We don't set a memory window on the vPHB or its devices because it's not >necessary. > >The effect of this patch is to copy the memory resources from the *real* PHB >to the vPHB, as given through the device tree. It shouldn't have any >practical effect other than squashing this message. > Andrew, thanks for the explanation. I will provide more details as reply to Michael's thread. We might need alternative fix for this issue. Lets have more discussion in that thread if needed. > >>--- >> arch/powerpc/kernel/pci-common.c | 1 + >> drivers/misc/cxl/vphb.c | 9 +++++++++ >> 2 files changed, 10 insertions(+) >> >>diff --git a/arch/powerpc/kernel/pci-common.c b/arch/powerpc/kernel/pci-common.c >>index 74bec54..b5ffd8a 100644 >>--- a/arch/powerpc/kernel/pci-common.c >>+++ b/arch/powerpc/kernel/pci-common.c >>@@ -824,6 +824,7 @@ void pci_process_bridge_OF_ranges(struct pci_controller *hose, >> } >> } >> } >>+EXPORT_SYMBOL_GPL(pci_process_bridge_OF_ranges); >> >> /* Decide whether to display the domain number in /proc */ >> int pci_proc_domain(struct pci_bus *bus) >>diff --git a/drivers/misc/cxl/vphb.c b/drivers/misc/cxl/vphb.c >>index 3519ace..8382761 100644 >>--- a/drivers/misc/cxl/vphb.c >>+++ b/drivers/misc/cxl/vphb.c >>@@ -215,6 +215,15 @@ int cxl_pci_vphb_add(struct cxl_afu *afu) >> if (!phb) >> return -ENODEV; >> >>+ /* Parse IO and memory ranges */ >>+ if (dev_is_pci(parent)) { > >Is this ever going to be false? > Yeah, I think it can be removed and to declare @pdev at the top of the function. >>+ struct pci_dev *pdev; > >I think we tend to keep declarations up at the top of the function? > I think it's not bad idea if @pdev is invisible out of the block of code, for a bit better code readability. However, those who are maintaining the code might have their own personal tastes. If it's your preferrence, I need to follow for sure. >>+ >>+ pdev = to_pci_dev(parent); >>+ vphb_dn = pnv_pci_get_phb_node(pdev); >>+ pci_process_bridge_OF_ranges(phb, vphb_dn, false); >>+ } >>+ >> /* Setup parent in sysfs */ >> phb->parent = parent; >> >> Thanks, Gavin
On Tue, Feb 07, 2017 at 10:12:48PM +1100, Michael Ellerman wrote: >Andrew Donnellan <andrew.donnellan@au1.ibm.com> writes: .../... >> The effect of this patch is to copy the memory resources from the *real* >> PHB to the vPHB, as given through the device tree. It shouldn't have any >> practical effect other than squashing this message. > >It sounds a bit backward to me. If we don't need the resources then >why have them? > >If we have code that thinks that's an error, than maybe that's what >needs fixing, or special casing for the vPHB? > Yeah, vPHB is a special case. There are basically two stages in PCI enumeration: probing and then resource assignment. vPHB is different from *real* PHB as the resource assignment is skipped on it. So vPHB doesn't need any resources to be populated. However, there is a check in probing stage and it's where the warning message comes from. drivers/misc/cxl/vphb.c::cxl_pci_vphb_add() arch/powerpc/kernel/pci-common.c::pcibios_scan_phb() pcibios_setup_phb_resources() static void pcibios_setup_phb_resources(struct pci_controller *hose, struct list_head *resources) { : for (i = 0; i < 3; ++i) { res = &hose->mem_resources[i]; if (!res->flags) { if (i == 0) printk(KERN_ERR "PCI: Memory resource 0 not set for " "host bridge %s (domain %d)\n", hose->dn->full_name, hose->global_number); continue; } : } Alternatively, we can replace prink(KERN_ERR) with pr_debug(). It's going to affect all PHBs including the real ones. Andrew and Michael, what do you think? :-) Thanks, Gavin
On 08/02/17 10:21, Gavin Shan wrote: > On Tue, Feb 07, 2017 at 10:12:48PM +1100, Michael Ellerman wrote: >> Andrew Donnellan <andrew.donnellan@au1.ibm.com> writes: > > .../... > >>> The effect of this patch is to copy the memory resources from the *real* >>> PHB to the vPHB, as given through the device tree. It shouldn't have any >>> practical effect other than squashing this message. >> >> It sounds a bit backward to me. If we don't need the resources then >> why have them? >> >> If we have code that thinks that's an error, than maybe that's what >> needs fixing, or special casing for the vPHB? >> > > Yeah, vPHB is a special case. There are basically two stages in PCI enumeration: > probing and then resource assignment. vPHB is different from *real* PHB as the > resource assignment is skipped on it. So vPHB doesn't need any resources to be > populated. However, there is a check in probing stage and it's where the warning > message comes from. > > drivers/misc/cxl/vphb.c::cxl_pci_vphb_add() > arch/powerpc/kernel/pci-common.c::pcibios_scan_phb() > pcibios_setup_phb_resources() > > static void pcibios_setup_phb_resources(struct pci_controller *hose, > struct list_head *resources) > { > : > for (i = 0; i < 3; ++i) { > res = &hose->mem_resources[i]; > if (!res->flags) { > if (i == 0) > printk(KERN_ERR "PCI: Memory resource 0 not set for " > "host bridge %s (domain %d)\n", > hose->dn->full_name, hose->global_number); > continue; > } > : > } > > Alternatively, we can replace prink(KERN_ERR) with pr_debug(). It's going to > affect all PHBs including the real ones. Andrew and Michael, what do you think? :-) In what other circumstances do we get this error printed on real PHBs?
On Wed, Feb 08, 2017 at 10:39:55AM +1100, Andrew Donnellan wrote: >On 08/02/17 10:21, Gavin Shan wrote: >>On Tue, Feb 07, 2017 at 10:12:48PM +1100, Michael Ellerman wrote: >>>Andrew Donnellan <andrew.donnellan@au1.ibm.com> writes: >> >>.../... >> >>>>The effect of this patch is to copy the memory resources from the *real* >>>>PHB to the vPHB, as given through the device tree. It shouldn't have any >>>>practical effect other than squashing this message. >>> >>>It sounds a bit backward to me. If we don't need the resources then >>>why have them? >>> >>>If we have code that thinks that's an error, than maybe that's what >>>needs fixing, or special casing for the vPHB? >>> >> >>Yeah, vPHB is a special case. There are basically two stages in PCI enumeration: >>probing and then resource assignment. vPHB is different from *real* PHB as the >>resource assignment is skipped on it. So vPHB doesn't need any resources to be >>populated. However, there is a check in probing stage and it's where the warning >>message comes from. >> >> drivers/misc/cxl/vphb.c::cxl_pci_vphb_add() >> arch/powerpc/kernel/pci-common.c::pcibios_scan_phb() >> pcibios_setup_phb_resources() >> >> static void pcibios_setup_phb_resources(struct pci_controller *hose, >> struct list_head *resources) >> { >> : >> for (i = 0; i < 3; ++i) { >> res = &hose->mem_resources[i]; >> if (!res->flags) { >> if (i == 0) >> printk(KERN_ERR "PCI: Memory resource 0 not set for " >> "host bridge %s (domain %d)\n", >> hose->dn->full_name, hose->global_number); >> continue; >> } >> : >> } >> >>Alternatively, we can replace prink(KERN_ERR) with pr_debug(). It's going to >>affect all PHBs including the real ones. Andrew and Michael, what do you think? :-) > >In what other circumstances do we get this error printed on real PHBs? > When hose->mem_resources[0] isn't built from PHB's device-tree node. It means the device-tree node's "ranges" isn't populated correctly by loader and it should be very rare ... I never saw it before. Thanks, Gavin
Gavin Shan <gwshan@linux.vnet.ibm.com> writes: > On Wed, Feb 08, 2017 at 10:39:55AM +1100, Andrew Donnellan wrote: >>On 08/02/17 10:21, Gavin Shan wrote: >>>On Tue, Feb 07, 2017 at 10:12:48PM +1100, Michael Ellerman wrote: >>>>Andrew Donnellan <andrew.donnellan@au1.ibm.com> writes: >>> >>>.../... >>> >>>>>The effect of this patch is to copy the memory resources from the *real* >>>>>PHB to the vPHB, as given through the device tree. It shouldn't have any >>>>>practical effect other than squashing this message. >>>> >>>>It sounds a bit backward to me. If we don't need the resources then >>>>why have them? >>>> >>>>If we have code that thinks that's an error, than maybe that's what >>>>needs fixing, or special casing for the vPHB? >>>> >>> >>>Yeah, vPHB is a special case. There are basically two stages in PCI enumeration: >>>probing and then resource assignment. vPHB is different from *real* PHB as the >>>resource assignment is skipped on it. So vPHB doesn't need any resources to be >>>populated. However, there is a check in probing stage and it's where the warning >>>message comes from. >>> >>> drivers/misc/cxl/vphb.c::cxl_pci_vphb_add() >>> arch/powerpc/kernel/pci-common.c::pcibios_scan_phb() >>> pcibios_setup_phb_resources() >>> >>> static void pcibios_setup_phb_resources(struct pci_controller *hose, >>> struct list_head *resources) >>> { >>> : >>> for (i = 0; i < 3; ++i) { >>> res = &hose->mem_resources[i]; >>> if (!res->flags) { >>> if (i == 0) >>> printk(KERN_ERR "PCI: Memory resource 0 not set for " >>> "host bridge %s (domain %d)\n", >>> hose->dn->full_name, hose->global_number); >>> continue; >>> } >>> : >>> } >>> >>>Alternatively, we can replace prink(KERN_ERR) with pr_debug(). It's going to >>>affect all PHBs including the real ones. Andrew and Michael, what do you think? :-) >> >>In what other circumstances do we get this error printed on real PHBs? >> > > When hose->mem_resources[0] isn't built from PHB's device-tree node. It means > the device-tree node's "ranges" isn't populated correctly by loader and it > should be very rare ... I never saw it before. Sounds to me like we could probably just drop the warning. Or make it pr_devel(). cheers
On Wed, Feb 08, 2017 at 01:41:19PM +1100, Michael Ellerman wrote: >Gavin Shan <gwshan@linux.vnet.ibm.com> writes: > >> On Wed, Feb 08, 2017 at 10:39:55AM +1100, Andrew Donnellan wrote: >>>On 08/02/17 10:21, Gavin Shan wrote: >>>>On Tue, Feb 07, 2017 at 10:12:48PM +1100, Michael Ellerman wrote: >>>>>Andrew Donnellan <andrew.donnellan@au1.ibm.com> writes: >>>> >>>>.../... >>>> >>>>>>The effect of this patch is to copy the memory resources from the *real* >>>>>>PHB to the vPHB, as given through the device tree. It shouldn't have any >>>>>>practical effect other than squashing this message. >>>>> >>>>>It sounds a bit backward to me. If we don't need the resources then >>>>>why have them? >>>>> >>>>>If we have code that thinks that's an error, than maybe that's what >>>>>needs fixing, or special casing for the vPHB? >>>>> >>>> >>>>Yeah, vPHB is a special case. There are basically two stages in PCI enumeration: >>>>probing and then resource assignment. vPHB is different from *real* PHB as the >>>>resource assignment is skipped on it. So vPHB doesn't need any resources to be >>>>populated. However, there is a check in probing stage and it's where the warning >>>>message comes from. >>>> >>>> drivers/misc/cxl/vphb.c::cxl_pci_vphb_add() >>>> arch/powerpc/kernel/pci-common.c::pcibios_scan_phb() >>>> pcibios_setup_phb_resources() >>>> >>>> static void pcibios_setup_phb_resources(struct pci_controller *hose, >>>> struct list_head *resources) >>>> { >>>> : >>>> for (i = 0; i < 3; ++i) { >>>> res = &hose->mem_resources[i]; >>>> if (!res->flags) { >>>> if (i == 0) >>>> printk(KERN_ERR "PCI: Memory resource 0 not set for " >>>> "host bridge %s (domain %d)\n", >>>> hose->dn->full_name, hose->global_number); >>>> continue; >>>> } >>>> : >>>> } >>>> >>>>Alternatively, we can replace prink(KERN_ERR) with pr_debug(). It's going to >>>>affect all PHBs including the real ones. Andrew and Michael, what do you think? :-) >>> >>>In what other circumstances do we get this error printed on real PHBs? >>> >> >> When hose->mem_resources[0] isn't built from PHB's device-tree node. It means >> the device-tree node's "ranges" isn't populated correctly by loader and it >> should be very rare ... I never saw it before. > >Sounds to me like we could probably just drop the warning. Or make it pr_devel(). > Agree, I will drop it and post the patch shortly. Thanks for confirm. Thanks, Gavin
diff --git a/arch/powerpc/kernel/pci-common.c b/arch/powerpc/kernel/pci-common.c index 74bec54..b5ffd8a 100644 --- a/arch/powerpc/kernel/pci-common.c +++ b/arch/powerpc/kernel/pci-common.c @@ -824,6 +824,7 @@ void pci_process_bridge_OF_ranges(struct pci_controller *hose, } } } +EXPORT_SYMBOL_GPL(pci_process_bridge_OF_ranges); /* Decide whether to display the domain number in /proc */ int pci_proc_domain(struct pci_bus *bus) diff --git a/drivers/misc/cxl/vphb.c b/drivers/misc/cxl/vphb.c index 3519ace..8382761 100644 --- a/drivers/misc/cxl/vphb.c +++ b/drivers/misc/cxl/vphb.c @@ -215,6 +215,15 @@ int cxl_pci_vphb_add(struct cxl_afu *afu) if (!phb) return -ENODEV; + /* Parse IO and memory ranges */ + if (dev_is_pci(parent)) { + struct pci_dev *pdev; + + pdev = to_pci_dev(parent); + vphb_dn = pnv_pci_get_phb_node(pdev); + pci_process_bridge_OF_ranges(phb, vphb_dn, false); + } + /* Setup parent in sysfs */ phb->parent = parent;
The following error message was observed. It's complaining M32 memory window is missed on virtual PHB, which is a bit confusing. The problem is the memory windows are never updated from its device-tree node. PCI: Memory resource 0 not set for host bridge \ /pciex@3fffe40000000/pci@0/device@0 This avoids the unnecessary error message by updating the PHB's memory windows with pci_process_bridge_OF_ranges(). The function is exported as well. Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com> --- arch/powerpc/kernel/pci-common.c | 1 + drivers/misc/cxl/vphb.c | 9 +++++++++ 2 files changed, 10 insertions(+)