Due to the __weak annotation in the forward declaration
of the 'pcibios_get_phb_of_node' function GCC will emit
a weak symbol for this functions even if the actual
implementation does not use the weak attribute.
If an architecture tries to override the function
by providing its own implementation there will be
multiple weak symbols with the same name in the
object files. When the kernel is linked from the
object files the linking order determines which
implementation will be used in the final image.
On x86 and on powerpc the architecture specific
version gets used:
$ readelf -s arch/x86/kernel/built-in.o drivers/pci/built-in.o \
vmlinux.o | grep pcibios_get_phb_of_node
3338: 00029b80 86 FUNC WEAK DEFAULT 1 pcibios_get_phb_of_node
1701: 00012710 77 FUNC WEAK DEFAULT 1 pcibios_get_phb_of_node
52072: 0002a170 86 FUNC WEAK DEFAULT 1 pcibios_get_phb_of_node
$
$ powerpc-openwrt-linux-uclibc-readelf -s arch/powerpc/kernel/built-in.o \
drivers/pci/built-in.o vmlinux.o | grep pcibios_get_phb_of_node
1001: 0000cbb8 12 FUNC WEAK DEFAULT 1 pcibios_get_phb_of_node
1484: 0001471c 88 FUNC WEAK DEFAULT 1 pcibios_get_phb_of_node
28652: 0000d6f8 12 FUNC WEAK DEFAULT 1 pcibios_get_phb_of_node
$
However on MIPS, the linker puts the default
implementation into the final image:
$ mipsel-openwrt-linux-readelf -s arch/mips/pci/built-in.o \
drivers/pci/built-in.o vmlinux.o | grep pcibios_get_phb_of_node
86: 0000046c 12 FUNC WEAK DEFAULT 2 pcibios_get_phb_of_node
1430: 00012e2c 104 FUNC WEAK DEFAULT 2 pcibios_get_phb_of_node
31898: 0017e4ec 104 FUNC WEAK DEFAULT 2 pcibios_get_phb_of_node
$
Rename the default implementation and remove the
__weak annotation of that. This ensures that there
will be no multiple weak symbols with the same name
in the object files. In order to keep the expected
behaviour, call the architecture specific function
if the weak symbol is resolved.
Also move the renamed function to the top instead
of adding a new forward declaration for that.
Signed-off-by: Gabor Juhos <[email protected]>
---
Notes:
Unfortunately I'm not a binutils/gcc expert, so
I don't know if this is the expected behaviour
of those or not.
Removing the __weak annotation from the forward
declaration of 'pcibios_get_phb_of_node' in
'include/linux/pci.h' also fixes the problem.
The microblaze architecture also provides its own
implementation. The behaviour of that is not tested
but I assume that the linker chooses the arch specific
implementation on that as well similarly to the
x86/powerpc.
The MIPS version is implemented in the followup
patch.
Removing the __weak annotation from the forward
declaration of 'pcibios_get_phb_of_node' in
'include/linux/pci.h' also fixes the problem.
-Gabor
---
drivers/pci/of.c | 41 +++++++++++++++++++++++------------------
1 file changed, 23 insertions(+), 18 deletions(-)
diff --git a/drivers/pci/of.c b/drivers/pci/of.c
index f092993..5794840 100644
--- a/drivers/pci/of.c
+++ b/drivers/pci/of.c
@@ -15,10 +15,32 @@
#include <linux/of_pci.h>
#include "pci.h"
+static struct device_node *__pcibios_get_phb_of_node(struct pci_bus *bus)
+{
+ /* This should only be called for PHBs */
+ if (WARN_ON(bus->self || bus->parent))
+ return NULL;
+
+ if (pcibios_get_phb_of_node)
+ return pcibios_get_phb_of_node(bus);
+
+ /* Look for a node pointer in either the intermediary device we
+ * create above the root bus or it's own parent. Normally only
+ * the later is populated.
+ */
+ if (bus->bridge->of_node)
+ return of_node_get(bus->bridge->of_node);
+ if (bus->bridge->parent && bus->bridge->parent->of_node)
+ return of_node_get(bus->bridge->parent->of_node);
+
+ return NULL;
+}
+
void pci_set_of_node(struct pci_dev *dev)
{
if (!dev->bus->dev.of_node)
return;
+
dev->dev.of_node = of_pci_find_child_device(dev->bus->dev.of_node,
dev->devfn);
}
@@ -32,7 +54,7 @@ void pci_release_of_node(struct pci_dev *dev)
void pci_set_bus_of_node(struct pci_bus *bus)
{
if (bus->self == NULL)
- bus->dev.of_node = pcibios_get_phb_of_node(bus);
+ bus->dev.of_node = __pcibios_get_phb_of_node(bus);
else
bus->dev.of_node = of_node_get(bus->self->dev.of_node);
}
@@ -42,20 +64,3 @@ void pci_release_bus_of_node(struct pci_bus *bus)
of_node_put(bus->dev.of_node);
bus->dev.of_node = NULL;
}
-
-struct device_node * __weak pcibios_get_phb_of_node(struct pci_bus *bus)
-{
- /* This should only be called for PHBs */
- if (WARN_ON(bus->self || bus->parent))
- return NULL;
-
- /* Look for a node pointer in either the intermediary device we
- * create above the root bus or it's own parent. Normally only
- * the later is populated.
- */
- if (bus->bridge->of_node)
- return of_node_get(bus->bridge->of_node);
- if (bus->bridge->parent && bus->bridge->parent->of_node)
- return of_node_get(bus->bridge->parent->of_node);
- return NULL;
-}
--
1.7.10
The of_node field of the device assigned to a
PCI bus is used during scanning of the PCI bus.
However on MIPS, the of_node field is assigned
only after the bus has been scanned.
Implement the architecture specific version of
'pcibios_get_phb_of_node'. Which ensures that the
PCI driver core will initialize the of_node field
before starting the scan.
Also remove the local assignment of bus->dev.of_node,
it is not needed after the patch.
Signed-off-by: Gabor Juhos <[email protected]>
---
arch/mips/pci/pci.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/arch/mips/pci/pci.c b/arch/mips/pci/pci.c
index 0872f12..594e60d 100644
--- a/arch/mips/pci/pci.c
+++ b/arch/mips/pci/pci.c
@@ -115,7 +115,6 @@ static void pcibios_scanbus(struct pci_controller *hose)
pci_bus_assign_resources(bus);
pci_enable_bridges(bus);
}
- bus->dev.of_node = hose->of_node;
}
}
@@ -169,6 +168,13 @@ void pci_load_of_ranges(struct pci_controller *hose, struct device_node *node)
}
}
}
+
+struct device_node *pcibios_get_phb_of_node(struct pci_bus *bus)
+{
+ struct pci_controller *hose = bus->sysdata;
+
+ return of_node_get(hose->of_node);
+}
#endif
static DEFINE_MUTEX(pci_scan_mutex);
--
1.7.10
On Thu, Apr 4, 2013 at 12:01 PM, Gabor Juhos <[email protected]> wrote:
> Due to the __weak annotation in the forward declaration
> of the 'pcibios_get_phb_of_node' function GCC will emit
> a weak symbol for this functions even if the actual
> implementation does not use the weak attribute.
>
> If an architecture tries to override the function
> by providing its own implementation there will be
> multiple weak symbols with the same name in the
> object files. When the kernel is linked from the
> object files the linking order determines which
> implementation will be used in the final image.
>
> On x86 and on powerpc the architecture specific
> version gets used:
>
> $ readelf -s arch/x86/kernel/built-in.o drivers/pci/built-in.o \
> vmlinux.o | grep pcibios_get_phb_of_node
> 3338: 00029b80 86 FUNC WEAK DEFAULT 1 pcibios_get_phb_of_node
> 1701: 00012710 77 FUNC WEAK DEFAULT 1 pcibios_get_phb_of_node
> 52072: 0002a170 86 FUNC WEAK DEFAULT 1 pcibios_get_phb_of_node
> $
>
> $ powerpc-openwrt-linux-uclibc-readelf -s arch/powerpc/kernel/built-in.o \
> drivers/pci/built-in.o vmlinux.o | grep pcibios_get_phb_of_node
> 1001: 0000cbb8 12 FUNC WEAK DEFAULT 1 pcibios_get_phb_of_node
> 1484: 0001471c 88 FUNC WEAK DEFAULT 1 pcibios_get_phb_of_node
> 28652: 0000d6f8 12 FUNC WEAK DEFAULT 1 pcibios_get_phb_of_node
> $
>
> However on MIPS, the linker puts the default
> implementation into the final image:
>
> $ mipsel-openwrt-linux-readelf -s arch/mips/pci/built-in.o \
> drivers/pci/built-in.o vmlinux.o | grep pcibios_get_phb_of_node
> 86: 0000046c 12 FUNC WEAK DEFAULT 2 pcibios_get_phb_of_node
> 1430: 00012e2c 104 FUNC WEAK DEFAULT 2 pcibios_get_phb_of_node
> 31898: 0017e4ec 104 FUNC WEAK DEFAULT 2 pcibios_get_phb_of_node
> $
>
> Rename the default implementation and remove the
> __weak annotation of that. This ensures that there
> will be no multiple weak symbols with the same name
> in the object files. In order to keep the expected
> behaviour, call the architecture specific function
> if the weak symbol is resolved.
>
> Also move the renamed function to the top instead
> of adding a new forward declaration for that.
>
> Signed-off-by: Gabor Juhos <[email protected]>
> ---
> Notes:
>
> Unfortunately I'm not a binutils/gcc expert, so
> I don't know if this is the expected behaviour
> of those or not.
>
> Removing the __weak annotation from the forward
> declaration of 'pcibios_get_phb_of_node' in
> 'include/linux/pci.h' also fixes the problem.
>
> The microblaze architecture also provides its own
> implementation. The behaviour of that is not tested
> but I assume that the linker chooses the arch specific
> implementation on that as well similarly to the
> x86/powerpc.
>
> The MIPS version is implemented in the followup
> patch.
>
> Removing the __weak annotation from the forward
> declaration of 'pcibios_get_phb_of_node' in
> 'include/linux/pci.h' also fixes the problem.
I think removing the __weak annotation from the declaration in
include/linux/pci.h is the correct solution.
__weak is an attribute of a function definition, not an attribute of
the interface, so I don't think it makes any sense to have it in
pci.h.
Bjorn
> -Gabor
> ---
> drivers/pci/of.c | 41 +++++++++++++++++++++++------------------
> 1 file changed, 23 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/pci/of.c b/drivers/pci/of.c
> index f092993..5794840 100644
> --- a/drivers/pci/of.c
> +++ b/drivers/pci/of.c
> @@ -15,10 +15,32 @@
> #include <linux/of_pci.h>
> #include "pci.h"
>
> +static struct device_node *__pcibios_get_phb_of_node(struct pci_bus *bus)
> +{
> + /* This should only be called for PHBs */
> + if (WARN_ON(bus->self || bus->parent))
> + return NULL;
> +
> + if (pcibios_get_phb_of_node)
> + return pcibios_get_phb_of_node(bus);
> +
> + /* Look for a node pointer in either the intermediary device we
> + * create above the root bus or it's own parent. Normally only
> + * the later is populated.
> + */
> + if (bus->bridge->of_node)
> + return of_node_get(bus->bridge->of_node);
> + if (bus->bridge->parent && bus->bridge->parent->of_node)
> + return of_node_get(bus->bridge->parent->of_node);
> +
> + return NULL;
> +}
> +
> void pci_set_of_node(struct pci_dev *dev)
> {
> if (!dev->bus->dev.of_node)
> return;
> +
> dev->dev.of_node = of_pci_find_child_device(dev->bus->dev.of_node,
> dev->devfn);
> }
> @@ -32,7 +54,7 @@ void pci_release_of_node(struct pci_dev *dev)
> void pci_set_bus_of_node(struct pci_bus *bus)
> {
> if (bus->self == NULL)
> - bus->dev.of_node = pcibios_get_phb_of_node(bus);
> + bus->dev.of_node = __pcibios_get_phb_of_node(bus);
> else
> bus->dev.of_node = of_node_get(bus->self->dev.of_node);
> }
> @@ -42,20 +64,3 @@ void pci_release_bus_of_node(struct pci_bus *bus)
> of_node_put(bus->dev.of_node);
> bus->dev.of_node = NULL;
> }
> -
> -struct device_node * __weak pcibios_get_phb_of_node(struct pci_bus *bus)
> -{
> - /* This should only be called for PHBs */
> - if (WARN_ON(bus->self || bus->parent))
> - return NULL;
> -
> - /* Look for a node pointer in either the intermediary device we
> - * create above the root bus or it's own parent. Normally only
> - * the later is populated.
> - */
> - if (bus->bridge->of_node)
> - return of_node_get(bus->bridge->of_node);
> - if (bus->bridge->parent && bus->bridge->parent->of_node)
> - return of_node_get(bus->bridge->parent->of_node);
> - return NULL;
> -}
> --
> 1.7.10
>
On Thu, Apr 4, 2013 at 12:01 PM, Gabor Juhos <[email protected]> wrote:
> The of_node field of the device assigned to a
> PCI bus is used during scanning of the PCI bus.
> However on MIPS, the of_node field is assigned
> only after the bus has been scanned.
>
> Implement the architecture specific version of
> 'pcibios_get_phb_of_node'. Which ensures that the
> PCI driver core will initialize the of_node field
> before starting the scan.
>
> Also remove the local assignment of bus->dev.of_node,
> it is not needed after the patch.
>
> Signed-off-by: Gabor Juhos <[email protected]>
I removed the __weak annotation from include/linux/pci.h and applied
this patch to my pci/gabor-get-of-node. Give it a try and make sure
it solves your problem. If so, and Ralph approves, I can push both
for v3.10. It should appear at
http://git.kernel.org/cgit/linux/kernel/git/helgaas/pci.git/log/?h=pci/gabor-get-of-node
soon.
Or if you prefer, you can take them through the MIPS tree.
Bjorn
> ---
> arch/mips/pci/pci.c | 8 +++++++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/arch/mips/pci/pci.c b/arch/mips/pci/pci.c
> index 0872f12..594e60d 100644
> --- a/arch/mips/pci/pci.c
> +++ b/arch/mips/pci/pci.c
> @@ -115,7 +115,6 @@ static void pcibios_scanbus(struct pci_controller *hose)
> pci_bus_assign_resources(bus);
> pci_enable_bridges(bus);
> }
> - bus->dev.of_node = hose->of_node;
> }
> }
>
> @@ -169,6 +168,13 @@ void pci_load_of_ranges(struct pci_controller *hose, struct device_node *node)
> }
> }
> }
> +
> +struct device_node *pcibios_get_phb_of_node(struct pci_bus *bus)
> +{
> + struct pci_controller *hose = bus->sysdata;
> +
> + return of_node_get(hose->of_node);
> +}
> #endif
>
> static DEFINE_MUTEX(pci_scan_mutex);
> --
> 1.7.10
>
2013.04.10. 18:31 keltez?ssel, Bjorn Helgaas ?rta:
> On Thu, Apr 4, 2013 at 12:01 PM, Gabor Juhos <[email protected]> wrote:
>> The of_node field of the device assigned to a
>> PCI bus is used during scanning of the PCI bus.
>> However on MIPS, the of_node field is assigned
>> only after the bus has been scanned.
>>
>> Implement the architecture specific version of
>> 'pcibios_get_phb_of_node'. Which ensures that the
>> PCI driver core will initialize the of_node field
>> before starting the scan.
>>
>> Also remove the local assignment of bus->dev.of_node,
>> it is not needed after the patch.
>>
>> Signed-off-by: Gabor Juhos <[email protected]>
>
> I removed the __weak annotation from include/linux/pci.h and applied
> this patch to my pci/gabor-get-of-node.
Thank you!
> Give it a try and make sure
> it solves your problem. If so, and Ralph approves, I can push both
> for v3.10. It should appear at
> http://git.kernel.org/cgit/linux/kernel/git/helgaas/pci.git/log/?h=pci/gabor-get-of-node
> soon.
I have tried your patch on top of 3.9-rc6. The resulting kernel uses the
architecture specific implementation, and it runs fine.
$ mipsel-openwrt-linux-readelf -s arch/mips/pci/built-in.o \
drivers/pci/built-in.o vmlinux.o | grep pcibios_get_phb_of_node
93: 0000046c 12 FUNC GLOBAL DEFAULT 2 pcibios_get_phb_of_node
1433: 00012a2c 104 FUNC WEAK DEFAULT 2 pcibios_get_phb_of_node
31863: 001d4dbc 12 FUNC GLOBAL DEFAULT 2 pcibios_get_phb_of_node
$
For completeness, I have compiled it for X64 and for powerpc as well. I did not
try to run these kernels, but the output of readelf seems to be ok:
$ readelf -s arch/x86/kernel/built-in.o drivers/pci/built-in.o vmlinux.o | \
grep pcibios_get_phb_of_node
2761: 000273a0 86 FUNC GLOBAL DEFAULT 1 pcibios_get_phb_of_node
1705: 00018770 77 FUNC WEAK DEFAULT 1 pcibios_get_phb_of_node
60364: 000278a0 86 FUNC GLOBAL DEFAULT 1 pcibios_get_phb_of_node
$
$ powerpc-openwrt-linux-readelf -s arch/powerpc/kernel/built-in.o \
drivers/pci/built-in.o vmlinux.o | grep pcibios_get_phb_of_node
1002: 0000ca28 12 FUNC GLOBAL DEFAULT 1 pcibios_get_phb_of_node
1485: 0001453c 88 FUNC WEAK DEFAULT 1 pcibios_get_phb_of_node
28959: 0000d598 12 FUNC GLOBAL DEFAULT 1 pcibios_get_phb_of_node
$
> Or if you prefer, you can take them through the MIPS tree.
Either is fine.
-Gabor
On Thu, Apr 11, 2013 at 11:32 AM, Gabor Juhos <[email protected]> wrote:
> 2013.04.10. 18:31 keltez?ssel, Bjorn Helgaas ?rta:
>> On Thu, Apr 4, 2013 at 12:01 PM, Gabor Juhos <[email protected]> wrote:
>>> The of_node field of the device assigned to a
>>> PCI bus is used during scanning of the PCI bus.
>>> However on MIPS, the of_node field is assigned
>>> only after the bus has been scanned.
>>>
>>> Implement the architecture specific version of
>>> 'pcibios_get_phb_of_node'. Which ensures that the
>>> PCI driver core will initialize the of_node field
>>> before starting the scan.
>>>
>>> Also remove the local assignment of bus->dev.of_node,
>>> it is not needed after the patch.
>>>
>>> Signed-off-by: Gabor Juhos <[email protected]>
>>
>> I removed the __weak annotation from include/linux/pci.h and applied
>> this patch to my pci/gabor-get-of-node.
>
> Thank you!
>
>> Give it a try and make sure
>> it solves your problem. If so, and Ralph approves, I can push both
>> for v3.10. It should appear at
>> http://git.kernel.org/cgit/linux/kernel/git/helgaas/pci.git/log/?h=pci/gabor-get-of-node
>> soon.
>
> I have tried your patch on top of 3.9-rc6. The resulting kernel uses the
> architecture specific implementation, and it runs fine.
>
> $ mipsel-openwrt-linux-readelf -s arch/mips/pci/built-in.o \
> drivers/pci/built-in.o vmlinux.o | grep pcibios_get_phb_of_node
> 93: 0000046c 12 FUNC GLOBAL DEFAULT 2 pcibios_get_phb_of_node
> 1433: 00012a2c 104 FUNC WEAK DEFAULT 2 pcibios_get_phb_of_node
> 31863: 001d4dbc 12 FUNC GLOBAL DEFAULT 2 pcibios_get_phb_of_node
> $
>
> For completeness, I have compiled it for X64 and for powerpc as well. I did not
> try to run these kernels, but the output of readelf seems to be ok:
>
> $ readelf -s arch/x86/kernel/built-in.o drivers/pci/built-in.o vmlinux.o | \
> grep pcibios_get_phb_of_node
> 2761: 000273a0 86 FUNC GLOBAL DEFAULT 1 pcibios_get_phb_of_node
> 1705: 00018770 77 FUNC WEAK DEFAULT 1 pcibios_get_phb_of_node
> 60364: 000278a0 86 FUNC GLOBAL DEFAULT 1 pcibios_get_phb_of_node
> $
>
> $ powerpc-openwrt-linux-readelf -s arch/powerpc/kernel/built-in.o \
> drivers/pci/built-in.o vmlinux.o | grep pcibios_get_phb_of_node
> 1002: 0000ca28 12 FUNC GLOBAL DEFAULT 1 pcibios_get_phb_of_node
> 1485: 0001453c 88 FUNC WEAK DEFAULT 1 pcibios_get_phb_of_node
> 28959: 0000d598 12 FUNC GLOBAL DEFAULT 1 pcibios_get_phb_of_node
> $
>
>> Or if you prefer, you can take them through the MIPS tree.
>
> Either is fine.
Thanks for checking these out! I put them in my "next" branch and
pushed it, so they should appear in v3.10.
Bjorn
2013.04.11. 19:35 keltez?ssel, Bjorn Helgaas ?rta:
> Thanks for checking these out! I put them in my "next" branch and
> pushed it, so they should appear in v3.10.
Great. Thank you!
-Gabor