[PATCH, resend] x86/PCI: don't export a __devinit function

[PATCH, resend] x86/PCI: don't export a __devinit function

[Jan Beulich]

Exporting a __devinit function (pcibios_scan_specific_bus()) isn't
correct. (Michal, any reason why modpost only warns about exported
__init functions?) Short of being able to think of a better solution,
and short of making the whole call tree (reaching into the arch-
independent part of the PCI subsystem) non-__devinit, export the
symbol only when HOTPLUG is enabled (which is always the case for non-
expert configurations), use section mismatch avoidance annotations for
that case (knowing that __devinit functions will not be discarded),
and mark the symbol __devinit only in the !HOTPLUG case.

Consequently, EDAC_I7CORE (consuming the export) then has to depend on
HOTPLUG. A fundamental question of course if whether this driver has
to use that function in the first place (i.e. whether it wouldn't be
better to just remove the export) - the problem it tries to address
happens on other systems too, but the PCI bus the devices in question
live on isn't necessarily bus 255. For the affected system I have, the
alternative approach is to set pcibios_last_bus from __pci_mmcfg_init()
based on the highest bus number on segment 0 being covered by MCFG.

Signed-off-by: Jan Beulich <jbeulich@novell.com>
Cc: Mauro Carvalho Chehab <mchehab@redhat.com>
Cc: Aristeu Sergio <arozansk@redhat.com>
Cc: Michal Marek <mmarek@suse.cz>

---
 arch/x86/pci/legacy.c |   21 ++++++++++++++++++---
 drivers/edac/Kconfig  |    2 +-
 2 files changed, 19 insertions(+), 4 deletions(-)

--- 2.6.38-rc5/arch/x86/pci/legacy.c
+++ 2.6.38-rc5-x86-pci-section-conflict/arch/x86/pci/legacy.c
@@ -36,7 +36,23 @@ int __init pci_legacy_init(void)
 	return 0;
 }
 
-void __devinit pcibios_scan_specific_bus(int busn)
+#ifdef CONFIG_HOTPLUG
+static void __ref
+#else
+static inline void
+#endif
+_pci_scan_bus_on_node(int busno, int node)
+{
+	pci_scan_bus_on_node(busno, &pci_root_ops, node);
+}
+
+#ifdef CONFIG_HOTPLUG
+EXPORT_SYMBOL_GPL(pcibios_scan_specific_bus);
+void
+#else
+void __devinit
+#endif
+pcibios_scan_specific_bus(int busn)
 {
 	int devfn;
 	long node;
@@ -51,12 +67,11 @@ void __devinit pcibios_scan_specific_bus
 		    l != 0x0000 && l != 0xffff) {
 			DBG("Found device at %02x:%02x [%04x]\n", busn, devfn, l);
 			printk(KERN_INFO "PCI: Discovered peer bus %02x\n", busn);
-			pci_scan_bus_on_node(busn, &pci_root_ops, node);
+			_pci_scan_bus_on_node(busn, node);
 			return;
 		}
 	}
 }
