| Message ID | 1348733519-24684-5-git-send-email-yinghai@kernel.org |
|---|---|
| State | Accepted |
| Headers | show |
On Thu, Sep 27, 2012 at 2:11 AM, Yinghai Lu <yinghai@kernel.org> wrote: > After we get hot-added ioapic registered. > pci_enable_bridges will try to enable ioapic irq for pci bridges. What makes this specifically related to IOAPICs? > Signed-off-by: Yinghai Lu <yinghai@kernel.org> > --- > drivers/acpi/pci_root.c | 7 +++++++ > 1 files changed, 7 insertions(+), 0 deletions(-) > > diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c > index bce469c..27adbfd 100644 > --- a/drivers/acpi/pci_root.c > +++ b/drivers/acpi/pci_root.c > @@ -644,12 +644,19 @@ static int acpi_pci_root_start(struct acpi_device *device) > struct acpi_pci_root *root = acpi_driver_data(device); > struct acpi_pci_driver *driver; > > + if (system_state != SYSTEM_BOOTING) > + pci_assign_unassigned_bus_resources(root->bus); > + > mutex_lock(&acpi_pci_root_lock); > list_for_each_entry(driver, &acpi_pci_drivers, node) > if (driver->add) > driver->add(root); > mutex_unlock(&acpi_pci_root_lock); > > + /* need to after hot-added ioapic is registered */ > + if (system_state != SYSTEM_BOOTING) > + pci_enable_bridges(root->bus); Theoretically, we should be able to assign resources and enable bridges here regardless of the system_state. I think the reason you don't want to do it while SYSTEM_BOOTING is because we currently do this at boot-time: acpi_pci_root_add pci_scan_child_bus acpi_pci_root_start pci_bus_add_devices <account for some motherboard (PNP0C01) resources> pcibios_assign_resources # fs_initcall pci_assign_unassigned_resources pci_enable_bridges and without the SYSTEM_BOOTING check, we might assign PCI resources at boot-time that conflict with motherboard resources. Is that right? This is completely non-obvious and future readers deserve a hint about what's going on here. The right way to do this would be to pay attention to the host bridge apertures, and assign resources from within the apertures. Then we could always assign PCI resources when adding a host bridge instead of in an fs_initcall. I understand we still have legacy issues and machines where we still don't pay attention to _CRS. But if we're doing things to work around a broken design, it's important to be aware of how the design is broken so we have some hope of eventually fixing the design. > pci_bus_add_devices(root->bus); > > return 0; > -- > 1.7.7 > -- 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, Sep 28, 2012 at 4:46 PM, Bjorn Helgaas <bhelgaas@google.com> wrote: > On Thu, Sep 27, 2012 at 2:11 AM, Yinghai Lu <yinghai@kernel.org> wrote: >> After we get hot-added ioapic registered. >> pci_enable_bridges will try to enable ioapic irq for pci bridges. > > What makes this specifically related to IOAPICs? > >> Signed-off-by: Yinghai Lu <yinghai@kernel.org> >> --- >> drivers/acpi/pci_root.c | 7 +++++++ >> 1 files changed, 7 insertions(+), 0 deletions(-) >> >> diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c >> index bce469c..27adbfd 100644 >> --- a/drivers/acpi/pci_root.c >> +++ b/drivers/acpi/pci_root.c >> @@ -644,12 +644,19 @@ static int acpi_pci_root_start(struct acpi_device *device) >> struct acpi_pci_root *root = acpi_driver_data(device); >> struct acpi_pci_driver *driver; >> >> + if (system_state != SYSTEM_BOOTING) >> + pci_assign_unassigned_bus_resources(root->bus); >> + >> mutex_lock(&acpi_pci_root_lock); >> list_for_each_entry(driver, &acpi_pci_drivers, node) >> if (driver->add) >> driver->add(root); >> mutex_unlock(&acpi_pci_root_lock); >> >> + /* need to after hot-added ioapic is registered */ >> + if (system_state != SYSTEM_BOOTING) >> + pci_enable_bridges(root->bus); > > Theoretically, we should be able to assign resources and enable > bridges here regardless of the system_state. > > I think the reason you don't want to do it while SYSTEM_BOOTING is > because we currently do this at boot-time: > > acpi_pci_root_add > pci_scan_child_bus > acpi_pci_root_start > pci_bus_add_devices > <account for some motherboard (PNP0C01) resources> > pcibios_assign_resources # fs_initcall > pci_assign_unassigned_resources > pci_enable_bridges > > and without the SYSTEM_BOOTING check, we might assign PCI resources at > boot-time that conflict with motherboard resources. > > Is that right? yes. > > This is completely non-obvious and future readers deserve a hint about > what's going on here. > > The right way to do this would be to pay attention to the host bridge > apertures, and assign resources from within the apertures. Then we > could always assign PCI resources when adding a host bridge instead of > in an fs_initcall. I understand we still have legacy issues and > machines where we still don't pay attention to _CRS. But if we're > doing things to work around a broken design, it's important to be > aware of how the design is broken so we have some hope of eventually > fixing the design. that could be another big topic. -- 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, Sep 28, 2012 at 7:56 PM, Yinghai Lu <yinghai@kernel.org> wrote: > On Fri, Sep 28, 2012 at 4:46 PM, Bjorn Helgaas <bhelgaas@google.com> wrote: >> On Thu, Sep 27, 2012 at 2:11 AM, Yinghai Lu <yinghai@kernel.org> wrote: >>> After we get hot-added ioapic registered. >>> pci_enable_bridges will try to enable ioapic irq for pci bridges. >> >> What makes this specifically related to IOAPICs? I think you missed this question. >>> Signed-off-by: Yinghai Lu <yinghai@kernel.org> >>> --- >>> drivers/acpi/pci_root.c | 7 +++++++ >>> 1 files changed, 7 insertions(+), 0 deletions(-) >>> >>> diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c >>> index bce469c..27adbfd 100644 >>> --- a/drivers/acpi/pci_root.c >>> +++ b/drivers/acpi/pci_root.c >>> @@ -644,12 +644,19 @@ static int acpi_pci_root_start(struct acpi_device *device) >>> struct acpi_pci_root *root = acpi_driver_data(device); >>> struct acpi_pci_driver *driver; >>> >>> + if (system_state != SYSTEM_BOOTING) >>> + pci_assign_unassigned_bus_resources(root->bus); >>> + >>> mutex_lock(&acpi_pci_root_lock); >>> list_for_each_entry(driver, &acpi_pci_drivers, node) >>> if (driver->add) >>> driver->add(root); >>> mutex_unlock(&acpi_pci_root_lock); >>> >>> + /* need to after hot-added ioapic is registered */ >>> + if (system_state != SYSTEM_BOOTING) >>> + pci_enable_bridges(root->bus); >> >> Theoretically, we should be able to assign resources and enable >> bridges here regardless of the system_state. >> >> I think the reason you don't want to do it while SYSTEM_BOOTING is >> because we currently do this at boot-time: >> >> acpi_pci_root_add >> pci_scan_child_bus >> acpi_pci_root_start >> pci_bus_add_devices >> <account for some motherboard (PNP0C01) resources> >> pcibios_assign_resources # fs_initcall >> pci_assign_unassigned_resources >> pci_enable_bridges >> >> and without the SYSTEM_BOOTING check, we might assign PCI resources at >> boot-time that conflict with motherboard resources. >> >> Is that right? > > yes. > >> >> This is completely non-obvious and future readers deserve a hint about >> what's going on here. >> >> The right way to do this would be to pay attention to the host bridge >> apertures, and assign resources from within the apertures. Then we >> could always assign PCI resources when adding a host bridge instead of >> in an fs_initcall. I understand we still have legacy issues and >> machines where we still don't pay attention to _CRS. But if we're >> doing things to work around a broken design, it's important to be >> aware of how the design is broken so we have some hope of eventually >> fixing the design. > > that could be another big topic. I agree, and I'm not asking you to fix that. I'm just trying to understand what's going on and write some comments and changelogs that will help make this maintainable in the future. Also note the question at the top -- your changelog calls out IOAPICs, but I'm still not sure what this patch has to do with IOAPICs. -- 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 2012-9-29 11:31, Bjorn Helgaas wrote: > On Fri, Sep 28, 2012 at 7:56 PM, Yinghai Lu <yinghai@kernel.org> wrote: >> On Fri, Sep 28, 2012 at 4:46 PM, Bjorn Helgaas <bhelgaas@google.com> wrote: >>> On Thu, Sep 27, 2012 at 2:11 AM, Yinghai Lu <yinghai@kernel.org> wrote: >>>> After we get hot-added ioapic registered. >>>> pci_enable_bridges will try to enable ioapic irq for pci bridges. >>> >>> What makes this specifically related to IOAPICs? > > I think you missed this question. > >>>> Signed-off-by: Yinghai Lu <yinghai@kernel.org> >>>> --- >>>> drivers/acpi/pci_root.c | 7 +++++++ >>>> 1 files changed, 7 insertions(+), 0 deletions(-) >>>> >>>> diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c >>>> index bce469c..27adbfd 100644 >>>> --- a/drivers/acpi/pci_root.c >>>> +++ b/drivers/acpi/pci_root.c >>>> @@ -644,12 +644,19 @@ static int acpi_pci_root_start(struct acpi_device *device) >>>> struct acpi_pci_root *root = acpi_driver_data(device); >>>> struct acpi_pci_driver *driver; >>>> >>>> + if (system_state != SYSTEM_BOOTING) >>>> + pci_assign_unassigned_bus_resources(root->bus); >>>> + >>>> mutex_lock(&acpi_pci_root_lock); >>>> list_for_each_entry(driver, &acpi_pci_drivers, node) >>>> if (driver->add) >>>> driver->add(root); >>>> mutex_unlock(&acpi_pci_root_lock); >>>> >>>> + /* need to after hot-added ioapic is registered */ >>>> + if (system_state != SYSTEM_BOOTING) >>>> + pci_enable_bridges(root->bus); >>> >>> Theoretically, we should be able to assign resources and enable >>> bridges here regardless of the system_state. >>> >>> I think the reason you don't want to do it while SYSTEM_BOOTING is >>> because we currently do this at boot-time: >>> >>> acpi_pci_root_add >>> pci_scan_child_bus >>> acpi_pci_root_start >>> pci_bus_add_devices >>> <account for some motherboard (PNP0C01) resources> >>> pcibios_assign_resources # fs_initcall >>> pci_assign_unassigned_resources >>> pci_enable_bridges >>> >>> and without the SYSTEM_BOOTING check, we might assign PCI resources at >>> boot-time that conflict with motherboard resources. >>> >>> Is that right? >> >> yes. >> >>> >>> This is completely non-obvious and future readers deserve a hint about >>> what's going on here. >>> >>> The right way to do this would be to pay attention to the host bridge >>> apertures, and assign resources from within the apertures. Then we >>> could always assign PCI resources when adding a host bridge instead of >>> in an fs_initcall. I understand we still have legacy issues and >>> machines where we still don't pay attention to _CRS. But if we're >>> doing things to work around a broken design, it's important to be >>> aware of how the design is broken so we have some hope of eventually >>> fixing the design. >> >> that could be another big topic. > > I agree, and I'm not asking you to fix that. I'm just trying to > understand what's going on and write some comments and changelogs that > will help make this maintainable in the future. > > Also note the question at the top -- your changelog calls out IOAPICs, > but I'm still not sure what this patch has to do with IOAPICs. Hi Bjorn， Another weay is to add an ACPI driver for IOAPIC, so we can control the order between IOAPIC and PCI host bridge. Thanks! Gerry -- 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, Sep 28, 2012 at 8:31 PM, Bjorn Helgaas <bhelgaas@google.com> wrote: > On Fri, Sep 28, 2012 at 7:56 PM, Yinghai Lu <yinghai@kernel.org> wrote: >> On Fri, Sep 28, 2012 at 4:46 PM, Bjorn Helgaas <bhelgaas@google.com> wrote: >>> On Thu, Sep 27, 2012 at 2:11 AM, Yinghai Lu <yinghai@kernel.org> wrote: >>>> After we get hot-added ioapic registered. >>>> pci_enable_bridges will try to enable ioapic irq for pci bridges. >>> >>> What makes this specifically related to IOAPICs? > > I think you missed this question. in another response: pci_enable_bridges ==> pci_enable_device ==> ... ===> pcibios_enable_device ===> pcibios_enable_irq and one of driver in acpi_pci_drivers, is ioapic drivers and it will enable ioapic there. So we need to enable bridges that later. -- 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, Sep 28, 2012 at 8:37 PM, Jiang Liu <jiang.liu@huawei.com> wrote: > On 2012-9-29 11:31, Bjorn Helgaas wrote: >> I agree, and I'm not asking you to fix that. I'm just trying to >> understand what's going on and write some comments and changelogs that >> will help make this maintainable in the future. >> >> Also note the question at the top -- your changelog calls out IOAPICs, >> but I'm still not sure what this patch has to do with IOAPICs. > Hi Bjorn， > Another weay is to add an ACPI driver for IOAPIC, so we can control > the order between IOAPIC and PCI host bridge. use acpi_driver instead of acpi_pci_driver ? -- 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/acpi/pci_root.c b/drivers/acpi/pci_root.c index bce469c..27adbfd 100644 --- a/drivers/acpi/pci_root.c +++ b/drivers/acpi/pci_root.c @@ -644,12 +644,19 @@ static int acpi_pci_root_start(struct acpi_device *device) struct acpi_pci_root *root = acpi_driver_data(device); struct acpi_pci_driver *driver; + if (system_state != SYSTEM_BOOTING) + pci_assign_unassigned_bus_resources(root->bus); + mutex_lock(&acpi_pci_root_lock); list_for_each_entry(driver, &acpi_pci_drivers, node) if (driver->add) driver->add(root); mutex_unlock(&acpi_pci_root_lock); + /* need to after hot-added ioapic is registered */ + if (system_state != SYSTEM_BOOTING) + pci_enable_bridges(root->bus); + pci_bus_add_devices(root->bus); return 0;
After we get hot-added ioapic registered. pci_enable_bridges will try to enable ioapic irq for pci bridges. Signed-off-by: Yinghai Lu <yinghai@kernel.org> --- drivers/acpi/pci_root.c | 7 +++++++ 1 files changed, 7 insertions(+), 0 deletions(-)