| Message ID | f232553753fe148118646c9013a3d962c19b58ec.1550121250.git.sbobroff@linux.ibm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/1] powerpc/64: Adjust order in pcibios_init() | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | success | next/apply_patch Successfully applied |
| snowpatch_ozlabs/build-ppc64le | success | build succeeded & removed 0 sparse warning(s) |
| snowpatch_ozlabs/build-ppc64be | success | build succeeded & removed 0 sparse warning(s) |
| snowpatch_ozlabs/build-ppc64e | success | build succeeded & removed 0 sparse warning(s) |
| snowpatch_ozlabs/build-pmac32 | success | build succeeded & removed 0 sparse warning(s) |
| snowpatch_ozlabs/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 19 lines checked |
On Thu, Feb 14, 2019 at 4:14 PM Sam Bobroff <sbobroff@linux.ibm.com> wrote: > > The pcibios_init() function for 64 bit PowerPC currently calls > pci_bus_add_devices() before pcibios_resource_survey(), which seems > incorrect because it adds devices and attempts to bind their drivers > before allocating their resources (although no problems seem to be > apparent). > > So move the call to pci_bus_add_devices() to after > pcibios_resource_survey(). > > This will also allow the ppc_md.pcibios_bus_add_device() hooks to > perform actions that depend on PCI resources, both during rescanning > (where this is already the case) and at boot time, which should > support improvements and refactoring. > > Signed-off-by: Sam Bobroff <sbobroff@linux.ibm.com> Reviewed-by: Oliver O'Halloran <oohall@gmail.com> > --- > Hi everyone, > > I've tested this on a P9 for both the host and a KVM guest, and the change > hasn't caused any differences in PCI resource assignments or the general boot > messages. > > I've also had a go at inspecting most of the code used by pci_bus_add_devices() > and pcibios_resource_survey() and it doesn't look like there are going to be > any changes in behaviour caused by reordering. It might be worth mentioning > that the hotplug path (see pcibios_finish_adding_to_bus()) already does > resource allocation before calling pci_bus_add_devices(). > > However, it would be great if someone could test this change on some older > hardware or comment on wether we should make the same change on 32 bit machines. > > Cheers, > Sam. > > arch/powerpc/kernel/pci_64.c | 8 +++++--- > 1 file changed, 5 insertions(+), 3 deletions(-) > > diff --git a/arch/powerpc/kernel/pci_64.c b/arch/powerpc/kernel/pci_64.c > index 9d8c10d55407..1ce28888dbdb 100644 > --- a/arch/powerpc/kernel/pci_64.c > +++ b/arch/powerpc/kernel/pci_64.c > @@ -58,14 +58,16 @@ static int __init pcibios_init(void) > pci_add_flags(PCI_ENABLE_PROC_DOMAINS | PCI_COMPAT_DOMAIN_0); > > /* Scan all of the recorded PCI controllers. */ > - list_for_each_entry_safe(hose, tmp, &hose_list, list_node) { > + list_for_each_entry_safe(hose, tmp, &hose_list, list_node) > pcibios_scan_phb(hose); > - pci_bus_add_devices(hose->bus); > - } > > /* Call common code to handle resource allocation */ > pcibios_resource_survey(); > > + /* Add devices. */ > + list_for_each_entry_safe(hose, tmp, &hose_list, list_node) > + pci_bus_add_devices(hose->bus); > + > printk(KERN_DEBUG "PCI: Probing PCI hardware done\n"); > > return 0; > -- > 2.19.0.2.gcad72f5712 >
Hey all, After some consideration, I've decided to post a v2 of this patch that will make it a bit safer (although I haven't seen any problems with it) and make it a little easier to refactor some of the EEH code that interacts with the hooks. Cheers, Sam. On Thu, Feb 14, 2019 at 04:14:42PM +1100, Sam Bobroff wrote: > The pcibios_init() function for 64 bit PowerPC currently calls > pci_bus_add_devices() before pcibios_resource_survey(), which seems > incorrect because it adds devices and attempts to bind their drivers > before allocating their resources (although no problems seem to be > apparent). > > So move the call to pci_bus_add_devices() to after > pcibios_resource_survey(). > > This will also allow the ppc_md.pcibios_bus_add_device() hooks to > perform actions that depend on PCI resources, both during rescanning > (where this is already the case) and at boot time, which should > support improvements and refactoring. > > Signed-off-by: Sam Bobroff <sbobroff@linux.ibm.com> > --- > Hi everyone, > > I've tested this on a P9 for both the host and a KVM guest, and the change > hasn't caused any differences in PCI resource assignments or the general boot > messages. > > I've also had a go at inspecting most of the code used by pci_bus_add_devices() > and pcibios_resource_survey() and it doesn't look like there are going to be > any changes in behaviour caused by reordering. It might be worth mentioning > that the hotplug path (see pcibios_finish_adding_to_bus()) already does > resource allocation before calling pci_bus_add_devices(). > > However, it would be great if someone could test this change on some older > hardware or comment on wether we should make the same change on 32 bit machines. > > Cheers, > Sam. > > arch/powerpc/kernel/pci_64.c | 8 +++++--- > 1 file changed, 5 insertions(+), 3 deletions(-) > > diff --git a/arch/powerpc/kernel/pci_64.c b/arch/powerpc/kernel/pci_64.c > index 9d8c10d55407..1ce28888dbdb 100644 > --- a/arch/powerpc/kernel/pci_64.c > +++ b/arch/powerpc/kernel/pci_64.c > @@ -58,14 +58,16 @@ static int __init pcibios_init(void) > pci_add_flags(PCI_ENABLE_PROC_DOMAINS | PCI_COMPAT_DOMAIN_0); > > /* Scan all of the recorded PCI controllers. */ > - list_for_each_entry_safe(hose, tmp, &hose_list, list_node) { > + list_for_each_entry_safe(hose, tmp, &hose_list, list_node) > pcibios_scan_phb(hose); > - pci_bus_add_devices(hose->bus); > - } > > /* Call common code to handle resource allocation */ > pcibios_resource_survey(); > > + /* Add devices. */ > + list_for_each_entry_safe(hose, tmp, &hose_list, list_node) > + pci_bus_add_devices(hose->bus); > + > printk(KERN_DEBUG "PCI: Probing PCI hardware done\n"); > > return 0; > -- > 2.19.0.2.gcad72f5712 >
On Fri, Feb 22, 2019 at 2:24 PM Sam Bobroff <sbobroff@linux.ibm.com> wrote: > > Hey all, > > After some consideration, I've decided to post a v2 of this patch that > will make it a bit safer (although I haven't seen any problems with it) > and make it a little easier to refactor some of the EEH code that > interacts with the hooks. Can you be a little more specific? > Cheers, > Sam. > > On Thu, Feb 14, 2019 at 04:14:42PM +1100, Sam Bobroff wrote: > > The pcibios_init() function for 64 bit PowerPC currently calls > > pci_bus_add_devices() before pcibios_resource_survey(), which seems > > incorrect because it adds devices and attempts to bind their drivers > > before allocating their resources (although no problems seem to be > > apparent). > > > > So move the call to pci_bus_add_devices() to after > > pcibios_resource_survey(). > > > > This will also allow the ppc_md.pcibios_bus_add_device() hooks to > > perform actions that depend on PCI resources, both during rescanning > > (where this is already the case) and at boot time, which should > > support improvements and refactoring. > > > > Signed-off-by: Sam Bobroff <sbobroff@linux.ibm.com> > > --- > > Hi everyone, > > > > I've tested this on a P9 for both the host and a KVM guest, and the change > > hasn't caused any differences in PCI resource assignments or the general boot > > messages. > > > > I've also had a go at inspecting most of the code used by pci_bus_add_devices() > > and pcibios_resource_survey() and it doesn't look like there are going to be > > any changes in behaviour caused by reordering. It might be worth mentioning > > that the hotplug path (see pcibios_finish_adding_to_bus()) already does > > resource allocation before calling pci_bus_add_devices(). > > > > However, it would be great if someone could test this change on some older > > hardware or comment on wether we should make the same change on 32 bit machines. > > > > Cheers, > > Sam. > > > > arch/powerpc/kernel/pci_64.c | 8 +++++--- > > 1 file changed, 5 insertions(+), 3 deletions(-) > > > > diff --git a/arch/powerpc/kernel/pci_64.c b/arch/powerpc/kernel/pci_64.c > > index 9d8c10d55407..1ce28888dbdb 100644 > > --- a/arch/powerpc/kernel/pci_64.c > > +++ b/arch/powerpc/kernel/pci_64.c > > @@ -58,14 +58,16 @@ static int __init pcibios_init(void) > > pci_add_flags(PCI_ENABLE_PROC_DOMAINS | PCI_COMPAT_DOMAIN_0); > > > > /* Scan all of the recorded PCI controllers. */ > > - list_for_each_entry_safe(hose, tmp, &hose_list, list_node) { > > + list_for_each_entry_safe(hose, tmp, &hose_list, list_node) > > pcibios_scan_phb(hose); > > - pci_bus_add_devices(hose->bus); > > - } > > > > /* Call common code to handle resource allocation */ > > pcibios_resource_survey(); > > > > + /* Add devices. */ > > + list_for_each_entry_safe(hose, tmp, &hose_list, list_node) > > + pci_bus_add_devices(hose->bus); > > + > > printk(KERN_DEBUG "PCI: Probing PCI hardware done\n"); > > > > return 0; > > -- > > 2.19.0.2.gcad72f5712 > >
On Fri, Feb 22, 2019 at 03:31:57PM +1100, Oliver wrote: > On Fri, Feb 22, 2019 at 2:24 PM Sam Bobroff <sbobroff@linux.ibm.com> wrote: > > > > Hey all, > > > > After some consideration, I've decided to post a v2 of this patch that > > will make it a bit safer (although I haven't seen any problems with it) > > and make it a little easier to refactor some of the EEH code that > > interacts with the hooks. > > Can you be a little more specific? Sure: When the original patch moves pci_bus_add_devices() to after pcibios_resource_survey(), this also causes the pcibios_fixup hook to move, because it's called at the end of pcibios_resource_survey(). So pcibios_fixup would run before pcibios_bus_add_device and while I don't think that will actually cause problems, there doesn't seem to be any reason to change the order either. So, I think it would be better to extract the pcibios_fixup hook out of pcibios_resource_survey() and call it from pcibios_init after the devices are added, preserving that ordering. That's the general reasoning but more specifically, I want to refactor the EEH code around adding devices, and it seems like I'm going to need the handlers in that order so that pcibios_bus_add_device() can probe for EEH support in devices before the EEH post init code needs to know if any were found. > > Cheers, > > Sam. > > > > On Thu, Feb 14, 2019 at 04:14:42PM +1100, Sam Bobroff wrote: > > > The pcibios_init() function for 64 bit PowerPC currently calls > > > pci_bus_add_devices() before pcibios_resource_survey(), which seems > > > incorrect because it adds devices and attempts to bind their drivers > > > before allocating their resources (although no problems seem to be > > > apparent). > > > > > > So move the call to pci_bus_add_devices() to after > > > pcibios_resource_survey(). > > > > > > This will also allow the ppc_md.pcibios_bus_add_device() hooks to > > > perform actions that depend on PCI resources, both during rescanning > > > (where this is already the case) and at boot time, which should > > > support improvements and refactoring. > > > > > > Signed-off-by: Sam Bobroff <sbobroff@linux.ibm.com> > > > --- > > > Hi everyone, > > > > > > I've tested this on a P9 for both the host and a KVM guest, and the change > > > hasn't caused any differences in PCI resource assignments or the general boot > > > messages. > > > > > > I've also had a go at inspecting most of the code used by pci_bus_add_devices() > > > and pcibios_resource_survey() and it doesn't look like there are going to be > > > any changes in behaviour caused by reordering. It might be worth mentioning > > > that the hotplug path (see pcibios_finish_adding_to_bus()) already does > > > resource allocation before calling pci_bus_add_devices(). > > > > > > However, it would be great if someone could test this change on some older > > > hardware or comment on wether we should make the same change on 32 bit machines. > > > > > > Cheers, > > > Sam. > > > > > > arch/powerpc/kernel/pci_64.c | 8 +++++--- > > > 1 file changed, 5 insertions(+), 3 deletions(-) > > > > > > diff --git a/arch/powerpc/kernel/pci_64.c b/arch/powerpc/kernel/pci_64.c > > > index 9d8c10d55407..1ce28888dbdb 100644 > > > --- a/arch/powerpc/kernel/pci_64.c > > > +++ b/arch/powerpc/kernel/pci_64.c > > > @@ -58,14 +58,16 @@ static int __init pcibios_init(void) > > > pci_add_flags(PCI_ENABLE_PROC_DOMAINS | PCI_COMPAT_DOMAIN_0); > > > > > > /* Scan all of the recorded PCI controllers. */ > > > - list_for_each_entry_safe(hose, tmp, &hose_list, list_node) { > > > + list_for_each_entry_safe(hose, tmp, &hose_list, list_node) > > > pcibios_scan_phb(hose); > > > - pci_bus_add_devices(hose->bus); > > > - } > > > > > > /* Call common code to handle resource allocation */ > > > pcibios_resource_survey(); > > > > > > + /* Add devices. */ > > > + list_for_each_entry_safe(hose, tmp, &hose_list, list_node) > > > + pci_bus_add_devices(hose->bus); > > > + > > > printk(KERN_DEBUG "PCI: Probing PCI hardware done\n"); > > > > > > return 0; > > > -- > > > 2.19.0.2.gcad72f5712 > > > >
diff --git a/arch/powerpc/kernel/pci_64.c b/arch/powerpc/kernel/pci_64.c index 9d8c10d55407..1ce28888dbdb 100644 --- a/arch/powerpc/kernel/pci_64.c +++ b/arch/powerpc/kernel/pci_64.c @@ -58,14 +58,16 @@ static int __init pcibios_init(void) pci_add_flags(PCI_ENABLE_PROC_DOMAINS | PCI_COMPAT_DOMAIN_0); /* Scan all of the recorded PCI controllers. */ - list_for_each_entry_safe(hose, tmp, &hose_list, list_node) { + list_for_each_entry_safe(hose, tmp, &hose_list, list_node) pcibios_scan_phb(hose); - pci_bus_add_devices(hose->bus); - } /* Call common code to handle resource allocation */ pcibios_resource_survey(); + /* Add devices. */ + list_for_each_entry_safe(hose, tmp, &hose_list, list_node) + pci_bus_add_devices(hose->bus); + printk(KERN_DEBUG "PCI: Probing PCI hardware done\n"); return 0;
The pcibios_init() function for 64 bit PowerPC currently calls pci_bus_add_devices() before pcibios_resource_survey(), which seems incorrect because it adds devices and attempts to bind their drivers before allocating their resources (although no problems seem to be apparent). So move the call to pci_bus_add_devices() to after pcibios_resource_survey(). This will also allow the ppc_md.pcibios_bus_add_device() hooks to perform actions that depend on PCI resources, both during rescanning (where this is already the case) and at boot time, which should support improvements and refactoring. Signed-off-by: Sam Bobroff <sbobroff@linux.ibm.com> --- Hi everyone, I've tested this on a P9 for both the host and a KVM guest, and the change hasn't caused any differences in PCI resource assignments or the general boot messages. I've also had a go at inspecting most of the code used by pci_bus_add_devices() and pcibios_resource_survey() and it doesn't look like there are going to be any changes in behaviour caused by reordering. It might be worth mentioning that the hotplug path (see pcibios_finish_adding_to_bus()) already does resource allocation before calling pci_bus_add_devices(). However, it would be great if someone could test this change on some older hardware or comment on wether we should make the same change on 32 bit machines. Cheers, Sam. arch/powerpc/kernel/pci_64.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)