LinuxLists.cc - [PATCH 0/9] MIPS: Remove "weak" usage

[permalink] [raw]

Subject: [PATCH 1/9] MIPS: CPC: Remove "weak" from mips_cpc_phys_base() and make it static

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;

2015-07-12 23:11:11

[permalink] [raw]

Subject: [PATCH 2/9] MIPS: Remove "weak" from platform_maar_init() declaration

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]>
CC: [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

2015-07-12 23:11:20

[permalink] [raw]

Subject: [PATCH 3/9] MIPS: VPE: Exit vpe_release() early if vpe_run() isn't defined

vpe_run() is a weak symbol. If there's no definition of it, its value is
zero.

If vpe_run is zero, return failure early. We're going to fail anyway, so
there's no point in getting a VPE and attempting to load it.

Signed-off-by: Bjorn Helgaas <[email protected]>
---
arch/mips/kernel/vpe.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/arch/mips/kernel/vpe.c b/arch/mips/kernel/vpe.c
index 11da314..72cae9f 100644
--- a/arch/mips/kernel/vpe.c
+++ b/arch/mips/kernel/vpe.c
@@ -821,13 +821,18 @@ static int vpe_release(struct inode *inode, struct file *filp)
Elf_Ehdr *hdr;
int ret = 0;

+ if (!vpe_run) {
+ pr_warn("VPE loader: ELF load failed.\n");
+ return -ENOEXEC;
+ }
+
v = get_vpe(aprp_cpu_index());
if (v == NULL)
return -ENODEV;

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");

2015-07-12 23:11:29

[permalink] [raw]

Subject: [PATCH 4/9] MIPS: MT: Remove "weak" from vpe_run() declaration

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 never
more than one definition:

- 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_CMP=y

- CONFIG_MIPS_VPE_LOADER_MT cannot be set if CONFIG_MIPS_CMP=y

But it's simpler to verify correctness if we remove "weak" from the picture
and test the config symbols directly.

Remove "weak" from the vpe_run() declaration and use #if to test whether a
definition should be present.

Signed-off-by: Bjorn Helgaas <[email protected]>
---
arch/mips/include/asm/vpe.h | 2 +-
arch/mips/kernel/vpe.c | 10 +++++-----
2 files changed, 6 insertions(+), 6 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 72cae9f..04539d6 100644
--- a/arch/mips/kernel/vpe.c
+++ b/arch/mips/kernel/vpe.c
@@ -817,15 +817,11 @@ static int vpe_open(struct inode *inode, struct file *filp)

static int vpe_release(struct inode *inode, struct file *filp)
{
+#if defined(CONFIG_MIPS_VPE_LOADER_MT) || defined(CONFIG_MIPS_CMP)
struct vpe *v;
Elf_Ehdr *hdr;
int ret = 0;

- if (!vpe_run) {
- pr_warn("VPE loader: ELF load failed.\n");
- return -ENOEXEC;
- }
-
v = get_vpe(aprp_cpu_index());
if (v == NULL)
return -ENODEV;
@@ -855,6 +851,10 @@ static int vpe_release(struct inode *inode, struct file *filp)
v->plen = 0;

return ret;
+#else
+ pr_warn("VPE loader: ELF load failed.\n");
+ return -ENOEXEC;
+#endif
}

static ssize_t vpe_write(struct file *file, const char __user *buffer,

2015-07-12 23:11:36

[permalink] [raw]

Subject: [PATCH 5/9] MIPS: Remove "weak" from get_c0_perfcount_int() declaration

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 | 10 +++++++++-
arch/mips/oprofile/op_model_mipsxx.c | 8 +-------
4 files changed, 12 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..ec7082d 100644
--- a/arch/mips/kernel/time.c
+++ b/arch/mips/kernel/time.c
@@ -55,9 +55,17 @@ static int null_perf_irq(void)
}

int (*perf_irq)(void) = null_perf_irq;
-
EXPORT_SYMBOL(perf_irq);

+#ifdef MIPS_CPU_IRQ_BASE
+int __weak get_c0_perfcount_int(void)
+{
+ if (cp0_perfcount_irq >= 0)
+ return MIPS_CPU_IRQ_BASE + cp0_perfcount_irq;
+ return -1;
+}
+#endif
+
/*
* 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 |

2015-07-12 23:11:43

[permalink] [raw]

Subject: [PATCH 6/9] MIPS: Remove "weak" from get_c0_compare_int() declaration

2015-07-12 23:11:52

[permalink] [raw]

Subject: [PATCH 7/9] MIPS: Remove "weak" from get_c0_fdc_int() declaration

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]>
CC: 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) {

2015-07-12 23:12:01

[permalink] [raw]

Subject: [PATCH 8/9] MIPS: Remove "weak" from mips_cdmm_phys_base() declaration

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]>
CC: 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)

2015-07-12 23:12:15

[permalink] [raw]

Subject: [PATCH 9/9] MIPS: Remove "__weak" definition from arch-specific linkage.h

"__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]>
---
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)

2015-07-13 09:20:21

[permalink] [raw]

Subject: Re: [PATCH 8/9] MIPS: Remove "weak" from mips_cdmm_phys_base() declaration

Hi Bjorn,

On 13/07/15 00:11, Bjorn Helgaas 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")).
>
> 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]>
> CC: James Hogan <[email protected]>

Reviewed-by: James Hogan <[email protected]>

Thanks for pointing out these problems. Hopefully eventually some of
these uses of weak symbols can be moved to device tree (its almost like
we need a DT binding for "this physical address isn't used for anything,
and would be suitable for an overlay").

Cheers
James

> ---
> 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)
>

Attachments:

2015-07-13 09:31:55

[permalink] [raw]

Subject: Re: [PATCH 7/9] MIPS: Remove "weak" from get_c0_fdc_int() declaration

Attachments:

2015-07-13 09:36:22

[permalink] [raw]

Subject: Re: [PATCH 6/9] MIPS: Remove "weak" from get_c0_compare_int() declaration

Attachments:

2015-07-13 09:43:52

[permalink] [raw]

Subject: Re: [PATCH 5/9] MIPS: Remove "weak" from get_c0_perfcount_int() declaration

On 13/07/15 00:11, Bjorn Helgaas 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]>
> ---
> arch/mips/include/asm/time.h | 2 +-
> arch/mips/kernel/perf_event_mipsxx.c | 7 +------
> arch/mips/kernel/time.c | 10 +++++++++-
> arch/mips/oprofile/op_model_mipsxx.c | 8 +-------
> 4 files changed, 12 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..ec7082d 100644
> --- a/arch/mips/kernel/time.c
> +++ b/arch/mips/kernel/time.c
> @@ -55,9 +55,17 @@ static int null_perf_irq(void)
> }
>
> int (*perf_irq)(void) = null_perf_irq;
> -
> EXPORT_SYMBOL(perf_irq);
>
> +#ifdef MIPS_CPU_IRQ_BASE

why the ifdef? This would be the only such ifdef in the kernel, and
arch/mips/include/asm/mach-generic/irq.h seems to ensure it is always
defined anyway.

Aside from that the patch looks good.

Cheers
James

> +int __weak get_c0_perfcount_int(void)
> +{
> + if (cp0_perfcount_irq >= 0)
> + return MIPS_CPU_IRQ_BASE + cp0_perfcount_irq;
> + return -1;
> +}
> +#endif
> +
> /*
> * 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 |
>

Attachments:

2015-07-13 09:46:55

[permalink] [raw]

Subject: Re: [PATCH 2/9] MIPS: Remove "weak" from platform_maar_init() declaration

Attachments:

2015-07-13 10:08:11

[permalink] [raw]

Subject: Re: [PATCH 9/9] MIPS: Remove "__weak" definition from arch-specific linkage.h

On 13/07/15 00:12, Bjorn Helgaas wrote:
> "__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]>

asm/linkage.h is only included from linux/linkage.h, which includes
linux/compiler.h first, so no chance of build problems AFAICT, therefore:

Reviewed-by: James Hogan <[email protected]>

Thanks
James

> ---
> 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)
>

Attachments:

2015-07-13 10:27:54

[permalink] [raw]

Subject: Re: [PATCH 4/9] MIPS: MT: Remove "weak" from vpe_run() declaration

On 13/07/15 00:11, Bjorn Helgaas 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")).
>
> That's not a problem for vpe_run() because Kconfig ensures there's never
> more than one definition:
>
> - 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_CMP=y
>
> - CONFIG_MIPS_VPE_LOADER_MT cannot be set if CONFIG_MIPS_CMP=y
>
> But it's simpler to verify correctness if we remove "weak" from the picture
> and test the config symbols directly.
>
> Remove "weak" from the vpe_run() declaration and use #if to test whether a
> definition should be present.
>
> Signed-off-by: Bjorn Helgaas <[email protected]>
> ---
> arch/mips/include/asm/vpe.h | 2 +-
> arch/mips/kernel/vpe.c | 10 +++++-----
> 2 files changed, 6 insertions(+), 6 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 72cae9f..04539d6 100644
> --- a/arch/mips/kernel/vpe.c
> +++ b/arch/mips/kernel/vpe.c
> @@ -817,15 +817,11 @@ static int vpe_open(struct inode *inode, struct file *filp)
>
> static int vpe_release(struct inode *inode, struct file *filp)
> {
> +#if defined(CONFIG_MIPS_VPE_LOADER_MT) || defined(CONFIG_MIPS_CMP)

That should be CONFIG_MIPS_VPE_LOADER_CMP, in which case the error case
in the #else bit is always dead code. This file is built only if
CONFIG_MIPS_VPE_LOADER, and the other ones are defined without prompts:

config MIPS_VPE_LOADER_CMP
bool
default "y"
depends on MIPS_VPE_LOADER && MIPS_CMP

config MIPS_VPE_LOADER_MT
bool
default "y"
depends on MIPS_VPE_LOADER && !MIPS_CMP

I.e. one xor the other must be "y" when MIPS_VPE_LOADER=y.

Maybe its worth just removing the weak and the vpe_run check?

Cheers
James

> struct vpe *v;
> Elf_Ehdr *hdr;
> int ret = 0;
>
> - if (!vpe_run) {
> - pr_warn("VPE loader: ELF load failed.\n");
> - return -ENOEXEC;
> - }
> -
> v = get_vpe(aprp_cpu_index());
> if (v == NULL)
> return -ENODEV;
> @@ -855,6 +851,10 @@ static int vpe_release(struct inode *inode, struct file *filp)
> v->plen = 0;
>
> return ret;
> +#else
> + pr_warn("VPE loader: ELF load failed.\n");
> + return -ENOEXEC;
> +#endif
> }
>
> static ssize_t vpe_write(struct file *file, const char __user *buffer,
>

Attachments:

2015-07-13 21:13:56

[permalink] [raw]

Subject: Re: [PATCH 5/9] MIPS: Remove "weak" from get_c0_perfcount_int() declaration

On Mon, Jul 13, 2015 at 10:43:54AM +0100, James Hogan wrote:
> On 13/07/15 00:11, Bjorn Helgaas 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]>
> > ---
> > arch/mips/include/asm/time.h | 2 +-
> > arch/mips/kernel/perf_event_mipsxx.c | 7 +------
> > arch/mips/kernel/time.c | 10 +++++++++-
> > arch/mips/oprofile/op_model_mipsxx.c | 8 +-------
> > 4 files changed, 12 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..ec7082d 100644
> > --- a/arch/mips/kernel/time.c
> > +++ b/arch/mips/kernel/time.c
> > @@ -55,9 +55,17 @@ static int null_perf_irq(void)
> > }
> >
> > int (*perf_irq)(void) = null_perf_irq;
> > -
> > EXPORT_SYMBOL(perf_irq);
> >
> > +#ifdef MIPS_CPU_IRQ_BASE
>
> why the ifdef? This would be the only such ifdef in the kernel, and
> arch/mips/include/asm/mach-generic/irq.h seems to ensure it is always
> defined anyway.

I added the ifdef because I got a build error in
arch/mips/kernel/time.c. But maybe I can fix that by including
<asm/irq.h> instead; that would be much nicer.

> > +int __weak get_c0_perfcount_int(void)
> > +{
> > + if (cp0_perfcount_irq >= 0)
> > + return MIPS_CPU_IRQ_BASE + cp0_perfcount_irq;
> > + return -1;
> > +}
> > +#endif

2015-07-13 21:35:38