Patchwork [RFC,v5,4/8] ACPI, PCI: avoid building pci_slot as module

login
register
mail settings
Submitter Jiang Liu
Date Jan. 18, 2013, 4:07 p.m.
Message ID <1358525267-14268-5-git-send-email-jiang.liu@huawei.com>
Download mbox | patch
Permalink /patch/213663/
State Not Applicable
Headers show

Comments

Jiang Liu - Jan. 18, 2013, 4:07 p.m.
As discussed in thread at https://patchwork.kernel.org/patch/1946851/,
there's no value in supporting CONFIG_ACPI_PCI_SLOT=m any more.
So change Kconfig and code to only support building pci_slot as
built-in driver.

Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
---
 drivers/acpi/Kconfig    |    5 +----
 drivers/acpi/internal.h |    5 +++++
 drivers/acpi/pci_slot.c |   13 +------------
 drivers/acpi/scan.c     |    1 +
 4 files changed, 8 insertions(+), 16 deletions(-)
Rafael J. Wysocki - Jan. 21, 2013, 12:01 a.m.
On Saturday, January 19, 2013 12:07:42 AM Jiang Liu wrote:
> As discussed in thread at https://patchwork.kernel.org/patch/1946851/,
> there's no value in supporting CONFIG_ACPI_PCI_SLOT=m any more.
> So change Kconfig and code to only support building pci_slot as
> built-in driver.
> 
> Signed-off-by: Jiang Liu <jiang.liu@huawei.com>

Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