-EXPORT_SYMBOL_GPL(pcibios_scan_specific_bus);
 
 int __init pci_subsys_init(void)
 {
--- 2.6.38-rc5/drivers/edac/Kconfig
+++ 2.6.38-rc5-x86-pci-section-conflict/drivers/edac/Kconfig
@@ -173,7 +173,7 @@ config EDAC_I5400
 
 config EDAC_I7CORE
 	tristate "Intel i7 Core (Nehalem) processors"
-	depends on EDAC_MM_EDAC && PCI && X86
+	depends on EDAC_MM_EDAC && PCI && X86 && HOTPLUG
 	select EDAC_MCE
 	help
 	  Support for error detection and correction the Intel

Re: [PATCH, resend] x86/PCI: don't export a __devinit function

[Yinghai Lu]

On Thu, Feb 17, 2011 at 8:08 AM, Jan Beulich <JBeulich@novell.com> wrote:
> Exporting a __devinit function (pcibios_scan_specific_bus()) isn't
> correct. (Michal, any reason why modpost only warns about exported
> __init functions?) Short of being able to think of a better solution,
> and short of making the whole call tree (reaching into the arch-
> independent part of the PCI subsystem) non-__devinit, export the
> symbol only when HOTPLUG is enabled (which is always the case for non-
> expert configurations), use section mismatch avoidance annotations for
> that case (knowing that __devinit functions will not be discarded),
> and mark the symbol __devinit only in the !HOTPLUG case.
>
> Consequently, EDAC_I7CORE (consuming the export) then has to depend on
> HOTPLUG. A fundamental question of course if whether this driver has
> to use that function in the first place (i.e. whether it wouldn't be
> better to just remove the export) - the problem it tries to address
> happens on other systems too, but the PCI bus the devices in question
> live on isn't necessarily bus 255. For the affected system I have, the
> alternative approach is to set pcibios_last_bus from __pci_mmcfg_init()
> based on the highest bus number on segment 0 being covered by MCFG.
>
> Signed-off-by: Jan Beulich <jbeulich@novell.com>
> Cc: Mauro Carvalho Chehab <mchehab@redhat.com>
> Cc: Aristeu Sergio <arozansk@redhat.com>
> Cc: Michal Marek <mmarek@suse.cz>
>
> ---
>  arch/x86/pci/legacy.c |   21 ++++++++++++++++++---
>  drivers/edac/Kconfig  |    2 +-
>  2 files changed, 19 insertions(+), 4 deletions(-)
>
> --- 2.6.38-rc5/arch/x86/pci/legacy.c
> +++ 2.6.38-rc5-x86-pci-section-conflict/arch/x86/pci/legacy.c
> @@ -36,7 +36,23 @@ int __init pci_legacy_init(void)
>        return 0;
>  }
>
> -void __devinit pcibios_scan_specific_bus(int busn)
> +#ifdef CONFIG_HOTPLUG
> +static void __ref
> +#else
> +static inline void
> +#endif
> +_pci_scan_bus_on_node(int busno, int node)
> +{
> +       pci_scan_bus_on_node(busno, &pci_root_ops, node);
> +}
> +
> +#ifdef CONFIG_HOTPLUG
> +EXPORT_SYMBOL_GPL(pcibios_scan_specific_bus);
> +void
> +#else
> +void __devinit
> +#endif

anyway to avoid or reduce those #ifdef?

Yinghai

