These patches don't fix any problem I'm aware of, but I think they make the
code easier to analyze, and they reduce the likelihood of issues if MIPS
ever builds a multi-platform kernel.
Weak function declarations in header files are hard to use safely because
they make every definition weak. If the linker sees multiple weak
definitions, it silently chooses one based on link order. That's not a
very obvious criterion, and it can easily lead to running the wrong
version.
These patches remove the weak attribute from function declarations and
rework the code to match. I don't have any of these platforms, so I can't
test them, but my intent is that these should cause no functional change.
Changes from v1-v2:
- Rework vpe_run() patch to simply drop "weak" and the test for definition
- #include <asm/irq.h> in kernel/time.c instead of #ifdef MIPS_CPU_IRQ_BASE
- Add Reviewed-by from James for unchanged patches
---
Bjorn Helgaas (8):
MIPS: CPC: Remove "weak" from mips_cpc_phys_base() and make it static
MIPS: Remove "weak" from platform_maar_init() declaration
MIPS: MT: Remove "weak" from vpe_run() declaration
MIPS: Remove "weak" from get_c0_perfcount_int() declaration
MIPS: Remove "weak" from get_c0_compare_int() declaration
MIPS: Remove "weak" from get_c0_fdc_int() declaration
MIPS: Remove "weak" from mips_cdmm_phys_base() declaration
MIPS: Remove "__weak" definition from arch-specific linkage.h
arch/mips/include/asm/cdmm.h | 4 ++--
arch/mips/include/asm/irq.h | 2 +-
arch/mips/include/asm/linkage.h | 1 -
arch/mips/include/asm/maar.h | 2 +-
arch/mips/include/asm/mips-cpc.h | 10 ----------
arch/mips/include/asm/time.h | 4 ++--
arch/mips/include/asm/vpe.h | 2 +-
arch/mips/kernel/cevt-r4k.c | 11 +++++++----
arch/mips/kernel/mips-cpc.c | 9 ++++++++-
arch/mips/kernel/perf_event_mipsxx.c | 7 +------
arch/mips/kernel/time.c | 9 ++++++++-
arch/mips/kernel/vpe.c | 2 +-
arch/mips/oprofile/op_model_mipsxx.c | 8 +-------
drivers/bus/mips_cdmm.c | 14 +++++++++++++-
drivers/tty/mips_ejtag_fdc.c | 9 ++++++---
15 files changed, 52 insertions(+), 42 deletions(-)
There's only one implementation of mips_cpc_phys_base(), and it's only used
within the same file, so it doesn't need to be weak, and it doesn't need an
extern declaration.
Remove the extern mips_cpc_phys_base() declaration and make it static.
Signed-off-by: Bjorn Helgaas <[email protected]>
CC: [email protected]
---
arch/mips/include/asm/mips-cpc.h | 10 ----------
arch/mips/kernel/mips-cpc.c | 9 ++++++++-
2 files changed, 8 insertions(+), 11 deletions(-)
diff --git a/arch/mips/include/asm/mips-cpc.h b/arch/mips/include/asm/mips-cpc.h
index 1cebe8c..f386f32 100644
--- a/arch/mips/include/asm/mips-cpc.h
+++ b/arch/mips/include/asm/mips-cpc.h
@@ -28,16 +28,6 @@ extern void __iomem *mips_cpc_base;
extern phys_addr_t mips_cpc_default_phys_base(void);
/**
- * mips_cpc_phys_base - retrieve the physical base address of the CPC
- *
- * This function returns the physical base address of the Cluster Power
- * Controller memory mapped registers, or 0 if no Cluster Power Controller
- * is present. It may be overriden by individual platforms which determine
- * this address in a different way.
- */
-extern phys_addr_t __weak mips_cpc_phys_base(void);
-
-/**
* mips_cpc_probe - probe for a Cluster Power Controller
*
* Attempt to detect the presence of a Cluster Power Controller. Returns 0 if
diff --git a/arch/mips/kernel/mips-cpc.c b/arch/mips/kernel/mips-cpc.c
index 1196450..7e9ea9b 100644
--- a/arch/mips/kernel/mips-cpc.c
+++ b/arch/mips/kernel/mips-cpc.c
@@ -21,7 +21,14 @@ static DEFINE_PER_CPU_ALIGNED(spinlock_t, cpc_core_lock);
static DEFINE_PER_CPU_ALIGNED(unsigned long, cpc_core_lock_flags);
-phys_addr_t __weak mips_cpc_phys_base(void)
+/**
+ * mips_cpc_phys_base - retrieve the physical base address of the CPC
+ *
+ * This function returns the physical base address of the Cluster Power
+ * Controller memory mapped registers, or 0 if no Cluster Power Controller
+ * is present.
+ */
+static phys_addr_t mips_cpc_phys_base(void)
{
u32 cpc_base;
Weak header file declarations are error-prone because they make every
definition weak, and the linker chooses one based on link order (see
10629d711ed7 ("PCI: Remove __weak annotation from pcibios_get_phb_of_node
decl")).
platform_maar_init() is defined in:
- arch/mips/mm/init.c (where it is marked "weak")
- arch/mips/mti-malta/malta-memory.c (without annotation)
The "weak" attribute on the platform_maar_init() extern declaration applies
to the platform-specific definition in arch/mips/mti-malta/malta-memory.c,
so both definitions are weak, and which one we get depends on link order.
Remove the "weak" attribute from the declaration. That makes the malta
definition strong, so it will always be preferred if it is present.
Signed-off-by: Bjorn Helgaas <[email protected]>
Reviewed-by: James Hogan <[email protected]>
---
arch/mips/include/asm/maar.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/mips/include/asm/maar.h b/arch/mips/include/asm/maar.h
index 6c62b0f..b02891f 100644
--- a/arch/mips/include/asm/maar.h
+++ b/arch/mips/include/asm/maar.h
@@ -26,7 +26,7 @@
*
* Return: The number of MAAR pairs configured.
*/
-unsigned __weak platform_maar_init(unsigned num_pairs);
+unsigned platform_maar_init(unsigned num_pairs);
/**
* write_maar_pair() - write to a pair of MAARs
Weak header file declarations are error-prone because they make every
definition weak, and the linker chooses one based on link order (see
10629d711ed7 ("PCI: Remove __weak annotation from pcibios_get_phb_of_node
decl")).
That's not a problem for vpe_run() because Kconfig ensures there's exactly
one definition if we build vpe.o:
- vpe_run() is defined in arch/mips/kernel/vpe-mt.c if
CONFIG_MIPS_VPE_LOADER_MT=y
- vpe_run() is defined in arch/mips/mti-malta/malta-amon.c if
CONFIG_MIPS_VPE_LOADER_CMP=y
- CONFIG_MIPS_VPE_LOADER_MT and CONFIG_MIPS_VPE_LOADER_CMP cannot both be
set (MIPS_VPE_LOADER_CMP depends on MIPS_VPE_LOADER && MIPS_CMP;
MIPS_VPE_LOADER_MT depends on MIPS_VPE_LOADER && !MIPS_CMP)
Remove "weak" from the vpe_run() declaration and don't test whether
vpe_run() is defined.
Simplification-by: James Hogan <[email protected]>
Signed-off-by: Bjorn Helgaas <[email protected]>
---
arch/mips/include/asm/vpe.h | 2 +-
arch/mips/kernel/vpe.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/mips/include/asm/vpe.h b/arch/mips/include/asm/vpe.h
index 7849f39..80e70db 100644
--- a/arch/mips/include/asm/vpe.h
+++ b/arch/mips/include/asm/vpe.h
@@ -122,7 +122,7 @@ void release_vpe(struct vpe *v);
void *alloc_progmem(unsigned long len);
void release_progmem(void *ptr);
-int __weak vpe_run(struct vpe *v);
+int vpe_run(struct vpe *v);
void cleanup_tc(struct tc *tc);
int __init vpe_module_init(void);
diff --git a/arch/mips/kernel/vpe.c b/arch/mips/kernel/vpe.c
index 11da314..1fd05b5 100644
--- a/arch/mips/kernel/vpe.c
+++ b/arch/mips/kernel/vpe.c
@@ -827,7 +827,7 @@ static int vpe_release(struct inode *inode, struct file *filp)
hdr = (Elf_Ehdr *) v->pbuffer;
if (memcmp(hdr->e_ident, ELFMAG, SELFMAG) == 0) {
- if ((vpe_elfload(v) >= 0) && vpe_run) {
+ if (vpe_elfload(v) >= 0) {
vpe_run(v);
} else {
pr_warn("VPE loader: ELF load failed.\n");
Weak header file declarations are error-prone because they make every
definition weak, and the linker chooses one based on link order (see
10629d711ed7 ("PCI: Remove __weak annotation from pcibios_get_phb_of_node
decl")).
get_c0_perfcount_int() is defined in several files. Every definition is
weak, so I assume Kconfig prevents two or more from being included. The
callers contain identical default code used when get_c0_perfcount_int()
isn't defined at all.
Add a weak get_c0_perfcount_int() definition with the default code and
remove the weak annotation from the declaration.
Then the platform implementations will be strong and will override the weak
default. If multiple platforms are ever configured in, we'll get a link
error instead of calling a random platform's implementation.
Signed-off-by: Bjorn Helgaas <[email protected]>
CC: Andrew Bresticker <[email protected]>
---
arch/mips/include/asm/time.h | 2 +-
arch/mips/kernel/perf_event_mipsxx.c | 7 +------
arch/mips/kernel/time.c | 9 ++++++++-
arch/mips/oprofile/op_model_mipsxx.c | 8 +-------
4 files changed, 11 insertions(+), 15 deletions(-)
diff --git a/arch/mips/include/asm/time.h b/arch/mips/include/asm/time.h
index 8ab2874..ce6a7d5 100644
--- a/arch/mips/include/asm/time.h
+++ b/arch/mips/include/asm/time.h
@@ -46,7 +46,7 @@ extern unsigned int mips_hpt_frequency;
* so it lives here.
*/
extern int (*perf_irq)(void);
-extern int __weak get_c0_perfcount_int(void);
+extern int get_c0_perfcount_int(void);
/*
* Initialize the calling CPU's compare interrupt as clockevent device
diff --git a/arch/mips/kernel/perf_event_mipsxx.c b/arch/mips/kernel/perf_event_mipsxx.c
index cc1b6fa..c126b1c 100644
--- a/arch/mips/kernel/perf_event_mipsxx.c
+++ b/arch/mips/kernel/perf_event_mipsxx.c
@@ -1682,12 +1682,7 @@ init_hw_perf_events(void)
counters = counters_total_to_per_cpu(counters);
#endif
- if (get_c0_perfcount_int)
- irq = get_c0_perfcount_int();
- else if (cp0_perfcount_irq >= 0)
- irq = MIPS_CPU_IRQ_BASE + cp0_perfcount_irq;
- else
- irq = -1;
+ irq = get_c0_perfcount_int();
mipspmu.map_raw_event = mipsxx_pmu_map_raw_event;
diff --git a/arch/mips/kernel/time.c b/arch/mips/kernel/time.c
index 8d01709..6fff600 100644
--- a/arch/mips/kernel/time.c
+++ b/arch/mips/kernel/time.c
@@ -27,6 +27,7 @@
#include <asm/cpu-type.h>
#include <asm/div64.h>
#include <asm/time.h>
+#include <asm/irq.h>
/*
* forward reference
@@ -55,9 +56,15 @@ static int null_perf_irq(void)
}
int (*perf_irq)(void) = null_perf_irq;
-
EXPORT_SYMBOL(perf_irq);
+int __weak get_c0_perfcount_int(void)
+{
+ if (cp0_perfcount_irq >= 0)
+ return MIPS_CPU_IRQ_BASE + cp0_perfcount_irq;
+ return -1;
+}
+
/*
* time_init() - it does the following things.
*
diff --git a/arch/mips/oprofile/op_model_mipsxx.c b/arch/mips/oprofile/op_model_mipsxx.c
index 6a6e2cc..c0cffa9 100644
--- a/arch/mips/oprofile/op_model_mipsxx.c
+++ b/arch/mips/oprofile/op_model_mipsxx.c
@@ -438,13 +438,7 @@ static int __init mipsxx_init(void)
save_perf_irq = perf_irq;
perf_irq = mipsxx_perfcount_handler;
- if (get_c0_perfcount_int)
- perfcount_irq = get_c0_perfcount_int();
- else if (cp0_perfcount_irq >= 0)
- perfcount_irq = MIPS_CPU_IRQ_BASE + cp0_perfcount_irq;
- else
- perfcount_irq = -1;
-
+ perfcount_irq = get_c0_perfcount_int();
if (perfcount_irq >= 0)
return request_irq(perfcount_irq, mipsxx_perfcount_int,
IRQF_PERCPU | IRQF_NOBALANCING |
Weak header file declarations are error-prone because they make every
definition weak, and the linker chooses one based on link order (see
10629d711ed7 ("PCI: Remove __weak annotation from pcibios_get_phb_of_node
decl")).
get_c0_compare_int() is defined in several files. Each definition is weak,
so I assume Kconfig prevents two or more from being included. The caller
contains default code used when get_c0_compare_int() isn't defined at all.
Add a weak get_c0_compare_int() definition with the default code and remove
the weak annotation from the declaration.
Then the platform implementations will be strong and will override the weak
default. If multiple platforms are ever configured in, we'll get a link
error instead of calling a random platform's implementation.
Signed-off-by: Bjorn Helgaas <[email protected]>
Reviewed-by: James Hogan <[email protected]>
---
arch/mips/include/asm/time.h | 2 +-
arch/mips/kernel/cevt-r4k.c | 11 +++++++----
2 files changed, 8 insertions(+), 5 deletions(-)
diff --git a/arch/mips/include/asm/time.h b/arch/mips/include/asm/time.h
index ce6a7d5..44a9c3a 100644
--- a/arch/mips/include/asm/time.h
+++ b/arch/mips/include/asm/time.h
@@ -51,7 +51,7 @@ extern int get_c0_perfcount_int(void);
/*
* Initialize the calling CPU's compare interrupt as clockevent device
*/
-extern unsigned int __weak get_c0_compare_int(void);
+extern unsigned int get_c0_compare_int(void);
extern int r4k_clockevent_init(void);
static inline int mips_clockevent_init(void)
diff --git a/arch/mips/kernel/cevt-r4k.c b/arch/mips/kernel/cevt-r4k.c
index d70c4d8..cc7cc46 100644
--- a/arch/mips/kernel/cevt-r4k.c
+++ b/arch/mips/kernel/cevt-r4k.c
@@ -174,6 +174,11 @@ int c0_compare_int_usable(void)
return 1;
}
+unsigned int __weak get_c0_compare_int(void)
+{
+ return MIPS_CPU_IRQ_BASE + cp0_compare_irq;
+}
+
int r4k_clockevent_init(void)
{
unsigned int cpu = smp_processor_id();
@@ -189,11 +194,9 @@ int r4k_clockevent_init(void)
/*
* With vectored interrupts things are getting platform specific.
* get_c0_compare_int is a hook to allow a platform to return the
- * interrupt number of it's liking.
+ * interrupt number of its liking.
*/
- irq = MIPS_CPU_IRQ_BASE + cp0_compare_irq;
- if (get_c0_compare_int)
- irq = get_c0_compare_int();
+ irq = get_c0_compare_int();
cd = &per_cpu(mips_clockevent_device, cpu);
Weak header file declarations are error-prone because they make every
definition weak, and the linker chooses one based on link order (see
10629d711ed7 ("PCI: Remove __weak annotation from pcibios_get_phb_of_node
decl")).
get_c0_fdc_int() is defined only in arch/mips/mti-malta/malta-time.c so
there's no problem with multiple definitions. But it works better to have
a weak default implementation and allow a strong function to override it.
Then we don't have to test whether a definition is present, and if there
are ever multiple strong definitions, we get a link error instead of
calling a random definition.
Add a weak get_c0_fdc_int() definition with the default code and remove the
weak annotation from the declaration.
Signed-off-by: Bjorn Helgaas <[email protected]>
Reviewed-by: James Hogan <[email protected]>
---
arch/mips/include/asm/irq.h | 2 +-
drivers/tty/mips_ejtag_fdc.c | 9 ++++++---
2 files changed, 7 insertions(+), 4 deletions(-)
diff --git a/arch/mips/include/asm/irq.h b/arch/mips/include/asm/irq.h
index f0db99f8..15e0fec 100644
--- a/arch/mips/include/asm/irq.h
+++ b/arch/mips/include/asm/irq.h
@@ -49,7 +49,7 @@ extern int cp0_compare_irq_shift;
extern int cp0_perfcount_irq;
extern int cp0_fdc_irq;
-extern int __weak get_c0_fdc_int(void);
+extern int get_c0_fdc_int(void);
void arch_trigger_all_cpu_backtrace(bool);
#define arch_trigger_all_cpu_backtrace arch_trigger_all_cpu_backtrace
diff --git a/drivers/tty/mips_ejtag_fdc.c b/drivers/tty/mips_ejtag_fdc.c
index 358323c..a8c8cfd 100644
--- a/drivers/tty/mips_ejtag_fdc.c
+++ b/drivers/tty/mips_ejtag_fdc.c
@@ -879,6 +879,11 @@ static const struct tty_operations mips_ejtag_fdc_tty_ops = {
.chars_in_buffer = mips_ejtag_fdc_tty_chars_in_buffer,
};
+int __weak get_c0_fdc_int(void)
+{
+ return -1;
+}
+
static int mips_ejtag_fdc_tty_probe(struct mips_cdmm_device *dev)
{
int ret, nport;
@@ -967,9 +972,7 @@ static int mips_ejtag_fdc_tty_probe(struct mips_cdmm_device *dev)
wake_up_process(priv->thread);
/* Look for an FDC IRQ */
- priv->irq = -1;
- if (get_c0_fdc_int)
- priv->irq = get_c0_fdc_int();
+ priv->irq = get_c0_fdc_int();
/* Try requesting the IRQ */
if (priv->irq >= 0) {
Weak header file declarations are error-prone because they make every
definition weak, and the linker chooses one based on link order (see
10629d711ed7 ("PCI: Remove __weak annotation from pcibios_get_phb_of_node
decl")).
mips_cdmm_phys_base() is defined only in arch/mips/mti-malta/malta-memory.c
so there's no problem with multiple definitions. But it works better to
have a weak default implementation and allow a strong function to override
it. Then we don't have to test whether a definition is present, and if
there are ever multiple strong definitions, we get a link error instead of
calling a random definition.
Add a weak mips_cdmm_phys_base() definition and remove the weak annotation
from the declaration in arch/mips/include/asm/cdmm.h.
Signed-off-by: Bjorn Helgaas <[email protected]>
Reviewed-by: James Hogan <[email protected]>
---
arch/mips/include/asm/cdmm.h | 4 ++--
drivers/bus/mips_cdmm.c | 14 +++++++++++++-
2 files changed, 15 insertions(+), 3 deletions(-)
diff --git a/arch/mips/include/asm/cdmm.h b/arch/mips/include/asm/cdmm.h
index 16e22ce..bece206 100644
--- a/arch/mips/include/asm/cdmm.h
+++ b/arch/mips/include/asm/cdmm.h
@@ -53,7 +53,7 @@ struct mips_cdmm_driver {
* mips_cdmm_phys_base() - Choose a physical base address for CDMM region.
*
* Picking a suitable physical address at which to map the CDMM region is
- * platform specific, so this weak function can be defined by platform code to
+ * platform specific, so this function can be defined by platform code to
* pick a suitable value if none is configured by the bootloader.
*
* This address must be 32kB aligned, and the region occupies a maximum of 32kB
@@ -61,7 +61,7 @@ struct mips_cdmm_driver {
*
* Returns: Physical base address for CDMM region, or 0 on failure.
*/
-phys_addr_t __weak mips_cdmm_phys_base(void);
+phys_addr_t mips_cdmm_phys_base(void);
extern struct bus_type mips_cdmm_bustype;
void __iomem *mips_cdmm_early_probe(unsigned int dev_type);
diff --git a/drivers/bus/mips_cdmm.c b/drivers/bus/mips_cdmm.c
index ab3bde1..1c543ef 100644
--- a/drivers/bus/mips_cdmm.c
+++ b/drivers/bus/mips_cdmm.c
@@ -332,6 +332,18 @@ static phys_addr_t mips_cdmm_cur_base(void)
}
/**
+ * mips_cdmm_phys_base() - Choose a physical base address for CDMM region.
+ *
+ * Picking a suitable physical address at which to map the CDMM region is
+ * platform specific, so this weak function can be overridden by platform
+ * code to pick a suitable value if none is configured by the bootloader.
+ */
+phys_addr_t __weak mips_cdmm_phys_base(void)
+{
+ return 0;
+}
+
+/**
* mips_cdmm_setup() - Ensure the CDMM bus is initialised and usable.
* @bus: Pointer to bus information for current CPU.
* IS_ERR(bus) is checked, so no need for caller to check.
@@ -368,7 +380,7 @@ static int mips_cdmm_setup(struct mips_cdmm_bus *bus)
if (!bus->phys)
bus->phys = mips_cdmm_cur_base();
/* Otherwise, ask platform code for suggestions */
- if (!bus->phys && mips_cdmm_phys_base)
+ if (!bus->phys)
bus->phys = mips_cdmm_phys_base();
/* Otherwise, copy what other CPUs have done */
if (!bus->phys)
"__weak" is defined in include/linux/compiler-gcc.h. We shouldn't need an
arch-specific definition.
Remove the "__weak" definition from arch/mips/include/asm/linkage.h.
Signed-off-by: Bjorn Helgaas <[email protected]>
Reviewed-by: James Hogan <[email protected]>
---
arch/mips/include/asm/linkage.h | 1 -
1 file changed, 1 deletion(-)
diff --git a/arch/mips/include/asm/linkage.h b/arch/mips/include/asm/linkage.h
index 2767dda..99651b0 100644
--- a/arch/mips/include/asm/linkage.h
+++ b/arch/mips/include/asm/linkage.h
@@ -5,7 +5,6 @@
#include <asm/asm.h>
#endif
-#define __weak __attribute__((weak))
#define cond_syscall(x) asm(".weak\t" #x "\n" #x "\t=\tsys_ni_syscall")
#define SYSCALL_ALIAS(alias, name) \
asm ( #alias " = " #name "\n\t.globl " #alias)
Hi Bjorn,
On Tue, Jul 14, 2015 at 9:46 AM, Bjorn Helgaas <[email protected]> wrote:
> Weak header file declarations are error-prone because they make every
> definition weak, and the linker chooses one based on link order (see
> 10629d711ed7 ("PCI: Remove __weak annotation from pcibios_get_phb_of_node
> decl")).
>
> get_c0_perfcount_int() is defined in several files. Every definition is
> weak, so I assume Kconfig prevents two or more from being included. The
> callers contain identical default code used when get_c0_perfcount_int()
> isn't defined at all.
>
> Add a weak get_c0_perfcount_int() definition with the default code and
> remove the weak annotation from the declaration.
>
> Then the platform implementations will be strong and will override the weak
> default. If multiple platforms are ever configured in, we'll get a link
> error instead of calling a random platform's implementation.
>
> Signed-off-by: Bjorn Helgaas <[email protected]>
> CC: Andrew Bresticker <[email protected]>
Reviewed-by: Andrew Bresticker <[email protected]>