> ---
>  drivers/acpi/Kconfig    |    5 +----
>  drivers/acpi/internal.h |    5 +++++
>  drivers/acpi/pci_slot.c |   13 +------------
>  drivers/acpi/scan.c     |    1 +
>  4 files changed, 8 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
> index 0300bf6..7efd0d0 100644
> --- a/drivers/acpi/Kconfig
> +++ b/drivers/acpi/Kconfig
> @@ -299,7 +299,7 @@ config ACPI_DEBUG_FUNC_TRACE
>  	  is about half of the penalty and is rarely useful.
>  
>  config ACPI_PCI_SLOT
> -	tristate "PCI slot detection driver"
> +	bool "PCI slot detection driver"
>  	depends on SYSFS
>  	default n
>  	help
> @@ -308,9 +308,6 @@ config ACPI_PCI_SLOT
>  	  i.e., segment/bus/device/function tuples, with physical slots in
>  	  the system.  If you are unsure, say N.
>  
> -	  To compile this driver as a module, choose M here:
> -	  the module will be called pci_slot.
> -
>  config X86_PM_TIMER
>  	bool "Power Management Timer Support" if EXPERT
>  	depends on X86
> diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h
> index e050254..7374cfc 100644
> --- a/drivers/acpi/internal.h
> +++ b/drivers/acpi/internal.h
> @@ -67,6 +67,11 @@ struct acpi_ec {
>  
>  extern struct acpi_ec *first_ec;
>  
> +#ifdef	CONFIG_ACPI_PCI_SLOT
> +void acpi_pci_slot_init(void);
> +#else
> +static inline void acpi_pci_slot_init(void) { }
> +#endif
>  int acpi_pci_root_init(void);
>  int acpi_ec_init(void);
>  int acpi_ec_ecdt_probe(void);
> diff --git a/drivers/acpi/pci_slot.c b/drivers/acpi/pci_slot.c
> index d22585f..a7d7e77 100644
> --- a/drivers/acpi/pci_slot.c
> +++ b/drivers/acpi/pci_slot.c
> @@ -330,19 +330,8 @@ static struct dmi_system_id acpi_pci_slot_dmi_table[] __initdata = {
>  	{}
>  };
>  
> -static int __init
> -acpi_pci_slot_init(void)
> +void __init acpi_pci_slot_init(void)
>  {
>  	dmi_check_system(acpi_pci_slot_dmi_table);
>  	acpi_pci_register_driver(&acpi_pci_slot_driver);
> -	return 0;
>  }
> -
> -static void __exit
> -acpi_pci_slot_exit(void)
> -{
> -	acpi_pci_unregister_driver(&acpi_pci_slot_driver);
> -}
> -
> -module_init(acpi_pci_slot_init);
> -module_exit(acpi_pci_slot_exit);
> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> index f8a0d0f..cb964ac 100644
> --- a/drivers/acpi/scan.c
> +++ b/drivers/acpi/scan.c
> @@ -1732,6 +1732,7 @@ int __init acpi_scan_init(void)
>  
>  	acpi_power_init();
>  	acpi_pci_root_init();
> +	acpi_pci_slot_init();
>  
>  	/*
>  	 * Enumerate devices in the ACPI namespace.
>
Bjorn Helgaas - Jan. 28, 2013, 9:09 p.m.
On Sun, Jan 20, 2013 at 5:01 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> On Saturday, January 19, 2013 12:07:42 AM Jiang Liu wrote:
>> As discussed in thread at https://patchwork.kernel.org/patch/1946851/,
>> there's no value in supporting CONFIG_ACPI_PCI_SLOT=m any more.
>> So change Kconfig and code to only support building pci_slot as
>> built-in driver.
>>
>> Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
>
> Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Acked-by: Bjorn Helgaas <bhelgaas@google.com>

I think we should eventually get rid of acpi_pci_register_driver() and
do this initialization directly from acpi_pci_root_add().  But
removing the module option here is a good first step.

Rafael, do you want to apply this (and [6/8]) via your tree?  If not,
I can take it.

>> ---
>>  drivers/acpi/Kconfig    |    5 +----
>>  drivers/acpi/internal.h |    5 +++++
>>  drivers/acpi/pci_slot.c |   13 +------------
>>  drivers/acpi/scan.c     |    1 +
>>  4 files changed, 8 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
>> index 0300bf6..7efd0d0 100644
>> --- a/drivers/acpi/Kconfig
>> +++ b/drivers/acpi/Kconfig
>> @@ -299,7 +299,7 @@ config ACPI_DEBUG_FUNC_TRACE
>>         is about half of the penalty and is rarely useful.
>>
>>  config ACPI_PCI_SLOT
>> -     tristate "PCI slot detection driver"
>> +     bool "PCI slot detection driver"
>>       depends on SYSFS
>>       default n
>>       help
>> @@ -308,9 +308,6 @@ config ACPI_PCI_SLOT
>>         i.e., segment/bus/device/function tuples, with physical slots in
>>         the system.  If you are unsure, say N.
>>
>> -       To compile this driver as a module, choose M here:
>> -       the module will be called pci_slot.
>> -
>>  config X86_PM_TIMER
>>       bool "Power Management Timer Support" if EXPERT
>>       depends on X86
>> diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h
>> index e050254..7374cfc 100644
>> --- a/drivers/acpi/internal.h
>> +++ b/drivers/acpi/internal.h
>> @@ -67,6 +67,11 @@ struct acpi_ec {
>>
>>  extern struct acpi_ec *first_ec;
>>
>> +#ifdef       CONFIG_ACPI_PCI_SLOT
>> +void acpi_pci_slot_init(void);
>> +#else
>> +static inline void acpi_pci_slot_init(void) { }
>> +#endif
>>  int acpi_pci_root_init(void);
>>  int acpi_ec_init(void);
>>  int acpi_ec_ecdt_probe(void);
>> diff --git a/drivers/acpi/pci_slot.c b/drivers/acpi/pci_slot.c
>> index d22585f..a7d7e77 100644
>> --- a/drivers/acpi/pci_slot.c
>> +++ b/drivers/acpi/pci_slot.c
>> @@ -330,19 +330,8 @@ static struct dmi_system_id acpi_pci_slot_dmi_table[] __initdata = {
>>       {}
>>  };
>>
>> -static int __init
>> -acpi_pci_slot_init(void)
>> +void __init acpi_pci_slot_init(void)
>>  {
>>       dmi_check_system(acpi_pci_slot_dmi_table);
>>       acpi_pci_register_driver(&acpi_pci_slot_driver);
>> -     return 0;
>>  }
>> -
>> -static void __exit
>> -acpi_pci_slot_exit(void)
>> -{
>> -     acpi_pci_unregister_driver(&acpi_pci_slot_driver);
>> -}
>> -
>> -module_init(acpi_pci_slot_init);
>> -module_exit(acpi_pci_slot_exit);
>> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
>> index f8a0d0f..cb964ac 100644
>> --- a/drivers/acpi/scan.c
>> +++ b/drivers/acpi/scan.c
>> @@ -1732,6 +1732,7 @@ int __init acpi_scan_init(void)
>>
>>       acpi_power_init();
>>       acpi_pci_root_init();
>> +     acpi_pci_slot_init();
>>
>>       /*
>>        * Enumerate devices in the ACPI namespace.
>>
> --
> I speak only for myself.
> Rafael J. Wysocki, Intel Open Source Technology Center.
--
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
Yinghai Lu - Jan. 28, 2013, 9:29 p.m.
On Mon, Jan 28, 2013 at 1:09 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> On Sun, Jan 20, 2013 at 5:01 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
>> On Saturday, January 19, 2013 12:07:42 AM Jiang Liu wrote:
>>> As discussed in thread at https://patchwork.kernel.org/patch/1946851/,
>>> there's no value in supporting CONFIG_ACPI_PCI_SLOT=m any more.
>>> So change Kconfig and code to only support building pci_slot as
>>> built-in driver.
>>>
>>> Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
>>
>> Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> Acked-by: Bjorn Helgaas <bhelgaas@google.com>
>
> I think we should eventually get rid of acpi_pci_register_driver() and
> do this initialization directly from acpi_pci_root_add().  But
> removing the module option here is a good first step.
>
> Rafael, do you want to apply this (and [6/8]) via your tree?  If not,
> I can take it.

If bios have messed up slot name or idx, we will get strange 1-1....
other crazy name.

if you really need to put it as built-in, may need to some command
line that user could switch it off.

Yinghai
--
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
Bjorn Helgaas - Jan. 28, 2013, 9:52 p.m.
On Mon, Jan 28, 2013 at 2:29 PM, Yinghai Lu <yinghai@kernel.org> wrote:
> On Mon, Jan 28, 2013 at 1:09 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>> On Sun, Jan 20, 2013 at 5:01 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
>>> On Saturday, January 19, 2013 12:07:42 AM Jiang Liu wrote:
>>>> As discussed in thread at https://patchwork.kernel.org/patch/1946851/,
>>>> there's no value in supporting CONFIG_ACPI_PCI_SLOT=m any more.
>>>> So change Kconfig and code to only support building pci_slot as
>>>> built-in driver.
>>>>
>>>> Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
>>>
>>> Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>
>> Acked-by: Bjorn Helgaas <bhelgaas@google.com>
>>
>> I think we should eventually get rid of acpi_pci_register_driver() and
>> do this initialization directly from acpi_pci_root_add().  But
>> removing the module option here is a good first step.
>>
>> Rafael, do you want to apply this (and [6/8]) via your tree?  If not,
>> I can take it.
>
> If bios have messed up slot name or idx, we will get strange 1-1....
> other crazy name.
>
> if you really need to put it as built-in, may need to some command
> line that user could switch it off.

It would save us all a lot of time if you gave an example and worked
through the scenario where this is a problem.

We already have the choice of having pci_slot built-in, so if there's
a bug in that config, we already have the bug.  This patch merely
removes a config where the bug might be covered up.

I don't know why "adding a command line switch" appeals to you as the
solution to every problem.  As far as I'm concerned that's not a
solution to ANY problem.  It might be a band-aid to enable users to
limp along while we figure out a correct solution, but it's certainly
not a resolution.

Bjorn
--
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
Yinghai Lu - Jan. 28, 2013, 10 p.m.
On Mon, Jan 28, 2013 at 1:52 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> On Mon, Jan 28, 2013 at 2:29 PM, Yinghai Lu <yinghai@kernel.org> wrote:
...
>> If bios have messed up slot name or idx, we will get strange 1-1....
>> other crazy name.
>>
>> if you really need to put it as built-in, may need to some command
>> line that user could switch it off.
>
> It would save us all a lot of time if you gave an example and worked
> through the scenario where this is a problem.
>
> We already have the choice of having pci_slot built-in, so if there's
> a bug in that config, we already have the bug.  This patch merely
> removes a config where the bug might be covered up.


for distribution, current it is with module, so user could blacklist
in module.conf

Now with built-in or not, distribution will have it built-in, and user
have no chance to
disable it.

> I don't know why "adding a command line switch" appeals to you as the
> solution to every problem.  As far as I'm concerned that's not a
> solution to ANY problem.  It might be a band-aid to enable users to
> limp along while we figure out a correct solution, but it's certainly
> not a resolution.

Looks like you want to remove command line support, right ?

Yinghai
--
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
Bjorn Helgaas - Jan. 28, 2013, 10:14 p.m.
On Mon, Jan 28, 2013 at 3:00 PM, Yinghai Lu <yinghai@kernel.org> wrote:
> On Mon, Jan 28, 2013 at 1:52 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>> On Mon, Jan 28, 2013 at 2:29 PM, Yinghai Lu <yinghai@kernel.org> wrote:
> ...
>>> If bios have messed up slot name or idx, we will get strange 1-1....
>>> other crazy name.
>>>
>>> if you really need to put it as built-in, may need to some command
>>> line that user could switch it off.
>>
>> It would save us all a lot of time if you gave an example and worked
>> through the scenario where this is a problem.
>>
>> We already have the choice of having pci_slot built-in, so if there's
>> a bug in that config, we already have the bug.  This patch merely
>> removes a config where the bug might be covered up.
>
>
> for distribution, current it is with module, so user could blacklist
> in module.conf
>
> Now with built-in or not, distribution will have it built-in, and user
> have no chance to
> disable it.

CONFIG_ACPI_PCI_SLOT=y in RHEL6, so evidently they have this problem.

Asking users to edit module.conf by hand is not a solution, just like
asking users to boot with a command line option is not a solution.
That sort of stuff is fine for a hobbyist OS intended only for techie
geeks.  It's not fine for Linux.

If you would give a concrete example of the ACPI namespace info and
device config, hotplug sequence, etc., required to show the problem,
we could have a useful discussion about ways to fix it.  But if all
you have is FUD about "this might break and users won't have the
ability to edit modules.conf," that doesn't help me see why this patch
is a bad idea.

>> I don't know why "adding a command line switch" appeals to you as the
>> solution to every problem.  As far as I'm concerned that's not a
>> solution to ANY problem.  It might be a band-aid to enable users to
>> limp along while we figure out a correct solution, but it's certainly
>> not a resolution.
>
> Looks like you want to remove command line support, right ?
>
> Yinghai
--
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
Yinghai Lu - Jan. 28, 2013, 10:58 p.m.
On Mon, Jan 28, 2013 at 2:14 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>
> CONFIG_ACPI_PCI_SLOT=y in RHEL6, so evidently they have this problem.

oh, I only checked opensuse that has that set to m.

>
> Asking users to edit module.conf by hand is not a solution, just like
> asking users to boot with a command line option is not a solution.
> That sort of stuff is fine for a hobbyist OS intended only for techie
> geeks.  It's not fine for Linux.

not sure. add something in command line or conf files.
but recompile kernel is another story.

>
> If you would give a concrete example of the ACPI namespace info and
> device config, hotplug sequence, etc., required to show the problem,
> we could have a useful discussion about ways to fix it.  But if all
> you have is FUD about "this might break and users won't have the
> ability to edit modules.conf," that doesn't help me see why this patch
> is a bad idea.

Never mind, We should save your bandwidth to more patches.

Yinghai
--
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
Rafael J. Wysocki - Jan. 29, 2013, 1 a.m.
On Monday, January 28, 2013 02:09:22 PM Bjorn Helgaas wrote:
> On Sun, Jan 20, 2013 at 5:01 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > On Saturday, January 19, 2013 12:07:42 AM Jiang Liu wrote:
> >> As discussed in thread at https://patchwork.kernel.org/patch/1946851/,
> >> there's no value in supporting CONFIG_ACPI_PCI_SLOT=m any more.
> >> So change Kconfig and code to only support building pci_slot as
> >> built-in driver.
> >>
> >> Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
> >
> > Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Acked-by: Bjorn Helgaas <bhelgaas@google.com>
> 
> I think we should eventually get rid of acpi_pci_register_driver() and
> do this initialization directly from acpi_pci_root_add().  But
> removing the module option here is a good first step.
> 
> Rafael, do you want to apply this (and [6/8]) via your tree?  If not,
> I can take it.

I will, thanks.

Rafael


> >> ---
> >>  drivers/acpi/Kconfig    |    5 +----
> >>  drivers/acpi/internal.h |    5 +++++
> >>  drivers/acpi/pci_slot.c |   13 +------------
> >>  drivers/acpi/scan.c     |    1 +
> >>  4 files changed, 8 insertions(+), 16 deletions(-)
> >>
> >> diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
> >> index 0300bf6..7efd0d0 100644
> >> --- a/drivers/acpi/Kconfig
> >> +++ b/drivers/acpi/Kconfig
> >> @@ -299,7 +299,7 @@ config ACPI_DEBUG_FUNC_TRACE
> >>         is about half of the penalty and is rarely useful.
> >>
> >>  config ACPI_PCI_SLOT
> >> -     tristate "PCI slot detection driver"
> >> +     bool "PCI slot detection driver"
> >>       depends on SYSFS
> >>       default n
> >>       help
> >> @@ -308,9 +308,6 @@ config ACPI_PCI_SLOT
> >>         i.e., segment/bus/device/function tuples, with physical slots in
> >>         the system.  If you are unsure, say N.
> >>
> >> -       To compile this driver as a module, choose M here:
> >> -       the module will be called pci_slot.
> >> -
> >>  config X86_PM_TIMER
> >>       bool "Power Management Timer Support" if EXPERT
> >>       depends on X86
> >> diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h
> >> index e050254..7374cfc 100644
> >> --- a/drivers/acpi/internal.h
> >> +++ b/drivers/acpi/internal.h
> >> @@ -67,6 +67,11 @@ struct acpi_ec {
> >>
> >>  extern struct acpi_ec *first_ec;
> >>
> >> +#ifdef       CONFIG_ACPI_PCI_SLOT
> >> +void acpi_pci_slot_init(void);
> >> +#else
> >> +static inline void acpi_pci_slot_init(void) { }
> >> +#endif
> >>  int acpi_pci_root_init(void);
> >>  int acpi_ec_init(void);
> >>  int acpi_ec_ecdt_probe(void);
> >> diff --git a/drivers/acpi/pci_slot.c b/drivers/acpi/pci_slot.c
> >> index d22585f..a7d7e77 100644
> >> --- a/drivers/acpi/pci_slot.c
> >> +++ b/drivers/acpi/pci_slot.c
> >> @@ -330,19 +330,8 @@ static struct dmi_system_id acpi_pci_slot_dmi_table[] __initdata = {
> >>       {}
> >>  };
> >>
> >> -static int __init
> >> -acpi_pci_slot_init(void)
> >> +void __init acpi_pci_slot_init(void)
> >>  {
> >>       dmi_check_system(acpi_pci_slot_dmi_table);
> >>       acpi_pci_register_driver(&acpi_pci_slot_driver);
> >> -     return 0;
> >>  }
> >> -
> >> -static void __exit
> >> -acpi_pci_slot_exit(void)
> >> -{
> >> -     acpi_pci_unregister_driver(&acpi_pci_slot_driver);
> >> -}
> >> -
> >> -module_init(acpi_pci_slot_init);
> >> -module_exit(acpi_pci_slot_exit);
> >> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> >> index f8a0d0f..cb964ac 100644
> >> --- a/drivers/acpi/scan.c
> >> +++ b/drivers/acpi/scan.c
> >> @@ -1732,6 +1732,7 @@ int __init acpi_scan_init(void)
> >>
> >>       acpi_power_init();
> >>       acpi_pci_root_init();
> >> +     acpi_pci_slot_init();
> >>
> >>       /*
> >>        * Enumerate devices in the ACPI namespace.
> >>
> > --
> > I speak only for myself.
> > Rafael J. Wysocki, Intel Open Source Technology Center.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jiang Liu - Jan. 29, 2013, 2:07 a.m.
On 2013-1-29 6:58, Yinghai Lu wrote:
> On Mon, Jan 28, 2013 at 2:14 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>>
>> CONFIG_ACPI_PCI_SLOT=y in RHEL6, so evidently they have this problem.
> 
> oh, I only checked opensuse that has that set to m.
> 
>>
>> Asking users to edit module.conf by hand is not a solution, just like
>> asking users to boot with a command line option is not a solution.
>> That sort of stuff is fine for a hobbyist OS intended only for techie
>> geeks.  It's not fine for Linux.
> 
> not sure. add something in command line or conf files.
> but recompile kernel is another story.
> 
>>
>> If you would give a concrete example of the ACPI namespace info and
>> device config, hotplug sequence, etc., required to show the problem,
>> we could have a useful discussion about ways to fix it.  But if all
>> you have is FUD about "this might break and users won't have the
>> ability to edit modules.conf," that doesn't help me see why this patch
>> is a bad idea.
> 
> Never mind, We should save your bandwidth to more patches.
Hi Yinghai,
	Could we use quirk to auto-disable PCIe native hotplug for
problematic platforms?
Regards!
Gerry

> 
> Yinghai
> 
> .
> 


--
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
Yinghai Lu - Jan. 29, 2013, 2:21 a.m.
On Mon, Jan 28, 2013 at 6:07 PM, Jiang Liu <jiang.liu@huawei.com> wrote:
>         Could we use quirk to auto-disable PCIe native hotplug for
> problematic platforms?

that is some kind of boot command line way?

Yinghai
--
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
Jiang Liu - Jan. 29, 2013, 2:45 a.m.
On 2013-1-29 10:21, Yinghai Lu wrote:
> On Mon, Jan 28, 2013 at 6:07 PM, Jiang Liu <jiang.liu@huawei.com> wrote:
>>         Could we use quirk to auto-disable PCIe native hotplug for
>> problematic platforms?
> 
> that is some kind of boot command line way?
We could negotiate between acpiphp and pciehp if those problematic 
platforms/chipsets could be identified by DMI info or PCI device ID.

> 
> Yinghai
> 
> 


--
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
Bjorn Helgaas - Jan. 29, 2013, 2:50 a.m.
On Mon, Jan 28, 2013 at 7:45 PM, Jiang Liu <jiang.liu@huawei.com> wrote:
> On 2013-1-29 10:21, Yinghai Lu wrote:
>> On Mon, Jan 28, 2013 at 6:07 PM, Jiang Liu <jiang.liu@huawei.com> wrote:
>>>         Could we use quirk to auto-disable PCIe native hotplug for
>>> problematic platforms?
>>
>> that is some kind of boot command line way?
> We could negotiate between acpiphp and pciehp if those problematic
> platforms/chipsets could be identified by DMI info or PCI device ID.

That's a sub-optimal solution because it requires us to identify all
the affected systems and also may require ongoing maintenance in the
future.  If we could get specifics on the issue, we might be able to
design a better solution that works for everybody.

(BTW, this thread is specifically about pci_slot, which I think is a
slightly different issue than the acpiphp/pciehp conflicts.)
--
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
Matthew Garrett - Jan. 29, 2013, 4:36 a.m.
On Mon, Jan 28, 2013 at 03:14:11PM -0700, Bjorn Helgaas wrote:

> CONFIG_ACPI_PCI_SLOT=y in RHEL6, so evidently they have this problem.

I'm not aware of anyone ever filing any bugs against it, either, though 
Myron would have a better idea. What's the worst case outcome of someone 
ending up with an incorrect slot number?
Matthew Garrett - Jan. 29, 2013, 4:36 a.m.
On Tue, Jan 29, 2013 at 10:07:22AM +0800, Jiang Liu wrote:

> 	Could we use quirk to auto-disable PCIe native hotplug for
> problematic platforms?

Do these problematic platforms successfully boot Windows 7?
Rafael J. Wysocki - Feb. 3, 2013, 8:18 p.m.
On Monday, January 28, 2013 02:09:22 PM Bjorn Helgaas wrote:
> On Sun, Jan 20, 2013 at 5:01 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > On Saturday, January 19, 2013 12:07:42 AM Jiang Liu wrote:
> >> As discussed in thread at https://patchwork.kernel.org/patch/1946851/,
> >> there's no value in supporting CONFIG_ACPI_PCI_SLOT=m any more.
> >> So change Kconfig and code to only support building pci_slot as
> >> built-in driver.
> >>
> >> Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
> >
> > Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Acked-by: Bjorn Helgaas <bhelgaas@google.com>
> 
> I think we should eventually get rid of acpi_pci_register_driver() and
> do this initialization directly from acpi_pci_root_add().  But
> removing the module option here is a good first step.
> 
> Rafael, do you want to apply this (and [6/8]) via your tree?  If not,
> I can take it.

Both applied to bleeding-edge.  I've added your ACK to the [6/8] too, hope
that's OK.

Thanks,
Rafael
Bjorn Helgaas - Feb. 3, 2013, 8:58 p.m.
On Sun, Feb 3, 2013 at 1:18 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> On Monday, January 28, 2013 02:09:22 PM Bjorn Helgaas wrote:
>> On Sun, Jan 20, 2013 at 5:01 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
>> > On Saturday, January 19, 2013 12:07:42 AM Jiang Liu wrote:
>> >> As discussed in thread at https://patchwork.kernel.org/patch/1946851/,
>> >> there's no value in supporting CONFIG_ACPI_PCI_SLOT=m any more.
>> >> So change Kconfig and code to only support building pci_slot as
>> >> built-in driver.
>> >>
>> >> Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
>> >
>> > Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>
>> Acked-by: Bjorn Helgaas <bhelgaas@google.com>
>>
>> I think we should eventually get rid of acpi_pci_register_driver() and
>> do this initialization directly from acpi_pci_root_add().  But
>> removing the module option here is a good first step.
>>
>> Rafael, do you want to apply this (and [6/8]) via your tree?  If not,
>> I can take it.
>
> Both applied to bleeding-edge.  I've added your ACK to the [6/8] too, hope
> that's OK.

Yep, that's fine, thanks!

Bjorn
--
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
Myron Stowe - Feb. 3, 2013, 10:47 p.m.
On Mon, 2013-01-28 at 14:09 -0700, Bjorn Helgaas wrote:
> On Sun, Jan 20, 2013 at 5:01 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > On Saturday, January 19, 2013 12:07:42 AM Jiang Liu wrote:
> >> As discussed in thread at https://patchwork.kernel.org/patch/1946851/,
> >> there's no value in supporting CONFIG_ACPI_PCI_SLOT=m any more.
> >> So change Kconfig and code to only support building pci_slot as
> >> built-in driver.
> >>
> >> Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
> >
> > Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Acked-by: Bjorn Helgaas <bhelgaas@google.com>
> 
> I think we should eventually get rid of acpi_pci_register_driver() and
> do this initialization directly from acpi_pci_root_add().  But
> removing the module option here is a good first step.

Bjorn, Rafael:

If either of you are interested in that still let me know and I can
re-work any or all of the "PCI/ACPI: Remove "pci_root" sub-driver
support" series - https://lkml.org/lkml/2012/12/7/11

Note [PATCH 13/15] was effectively the same as below and continued on
with initializing directly from acpi_pci_root_add() in [PATCH 14/15].
There may have been worthwhile fixes in some of the earlier content such
as [PATCH 11/15] worth re-considering also.

Thanks,
 Myron

> 
> Rafael, do you want to apply this (and [6/8]) via your tree?  If not,
> I can take it.
> 
> >> ---
> >>  drivers/acpi/Kconfig    |    5 +----
> >>  drivers/acpi/internal.h |    5 +++++
> >>  drivers/acpi/pci_slot.c |   13 +------------
> >>  drivers/acpi/scan.c     |    1 +
> >>  4 files changed, 8 insertions(+), 16 deletions(-)
> >>
> >> diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
> >> index 0300bf6..7efd0d0 100644
> >> --- a/drivers/acpi/Kconfig
> >> +++ b/drivers/acpi/Kconfig
> >> @@ -299,7 +299,7 @@ config ACPI_DEBUG_FUNC_TRACE
> >>         is about half of the penalty and is rarely useful.
> >>
> >>  config ACPI_PCI_SLOT
> >> -     tristate "PCI slot detection driver"
> >> +     bool "PCI slot detection driver"
> >>       depends on SYSFS
> >>       default n
> >>       help
> >> @@ -308,9 +308,6 @@ config ACPI_PCI_SLOT
> >>         i.e., segment/bus/device/function tuples, with physical slots in
> >>         the system.  If you are unsure, say N.
> >>
> >> -       To compile this driver as a module, choose M here:
> >> -       the module will be called pci_slot.
> >> -
> >>  config X86_PM_TIMER
> >>       bool "Power Management Timer Support" if EXPERT
> >>       depends on X86
> >> diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h
> >> index e050254..7374cfc 100644
> >> --- a/drivers/acpi/internal.h
> >> +++ b/drivers/acpi/internal.h
> >> @@ -67,6 +67,11 @@ struct acpi_ec {
> >>
> >>  extern struct acpi_ec *first_ec;
> >>
> >> +#ifdef       CONFIG_ACPI_PCI_SLOT
> >> +void acpi_pci_slot_init(void);
> >> +#else
> >> +static inline void acpi_pci_slot_init(void) { }
> >> +#endif
> >>  int acpi_pci_root_init(void);
> >>  int acpi_ec_init(void);
> >>  int acpi_ec_ecdt_probe(void);
> >> diff --git a/drivers/acpi/pci_slot.c b/drivers/acpi/pci_slot.c
> >> index d22585f..a7d7e77 100644
> >> --- a/drivers/acpi/pci_slot.c
> >> +++ b/drivers/acpi/pci_slot.c
> >> @@ -330,19 +330,8 @@ static struct dmi_system_id acpi_pci_slot_dmi_table[] __initdata = {
> >>       {}
> >>  };
> >>
> >> -static int __init
> >> -acpi_pci_slot_init(void)
> >> +void __init acpi_pci_slot_init(void)
> >>  {
> >>       dmi_check_system(acpi_pci_slot_dmi_table);
> >>       acpi_pci_register_driver(&acpi_pci_slot_driver);
> >> -     return 0;
> >>  }
> >> -
> >> -static void __exit
> >> -acpi_pci_slot_exit(void)
> >> -{
> >> -     acpi_pci_unregister_driver(&acpi_pci_slot_driver);
> >> -}
> >> -
> >> -module_init(acpi_pci_slot_init);
> >> -module_exit(acpi_pci_slot_exit);
> >> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> >> index f8a0d0f..cb964ac 100644
> >> --- a/drivers/acpi/scan.c
> >> +++ b/drivers/acpi/scan.c
> >> @@ -1732,6 +1732,7 @@ int __init acpi_scan_init(void)
> >>
> >>       acpi_power_init();
> >>       acpi_pci_root_init();
> >> +     acpi_pci_slot_init();
> >>
> >>       /*
> >>        * Enumerate devices in the ACPI namespace.
> >>
> > --
> > I speak only for myself.
> > Rafael J. Wysocki, Intel Open Source Technology Center.


--
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
Rafael J. Wysocki - Feb. 3, 2013, 11:38 p.m.
On Sunday, February 03, 2013 03:47:15 PM Myron Stowe wrote:
> On Mon, 2013-01-28 at 14:09 -0700, Bjorn Helgaas wrote:
> > On Sun, Jan 20, 2013 at 5:01 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > > On Saturday, January 19, 2013 12:07:42 AM Jiang Liu wrote:
> > >> As discussed in thread at https://patchwork.kernel.org/patch/1946851/,
> > >> there's no value in supporting CONFIG_ACPI_PCI_SLOT=m any more.
> > >> So change Kconfig and code to only support building pci_slot as
> > >> built-in driver.
> > >>
> > >> Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
> > >
> > > Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > 
> > Acked-by: Bjorn Helgaas <bhelgaas@google.com>
> > 
> > I think we should eventually get rid of acpi_pci_register_driver() and
> > do this initialization directly from acpi_pci_root_add().  But
> > removing the module option here is a good first step.
> 
> Bjorn, Rafael:
> 
> If either of you are interested in that still let me know and I can
> re-work any or all of the "PCI/ACPI: Remove "pci_root" sub-driver
> support" series - https://lkml.org/lkml/2012/12/7/11

I am.  However, I think it's better to wait with the rework until our trees
are merged into the mainline, so that you can base the patchset on a common
tree.

> Note [PATCH 13/15] was effectively the same as below and continued on
> with initializing directly from acpi_pci_root_add() in [PATCH 14/15].
> There may have been worthwhile fixes in some of the earlier content such
> as [PATCH 11/15] worth re-considering also.

Well, as I said.  All of the patches in the series making sense after v3.9-rc1
will be interesting to me certainly.

Thanks,
Rafael


> > Rafael, do you want to apply this (and [6/8]) via your tree?  If not,
> > I can take it.
> > 
> > >> ---
> > >>  drivers/acpi/Kconfig    |    5 +----
> > >>  drivers/acpi/internal.h |    5 +++++
> > >>  drivers/acpi/pci_slot.c |   13 +------------
> > >>  drivers/acpi/scan.c     |    1 +
> > >>  4 files changed, 8 insertions(+), 16 deletions(-)
> > >>
> > >> diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
> > >> index 0300bf6..7efd0d0 100644
> > >> --- a/drivers/acpi/Kconfig
> > >> +++ b/drivers/acpi/Kconfig
> > >> @@ -299,7 +299,7 @@ config ACPI_DEBUG_FUNC_TRACE
> > >>         is about half of the penalty and is rarely useful.
> > >>
> > >>  config ACPI_PCI_SLOT
> > >> -     tristate "PCI slot detection driver"
> > >> +     bool "PCI slot detection driver"
> > >>       depends on SYSFS
> > >>       default n
> > >>       help
> > >> @@ -308,9 +308,6 @@ config ACPI_PCI_SLOT
> > >>         i.e., segment/bus/device/function tuples, with physical slots in
> > >>         the system.  If you are unsure, say N.
> > >>
> > >> -       To compile this driver as a module, choose M here:
> > >> -       the module will be called pci_slot.
> > >> -
> > >>  config X86_PM_TIMER
> > >>       bool "Power Management Timer Support" if EXPERT
> > >>       depends on X86
> > >> diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h
> > >> index e050254..7374cfc 100644
> > >> --- a/drivers/acpi/internal.h
> > >> +++ b/drivers/acpi/internal.h
> > >> @@ -67,6 +67,11 @@ struct acpi_ec {
> > >>
> > >>  extern struct acpi_ec *first_ec;
> > >>
> > >> +#ifdef       CONFIG_ACPI_PCI_SLOT
> > >> +void acpi_pci_slot_init(void);
> > >> +#else
> > >> +static inline void acpi_pci_slot_init(void) { }
> > >> +#endif
> > >>  int acpi_pci_root_init(void);
> > >>  int acpi_ec_init(void);
> > >>  int acpi_ec_ecdt_probe(void);
> > >> diff --git a/drivers/acpi/pci_slot.c b/drivers/acpi/pci_slot.c
> > >> index d22585f..a7d7e77 100644
> > >> --- a/drivers/acpi/pci_slot.c
> > >> +++ b/drivers/acpi/pci_slot.c
> > >> @@ -330,19 +330,8 @@ static struct dmi_system_id acpi_pci_slot_dmi_table[] __initdata = {
> > >>       {}
> > >>  };
> > >>
> > >> -static int __init
> > >> -acpi_pci_slot_init(void)
> > >> +void __init acpi_pci_slot_init(void)
> > >>  {
> > >>       dmi_check_system(acpi_pci_slot_dmi_table);
> > >>       acpi_pci_register_driver(&acpi_pci_slot_driver);
> > >> -     return 0;
> > >>  }
> > >> -
> > >> -static void __exit
> > >> -acpi_pci_slot_exit(void)
> > >> -{
> > >> -     acpi_pci_unregister_driver(&acpi_pci_slot_driver);
> > >> -}
> > >> -
> > >> -module_init(acpi_pci_slot_init);
> > >> -module_exit(acpi_pci_slot_exit);
> > >> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> > >> index f8a0d0f..cb964ac 100644
> > >> --- a/drivers/acpi/scan.c
> > >> +++ b/drivers/acpi/scan.c
> > >> @@ -1732,6 +1732,7 @@ int __init acpi_scan_init(void)
> > >>
> > >>       acpi_power_init();
> > >>       acpi_pci_root_init();
> > >> +     acpi_pci_slot_init();
> > >>
> > >>       /*
> > >>        * Enumerate devices in the ACPI namespace.
> > >>
> > > --
> > > I speak only for myself.
> > > Rafael J. Wysocki, Intel Open Source Technology Center.
> 
>

Patch

diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
index 0300bf6..7efd0d0 100644
--- a/drivers/acpi/Kconfig
+++ b/drivers/acpi/Kconfig
@@ -299,7 +299,7 @@  config ACPI_DEBUG_FUNC_TRACE
 	  is about half of the penalty and is rarely useful.
 
 config ACPI_PCI_SLOT
-	tristate "PCI slot detection driver"
+	bool "PCI slot detection driver"
 	depends on SYSFS
 	default n
 	help
@@ -308,9 +308,6 @@  config ACPI_PCI_SLOT
 	  i.e., segment/bus/device/function tuples, with physical slots in
 	  the system.  If you are unsure, say N.
 
-	  To compile this driver as a module, choose M here:
-	  the module will be called pci_slot.
-
 config X86_PM_TIMER
 	bool "Power Management Timer Support" if EXPERT
 	depends on X86
diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h
index e050254..7374cfc 100644
--- a/drivers/acpi/internal.h
+++ b/drivers/acpi/internal.h
@@ -67,6 +67,11 @@  struct acpi_ec {
 
 extern struct acpi_ec *first_ec;
 
+#ifdef	CONFIG_ACPI_PCI_SLOT
+void acpi_pci_slot_init(void);
+#else
+static inline void acpi_pci_slot_init(void) { }
+#endif
 int acpi_pci_root_init(void);
 int acpi_ec_init(void);
 int acpi_ec_ecdt_probe(void);
diff --git a/drivers/acpi/pci_slot.c b/drivers/acpi/pci_slot.c
index d22585f..a7d7e77 100644
--- a/drivers/acpi/pci_slot.c
+++ b/drivers/acpi/pci_slot.c
@@ -330,19 +330,8 @@  static struct dmi_system_id acpi_pci_slot_dmi_table[] __initdata = {
 	{}
 };
 
-static int __init
-acpi_pci_slot_init(void)
+void __init acpi_pci_slot_init(void)
 {
 	dmi_check_system(acpi_pci_slot_dmi_table);
 	acpi_pci_register_driver(&acpi_pci_slot_driver);
-	return 0;
 }
-
-static void __exit
-acpi_pci_slot_exit(void)
-{
-	acpi_pci_unregister_driver(&acpi_pci_slot_driver);
-}
-
-module_init(acpi_pci_slot_init);
-module_exit(acpi_pci_slot_exit);
diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index f8a0d0f..cb964ac 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -1732,6 +1732,7 @@  int __init acpi_scan_init(void)
 
 	acpi_power_init();
 	acpi_pci_root_init();
+	acpi_pci_slot_init();
 
 	/*
 	 * Enumerate devices in the ACPI namespace.