| Message ID | 20090416193110.GF1926@parisc-linux.org (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
Matthew Wilcox wrote: > Commit 30a18d6c3f1e774de656ebd8ff219d53e2ba4029 introduced a new function > to set the PCI bus resources. Unfortunately, neither the author, nor > the committers seemed to know that we already have somewhere to do that > -- pcibios_fixup_bus(). This patch moves the hook (used only by the K8 > code) into x86-specific code where it should have been in the first place. > > Signed-off-by: Matthew Wilcox <willy@linux.intel.com> > Cc: Yinghai Lu <yinghai.lu@sun.com> > Cc: Ingo Molnar <mingo@elte.hu> > Cc: Thomas Gleixner <tglx@linutronix.de> > > diff --git a/arch/x86/pci/common.c b/arch/x86/pci/common.c > index 8c362b9..f80ece5 100644 > --- a/arch/x86/pci/common.c > +++ b/arch/x86/pci/common.c > @@ -142,15 +142,20 @@ static void __devinit pcibios_fixup_device_resources(struct pci_dev *dev) > } > } > > +void __attribute__((weak)) set_pci_bus_resources_arch_default(struct pci_bus *b) > +{ > +} > + > /* > * Called after each bus is probed, but before its children > * are examined. > */ > > -void __devinit pcibios_fixup_bus(struct pci_bus *b) > +void __devinit pcibios_fixup_bus(struct pci_bus *b) > { > struct pci_dev *dev; > > + set_pci_bus_resources_arch_default(b); > pci_read_bridge_bases(b); > list_for_each_entry(dev, &b->devices, bus_list) > pcibios_fixup_device_resources(dev); > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c > index 8eb50df..e3c3e08 100644 > --- a/drivers/pci/probe.c > +++ b/drivers/pci/probe.c > @@ -1118,10 +1118,6 @@ unsigned int __devinit pci_scan_child_bus(struct pci_bus *bus) > return max; > } > > -void __attribute__((weak)) set_pci_bus_resources_arch_default(struct pci_bus *b) > -{ > -} > - > struct pci_bus * pci_create_bus(struct device *parent, > int bus, struct pci_ops *ops, void *sysdata) > { > @@ -1180,8 +1176,6 @@ struct pci_bus * pci_create_bus(struct device *parent, > b->resource[0] = &ioport_resource; > b->resource[1] = &iomem_resource; > > - set_pci_bus_resources_arch_default(b); > - > return b; > > dev_create_file_err: > > looks good, Thanks YH -- 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, Apr 16, 2009 at 01:17:56PM -0700, Yinghai Lu wrote: > Matthew Wilcox wrote: > > +void __attribute__((weak)) set_pci_bus_resources_arch_default(struct pci_bus *b) > > +{ > > +} > > + > > looks good, Thanks You might want to rename the hook from set_pci_bus_resources_arch_default but I don't have a good suggestion for a new name, plus I wanted to make this patch disturb as little as possible.
* Matthew Wilcox <matthew@wil.cx> wrote: > Commit 30a18d6c3f1e774de656ebd8ff219d53e2ba4029 introduced a new > function to set the PCI bus resources. Unfortunately, neither the > author, nor the committers seemed to know that we already have > somewhere to do that -- pcibios_fixup_bus(). This patch moves the > hook (used only by the K8 code) into x86-specific code where it > should have been in the first place. That's one scary commit! Thanks for cleaning this up. Acked-by: Ingo Molnar <mingo@elte.hu> A better name for the callback would be x86_pci_root_bus_quirks() ? Ingo -- 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
Ingo Molnar wrote: > * Matthew Wilcox <matthew@wil.cx> wrote: > > >> Commit 30a18d6c3f1e774de656ebd8ff219d53e2ba4029 introduced a new >> function to set the PCI bus resources. Unfortunately, neither the >> author, nor the committers seemed to know that we already have >> somewhere to do that -- pcibios_fixup_bus(). This patch moves the >> hook (used only by the K8 code) into x86-specific code where it >> should have been in the first place. >> > > That's one scary commit! > > Thanks for cleaning this up. > > Acked-by: Ingo Molnar <mingo@elte.hu> > > A better name for the callback would be x86_pci_root_bus_quirks() ? > ok, back to this thread. pci_create_bus is only for root bus, and pci_scan_child_bus is called on that root bus. if we moved x86_pci_root_bus_res_quirks calling in pci_scan_child_bus, it will be checked with all child buses. so we'd better to keep that calling in pci_create_bus. YH -- 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
Matthew Wilcox wrote: > Commit 30a18d6c3f1e774de656ebd8ff219d53e2ba4029 introduced a new function > to set the PCI bus resources. Unfortunately, neither the author, nor > the committers seemed to know that we already have somewhere to do that > -- pcibios_fixup_bus(). This patch moves the hook (used only by the K8 > code) into x86-specific code where it should have been in the first place. > > Signed-off-by: Matthew Wilcox <willy@linux.intel.com> > Cc: Yinghai Lu <yinghai.lu@sun.com> > Cc: Ingo Molnar <mingo@elte.hu> > Cc: Thomas Gleixner <tglx@linutronix.de> > > diff --git a/arch/x86/pci/common.c b/arch/x86/pci/common.c > index 8c362b9..f80ece5 100644 > --- a/arch/x86/pci/common.c > +++ b/arch/x86/pci/common.c > @@ -142,15 +142,20 @@ static void __devinit pcibios_fixup_device_resources(struct pci_dev *dev) > } > } > > +void __attribute__((weak)) set_pci_bus_resources_arch_default(struct pci_bus *b) > +{ > +} > + > /* > * Called after each bus is probed, but before its children > * are examined. > */ > > -void __devinit pcibios_fixup_bus(struct pci_bus *b) > +void __devinit pcibios_fixup_bus(struct pci_bus *b) > { > struct pci_dev *dev; > > + set_pci_bus_resources_arch_default(b); > pci_read_bridge_bases(b); > list_for_each_entry(dev, &b->devices, bus_list) > pcibios_fixup_device_resources(dev); > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c > index 8eb50df..e3c3e08 100644 > --- a/drivers/pci/probe.c > +++ b/drivers/pci/probe.c > @@ -1118,10 +1118,6 @@ unsigned int __devinit pci_scan_child_bus(struct pci_bus *bus) > return max; > } > > -void __attribute__((weak)) set_pci_bus_resources_arch_default(struct pci_bus *b) > -{ > -} > - > struct pci_bus * pci_create_bus(struct device *parent, > int bus, struct pci_ops *ops, void *sysdata) > { > @@ -1180,8 +1176,6 @@ struct pci_bus * pci_create_bus(struct device *parent, > b->resource[0] = &ioport_resource; > b->resource[1] = &iomem_resource; > > - set_pci_bus_resources_arch_default(b); > - > return b; > > dev_create_file_err: > > Nacked-by: Yinghai Lu <yinghai@kernel.org> we should not move that calling from pci_create_bus to pci_scan_child_bus/pcibios_fixup_bus becuase that calling will be checked for all child buses. -- 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 Fri, Apr 17, 2009 at 02:25:13PM -0700, Yinghai Lu wrote: > pci_create_bus is only for root bus, and pci_scan_child_bus is called on > that root bus. > > if we moved x86_pci_root_bus_res_quirks calling in pci_scan_child_bus, > it will be checked with all child buses. > > so we'd better to keep that calling in pci_create_bus. But set_pci_bus_resources_arch_default() already checks that it's being called for a root bus: for (i = 0; i < pci_root_num; i++) { if (pci_root_info[i].bus_min == b->number) break; } if (i == pci_root_num) return;
Matthew Wilcox wrote: > On Fri, Apr 17, 2009 at 02:25:13PM -0700, Yinghai Lu wrote: > >> pci_create_bus is only for root bus, and pci_scan_child_bus is called on >> that root bus. >> >> if we moved x86_pci_root_bus_res_quirks calling in pci_scan_child_bus, >> it will be checked with all child buses. >> >> so we'd better to keep that calling in pci_create_bus. >> > > But set_pci_bus_resources_arch_default() already checks that it's being > called for a root bus: > > for (i = 0; i < pci_root_num; i++) { > if (pci_root_info[i].bus_min == b->number) > break; > } > > if (i == pci_root_num) > return; > > > We should have a better name for pci_create_bus and set_pci_bus_resources_arch_default like pci_create_root_bus and set_root_bus_res_arch. YH -- 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 Fri, Apr 17, 2009 at 02:45:55PM -0700, Yinghai Lu wrote: > Matthew Wilcox wrote: > > On Fri, Apr 17, 2009 at 02:25:13PM -0700, Yinghai Lu wrote: > >> if we moved x86_pci_root_bus_res_quirks calling in pci_scan_child_bus, > >> it will be checked with all child buses. > >> > >> so we'd better to keep that calling in pci_create_bus. > > > > But set_pci_bus_resources_arch_default() already checks that it's being > > called for a root bus: > > We should have a better name for pci_create_bus and > set_pci_bus_resources_arch_default > like pci_create_root_bus and set_root_bus_res_arch. The naming is orthogonal to this patch. Please withdraw your nacked-by.
Matthew Wilcox wrote: > On Fri, Apr 17, 2009 at 02:45:55PM -0700, Yinghai Lu wrote: > >> Matthew Wilcox wrote: >> >>> On Fri, Apr 17, 2009 at 02:25:13PM -0700, Yinghai Lu wrote: >>> >>>> if we moved x86_pci_root_bus_res_quirks calling in pci_scan_child_bus, >>>> it will be checked with all child buses. >>>> >>>> so we'd better to keep that calling in pci_create_bus. >>>> >>> But set_pci_bus_resources_arch_default() already checks that it's being >>> called for a root bus: >>> >> We should have a better name for pci_create_bus and >> set_pci_bus_resources_arch_default >> like pci_create_root_bus and set_root_bus_res_arch. >> > > The naming is orthogonal to this patch. Please withdraw your nacked-by. > > 1. put the calling in pci_create_bus, archs other than x86 will have blank weak function 2. if put that calling in pci_scan_child_bus, will make every child bus do that checking on x86 platform. other arch does not gain anything in final result on x86 it will keep on calling set_pci_bus_resources_arch_default for every bus in addition to root bus. because pci_scan_child_bus is called for all buses, and pci_create_bus is only for root bus. YH -- 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 Sat, Apr 18, 2009 at 12:56:34AM -0700, Yinghai Lu wrote: > 1. put the calling in pci_create_bus, archs other than x86 will have > blank weak function > 2. if put that calling in pci_scan_child_bus, will make every child bus > do that checking on x86 platform. > > other arch does not gain anything in final result Having x86 do its PCI fixups somewhere different from every other arch is confusing. I know this because it confused me. This isn't about saving a couple of hundred cycles at boot time, it's about maintainability. > on x86 it will keep on calling set_pci_bus_resources_arch_default for every bus in addition to root bus. > because pci_scan_child_bus is called for all buses, and pci_create_bus is only for root bus. > > YH
On Thu, 16 Apr 2009 13:31:10 -0600 Matthew Wilcox <matthew@wil.cx> wrote: > > Commit 30a18d6c3f1e774de656ebd8ff219d53e2ba4029 introduced a new > function to set the PCI bus resources. Unfortunately, neither the > author, nor the committers seemed to know that we already have > somewhere to do that -- pcibios_fixup_bus(). This patch moves the > hook (used only by the K8 code) into x86-specific code where it > should have been in the first place. > > Signed-off-by: Matthew Wilcox <willy@linux.intel.com> > Cc: Yinghai Lu <yinghai.lu@sun.com> > Cc: Ingo Molnar <mingo@elte.hu> > Cc: Thomas Gleixner <tglx@linutronix.de> Applied this and Yinghai's patch on top. Thanks.
diff --git a/arch/x86/pci/common.c b/arch/x86/pci/common.c index 8c362b9..f80ece5 100644 --- a/arch/x86/pci/common.c +++ b/arch/x86/pci/common.c @@ -142,15 +142,20 @@ static void __devinit pcibios_fixup_device_resources(struct pci_dev *dev) } } +void __attribute__((weak)) set_pci_bus_resources_arch_default(struct pci_bus *b) +{ +} + /* * Called after each bus is probed, but before its children * are examined. */ -void __devinit pcibios_fixup_bus(struct pci_bus *b) +void __devinit pcibios_fixup_bus(struct pci_bus *b) { struct pci_dev *dev; + set_pci_bus_resources_arch_default(b); pci_read_bridge_bases(b); list_for_each_entry(dev, &b->devices, bus_list) pcibios_fixup_device_resources(dev); diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c index 8eb50df..e3c3e08 100644 --- a/drivers/pci/probe.c +++ b/drivers/pci/probe.c @@ -1118,10 +1118,6 @@ unsigned int __devinit pci_scan_child_bus(struct pci_bus *bus) return max; } -void __attribute__((weak)) set_pci_bus_resources_arch_default(struct pci_bus *b) -{ -} - struct pci_bus * pci_create_bus(struct device *parent, int bus, struct pci_ops *ops, void *sysdata) { @@ -1180,8 +1176,6 @@ struct pci_bus * pci_create_bus(struct device *parent, b->resource[0] = &ioport_resource; b->resource[1] = &iomem_resource; - set_pci_bus_resources_arch_default(b); - return b; dev_create_file_err:
Commit 30a18d6c3f1e774de656ebd8ff219d53e2ba4029 introduced a new function to set the PCI bus resources. Unfortunately, neither the author, nor the committers seemed to know that we already have somewhere to do that -- pcibios_fixup_bus(). This patch moves the hook (used only by the K8 code) into x86-specific code where it should have been in the first place. Signed-off-by: Matthew Wilcox <willy@linux.intel.com> Cc: Yinghai Lu <yinghai.lu@sun.com> Cc: Ingo Molnar <mingo@elte.hu> Cc: Thomas Gleixner <tglx@linutronix.de>