| Message ID | 20130725175727.6006.98109.stgit@bhelgaas-glaptop (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Benjamin Herrenschmidt |
| Headers | show |
On Thu, Jul 25, 2013 at 10:57 AM, Bjorn Helgaas <bhelgaas@google.com> wrote: > Convert pciehp to be builtin only, with no module option. > > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> > Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > --- > drivers/pci/pcie/Kconfig | 5 +---- > 1 file changed, 1 insertion(+), 4 deletions(-) > > diff --git a/drivers/pci/pcie/Kconfig b/drivers/pci/pcie/Kconfig > index 569f82f..3b94cfc 100644 > --- a/drivers/pci/pcie/Kconfig > +++ b/drivers/pci/pcie/Kconfig > @@ -14,15 +14,12 @@ config PCIEPORTBUS > # Include service Kconfig here > # > config HOTPLUG_PCI_PCIE > - tristate "PCI Express Hotplug driver" > + bool "PCI Express Hotplug driver" > depends on HOTPLUG_PCI && PCIEPORTBUS > help > Say Y here if you have a motherboard that supports PCI Express Native > Hotplug > > - To compile this driver as a module, choose M here: the > - module will be called pciehp. > - > When in doubt, say N. > > source "drivers/pci/pcie/aer/Kconfig" > Acked-by: Yinghai Lu <yinghai@kernel.org>
On Fri, Jul 26, 2013 at 5:43 AM, Yinghai Lu <yinghai@kernel.org> wrote: > On Thu, Jul 25, 2013 at 10:57 AM, Bjorn Helgaas <bhelgaas@google.com> wrote: >> Convert pciehp to be builtin only, with no module option. >> >> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> >> Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> >> --- >> drivers/pci/pcie/Kconfig | 5 +---- >> 1 file changed, 1 insertion(+), 4 deletions(-) >> >> diff --git a/drivers/pci/pcie/Kconfig b/drivers/pci/pcie/Kconfig >> index 569f82f..3b94cfc 100644 >> --- a/drivers/pci/pcie/Kconfig >> +++ b/drivers/pci/pcie/Kconfig >> @@ -14,15 +14,12 @@ config PCIEPORTBUS >> # Include service Kconfig here >> # >> config HOTPLUG_PCI_PCIE >> - tristate "PCI Express Hotplug driver" >> + bool "PCI Express Hotplug driver" >> depends on HOTPLUG_PCI && PCIEPORTBUS >> help >> Say Y here if you have a motherboard that supports PCI Express Native >> Hotplug >> >> - To compile this driver as a module, choose M here: the >> - module will be called pciehp. >> - >> When in doubt, say N. >> >> source "drivers/pci/pcie/aer/Kconfig" >> > > Acked-by: Yinghai Lu <yinghai@kernel.org> Hi Bjorn, Looks like we lose the option to disable pciehp after we make it as built-in. Before acpiphp and pciehp could be compiled as modules, and user could blacklist to disable them. Now they are all built-in, but only acpiphp has acpiphp.disable to disable acpiphp. we don't have pciehp.disable yet. Do you think if we should add pciehp.disable ? BTW we don't have any description for acpiphp.disable anywhere. Thanks Yinghai
[updated Rafael's email addr; not sure if sisk.pl still works or not] On Wed, May 27, 2015 at 11:31:21AM -0700, Yinghai Lu wrote: > On Fri, Jul 26, 2013 at 5:43 AM, Yinghai Lu <yinghai@kernel.org> wrote: > > On Thu, Jul 25, 2013 at 10:57 AM, Bjorn Helgaas <bhelgaas@google.com> wrote: > >> Convert pciehp to be builtin only, with no module option. > >> > >> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> > >> Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > >> --- > >> drivers/pci/pcie/Kconfig | 5 +---- > >> 1 file changed, 1 insertion(+), 4 deletions(-) > >> > >> diff --git a/drivers/pci/pcie/Kconfig b/drivers/pci/pcie/Kconfig > >> index 569f82f..3b94cfc 100644 > >> --- a/drivers/pci/pcie/Kconfig > >> +++ b/drivers/pci/pcie/Kconfig > >> @@ -14,15 +14,12 @@ config PCIEPORTBUS > >> # Include service Kconfig here > >> # > >> config HOTPLUG_PCI_PCIE > >> - tristate "PCI Express Hotplug driver" > >> + bool "PCI Express Hotplug driver" > >> depends on HOTPLUG_PCI && PCIEPORTBUS > >> help > >> Say Y here if you have a motherboard that supports PCI Express Native > >> Hotplug > >> > >> - To compile this driver as a module, choose M here: the > >> - module will be called pciehp. > >> - > >> When in doubt, say N. > >> > >> source "drivers/pci/pcie/aer/Kconfig" > >> > > > > Acked-by: Yinghai Lu <yinghai@kernel.org> > > Hi Bjorn, > > Looks like we lose the option to disable pciehp after we make it as built-in. > > Before acpiphp and pciehp could be compiled as modules, and user could > blacklist to disable them. > > Now they are all built-in, but only acpiphp has acpiphp.disable to > disable acpiphp. > we don't have pciehp.disable yet. > > Do you think if we should add pciehp.disable ? Did you find a situation that would require pciehp.disable? I hesitate to add it because if there's a problem and pciehp.disable fixes it, people tend to think the solution is "boot with pciehp.disable." But the *real* solution is to fix whatever is broken in the kernel, so no parameter is needed at all. > BTW we don't have any description for acpiphp.disable anywhere. True. I'll give you my opinion; Rafael may have a different one. I don't know whether it's a good idea to add a description or not, for the same reason as above. I think we should actively discourage people from using kernel parameters, except for debugging purposes and for some legacy issues where there's no way for the kernel to figure things out by itself. But in my opinion, acpiphp isn't in any of those categories, so I'm content to have the parameter present but undocumented. It seems more likely that we'll hear about issues then, and we might be able to do something about them. Bjorn
On Wednesday, May 27, 2015 02:31:49 PM Bjorn Helgaas wrote: > [updated Rafael's email addr; not sure if sisk.pl still works or not] > > On Wed, May 27, 2015 at 11:31:21AM -0700, Yinghai Lu wrote: > > On Fri, Jul 26, 2013 at 5:43 AM, Yinghai Lu <yinghai@kernel.org> wrote: > > > On Thu, Jul 25, 2013 at 10:57 AM, Bjorn Helgaas <bhelgaas@google.com> wrote: > > >> Convert pciehp to be builtin only, with no module option. > > >> > > >> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> > > >> Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > >> --- > > >> drivers/pci/pcie/Kconfig | 5 +---- > > >> 1 file changed, 1 insertion(+), 4 deletions(-) > > >> > > >> diff --git a/drivers/pci/pcie/Kconfig b/drivers/pci/pcie/Kconfig > > >> index 569f82f..3b94cfc 100644 > > >> --- a/drivers/pci/pcie/Kconfig > > >> +++ b/drivers/pci/pcie/Kconfig > > >> @@ -14,15 +14,12 @@ config PCIEPORTBUS > > >> # Include service Kconfig here > > >> # > > >> config HOTPLUG_PCI_PCIE > > >> - tristate "PCI Express Hotplug driver" > > >> + bool "PCI Express Hotplug driver" > > >> depends on HOTPLUG_PCI && PCIEPORTBUS > > >> help > > >> Say Y here if you have a motherboard that supports PCI Express Native > > >> Hotplug > > >> > > >> - To compile this driver as a module, choose M here: the > > >> - module will be called pciehp. > > >> - > > >> When in doubt, say N. > > >> > > >> source "drivers/pci/pcie/aer/Kconfig" > > >> > > > > > > Acked-by: Yinghai Lu <yinghai@kernel.org> > > > > Hi Bjorn, > > > > Looks like we lose the option to disable pciehp after we make it as built-in. > > > > Before acpiphp and pciehp could be compiled as modules, and user could > > blacklist to disable them. > > > > Now they are all built-in, but only acpiphp has acpiphp.disable to > > disable acpiphp. > > we don't have pciehp.disable yet. > > > > Do you think if we should add pciehp.disable ? > > Did you find a situation that would require pciehp.disable? I hesitate to > add it because if there's a problem and pciehp.disable fixes it, people > tend to think the solution is "boot with pciehp.disable." But the *real* > solution is to fix whatever is broken in the kernel, so no parameter is > needed at all. Agreed. For debug you can always use pcie_ports=compat and that will disable pciehp too. > > BTW we don't have any description for acpiphp.disable anywhere. > > True. I'll give you my opinion; Rafael may have a different one. > > I don't know whether it's a good idea to add a description or not, for the > same reason as above. I think we should actively discourage people from > using kernel parameters, except for debugging purposes and for some legacy > issues where there's no way for the kernel to figure things out by itself. > But in my opinion, acpiphp isn't in any of those categories, so I'm content > to have the parameter present but undocumented. It seems more likely that > we'll hear about issues then, and we might be able to do something about > them. Agreed again. Rafael
On Wed, May 27, 2015 at 6:30 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: > On Wednesday, May 27, 2015 02:31:49 PM Bjorn Helgaas wrote: > For debug you can always use pcie_ports=compat and that will disable > pciehp too. That will disable AER at the same time, right?.
On Thu, 2015-05-28 at 15:08 -0700, Yinghai Lu wrote: > On Wed, May 27, 2015 at 6:30 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: > > On Wednesday, May 27, 2015 02:31:49 PM Bjorn Helgaas wrote: > > For debug you can always use pcie_ports=compat and that will disable > > pciehp too. > > That will disable AER at the same time, right?. For ppc machines under either our hypervisor or running bare metal with our EEH-enabled bridges, both generic AER and hotplug code are going to interfere. EEH "subsumes" AER, and we have firmware interfaces to do hotplug that know more than what the generic code can know about (such as on-board GPIOs that can control PERST, it's not always via the hotplug registers or hotplug from top-level slots which isn't the same as hotplug from switch slots on our platforms). So basically, at this point, we must not have the PCIe port drivers at all, just just interfere. We should replace that with the appropriate "hooks" for the platform to disable selected functions (hotplug and AER) from the generic port drivers. Cheers, Ben.
diff --git a/drivers/pci/pcie/Kconfig b/drivers/pci/pcie/Kconfig index 569f82f..3b94cfc 100644 --- a/drivers/pci/pcie/Kconfig +++ b/drivers/pci/pcie/Kconfig @@ -14,15 +14,12 @@ config PCIEPORTBUS # Include service Kconfig here # config HOTPLUG_PCI_PCIE - tristate "PCI Express Hotplug driver" + bool "PCI Express Hotplug driver" depends on HOTPLUG_PCI && PCIEPORTBUS help Say Y here if you have a motherboard that supports PCI Express Native Hotplug - To compile this driver as a module, choose M here: the - module will be called pciehp. - When in doubt, say N. source "drivers/pci/pcie/aer/Kconfig"