> +pcibios_scan_specific_bus(int busn)
>  {
>        int devfn;
>        long node;
> @@ -51,12 +67,11 @@ void __devinit pcibios_scan_specific_bus
>                    l != 0x0000 && l != 0xffff) {
>                        DBG("Found device at %02x:%02x [%04x]\n", busn, devfn, l);
>                        printk(KERN_INFO "PCI: Discovered peer bus %02x\n", busn);
> -                       pci_scan_bus_on_node(busn, &pci_root_ops, node);
> +                       _pci_scan_bus_on_node(busn, node);
>                        return;
>                }
>        }
>  }
> -EXPORT_SYMBOL_GPL(pcibios_scan_specific_bus);
>
>  int __init pci_subsys_init(void)
>  {
> --- 2.6.38-rc5/drivers/edac/Kconfig
> +++ 2.6.38-rc5-x86-pci-section-conflict/drivers/edac/Kconfig
> @@ -173,7 +173,7 @@ config EDAC_I5400
>
>  config EDAC_I7CORE
>        tristate "Intel i7 Core (Nehalem) processors"
> -       depends on EDAC_MM_EDAC && PCI && X86
> +       depends on EDAC_MM_EDAC && PCI && X86 && HOTPLUG
>        select EDAC_MCE
>        help
>          Support for error detection and correction the Intel
>
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>

Re: [PATCH, resend] x86/PCI: don't export a __devinit function

[Jan Beulich]

>>> On 17.02.11 at 18:21, Yinghai Lu <yinghai@kernel.org> wrote:
> On Thu, Feb 17, 2011 at 8:08 AM, Jan Beulich <JBeulich@novell.com> wrote:
>> Exporting a __devinit function (pcibios_scan_specific_bus()) isn't
>> correct. (Michal, any reason why modpost only warns about exported
>> __init functions?) Short of being able to think of a better solution,
>> and short of making the whole call tree (reaching into the arch-
>> independent part of the PCI subsystem) non-__devinit, export the
>> symbol only when HOTPLUG is enabled (which is always the case for non-
>> expert configurations), use section mismatch avoidance annotations for
>> that case (knowing that __devinit functions will not be discarded),
>> and mark the symbol __devinit only in the !HOTPLUG case.
>>
>> Consequently, EDAC_I7CORE (consuming the export) then has to depend on
>> HOTPLUG. A fundamental question of course if whether this driver has
>> to use that function in the first place (i.e. whether it wouldn't be
>> better to just remove the export) - the problem it tries to address
>> happens on other systems too, but the PCI bus the devices in question
>> live on isn't necessarily bus 255. For the affected system I have, the
>> alternative approach is to set pcibios_last_bus from __pci_mmcfg_init()
>> based on the highest bus number on segment 0 being covered by MCFG.
>>
>> Signed-off-by: Jan Beulich <jbeulich@novell.com>
>> Cc: Mauro Carvalho Chehab <mchehab@redhat.com>
>> Cc: Aristeu Sergio <arozansk@redhat.com>
>> Cc: Michal Marek <mmarek@suse.cz>
>>
>> ---
>>  arch/x86/pci/legacy.c |   21 ++++++++++++++++++---
>>  drivers/edac/Kconfig  |    2 +-
>>  2 files changed, 19 insertions(+), 4 deletions(-)
>>
>> --- 2.6.38-rc5/arch/x86/pci/legacy.c
>> +++ 2.6.38-rc5-x86-pci-section-conflict/arch/x86/pci/legacy.c
>> @@ -36,7 +36,23 @@ int __init pci_legacy_init(void)
>>        return 0;
>>  }
>>
>> -void __devinit pcibios_scan_specific_bus(int busn)
>> +#ifdef CONFIG_HOTPLUG
>> +static void __ref
>> +#else
>> +static inline void
>> +#endif
>> +_pci_scan_bus_on_node(int busno, int node)
>> +{
>> +       pci_scan_bus_on_node(busno, &pci_root_ops, node);
>> +}
>> +
>> +#ifdef CONFIG_HOTPLUG
>> +EXPORT_SYMBOL_GPL(pcibios_scan_specific_bus);
>> +void
>> +#else
>> +void __devinit
>> +#endif
> 
> anyway to avoid or reduce those #ifdef?

