Patchwork [2/2] PCI: pciehp: Convert pciehp to be builtin only, not modular

login
register
mail settings
Submitter Bjorn Helgaas
Date July 25, 2013, 5:57 p.m.
Message ID <20130725175727.6006.98109.stgit@bhelgaas-glaptop>
Download mbox | patch
Permalink /patch/261860/
State Not Applicable
Delegated to: Benjamin Herrenschmidt
Headers show

Comments

Bjorn Helgaas - July 25, 2013, 5:57 p.m.
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(-)
Yinghai Lu - July 26, 2013, 12:43 p.m.
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>
Yinghai Lu - May 27, 2015, 6:31 p.m.
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
Bjorn Helgaas - May 27, 2015, 7:31 p.m.
[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
Rafael J. Wysocki - May 28, 2015, 1:30 a.m.
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
Yinghai Lu - May 28, 2015, 10:08 p.m.
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?.
Benjamin Herrenschmidt - May 28, 2015, 10:19 p.m.
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.

Patch

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"