| Message ID | 1416219710-26088-2-git-send-email-wangyijing@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
On Monday 17 November 2014 18:21:35 Yijing Wang wrote: > - list_for_each_entry(window, resources, list) > - if (window->res->flags & IORESOURCE_BUS) { > - found = true; > - break; > - } > + if (!resources) { > + pci_add_resource(&default_res, &ioport_resource); > + pci_add_resource(&default_res, &iomem_resource); > + pci_add_resource(&default_res, &busn_resource); > + } else { > Isn't it almost always wrong to do this? You are adding all of the I/O ports and memory to the host bridge, which will prevent you from adding another host bridge, and the iomem_resource normally includes a lot of addresses that are not accessible by the PCI host. Arnd -- 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 2014/11/17 18:08, Arnd Bergmann wrote: > On Monday 17 November 2014 18:21:35 Yijing Wang wrote: >> - list_for_each_entry(window, resources, list) >> - if (window->res->flags & IORESOURCE_BUS) { >> - found = true; >> - break; >> - } >> + if (!resources) { >> + pci_add_resource(&default_res, &ioport_resource); >> + pci_add_resource(&default_res, &iomem_resource); >> + pci_add_resource(&default_res, &busn_resource); >> + } else { >> > > Isn't it almost always wrong to do this? You are adding all of the > I/O ports and memory to the host bridge, which will prevent you from > adding another host bridge, and the iomem_resource normally > includes a lot of addresses that are not accessible by the PCI host. Hi Arnd, pci host bridge windows are the ranges allow child devices to setup from. Add all of IO/MEM here just a limit to child devices, no request for these resources, so it won't hurt another host bridge. Some platforms have no dts or ACPI report host bridge resources, in this case, we directly assign ioport/iomem_resources as the root resources of PCI devices. Thanks! Yijing. > > Arnd > > . >
On Tuesday 18 November 2014 15:44:23 Yijing Wang wrote: > On 2014/11/17 18:08, Arnd Bergmann wrote: > > On Monday 17 November 2014 18:21:35 Yijing Wang wrote: > >> - list_for_each_entry(window, resources, list) > >> - if (window->res->flags & IORESOURCE_BUS) { > >> - found = true; > >> - break; > >> - } > >> + if (!resources) { > >> + pci_add_resource(&default_res, &ioport_resource); > >> + pci_add_resource(&default_res, &iomem_resource); > >> + pci_add_resource(&default_res, &busn_resource); > >> + } else { > >> > > > > Isn't it almost always wrong to do this? You are adding all of the > > I/O ports and memory to the host bridge, which will prevent you from > > adding another host bridge, and the iomem_resource normally > > includes a lot of addresses that are not accessible by the PCI host. > > Hi Arnd, pci host bridge windows are the ranges allow child devices to setup > from. Add all of IO/MEM here just a limit to child devices, no request for these > resources, so it won't hurt another host bridge. Some platforms have no dts or ACPI > report host bridge resources, in this case, we directly assign ioport/iomem_resources > as the root resources of PCI devices. But it would be wrong to allow hosts to allocate a device BAR that is not visible through the host bridge. I think we need to keep these separate from the general case: if you call any of the modern interfaces you have to provide the resources and a device. I notice that there is only one caller of pci_scan_bus_parented(), we should probably change that over to pci_scan_root_bus() or your new interface and remove the old one, but keep pci_scan_bus() as the only entry point for all of the legacy users that do not know about the resources. Arnd -- 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 2014/11/18 17:36, Arnd Bergmann wrote: > On Tuesday 18 November 2014 15:44:23 Yijing Wang wrote: >> On 2014/11/17 18:08, Arnd Bergmann wrote: >>> On Monday 17 November 2014 18:21:35 Yijing Wang wrote: >>>> - list_for_each_entry(window, resources, list) >>>> - if (window->res->flags & IORESOURCE_BUS) { >>>> - found = true; >>>> - break; >>>> - } >>>> + if (!resources) { >>>> + pci_add_resource(&default_res, &ioport_resource); >>>> + pci_add_resource(&default_res, &iomem_resource); >>>> + pci_add_resource(&default_res, &busn_resource); >>>> + } else { >>>> >>> >>> Isn't it almost always wrong to do this? You are adding all of the >>> I/O ports and memory to the host bridge, which will prevent you from >>> adding another host bridge, and the iomem_resource normally >>> includes a lot of addresses that are not accessible by the PCI host. >> >> Hi Arnd, pci host bridge windows are the ranges allow child devices to setup >> from. Add all of IO/MEM here just a limit to child devices, no request for these >> resources, so it won't hurt another host bridge. Some platforms have no dts or ACPI >> report host bridge resources, in this case, we directly assign ioport/iomem_resources >> as the root resources of PCI devices. > > But it would be wrong to allow hosts to allocate a device BAR that is not > visible through the host bridge. I think we need to keep these separate > from the general case: if you call any of the modern interfaces you have > to provide the resources and a device. I notice that there is only one > caller of pci_scan_bus_parented(), we should probably change that over to > pci_scan_root_bus() or your new interface and remove the old one, but > keep pci_scan_bus() as the only entry point for all of the legacy users > that do not know about the resources. Ok, I will move this out of the generic interface. Thanks! Yijing. > > Arnd > > . >
On Tue, Nov 18, 2014 at 11:46:06AM +0000, Yijing Wang wrote: > On 2014/11/18 17:36, Arnd Bergmann wrote: > > On Tuesday 18 November 2014 15:44:23 Yijing Wang wrote: > >> On 2014/11/17 18:08, Arnd Bergmann wrote: > >>> On Monday 17 November 2014 18:21:35 Yijing Wang wrote: > >>>> - list_for_each_entry(window, resources, list) > >>>> - if (window->res->flags & IORESOURCE_BUS) { > >>>> - found = true; > >>>> - break; > >>>> - } > >>>> + if (!resources) { > >>>> + pci_add_resource(&default_res, &ioport_resource); > >>>> + pci_add_resource(&default_res, &iomem_resource); > >>>> + pci_add_resource(&default_res, &busn_resource); > >>>> + } else { > >>>> > >>> > >>> Isn't it almost always wrong to do this? You are adding all of the > >>> I/O ports and memory to the host bridge, which will prevent you from > >>> adding another host bridge, and the iomem_resource normally > >>> includes a lot of addresses that are not accessible by the PCI host. > >> > >> Hi Arnd, pci host bridge windows are the ranges allow child devices to setup > >> from. Add all of IO/MEM here just a limit to child devices, no request for these > >> resources, so it won't hurt another host bridge. Some platforms have no dts or ACPI > >> report host bridge resources, in this case, we directly assign ioport/iomem_resources > >> as the root resources of PCI devices. > > > > But it would be wrong to allow hosts to allocate a device BAR that is not > > visible through the host bridge. I think we need to keep these separate > > from the general case: if you call any of the modern interfaces you have > > to provide the resources and a device. I notice that there is only one > > caller of pci_scan_bus_parented(), we should probably change that over to > > pci_scan_root_bus() or your new interface and remove the old one, but > > keep pci_scan_bus() as the only entry point for all of the legacy users > > that do not know about the resources. > > Ok, I will move this out of the generic interface. My suggestion would actually be to trigger a warning/error if you detect that the resources are missing. That way we can force the drivers to clean up. Best regards, Liviu > > Thanks! > Yijing. > > > > > Arnd > > > > . > > > > > -- > Thanks! > Yijing > > -- > 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 2014/11/18 22:23, Liviu Dudau wrote: > On Tue, Nov 18, 2014 at 11:46:06AM +0000, Yijing Wang wrote: >> On 2014/11/18 17:36, Arnd Bergmann wrote: >>> On Tuesday 18 November 2014 15:44:23 Yijing Wang wrote: >>>> On 2014/11/17 18:08, Arnd Bergmann wrote: >>>>> On Monday 17 November 2014 18:21:35 Yijing Wang wrote: >>>>>> - list_for_each_entry(window, resources, list) >>>>>> - if (window->res->flags & IORESOURCE_BUS) { >>>>>> - found = true; >>>>>> - break; >>>>>> - } >>>>>> + if (!resources) { >>>>>> + pci_add_resource(&default_res, &ioport_resource); >>>>>> + pci_add_resource(&default_res, &iomem_resource); >>>>>> + pci_add_resource(&default_res, &busn_resource); >>>>>> + } else { >>>>>> >>>>> >>>>> Isn't it almost always wrong to do this? You are adding all of the >>>>> I/O ports and memory to the host bridge, which will prevent you from >>>>> adding another host bridge, and the iomem_resource normally >>>>> includes a lot of addresses that are not accessible by the PCI host. >>>> >>>> Hi Arnd, pci host bridge windows are the ranges allow child devices to setup >>>> from. Add all of IO/MEM here just a limit to child devices, no request for these >>>> resources, so it won't hurt another host bridge. Some platforms have no dts or ACPI >>>> report host bridge resources, in this case, we directly assign ioport/iomem_resources >>>> as the root resources of PCI devices. >>> >>> But it would be wrong to allow hosts to allocate a device BAR that is not >>> visible through the host bridge. I think we need to keep these separate >>> from the general case: if you call any of the modern interfaces you have >>> to provide the resources and a device. I notice that there is only one >>> caller of pci_scan_bus_parented(), we should probably change that over to >>> pci_scan_root_bus() or your new interface and remove the old one, but >>> keep pci_scan_bus() as the only entry point for all of the legacy users >>> that do not know about the resources. >> >> Ok, I will move this out of the generic interface. > > My suggestion would actually be to trigger a warning/error if you detect that the resources > are missing. That way we can force the drivers to clean up. It make sense to me, thanks. > > Best regards, > Liviu > >> >> Thanks! >> Yijing. >> >>> >>> Arnd >>> >>> . >>> >> >> >> -- >> Thanks! >> Yijing >> >> -- >> 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 >> >
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c index 5ed9930..fc99e88 100644 --- a/drivers/pci/probe.c +++ b/drivers/pci/probe.c @@ -2069,15 +2069,23 @@ struct pci_bus *pci_scan_root_bus(struct device *parent, int bus, struct pci_host_bridge_window *window; bool found = false; struct pci_bus *b; + LIST_HEAD(default_res); int max; - list_for_each_entry(window, resources, list) - if (window->res->flags & IORESOURCE_BUS) { - found = true; - break; - } + if (!resources) { + pci_add_resource(&default_res, &ioport_resource); + pci_add_resource(&default_res, &iomem_resource); + pci_add_resource(&default_res, &busn_resource); + } else { + list_for_each_entry(window, resources, list) + if (window->res->flags & IORESOURCE_BUS) { + found = true; + break; + } + } - b = pci_create_root_bus(parent, bus, ops, sysdata, resources); + b = pci_create_root_bus(parent, bus, ops, sysdata, + resources ? resources : &default_res); if (!b) return NULL;