Yes, they look ugly, but no, I have no better idea (other than
creating single-use abstractions that wouldn't look any better).

Jan

Re: [PATCH, resend] x86/PCI: don't export a __devinit function

[Mauro Carvalho Chehab]

Em 17-02-2011 14:08, Jan Beulich escreveu:
> Exporting a __devinit function (pcibios_scan_specific_bus()) isn't
> correct. (Michal, any reason why modpost only warns about exported
> __init functions?) Short of being able to think of a better solution,
> and short of making the whole call tree (reaching into the arch-
> independent part of the PCI subsystem) non-__devinit, export the
> symbol only when HOTPLUG is enabled (which is always the case for non-
> expert configurations), use section mismatch avoidance annotations for
> that case (knowing that __devinit functions will not be discarded),
> and mark the symbol __devinit only in the !HOTPLUG case.
> 
> Consequently, EDAC_I7CORE (consuming the export) then has to depend on
> HOTPLUG. 

Having the entire i7core_edac driver depending on HOTPLUG, just because
a few BIOSes want to hide the non-core PCI devices doesn't seem nice.
One alternative would be to enclose the code that needs this function
with #ifdef CONFIG_HOTPLUG.

> A fundamental question of course if whether this driver has
> to use that function in the first place (i.e. whether it wouldn't be
> better to just remove the export) - the problem it tries to address
> happens on other systems too, but the PCI bus the devices in question
> live on isn't necessarily bus 255. For the affected system I have, the
> alternative approach is to set pcibios_last_bus from __pci_mmcfg_init()
> based on the highest bus number on segment 0 being covered by MCFG.

I received a few days ago a report that some BIOSes that hide those
PCI devices also use a different address for the last bus (0x3f, instead
of 0xff). So, it seems that the better would be to use an alternative
way to retrieve the last bus.

> 
> Signed-off-by: Jan Beulich <jbeulich@novell.com>
> Cc: Mauro Carvalho Chehab <mchehab@redhat.com>
> Cc: Aristeu Sergio <arozansk@redhat.com>
> Cc: Michal Marek <mmarek@suse.cz>
> 
> ---
>  arch/x86/pci/legacy.c |   21 ++++++++++++++++++---
>  drivers/edac/Kconfig  |    2 +-
>  2 files changed, 19 insertions(+), 4 deletions(-)
> 
> --- 2.6.38-rc5/arch/x86/pci/legacy.c
> +++ 2.6.38-rc5-x86-pci-section-conflict/arch/x86/pci/legacy.c
> @@ -36,7 +36,23 @@ int __init pci_legacy_init(void)
>  	return 0;
>  }
>  
> -void __devinit pcibios_scan_specific_bus(int busn)
> +#ifdef CONFIG_HOTPLUG
> +static void __ref
> +#else
> +static inline void
> +#endif
> +_pci_scan_bus_on_node(int busno, int node)
> +{
> +	pci_scan_bus_on_node(busno, &pci_root_ops, node);
> +}
> +
> +#ifdef CONFIG_HOTPLUG
> +EXPORT_SYMBOL_GPL(pcibios_scan_specific_bus);
> +void
> +#else
> +void __devinit
> +#endif
> +pcibios_scan_specific_bus(int busn)
>  {
>  	int devfn;
>  	long node;
> @@ -51,12 +67,11 @@ void __devinit pcibios_scan_specific_bus
>  		    l != 0x0000 && l != 0xffff) {
>  			DBG("Found device at %02x:%02x [%04x]\n", busn, devfn, l);
>  			printk(KERN_INFO "PCI: Discovered peer bus %02x\n", busn);
> -			pci_scan_bus_on_node(busn, &pci_root_ops, node);
> +			_pci_scan_bus_on_node(busn, node);
>  			return;
>  		}
>  	}
>  }
> -EXPORT_SYMBOL_GPL(pcibios_scan_specific_bus);
>  
>  int __init pci_subsys_init(void)
>  {
> --- 2.6.38-rc5/drivers/edac/Kconfig
> +++ 2.6.38-rc5-x86-pci-section-conflict/drivers/edac/Kconfig
> @@ -173,7 +173,7 @@ config EDAC_I5400
>  
>  config EDAC_I7CORE
>  	tristate "Intel i7 Core (Nehalem) processors"
> -	depends on EDAC_MM_EDAC && PCI && X86
> +	depends on EDAC_MM_EDAC && PCI && X86 && HOTPLUG
>  	select EDAC_MCE
>  	help
>  	  Support for error detection and correction the Intel
> 
> 
> 

Re: [PATCH, resend] x86/PCI: don't export a __devinit function

[Yinghai Lu]

On Thu, Feb 17, 2011 at 10:12 AM, Mauro Carvalho Chehab
<mchehab@redhat.com> wrote:
> Em 17-02-2011 14:08, Jan Beulich escreveu:
>> Exporting a __devinit function (pcibios_scan_specific_bus()) isn't
>> correct. (Michal, any reason why modpost only warns about exported
>> __init functions?) Short of being able to think of a better solution,
>> and short of making the whole call tree (reaching into the arch-
>> independent part of the PCI subsystem) non-__devinit, export the
>> symbol only when HOTPLUG is enabled (which is always the case for non-
>> expert configurations), use section mismatch avoidance annotations for
>> that case (knowing that __devinit functions will not be discarded),
>> and mark the symbol __devinit only in the !HOTPLUG case.
>>
>> Consequently, EDAC_I7CORE (consuming the export) then has to depend on
>> HOTPLUG.
>
> Having the entire i7core_edac driver depending on HOTPLUG, just because
> a few BIOSes want to hide the non-core PCI devices doesn't seem nice.
> One alternative would be to enclose the code that needs this function
> with #ifdef CONFIG_HOTPLUG.
>
>> A fundamental question of course if whether this driver has
>> to use that function in the first place (i.e. whether it wouldn't be
>> better to just remove the export) - the problem it tries to address
>> happens on other systems too, but the PCI bus the devices in question
>> live on isn't necessarily bus 255. For the affected system I have, the
>> alternative approach is to set pcibios_last_bus from __pci_mmcfg_init()
>> based on the highest bus number on segment 0 being covered by MCFG.
>
> I received a few days ago a report that some BIOSes that hide those
> PCI devices also use a different address for the last bus (0x3f, instead
> of 0xff). So, it seems that the better would be to use an alternative
> way to retrieve the last bus.

just append "pci=lastbus=255" will get all those devices.

Yinghai

Re: [PATCH, resend] x86/PCI: don't export a __devinit function

[Mauro Carvalho Chehab]

Em 17-02-2011 21:12, Yinghai Lu escreveu:
> On Thu, Feb 17, 2011 at 10:12 AM, Mauro Carvalho Chehab
> <mchehab@redhat.com> wrote:
>> Em 17-02-2011 14:08, Jan Beulich escreveu:
>>> Exporting a __devinit function (pcibios_scan_specific_bus()) isn't
>>> correct. (Michal, any reason why modpost only warns about exported
>>> __init functions?) Short of being able to think of a better solution,
>>> and short of making the whole call tree (reaching into the arch-
>>> independent part of the PCI subsystem) non-__devinit, export the
>>> symbol only when HOTPLUG is enabled (which is always the case for non-
>>> expert configurations), use section mismatch avoidance annotations for
>>> that case (knowing that __devinit functions will not be discarded),
>>> and mark the symbol __devinit only in the !HOTPLUG case.
>>>
>>> Consequently, EDAC_I7CORE (consuming the export) then has to depend on
>>> HOTPLUG.
>>
>> Having the entire i7core_edac driver depending on HOTPLUG, just because
>> a few BIOSes want to hide the non-core PCI devices doesn't seem nice.
>> One alternative would be to enclose the code that needs this function
>> with #ifdef CONFIG_HOTPLUG.
>>
>>> A fundamental question of course if whether this driver has
>>> to use that function in the first place (i.e. whether it wouldn't be
>>> better to just remove the export) - the problem it tries to address
>>> happens on other systems too, but the PCI bus the devices in question
>>> live on isn't necessarily bus 255. For the affected system I have, the
>>> alternative approach is to set pcibios_last_bus from __pci_mmcfg_init()
>>> based on the highest bus number on segment 0 being covered by MCFG.
>>
>> I received a few days ago a report that some BIOSes that hide those
>> PCI devices also use a different address for the last bus (0x3f, instead
>> of 0xff). So, it seems that the better would be to use an alternative
>> way to retrieve the last bus.
> 
> just append "pci=lastbus=255" will get all those devices.

I know, but the better would be if this could be detected, instead of
relying on a modprobe parameter.

Cheers,
Mauro

Re: [PATCH, resend] x86/PCI: don't export a __devinit function

[Jan Beulich]

>>> On 17.02.11 at 19:12, Mauro Carvalho Chehab <mchehab@redhat.com> wrote:
> Em 17-02-2011 14:08, Jan Beulich escreveu:
>> Exporting a __devinit function (pcibios_scan_specific_bus()) isn't
>> correct. (Michal, any reason why modpost only warns about exported
>> __init functions?) Short of being able to think of a better solution,
>> and short of making the whole call tree (reaching into the arch-
>> independent part of the PCI subsystem) non-__devinit, export the
>> symbol only when HOTPLUG is enabled (which is always the case for non-
>> expert configurations), use section mismatch avoidance annotations for
>> that case (knowing that __devinit functions will not be discarded),
>> and mark the symbol __devinit only in the !HOTPLUG case.
>> 
>> Consequently, EDAC_I7CORE (consuming the export) then has to depend on
>> HOTPLUG. 
> 
> Having the entire i7core_edac driver depending on HOTPLUG, just because
> a few BIOSes want to hide the non-core PCI devices doesn't seem nice.
> One alternative would be to enclose the code that needs this function
> with #ifdef CONFIG_HOTPLUG.

Yes, I can certainly do it that way, unless the below could serve to
make the export unnecessary altogether.

>> A fundamental question of course if whether this driver has
>> to use that function in the first place (i.e. whether it wouldn't be
>> better to just remove the export) - the problem it tries to address
>> happens on other systems too, but the PCI bus the devices in question
>> live on isn't necessarily bus 255. For the affected system I have, the
>> alternative approach is to set pcibios_last_bus from __pci_mmcfg_init()
>> based on the highest bus number on segment 0 being covered by MCFG.
> 
> I received a few days ago a report that some BIOSes that hide those
> PCI devices also use a different address for the last bus (0x3f, instead
> of 0xff). So, it seems that the better would be to use an alternative
> way to retrieve the last bus.

Like this:

--- a/arch/x86/pci/mmconfig-shared.c
+++ b/arch/x86/pci/mmconfig-shared.c
@@ -606,6 +606,16 @@ static void __init __pci_mmcfg_init(int 
 	if (list_empty(&pci_mmcfg_list))
 		return;
 
+	if (pcibios_last_bus < 0) {
+		const struct pci_mmcfg_region *cfg;
+
+		list_for_each_entry(cfg, &pci_mmcfg_list, list) {
+			if (cfg->segment)
+				break;
+			pcibios_last_bus = cfg->end_bus;
+		}
+	}
+
 	if (pci_mmcfg_arch_init())
 		pci_probe = (pci_probe & ~PCI_PROBE_MASK) | PCI_PROBE_MMCONF;
 	else {

I (unsuccessfully so far) tried to find someone at Intel who could
confirm that this approach is usable namely on those Xeon 55xx
systems that the edac driver tries to deal with, which is why I
didn't submit this so far.

Jan

Re: [PATCH, resend] x86/PCI: don't export a __devinit function

[Jesse Barnes]

On Fri, 18 Feb 2011 09:12:11 +0000
"Jan Beulich" <JBeulich@novell.com> wrote:

> Like this:
> 
> --- a/arch/x86/pci/mmconfig-shared.c
> +++ b/arch/x86/pci/mmconfig-shared.c
> @@ -606,6 +606,16 @@ static void __init __pci_mmcfg_init(int 
>  	if (list_empty(&pci_mmcfg_list))
>  		return;
>  
> +	if (pcibios_last_bus < 0) {
> +		const struct pci_mmcfg_region *cfg;
> +
> +		list_for_each_entry(cfg, &pci_mmcfg_list, list) {
> +			if (cfg->segment)
> +				break;
> +			pcibios_last_bus = cfg->end_bus;
> +		}
> +	}
> +
>  	if (pci_mmcfg_arch_init())
>  		pci_probe = (pci_probe & ~PCI_PROBE_MASK) | PCI_PROBE_MMCONF;
>  	else {
> 
> I (unsuccessfully so far) tried to find someone at Intel who could
> confirm that this approach is usable namely on those Xeon 55xx
> systems that the edac driver tries to deal with, which is why I
> didn't submit this so far.

What's the actual user of pcibios_last_bus we're concerned about here?
Presumably pcibios_fixup_peer_bridges(), since that's the only place
that might be affected by a positive value...

So in that sense, the above seems like a good fix; we should initialize
pcibios_last_bus to a reasonable value based on the configuration we
discover in the MCFG table.

-- 
Jesse Barnes, Intel Open Source Technology Center